Skip to content

fix(identity): enforce tenant and credential isolation - #1001

Open
bokelley wants to merge 1 commit into
mainfrom
codex/security-identity-isolation
Open

fix(identity): enforce tenant and credential isolation#1001
bokelley wants to merge 1 commit into
mainfrom
codex/security-identity-isolation

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

  • enforce tenant/account ownership across registries, sessions, proposals, and reference-seller state
  • redact notification credentials from typed and generic response paths
  • reject ambiguous credential ownership and add a database uniqueness migration
  • require explicit roster authorization instead of silently allowing access

Why

The audit found cross-tenant lookup paths, response shapes that could expose credentials, and race-prone credential ownership without a database uniqueness guarantee.

Validation

  • 261 core identity tests passed (1 skipped)
  • 59 reference-seller and migration tests passed
  • independently reviewed and approved against current origin/main

Compatibility

Roster stores now require an authorization callback at construction. The migration preflights duplicate credential hashes and fails safely before creating the unique index.

monkeypatch: pytest.MonkeyPatch,
) -> None:
"""The request host cannot replace the tenant authenticated by the token."""
import src.app as app_module
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""An account id owned by another buyer is indistinguishable from missing."""
import src.platform as platform_module
async def test_account_store_upsert_cannot_overwrite_another_buyers_account(
monkeypatch: pytest.MonkeyPatch,
) -> None:
import src.platform as platform_module
import sqlalchemy as sa
from alembic import context, op

revision: str = "0003"
from alembic import context, op

revision: str = "0003"
down_revision: str | None = "0002"

revision: str = "0003"
down_revision: str | None = "0002"
branch_labels: str | Sequence[str] | None = None
revision: str = "0003"
down_revision: str | None = "0002"
branch_labels: str | Sequence[str] | None = None
depends_on: str | Sequence[str] | None = None

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid audit fix — the isolation work is correct and well-tested. One blocker, and it's the semver signal, not the code: a required-argument change to a public export is shipping under fix: without a breaking marker.

MUST FIX (blocking)

Breaking public-API change without the semver signal. create_roster_account_store is a public export (src/adcp/decisioning/__init__.py:455). This PR adds a required authorize argument (src/adcp/decisioning/roster_store.py:299), and your own test test_omitted_authorization_callback_fails_at_construction asserts the old call shape now raises TypeError. Failure mode in one sentence: any adopter calling create_roster_account_store(roster=...) — the documented signature until this PR — gets TypeError: missing 1 required keyword-only argument: 'authorize' after upgrading within what release-please will cut as a fix: patch.

The change itself is right — roster membership is not authorization, and failing closed at construction beats a silent fail-open. The block is purely the conventional-commit signal. release-please reads the commit/PR title (fix(identity): enforce tenant and credential isolation), not the PR body's Compatibility section. Retitle to fix(identity)!: or add a BREAKING CHANGE: footer naming the roster-store constructor change, so this cuts a major bump instead of a patch. The migration note in the PR body is already good — it just needs to be where release-please can see it.

Things I checked

  • code-reviewer: proposal_store composite-key migration ((account_id, proposal_id)) is consistent across every read/write/evict/index path including get_by_media_buy_id and _record_key's ambiguous-unscoped resolver. No bare-proposal_id lookup survives.
  • code-reviewer: registry_cache.py charges the aggregate bucket before the per-lookup bucket (_spend_locked), so rotating lookup keys can't bypass the tenant cap; prune-then-spend is behavior-preserving since a re-created bucket starts at full burst.
  • code-reviewer: bounded LRU client cache inserts-then-evicts (platform.py:621-624), never evicting the just-created client; _close_evicted_upstream_client handles the no-running-loop case.
  • security-reviewer: sound, no High. Credential redaction closes both the loose-dict and the Pydantic-model path in responses.py (_serialize now dumps then scrubs), confirmed by test_response_builder_scrubs_notification_credentials_from_pydantic_models. Idempotency replay cache dumps-then-scrubs at dispatch.py:1855.
  • security-reviewer: MCP session binding (server/auth.py) is enforced by a real ASGI integration test — a second valid bearer replaying alice's Mcp-Session-Id gets 404, alice's own bearer gets 200.
  • ad-tech-protocol-expert: sound-with-caveats. notification_config.json marks Authentication.credentials write-only (same class as BusinessEntity.bank), so stripping on the response path is spec-correct. schemes: min_length=1 max_length=1 matches the generated model exactly.
  • ad-tech-protocol-expert: cross-tenant sync_accountsACCOUNT_NOT_FOUND is the spec-correct existence-hiding code, aligning upsert with the pre-existing resolve behavior. Both codes are terminal, so conforming buyers' retry logic is unaffected — not a breaking wire change.
  • Migration 0003 preflights HAVING COUNT(*) > 1 and fails closed before touching indexes; creates the unique index before dropping the old one so a duplicate rolls back. Runtime LIMIT 2 + fetchall defense-in-depth agrees with the DB guarantee.
  • upstream.py: _project_status no longer takes body_text; the only other error path interpolates a JSONDecodeError position string, not the body. test_error_response_body_is_not_exposed covers it.

Follow-ups (non-blocking — file as issues)

  • Timing-oracle equalization for the new ACCOUNT_NOT_FOUND branch. ad-tech-protocol-expert flagged that cross-tenant rejection moved out of the PERMISSION_DENIED branch that _permission_denied_budget.py timing-clamps. The cross-tenant path does more work (resolve succeeds, then tenant compare fails) than the unknown-ref path (early None). Confirm the clamp covers the cross-tenant ACCOUNT_NOT_FOUND branch, or existence-hiding leaks through timing.
  • Raw bearer in scope['user']. server/auth.py sets AccessToken(token=bearer, ...) while the adjacent comment claims only derived context is stored. No round-trip today (the framework reads request.state, keeps credential=None), but a logging middleware serializing scope['user'] would surface the plaintext token. Pass a hash/opaque id since only the derived identity drives binding.
  • Unguarded mcp import on the authenticated path. The from mcp.server.auth... imports in server/auth.py run per-request outside any try/except; an mcp version drift turns every authenticated request into a 500. Import once at module load or guard it.
  • Discovery + stale bearer now 401s. The and not bearer narrowing means a client attaching an expired token to get_adcp_capabilities/initialize now gets 401 instead of the pre-auth discovery response. More correct, but a behavioral change worth a release note for buyers that token every request.
  • Reference-seller order ownership keyed on advertiser_id, creatives on _account_scope. Two different isolation identities in examples/.../platform.py. Safe only if (network_code, advertiser_id) is 1:1 with an account under the seeding contract — worth a one-line justification in the file.

Minor nits (non-blocking)

  1. Stale rationale comment. account_projection.py:457-467 still says strip_credentials_from_wire_result passes Pydantic models through because "response-side codegen shapes don't define authentication/bank." The base model now structurally carries notification_configs[i].authentication.credentials — that's why projections.py needed _NotificationConfigResponse. The load-bearing safety is the handler-layer model_dump, not schema structure; point the comment there.
  2. SchemaVariant on list drops max_length=16. Per ad-tech-protocol-expert, the marker is correctly required here (invariant list[...] would otherwise be a Liskov violation), but the runtime collapse silently drops the base field's length constraint on the response projection. Harmless on a read edge — worth a one-line comment.

Fix the commit prefix and this is a clean ship. Everything else is a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant