Skip to content

test(signing): multi-tenant operating model + cross-tenant isolation tests - #4054

Closed
zmanian wants to merge 1 commit into
attested-signing-14-loop-raisefrom
attested-signing-multitenant-model
Closed

zmanian wants to merge 1 commit into
attested-signing-14-loop-raisefrom
attested-signing-multitenant-model

Conversation

@zmanian

@zmanian zmanian commented May 25, 2026 •

Copy link
Copy Markdown
Collaborator

Adds the multi-tenant operating-model plan-doc section (already committed) plus cross-tenant isolation tests that lock the grant / key-AAD / gate-resolve / ledger isolation guarantees against silent regression. The isolation already exists in code; this PR makes the tenant dimension explicit and verified at every surface. Test-only additions plus the committed doc — no production behavior change.

Surfaces locked

  • Sealed grant — cross_tenant_claim_is_not_found added to the sealed_grant_store_contract_cases! suite, so it runs for the in-memory reference impl and both durable backends (libSQL local + PG env-gated). A grant sealed for tenant A is NotFound when claimed with a GrantKey identical except tenant = B; tenant A's grant stays claimable.
  • Custodial key AAD (highest value) — chain_key_aad_differs_per_tenant and chain_key_ciphertext_fails_under_wrong_tenant_aad at the crypto level, plus consume_under_other_tenant_is_tenant_isolated and cross_tenant_ciphertext_injection_fails_closed at the keystore level. The injection test models an attacker with raw store access copying tenant A's ciphertext into tenant B's slot and proves consume fails closed on the AES-GCM tag, never exposing key bytes.
  • Gate resolve — gate_resolve_is_tenant_isolated_cross_tenant_grant_fails_closed drives the real custodial continuation driver: a gate bound to tenant A with only a tenant-B grant sealed fails closed (ChainSigning(Grant::NotFound)), no broadcast, tenant-B grant left unclaimed.
  • Ledger — distinct_gates_advance_independently added to the signing_ledger_contract_cases! suite (in-memory + durable), proving two tenants' gate flows advance with no state bleed.

Verification

  • cargo fmt --all, cargo clippy --all --tests --examples --all-features -- -D warnings clean.
  • New tests green for in-memory, libSQL (local temp-file), and the PG path (compiles + env-gated skip). Concurrent CAS case run repeatedly for determinism.
  • cargo tree -i openssl-sys empty on host and x86_64-unknown-linux-gnu.

No real isolation gap was found — every surface already fails closed; these tests pin that behavior. The remaining operational gaps (production runtime wiring, per-tenant config, tenant lifecycle) are tracked separately, not in scope here.

🤖 Generated with Claude Code


Cascade port onto current main (2026-07-24)

Replayed onto the ported attested-signing cascade; base is the ported attested-signing-14-loop-raise. The branch forks off the -14 line and touches crates the PR14 GatedResolution re-expression never moved, so all three commits cherry-picked clean. Two cases needed re-expression against tenant-scoping this cascade introduced in the -12 (durable stores) hop, which this branch predates:

  • distinct_gates_advance_independently is now the strong form. It asserted isolation using two DISTINCT gate_refs, on the stated premise that "the ledger is keyed purely by gate_ref (it carries no tenant component)". That premise is no longer true — the ledger is keyed (tenant, gate_ref) — and the weak form would pass even on a ledger that ignored the tenant entirely. Re-expressed so the SAME gate_ref under two tenants must still be two independent rows, which only the tenant component can deliver.
  • The two cross-tenant threat-matrix assertions read the ledger by bare gate_ref; they now build the composite LedgerKey for the binding's tenant, and tolerate a missing row (strictly stronger than a non-broadcast row).

claim_expired_grant_is_rejected lands here as the #[ignore]d intent stub it was written as — no backend enforced expiry at this point in the stack. #4104 (downstack) implements enforcement and replaces this stub with a running contract case, closing the H2 follow-up it tracks.

Verification: 244 tests pass across attestation / attested_runtime / attested_store / chain_signing / secrets; clippy + fmt clean.

@github-actions github-actions Bot added scope: docs Documentation size: L 200-499 changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels May 25, 2026

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces comprehensive testing and documentation for the multi-tenant isolation model across the attestation, ledger, keystore, and cryptographic components. It adds a detailed "Multi-Tenant Operating Model" section to the documentation and implements various cross-tenant isolation tests to ensure that grants, keys, and ledger states remain strictly separated between tenants. Review feedback suggests renaming variables in tests to better distinguish between grant keys and tenant IDs, and highlights a discrepancy between the code comments and documentation regarding whether the gate_ref component inherently carries tenant information.

Comment on lines +308 to +329
let tenant_a = key(8); // tenant = "tenant-a" (see `key`)
store
.seal(AttestedSigningGrant::seal(tenant_a.clone(), 0, None))
.await
.expect("seal tenant-a grant");

// Same run/user/gate/hash/key/chain, different tenant.
let mut tenant_b = tenant_a.clone();
tenant_b.tenant = TenantId::new("tenant-b");
assert_ne!(tenant_b.tenant, tenant_a.tenant);
assert_eq!(
store.claim(&tenant_b).await,
Err(GrantError::NotFound),
"a claim carrying a different tenant must not match tenant-a's grant"
);

// Tenant A's grant is untouched and still wins its one-shot claim.
let claimed = store
.claim(&tenant_a)
.await
.expect("original tenant-a grant must still be claimable");
assert_eq!(claimed.key.tenant, tenant_a.tenant);

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.

medium

The variable names tenant_a and tenant_b are misleading as they hold GrantKey instances, not TenantIds. This makes the subsequent logic, such as tenant_b.tenant = ..., harder to follow. Renaming these to key_a and key_b (or grant_key_a/b) would improve readability and clarify that the test is comparing two full grant keys that differ only in their tenant component.

        let key_a = key(8); // tenant = "tenant-a" (see key)
        store
            .seal(AttestedSigningGrant::seal(key_a.clone(), 0, None))
            .await
            .expect("seal tenant-a grant");

        // Same run/user/gate/hash/key/chain, different tenant.
        let mut key_b = key_a.clone();
        key_b.tenant = TenantId::new("tenant-b");
        assert_ne!(key_b.tenant, key_a.tenant);
        assert_eq!(
            store.claim(&key_b).await,
            Err(GrantError::NotFound),
            "a claim carrying a different tenant must not match tenant-a's grant"
        );

        // Tenant A's grant is untouched and still wins its one-shot claim.
        let claimed = store
            .claim(&key_a)
            .await
            .expect("original tenant-a grant must still be claimable");
        assert_eq!(claimed.key.tenant, key_a.tenant);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Keeping the tenant_a/tenant_b names. This is specifically a cross-tenant isolation test: the variables stand in for two tenants' grant keys that differ only in their tenant field, and the inline comment (tenant = "tenant-a") makes the binding explicit. Naming them key_a/key_b would obscure the one dimension the test is actually exercising.

Comment on lines +294 to +296
/// between them. The ledger is keyed purely by `gate_ref` (it carries no
/// tenant component), so tenant isolation here is inherited from the fact
/// that two tenants' flows always carry distinct `gate_ref`s. This case

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.

medium

There is a discrepancy between this implementation comment and the multi-tenant operating model described in the plan documentation. The documentation (line 74 of the plan doc) states that gate_ref carries the tenant, while this comment explicitly says it does not.

If isolation relies on gate_ref strings being distinct by convention (e.g., prefixed with the tenant ID), this should be explicitly documented as a requirement for callers of the SigningLedger trait, especially since the InMemorySigningLedger reference implementation does not enforce tenant isolation via its internal HashMap key structure.

References
  1. Documentation for complex logic, such as multi-tenant isolation policies, must precisely match the implementation to avoid misleading developers about the security model.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The ledger.rs comment is accurate as written — gate_ref does carry no tenant component, and the InMemorySigningLedger keys purely by gate_ref. The discrepancy was in the plan doc, which incorrectly claimed the ledger/binding stores are tenant-columned. Fixed the doc in d89482a to state the real invariant: gate_ref must be globally unique (tenant-qualified by the gate-raising path), so isolation is inherited rather than enforced by a tenant column.

|---------|----------------------------------|
| **One-shot grant** | The sealed-grant `key_hash` (PRIMARY KEY of the durable store) is `grant_key_hash(GrantKey)` over the full 7-tuple **including `tenant`**; the durable row also carries an explicit `tenant` column. The atomic CAS claim is therefore tenant-scoped — tenant A's gate yields a different `key_hash` than B's, so one tenant can never claim or replay another's grant. |
| **Custodial keys** | Encrypted with `chain_key_aad(scope, chain)` = AES-256-GCM AAD over `tenant_id ∥ user_id ∥ agent_id ∥ project_id ∥ chain` (owner-scope only — independent of invocation/mission/thread). A key sealed for tenant A **cannot decrypt** under tenant B's AAD (authentication-tag failure). The master key is an **operator/instance-level** secret; per-(tenant,user,…) custodial keys are stored ciphertexts under it. |
| **Ledger / challenge / binding stores** | Keyed by `gate_ref` / `GrantKey`, which carry the tenant; durable stores are tenant-columned and one-shot per `gate_ref`. |

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.

medium

The statement that gate_ref carries the tenant appears to conflict with the implementation notes in crates/ironclaw_attestation/src/ledger.rs, which state that gate_ref carries no tenant component. Additionally, if durable stores are tenant-columned, the primary key is likely (tenant, gate_ref), making the operation one-shot per tenant-gate pair rather than just per gate_ref.

Consider refining this description to clarify whether gate_ref is expected to be a tenant-prefixed string or if the isolation is handled by a separate tenant column that is not currently reflected in the SigningLedger trait API.

References
  1. Documentation for complex logic, such as multi-tenant isolation policies, must precisely match the implementation to avoid misleading developers about the security model.
  2. Ensure documentation precisely reflects the current implementation of traits and methods; remove or explicitly mark as 'planned' any references to features that were renamed or not implemented during development.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in d89482a: corrected the line-74 row. The durable attested_signing_ledger and attested_gate_bindings schemas are keyed purely by gate_ref (PRIMARY KEY) and the SigningLedger/binding APIs carry no tenant component, so the previous 'tenant-columned / gate_ref carries the tenant' claim was wrong. The doc now states isolation here is inherited from globally-unique tenant-qualified gate_ref plus the tenant-scoped grant key_hash and bound-account check at resolve.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a6d504c140

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

|---------|----------------------------------|
| **One-shot grant** | The sealed-grant `key_hash` (PRIMARY KEY of the durable store) is `grant_key_hash(GrantKey)` over the full 7-tuple **including `tenant`**; the durable row also carries an explicit `tenant` column. The atomic CAS claim is therefore tenant-scoped — tenant A's gate yields a different `key_hash` than B's, so one tenant can never claim or replay another's grant. |
| **Custodial keys** | Encrypted with `chain_key_aad(scope, chain)` = AES-256-GCM AAD over `tenant_id ∥ user_id ∥ agent_id ∥ project_id ∥ chain` (owner-scope only — independent of invocation/mission/thread). A key sealed for tenant A **cannot decrypt** under tenant B's AAD (authentication-tag failure). The master key is an **operator/instance-level** secret; per-(tenant,user,…) custodial keys are stored ciphertexts under it. |
| **Ledger / challenge / binding stores** | Keyed by `gate_ref` / `GrantKey`, which carry the tenant; durable stores are tenant-columned and one-shot per `gate_ref`. |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Correct tenant-isolation claim for gate-scoped stores

This row overstates current isolation semantics: gate_ref does not intrinsically carry tenant information, and the durable ledger/binding schemas are not tenant-columned (attested_signing_ledger and attested_gate_bindings are keyed only by gate_ref in crates/ironclaw_attested_store/src/ledger.rs:27-32 and crates/ironclaw_attested_store/src/binding.rs:34-37). Leaving this as-is can mislead reviewers/operators into assuming database-level tenant partitioning that is not actually enforced; the doc should either describe the real invariant (globally unique/tenant-qualified gate_ref) or remove the tenant-column statement.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in d89482a. Confirmed against crates/ironclaw_attested_store/src/ledger.rs and binding.rs — both schemas are keyed only by gate_ref with no tenant column. Rewrote the line-74 row to describe the real invariant (globally unique / tenant-qualified gate_ref; isolation inherited, not database-partitioned) and removed the tenant-column statement.

