fix(mfa): bind the challenge to its tenant, and scope every MFA key by it by msalvatti · Pull Request #141 · bymaxone/rust-auth

msalvatti · 2026-08-10T14:31:30Z

What was wrong

The MFA temp token carried no tenant, so the challenge had none in scope and resolved its subject with find_by_id(sub, None) — by id, across every tenant. Everything downstream then ran on whatever row the repository returned:

the status gate (assert_not_blocked) — evaluated against the wrong account
mfa_enabled, the encrypted secret it decrypts, the recovery-code digests it scans
and the account the session is finally minted for

A library cannot assume the host's ids are unique across tenants — find_by_id takes a tenant precisely because they may not be — and under a schema that numbers users per tenant, every tenant has a user 1. This is the same find_by_id(sub, None) class closed for the status gate in #139, still live on the challenge path because there was no tenant to pass.

Eight preimages, not five — and the counters are the worst

Eight key derivations were scoped by plane but not by tenant:

key	preimage
`mfalock:`	transition lock
`mfa_setup:`	pending-enrolment record
`ra:`	recent-auth marker
`tu:`	TOTP anti-replay marker
`rcu:`	recovery-code claim
`lf:` ×3	challenge / disable / reauth failure counters

The three counters are the worst and it is not close. The other five cost a collision between two accounts that happen to share an id. A shared counter is a credential-free cross-tenant lockout: failures against tenant A's user spend tenant B's budget, and a success on either side clears the other's. That is not the per-subscriber-account rate limiting NIST SP 800-63B requires, nor the per-tenant isolation the OWASP Multi-Tenant Security Cheat Sheet asks of any key in a shared datastore.

The contract's own identifierPreimages comment documents this exact bug one level up — a tenant literally named platform collided with the platform plane and locked an operator out of the console. That fix landed for the login counter and stopped there.

Refuse, don't fall back

tenantId is optional on the wire and mandatory in effect. A dashboard challenge token without it is refused; a platform token with one is refused too. A fallback would leave the vulnerable derivation reachable by omitting an optional field — the attacker picks the old path.

RFC 8725 §3.9 — absent audience ⇒ MUST reject
RFC 8725 §3.12 — validation rules must be mutually exclusive, rejecting a token of the wrong kind
ASVS 5.0 6.6.2 — the out-of-band token must be bound to the authentication request that generated it

Design notes

All eight derive from one scoped_subject, driven by the plane rather than by whether a tenant was supplied — so a platform caller that passes one cannot move the platform preimage off platform:{userId}, which is half of a cross-implementation agreement with nest-auth. Pinned by scoped_subject(Platform, Some("t1"), u) == scoped_subject(Platform, None, u).
require_plane_tenant refuses a dashboard operation with no tenant at the five public entry points, before any key is derived — the core API is public and a host can call it directly without going through the axum layer.
The tenant on the temp token comes from the authenticated account, never from the request that named it.

Contract

Six of the eight derivations were unpinned, so they could drift with nothing to detect it — the same blind spot that let the OTP stored value move unnoticed. All eight are now named under mfaSubjectPreimages (the single definition) and mfaSubjectDerivedKeys, with a test that reads the file and compares it against what the code builds.

identifierPreimages is untouched — it is the email-keyed lf: login preimage and was already correct. It looked like it covered the three counters because they share the lf: prefix; they do not.

Mutation anchor

Re-anchors the issue_mfa_temp_token equivalent-mutant exclusion, which had been stale on main since before this change (the method sat at 1022, the anchor read 632). The stale anchor did surface both mutants as survivors — but the sweep runs post-merge, so a survivor is a report line and not a red PR. A broken anchor there does not fail closed, which is now written down beside it.

Gates

cargo fmt --check, cargo clippy -D warnings, RUSTDOCFLAGS="-D warnings" cargo doc — clean
full suite green
cargo llvm-cov — lines 100%, functions 100%
cargo mutants --in-diff — 62 tested, 55 caught, 7 unviable, 0 survivors
the cross-tenant test was red-checked: reverting find_by_id(sub, tenant) to None turns that test red and only that one

Cross-repo

nest-auth mirrors the same gaps and is implementing them in parallel; the contract text here is what that side mirrors byte-for-byte. rust-auth is unpublished with no consumer, so it takes a hard cutover with no migration machinery — nest-auth, with 17 published versions, needs dual-write on tu:/the counters and dual-acquire on mfalock: for one release. Byte-identity is required on the final format, not on the migration path.

…y it The MFA temp token carried no tenant, so the challenge had none in scope and resolved its subject with `find_by_id(sub, None)` — by id, across every tenant. Everything downstream then ran on whatever row the repository returned: the status gate, `mfa_enabled`, the secret it decrypts, the recovery digests it scans, and the account the session is finally minted for. A library cannot assume the host's ids are unique across tenants — `find_by_id` takes a tenant precisely because they may not be — and under a schema that numbers users per tenant, every tenant has a user `1`. This is the same `find_by_id(sub, None)` class closed for the status gate in #139, still live on the challenge path because there was no tenant to pass. The token now carries `tenantId` on the dashboard arm. It is optional on the wire and mandatory in effect: a dashboard challenge token WITHOUT it is refused, and a platform token WITH one is refused too. Falling back to the unscoped shape would leave the vulnerable derivation reachable by omitting an optional field, which lets the attacker pick the old path. RFC 8725 §3.9 sets absent-means-reject for a missing audience, §3.12 asks for validation rules that reject a token of the wrong kind, and ASVS 5.0 6.6.2 requires the out-of-band token be bound to the authentication request that generated it. Eight key derivations were scoped by plane but not by tenant, not five: the transition lock, the pending-enrolment record, the recent-auth marker, the TOTP anti-replay marker, the recovery-code claim, and the challenge, disable and reauth failure counters. The three counters were the worst and it is not close — the other five cost a collision between two accounts sharing an id, while a shared counter is a CREDENTIAL-FREE CROSS-TENANT LOCKOUT: failures against one tenant's user spend another tenant's budget and a success on either side clears the other's. That is not the per-subscriber-account rate limiting NIST SP 800-63B requires. The contract's own `identifierPreimages` comment documents this exact bug one level up, where a tenant literally named `platform` locked an operator out of the console; that fix landed for the login counter and stopped there. All eight now derive from one `scoped_subject`, driven by the PLANE rather than by whether a tenant was supplied — so a platform caller that passes one cannot move the platform preimage off `platform:{userId}`, which is half of a cross-implementation agreement. `require_plane_tenant` refuses a dashboard operation with no tenant at the five public entry points, before any key is derived, because the core API is public and a host can call it directly. The contract pinned only two of the eight, so six derivations could drift with nothing to detect it — the same blind spot that let the OTP stored value move unnoticed. All eight are now named, under `mfaSubjectPreimages` (the single definition) and `mfaSubjectDerivedKeys`, and a test reads the file and compares it against what the code builds. `identifierPreimages` is untouched: it is the email-keyed `lf:` login preimage and it was already correct. Also re-anchors the `issue_mfa_temp_token` equivalent-mutant exclusion, which had been stale on `main` since before this change (the method sat at 1022, the anchor read 632). The stale anchor did surface both mutants as survivors, but the sweep runs post-merge, so a survivor is a report line and not a red PR — a broken anchor there does not fail closed, which is now written down beside it. Gates: fmt, clippy -D warnings, rustdoc -D warnings, full suite green, llvm-cov lines 100% / functions 100%, cargo-mutants in-diff 62 tested, 55 caught, 7 unviable, 0 survivors.

Copilot

Pull request overview

Binds MFA challenges and datastore keys to tenant-scoped identities, preventing cross-tenant account resolution and lockout collisions.

Changes:

Adds tenant claims to MFA temporary tokens and validates plane/tenant combinations.
Tenant-scopes MFA keys, counters, lookups, and related tests.
Extends the wire contract and updates mutation-test configuration.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`packages/rust-auth/src/shared/jwt-payload.types.ts`	Exposes the optional tenant claim.
`crates/bymax-auth-types/src/claims.rs`	Adds and tests MFA tenant serialization.
`crates/bymax-auth-jwt/src/keys.rs`	Updates JWT claim fixtures.
`crates/bymax-auth-jwt/src/hs256.rs`	Updates HS256 fixtures.
`crates/bymax-auth-core/src/services/token_manager.rs`	Issues and verifies tenant-bound challenges.
`crates/bymax-auth-core/src/services/platform.rs`	Issues tenantless platform challenges.
`crates/bymax-auth-core/src/services/oauth.rs`	Uses the authenticated OAuth account tenant.
`crates/bymax-auth-core/src/services/mfa/tests.rs`	Adds tenant-isolation and contract tests.
`crates/bymax-auth-core/src/services/mfa/setup.rs`	Tenant-scopes MFA enrollment.
`crates/bymax-auth-core/src/services/mfa/mod.rs`	Centralizes tenant-scoped MFA key derivation.
`crates/bymax-auth-core/src/services/mfa/manage.rs`	Tenant-scopes management operations.
`crates/bymax-auth-core/src/services/mfa/challenge.rs`	Resolves and processes tenant-bound challenges.
`crates/bymax-auth-core/src/services/auth/login.rs`	Adds the authenticated tenant to challenges.
`conformance/wire-contract.json`	Defines shared MFA key preimages.
`bindings/bymax-auth-wasm/src/jwt_edge.rs`	Updates WASM claim fixtures.
`.cargo/mutants.toml`	Re-anchors the token issuer exclusion.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…tenant at issuance Addresses the four review findings on #141. All were real; none dismissed. Scoping the READ was not enough. `UserRepository::update_mfa` carried no tenant, so the write every MFA transition performs could not be scoped: under a schema that numbers users per tenant the update lands on another tenant's row, or the row this flow read is not the row it wrote and a recovery code spent by a successful challenge is never spliced out. The trait now takes `tenant_id`, threaded from `transition_mfa_record` through `persist_mfa`, and the in-memory fixture enforces it on writes so a test cannot pass against a fixture laxer than the contract. The existing single-use test did NOT catch that. A second challenge with the same code is refused by the `rcu:` claim, planted before the splice and living 300 s, so the code reads as spent for the whole window whether or not the write landed — and comes back once the claim lapses. The new regression asserts the PERSISTED digest list shrinks by one in the tenant's own row, which is what distinguishes "refused by the claim" from "actually spent" (ASVS 5.0 6.5.1). Passing `tenant_id` to the repository is a request, not an enforcement: the repository is the host's, and a single-tenant host whose `find_by_id` ignores its second argument is the shape nobody notices — precisely the deployment where ids collide. `login` already refuses such an answer; the challenge is the other door into the same account and had no such check. It now warns and filters, covered by a deliberately non-compliant fixture repository. Issuance accepted shapes verification rejects, so a host calling the public core API received a signed credential — with a planted `mfa:` marker — that could never be redeemed, surfacing one round-trip later as an opaque invalid token. Both now consult one predicate, `plane_tenant_is_well_formed`, which also refuses `Some("")`: a blank tenant would build `dashboard::{userId}`, a third keyspace neither implementation derives, and a blank string is what an unset environment variable looks like by the time it reaches a call site. The errors differ deliberately — a caller ASKING for a malformed challenge gets `auth.validation` naming the field, one PRESENTING it gets the same opaque refusal every bad temp token gets. That guard immediately caught a fixture passing `Some("t1")` on the platform plane, which had always "worked" because nothing checked the pair. One coverage note worth keeping: a `tracing` field on its own line is only evaluated when a subscriber is listening, so it reads as uncovered in every run that does not install one. Bound before the macro instead, with the reason at the call site. Gates: fmt, clippy -D warnings, rustdoc -D warnings, full suite green, llvm-cov lines 100% / functions 100%, cargo-mutants in-diff 71 tested, 64 caught, 7 unviable, 0 survivors. The cross-tenant and splice tests were red-checked.

Copilot

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

crates/bymax-auth-core/src/services/mfa/tests.rs:3014

This only checks that each contract entry is a string; any formula is accepted. A typo or peer-contract change such as dropping the challenge: namespace therefore leaves Rust's derivation unchanged while this drift detector stays green. Compare every contract formula with an independently constructed value from the corresponding Rust helper, rather than checking presence alone.

        assert!(
            derived
                .get(key)
                .and_then(serde_json::Value::as_str)
                .is_some(),

crates/bymax-auth-core/src/services/mfa/mod.rs:247

This new Validation return changes the documented error contract of all five public callers, but their # Errors sections still omit it (setup.rs:18-22,100-103 and manage.rs:18-22,85-89,130-134). Document malformed plane/tenant pairs so API consumers can handle the new error.

    Err(AuthError::Validation {
        details: vec![bymax_auth_types::FieldError {
            field: "tenantId".to_owned(),
            message: "a dashboard MFA operation requires a non-empty tenantId, and a platform \
                      one requires none"
                .to_owned(),
        }],

crates/bymax-auth-core/src/traits/repository.rs:75

The public trait now requires tenant_id, but the implementer example in README.md:212 and the trait definition in docs/technical_specification.md:1484 still show the old two-argument method. Copied integrations no longer compile and the specification omits the required tenant-scoped write; update both references with this signature.

    async fn update_mfa(
        &self,
        id: &str,
        tenant_id: Option<&str>,
        data: UpdateMfaData,
    ) -> Result<(), RepositoryError>;

packages/rust-auth/src/shared/jwt-payload.types.ts:127

Adding this claim makes the public getTenantId documentation inaccurate: packages/rust-auth/src/nextjs/jwt.ts:224 still promises undefined for every MFA-temp token, while the helper now returns this value for dashboard challenges. Update that return contract to distinguish dashboard and platform MFA-temp tokens.

 * Optional in the wire format so the claim is additive, but a dashboard challenge token
 * WITHOUT it is refused at verification rather than falling back to the unscoped shape —
 * a fallback would leave the vulnerable derivation reachable by simply omitting a field.
 */
tenantId?: string,

crates/bymax-auth-core/src/services/token_manager.rs:1066

This public method now returns AuthError::Validation for a malformed plane/tenant pair, but its # Errors section still lists only signing and store failures. Add the validation case so callers are not surprised by an undocumented error variant.

        tenant_id: Option<&str>,

+    match (ctx, tenant_id) {
+        (MfaContext::Dashboard, Some(tenant)) => {
+            format!("{}:{tenant}:{user_id}", ctx.as_str())
+        }
+        _ => format!("{}:{user_id}", ctx.as_str()),


Copilot AI balanced review requested due to automatic review settings August 10, 2026 14:31

Copilot started reviewing on behalf of msalvatti August 10, 2026 14:32 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

Comment thread crates/bymax-auth-core/src/services/mfa/challenge.rs

Comment thread crates/bymax-auth-core/src/services/mfa/mod.rs Outdated

Comment thread crates/bymax-auth-core/src/services/token_manager.rs

Comment thread crates/bymax-auth-core/src/services/mfa/challenge.rs

Copilot AI review requested due to automatic review settings August 10, 2026 15:28

Copilot started reviewing on behalf of msalvatti August 10, 2026 15:28 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

Comment thread crates/bymax-auth-core/src/services/mfa/mod.rs

Comment on lines +193 to +197

match (ctx, tenant_id) {

(MfaContext::Dashboard, Some(tenant)) => {

format!("{}:{tenant}:{user_id}", ctx.as_str())

}

_ => format!("{}:{user_id}", ctx.as_str()),

msalvatti merged commit f4028a2 into main Aug 10, 2026
28 checks passed

msalvatti deleted the fix/scope-mfa-preimages-to-tenant branch August 10, 2026 15:59

Navigation Menu

Uh oh!

fix(mfa): bind the challenge to its tenant, and scope every MFA key by it - #141

fix(mfa): bind the challenge to its tenant, and scope every MFA key by it#141
msalvatti merged 2 commits into
mainfrom
fix/scope-mfa-preimages-to-tenant

msalvatti commented Aug 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

msalvatti commented Aug 10, 2026

What was wrong

Eight preimages, not five — and the counters are the worst

Refuse, don't fall back

Design notes

Contract

Mutation anchor

Gates

Cross-repo

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants