Skip to content

fix(security): make single-use token primitives atomic - #736

Merged
lakhansamani merged 1 commit into
mainfrom
security/single-use-primitives-and-m2m-audience
Jul 30, 2026
Merged

fix(security): make single-use token primitives atomic#736
lakhansamani merged 1 commit into
mainfrom
security/single-use-primitives-and-m2m-audience

Conversation

@lakhansamani

Copy link
Copy Markdown
Contributor

Why

Authorization codes and refresh tokens were validated by a read followed by a
separate delete. Concurrent redemption therefore handed the same credential to
every caller. Both reproduced against a real build:

  • 39 of 40 concurrent double-redemptions returned the same authorization
    code to both callers (RFC 6749 §4.1.2 single-use defeated).
  • Two simultaneous refreshes both returned 200 and each minted an
    independent, separately-rotating token family from one refresh token
    (OAuth 2.1 §6.1 reuse detection defeated).

Neither was reachable by the existing suite: every replay assertion in the repo
is sequential (await first, then send second), which a read-then-delete passes
perfectly, and there was not one Promise.all anywhere in the e2e specs.

What changed

Atomic single-use claims. DeleteOAuthStateByKey and
DeleteSessionTokenByUserIDAndKey now return whether this call removed the
row, decided by one atomic operation per backend:

Backend Mechanism
SQL RowsAffected
MongoDB DeleteOne + DeletedCount
ArangoDB REMOVE … RETURN OLD
Cassandra / ScyllaDB DELETE … IF EXISTS (Paxos LWT)
DynamoDB conditional delete on attribute_exists
Couchbase KV Remove + ErrDocumentNotFound

Redis and in-memory were already correct. TokenHandler gates issuance on
winning the claim; the loser gets the same opaque invalid_grant a replay does.

DB-backed cache was process-local. The store selected for any deployment
with a database and no REDIS_URL kept its cache in a package-level map, so
SAML assertion single-use, RFC 7523 jti replay and OTP lockout counters were
per-replica — N replicas granted N× the attempts. Moved into
authorizer_session_tokens, which has an indexed (user_id, key_name), a real
expires_at, and an existing reaper. (oauth_states has no reaper at all.)

M2M audience binding. client_credentials ignored RFC 8707 resource, so
every service-account token carried the same aud and was valid at every
internal service. Auth0 makes audience mandatory on this grant; Keycloak binds
it via audience mappers. Omitting resource keeps the previous behaviour.

Also: token-exchange accepted any opaque string as resource where
/authorize enforced absolute-URI; DB-store session expiry was exclusive
(entries lived one second past expiry) where in-memory and Redis are inclusive;
startup now warns when --url is unset (GHSA-m82j-rq33-qjx2 — reset links are
otherwise built from attacker-controllable Host headers).

New coverage for the blind spots

  • internal/memory_store/single_use_test.go — concurrent claim contract, run
    against every provider. The db provider was declared in the test matrix
    but never exercised; adding it immediately surfaced the expiry off-by-one.
  • e2e-playground/tests/concurrency.spec.ts — fires redemptions with
    Promise.all instead of sequentially. This is what caught the refresh race.
  • e2e-playground/tests/replica-shared-state.spec.ts + two authorizer replicas
    over a shared Postgres — the first place two instances share state at all, so
    cross-replica cache coherence is testable.
  • internal/parsers/host_header_injection_test.go — pins both halves of
    GHSA-m82j-rq33-qjx2 (mitigated with --url, vulnerable without).
  • make e2e-playground never rebuilt the Playwright image, so any spec edit
    was silently ignored and the stale suite re-ran. Added --build.

Known gaps, documented in place — not fixed here

Each needs a design decision rather than a patch:

  1. Stale roles on refresh. Roles are carried from the old token, so a
    stripped role keeps being minted. Do not intersect with user.Roles: SSO
    and SAML build session roles as
    unionRoles(splitRoles(user.Roles), orgGroupDerivedRoles(...)), where the
    derived half comes from the FGA engine. Intersecting strips every
    org-group-derived role and, for SCIM users (created with no Roles field),
    rejects the grant outright. Verified: ["user","org_admin"] ∩ "user" → ["user"].
    Correct fix re-derives effective roles at refresh, which needs org context in
    the token plus an FGA lookup.
  2. Delegated (RFC 8693) tokens have no revocation. The 5-minute TTL is the
    only bound; a prior comment claiming /oauth/introspect enforced it was
    false and is corrected. Real revocation needs a resource registry.
  3. Concurrent AutoMigrate kills a replica on a fresh shared database.
    Worked around in the e2e compose with a comment; wants an advisory lock.

Fault tolerance

Fail-closed on security gates, fail-open on availability aids — matching the
existing philosophy. A claim error returns a retryable 503, not
invalid_grant, so a store fault is never reported as permanently-bad
credentials. On error the claim bool is always false even if a row was already
removed (fail-safe direction), written into the interface contract.

Residual, all documented and all absent when REDIS_URL is set: the DB cache
fails open on read faults (the storage layer cannot distinguish "no row" from
"read failed" — 7 different error types), SetCache is delete-then-insert so a
partial failure loses the key, and IncrementCache can undercount under exact
cross-replica concurrency. Multi-instance production should use Redis, not
just a shared database.

Verification

All run on the final tree:

Gate Result
go build / go vet pass
make lint 0 issues
go test ./... (SQLite) exit 0, 39 packages
memory_store with Redis pass
make test-all-db (7 backends) exit 0
make e2e-playground 75/75, 0 skipped
make smoke pass

test-all-db earned its keep: the first run failed on ScyllaDB only, because
ScanCAS() with no destinations cannot read an LWT result set that carries the
row's columns (have 1 want 8). Every claim errored to false, which would
have made Cassandra/ScyllaDB deployments unable to redeem any code or refresh
token at all. Fixed with MapScanCAS, matching the existing LWT usage in
org_domain.go.

Review notes

Two changes in an earlier pass of this work were wrong and were reverted or
reworked after review — the role narrowing above, and the cache initially landing
in oauth_states (a table with no reaper, scanned on every successful OTP
verification). Both are called out because the diff is large and
security-sensitive; the storage-interface changes in particular touch all seven
backends.

Authorization codes and refresh tokens were validated by a read
followed by a separate delete, so concurrent redemption handed the
same credential to every caller. Reproduced: 39/40 concurrent code
redemptions returned the code to both callers, and two simultaneous
refreshes both minted an independent rotating family, defeating
OAuth 2.1 6.1 reuse detection.

DeleteOAuthStateByKey and DeleteSessionTokenByUserIDAndKey now report
whether THIS call removed the row, decided by one atomic operation per
backend: SQL RowsAffected, Mongo DeleteOne, Arango REMOVE RETURN OLD,
Cassandra DELETE IF EXISTS (LWT), DynamoDB conditional delete,
Couchbase KV Remove. Callers gate issuance on winning the claim.

Also fixed:
- DB-backed memory store kept its cache in a process-local map, so
  SAML assertion replay defence, RFC 7523 jti replay and OTP lockout
  counters were per-replica. Moved into authorizer_session_tokens,
  which has an indexed (user_id, key_name), a real expires_at and an
  existing reaper.
- client_credentials ignored RFC 8707 resource, so every service
  account token shared one aud and was valid at every internal
  service. Auth0 requires audience here; Keycloak binds it via
  mappers.
- token-exchange accepted any opaque string as resource where
  /authorize enforced absolute-URI.
- Session expiry was exclusive on the DB store only, keeping entries
  alive one second past expiry.
- Warn at startup when --url is unset (GHSA-m82j-rq33-qjx2): reset
  links are then built from attacker-controllable Host headers.

Known gaps left documented in place, each needing a design change
rather than a patch: stale roles on refresh (needs effective-role
re-derivation; narrowing to user.Roles strips org-group-derived roles
and locks out SCIM users), delegated-token revocation (needs a
resource registry), concurrent AutoMigrate on a fresh shared database.

Verified: go vet, lint, SQLite suite, memory_store with Redis,
test-all-db across 7 backends, e2e 75/75, smoke.
@lakhansamani

Copy link
Copy Markdown
Contributor Author

Review pass

CI green across all 5 required checks (Go tests, golangci-lint, lint & breaking, release smoke, govulncheck).

Hygiene: no secrets, no debug statements, no stray fmt.Print. The two TODOs are in provider_template/, which is the scaffold whose purpose is TODOs — both now carry the atomicity contract so a new backend cannot implement the claim as read-then-delete.

On the three t.Skipf calls in single_use_test.go: these skip only when Redis is not running on :6380, matching the pre-existing pattern in memory_store/provider_test.go. They are not unconditional skips — the Redis path was explicitly exercised for this change (TEST_ENABLE_REDIS=1, all three single-use tests pass including 40 rounds of concurrent claims). The e2e suite has zero skips.

Highest-risk area for a reviewer to focus on: the storage-interface signature change touches all seven backends, and only SQL is covered by the default CI job. make test-all-db was run locally and passes, but note it caught a genuine break on the first attempt — ScanCAS() cannot read an LWT result set carrying the row's columns, so every Cassandra/ScyllaDB claim errored to false, which would have made those deployments unable to redeem any authorization code or refresh token. Fixed with MapScanCAS. CI does not cover this; the six non-SQL claim paths rest on the local all-DB run.

Second focus area: internal/memory_store/db/cache.go is a rewrite, not a tweak — the cache moved tables. Worth confirming the reserved user_id namespace reasoning holds (it must never collide with a real session key, and must not be swept by DeleteSessionTokensByNamespace or DeleteAllSessionTokensByUserID).

Not approved by me — I authored this, and self-approval is both blocked by GitHub and inappropriate for a security-sensitive diff of this size. Two changes in an earlier pass of this same work were wrong (documented in the description); that is the argument for a genuine second reader rather than a formality.

@lakhansamani
lakhansamani merged commit d0a1bc9 into main Jul 30, 2026
6 checks passed
@lakhansamani
lakhansamani deleted the security/single-use-primitives-and-m2m-audience branch July 30, 2026 12:24
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