zmanian added a commit that referenced this pull request May 25, 2026
Correct the isolation-backbone table's claim that the ledger/binding
durable stores are tenant-columned. The attested_signing_ledger and
attested_gate_bindings schemas are keyed purely by gate_ref and the
SigningLedger/binding trait APIs carry no tenant component; isolation
there is inherited from globally-unique (tenant-qualified) gate_ref plus
the tenant-scoped grant key_hash and bound-account check at resolve.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@github-actions github-actions Bot added the scope: dependencies Dependency updates label May 26, 2026
@zmanian
zmanian force-pushed the attested-signing-14-loop-raise branch from 0d4442b to 3c0a01b Compare May 26, 2026 14:11
@zmanian
zmanian force-pushed the attested-signing-multitenant-model branch from 35fa7fd to 0987fd4 Compare May 26, 2026 14:20

@henrypark133 henrypark133 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Paranoid-Architect Review — PR #4054 multi-tenant operating model

Summary: This PR is test-only + doc-only. No production behaviour is changed. The cross-tenant isolation guarantees it verifies are real and the tests are well-constructed. Several gaps remain that must be addressed before this PR should merge.


CRITICAL

None.


HIGH

H1 — gate_ref global-uniqueness is an unverified caller contract with no enforcement layer

attested_gate_bindings and attested_signing_ledger are both keyed only by gate_ref with no tenant column (confirmed in crates/ironclaw_attested_store/src/binding.rs:41 and src/ledger.rs). The doc (line 74) now correctly says isolation is "inherited from globally-unique / tenant-qualified gate_ref," but nothing in the code — neither the GateRef newtype (transparent string, no validation), the AttestedGateBindingStore::put path, nor the SigningLedger::create path — enforces or even asserts that the string carries a tenant prefix. If two tenants' gate-raising paths independently produce the same gate_ref string (clock-based, sequential counter, etc.), the second put silently drops to ON CONFLICT DO NOTHING, the first tenant's binding wins, and the second tenant's continue_after_resolved verifies against a binding it did not create. This is a cross-tenant IDOR at the binding/ledger surface. The tests in this PR do NOT cover this vector; they only show that distinct gate_ref strings advance independently.

Required: either add a tenant column to both durable schemas and thread it through the SigningLedger and AttestedGateBindingStore traits (clean fix, aligns with PR3 grant-ledger CHANGES_REQUESTED), or enforce at GateRef::new that the string includes a validated tenant prefix and add a contract test that a binding stored with gate_ref "gate:tenant-a:X" is NOT retrievable by a different tenant's resolve path.

H2 — Expiry field present but never enforced on claim — no test locks the gap

AttestedSigningGrant.expiry_ms is carried in both the in-memory and durable stores but the claim path in every backend ignores it (confirmed crates/ironclaw_attestation/src/grant.rs in-memory claim, crates/ironclaw_attested_store/src/grant.rs durable UPDATE). The doc says "enforcement noted for later PRs." A stale grant with a past expiry_ms can still be claimed indefinitely. This PR adds cross-tenant tests but no expiry-enforcement test, so the gap is invisible to the contract suite. At minimum, add a claim_expired_grant_is_rejected stub or a #[ignore] test with a comment pointing to the tracking issue to the sealed_grant_store_contract_cases! macro, so every future backend must pass it once enforcement lands.


MEDIUM

M1 — scope in AttestedGateBinding is hardcoded to owner_scope() (tenant "default") in the cross-tenant driver test

In crates/ironclaw_attested_runtime/tests/threat_matrix.rs, put_binding_res always sets scope: owner_scope() where owner_scope() is hardcoded to TenantId::new("default"). The new test gate_resolve_is_tenant_isolated_cross_tenant_grant_fails_closed uses ctx_a (tenant "default") so the keystore lookup succeeds because both the binding's scope and the keystore's bound scope share the same tenant. The test correctly catches grant-CAS isolation (GrantError::NotFound), but it does not verify that if a tenant-B user's scope is used for keystore lookup, consume would also fail closed. The two isolation layers should each be independently tested.

M2 — distinct_gates_advance_independently does not include a concurrent cross-gate collision test

The ledger isolation test drives gates sequentially. A concurrent test (two contexts racing create with the same gate_ref string) would surface the H1 gap in CI rather than only in production.

M3 — No contract test for the binding store's cross-tenant invariant

AttestedGateBindingStore has no cross-tenant contract case. A cross_tenant_binding_isolation case analogous to cross_tenant_claim_is_not_found should be added to the binding store contract suite so future durable backends cannot pass without demonstrating tenant isolation.

M4 — Doc overstates certainty on gate_ref uniqueness enforcement

docs/plans/2026-05-23-attested-signing-substrate.md line 74 says "gate_ref is required to be globally unique (tenant-qualified by the gate-raising path)." This describes a caller obligation with zero enforcement at any layer. Change to: "gate_ref MUST be globally unique and tenant-qualified; this is a caller obligation not enforced by the store or newtype" and reference the tracking issue for the tenant column or prefix validation.


LOW

L1 — cross_tenant_ciphertext_injection_fails_closed accesses store.keys directly

crates/ironclaw_chain_signing/src/keystore.rs test reaches into store.keys.lock().unwrap(). Test-only and correct, but a #[cfg(test)] fn inject_for_test(...) method would be future-proof against internal representation changes.

L2 — Shared GATE constant in other_tenant_signing_context is load-bearing but undocumented

Both signing_context and other_tenant_signing_context use SigningGateRef::new(GATE). The shared gate_ref is the whole point of the cross-tenant test (same gate, different tenant), but this is not commented. A future reader may change it to a distinct value, silently neutering the test.


NITS

  • crates/ironclaw_attestation/src/grant.rs new test: let tenant_a = key(8) names a GrantKey as a tenant. The key() factory doc should note it always sets tenant = "tenant-a" so the naming convention is explicit.
  • crates/ironclaw_secrets/src/crypto.rs test-local scope() helper has 3 args vs the outer 2-arg version — rename to avoid shadowing confusion.

Summary table

ID Severity Surface Issue
H1 High attested_gate_bindings, attested_signing_ledger gate_ref uniqueness unenforced; cross-tenant IDOR possible if strings collide
H2 High SealedGrantStore::claim (all backends) expiry_ms not enforced; no contract test tracking the gap
M1 Medium threat_matrix.rs Cross-tenant driver test only covers grant-CAS, not keystore-scope isolation
M2 Medium ledger.rs contract suite No concurrent same-gate_ref collision test
M3 Medium AttestedGateBindingStore No cross-tenant contract case
M4 Medium Plan doc line 74 Caller obligation stated as code invariant
L1 Low keystore.rs tests Direct field access in injection test
L2 Low threat_matrix.rs Shared GATE constant undocumented as load-bearing

Verdict: CHANGES_REQUESTED. H1 and H2 must be resolved before merge. M1–M3 should be addressed in the same PR or explicitly tracked with issues.

zmanian added a commit that referenced this pull request May 27, 2026
…iry contract gaps

Test+doc-only follow-ups to the multi-tenant operating-model review. No
production behaviour changes.

- M4/H1 doc: rewrite plan-doc line 74 so gate_ref global-uniqueness /
  tenant-qualification is stated as a CALLER OBLIGATION explicitly NOT
  enforced by the store or the GateRef/SigningGateRef newtype, and note the
  tracked tenant-column / prefix-validation follow-up. The fail-closed
  account+grant-key_hash lookup that still blocks a cross-tenant signature
  even on a collided row is called out separately.
- H2: add a #[ignore]d `claim_expired_grant_is_rejected` case to the
  sealed_grant_store_contract_cases! macro pinning the intended
  fail-closed-on-expiry contract every backend must satisfy once expiry
  enforcement lands (currently unenforced).
- M2: add `concurrent_create_same_gate_ref_yields_one_winner` to the
  signing_ledger_contract_cases! suite — the concurrent face of the one-shot
  create that surfaces a colliding-gate_ref race in CI (exactly one Ok, rest
  AlreadyExists). Documents that this proves create-CAS atomicity, not
  cross-tenant isolation.
- M1: add `gate_resolve_cross_tenant_keystore_scope_fails_closed` driving the
  REAL custodial continuation where the grant matches (tenant A) but the
  binding's authoritative keystore scope is tenant B — the keystore lookup
  fails closed before the grant is touched, proving keystore-scope isolation
  independently of grant-CAS isolation.
- M3: add `binding_store_is_tenant_isolated_by_gate_ref` — the binding-store
  analogue of cross_tenant_claim_is_not_found: tenant-qualified gate_refs
  resolve only their own tenant's binding (async + sync read paths), and a
  binding whose context.gate_ref != store key is rejected (GateRefMismatch).
- L1: replace the direct `store.keys.lock()` reach-in in the keystore
  cross-tenant ciphertext-injection test with a #[cfg(test)]
  `inject_stored_for_test` helper that keeps StoredKey/KeyStoreKey private.
- L2: document the shared GATE constant in threat_matrix.rs as load-bearing
  for the cross-tenant cases (same gate, different tenant).
- NITs: document that grant `key()` always uses tenant "tenant-a"; rename the
  crypto test-local `scope()` helper to `account_scope()` to avoid confusion
  with the keystore 2-arg scope helpers.

Preserves every cross-tenant isolation property under test; openssl-free; no
developer-local absolute paths in committed docs.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@zmanian

zmanian commented May 27, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the latest paranoid-architect CHANGES_REQUESTED review (tip d027746e7). This PR is test+doc-only, so the genuine production-enforcement asks (tenant column / GateRef prefix validation, expiry enforcement) are tracked as follow-ups rather than landed here; the test-suite and doc gaps the review called out are all closed.

H1 — gate_ref uniqueness unenforced (doc / contract): Declined the schema change in this PR (adding a tenant column threaded through the SigningLedger/AttestedGateBindingStore traits + durable backends is a production, cross-crate change out of scope for a test+doc PR and is the PR3 grant-ledger follow-up). Instead: (a) M4 rewrote plan-doc line 74 to state global-uniqueness/tenant-qualification as a caller obligation explicitly NOT enforced by the store or the GateRef/SigningGateRef newtype, and to reference the tracked tenant-column/prefix-validation follow-up; (b) M2 added concurrent_create_same_gate_ref_yields_one_winner to the ledger contract suite so a colliding-gate_ref race surfaces in CI (exactly one Ok, rest AlreadyExists) — with a comment that this proves create-CAS atomicity, not cross-tenant isolation; (c) M3 added binding_store_is_tenant_isolated_by_gate_ref (see below). The collided-row residual risk is now documented, and the fail-closed account+grant-key_hash lookup that still blocks a cross-tenant signature even on a shared row is called out separately.

H2 — expiry never enforced on claim: Added a #[ignore]d claim_expired_grant_is_rejected case to sealed_grant_store_contract_cases!. It seals a grant already expired at seal time and asserts the claim fails closed. It is ignored (and intentionally fails today under --include-ignored) because no backend enforces expiry yet — it pins the intended contract so every future backend must satisfy it once enforcement lands. Comment references the H2 follow-up.

M1 — keystore-scope isolation untested: Added gate_resolve_cross_tenant_keystore_scope_fails_closed, driving the REAL custodial continuation where the grant matches (tenant A context) but the bindings authoritative custodial scopeis tenant B while the key is bound only under tenant A. The signer looks the key up bybinding.scope BEFORE claiming the grant, so the lookup fails closed (ChainSigningError::KeyStore`) and the grant is never touched — proving keystore-scope isolation independently of grant-CAS isolation.

M2 — concurrent same-gate_ref collision: Done (see H1 above).

M3 — binding-store cross-tenant contract case: Added binding_store_is_tenant_isolated_by_gate_ref, the binding-store analogue of cross_tenant_claim_is_not_found: tenant-qualified gate_refs resolve only their own tenants binding (both async getand syncget_sync), and a binding whose context.gate_ref != store key is rejected (GateRefMismatch), so a tenant cannot smuggle its binding under another tenants key. (A full exported binding-store contract macro wired through the durable backends is a larger structural change deferred with the H1 tenant-column work; this direct case locks the invariant now.)

L1 — direct field access in injection test: Replaced the store.keys.lock() reach-in with a #[cfg(test)] inject_stored_for_test(from, to, chain) helper that keeps StoredKey/KeyStoreKey private and is robust to internal-layout changes.

L2 — undocumented load-bearing GATE constant: Added a comment in threat_matrix.rs marking the shared GATE as load-bearing for the cross-tenant cases (same gate, different tenant) with a do-not-change warning.

NITs: Documented that grant key() always uses tenant "tenant-a" (so let tenant_a = key(8) names the constant tenant, not the seed); renamed the crypto test-local scope() helper to account_scope() to avoid confusion with the keystore 2-arg scope/scope_t helpers.

Verification (IRONCLAW_DISABLE_OS_KEYCHAIN=1): cargo test --all-features on ironclaw_attestation, ironclaw_attested_runtime, ironclaw_chain_signing, ironclaw_secrets passes (all cross-tenant isolation cases green; H2 case correctly ignored); cargo fmt and cargo clippy --all-features --tests clean. Preserved every cross-tenant isolation property under test, the MT operating-model doc section, openssl-free, and no developer-local absolute paths in committed docs.

@abbyshekit

Copy link
Copy Markdown
Contributor

Review — multi-tenant operating model + cross-tenant isolation tests

Combined pass (Claude Code + Codex 5.5 gpt-5.5). Both passes converge: approve. Codex found no discrete issues; my pass confirmed the tests exercise the real enforcement paths (durable grant-CAS, AES-GCM AAD, keystore scope-keying, binding-store gate_ref), cover the dangerous direction (a tenant A flow holding only tenant B's grant/key/binding → fail-closed), and assert on real NotFound/Decryption/KeyStore outcomes — no tautologies, no over-mocking, no production behavior change. Reviewed against this PR's own base (attested-signing-14-loop-raise).

Low

crates/ironclaw_attested_runtime/tests/threat_matrix.rs:937 — the gate_resolve_cross_tenant_keystore_scope_fails_closed docstring says the signer "looks the key up by binding.scope … before it claims the grant"; the real fail-closed point is the public keystore.binding() read at custodial.rs:178 (→ NotFound → KeyStore error), which precedes the grant claim at custodial.rs:197 (and the consume at 228 it describes is later still). The assertion is correct and order-accurate; only the wording conflates the binding-read with consume. Reword to "reads the public binding under binding.scope."

What's good

The high-value tests are real, not mocked: chain_key_ciphertext_fails_under_wrong_tenant_aad and cross_tenant_ciphertext_injection_fails_closed move tenant A's ciphertext into tenant B's slot (via #[cfg(test)] inject_stored_for_test) and prove the AES-GCM tag fails — no key bytes returned; cross_tenant_claim_is_not_found runs across in-memory + libSQL + Postgres (env-gated); every test asserts the victim row stays claimable/unbroadcast. claim_expired_grant_is_rejected is an honest #[ignore] red-flag stub (confirmed no claim path consults expiry_ms yet). No unwrap/expect/panic in production code (only tests).

Coverage gaps (all acknowledged in-PR as out of scope)

Expiry enforcement is untested-live (the ignored test); cross-tenant gate_ref uniqueness isn't enforced (isolation is inherited from callers minting distinct gate_refs; concurrent_create_same_gate_ref_yields_one_winner proves create-CAS atomicity, not tenant isolation); no test of two different keys under the same scope but different tenants both succeeding independently.

Both reviewers converged on approve; the threat-matrix tests genuinely exercise enforcement (not tautological). Reviewed against this PR's own base.

@zmanian

zmanian commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator Author

Both passes converging on approve matches the intent of this PR: lock the cross-tenant isolation invariants under test without any production behavior change. Point-by-point on the residual notes:

Low — threat_matrix.rs:937 docstring wording. Correct. The fail-closed read is the public keystore.binding(&req.scope, &req.chain) at custodial.rs:178 (KeyStore error), which precedes the grant claim at custodial.rs:197; the consume path is the later hot-key step and isn't what trips here. The assertion and ordering are accurate — only the prose conflates the binding-read with consume. Will fix: reword the docstring to "reads the public binding under binding.scope BEFORE it claims the grant" so it names the actual enforcement read.

Coverage gaps (expiry-live, gate_ref uniqueness, same-scope/different-tenant dual-success). Agree these are gaps and they're intentionally out of scope here. Expiry-live is pinned by the #[ignore]d claim_expired_grant_is_rejected stub against the H2 follow-up; gate_ref uniqueness is the H1 tenant-column work (a cross-crate trait/backend change deferred to the PR3 grant-ledger follow-up — concurrent_create_same_gate_ref_yields_one_winner only claims create-CAS atomicity, as its comment states). The same-scope/different-tenant dual-success case is a reasonable add; I'll fold it into the same follow-up that lands tenant-column qualification rather than this test+doc PR.

zmanian added a commit that referenced this pull request Jun 4, 2026
…4054)

The gate_resolve_cross_tenant_keystore_scope_fails_closed docstring
said the signer 'looks the key up by binding.scope BEFORE it claims
the grant', conflating the public binding read with key lookup/consume.
The actual fail-closed point is the public keystore.binding(&req.scope,
&req.chain) read at custodial.rs:178 (KeyStore error), which precedes
the grant claim at custodial.rs:197. Reworded the docstring to name
that read. Doc-only; the assertion and ordering were already accurate.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@zmanian

zmanian commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed in 9100199 (doc-only). Reworded the gate_resolve_cross_tenant_keystore_scope_fails_closed docstring at threat_matrix.rs so it names the actual fail-closed read: the public keystore.binding(&req.scope, &req.chain) at custodial.rs:178 (KeyStore error), which precedes the grant claim at custodial.rs:197 — rather than conflating it with the later key lookup/consume. The assertion and ordering were already accurate; only the prose changed. The coverage gaps (expiry-live, gate_ref uniqueness, same-scope/different-tenant dual-success) remain intentionally out of scope here and tracked against the H1/H2 grant-ledger follow-ups as noted. Verified: cargo test -p ironclaw_attested_runtime --test threat_matrix gate_resolve_cross_tenant_keystore_scope_fails_closed passes; fmt and panic-check clean.

…4051/#4054)

Folds the `attested-signing-multitenant-model` side branch (3 commits: #4051
model + tests, the #4054 review round, and its docstring fix) onto the ported
-14 hop. It forked off the -14 line and touches crates the GatedResolution
re-expression never moved, so it applies clean.

Adds cross-tenant isolation coverage at every layer the substrate keys by
tenant: sealed grants (`GrantKey.tenant` is part of the composite identity and
of `grant_key_hash`), the signing ledger, the custodial keystore scope, and the
secrets AAD — plus the end-to-end threat-matrix cases driving the REAL custodial
continuation with a foreign-tenant grant / foreign-tenant keystore scope.

Two cases needed re-expression against the tenant-scoping this cascade added in
the -12 hop (the branch predates it):

- `distinct_gates_advance_independently` asserted isolation via two DISTINCT
  `gate_ref`s, on the premise that "the ledger is keyed purely by `gate_ref`
  (it carries no tenant component)". That premise is no longer true — the
  ledger is keyed `(tenant, gate_ref)` — and the weak form would pass even on a
  ledger that ignored the tenant entirely. Re-expressed as the strong form: the
  SAME `gate_ref` under two tenants must still be two independent rows, so only
  the tenant component can be doing the isolating.
- the two cross-tenant threat-matrix assertions read the ledger by bare
  `gate_ref`; they now build the composite `LedgerKey` for the binding's tenant,
  and tolerate a missing row (strictly stronger than a non-broadcast row).

`claim_expired_grant_is_rejected` stays `#[ignore]`d: it pins the intended
fail-closed contract for an expired grant, which no backend enforces yet. The
#4102 fold lands that enforcement and should drop the ignore.

244 tests pass across attestation/attested_runtime/attested_store/chain_signing/
secrets (2 ignored); clippy + fmt clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@zmanian
zmanian force-pushed the attested-signing-multitenant-model branch from 9100199 to a2b7572 Compare July 24, 2026 22:11
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)
  • staging
  • reborn-integration

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 16c256be-5d07-4bf2-a68c-d7977b202db2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@zmanian

zmanian commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #6813, part of consolidating the 20-PR attested-signing-* stack into 8 PRs against current main.

Verified before closing: this branch is an ancestor of the old stack tip, so its content is carried forward in full. The consolidation also re-based everything onto current main — which had moved 79 commits and retired ironclaw_product_workflow, the WebUI composition facade, and the old failure-kind vocabulary — and fixed the CI failures that were red here, including the missing [package.metadata.ironclaw] layer declarations that had been blocking the bottom of the stack.

Closing to keep the queue honest. The review history stays on this PR and remains readable; reopen if the consolidation is rejected.

@zmanian zmanian closed this Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: low Changes to docs, tests, or low-risk modules scope: dependencies Dependency updates scope: docs Documentation size: L 200-499 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants