Repository navigation
fix(oauth2): make refresh-family revocation reliable across stores - #91
Merged
Merged
Conversation
The token-family work of #82 needs a vocabulary the storage contract can rely on before any store changes. ErrTokenFamilyRevoked is the invalid_grant a store answers for an operation on a revoked family. ErrTokenFamilyRevocationFailed reports a family revocation that could not be recorded; it is a plain sentinel rather than an *Error, so it never reaches the wire on its own and keeps matching through errors.Is once wrapped (an *Error has no Is method: a WithDescription copy no longer matches its sentinel). TokenPair.Validate rejects a pair no store may persist: a refresh token without a family, in another family than its access token or already consumed, and a family identifier longer than MaxFamilyIDLength — the 64 bytes the SQL schema stores, so no backend can silently truncate two families into one. FamilyRevocationRetention bounds how long a revoked family is remembered. ErrRefreshTokenReused now reads "refresh token reused": the former description claimed the family was revoked even when that revocation had failed, and its em dash lies outside the RFC 6749 §5.2 error_description charset. Refs #82.
The sqlstore module documents PostgreSQL, MySQL and SQLite, but its DDL only ran on SQLite, the one engine CI exercises. PostgreSQL rejected the refresh-token table outright — a BOOLEAN column cannot default to 0 — and would then have refused every consumed = 0 / consumed = 1 comparison the store issues, since it has no implicit integer/boolean cast. MySQL rejected the CREATE INDEX IF NOT EXISTS statements of the two token-family indexes. consumed is now a SMALLINT holding 0/1 on every engine, and the family indexes are dropped: the authoritative family record #82 introduces makes lookups by family unnecessary, and no portable idempotent CREATE INDEX exists. Schema therefore returns the same DDL whatever the dialect; the argument is kept and ignored. Refs #82.
The storage contract promised family revocation on refresh-token reuse,
but no store gave it a family-wide boundary. The SQL store ran its
UPDATE and DELETE outside a shared transaction, the Redis store
enumerated the family before mutating it member by member, and nothing
gated issuance: a rotation, or the non-rotating Profile20 refresh, could
leave a usable token in a family whose revocation had just returned,
and a family revoked before its first token left no trace at all. On
the reuse path the SQL and Redis stores discarded the revocation error
and returned the bare ErrRefreshTokenReused. The grants persisted the
access token before minting and rotating the refresh token, so a later
failure left a live access token the client never received.
Every store now keeps one authoritative record per token family —
active or revoked — and serializes every write into the family on it:
a row locked with SELECT ... FOR UPDATE in SQL (SQLite's single writer
otherwise), one key written by Lua scripts in Redis, the mutex in
memory. Lookups consult the record, so a revocation is one idempotent
write that needs no purge and works before the first issuance, and a
missing record fails closed. Issuance and rotation become single atomic
operations: SaveTokenPair replaces SaveAccessToken and
SaveRefreshToken, RotateRefreshToken takes the whole replacement pair,
and each grant exchange makes exactly one persistence call.
A reuse whose revocation fails now returns ErrRefreshTokenReused joined
with ErrTokenFamilyRevocationFailed; the refresh grant retries the
revocation on a context detached from the request and reports a second
failure through grant.Config.OnError, while the client still gets
invalid_grant. A token that vanished before its rotation answers
invalid_grant instead of server_error, the empty family ID is refused
instead of revoking every family-less token, and a Redis rotation
resent by go-redis after a lost reply is no longer taken for a reuse.
This breaks oauth2.Storage, the SQL schema (new oauth2_token_families
table) and the Redis key layout (fam:{id} replaces the famrt:/famat:
sets). Tokens issued before the upgrade belong to a family without a
record and are therefore unusable: users re-authenticate. The
guarantees are stated on oauth2.Storage and enforced by storetest,
which the memory, SQL and Redis stores pass, concurrent revocations
included. Refs #82.
/revoke answered 200 OK whatever happened. A failed RevokeAccessToken or RevokeRefreshFamily only reached the error hook, and a token lookup that failed for a backend reason skipped the revocation without any trace: in both cases the client was told its token was gone while it may still have been usable. RFC 7009 §2.2 answers 200 for a token that was revoked or is invalid; §2.2.1 provides 503 for a server that cannot complete the revocation, after which the client must assume the token still exists and may retry. /revoke now answers 503 temporarily_unavailable when a lookup fails for another reason than an unknown token, or when the revocation fails and leaves the token usable: the access token could not be deleted and its family, if any, could not be revoked either, or the refresh token's family could not be revoked. A deleted access token whose family revocation failed, or a failed delete covered by the family revocation, still answers 200. Unknown tokens and other clients' tokens answer 200 as before, so the response still tells nothing about a token the caller does not hold. Retries are safe: revocation is idempotent. Each failure reaches the error hook exactly once, as a server_error carrying its cause — a failed lookup as "revoke: token lookup failed"; the 503 body is written without a second notification, so sinks filtering on server_error see the same events as before. This reverses the "responses unchanged" of the error-hook entry for failed revocations. Refs #82.
… JWT limits The storage contract now guarantees what a family revocation means, and the deployment documentation has to say it, along with what it does not cover. docs/security-considerations.md gains a "Token-family revocation" section: when a revocation is complete, how concurrent issuance and rotation are ordered around it, revocation before the first token and its retention, atomic writes, idempotence and isolation, the failure and retry rule behind the /revoke 503 and the reuse-path report, the fail-closed treatment of lost family state, and the backend assumptions the guarantees rest on. A separate "Offline JWT access tokens" section states that a store revocation never reaches a resource server verifying JWTs offline: the RFC 9068 payload carries no family or jti claim, so such a token stays valid until it expires (RFC 7009 §2.1, §3), and lists the mitigations. The rotation bullet links to the new section, and the atomicity bullet replaces the inaccurate "100-goroutine races" with what the conformance suite checks. MIGRATION.md maps the removed Storage methods to SaveTokenPair and the new RotateRefreshToken, lists what custom stores, the SQL and Redis deployments and pre-upgrade tokens need, and describes the /revoke 503. LIMITATIONS.md records offline JWT revocation, Redis Cluster, the missing SQL expiry job and the PostgreSQL/MySQL CI gap; architecture.md, observability.md and the JWT generator godoc are updated to match. Refs #82.
…evoke Revoking an access token at /revoke deleted the token first, then revoked its family, and answered 200 OK whenever the delete had succeeded. A family revocation that failed was reported to the error hook but could never be redone: a retry looked the deleted access token up, missed it, and answered 200 OK again, while the family's refresh tokens stayed usable — although the documentation promised a 503 for an unfinished revocation. The family is now revoked first. When that fails, the access token is left in place and /revoke answers 503 temporarily_unavailable (RFC 7009 §2.2.1), so the retry the client is told to make redoes both. Once the family is revoked, no token of it is usable, and deleting the access token is cleanup: a failed delete is reported but still answers 200 OK. A token outside any family is deleted, and a failed delete still answers 503. Each storage failure keeps reaching the error hook exactly once. Refs #82.
…rder Two branches of the token-family contract were implemented but not pinned by a test, so a regression would have gone unnoticed. The SQL store reads a family it could not find, right after its insert-if-absent, as revoked: the record may vanish under a concurrent cleanup on PostgreSQL. A SQLite trigger that silently drops the insert now exercises it: neither an issuance nor a rotation goes through, and nothing is persisted. The storage contract decides a revoked family before a consumed token. The conformance suite now replays a token once more after its reuse revoked the family, and every store must refuse it as a revoked family rather than as another reuse. Also drop the nolint directives of the new test doubles: test files are not linted. Refs #82.
The token-family documentation claimed more than the stores guarantee. A lookup does treat a token whose family state is lost as revoked, but the next issuance into that family records it active again: losing a revoked family's state before its retention ends — an evicted Redis key, a family row deleted early — can revive its tokens. The Redis package documentation called eviction harmless and noeviction a recommendation, and the SQL one called an early family delete safe. Keeping that state for its whole retention is now a stated requirement: the Storage godoc (I8), the SQL cleanup rules (never delete a family row before its expires_at), the Redis requirements (maxmemory-policy noeviction, since family keys carry TTLs and volatile-* policies evict them too), the security considerations, the migration guide and the changelog, which also names the Redis 6.0 requirement KEEPTTL brings. I6 now mentions the invalid_request for a malformed family ID, the READ COMMITTED pin is scoped to the token-family transactions it applies to, and only the token-family Redis scripts are claimed safe to resend. Refs #82.
Coverage Report for CI Build 38014578498Coverage increased (+2.4%) to 94.398%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #82.
Problem
The storage contract promised that replaying a rotated refresh token revokes
its whole token family, but no store gave that revocation a family-wide
boundary, and nothing gated issuance against it:
RevokeRefreshFamilyranUPDATE oauth2_refresh_tokens SET consumed = 1 WHERE family_id = ?andDELETE FROM oauth2_access_tokens WHERE family_id = ?as two autocommit statements: a token committed by aconcurrent rotation outside the UPDATE's snapshot escaped it (PostgreSQL
READ COMMITTED), and a failure between the two statements left half a
revocation that nothing recorded.
famrt:F/famat:FwithSMEMBERS,then mutated the members one command at a time: a rotation landing after
the snapshot stayed active.
discarded the revocation error (
_ = s.RevokeRefreshFamily(...)) andreturned the bare
ErrRefreshTokenReused, whose description still said"family revoked".
the refresh token and rotating: any later failure left a live access token
the client never received.
stopped a non-rotating
Profile20refresh from issuing into a family whoserevocation had just completed.
Reproductions run on the base before any change (scratch tests, not
committed), all confirmed:
SMEMBERS famrt:Fand its writesRevokeRefreshFamilyreturns nil, RT2 readsConsumed=falseProfile20refresh; the family is revoked right after the grant's lookupauthorization_codeserver_error, yet the minted access token is live in the storeErrRefreshTokenReusedsentinel, no cause; the family's access token stays liveRevokeRefreshFamily(""), memory and SQLclient_credentialsaccess tokenAlso found on the way:
token issued outside a family (
client_credentials, implicit).consumed BOOLEAN NOT NULL DEFAULT 0and the store'sconsumed = 0/1comparisons (no implicit integer/boolean cast); MySQL rejects
CREATE INDEX IF NOT EXISTS.script saw its own consumption and answered "refresh token reused",
revoking the family the request had just rotated into (reproduced with a
hook resending the script against the old store).
Redis TTL) answered
server_errorinstead ofinvalid_grant; the Redisreuse path revoked the family the caller declared, not the stored one; and
/revokeanswered200 OKwhen its lookup failed for a backend reason (therevocation was skipped without a trace) or when the revocation failed.
Fix
One authoritative record per family. Every store keeps the state of each
token family — active or revoked — and serializes every write into the
family on it: a row of the new
oauth2_token_familiestable locked withSELECT … FOR UPDATEin SQL (SQLite's single writer otherwise), onefam:{id}key written only by Lua scripts in Redis, the mutex in memory.Lookups consult the record, so a revocation is a single idempotent write:
nothing to enumerate or purge, and it works before the family's first token
exists.
Atomic pairs.
SaveAccessToken/SaveRefreshTokengive way toSaveTokenPair, which persists a newly issued pair — both tokens or neither— and refuses a revoked family;
RotateRefreshToken(ctx, oldHash, next *TokenPair)consumes the presented token and persists the wholereplacement pair atomically. Each grant exchange makes exactly one
persistence call.
The guarantees, stated on the
oauth2.Storagegodoc and checked bystoretest:(
RevokeRefreshFamilyreturned nil, or a reuse was reported withoutErrTokenFamilyRevocationFailed), no token of the family is usable throughthe store — access lookups fail with
invalid_grant, refresh tokens readconsumed — and every issuance or rotation into the family, including one
racing the revocation, is ordered either before it, and covered, or after
it, and refused with
ErrTokenFamilyRevokedwithout persisting anything.SaveTokenPairorRotateRefreshTokenpersists no token and consumes nothing; reuse only records the revocation.
ErrRefreshTokenReused; when the store's revocation fails, the error alsowraps the new plain sentinel
ErrTokenFamilyRevocationFailedand itscause, and the family must be treated as active.
again and revoking it again is a no-op; it is kept at least as long as its
tokens, and for
FamilyRevocationRetention(24h) after its revocation.be unknown: treat the family as active and retry.
outside any family; an empty or over-long (
MaxFamilyIDLength, 64 bytes)family ID is refused with
invalid_request.This protects lookups only, hence the retention requirements below.
One decision order for a rotation, in every store: a malformed
nextisrefused with
invalid_requestbefore any I/O; then an unknown old token →invalid_grant;nextin another family →invalid_request; a revoked orunknown family →
ErrTokenFamilyRevoked; a consumed old token → reuse.Per backend:
the family row:
ON CONFLICT DO NOTHING/ON DUPLICATE KEY UPDATE), thenlocks the family row and the old token (
FOR UPDATEon PostgreSQL andMySQL), decides, and writes. A rotation inserts an unknown family as
revoked, so only an issuance creates an active family. PostgreSQL's
token-family transactions are pinned to READ COMMITTED. Lookups are one
statement with a
LEFT JOINon the family row; no row reads as revoked.issuePairrefuses any family state but unknownor
active.rotatePairchecks the old token, the family and theconsumption before its first write, keeps the consumed token's TTL
(
SET … KEEPTTL), and recognizes its own resend (the new refresh keyalready exists →
ok, notreused).revokeFamilywrites the singlefamily key with the longer of its TTL and the retention. On reuse the store
runs
revokeFamilyright after the rotation script: anything committed inbetween is ordered before the revocation, which covers it. Lookups require
the family key to hold exactly
active.Grants.
issueTokenPairand the refresh grant mint everything first,then persist once. A reuse — found by the grant (
rt.Consumed) or reported bythe store — revokes the family on a context detached from the request
(
context.WithoutCancelplus a 10s timeout), so a client hanging up cannotabort it. When the store reports a failed revocation the grant retries it
once; a remaining failure reaches
grant.Config.OnErroras exactly oneserver_error(revoke refresh family failed after reuse detection, itscause naming the family). The client gets
invalid_granteither way. Arefusal the store reports as
invalid_grant(revoked family, vanished token)passes through; any other storage error is a
server_errorwith its causeoff the wire.
/revokeand RFC 7009 §2.2.1. RFC 7009 §2.1 lets a client take200 OKas "the token cannot be used again", so a revocation that may not have taken
effect now answers
503 temporarily_unavailable— after which the client"must assume the token still exists and may retry":
/revokerevokes the familyfirst: if that fails, it answers 503 without deleting the access
token, so the retry redoes both. Once the family is revoked, the access
token is deleted best-effort: a failed delete is reported and still
answers
200 OK, the token being unusable through its revoked family;Each storage failure reaches
ServerConfig.OnErrorexactly once as aserver_errorcarrying its cause; the 503 envelope is written without asecond notification, so sinks filtering on
server_errorsee one event perfailure. Unknown tokens and other clients' tokens still answer
200 OK.Schema portability.
consumedis aSMALLINTon every engine and thefamily indexes are gone;
Schemareturns the same DDL for every dialect.Docs: the
oauth2.Storagegodoc (I1–I8), the SQL and Redis package docs(deployment requirements, key layout, cleanup rules),
CHANGELOG.md,MIGRATION.md,LIMITATIONS.md,docs/security-considerations.md(new"Token-family revocation" and "Offline JWT access tokens" sections),
docs/architecture.md,docs/observability.md.Breaking changes
Nothing is tagged yet, but code tracking
masteris affected — seeMIGRATION.md.oauth2.Storagecontract —SaveAccessTokenandSaveRefreshTokenare removed for
SaveTokenPair;RotateRefreshTokentakes a*TokenPair;RevokeRefreshFamilyrefuses""and IDs over 64 bytes.Access and refresh tokens must share one backend, and
ServerConfig.Storageand everygrant.Config.Storagemust be the samestore. Custom stores must pass
storetest.Migrateaddsoauth2_token_familiesand leaves theexisting tables untouched. Never delete a family row before its
expires_at.fam:{id}replaces thefamrt:/famat:sets (old keys are ignored; delete them withSCANif you like).Redis ≥ 6.0 (
SET … KEEPTTL) andmaxmemory-policy noevictionarenow required: family keys carry TTLs, so the volatile-* policies evict them
too, and a revoked family whose key is lost can be recorded active again by
an issuance into it. Standalone server or Sentinel primary only (not
Cluster), reads from the primary.
family-bound access token becomes unusable (no family record, fail closed):
users re-authenticate, and pre-upgrade refresh tokens answer
invalid_grant("refresh token reused").client_credentialsandimplicit tokens keep working.
/revokeanswers 503 when a revocation may not have taken effect. Thisreverses the "responses unchanged" of the error-hook entry (oauth2: the cause of a server_error is unobservable (no logger or error hook on ServerConfig) #63) for failed
revocations; clients that ignored the status of
/revokeshould retry on503.
error_descriptionofErrRefreshTokenReusedis nowrefresh token reused: "family revoked" could be false, and the em dash was outside theRFC 6749 §5.2 charset.
Tests
storetest — run by memory, the suite itself, SQLite and Redis. The
existing cases are migrated and twelve family cases are added in the new
(linted)
storetest/family.go: revocation before issuance, rotate-then-revoke,revoke-then-rotate, idempotence, isolation, empty and over-long IDs, failed
rotations that persist nothing (unknown token, family mismatch, no refresh
token, nil target, revoked family — each target declaring an existing active
family, so a wrongly persisted token would be found), invalid pairs,
access-only issuance gating, and two race cases (a revocation racing a
rotation chain, 20 concurrent issuances racing a revocation). Race results are
asserted after
Wait— not.Fatalin goroutines — and every assertion goesthrough helpers. The reuse case replays twice and pins the revoked-before-
consumed order; the concurrent-rotation case asserts that the winner's tokens
end up revoked by the first loser.
SQL (
store_family_test.go, SQLite triggers) — a revocation failure ateach statement is reported (
ErrTokenFamilyRevocationFailed, family ID,cause) and a retry succeeds; a reuse whose revocation fails reports both
sentinels and leaves the family active; rotation and issuance stay atomic
under injected failures (tables inspected directly); retention arithmetic; a
missing family row fails closed and a rotation never re-creates it; a
RAISE(IGNORE)insert pins the no-row lock branch; a refresh token storedwithout a family reads consumed; exact dialect statements, the PostgreSQL
rebind and the isolation pin.
Redis (
store_family_test.go, go-redis hooks acting before EVALSHA /EVAL) — failure injection on the revoke script, directly and on the reuse
path; exact interleavings: a rotation just before the revocation, a
revocation just before the rotation, an attacker rotating between the reuse
detection and its revocation, issuance before and after a revocation; a
resent rotation is not a reuse; family TTLs (cover every token, never shrink,
retention floor,
KEEPTTLon the consumed token); missing, unexpected andWRONGTYPE family state fail closed; no
famrt:/famat:key is written.Grant (
grant/family_test.go, a recording decorator over memory) — onepersistence call per exchange, kind and arguments asserted; no access token
left behind by a failing generator or rotation (refresh and
authorization_code); a store-reported reuse with a failed revocation givesone hook event whose cause matches
ErrTokenFamilyRevocationFailedand notErrRefreshTokenReused, and none when the retry succeeds; an alreadycanceled request still revokes, on a live context with a deadline; a
revocation or a reuse landing between lookup and rotation; a non-rotating
Profile20refresh refused in a revoked family; a vanished token answersinvalid_grant; storage failures map toinvalid_grant/server_error.HTTP — the store-reported reuse failure over
/token(exact wire body,cause on the grant hook,
invalid_granton the server hook);/revokeofthe newest refresh token revokes the whole family (introspection inactive,
refresh refused); tokens of a revoked family never reach the
IntrospectionPolicy; the/revoke503 matrix (lookup failures, a familyfailure leaving the access token for the retry, a failed delete covered by
the family, a family-less delete failure, another client's token, an already
revoked family). The #78 and #81 tests are kept unweakened: their seeds go
through
SaveTokenPair, and consumed tokens now come from a real rotation.Red phase — beyond the reproductions above, every new test was run
against the previous behavior: the nine family conformance cases the old
semantics break (issuance into a revoked family accepted, the access token of
a failed rotation persisted, …), every new SQL and Redis test against a port
of the old stores, the eleven grant tests written first against an emulation
of the old flow (two persistence calls, orphan tokens, reuse instead of
ErrTokenFamilyRevoked,server_errorfor a vanished token, no retry, nohook), and the
/revoketests (200 instead of 503, lookup failures neverreported).
Stability — every race-sensitive test passes with
-race -count=30onmemory, SQLite and Redis.
Reviews — two independent adversarial reviews. The store review ran the
conformance suite and stress tests against real PostgreSQL 17, MySQL 9.6,
Redis 8.8 and a multi-connection SQLite, with 36 mutants; the grant/endpoint
review killed 17 mutants. Their findings are applied in the last three
commits: family-first revocation at
/revoke, two newly pinned branches,and the retention requirement in the docs.
Checks
make build— OK.make test— every package passes with-race:oauth2oauth2/grantoauth2/storage/memoryoauth2/storetestoauth2/store/sqloauth2/store/redisWhat remains uncovered cannot be reached on SQLite or miniredis (commit
failures, lock-invariant guards, JSON-encoding errors, defensive
"unexpected script result" branches) or is pre-existing.
make lint— 0 issues.Follow-ups (out of scope)
LIMITATIONS.md);a job with service containers would turn the locking argument into a test.
/revokeof another client's token answers 200 without revoking anything(unchanged).
/introspectstill reads a storage error as an unknown token(
{"active":false}) without reporting it.store).
oauth2/token/generator.go.Notes for the maintainer
CLAUDE.mdis not edited here. Proposed addition to the storage bullet ofthe "OAuth2 server" section: