diff --git a/CHANGELOG.md b/CHANGELOG.md index 891ac4f..d575077 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,9 +63,10 @@ legacy packages (`authentication/`, `authorization/`, the in-tree - **OAuth2 error hook**: `ServerConfig.OnError` and `grant.Config.OnError` (both `oauth2.ErrorHook`) observe every error the authorization server turns into an RFC 6749 §5.2 response — carrying the cause a `server_error` - never puts on the wire — plus the errors it swallows on purpose, such as a - best-effort revocation (RFC 7009 §2.2) or a family revocation that failed - during reuse detection. Purely observational: the responses are unchanged. + never puts on the wire — plus the errors behind an answer that cannot + carry them, such as the storage failure behind a `/revoke` 503 or a family + revocation that failed during reuse detection. Purely observational: the + hook never changes a response. - **OAuth2 profile consistency checks**: `oauth2.ProfileValidator`, an optional `Grant` capability: `oauth2.NewServer` asks every registered grant implementing it whether it can honor the active `Profile`, and @@ -91,6 +92,16 @@ legacy packages (`authentication/`, `authorization/`, the in-tree same bytes as an unknown token; a policy error fails closed the same way and reaches `OnError` as a `server_error`. Optional: without a policy, any authenticated confidential client may introspect any token. +- **OAuth2 token-family primitives**: `oauth2.ErrTokenFamilyRevoked` + (`invalid_grant`) refuses an operation on a revoked token family; + `oauth2.ErrTokenFamilyRevocationFailed`, a plain sentinel that never + reaches the wire, reports a family revocation that could not be recorded + (the family must then be treated as active and the revocation retried); + `oauth2.TokenPair.Validate` rejects a pair no store may persist (a refresh + token outside the access token's family, without a family, or already + consumed); `oauth2.MaxFamilyIDLength` (64 bytes) and + `oauth2.FamilyRevocationRetention` (24h) bound family identifiers and how + long a revoked family is remembered. ### Changed @@ -107,6 +118,28 @@ legacy packages (`authentication/`, `authorization/`, the in-tree default (`ErrMissingExpiry`), aligning with RFC 9068 §2.2 and the fail-closed doctrine. Opt out with `jwtsec.WithOptionalExpiry()` to verify deliberately non-expiring assertions. +- **OAuth2 storage contract (breaking).** `oauth2.Storage` drops + `SaveAccessToken` and `SaveRefreshToken` for `SaveTokenPair`, which + persists a newly issued pair atomically; `RotateRefreshToken` takes the + whole replacement `*oauth2.TokenPair` and persists it atomically with the + consumption of the presented token; `RevokeRefreshFamily` records a family + revoked even before any of its tokens exists, is idempotent, and rejects + an empty or over-long ID with `invalid_request`. Every store keeps one + authoritative state per token family that issuance, rotation and lookups + go through: a token of a revoked family is no longer usable — its access + token lookup fails with `invalid_grant`, its refresh token reads consumed + — and nothing can be issued into it. Access and refresh tokens must share + one backend. `store/sql` adds the `oauth2_token_families` table (`Migrate` + creates it); `store/redis` replaces the `famrt:`/`famat:` sets with one + `fam:{id}` key per family and now requires Redis 6.0 or later + (`SET … KEEPTTL`) and `maxmemory-policy noeviction`. The guarantees are + documented on `oauth2.Storage` and checked by `oauth2/storetest`, which + every implementation must pass. Upgrading invalidates every refresh token + and every family-bound access token issued before it — their family has no + record and the stores fail closed — so users re-authenticate, and + presenting such a refresh token answers `invalid_grant`; + `client_credentials` and implicit tokens keep working. See + [MIGRATION.md](MIGRATION.md). ### Fixed @@ -126,6 +159,48 @@ legacy packages (`authentication/`, `authorization/`, the in-tree access token. The replacement used to inherit the narrowed scope, so the grant shrank permanently and a later request for the original scope was refused with `invalid_scope`. +- The SQL store's schema (`oauth2/store/sql`) runs on PostgreSQL and MySQL: + the refresh-token `consumed` flag is a `SMALLINT` — PostgreSQL rejected + the former `BOOLEAN NOT NULL DEFAULT 0` column and the store's integer + comparisons — and the token-family indexes are gone, since MySQL rejects + their `CREATE INDEX IF NOT EXISTS`. `Schema` returns the same DDL for + every dialect. +- A failed family revocation after a refresh-token reuse the store detects + is no longer swallowed: the SQL and Redis stores returned the bare + `ErrRefreshTokenReused` whatever the revocation did. The error now also + wraps `oauth2.ErrTokenFamilyRevocationFailed` and its cause; the + `refresh_token` grant then retries the revocation and reports a second + failure to `grant.Config.OnError`, while the client still gets + `invalid_grant`. The revocations a reuse triggers now run on a context + detached from the request, so a client hanging up cannot abort them. +- A failed exchange no longer leaves a live access token behind: the + `authorization_code`, `refresh_token` and legacy `password` grants + persisted the access token before minting the refresh token and rotating, + so a later failure left an access token the client never received. Each + exchange now persists its tokens in one atomic call. +- A refresh token that disappears between the grant's lookup and its + rotation — e.g. expired from Redis — is answered `invalid_grant` (RFC 6749 + §5.2) instead of `server_error`. +- `/revoke` no longer answers `200 OK` for a revocation that may not have + taken effect: when a token lookup fails for another reason than an + unknown token, the token's family cannot be revoked, or a token outside + any family cannot be deleted, it answers `503 temporarily_unavailable` + (RFC 7009 §2.2.1), so the client knows to assume the token still exists + and retry. An access token is revoked through its family first, and only + then deleted, so the retry redoes the whole revocation. A failed lookup + used to skip the revocation silently; every storage failure now reaches + `ServerConfig.OnError` once, as a `server_error` with its cause. An + unknown token, or another client's, still answers `200 OK`. +- `RevokeRefreshFamily("")` no longer deletes every access token outside a + family (`client_credentials`, implicit) in the memory and SQL stores; it + is rejected with `invalid_request`. +- A rotation whose replacement declares another family than the presented + token's is refused with `invalid_request`; the Redis store used to revoke + the declared family, not the stored one, on reuse. +- The `error_description` of `oauth2.ErrRefreshTokenReused` is now + `refresh token reused`. The former `refresh token reused — family + revoked` claimed a revocation that may have failed, and its em dash lies + outside the RFC 6749 §5.2 `error_description` charset. ### Removed @@ -157,6 +232,21 @@ legacy packages (`authentication/`, `authorization/`, the in-tree it fails with `server_error` before any token is minted or persisted. Non-rotating refresh tokens remain an explicit `Profile20` choice (`RotateRefreshTokens: false`). +- OAuth2 token-family revocation is race-free and authoritative in every + store (RFC 9700 §4.14.2, RFC 7009 §2.1). Once `RevokeRefreshFamily` + returns — or a reuse is reported without + `ErrTokenFamilyRevocationFailed` — no token of the family is usable + through the store, and no issuance or rotation into the family succeeds, + including one racing the revocation. Before, a rotation or a non-rotating + `Profile20` refresh could leave a usable token in a family whose + revocation had just completed: the SQL statements ran outside a shared + transaction, the Redis revocation enumerated the family non-atomically, + and nothing gated issuance. A revocation also left no trace for a family + without tokens yet. A Redis rotation resent by go-redis after a lost + reply is no longer mistaken for a reuse that revokes the family it just + rotated into. JWT access tokens verified offline stay out of the store's + reach until they expire: see + [docs/security-considerations.md](docs/security-considerations.md). - The OAuth2 `/introspect` endpoint now answers only clients whose `Type()` is `oauth2.ClientConfidential` (RFC 7662 §2.1 / §4, RFC 6749 §2.2, §2.3). It used to answer any client the shared `ClientAuth` chain diff --git a/LIMITATIONS.md b/LIMITATIONS.md index 750c223..d2601d5 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -13,6 +13,23 @@ than future refactor phases. - **`/.well-known/jwks.json` endpoint** — not exposed. JWKS publication depends on a server-side public-key store; the `jwtsec` module already provides the building blocks (`NewStaticJWKS`). +- **Offline JWT access tokens are not revocable** — a token-family + revocation takes effect in the store, not at resource servers verifying + JWT access tokens offline (`jwtsec.BearerVerifier`): the RFC 9068 payload + carries no family or `jti` claim to deny-list, so such a token stays + valid until it expires. Keep their lifetime short, or validate against + the store. See + [docs/security-considerations.md](docs/security-considerations.md#offline-jwt-access-tokens). +- **Redis Cluster** — not supported by `oauth2/store/redis`: its Lua + scripts touch keys of several hash slots. Use a standalone server or a + Sentinel primary. +- **SQL expired-row cleanup** — `oauth2/store/sql` ships no expiry job: + rows stay until an operator deletes those past `expires_at` (see the + package documentation for a safe order). +- **PostgreSQL and MySQL are not exercised in CI** — the SQL store's tests + run on SQLite only; its PostgreSQL and MySQL guarantees (row locks, + READ COMMITTED on PostgreSQL, InnoDB on MySQL) are argued from their + locking semantics, not tested. ## Transports diff --git a/MIGRATION.md b/MIGRATION.md index 7bc3044..c0ba4c3 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -170,3 +170,80 @@ To migrate: caller authenticated as a confidential client, and gets the request context; see the `oauth2.IntrospectionPolicy` godoc for the full contract. + +### `oauth2`: atomic token pairs, authoritative token families (#82) + +Refresh-token reuse detection promised to revoke the whole token family, +but no store could guarantee it: revocation searched and purged the +family's tokens non-atomically, nothing stopped a rotation or an issuance +racing it, a reuse whose revocation failed was reported as a success, and +the grants persisted an access token before the rest of the exchange could +fail. Every store now keeps one authoritative state per token family — +active or revoked — that issuance, rotation and lookups go through, and +writes each token pair in one atomic call. The guarantees are stated on the +`oauth2.Storage` godoc and checked by `oauth2/storetest`. + +| Before | After | +| ------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------- | +| `SaveAccessToken(ctx, at)` | `SaveTokenPair(ctx, &oauth2.TokenPair{Access: *at})` | +| `SaveAccessToken` then `SaveRefreshToken` for one issuance | one `SaveTokenPair(ctx, &oauth2.TokenPair{Access: at, Refresh: &rt})` | +| `SaveAccessToken`, then `RotateRefreshToken(ctx, oldHash, nextRefresh)` | one `RotateRefreshToken(ctx, oldHash, &oauth2.TokenPair{Access: at, Refresh: &rt})` | +| `RevokeRefreshFamily` purges the family's tokens | records the family revoked, even before its first token; `""` and IDs over 64 bytes are refused | +| a token of a revoked family: access token deleted, refresh token consumed | `LookupAccessToken` fails with `invalid_grant` (`ErrTokenFamilyRevoked`); the refresh token reads consumed | +| a reuse returns `ErrRefreshTokenReused` whatever its revocation did | the error also wraps `ErrTokenFamilyRevocationFailed` when the revocation failed | + +To migrate: + +1. **Wiring.** `ServerConfig.Storage` and the `Storage` of every grant MUST + be the same store: access and refresh tokens now share one backend (only + authorization codes may live elsewhere). +2. **Custom stores.** Implement the new methods as the `oauth2.Storage` + godoc describes, refuse what `TokenPair.Validate` rejects — and a + rotation without a refresh token — before any I/O, return the `oauth2` + sentinels as-is or wrapped with `%w` (a `WithDescription` copy no longer + matches `errors.Is`), and pass `storetest.RunConformance`. +3. **Code seeding tokens** (tests, fixtures) calls `SaveTokenPair`; a + consumed refresh token can only come from a real `RotateRefreshToken`. +4. **SQL.** Run `Store.Migrate` (or apply `Schema`) once: it creates the + `oauth2_token_families` table and leaves the existing tables untouched. + The `idx_oauth2_access_family` and `idx_oauth2_refresh_family` indexes + are no longer created or used; dropping them is optional. Schedule the + expired-row cleanup the `store/sql` package documentation describes, and + never delete a family row before its `expires_at`: an issuance into a + family whose revoked row is gone records it active again. +5. **Redis.** Families now live in `fam:{id}` keys (`` being + the `WithKeyPrefix` value, `oauth2:` by default); the store ignores the + former `famrt:*` and `famat:*` sets, which you may delete + with `SCAN`. The store requires Redis 6.0 or later, a standalone server + or a Sentinel primary, reads from the primary only, and + `maxmemory-policy noeviction`: an evicted revoked family can be + recorded active again by an issuance into it. +6. **Tokens issued before the upgrade.** Upgrading invalidates every + outstanding refresh token and every access token bound to a family: + their family has no record, and the stores treat an unknown family as + revoked. Users re-authenticate; expect a one-off burst of `invalid_grant` + ("refresh token reused") answers as clients present their pre-upgrade + refresh tokens. Access tokens outside a family (`client_credentials`, + implicit) keep working until they expire. Flushing the old token rows + or keys is good hygiene: they can no longer be used. + +### `oauth2`: `/revoke` answers 503 when a revocation may have failed (#82) + +`/revoke` answered `200 OK` whatever happened, telling the client its token +could no longer be used (RFC 7009 §2.1) even when the store had failed to +revoke it. It now answers `503 temporarily_unavailable` +(`{"error":"temporarily_unavailable","error_description":"revocation temporarily unavailable"}`) +when a token lookup fails for another reason than an unknown token, when +the token's family cannot be revoked, or when a token outside any family +cannot be deleted. RFC 7009 §2.2.1: the client must then assume the token +still exists, and may retry after a delay — revocation is idempotent, so +retrying is safe. An access token is revoked through its family first and +only then deleted: a failed family revocation leaves it in place for the +retry, while a failed delete after the family was revoked is reported but +answers `200 OK`, the token being unusable through its family. Clients that +ignored the status of `/revoke` should now retry on 503. + +Unknown tokens and other clients' tokens still answer `200 OK`. Every +underlying failure still reaches `ServerConfig.OnError` exactly once, as a +`server_error` carrying its cause (a failed lookup is now reported too, as +`revoke: token lookup failed`); the 503 itself is not reported again. diff --git a/docs/architecture.md b/docs/architecture.md index d1041e2..6205d47 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -157,10 +157,16 @@ The `oauth2` module is an authorization server, not just a provider: `/.well-known/oauth-authorization-server` (RFC 8414). The endpoint path prefix used in the metadata document is configurable via `ServerConfig.RoutePrefix`. -- **`Storage`** — an interface with explicit atomicity contracts - (`ConsumeAuthorizationCode`, `RotateRefreshToken`). Three implementations: - in-memory, SQL (Postgres/MySQL/SQLite), Redis (Lua scripts). All three - pass the shared `oauth2/storetest` conformance suite. +- **`Storage`** — an interface with explicit atomicity contracts: a + single-use `ConsumeAuthorizationCode`, and token pairs persisted + atomically (`SaveTokenPair`, `RotateRefreshToken`) against one + authoritative state per token family, so that a revoked family + (`RevokeRefreshFamily`) can neither be used nor issued into — even before + its first token, and whatever races the revocation. Access and refresh + tokens share one backend; codes may live elsewhere. Three + implementations: in-memory, SQL (Postgres/MySQL/SQLite, row locks), Redis + (Lua scripts). All three pass the shared `oauth2/storetest` conformance + suite, which states the guarantees through the public API. Tokens and authorization codes are **never stored in cleartext** — the store only ever sees a hash. diff --git a/docs/observability.md b/docs/observability.md index baaf155..9fd8ed5 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -32,9 +32,9 @@ through the OAuth2 error hook described below. An RFC 6749 §5.2 response carries a code, a description and a URI — never the cause. A `server_error` therefore reaches the client as an opaque 500, -and RFC 7009 §2.2 goes further: a revocation answers `200 OK` even when the -revocation itself failed. Both are protocol requirements, and both leave an -operator with nothing to diagnose. +and a revocation that may not have taken effect as a bare +`503 temporarily_unavailable` (RFC 7009 §2.2.1). Both are protocol +requirements, and both leave an operator with nothing to diagnose. `oauth2.ServerConfig.OnError` closes that gap. It is an `oauth2.ErrorHook` (`func(ctx context.Context, err error)`) called with the `*oauth2.Error` @@ -59,16 +59,35 @@ The hook fires for: `/introspect`, metadata) and every error redirected back to the client by `/authorize`, including the pre-redirect refusals answered with a bare 400 (unknown client, unregistered `redirect_uri`); -- the best-effort revocations `/revoke` swallows to honour RFC 7009 §2.2; +- every storage failure at `/revoke`: a token lookup that failed for another + reason than an unknown token (`revoke: token lookup failed`), and a failed + family or access-token revocation (`revoke refresh family failed`, + `revoke access token failed`) — one `server_error` per failure, cause + attached. When the failure leaves the revocation unfinished — a failed + lookup, a family that could not be revoked, a token outside any family + that could not be deleted — `/revoke` answers + `503 temporarily_unavailable` (RFC 7009 §2.2.1: the client must assume the + token still exists, and may retry); that 503 is not reported again. An + access token whose family was revoked but whose delete failed answers + `200 OK`: it is unusable through its family. An unknown token, or another + client's, answers `200 OK` and is not reported; - an `IntrospectionPolicy` that failed: `/introspect` withholds the token behind a bare `{"active":false}` and reports a `server_error` (`introspection policy failed`, the policy's error as cause). A plain refusal by the policy is an expected outcome and is not reported. `grant.Config.OnError` is the same hook for the errors a grant swallows -before returning — today, a family revocation that failed during BCP §8.10.3 -reuse detection. Errors a grant *returns* travel to the server and reach -`ServerConfig.OnError`, so wire both fields to the same sink. +before returning — today, a family revocation that failed after a +refresh-token reuse (RFC 9700 §4.14.2), whether the grant found the +presented token consumed or the store reported the reuse while rotating. +The grant retries a revocation the store could not record, on a context +detached from the request, and reports one `server_error` per revocation +that still failed: `revoke refresh family failed after reuse detection`, +whose cause matches `oauth2.ErrTokenFamilyRevocationFailed` and names the +family with a conforming store. It does not match +`oauth2.ErrRefreshTokenReused`: the reuse itself is the `invalid_grant` the +server reports as usual. Errors a grant *returns* travel to the server and +reach `ServerConfig.OnError`, so wire both fields to the same sink. The hook is purely observational: it MUST NOT influence the response, and it runs synchronously on the request goroutine, so keep it fast. It receives no diff --git a/docs/security-considerations.md b/docs/security-considerations.md index eae5f64..e0b0f81 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -54,28 +54,32 @@ configurable clock skew, and resolves keys by `kid` against a JWKS provider every refresh exchange consumes the presented token and issues a new one in the same family (RFC 9700 §4.14.2). The profiles apply it to every client, stricter than RFC 9700 §2.2.2, which requires rotation — or - sender-constrained refresh tokens, which this library does not issue — - for public clients only. Under these profiles — and under `Profile20` - when `RotateRefreshTokens` is set — `NewServer` refuses a - `refresh_token` grant without a `RefreshTokens` generator, and the grant - itself fails with `server_error` before minting anything when it cannot - rotate. Re-use of an already-rotated token is treated as theft: the - whole family is revoked (`ErrRefreshTokenReused`). Clients MUST keep the - refresh token returned by each exchange (RFC 6749 §6) and must not - refresh concurrently with the same token, or they lose the family. Under - `Profile20` rotation is opt-in (`RotateRefreshTokens`); without it a - refresh token stays reusable until it expires, which RFC 9700 §2.2.2 - forbids for public clients. + sender-constrained refresh tokens, which this library does not issue — for + public clients only. Under these profiles — and under `Profile20` when + `RotateRefreshTokens` is set — `NewServer` refuses a `refresh_token` grant + without a `RefreshTokens` generator, and the grant itself fails with + `server_error` before minting anything when it cannot rotate. Re-use of an + already-rotated token is treated as theft: the whole family is revoked + (`ErrRefreshTokenReused`), with the guarantees described under + [Token-family revocation](#token-family-revocation-storage-guarantees). + Clients MUST keep the refresh token returned by each exchange (RFC 6749 + §6) and must not refresh concurrently with the same token, or they lose + the family. Under `Profile20` rotation is opt-in (`RotateRefreshTokens`); + without it a refresh token stays reusable until it expires, which RFC 9700 + §2.2.2 forbids for public clients. - **Token storage** — access tokens, refresh tokens, and authorization codes are stored **hashed only** (SHA-256, via `oauth2.HashToken`). The store never sees cleartext, so a database compromise does not yield usable tokens. Tokens carry ≥ 128 bits of entropy, so the bare hash is preimage- and brute-force-resistant. -- **Atomic single-use** — `ConsumeAuthorizationCode` and - `RotateRefreshToken` are atomic in every `Storage` implementation (SQL - transactions, Redis Lua scripts). Concurrent use of the same code/token - yields exactly one winner; the conformance suite verifies this under - 100-goroutine races. +- **Atomic single-use** — `ConsumeAuthorizationCode` is atomic in every + `Storage` implementation, and so are the token writes: `SaveTokenPair` + and `RotateRefreshToken` persist a whole pair or nothing, gated by the + token family (SQL transactions with row locks, Redis Lua scripts, a mutex + in memory). Concurrent use of the same code or refresh token yields + exactly one winner; the conformance suite checks it with 50 concurrent + consumers of one code and 30 concurrent rotations of one token, and races + revocations against rotation chains and issuance. - **Profiles** — `Profile20BCP` (the default) follows the OAuth 2.0 Security BCP: it refuses the `implicit` grant. `Profile21Draft` additionally refuses the `password` grant. Legacy grants are opt-in and @@ -107,6 +111,97 @@ configurable clock skew, and resolves keys by `kid` against a JWKS provider (`{"active":false}`), so the response discloses nothing (its latency may still differ); a policy failure is reported to `OnError`. +### Token-family revocation (storage guarantees) + +Every access and refresh token issued from one authorization shares a token +family; `client_credentials` and implicit tokens belong to none. Revoking +the family is how the server answers a replayed refresh token (RFC 9700 +§4.14.2, RFC 6749 §10.4) and how `/revoke` honours RFC 7009 §2.1, which asks +the server to invalidate the access tokens of a revoked refresh token's +grant (and allows revoking the refresh tokens of a revoked access token). +Every `Storage` keeps one authoritative state per family — active or +revoked — that issuance, rotation and lookups go through, so a revocation +never has to find the family's tokens: + +- **Complete revocation.** A revocation is complete once + `RevokeRefreshFamily` returned `nil`, or once a reuse was reported without + `oauth2.ErrTokenFamilyRevocationFailed`. From then on no token of the + family is usable through the store — access-token lookups fail with + `invalid_grant`, refresh tokens read consumed — and every issuance or + rotation into the family fails without persisting anything, including one + that raced the revocation: it is ordered either before the revocation, + which then covers its tokens, or after it, and is refused. A lookup + running concurrently with the revocation may still answer "active". +- **Before the first token.** A family can be revoked before any of its + tokens exists; the revocation is kept for at least + `oauth2.FamilyRevocationRetention` (24 hours), and never for less than the + lifetime of the family's tokens, so a late issuance is still refused. +- **Atomicity.** A failed issuance or rotation persists no token and + consumes nothing: a failed exchange no longer leaves an access token + behind. +- **Idempotence and isolation.** Revoking a revoked family changes nothing + and succeeds; a revoked family never becomes active again. Revoking one + family affects no other family and no token outside a family, and the + empty family ID is refused. +- **Failure.** A revocation that cannot be recorded fails with + `oauth2.ErrTokenFamilyRevocationFailed`, and its outcome may be unknown (a + lost acknowledgement): treat the family as active and retry, which + idempotence makes safe. `/revoke` then answers + `503 temporarily_unavailable` — RFC 7009 §2.2.1: the client must assume + the token still exists and may retry — since a `200` would tell the client + the token can no longer be used (RFC 7009 §2.1). It revokes an access + token's family before deleting the token, so a failed family revocation + leaves everything in place for that retry. On the reuse path the store + reports `ErrRefreshTokenReused` joined with the revocation failure, the + `refresh_token` grant retries the revocation on a context detached from + the request, and a second failure reaches `grant.Config.OnError`; the + client gets `invalid_grant` either way. +- **Fail closed.** A token whose family state is missing — expired, + evicted, deleted by a premature cleanup, or never written by a build + predating token families — is treated as revoked. This protects lookups + only: the next issuance into a family whose revoked state was lost records + it active again and revives its tokens, so a revoked family's state must + never be lost within its retention — no eviction, no early cleanup. +- **Lost responses look like theft.** When the response to a successful + rotation is lost, the client retries with the presented token, which is + now a reuse: the family is revoked and the user re-authenticates. This is + inherent to rotation; there is no grace period. A Redis rotation resent + by the client library after a lost reply is recognized, though, and does + not count as a reuse. + +These guarantees rest on deployment assumptions, detailed in the package +documentation of [`store/sql`](../oauth2/store/sql/doc.go) and +[`store/redis`](../oauth2/store/redis/doc.go): every read reaches the +primary (no replica reads, no client-side caching), the store's PostgreSQL +token-family transactions run at READ COMMITTED (it pins the level) and +MySQL tables use InnoDB, Redis is a standalone server or a Sentinel primary +(not Cluster) running 6.0 or later with `maxmemory-policy noeviction`, no +family row or key is deleted before it expires, and durability is the +backend's: a revocation acknowledged by a primary that fails before +persisting or replicating it can be lost in a failover. Only SQLite is +exercised in CI; the PostgreSQL and MySQL guarantees rest on their locking +semantics. + +### Offline JWT access tokens + +The store's revocation reaches every token looked up in the store — +`/introspect`, a resource server calling `LookupAccessToken` — but not a +resource server that verifies JWT access tokens offline, from their +signature and claims (`jwtsec.BearerVerifier`): such a token stays valid +until its `exp`, whatever happened to its family. The RFC 9068 payload the +`jwtsec` signer emits carries no family or token identifier (`jti`), so it +cannot be deny-listed today either. RFC 7009 §2.1 acknowledges such a +propagation delay and asks implementations to minimize it; its §3 leaves +immediate revocation of self-contained tokens to non-standardized backend +interaction, and suggests short-lived access tokens instead. To bound the +window: + +- keep `grant.Config.AccessTTL` short when access tokens are JWTs; +- for high-value operations, validate the token against the authorization + server — RFC 7662 introspection, or `LookupAccessToken` when the resource + server shares the store — rather than offline; +- or issue opaque access tokens, which only the store can validate. + ## Sessions `session` issues a **stateless encrypted cookie** — there is no server-side @@ -152,6 +247,17 @@ span catalog. - [ ] Register the resource servers calling `/introspect` as confidential clients, and set `ServerConfig.IntrospectionPolicy` when several resource servers or tenants share the authorization server. +- [ ] Give `ServerConfig.Storage` and every grant the same store, and wire + `ServerConfig.OnError` and `grant.Config.OnError` to the same sink: + failed family revocations are reported there. +- [ ] With `store/sql`, periodically delete rows past `expires_at` — never + a family row before it — in the deadlock-free order the package + documentation gives; with `store/redis`, run Redis 6.0+ standalone or + behind Sentinel with `maxmemory-policy noeviction`, and read from the + primary only. A revoked family whose state is lost before it expires + can be revived. +- [ ] Keep JWT access tokens short-lived: a store revocation does not + reach resource servers that verify them offline. - [ ] Serve over HTTPS so `Secure` cookies and bearer tokens are protected. - [ ] Supply at least two session keys so rotation is possible without invalidating live sessions. diff --git a/oauth2/authorize_endpoint.go b/oauth2/authorize_endpoint.go index 4331e7a..0f55706 100644 --- a/oauth2/authorize_endpoint.go +++ b/oauth2/authorize_endpoint.go @@ -427,7 +427,8 @@ func (s *Server) issueImplicitToken( ExpiresAt: now.Add(cfg.ImplicitTTL), } - if err := s.cfg.Storage.SaveAccessToken(r.Context(), at); err != nil { + // An implicit token belongs to no family: it is never gated by one. + if err := s.cfg.Storage.SaveTokenPair(r.Context(), &TokenPair{Access: *at}); err != nil { s.redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrServerError.WithCause(err), true) return diff --git a/oauth2/doc.go b/oauth2/doc.go index 98a3e19..ab339c0 100644 --- a/oauth2/doc.go +++ b/oauth2/doc.go @@ -25,12 +25,17 @@ // - Tokens are opaque by default; refresh tokens and authorization codes // are stored hashed. JWT access tokens are available via an adapter to // the jwt sub-module (no hard dependency from oauth2 to jwt). -// - Stores expose atomic ConsumeAuthorizationCode and RotateRefreshToken -// to guarantee single-use semantics and reuse-detection. +// - Stores expose an atomic ConsumeAuthorizationCode (single use), and +// persist every token pair atomically — SaveTokenPair, and +// RotateRefreshToken with reuse detection — against one authoritative +// state per token family: once revoked, no token of the family is +// usable and none can be issued into it (see Storage). // - ServerConfig.OnError (and grant.Config.OnError) observe the errors the // protocol hides: the cause of a server_error, which never reaches the -// wire, the revocations RFC 7009 §2.2 requires to answer 200 OK -// regardless, and a failed IntrospectionPolicy. See ErrorHook. +// wire, the storage failures of /revoke — an unfinished revocation +// answers a bare 503 (RFC 7009 §2.2.1) — a family revocation that +// failed after a refresh-token reuse, and a failed IntrospectionPolicy. +// See ErrorHook. // // Allowed dependencies: // - github.com/hyperscale-stack/security (core) diff --git a/oauth2/errors.go b/oauth2/errors.go index 3e917f8..e9222f5 100644 --- a/oauth2/errors.go +++ b/oauth2/errors.go @@ -98,11 +98,26 @@ var ( // invalid_grant per RFC 6749 §4.1.2. ErrCodeAlreadyUsed = newCoded(CodeInvalidGrant, "authorization code already consumed", security.ErrInvalidCredentials) // ErrRefreshTokenReused signals refresh-token reuse — surfaced as - // invalid_grant per OAuth 2.0 BCP §8.10.3. Storage implementations - // MUST also revoke the entire token family when this occurs. - ErrRefreshTokenReused = newCoded(CodeInvalidGrant, "refresh token reused — family revoked", security.ErrInvalidCredentials) + // invalid_grant per RFC 9700 §4.14.2. Storage implementations MUST also + // revoke the entire token family when this occurs; when that revocation + // fails, the error also wraps [ErrTokenFamilyRevocationFailed]. + ErrRefreshTokenReused = newCoded(CodeInvalidGrant, "refresh token reused", security.ErrInvalidCredentials) + // ErrTokenFamilyRevoked signals an operation on a token family that has + // been revoked — surfaced as invalid_grant (RFC 6749 §5.2: the grant + // was revoked). + ErrTokenFamilyRevoked = newCoded(CodeInvalidGrant, "token family revoked", security.ErrInvalidCredentials) ) +// ErrTokenFamilyRevocationFailed reports that a token family could not be +// recorded as revoked: the family MUST be treated as still active, and the +// revocation retried (it is idempotent). It is a plain sentinel, not an +// [*Error], so it never reaches the wire on its own — a caller that turns +// it into a response answers server_error. Stores wrap it, with the +// family ID and the backend cause, in the error RevokeRefreshFamily +// returns, and after [ErrRefreshTokenReused] when the revocation a reuse +// triggers fails; match it with errors.Is. +var ErrTokenFamilyRevocationFailed = errors.New("oauth2: token family revocation failed") + // newCoded constructs an Error sentinel. The cause chain reaches the supplied // security sentinel via Unwrap so errors.Is keeps working transparently. func newCoded(code, desc string, cause error) *Error { diff --git a/oauth2/family_test.go b/oauth2/family_test.go new file mode 100644 index 0000000..34d27d3 --- /dev/null +++ b/oauth2/family_test.go @@ -0,0 +1,294 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package oauth2_test + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + "github.com/hyperscale-stack/security" + "github.com/hyperscale-stack/security/oauth2" + "github.com/hyperscale-stack/security/oauth2/grant" + "github.com/hyperscale-stack/security/oauth2/storage/memory" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newClientPair returns a pair issued to the test client in family: +// refresh token raw and access token raw+"-access", for alice with scope +// api:read, both valid for an hour. +func newClientPair(raw, family string) *oauth2.TokenPair { + now := time.Now() + + return &oauth2.TokenPair{ + Access: oauth2.AccessToken{ + Token: raw + "-access", TokenHash: oauth2.HashToken(nil, raw+"-access"), + ClientID: testClientID, Subject: "alice", Scope: "api:read", Audience: "api", + FamilyID: family, IssuedAt: now, ExpiresAt: now.Add(time.Hour), + }, + Refresh: &oauth2.RefreshToken{ + Token: raw, TokenHash: oauth2.HashToken(nil, raw), + ClientID: testClientID, Subject: "alice", Scope: "api:read", + FamilyID: family, IssuedAt: now, ExpiresAt: now.Add(time.Hour), + }, + } +} + +// seedPair persists pair straight into store, the way a grant issues it, +// and returns it. +func seedPair(t *testing.T, store oauth2.Storage, pair *oauth2.TokenPair) *oauth2.TokenPair { + t.Helper() + + require.NoError(t, store.SaveTokenPair(t.Context(), pair)) + + return pair +} + +// seedRotated persists pair, then rotates its refresh token into next: the +// only way a consumed refresh token comes to exist. +func seedRotated(t *testing.T, store oauth2.Storage, pair, next *oauth2.TokenPair) { + t.Helper() + + seedPair(t, store, pair) + require.NoError(t, store.RotateRefreshToken(t.Context(), pair.Refresh.TokenHash, next)) +} + +func TestTokenPairValidate(t *testing.T) { + t.Parallel() + + access := func(family string) oauth2.AccessToken { + return oauth2.AccessToken{TokenHash: "at-hash", ClientID: "c", FamilyID: family} + } + refresh := func(family string) *oauth2.RefreshToken { + return &oauth2.RefreshToken{TokenHash: "rt-hash", ClientID: "c", FamilyID: family} + } + + maxFamily := strings.Repeat("f", oauth2.MaxFamilyIDLength) + tooLong := maxFamily + "f" + + cases := []struct { + name string + pair *oauth2.TokenPair + wantErr bool + }{ + {"nil pair", nil, true}, + {"access token without hash", &oauth2.TokenPair{Access: oauth2.AccessToken{FamilyID: "F"}}, true}, + {"access only, no family", &oauth2.TokenPair{Access: access("")}, false}, + {"access only, in a family", &oauth2.TokenPair{Access: access("F")}, false}, + {"access and refresh in one family", &oauth2.TokenPair{Access: access("F"), Refresh: refresh("F")}, false}, + {"family id at the length limit", &oauth2.TokenPair{Access: access(maxFamily), Refresh: refresh(maxFamily)}, false}, + {"family id over the length limit", &oauth2.TokenPair{Access: access(tooLong)}, true}, + {"refresh token without hash", &oauth2.TokenPair{ + Access: access("F"), Refresh: &oauth2.RefreshToken{FamilyID: "F"}, + }, true}, + {"refresh token without family", &oauth2.TokenPair{Access: access(""), Refresh: refresh("")}, true}, + {"refresh token in another family", &oauth2.TokenPair{Access: access("F"), Refresh: refresh("G")}, true}, + {"refresh token already consumed", &oauth2.TokenPair{ + Access: access("F"), + Refresh: &oauth2.RefreshToken{ + TokenHash: "rt-hash", FamilyID: "F", Consumed: true, ExpiresAt: time.Now().Add(time.Hour), + }, + }, true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + err := tc.pair.Validate() + if !tc.wantErr { + assert.NoError(t, err) + + return + } + + require.Error(t, err) + assert.Equal(t, oauth2.CodeInvalidRequest, oauth2.IsCode(err)) + assert.ErrorIs(t, err, security.ErrInvalidCredentials) + + var oe *oauth2.Error + require.ErrorAs(t, err, &oe) + assert.Regexp(t, descriptionCharset, oe.Description) + assert.True(t, strings.HasPrefix(oe.Description, "storage: "), oe.Description) + }) + } +} + +func TestFamilySentinels(t *testing.T) { + t.Parallel() + + t.Run("revoked family is invalid_grant", func(t *testing.T) { + t.Parallel() + + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(oauth2.ErrTokenFamilyRevoked)) + assert.Equal(t, http.StatusBadRequest, oauth2.ErrTokenFamilyRevoked.HTTPStatus()) + assert.ErrorIs(t, oauth2.ErrTokenFamilyRevoked, security.ErrInvalidCredentials) + assert.Equal(t, "token family revoked", oauth2.ErrTokenFamilyRevoked.Description) + }) + + t.Run("revocation failure never reaches the wire", func(t *testing.T) { + t.Parallel() + + // A plain sentinel, not an *Error: it carries no OAuth2 code, so a + // response writer can only ever turn it into a server_error. + assert.Empty(t, oauth2.IsCode(oauth2.ErrTokenFamilyRevocationFailed)) + assert.EqualError(t, oauth2.ErrTokenFamilyRevocationFailed, "oauth2: token family revocation failed") + }) + + t.Run("reuse no longer claims the family was revoked", func(t *testing.T) { + t.Parallel() + + // RFC 6749 §5.2 limits error_description to %x20-21 / %x23-5B / + // %x5D-7E: the former em dash was outside that charset. + assert.Equal(t, "refresh token reused", oauth2.ErrRefreshTokenReused.Description) + assert.Regexp(t, descriptionCharset, oauth2.ErrRefreshTokenReused.Description) + assert.Regexp(t, descriptionCharset, oauth2.ErrTokenFamilyRevoked.Description) + }) + + t.Run("a failed reuse revocation matches both sentinels", func(t *testing.T) { + t.Parallel() + + // The shape a store returns when it detects reuse but cannot + // revoke the family: reuse first, so the OAuth2 code stays + // invalid_grant, with the revocation failure and its cause after. + cause := errors.New("backend down") + err := fmt.Errorf("%w: %w", oauth2.ErrRefreshTokenReused, + fmt.Errorf("store: revoke family %q: %w: %w", "F", oauth2.ErrTokenFamilyRevocationFailed, cause)) + + assert.ErrorIs(t, err, oauth2.ErrRefreshTokenReused) + assert.ErrorIs(t, err, oauth2.ErrTokenFamilyRevocationFailed) + assert.ErrorIs(t, err, cause) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + }) + + t.Run("retention outlives a revocation recorded before issuance", func(t *testing.T) { + t.Parallel() + + assert.Equal(t, 24*time.Hour, oauth2.FamilyRevocationRetention) + assert.Equal(t, 64, oauth2.MaxFamilyIDLength) + }) +} + +// familyRevocationFailure is the error a conforming store returns when it +// cannot record family as revoked. +func familyRevocationFailure(family string) error { + return fmt.Errorf("store: revoke family %q: %w: %w", + family, oauth2.ErrTokenFamilyRevocationFailed, errors.New("backend down")) +} + +// reuseFailingStore reports, on every rotation, a reuse the store detected +// but could not answer with a family revocation, and fails every +// revocation: the grant's retry fails too. +type reuseFailingStore struct{ *memory.Store } + +func (s *reuseFailingStore) RotateRefreshToken(_ context.Context, _ string, next *oauth2.TokenPair) error { + return fmt.Errorf("%w: %w", oauth2.ErrRefreshTokenReused, familyRevocationFailure(next.Access.FamilyID)) +} + +func (s *reuseFailingStore) RevokeRefreshFamily(_ context.Context, familyID string) error { + return familyRevocationFailure(familyID) +} + +func TestOnErrorReportsStoreReuseRevocationFailure(t *testing.T) { + t.Parallel() + + grantSink, serverSink := &errorSink{}, &errorSink{} + store := &reuseFailingStore{Store: memory.New()} + seedServerRefresh(t, store, "store-reuse-rt") + + gcfg := nonRotatingConfig(store) // Profile20BCP rotates all the same + gcfg.OnError = grantSink.hook + + cfg := refreshServerConfig(oauth2.Profile20BCP, store, grant.NewRefreshToken(gcfg)) + cfg.OnError = serverSink.hook + + srv, err := oauth2.NewServer(cfg) + require.NoError(t, err) + + rec := refreshExchange(t, srv, "store-reuse-rt") + + // The wire carries the reuse refusal, nothing of the failed revocation... + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.JSONEq(t, `{"error":"invalid_grant","error_description":"refresh token reused"}`, rec.Body.String()) + assert.NotContains(t, rec.Body.String(), "backend down") + + // ...the grant hook gets one server_error carrying the cause... + got := grantSink.collected() + require.Len(t, got, 1) + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(got[0])) + assert.ErrorIs(t, got[0], oauth2.ErrTokenFamilyRevocationFailed) + assert.ErrorContains(t, errors.Unwrap(got[0]), `revoke family "family-rot"`) + assert.ErrorContains(t, errors.Unwrap(got[0]), "backend down") + + // ...and the server hook the invalid_grant it answered. + assert.Equal(t, []string{oauth2.CodeInvalidGrant}, serverSink.codes()) +} + +func TestRevokeEndpointRevokesWholeFamily(t *testing.T) { + t.Parallel() + + srv, store := newTestServer(t) + seedPair(t, store, newClientPair("whole-rt", "fam-whole")) + + rec := refreshExchange(t, srv, "whole-rt") + require.Equal(t, http.StatusOK, rec.Code) + + var issued struct { + AccessToken string `json:"access_token"` + RefreshToken string `json:"refresh_token"` + } + + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &issued)) + require.NotEmpty(t, issued.RefreshToken) + + rec = httptest.NewRecorder() + srv.RevokeHandler().ServeHTTP(rec, formRequest("/oauth2/revoke", url.Values{"token": {issued.RefreshToken}}, true)) + require.Equal(t, http.StatusOK, rec.Code) + + // Revoking the newest refresh token revokes the whole family: the + // access tokens issued before it and alongside it, and the refresh + // token itself. + for _, raw := range []string{"whole-rt-access", issued.AccessToken, issued.RefreshToken} { + assert.False(t, introspect(t, srv, raw)["active"].(bool), raw) + } + + rec = refreshExchange(t, srv, issued.RefreshToken) + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Equal(t, oauth2.CodeInvalidGrant, decodeError(t, rec)) +} + +func TestIntrospectionPolicyNeverSeesRevokedFamily(t *testing.T) { + t.Parallel() + + store := memory.New() + probe := &policyProbe{allow: true} + + cfg := hookServerConfig(store) + cfg.IntrospectionPolicy = probe.decide + + srv, err := oauth2.NewServer(cfg) + require.NoError(t, err) + + seedPair(t, store, newClientPair("policy-revoked", "fam-policy-revoked")) + require.NoError(t, store.RevokeRefreshFamily(t.Context(), "fam-policy-revoked")) + + for _, raw := range []string{"policy-revoked", "policy-revoked-access"} { + assert.False(t, introspect(t, srv, raw)["active"].(bool), raw) + } + + assert.Zero(t, probe.calls.Load(), "a token of a revoked family is inactive before any policy runs") + + seedPair(t, store, newClientPair("policy-live", "fam-policy-live")) + assert.True(t, introspect(t, srv, "policy-live-access")["active"].(bool)) + assert.Equal(t, int32(1), probe.calls.Load(), "an active token still goes through the policy") +} diff --git a/oauth2/grant/client_credentials.go b/oauth2/grant/client_credentials.go index 39c1dde..ee23b4c 100644 --- a/oauth2/grant/client_credentials.go +++ b/oauth2/grant/client_credentials.go @@ -66,7 +66,8 @@ func (g *ClientCredentials) Handle(ctx context.Context, req Request) (*Response, Token: atRaw, TokenHash: atHash, ClientID: req.Client.ID(), Subject: req.Client.ID(), Scope: scope, IssuedAt: req.Now, ExpiresAt: expires, Audience: req.Audience, } - if err := g.cfg.Storage.SaveAccessToken(ctx, access); err != nil { + // RFC 6749 §4.4.3: no refresh token, and a token outside any family. + if err := g.cfg.Storage.SaveTokenPair(ctx, &oauth2.TokenPair{Access: *access}); err != nil { return nil, oauth2.ErrServerError.WithCause(err) } diff --git a/oauth2/grant/family_test.go b/oauth2/grant/family_test.go new file mode 100644 index 0000000..e10ee44 --- /dev/null +++ b/oauth2/grant/family_test.go @@ -0,0 +1,579 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package grant_test + +import ( + "context" + "errors" + "fmt" + "net/url" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" + "github.com/hyperscale-stack/security/oauth2/grant" + "github.com/hyperscale-stack/security/oauth2/storage/memory" + "github.com/hyperscale-stack/security/oauth2/token" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// errBackendDown is the backend failure the token-family tests inject. +var errBackendDown = errors.New("backend down") + +// clientPair returns a pair issued to the test client in family: refresh +// token raw and access token raw+"-access", both valid until expiresAt. +func clientPair(raw, family, scope string, expiresAt time.Time) *oauth2.TokenPair { + issuedAt := time.Now().Add(-time.Hour) + + return &oauth2.TokenPair{ + Access: oauth2.AccessToken{ + Token: raw + "-access", TokenHash: oauth2.HashToken(nil, raw+"-access"), + ClientID: clientID, Subject: subject, Scope: scope, Audience: "api", + FamilyID: family, IssuedAt: issuedAt, ExpiresAt: expiresAt, + }, + Refresh: &oauth2.RefreshToken{ + Token: raw, TokenHash: oauth2.HashToken(nil, raw), + ClientID: clientID, Subject: subject, Scope: scope, + FamilyID: family, IssuedAt: issuedAt, ExpiresAt: expiresAt, + }, + } +} + +// seedPair persists clientPair(raw, family, scope, expiresAt) straight into +// store, the way a grant issues it, and returns it. +func seedPair(t *testing.T, store oauth2.Storage, raw, family, scope string, expiresAt time.Time) *oauth2.TokenPair { + t.Helper() + + pair := clientPair(raw, family, scope, expiresAt) + require.NoError(t, store.SaveTokenPair(t.Context(), pair)) + + return pair +} + +// seedRotated seeds a pair in family and rotates its refresh token raw, so +// that raw is consumed the only way a refresh token can be: by a real +// rotation. It returns the pair the rotation issued, still active. +func seedRotated(t *testing.T, store oauth2.Storage, raw, family string) *oauth2.TokenPair { + t.Helper() + + expiresAt := time.Now().Add(time.Hour) + seedPair(t, store, raw, family, "read:mail", expiresAt) + + next := clientPair(raw+"-next", family, "read:mail", expiresAt) + require.NoError(t, store.RotateRefreshToken(t.Context(), oauth2.HashToken(nil, raw), next)) + + return next +} + +// persistCall is one persistence call a grant made on a familyStore. +type persistCall struct { + kind string // "save" or "rotate" + oldHash string // the rotated refresh token, for "rotate" + pair oauth2.TokenPair +} + +// familyStore decorates the memory store for the token-family grant tests: +// it records the grant's persistence calls, runs afterLookup once right +// after the grant's refresh-token lookup — a competing request landing at +// that exact point — and can force the result of a rotation or a family +// revocation. Seed through the embedded Store, so seeding is not recorded. +type familyStore struct { + *memory.Store + + afterLookup func(*memory.Store) + lookup *oauth2.RefreshToken // served instead of the stored token + saveErr error + rotateErr error + revokeErr error + + calls []persistCall + revokes []revokeCall +} + +// revokeCall records the context a family revocation ran on, as it was +// when the store received the call. +type revokeCall struct { + ctxErr error + hasDeadline bool +} + +func (s *familyStore) LookupRefreshToken(ctx context.Context, hash string) (*oauth2.RefreshToken, error) { + if s.lookup != nil { + rt := *s.lookup + + return &rt, nil + } + + rt, err := s.Store.LookupRefreshToken(ctx, hash) + + if fn := s.afterLookup; fn != nil { + s.afterLookup = nil + fn(s.Store) + } + + return rt, err +} + +func (s *familyStore) SaveTokenPair(ctx context.Context, pair *oauth2.TokenPair) error { + s.calls = append(s.calls, persistCall{kind: "save", pair: *pair}) + + if s.saveErr != nil { + return s.saveErr + } + + return s.Store.SaveTokenPair(ctx, pair) +} + +func (s *familyStore) RotateRefreshToken(ctx context.Context, oldHash string, next *oauth2.TokenPair) error { + s.calls = append(s.calls, persistCall{kind: "rotate", oldHash: oldHash, pair: *next}) + + if s.rotateErr != nil { + return s.rotateErr + } + + return s.Store.RotateRefreshToken(ctx, oldHash, next) +} + +func (s *familyStore) RevokeRefreshFamily(ctx context.Context, familyID string) error { + _, hasDeadline := ctx.Deadline() + s.revokes = append(s.revokes, revokeCall{ctxErr: ctx.Err(), hasDeadline: hasDeadline}) + + if s.revokeErr != nil { + return s.revokeErr + } + + return s.Store.RevokeRefreshFamily(ctx, familyID) +} + +// revocationFailure is the error a conforming store returns when it cannot +// record family as revoked. +func revocationFailure(family string) error { + return fmt.Errorf("store: revoke family %q: %w: %w", family, oauth2.ErrTokenFamilyRevocationFailed, errBackendDown) +} + +// recordingAccessGen mints opaque access tokens and records their hashes, +// so a test can look a token up even when the grant never returned it. +type recordingAccessGen struct{ hashes []string } + +func (g *recordingAccessGen) Generate(ctx context.Context, c token.AccessTokenClaims) (string, string, error) { + raw, hash, err := newAccessGen().Generate(ctx, c) + g.hashes = append(g.hashes, hash) + + return raw, hash, err +} + +// failingAccessGen is an access-token generator that always fails. +type failingAccessGen struct{} + +func (failingAccessGen) Generate(context.Context, token.AccessTokenClaims) (string, string, error) { + return "", "", errBackendDown +} + +// failingRefreshGen is a refresh-token generator that always fails. +type failingRefreshGen struct{} + +func (failingRefreshGen) Generate(context.Context) (string, string, error) { + return "", "", errBackendDown +} + +// hookRecorder collects what a grant reports to its error hook. +type hookRecorder struct{ errs []error } + +func (h *hookRecorder) hook(_ context.Context, err error) { h.errs = append(h.errs, err) } + +// rotatingConfig is a rotating refresh_token grant configuration over store. +func rotatingConfig(store oauth2.Storage, hooks *hookRecorder) grant.Config { + return grant.Config{ + Storage: store, AccessTokens: newAccessGen(), RefreshTokens: newRefreshGen(), + AccessTTL: time.Hour, RefreshTTL: 24 * time.Hour, OnError: hooks.hook, + } +} + +// requireNoAccessToken asserts that no access token hash is usable. +func requireNoAccessToken(t *testing.T, store oauth2.Storage, hashes ...string) { + t.Helper() + + for _, hash := range hashes { + _, err := store.LookupAccessToken(t.Context(), hash) + require.Error(t, err, "access token %s must not be usable", hash) + } +} + +func TestRefreshRotationFailureLeavesNoAccessToken(t *testing.T) { + t.Parallel() + + store := &familyStore{Store: memory.New(), rotateErr: errBackendDown} + seedPair(t, store.Store, "fail-rotate", "fam-fail", "read:mail", time.Now().Add(time.Hour)) + + resp, err := grant.NewRefreshToken(rotatingConfig(store, &hookRecorder{})). + Handle(t.Context(), refreshReq("fail-rotate", oauth2.Profile20BCP)) + assert.Nil(t, resp) + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(err)) + assert.NotContains(t, err.Error(), "backend down", "the cause stays off the wire") + + // One persistence call — the atomic rotation — so its failure leaves + // nothing behind: no access token is persisted before it any more. + require.Len(t, store.calls, 1) + call := store.calls[0] + assert.Equal(t, "rotate", call.kind) + assert.Equal(t, oauth2.HashToken(nil, "fail-rotate"), call.oldHash) + require.NotNil(t, call.pair.Refresh) + assert.Equal(t, "fam-fail", call.pair.Access.FamilyID) + assert.Equal(t, "fam-fail", call.pair.Refresh.FamilyID) + + requireNoAccessToken(t, store, call.pair.Access.TokenHash) + + presented, err := store.LookupRefreshToken(t.Context(), oauth2.HashToken(nil, "fail-rotate")) + require.NoError(t, err) + assert.False(t, presented.Consumed, "the failed rotation consumed nothing") +} + +func TestRefreshGeneratorFailureLeavesNoAccessToken(t *testing.T) { + t.Parallel() + + store := &familyStore{Store: memory.New()} + seedPair(t, store.Store, "fail-gen", "fam-gen", "read:mail", time.Now().Add(time.Hour)) + + access := &recordingAccessGen{} + cfg := rotatingConfig(store, &hookRecorder{}) + cfg.AccessTokens = access + cfg.RefreshTokens = failingRefreshGen{} + + resp, err := grant.NewRefreshToken(cfg).Handle(t.Context(), refreshReq("fail-gen", oauth2.Profile20BCP)) + assert.Nil(t, resp) + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(err)) + + require.Len(t, access.hashes, 1, "the access token was minted") + assert.Empty(t, store.calls, "nothing is persisted before the pair is complete") + requireNoAccessToken(t, store, access.hashes...) +} + +func TestAuthorizationCodeGeneratorFailureLeavesNoAccessToken(t *testing.T) { + t.Parallel() + + store := &familyStore{Store: memory.New()} + _, req := newAuthCodeReq(t.Context(), store.Store, true) + + access := &recordingAccessGen{} + g := grant.NewAuthorizationCode(grant.Config{ + Storage: store, AccessTokens: access, RefreshTokens: failingRefreshGen{}, + AccessTTL: time.Hour, RefreshTTL: time.Hour, + }) + + resp, err := g.Handle(t.Context(), req) + assert.Nil(t, resp) + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(err)) + + require.Len(t, access.hashes, 1, "the access token was minted") + assert.Empty(t, store.calls, "nothing is persisted before the pair is complete") + requireNoAccessToken(t, store, access.hashes...) +} + +func TestAuthorizationCodeIssuesOnePairAtomically(t *testing.T) { + t.Parallel() + + store := &familyStore{Store: memory.New()} + _, req := newAuthCodeReq(t.Context(), store.Store, true) + g := grant.NewAuthorizationCode(grant.Config{ + Storage: store, AccessTokens: newAccessGen(), RefreshTokens: newRefreshGen(), + AccessTTL: time.Hour, RefreshTTL: time.Hour, + }) + + resp, err := g.Handle(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, resp.Pair.Refresh) + + require.Len(t, store.calls, 1) + assert.Equal(t, "save", store.calls[0].kind) + assert.Equal(t, resp.Pair, store.calls[0].pair) + assert.NotEmpty(t, resp.Pair.Access.FamilyID) + assert.Equal(t, resp.Pair.Access.FamilyID, resp.Pair.Refresh.FamilyID, "both tokens open one family") +} + +// TestRefreshStoreReuseRevocationFailureIsReported covers the reuse the +// store detects while rotating — the token was consumed between the +// grant's lookup and its rotation — when the store could not revoke the +// family: the grant retries the revocation, and its failure is reported +// exactly once. +func TestRefreshStoreReuseRevocationFailureIsReported(t *testing.T) { + t.Parallel() + + store := &familyStore{ + Store: memory.New(), + rotateErr: fmt.Errorf("%w: %w", oauth2.ErrRefreshTokenReused, revocationFailure("fam-reuse")), + revokeErr: revocationFailure("fam-reuse"), + } + seedPair(t, store.Store, "store-reuse", "fam-reuse", "read:mail", time.Now().Add(time.Hour)) + + hooks := &hookRecorder{} + + resp, err := grant.NewRefreshToken(rotatingConfig(store, hooks)). + Handle(t.Context(), refreshReq("store-reuse", oauth2.Profile20BCP)) + assert.Nil(t, resp) + assert.Same(t, oauth2.ErrRefreshTokenReused, err, "the client gets the bare reuse refusal") + + require.Len(t, store.revokes, 1, "the grant retried the revocation") + require.Len(t, hooks.errs, 1, "one report per failed revocation") + + got := hooks.errs[0] + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(got)) + assert.ErrorContains(t, got, "revoke refresh family failed after reuse detection") + assert.ErrorIs(t, got, oauth2.ErrTokenFamilyRevocationFailed) + assert.ErrorContains(t, errors.Unwrap(got), `revoke family "fam-reuse"`) + assert.ErrorContains(t, errors.Unwrap(got), "backend down") + assert.NotErrorIs(t, got, oauth2.ErrRefreshTokenReused, "the report counts the revocation, not the reuse") +} + +func TestRefreshStoreReuseRevocationRetried(t *testing.T) { + t.Parallel() + + store := &familyStore{ + Store: memory.New(), + rotateErr: fmt.Errorf("%w: %w", oauth2.ErrRefreshTokenReused, revocationFailure("fam-retry")), + } + seed := seedPair(t, store.Store, "store-retry", "fam-retry", "read:mail", time.Now().Add(time.Hour)) + + hooks := &hookRecorder{} + + _, err := grant.NewRefreshToken(rotatingConfig(store, hooks)). + Handle(t.Context(), refreshReq("store-retry", oauth2.Profile20BCP)) + assert.Same(t, oauth2.ErrRefreshTokenReused, err) + + // The store's revocation failed, the grant's retry succeeded: the + // family is revoked and there is nothing to report. + assert.Len(t, store.revokes, 1) + assert.Empty(t, hooks.errs) + requireNoAccessToken(t, store, seed.Access.TokenHash) +} + +// TestReuseRevocationDetachedFromRequest replays a consumed token on a +// request whose context is already canceled — a client that hung up: the +// revocation its replay triggers still runs, on a live context with a +// deadline. +func TestReuseRevocationDetachedFromRequest(t *testing.T) { + t.Parallel() + + store := &familyStore{Store: memory.New()} + next := seedRotated(t, store.Store, "detached", "fam-detached") + hooks := &hookRecorder{} + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + _, err := grant.NewRefreshToken(rotatingConfig(store, hooks)).Handle(ctx, refreshReq("detached", oauth2.Profile20BCP)) + assert.Same(t, oauth2.ErrRefreshTokenReused, err) + + require.Len(t, store.revokes, 1) + assert.NoError(t, store.revokes[0].ctxErr, "the revocation is detached from the request's cancellation") + assert.True(t, store.revokes[0].hasDeadline, "the detached revocation is bounded") + assert.Empty(t, hooks.errs) + + requireNoAccessToken(t, store, next.Access.TokenHash) +} + +// TestRefreshReusedBetweenLookupAndRotation lets a competing exchange of the +// same token land between the grant's lookup and its rotation: the store +// detects the reuse and revokes the family, the grant refuses quietly. +func TestRefreshReusedBetweenLookupAndRotation(t *testing.T) { + t.Parallel() + + var attacker *oauth2.TokenPair + + store := &familyStore{Store: memory.New()} + seedPair(t, store.Store, "raced", "fam-raced", "read:mail", time.Now().Add(time.Hour)) + + store.afterLookup = func(inner *memory.Store) { + attacker = clientPair("attacker", "fam-raced", "read:mail", time.Now().Add(time.Hour)) + require.NoError(t, inner.RotateRefreshToken(t.Context(), oauth2.HashToken(nil, "raced"), attacker)) + } + + hooks := &hookRecorder{} + + _, err := grant.NewRefreshToken(rotatingConfig(store, hooks)).Handle(t.Context(), refreshReq("raced", oauth2.Profile20BCP)) + assert.Same(t, oauth2.ErrRefreshTokenReused, err) + assert.Empty(t, hooks.errs, "the store revoked the family: nothing to report") + assert.Empty(t, store.revokes, "the grant does not revoke again") + + require.Len(t, store.calls, 1) + requireNoAccessToken(t, store, attacker.Access.TokenHash, store.calls[0].pair.Access.TokenHash) + + rt, err := store.LookupRefreshToken(t.Context(), attacker.Refresh.TokenHash) + require.NoError(t, err) + assert.True(t, rt.Consumed, "the competing exchange's refresh token is revoked too") +} + +// TestRefreshRevokedBetweenLookupAndRotation revokes the family between the +// grant's lookup and its rotation: the rotation is refused with +// ErrTokenFamilyRevoked and persists nothing. +func TestRefreshRevokedBetweenLookupAndRotation(t *testing.T) { + t.Parallel() + + store := &familyStore{Store: memory.New()} + seedPair(t, store.Store, "revoked-mid", "fam-mid", "read:mail", time.Now().Add(time.Hour)) + + store.afterLookup = func(inner *memory.Store) { + require.NoError(t, inner.RevokeRefreshFamily(t.Context(), "fam-mid")) + } + + resp, err := grant.NewRefreshToken(rotatingConfig(store, &hookRecorder{})). + Handle(t.Context(), refreshReq("revoked-mid", oauth2.Profile20BCP)) + assert.Nil(t, resp) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + + require.Len(t, store.calls, 1) + requireNoAccessToken(t, store, store.calls[0].pair.Access.TokenHash) + + _, err = store.LookupRefreshToken(t.Context(), store.calls[0].pair.Refresh.TokenHash) + assert.Error(t, err, "the refused rotation persisted no refresh token") +} + +// TestRefreshNonRotatingRefusedInRevokedFamily covers the legacy Profile20 +// refresh: the family revoked between the lookup and the issuance refuses +// the new access token instead of letting it outlive the revocation. +func TestRefreshNonRotatingRefusedInRevokedFamily(t *testing.T) { + t.Parallel() + + store := &familyStore{Store: memory.New()} + seedPair(t, store.Store, "static-mid", "fam-static", "read:mail", time.Now().Add(time.Hour)) + + store.afterLookup = func(inner *memory.Store) { + require.NoError(t, inner.RevokeRefreshFamily(t.Context(), "fam-static")) + } + + resp, err := grant.NewRefreshToken(grant.Config{Storage: store, AccessTokens: newAccessGen(), AccessTTL: time.Hour}). + Handle(t.Context(), refreshReq("static-mid", oauth2.Profile20)) + assert.Nil(t, resp) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + + require.Len(t, store.calls, 1) + assert.Equal(t, "save", store.calls[0].kind) + assert.Nil(t, store.calls[0].pair.Refresh) + requireNoAccessToken(t, store, store.calls[0].pair.Access.TokenHash) +} + +// TestRefreshRotationVanishedTokenIsInvalidGrant lets the store report the +// presented token unknown at rotation time — e.g. it expired from Redis +// since the lookup: that is an invalid grant (RFC 6749 §5.2), not a +// server error. +func TestRefreshRotationVanishedTokenIsInvalidGrant(t *testing.T) { + t.Parallel() + + store := &familyStore{ + Store: memory.New(), + rotateErr: oauth2.ErrInvalidGrant.WithDescription("refresh token not found"), + } + seedPair(t, store.Store, "vanished", "fam-vanished", "read:mail", time.Now().Add(time.Hour)) + + resp, err := grant.NewRefreshToken(rotatingConfig(store, &hookRecorder{})). + Handle(t.Context(), refreshReq("vanished", oauth2.Profile20BCP)) + assert.Nil(t, resp) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) +} + +// TestIssuanceMapsStorageFailures covers the answer of a failed SaveTokenPair: +// a family refused as revoked keeps its invalid_grant — a replayed +// authorization code revokes the family of the exchange it races — while a +// backend failure is a server_error whose cause stays off the wire. +func TestIssuanceMapsStorageFailures(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + saveErr error + code string + }{ + {"revoked family", oauth2.ErrTokenFamilyRevoked, oauth2.CodeInvalidGrant}, + {"backend failure", errBackendDown, oauth2.CodeServerError}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + store := &familyStore{Store: memory.New(), saveErr: tc.saveErr} + _, req := newAuthCodeReq(t.Context(), store.Store, true) + g := grant.NewAuthorizationCode(grant.Config{ + Storage: store, AccessTokens: newAccessGen(), RefreshTokens: newRefreshGen(), + AccessTTL: time.Hour, RefreshTTL: time.Hour, + }) + + resp, err := g.Handle(t.Context(), req) + assert.Nil(t, resp) + assert.Equal(t, tc.code, oauth2.IsCode(err)) + assert.ErrorIs(t, err, tc.saveErr, "the cause reaches the hook") + require.Len(t, store.calls, 1) + }) + } +} + +func TestAccessGeneratorFailurePersistsNothing(t *testing.T) { + t.Parallel() + + t.Run("authorization_code", func(t *testing.T) { + t.Parallel() + + store := &familyStore{Store: memory.New()} + _, req := newAuthCodeReq(t.Context(), store.Store, true) + + resp, err := grant.NewAuthorizationCode(grant.Config{ + Storage: store, AccessTokens: failingAccessGen{}, AccessTTL: time.Hour, + }).Handle(t.Context(), req) + assert.Nil(t, resp) + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(err)) + assert.Empty(t, store.calls) + }) + + t.Run("refresh_token", func(t *testing.T) { + t.Parallel() + + store := &familyStore{Store: memory.New()} + seedPair(t, store.Store, "no-access", "fam-no-access", "read:mail", time.Now().Add(time.Hour)) + + cfg := rotatingConfig(store, &hookRecorder{}) + cfg.AccessTokens = failingAccessGen{} + + resp, err := grant.NewRefreshToken(cfg).Handle(t.Context(), refreshReq("no-access", oauth2.Profile20BCP)) + assert.Nil(t, resp) + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(err)) + assert.Empty(t, store.calls) + }) +} + +func TestClientCredentialsStorageFailureIsServerError(t *testing.T) { + t.Parallel() + + store := &familyStore{Store: memory.New(), saveErr: errBackendDown} + + resp, err := grant.NewClientCredentials(grant.Config{ + Storage: store, AccessTokens: newAccessGen(), AccessTTL: time.Hour, + }).Handle(t.Context(), grant.Request{Client: newClient(), Form: url.Values{}, Now: time.Now()}) + assert.Nil(t, resp) + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(err)) + + require.Len(t, store.calls, 1) + assert.Empty(t, store.calls[0].pair.Access.FamilyID, "a client_credentials token belongs to no family") + assert.Nil(t, store.calls[0].pair.Refresh) +} + +// TestRefreshReuseWithoutFamilyRevokesNothing presents a consumed refresh +// token stored without a family — only a store predating token families +// holds one: it is refused as reuse, with no family to revoke. +func TestRefreshReuseWithoutFamilyRevokesNothing(t *testing.T) { + t.Parallel() + + store := &familyStore{Store: memory.New(), lookup: &oauth2.RefreshToken{ + TokenHash: oauth2.HashToken(nil, "legacy"), ClientID: clientID, Subject: subject, + Scope: "read:mail", ExpiresAt: time.Now().Add(time.Hour), Consumed: true, + }} + hooks := &hookRecorder{} + + _, err := grant.NewRefreshToken(rotatingConfig(store, hooks)).Handle(t.Context(), refreshReq("legacy", oauth2.Profile20BCP)) + assert.Same(t, oauth2.ErrRefreshTokenReused, err) + assert.Empty(t, store.revokes, "RevokeRefreshFamily refuses the empty family id") + assert.Empty(t, hooks.errs) +} diff --git a/oauth2/grant/grant.go b/oauth2/grant/grant.go index c706fbc..fd0a4aa 100644 --- a/oauth2/grant/grant.go +++ b/oauth2/grant/grant.go @@ -58,9 +58,13 @@ type Config struct { RotateRefreshTokens bool // OnError, when set, observes the errors a grant swallows to keep the // protocol response intact — today, a family revocation that failed - // during reuse detection. Errors returned to the server travel to - // [oauth2.ServerConfig.OnError] instead, so wire both to the same sink. - // Optional; see [oauth2.ErrorHook]. + // after a refresh-token reuse, whether the grant found the token + // consumed or the store reported the reuse while rotating: exactly one + // server_error per failed revocation, whose cause matches + // [oauth2.ErrTokenFamilyRevocationFailed] with a conforming store. + // Errors returned to the server travel to [oauth2.ServerConfig.OnError] + // instead, so wire both to the same sink. Optional; see + // [oauth2.ErrorHook]. OnError oauth2.ErrorHook } @@ -73,6 +77,22 @@ func (c Config) notifyError(ctx context.Context, err error) { c.OnError(ctx, err) } +// detachedRevocationTimeout bounds a family revocation detached from the +// request that triggered it. +const detachedRevocationTimeout = 10 * time.Second + +// revokeFamilyDetached revokes familyID as the security response to a +// detected replay. It runs on a context detached from the request's +// cancellation — a client hanging up must not abort the revocation its +// replay triggered — bounded by detachedRevocationTimeout. The caller +// reports a failure: it cannot change the answer the client gets. +func (c Config) revokeFamilyDetached(ctx context.Context, familyID string) error { + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), detachedRevocationTimeout) + defer cancel() + + return c.Storage.RevokeRefreshFamily(ctx, familyID) //nolint:wrapcheck // reported as the hook cause, unchanged +} + // Request and Response are type aliases anchoring the contract in the // parent oauth2 package so handlers and the Server share one definition. type ( diff --git a/oauth2/grant/grant_more_test.go b/oauth2/grant/grant_more_test.go index 3a875d3..10b05e7 100644 --- a/oauth2/grant/grant_more_test.go +++ b/oauth2/grant/grant_more_test.go @@ -268,17 +268,12 @@ func TestClientCredentialsDefaultsToFirstScope(t *testing.T) { // --- refresh_token edge cases ------------------------------------------- -// seedRefresh stores a refresh token and returns its raw value. -func seedRefresh(t *testing.T, store interface { - SaveRefreshToken(context.Context, *oauth2.RefreshToken) error -}, raw, scope string, expiresAt time.Time) { +// seedRefresh stores a refresh token raw in family "fam-x", alongside an +// access token of its family. +func seedRefresh(t *testing.T, store oauth2.Storage, raw, scope string, expiresAt time.Time) { t.Helper() - require.NoError(t, store.SaveRefreshToken(context.Background(), &oauth2.RefreshToken{ - Token: raw, TokenHash: oauth2.HashToken(nil, raw), - ClientID: clientID, Subject: subject, Scope: scope, - IssuedAt: time.Now().Add(-time.Hour), ExpiresAt: expiresAt, FamilyID: "fam-x", - })) + seedPair(t, store, raw, "fam-x", scope, expiresAt) } func TestRefreshTokenMissing(t *testing.T) { diff --git a/oauth2/grant/grant_test.go b/oauth2/grant/grant_test.go index d3ab6ad..6aaf915 100644 --- a/oauth2/grant/grant_test.go +++ b/oauth2/grant/grant_test.go @@ -220,13 +220,7 @@ func TestRefreshTokenRotationDetectsReuse(t *testing.T) { // Seed an existing refresh token. rawRT := "raw-refresh-token-xyz" - rtHash := oauth2.HashToken(nil, rawRT) - rt := &oauth2.RefreshToken{ - Token: rawRT, TokenHash: rtHash, ClientID: clientID, Subject: subject, - Scope: "read:mail", IssuedAt: now, ExpiresAt: now.Add(24 * time.Hour), - FamilyID: "family-1", - } - require.NoError(t, store.SaveRefreshToken(context.Background(), rt)) + seedPair(t, store, rawRT, "family-1", "read:mail", now.Add(24*time.Hour)) g := grant.NewRefreshToken(grant.Config{ Storage: store, AccessTokens: newAccessGen(), RefreshTokens: newRefreshGen(), diff --git a/oauth2/grant/hook_test.go b/oauth2/grant/hook_test.go index 761fd7a..ac87d10 100644 --- a/oauth2/grant/hook_test.go +++ b/oauth2/grant/hook_test.go @@ -30,22 +30,14 @@ func (s *familyRevokeFailingStore) RevokeRefreshFamily(context.Context, string) return s.err } -// consumedRefreshRequest stores an already-consumed refresh token and -// returns the matching /token request — the BCP §8.10.3 reuse case. -func consumedRefreshRequest(ctx context.Context, store oauth2.Storage) grant.Request { - raw := "reused-refresh-token" +// consumedRefreshRequest stores a refresh token of family "family-1", +// consumes it by a real rotation, and returns the /token request replaying +// it — the RFC 9700 §4.14.2 reuse case. +func consumedRefreshRequest(t *testing.T, store oauth2.Storage) grant.Request { + t.Helper() - _ = store.SaveRefreshToken(ctx, &oauth2.RefreshToken{ - Token: raw, - TokenHash: oauth2.HashToken(nil, raw), - ClientID: clientID, - Subject: subject, - Scope: "read:mail", - FamilyID: "family-1", - Consumed: true, - IssuedAt: time.Date(2026, 5, 20, 12, 0, 0, 0, time.UTC), - ExpiresAt: time.Date(2026, 5, 21, 12, 0, 0, 0, time.UTC), - }) + raw := "reused-refresh-token" + seedRotated(t, store, raw, "family-1") return grant.Request{ Client: newClient(), @@ -80,7 +72,7 @@ func TestRefreshTokenOnErrorReportsFamilyRevokeFailure(t *testing.T) { }, }) - resp, err := g.Handle(ctx, consumedRefreshRequest(ctx, store)) + resp, err := g.Handle(ctx, consumedRefreshRequest(t, store)) // The protocol answer is unchanged: reuse stays invalid_grant. assert.Nil(t, resp) @@ -112,7 +104,7 @@ func TestRefreshTokenOnErrorIsOptional(t *testing.T) { RotateRefreshTokens: true, }) - req := consumedRefreshRequest(ctx, store) + req := consumedRefreshRequest(t, store) assert.NotPanics(t, func() { _, err := g.Handle(ctx, req) @@ -140,7 +132,7 @@ func TestRefreshTokenOnErrorQuietOnSuccessfulRevoke(t *testing.T) { }, }) - _, err := g.Handle(ctx, consumedRefreshRequest(ctx, store)) + _, err := g.Handle(ctx, consumedRefreshRequest(t, store)) require.ErrorIs(t, err, oauth2.ErrRefreshTokenReused) assert.Empty(t, seen, "a successful family revocation must stay quiet") } diff --git a/oauth2/grant/issue.go b/oauth2/grant/issue.go index 7423d0a..bbcd686 100644 --- a/oauth2/grant/issue.go +++ b/oauth2/grant/issue.go @@ -12,19 +12,41 @@ import ( ) // issueTokenPair mints an access token — and, when the config carries a -// refresh-token generator, a companion refresh token in the same family — -// for the given subject and scope, persists them, and returns the grant -// response. It is the shared issuance path of the authorization_code and -// legacy password grants. +// refresh-token generator, a companion refresh token — in a new family for +// the given subject and scope, persists them atomically, and returns the +// grant response. It is the shared issuance path of the authorization_code +// and legacy password grants. func issueTokenPair(ctx context.Context, cfg Config, req Request, subject, scope string) (*Response, error) { familyID, err := newFamilyID() if err != nil { return nil, oauth2.ErrServerError.WithCause(err) } + access, err := mintAccess(ctx, cfg, req, subject, scope, familyID) + if err != nil { + return nil, err + } + + pair := &oauth2.TokenPair{Access: *access} + + if cfg.RefreshTokens != nil { + if pair.Refresh, err = mintRefresh(ctx, cfg, req, subject, scope, familyID); err != nil { + return nil, err + } + } + + if err := cfg.Storage.SaveTokenPair(ctx, pair); err != nil { + return nil, storageFailure(err) + } + + return pairResponse(pair, scope), nil +} + +// mintAccess generates an access token in familyID; nothing is persisted. +func mintAccess(ctx context.Context, cfg Config, req Request, subject, scope, familyID string) (*oauth2.AccessToken, error) { expires := req.Now.Add(cfg.AccessTTL) - atRaw, atHash, err := cfg.AccessTokens.Generate(ctx, token.AccessTokenClaims{ + raw, hash, err := cfg.AccessTokens.Generate(ctx, token.AccessTokenClaims{ Issuer: req.Issuer, Subject: subject, Audience: req.Audience, @@ -38,40 +60,42 @@ func issueTokenPair(ctx context.Context, cfg Config, req Request, subject, scope return nil, oauth2.ErrServerError.WithCause(err) } - access := &oauth2.AccessToken{ - Token: atRaw, TokenHash: atHash, ClientID: req.Client.ID(), Subject: subject, + return &oauth2.AccessToken{ + Token: raw, TokenHash: hash, ClientID: req.Client.ID(), Subject: subject, Scope: scope, IssuedAt: req.Now, ExpiresAt: expires, FamilyID: familyID, Audience: req.Audience, - } - if err := cfg.Storage.SaveAccessToken(ctx, access); err != nil { - return nil, oauth2.ErrServerError.WithCause(err) - } - - resp := &Response{ - Pair: oauth2.TokenPair{Access: *access}, - Scope: scope, - TokenType: oauth2.TokenTypeBearer, - } - - if cfg.RefreshTokens == nil { - return resp, nil - } + }, nil +} - rtRaw, rtHash, err := cfg.RefreshTokens.Generate(ctx) +// mintRefresh generates a refresh token in familyID with the RefreshTokens +// generator, which the caller ensured is set; nothing is persisted. +func mintRefresh(ctx context.Context, cfg Config, req Request, subject, scope, familyID string) (*oauth2.RefreshToken, error) { + raw, hash, err := cfg.RefreshTokens.Generate(ctx) if err != nil { return nil, oauth2.ErrServerError.WithCause(err) } - refresh := &oauth2.RefreshToken{ - Token: rtRaw, TokenHash: rtHash, ClientID: req.Client.ID(), Subject: subject, + return &oauth2.RefreshToken{ + Token: raw, TokenHash: hash, ClientID: req.Client.ID(), Subject: subject, Scope: scope, IssuedAt: req.Now, ExpiresAt: req.Now.Add(cfg.RefreshTTL), FamilyID: familyID, - } - if err := cfg.Storage.SaveRefreshToken(ctx, refresh); err != nil { - return nil, oauth2.ErrServerError.WithCause(err) - } + }, nil +} - resp.Pair.Refresh = refresh +// pairResponse wraps a persisted pair into the grant response. +func pairResponse(pair *oauth2.TokenPair, scope string) *Response { + return &Response{Pair: *pair, Scope: scope, TokenType: oauth2.TokenTypeBearer} +} + +// storageFailure maps a failed persistence call to the grant's answer. A +// refusal the store reports as invalid_grant — a revoked family, a refresh +// token that vanished — is the answer RFC 6749 §5.2 prescribes and passes +// through; anything else becomes a server_error whose cause stays off the +// wire. +func storageFailure(err error) error { + if oauth2.IsCode(err) == oauth2.CodeInvalidGrant { + return err + } - return resp, nil + return oauth2.ErrServerError.WithCause(err) } diff --git a/oauth2/grant/refresh_token.go b/oauth2/grant/refresh_token.go index 1f1e2f4..9a6aae1 100644 --- a/oauth2/grant/refresh_token.go +++ b/oauth2/grant/refresh_token.go @@ -9,7 +9,6 @@ import ( "errors" "github.com/hyperscale-stack/security/oauth2" - "github.com/hyperscale-stack/security/oauth2/token" ) // RefreshToken implements the RFC 6749 §6 refresh_token grant with the @@ -17,8 +16,8 @@ import ( // // The flow: // -// 1. Look up the refresh token; treat consumed tokens as reuse and revoke -// the family. +// 1. Look up the refresh token; a consumed one (rotated, or of a revoked +// family) is reuse: revoke the family and refuse with invalid_grant. // 2. Re-bind client (refresh.ClientID MUST match the authenticated client). // 3. Optionally narrow scope (RFC 6749 §6 forbids broadening). // 4. Decide rotation before minting anything: mandatory under @@ -26,10 +25,14 @@ import ( // [Config.RotateRefreshTokens] under [oauth2.Profile20]. A rotation // that cannot be honored fails with server_error // ([ErrRotationRequiresGenerator]). -// 5. Issue a fresh access token. -// 6. When rotating, issue a fresh refresh token in the same family with -// the presented token's scope (RFC 6749 §6) AND atomically mark the -// old one consumed. +// 5. Mint a fresh access token. +// 6. When rotating, mint a fresh refresh token in the same family with +// the presented token's scope (RFC 6749 §6), then consume the old token +// and persist the new pair in one atomic +// [oauth2.RefreshTokenStore.RotateRefreshToken]; otherwise persist the +// access token alone into the family. Either write fails, persisting +// nothing, once the family is revoked — the refusal stays invalid_grant +// — and a reuse the store detects is answered like step 1. type RefreshToken struct { cfg Config } @@ -77,14 +80,9 @@ func (g *RefreshToken) Handle(ctx context.Context, req Request) (*Response, erro } if rt.Consumed { - // Reuse detected — revoke the whole family and refuse. The refusal - // stays invalid_grant whatever the revocation did, so a failure - // there is only observable through the error hook. - if err := g.cfg.Storage.RevokeRefreshFamily(ctx, rt.FamilyID); err != nil { - g.cfg.notifyError(ctx, oauth2.ErrServerError. - WithDescription("revoke refresh family failed after reuse detection"). - WithCause(err)) - } + // Reuse detected — revoke the whole family and refuse (RFC 9700 + // §4.14.2). + g.revokeReusedFamily(ctx, rt.FamilyID) return nil, oauth2.ErrRefreshTokenReused } @@ -134,71 +132,74 @@ func (g *RefreshToken) ValidateProfile(p oauth2.Profile) error { func (g *RefreshToken) issueRotated(ctx context.Context, req Request, old *oauth2.RefreshToken, scope string) (*Response, error) { // Decided before anything is minted: a rotation that cannot be honored - // must not leave an access token behind. + // must not mint an access token. rotate, err := g.rotates(req.Profile) if err != nil { return nil, oauth2.ErrServerError.WithCause(err) } - expires := req.Now.Add(g.cfg.AccessTTL) - - atRaw, atHash, err := g.cfg.AccessTokens.Generate(ctx, token.AccessTokenClaims{ - Issuer: req.Issuer, - Subject: old.Subject, - Audience: req.Audience, - ClientID: req.Client.ID(), - Scope: scope, - FamilyID: old.FamilyID, - IssuedAt: req.Now, - ExpiresAt: expires, - }) + access, err := mintAccess(ctx, g.cfg, req, old.Subject, scope, old.FamilyID) if err != nil { - return nil, oauth2.ErrServerError.WithCause(err) - } - - access := &oauth2.AccessToken{ - Token: atRaw, TokenHash: atHash, ClientID: req.Client.ID(), Subject: old.Subject, - Scope: scope, IssuedAt: req.Now, ExpiresAt: expires, - FamilyID: old.FamilyID, Audience: req.Audience, - } - if err := g.cfg.Storage.SaveAccessToken(ctx, access); err != nil { - return nil, oauth2.ErrServerError.WithCause(err) + return nil, err } - resp := &Response{ - Pair: oauth2.TokenPair{Access: *access}, - Scope: scope, - TokenType: oauth2.TokenTypeBearer, - } + pair := &oauth2.TokenPair{Access: *access} if !rotate { - return resp, nil - } + // The legacy Profile20 refresh: the access token alone joins the + // family, which the store refuses once revoked. + if err := g.cfg.Storage.SaveTokenPair(ctx, pair); err != nil { + return nil, storageFailure(err) + } - rtRaw, rtHash, err := g.cfg.RefreshTokens.Generate(ctx) - if err != nil { - return nil, oauth2.ErrServerError.WithCause(err) + return pairResponse(pair, scope), nil } // RFC 6749 §6: the new refresh token keeps the scope of the one it // replaces; a narrowed request only narrows the access token. - next := &oauth2.RefreshToken{ - Token: rtRaw, TokenHash: rtHash, ClientID: req.Client.ID(), Subject: old.Subject, - Scope: old.Scope, IssuedAt: req.Now, ExpiresAt: req.Now.Add(g.cfg.RefreshTTL), - FamilyID: old.FamilyID, + if pair.Refresh, err = mintRefresh(ctx, g.cfg, req, old.Subject, old.Scope, old.FamilyID); err != nil { + return nil, err } - if err := g.cfg.Storage.RotateRefreshToken(ctx, old.TokenHash, next); err != nil { - if errors.Is(err, oauth2.ErrRefreshTokenReused) { - return nil, err //nolint:wrapcheck // oauth2.* sentinels pass through - } + if err := g.cfg.Storage.RotateRefreshToken(ctx, old.TokenHash, pair); err != nil { + return nil, g.rotationFailure(ctx, old.FamilyID, err) + } - return nil, oauth2.ErrServerError.WithCause(err) + return pairResponse(pair, scope), nil +} + +// rotationFailure maps a failed rotation. Reuse — the token was consumed +// since the lookup — answers the bare invalid_grant +// [oauth2.ErrRefreshTokenReused]; when the store could not revoke the +// family on that path, the revocation is retried as for a reuse the grant +// detects itself. Anything else goes through storageFailure. +func (g *RefreshToken) rotationFailure(ctx context.Context, familyID string, err error) error { + if !errors.Is(err, oauth2.ErrRefreshTokenReused) { + return storageFailure(err) } - resp.Pair.Refresh = next + if errors.Is(err, oauth2.ErrTokenFamilyRevocationFailed) { + g.revokeReusedFamily(ctx, familyID) + } + + return oauth2.ErrRefreshTokenReused +} - return resp, nil +// revokeReusedFamily answers a detected refresh-token reuse: it revokes the +// token's family on a context detached from the request +// ([Config.revokeFamilyDetached]) and reports a failure to the error hook, +// since the client gets invalid_grant either way. A token outside any +// family leaves nothing to revoke. +func (g *RefreshToken) revokeReusedFamily(ctx context.Context, familyID string) { + if familyID == "" { + return + } + + if err := g.cfg.revokeFamilyDetached(ctx, familyID); err != nil { + g.cfg.notifyError(ctx, oauth2.ErrServerError. + WithDescription("revoke refresh family failed after reuse detection"). + WithCause(err)) + } } // narrowScopesForRefresh refuses broadening (RFC 6749 §6). An empty diff --git a/oauth2/grant/refresh_token_test.go b/oauth2/grant/refresh_token_test.go index 5e14f81..310088c 100644 --- a/oauth2/grant/refresh_token_test.go +++ b/oauth2/grant/refresh_token_test.go @@ -211,15 +211,11 @@ func TestRefreshTokenReuseDetectedWithoutGenerator(t *testing.T) { ctx := t.Context() store := newStore() - req := consumedRefreshRequest(ctx, store) // family-1, Profile20BCP + req := consumedRefreshRequest(t, store) // family-1, Profile20BCP const sibling = "active-sibling" - require.NoError(t, store.SaveRefreshToken(ctx, &oauth2.RefreshToken{ - Token: sibling, TokenHash: oauth2.HashToken(nil, sibling), - ClientID: clientID, Subject: subject, Scope: "read:mail", FamilyID: "family-1", - IssuedAt: req.Now, ExpiresAt: req.Now.Add(time.Hour), - })) + seedPair(t, store, sibling, "family-1", "read:mail", time.Now().Add(time.Hour)) g := grant.NewRefreshToken(grant.Config{ Storage: store, AccessTokens: newAccessGen(), AccessTTL: time.Hour, // no RefreshTokens generator diff --git a/oauth2/hook.go b/oauth2/hook.go index 4966edc..4a89c5f 100644 --- a/oauth2/hook.go +++ b/oauth2/hook.go @@ -13,10 +13,12 @@ import "context" // The hook receives the [*Error] envelope with its Cause intact, which // is the only place the cause of a server_error is ever exposed — the // wire response deliberately drops it. -// - the errors the server swallows to stay protocol-compliant, such as a -// best-effort revocation that failed (RFC 7009 §2.2 mandates 200 OK -// whatever happened) or an [IntrospectionPolicy] that failed (the token -// is withheld behind a bare {"active":false}, RFC 7662 §2.2). +// - the errors behind an answer that cannot carry them: every storage +// failure at /revoke, reported as a server_error — when it leaves the +// revocation unfinished, /revoke answers a bare 503 +// temporarily_unavailable (RFC 7009 §2.2.1) that is not reported +// again — or an [IntrospectionPolicy] that failed (the token is +// withheld behind a bare {"active":false}, RFC 7662 §2.2). // // The hook is a pure observability sink: it MUST NOT influence the // response. It runs synchronously on the request goroutine, so a slow diff --git a/oauth2/hook_test.go b/oauth2/hook_test.go index a27a3e0..d5cfd25 100644 --- a/oauth2/hook_test.go +++ b/oauth2/hook_test.go @@ -289,13 +289,14 @@ func TestOnErrorReportsFailedRevocation(t *testing.T) { require.NotEmpty(t, issued.AccessToken) require.Empty(t, sink.codes(), "a successful /token must not notify") - // RFC 7009 §2.2: the revocation still answers 200 OK... + // RFC 7009 §2.2.1: the token may still be usable, so the revocation + // answers 503 — the client must assume the token still exists... rec = httptest.NewRecorder() srv.RevokeHandler().ServeHTTP(rec, formRequest("/revoke", url.Values{"token": {issued.AccessToken}}, true)) - assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, http.StatusServiceUnavailable, rec.Code) - // ...and the failure surfaces through the hook only. + // ...and the failure's cause surfaces through the hook only. got := sink.collected() require.NotEmpty(t, got) @@ -338,22 +339,14 @@ func TestOnErrorReportsFailedFamilyRevocation(t *testing.T) { require.NoError(t, err) raw := "refresh-to-revoke" - require.NoError(t, store.SaveRefreshToken(t.Context(), &oauth2.RefreshToken{ - Token: raw, - TokenHash: oauth2.HashToken(nil, raw), - ClientID: testClientID, - Subject: "alice", - Scope: "api:read", - FamilyID: "family-1", - IssuedAt: time.Now(), - ExpiresAt: time.Now().Add(time.Hour), - })) + seedPair(t, store, newClientPair(raw, "family-1")) rec := httptest.NewRecorder() srv.RevokeHandler().ServeHTTP(rec, formRequest("/revoke", url.Values{"token": {raw}}, true)) - // RFC 7009 §2.2 still mandates 200 OK; the hook carries the failure. - assert.Equal(t, http.StatusOK, rec.Code) + // RFC 7009 §2.2.1: 503, the token may still be usable; the hook + // carries the failure. + assert.Equal(t, http.StatusServiceUnavailable, rec.Code) got := sink.collected() require.Len(t, got, 1) diff --git a/oauth2/introspect_endpoint_test.go b/oauth2/introspect_endpoint_test.go index 4530f08..9c7a7d0 100644 --- a/oauth2/introspect_endpoint_test.go +++ b/oauth2/introspect_endpoint_test.go @@ -138,7 +138,7 @@ func newIntrospectServer(t *testing.T, policy oauth2.IntrospectionPolicy, onErro // - at-app / rt-app (fam-app): active, for alice, scope read, audience api. // - at-expired / rt-expired (fam-expired): past their expiry. // - at-consumed / rt-consumed (fam-consumed): the refresh token was -// already rotated. +// already rotated, into at-consumed-next / rt-consumed-next. func seedTokens(t *testing.T, store *memory.Store) { t.Helper() @@ -174,15 +174,26 @@ func seedTokens(t *testing.T, store *memory.Store) { }, refresh: oauth2.RefreshToken{ TokenHash: oauth2.HashToken(nil, "rt-consumed"), ClientID: "app", Subject: "alice", Scope: "read", - FamilyID: "fam-consumed", IssuedAt: seededIssuedAt, ExpiresAt: seededRefreshExp, Consumed: true, + FamilyID: "fam-consumed", IssuedAt: seededIssuedAt, ExpiresAt: seededRefreshExp, }, }, } for _, p := range pairs { - require.NoError(t, store.SaveAccessToken(t.Context(), &p.access)) - require.NoError(t, store.SaveRefreshToken(t.Context(), &p.refresh)) + require.NoError(t, store.SaveTokenPair(t.Context(), &oauth2.TokenPair{Access: p.access, Refresh: &p.refresh})) } + + // A refresh token is only ever consumed by a rotation. + require.NoError(t, store.RotateRefreshToken(t.Context(), oauth2.HashToken(nil, "rt-consumed"), &oauth2.TokenPair{ + Access: oauth2.AccessToken{ + TokenHash: oauth2.HashToken(nil, "at-consumed-next"), ClientID: "app", Subject: "alice", Scope: "read", + Audience: "api", FamilyID: "fam-consumed", IssuedAt: seededIssuedAt, ExpiresAt: seededAccessExp, + }, + Refresh: &oauth2.RefreshToken{ + TokenHash: oauth2.HashToken(nil, "rt-consumed-next"), ClientID: "app", Subject: "alice", Scope: "read", + FamilyID: "fam-consumed", IssuedAt: seededIssuedAt, ExpiresAt: seededRefreshExp, + }, + })) } // basicForm builds a form POST authenticated with HTTP Basic as id:secret diff --git a/oauth2/models.go b/oauth2/models.go index 32abdd1..6536f20 100644 --- a/oauth2/models.go +++ b/oauth2/models.go @@ -62,7 +62,8 @@ type AccessToken struct { ExpiresAt time.Time // FamilyID identifies the refresh-token family the token belongs to, // so revoking the family revokes it too. Empty for tokens issued - // outside a family (client_credentials, the implicit flow). + // outside a family (client_credentials, the implicit flow). At most + // [MaxFamilyIDLength] bytes. FamilyID string // Audience is the configured aud claim (typically the resource server // identifier). Single-valued in this model; servers needing multi-aud @@ -88,10 +89,13 @@ type RefreshToken struct { // FamilyID groups every refresh token derived from the same original // authorisation. Rotation issues a new RefreshToken with the same // FamilyID; reuse of a consumed token leads to revocation of the - // entire family (OAuth 2.0 BCP §8.10.3). + // entire family (RFC 9700 §4.14.2). Never empty for a stored refresh + // token; at most [MaxFamilyIDLength] bytes. FamilyID string - // Consumed indicates whether the token has been rotated. Reuse of a - // consumed token MUST trigger family revocation. + // Consumed reports that the token can no longer be exchanged: it was + // rotated, or its family was revoked. Presenting a consumed token is + // reuse, which MUST trigger family revocation. Lookups set it; a new + // token is never persisted consumed. Consumed bool } @@ -103,9 +107,40 @@ func (t *RefreshToken) IsExpired(now time.Time) bool { // TokenPair couples an access token with the refresh token issued alongside // it. Refresh is nil when the grant issues none: client_credentials, a grant // without a RefreshTokens generator, or a non-rotating refresh under -// Profile20. The grant handlers return this; the response writer turns it -// into the RFC 6749 §5.1 JSON body. +// Profile20. It is the unit of persistence — [Storage] writes a pair +// atomically, through SaveTokenPair or RotateRefreshToken — and what the +// grant handlers return; the response writer turns it into the RFC 6749 +// §5.1 JSON body. type TokenPair struct { Access AccessToken Refresh *RefreshToken } + +// Validate reports, as an invalid_request [*Error], a pair no [Storage] +// may persist: a nil pair, an access token without hash or with a family +// ID longer than [MaxFamilyIDLength], or a refresh token without hash, +// outside a family, in another family than the access token, or already +// consumed. A refresh token only ever travels with an access token of its +// own family. +func (p *TokenPair) Validate() error { + switch { + case p == nil: + return ErrInvalidRequest.WithDescription("storage: nil token pair") + case p.Access.TokenHash == "": + return ErrInvalidRequest.WithDescription("storage: empty access token hash") + case len(p.Access.FamilyID) > MaxFamilyIDLength: + return ErrInvalidRequest.WithDescription("storage: token family id too long") + case p.Refresh == nil: + return nil + case p.Refresh.TokenHash == "": + return ErrInvalidRequest.WithDescription("storage: empty refresh token hash") + case p.Refresh.FamilyID == "": + return ErrInvalidRequest.WithDescription("storage: refresh token outside a token family") + case p.Refresh.FamilyID != p.Access.FamilyID: + return ErrInvalidRequest.WithDescription("storage: refresh and access tokens in different families") + case p.Refresh.Consumed: + return ErrInvalidRequest.WithDescription("storage: refresh token already consumed") + default: + return nil + } +} diff --git a/oauth2/refresh_rotation_test.go b/oauth2/refresh_rotation_test.go index 51a4796..ae645a3 100644 --- a/oauth2/refresh_rotation_test.go +++ b/oauth2/refresh_rotation_test.go @@ -38,20 +38,12 @@ func (s *spyAccessGen) Generate(context.Context, token.AccessTokenClaims) (strin // wrapped grant implements. type hiddenGrant struct{ oauth2.Grant } -// seedServerRefresh stores a live refresh token issued to the test client. +// seedServerRefresh stores a live refresh token issued to the test client +// in family "family-rot", alongside an access token of its family. func seedServerRefresh(t *testing.T, store oauth2.Storage, raw string) { t.Helper() - require.NoError(t, store.SaveRefreshToken(t.Context(), &oauth2.RefreshToken{ - Token: raw, - TokenHash: oauth2.HashToken(nil, raw), - ClientID: testClientID, - Subject: "alice", - Scope: "api:read", - FamilyID: "family-rot", - IssuedAt: time.Now(), - ExpiresAt: time.Now().Add(time.Hour), - })) + seedPair(t, store, newClientPair(raw, "family-rot")) } // refreshServerConfig is hookServerConfig serving the single grant g under diff --git a/oauth2/revoke_endpoint.go b/oauth2/revoke_endpoint.go index 59332a9..b4544a3 100644 --- a/oauth2/revoke_endpoint.go +++ b/oauth2/revoke_endpoint.go @@ -9,14 +9,26 @@ import ( "net/http" ) +// errRevocationUnavailable answers a revocation that may not have taken +// effect (RFC 7009 §2.2.1): the client must assume the token still exists, +// and may retry — revocation is idempotent. +var errRevocationUnavailable = newCoded(CodeTemporarilyUnavailable, "revocation temporarily unavailable", nil) + // RevokeHandler returns the http.Handler for RFC 7009 token revocation. // Both access and refresh tokens are accepted; the server tries each -// kind in turn. Revocation of a refresh token also revokes the rest of -// its family (the BCP §8.10.3 reuse-detection mechanism reuses the same -// hammer). +// kind in turn. Revoking a refresh token revokes its whole family, and +// revoking an access token revokes its family too (RFC 7009 §2.1). // -// The handler always returns 200 OK on completion — RFC 7009 §2.2 says -// revocation requests MUST NOT leak whether the token existed. +// The answer is 200 OK whether or not the token existed or belonged to the +// caller (RFC 7009 §2.2), and 503 temporarily_unavailable when a storage +// failure leaves the revocation unfinished (RFC 7009 §2.2.1): the token's +// lookup failed, its family could not be revoked, or — for a token outside +// any family — it could not be deleted. The client then retries, which +// redoes the whole revocation. Each storage failure reaches the +// [ErrorHook] once, as a server_error carrying its cause; the 503 is not +// reported again. A 503 hints that the token exists only when the backend +// fails while the caller presents it, a caller that already holds the +// token. func (s *Server) RevokeHandler() http.Handler { return http.HandlerFunc(s.serveRevoke) } @@ -50,55 +62,96 @@ func (s *Server) serveRevoke(w http.ResponseWriter, r *http.Request) { // RFC 7009 §2.1: the hint is optional. We try access then refresh // regardless so the caller's hint is treated as advisory. - s.bestEffortRevoke(r.Context(), client, rawToken) + if !s.revoke(r.Context(), client, rawToken) { + writeErrorResponse(w, errRevocationUnavailable) + + return + } w.Header().Set("Cache-Control", "no-store") w.Header().Set("Pragma", "no-cache") w.WriteHeader(http.StatusOK) } -// bestEffortRevoke tries to revoke a token assuming it is an access token, -// then assuming it is a refresh token. The wire response stays silent: per -// RFC 7009 §2.2 it must not reveal whether the token existed, and it stays -// 200 OK even when the revocation itself failed. A failed revocation is -// exactly the event an operator needs, so it is reported to the configured -// [ErrorHook] instead — a missing token is not, so the lookups stay quiet. -func (s *Server) bestEffortRevoke(ctx context.Context, client Client, rawToken string) { +// revoke revokes rawToken when it is an access or a refresh token of +// client, and reports whether the token can no longer be used: true when it +// was revoked, is unknown, or belongs to another client; false when a +// lookup or a revocation failed and the token may still be usable. Each +// failure is reported to the [ErrorHook]; an unknown token is not. +func (s *Server) revoke(ctx context.Context, client Client, rawToken string) bool { hash := HashToken(nil, rawToken) - if at, err := s.cfg.Storage.LookupAccessToken(ctx, hash); err == nil { - s.revokeAccess(ctx, client, at, hash) + at, err := s.cfg.Storage.LookupAccessToken(ctx, hash) + if err == nil { + return s.revokeAccess(ctx, client, at, hash) + } - return + if !s.lookupMissed(ctx, err) { + return false } - if rt, err := s.cfg.Storage.LookupRefreshToken(ctx, hash); err == nil { - if rt.ClientID == client.ID() && rt.FamilyID != "" { - s.revokeFamily(ctx, rt.FamilyID) - } + rt, err := s.cfg.Storage.LookupRefreshToken(ctx, hash) + if err != nil { + return s.lookupMissed(ctx, err) + } + + if rt.ClientID != client.ID() || rt.FamilyID == "" { + return true } + + return s.revokeFamily(ctx, rt.FamilyID) } -// revokeAccess revokes an access token — and the refresh family it belongs -// to — once the token is confirmed to belong to the calling client. -func (s *Server) revokeAccess(ctx context.Context, client Client, at *AccessToken, hash string) { +// lookupMissed reports whether a failed lookup merely missed the token: +// invalid_grant, for a token that is unknown, expired from the store, or of +// a revoked family. Any other failure leaves the token possibly usable and +// is reported to the [ErrorHook]. +func (s *Server) lookupMissed(ctx context.Context, err error) bool { + if IsCode(err) == CodeInvalidGrant { + return true + } + + s.notifyError(ctx, ErrServerError.WithDescription("revoke: token lookup failed").WithCause(err)) + + return false +} + +// revokeAccess revokes an access token once it is confirmed to belong to +// the calling client, and reports whether the token can no longer be used. +// Each failure is reported to the [ErrorHook]. +// +// A token of a family is revoked through its family first (RFC 7009 §2.1). +// Should that fail, the token is left in place and the revocation reported +// unfinished, so that the retry it calls for redoes both. Once the family +// is revoked no token of it is usable, and deleting the access token is +// mere cleanup: a failed delete is reported but leaves the answer +// unchanged. A token outside any family only goes away when it is deleted. +func (s *Server) revokeAccess(ctx context.Context, client Client, at *AccessToken, hash string) bool { if at.ClientID != client.ID() { - return + return true + } + + if at.FamilyID != "" && !s.revokeFamily(ctx, at.FamilyID) { + return false } if err := s.cfg.Storage.RevokeAccessToken(ctx, hash); err != nil { s.notifyError(ctx, ErrServerError.WithDescription("revoke access token failed").WithCause(err)) - } - if at.FamilyID != "" { - s.revokeFamily(ctx, at.FamilyID) + return at.FamilyID != "" } + + return true } -// revokeFamily revokes a refresh-token family, reporting a failure to the -// [ErrorHook] rather than dropping it. -func (s *Server) revokeFamily(ctx context.Context, familyID string) { +// revokeFamily revokes a refresh-token family and reports whether it did, +// handing a failure to the [ErrorHook] rather than dropping it. +func (s *Server) revokeFamily(ctx context.Context, familyID string) bool { if err := s.cfg.Storage.RevokeRefreshFamily(ctx, familyID); err != nil { s.notifyError(ctx, ErrServerError.WithDescription("revoke refresh family failed").WithCause(err)) + + return false } + + return true } diff --git a/oauth2/revoke_failure_test.go b/oauth2/revoke_failure_test.go new file mode 100644 index 0000000..fbb53fe --- /dev/null +++ b/oauth2/revoke_failure_test.go @@ -0,0 +1,271 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package oauth2_test + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/hyperscale-stack/security/oauth2" + "github.com/hyperscale-stack/security/oauth2/storage/memory" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// unavailableBody is the /revoke answer when the revocation may not have +// taken effect (RFC 7009 §2.2.1). +const unavailableBody = `{"error":"temporarily_unavailable","error_description":"revocation temporarily unavailable"}` + +// revokeStore fails, one switch each, the storage calls /revoke makes. +type revokeStore struct { + *memory.Store + + accessLookupErr error + refreshLookupErr error + revokeAccessErr error + revokeFamilyErr error +} + +func (s *revokeStore) LookupAccessToken(ctx context.Context, hash string) (*oauth2.AccessToken, error) { + if s.accessLookupErr != nil { + return nil, s.accessLookupErr + } + + return s.Store.LookupAccessToken(ctx, hash) +} + +func (s *revokeStore) LookupRefreshToken(ctx context.Context, hash string) (*oauth2.RefreshToken, error) { + if s.refreshLookupErr != nil { + return nil, s.refreshLookupErr + } + + return s.Store.LookupRefreshToken(ctx, hash) +} + +func (s *revokeStore) RevokeAccessToken(ctx context.Context, hash string) error { + if s.revokeAccessErr != nil { + return s.revokeAccessErr + } + + return s.Store.RevokeAccessToken(ctx, hash) +} + +func (s *revokeStore) RevokeRefreshFamily(ctx context.Context, familyID string) error { + if s.revokeFamilyErr != nil { + return s.revokeFamilyErr + } + + return s.Store.RevokeRefreshFamily(ctx, familyID) +} + +// revokeServer serves /revoke over store, reporting to sink. +func revokeServer(t *testing.T, store oauth2.Storage, sink *errorSink) *oauth2.Server { + t.Helper() + + cfg := hookServerConfig(store) + cfg.OnError = sink.hook + + srv, err := oauth2.NewServer(cfg) + require.NoError(t, err) + + return srv +} + +// revoke POSTs raw to /revoke as the test client. +func revoke(srv *oauth2.Server, raw string) *httptest.ResponseRecorder { + rec := httptest.NewRecorder() + srv.RevokeHandler().ServeHTTP(rec, formRequest("/oauth2/revoke", url.Values{"token": {raw}}, true)) + + return rec +} + +// requireUnavailable asserts the 503 answer of a revocation that may not +// have taken effect, and that the hook received exactly the server_errors +// described by want — the 503 itself is not reported again. +func requireUnavailable(t *testing.T, rec *httptest.ResponseRecorder, sink *errorSink, want ...string) { + t.Helper() + + assert.Equal(t, http.StatusServiceUnavailable, rec.Code) + assert.JSONEq(t, unavailableBody, rec.Body.String()) + assert.Equal(t, "no-store", rec.Header().Get("Cache-Control")) + assert.NotContains(t, rec.Body.String(), "backend down") + + requireReported(t, sink, want...) +} + +// requireReported asserts the hook received one server_error per +// description in want, in order, each carrying the backend cause. +func requireReported(t *testing.T, sink *errorSink, want ...string) { + t.Helper() + + got := sink.collected() + require.Len(t, got, len(want)) + + for i, desc := range want { + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(got[i])) + assert.ErrorContains(t, got[i], desc) + assert.ErrorContains(t, errors.Unwrap(got[i]), "backend down") + } +} + +func TestRevokeAnswersUnavailableWhenTheTokenMayStillBeUsable(t *testing.T) { + t.Parallel() + + backendDown := errors.New("backend down") + + cases := []struct { + name string + store func(*memory.Store) *revokeStore + want []string + }{ + { + "access token lookup failed", + func(m *memory.Store) *revokeStore { return &revokeStore{Store: m, accessLookupErr: backendDown} }, + []string{"revoke: token lookup failed"}, + }, + { + "refresh token lookup failed", + func(m *memory.Store) *revokeStore { return &revokeStore{Store: m, refreshLookupErr: backendDown} }, + []string{"revoke: token lookup failed"}, + }, + { + "refresh family revocation failed", + func(m *memory.Store) *revokeStore { return &revokeStore{Store: m, revokeFamilyErr: backendDown} }, + []string{"revoke refresh family failed"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + store := tc.store(memory.New()) + srv := revokeServer(t, store, sink) + + // A refresh token: the access-token lookup misses it first. + seedPair(t, store.Store, newClientPair("unavailable-rt", "fam-unavailable")) + + requireUnavailable(t, revoke(srv, "unavailable-rt"), sink, tc.want...) + + // Nothing was revoked, and a retry once the backend is back + // revokes the token: the revocation is idempotent. + rt, err := store.Store.LookupRefreshToken(t.Context(), oauth2.HashToken(nil, "unavailable-rt")) + require.NoError(t, err) + assert.False(t, rt.Consumed) + + retry := revokeServer(t, store.Store, &errorSink{}) + assert.Equal(t, http.StatusOK, revoke(retry, "unavailable-rt").Code) + + rt, err = store.Store.LookupRefreshToken(t.Context(), oauth2.HashToken(nil, "unavailable-rt")) + require.NoError(t, err) + assert.True(t, rt.Consumed) + }) + } +} + +func TestRevokeAccessTokenAnswersByTheTokenOutcome(t *testing.T) { + t.Parallel() + + backendDown := errors.New("backend down") + + t.Run("the family revocation failed", func(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + store := &revokeStore{Store: memory.New(), revokeFamilyErr: familyRevocationFailure("fam-failed")} + pair := seedPair(t, store.Store, newClientPair("failed-rt", "fam-failed")) + + requireUnavailable(t, revoke(revokeServer(t, store, sink), pair.Access.Token), sink, + "revoke refresh family failed") + assert.ErrorContains(t, errors.Unwrap(sink.collected()[0]), `"fam-failed"`) + + // The access token was left in place, so the retry the 503 calls for + // redoes both revocations once the backend is back. + _, err := store.Store.LookupAccessToken(t.Context(), pair.Access.TokenHash) + require.NoError(t, err, "the access token was not deleted") + + rec := revoke(revokeServer(t, store.Store, &errorSink{}), pair.Access.Token) + assert.Equal(t, http.StatusOK, rec.Code) + + _, err = store.Store.LookupAccessToken(t.Context(), pair.Access.TokenHash) + assert.Error(t, err, "the access token is revoked") + + rt, err := store.Store.LookupRefreshToken(t.Context(), pair.Refresh.TokenHash) + require.NoError(t, err) + assert.True(t, rt.Consumed, "the family's refresh token is revoked") + }) + + t.Run("the family revocation covers a failed delete", func(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + store := &revokeStore{Store: memory.New(), revokeAccessErr: backendDown} + pair := seedPair(t, store.Store, newClientPair("covered-rt", "fam-covered")) + + rec := revoke(revokeServer(t, store, sink), pair.Access.Token) + assert.Equal(t, http.StatusOK, rec.Code, "the token is unusable through its revoked family") + requireReported(t, sink, "revoke access token failed") + + _, err := store.LookupAccessToken(t.Context(), pair.Access.TokenHash) + assert.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked) + }) + + t.Run("a token outside any family", func(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + store := &revokeStore{Store: memory.New(), revokeAccessErr: backendDown} + cc := newClientPair("cc-rt", "") + cc.Access.FamilyID, cc.Refresh = "", nil + seedPair(t, store.Store, cc) + + requireUnavailable(t, revoke(revokeServer(t, store, sink), cc.Access.Token), sink, + "revoke access token failed") + }) +} + +func TestRevokeLeavesOtherClientsTokensAlone(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + store := memory.New() + srv := revokeServer(t, store, sink) + + other := newClientPair("other-rt", "fam-other") + other.Access.ClientID, other.Refresh.ClientID = "someone-else", "someone-else" + seedPair(t, store, other) + + for _, raw := range []string{other.Access.Token, other.Refresh.Token} { + rec := revoke(srv, raw) + assert.Equal(t, http.StatusOK, rec.Code, "RFC 7009 §2.2: no hint the token exists") + } + + assert.Empty(t, sink.collected()) + + _, err := store.LookupAccessToken(t.Context(), other.Access.TokenHash) + require.NoError(t, err, "a client cannot revoke another client's token") +} + +func TestRevokeIsQuietForAlreadyRevokedFamilies(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + store := memory.New() + srv := revokeServer(t, store, sink) + + pair := seedPair(t, store, newClientPair("done-rt", "fam-done")) + require.NoError(t, store.RevokeRefreshFamily(t.Context(), "fam-done")) + + for _, raw := range []string{pair.Access.Token, pair.Refresh.Token} { + assert.Equal(t, http.StatusOK, revoke(srv, raw).Code) + } + + assert.Empty(t, sink.collected(), "a revoked family's token reads like an unknown one") +} diff --git a/oauth2/server.go b/oauth2/server.go index 4fab0ba..6df99cf 100644 --- a/oauth2/server.go +++ b/oauth2/server.go @@ -65,9 +65,9 @@ type ServerConfig struct { Now func() time.Time // OnError, when set, observes every error the server turns into an // RFC 6749 §5.2 response — including the cause of a server_error, which - // never reaches the wire — plus the errors it swallows on purpose - // (best-effort revocation, a failed [IntrospectionPolicy]). Optional; - // see [ErrorHook]. + // never reaches the wire — plus the errors behind an answer that cannot + // carry them (the storage failures of /revoke, a failed + // [IntrospectionPolicy]). Optional; see [ErrorHook]. OnError ErrorHook // IntrospectionPolicy, when set, decides which active tokens each // confidential caller of /introspect may see — the place to enforce diff --git a/oauth2/server_test.go b/oauth2/server_test.go index d82b034..102eb4b 100644 --- a/oauth2/server_test.go +++ b/oauth2/server_test.go @@ -309,12 +309,12 @@ func TestRevokeEndpoint(t *testing.T) { srv, store := newTestServer(t) ctx := context.Background() - require.NoError(t, store.SaveAccessToken(ctx, &oauth2.AccessToken{ + require.NoError(t, store.SaveTokenPair(ctx, &oauth2.TokenPair{Access: oauth2.AccessToken{ TokenHash: oauth2.HashToken(nil, "raw-access"), ClientID: testClientID, FamilyID: "fam-1", ExpiresAt: time.Now().Add(time.Hour), - })) + }})) rec := httptest.NewRecorder() srv.RevokeHandler().ServeHTTP(rec, @@ -330,17 +330,19 @@ func TestRevokeEndpoint(t *testing.T) { srv, store := newTestServer(t) ctx := context.Background() - require.NoError(t, store.SaveRefreshToken(ctx, &oauth2.RefreshToken{ - TokenHash: oauth2.HashToken(nil, "raw-refresh"), - ClientID: testClientID, - FamilyID: "fam-2", - ExpiresAt: time.Now().Add(time.Hour), - })) + pair := seedPair(t, store, newClientPair("raw-refresh", "fam-2")) rec := httptest.NewRecorder() srv.RevokeHandler().ServeHTTP(rec, formRequest("/oauth2/revoke", url.Values{"token": {"raw-refresh"}}, true)) assert.Equal(t, http.StatusOK, rec.Code) + + rt, err := store.LookupRefreshToken(ctx, pair.Refresh.TokenHash) + require.NoError(t, err) + assert.True(t, rt.Consumed, "the refresh token's family must be revoked") + + _, err = store.LookupAccessToken(ctx, pair.Access.TokenHash) + assert.Error(t, err, "the family's access token must be revoked") }) } @@ -376,14 +378,14 @@ func TestIntrospectEndpoint(t *testing.T) { t.Parallel() srv, store := newTestServer(t) - require.NoError(t, store.SaveAccessToken(context.Background(), &oauth2.AccessToken{ + require.NoError(t, store.SaveTokenPair(context.Background(), &oauth2.TokenPair{Access: oauth2.AccessToken{ TokenHash: oauth2.HashToken(nil, "live-at"), ClientID: testClientID, Subject: "user-1", Scope: "api:read", IssuedAt: time.Now().Add(-time.Minute), ExpiresAt: time.Now().Add(time.Hour), - })) + }})) body := introspect(t, srv, "live-at") assert.True(t, body["active"].(bool)) @@ -395,11 +397,11 @@ func TestIntrospectEndpoint(t *testing.T) { t.Parallel() srv, store := newTestServer(t) - require.NoError(t, store.SaveAccessToken(context.Background(), &oauth2.AccessToken{ + require.NoError(t, store.SaveTokenPair(context.Background(), &oauth2.TokenPair{Access: oauth2.AccessToken{ TokenHash: oauth2.HashToken(nil, "dead-at"), ClientID: testClientID, ExpiresAt: time.Now().Add(-time.Hour), - })) + }})) assert.False(t, introspect(t, srv, "dead-at")["active"].(bool)) }) @@ -407,11 +409,7 @@ func TestIntrospectEndpoint(t *testing.T) { t.Parallel() srv, store := newTestServer(t) - require.NoError(t, store.SaveRefreshToken(context.Background(), &oauth2.RefreshToken{ - TokenHash: oauth2.HashToken(nil, "live-rt"), - ClientID: testClientID, - ExpiresAt: time.Now().Add(time.Hour), - })) + seedPair(t, store, newClientPair("live-rt", "fam-live")) body := introspect(t, srv, "live-rt") assert.True(t, body["active"].(bool)) @@ -422,12 +420,7 @@ func TestIntrospectEndpoint(t *testing.T) { t.Parallel() srv, store := newTestServer(t) - require.NoError(t, store.SaveRefreshToken(context.Background(), &oauth2.RefreshToken{ - TokenHash: oauth2.HashToken(nil, "used-rt"), - ClientID: testClientID, - ExpiresAt: time.Now().Add(time.Hour), - Consumed: true, - })) + seedRotated(t, store, newClientPair("used-rt", "fam-used"), newClientPair("used-rt-next", "fam-used")) assert.False(t, introspect(t, srv, "used-rt")["active"].(bool)) }) } diff --git a/oauth2/storage.go b/oauth2/storage.go index 829eedb..82a5db2 100644 --- a/oauth2/storage.go +++ b/oauth2/storage.go @@ -4,7 +4,23 @@ package oauth2 -import "context" +import ( + "context" + "time" +) + +// FamilyRevocationRetention is the minimum time a [Storage] keeps a token +// family's revoked state after the revocation; it is never shorter than +// the expiry of any token of the family. It bounds a revocation recorded +// before the family's first token is issued. +const FamilyRevocationRetention = 24 * time.Hour + +// MaxFamilyIDLength is the longest token-family identifier, in bytes, a +// [Storage] accepts: [TokenPair.Validate] and every RevokeRefreshFamily +// implementation refuse a longer one with invalid_request, so no backend +// can silently truncate two identifiers into the same family (the SQL +// store keeps them in a VARCHAR(64) column). +const MaxFamilyIDLength = 64 // AuthorizationCodeStore persists single-use authorization codes. The // Consume* operation MUST be atomic: a code may be returned successfully @@ -22,42 +38,135 @@ type AuthorizationCodeStore interface { ConsumeAuthorizationCode(ctx context.Context, codeHash string) (*AuthorizationCode, error) } -// AccessTokenStore persists access tokens. Implementations MUST store hashes -// (the canonical hash function is [HashToken]). +// AccessTokenStore is the access-token view of a [Storage] — the part a +// resource server validating opaque tokens needs. It is a consumer view, +// not a separately implementable half: see [Storage]. Implementations MUST +// store hashes (the canonical hash function is [HashToken]). type AccessTokenStore interface { - SaveAccessToken(ctx context.Context, t *AccessToken) error // LookupAccessToken returns the token record matching tokenHash, or - // nil + ErrInvalidGrant when none matches. + // nil + an invalid_grant [*Error] when none matches or when the + // token's family is revoked. Tokens outside any family are never + // affected by family revocation. LookupAccessToken(ctx context.Context, tokenHash string) (*AccessToken, error) + // RevokeAccessToken deletes the token matching tokenHash. Revoking an + // unknown token is a no-op. RevokeAccessToken(ctx context.Context, tokenHash string) error } -// RefreshTokenStore persists refresh tokens. The rotation operation MUST -// be atomic: rotating a token consumed elsewhere MUST fail with -// [ErrRefreshTokenReused] and trigger family revocation. +// RefreshTokenStore is the refresh-token view of a [Storage]. It is a +// consumer view, not a separately implementable half: see [Storage]. type RefreshTokenStore interface { - SaveRefreshToken(ctx context.Context, t *RefreshToken) error - // RotateRefreshToken atomically marks oldHash as consumed and persists - // next as the active refresh token. Returns the new TokenPair on - // success, ErrRefreshTokenReused when oldHash was already consumed - // (in which case the implementation MUST also call - // [RevokeRefreshFamily] for the offending FamilyID before returning). - RotateRefreshToken(ctx context.Context, oldHash string, next *RefreshToken) error // LookupRefreshToken returns the refresh-token record matching - // tokenHash, or nil + ErrInvalidGrant when none matches. Consumed - // tokens MUST be returned with Consumed=true so the caller can treat - // them as reuse. + // tokenHash, or nil + an invalid_grant [*Error] when none matches. A + // rotated token, or a token of a revoked family, comes back with + // Consumed=true: the caller treats presenting it as reuse. LookupRefreshToken(ctx context.Context, tokenHash string) (*RefreshToken, error) - // RevokeRefreshFamily marks every refresh token in familyID as - // consumed AND revokes every access token whose FamilyID matches. + // RotateRefreshToken atomically consumes the refresh token oldHash and + // persists next — an access token and a refresh token, both in + // oldHash's family — or changes nothing (I3). + // + // A malformed next — nil, without a refresh token, or failing + // [TokenPair.Validate] — is refused with invalid_request before + // anything is read. The other errors come in this order: invalid_grant + // when oldHash is unknown; invalid_request when next declares another + // family than oldHash's; [ErrTokenFamilyRevoked] when the family is + // revoked; [ErrRefreshTokenReused] when oldHash was already consumed. + // On reuse the family is revoked before the return; when that + // revocation fails, the error also wraps [ErrTokenFamilyRevocationFailed] + // and its cause, and the family MUST be treated as active (I4). Callers + // therefore test errors.Is(err, ErrTokenFamilyRevocationFailed) + // themselves: the OAuth2 code stays invalid_grant either way. + // + // Retrying a rotation that was already applied, with the identical + // next, MAY succeed: callers always pass a freshly generated next. + RotateRefreshToken(ctx context.Context, oldHash string, next *TokenPair) error + // RevokeRefreshFamily records familyID as revoked — even when no token + // of the family exists yet — so that I1 and I2 hold once it returns + // nil. It is idempotent (I5). A failure wraps + // [ErrTokenFamilyRevocationFailed]; its outcome may be unknown, so the + // caller treats the family as active and retries (I6). An empty ID, or + // one longer than [MaxFamilyIDLength], is rejected with invalid_request + // (I7). RevokeRefreshFamily(ctx context.Context, familyID string) error } -// Storage groups the per-aspect interfaces. Implementations MAY decide to -// satisfy individual sub-interfaces with different backends (e.g. SQL for -// authorization codes, Redis for tokens). +// Storage is the persistence contract of the authorization server. +// +// The token operations — [AccessTokenStore], [RefreshTokenStore] and +// SaveTokenPair — MUST share one backend: SaveTokenPair and +// RotateRefreshToken write access and refresh tokens atomically, and every +// lookup consults the same token-family state. AccessTokenStore and +// RefreshTokenStore are consumer views, not halves that may be implemented +// on different backends; only the [AuthorizationCodeStore] may live +// elsewhere. [ServerConfig.Storage] and the Storage of every grant MUST be +// the same store. +// +// # Token families +// +// The access and refresh tokens issued from one authorization share a +// non-empty FamilyID: they form a family. client_credentials and implicit +// access tokens belong to none and are never affected by a family +// revocation. A store keeps one authoritative state per family — active +// or revoked — and serializes every write into the family on it, so a +// revocation never has to find and purge tokens: lookups honor the state. +// +// A revocation R of family F is complete once RevokeRefreshFamily(F) +// returned nil, or once a RotateRefreshToken reporting reuse returned an +// error that does not match [ErrTokenFamilyRevocationFailed]. "After R" +// means invoked after R returned, while F is retained: +// +// - I1, visibility: after R, LookupAccessToken fails with invalid_grant +// for every access token of F, and LookupRefreshToken returns every +// refresh token of F with Consumed set (or fails with invalid_grant). +// - I2, gating: every SaveTokenPair or RotateRefreshToken into F is +// ordered either before R — I1 then covers its tokens — or after it, +// failing with [ErrTokenFamilyRevoked] (or [ErrRefreshTokenReused] for +// a consumed token) without persisting anything. A lookup concurrent +// with R may still see F active: it is ordered before R. +// - I3, atomic writes: SaveTokenPair and RotateRefreshToken apply all +// their writes or none. A failed call persists no token and consumes +// nothing; the one exception is reuse, which only records F as +// revoked. +// - I4, reuse: RotateRefreshToken on a consumed token of a non-revoked +// family fails with [ErrRefreshTokenReused]; one of an already revoked +// family may fail with [ErrTokenFamilyRevoked] instead (both are +// invalid_grant). Unless the error also matches +// [ErrTokenFamilyRevocationFailed], F was revoked before the return; +// otherwise it also wraps the cause, and F MUST be treated as active. +// - I5, idempotence and monotonicity: a revoked family never becomes +// active again, and revoking it again returns nil and changes nothing +// observable. F is retained at least until every token of F has +// expired and for [FamilyRevocationRetention] after its first +// revocation, so a revocation recorded before the first issuance still +// refuses it. +// - I6, failure: RevokeRefreshFamily returns nil or an error wrapping +// [ErrTokenFamilyRevocationFailed] (invalid_request for an empty or +// over-long ID, I7). A failed call applies nothing, but its outcome +// can be unknown (a lost acknowledgement): the caller treats F as +// active and retries, which I5 makes safe. +// - I7, isolation: nothing done to F affects another family or a token +// outside any family; RevokeRefreshFamily("") is rejected. +// - I8, fail closed: a store that can lose family state (expiry, +// eviction, cleanup) treats a token whose family it no longer knows as +// a token of a revoked family. This protects lookups only: the next +// issuance into a family whose revoked state was lost records it +// active again, reviving its tokens, so a store MUST NOT lose a +// revoked family's state within its retention (I5). +// +// A store returns the sentinels themselves or wraps them with %w: an +// [*Error] has no Is method, so a WithDescription or WithCause copy no +// longer matches its sentinel. Every implementation MUST pass the shared +// oauth2/storetest conformance suite. type Storage interface { AuthorizationCodeStore AccessTokenStore RefreshTokenStore + + // SaveTokenPair atomically persists a newly issued pair: both tokens + // or neither (I3). A pair whose access token carries a FamilyID joins + // that family, creating it when unknown, and fails with + // [ErrTokenFamilyRevoked] once the family is revoked (I2); a pair + // outside any family carries no refresh token and is never gated. A + // pair failing [TokenPair.Validate] is refused with invalid_request. + SaveTokenPair(ctx context.Context, pair *TokenPair) error } diff --git a/oauth2/storage/memory/memory.go b/oauth2/storage/memory/memory.go index 47a5456..212f63f 100644 --- a/oauth2/storage/memory/memory.go +++ b/oauth2/storage/memory/memory.go @@ -9,11 +9,13 @@ // // All operations are guarded by a single sync.Mutex; the resulting // throughput is fine for tens of thousands of req/s but the structure is -// optimized for clarity, not for scale. +// optimized for clarity, not for scale. Nothing is ever purged: expired +// tokens and revoked families stay in memory for the process lifetime. package memory import ( "context" + "fmt" "sync" "github.com/hyperscale-stack/security/oauth2" @@ -21,21 +23,27 @@ import ( // Store is an in-memory [oauth2.Storage]. The zero value is unusable; // build one with [New]. +// +// The mutex is the linearization point of the token-family contract: every +// issuance, rotation, revocation and lookup checks or records the revoked +// families under it. A family needs no record while it is active, and the +// store never loses one once revoked, so the I8 fail-closed rule of +// [oauth2.Storage] never comes into play. type Store struct { - mu sync.Mutex - codes map[string]oauth2.AuthorizationCode - access map[string]oauth2.AccessToken - refresh map[string]oauth2.RefreshToken - families map[string][]string // familyID -> refresh-token hashes (for revocation) + mu sync.Mutex + codes map[string]oauth2.AuthorizationCode + access map[string]oauth2.AccessToken + refresh map[string]oauth2.RefreshToken + revoked map[string]struct{} // revoked token families } // New returns a fresh [Store]. func New() *Store { return &Store{ - codes: make(map[string]oauth2.AuthorizationCode), - access: make(map[string]oauth2.AccessToken), - refresh: make(map[string]oauth2.RefreshToken), - families: make(map[string][]string), + codes: make(map[string]oauth2.AuthorizationCode), + access: make(map[string]oauth2.AccessToken), + refresh: make(map[string]oauth2.RefreshToken), + revoked: make(map[string]struct{}), } } @@ -75,21 +83,27 @@ func (s *Store) ConsumeAuthorizationCode(_ context.Context, codeHash string) (*o return &cp, nil } -// SaveAccessToken implements [oauth2.AccessTokenStore]. -func (s *Store) SaveAccessToken(_ context.Context, t *oauth2.AccessToken) error { +// SaveTokenPair implements [oauth2.Storage]: under the store's mutex it +// refuses a pair joining a revoked family, then persists both tokens. +func (s *Store) SaveTokenPair(_ context.Context, pair *oauth2.TokenPair) error { + if err := pair.Validate(); err != nil { + return fmt.Errorf("memory: save token pair: %w", err) + } + s.mu.Lock() defer s.mu.Unlock() - if t.TokenHash == "" { - return oauth2.ErrInvalidRequest.WithDescription("storage: empty access token hash") + if s.isRevoked(pair.Access.FamilyID) { + return oauth2.ErrTokenFamilyRevoked } - s.access[t.TokenHash] = *t + s.persist(pair) return nil } -// LookupAccessToken implements [oauth2.AccessTokenStore]. +// LookupAccessToken implements [oauth2.AccessTokenStore]. A token of a +// revoked family is reported as [oauth2.ErrTokenFamilyRevoked]. func (s *Store) LookupAccessToken(_ context.Context, tokenHash string) (*oauth2.AccessToken, error) { s.mu.Lock() defer s.mu.Unlock() @@ -99,6 +113,10 @@ func (s *Store) LookupAccessToken(_ context.Context, tokenHash string) (*oauth2. return nil, oauth2.ErrInvalidGrant.WithDescription("access token not found") } + if s.isRevoked(t.FamilyID) { + return nil, oauth2.ErrTokenFamilyRevoked + } + cp := t return &cp, nil @@ -114,27 +132,9 @@ func (s *Store) RevokeAccessToken(_ context.Context, tokenHash string) error { return nil } -// SaveRefreshToken implements [oauth2.RefreshTokenStore]. The token is -// registered in its family so that subsequent revocation can iterate every -// sibling. -func (s *Store) SaveRefreshToken(_ context.Context, t *oauth2.RefreshToken) error { - s.mu.Lock() - defer s.mu.Unlock() - - if t.TokenHash == "" { - return oauth2.ErrInvalidRequest.WithDescription("storage: empty refresh token hash") - } - - s.refresh[t.TokenHash] = *t - - if t.FamilyID != "" { - s.families[t.FamilyID] = append(s.families[t.FamilyID], t.TokenHash) - } - - return nil -} - -// LookupRefreshToken implements [oauth2.RefreshTokenStore]. +// LookupRefreshToken implements [oauth2.RefreshTokenStore]. A token of a +// revoked family — or outside any family, which no valid pair carries — +// comes back consumed. func (s *Store) LookupRefreshToken(_ context.Context, tokenHash string) (*oauth2.RefreshToken, error) { s.mu.Lock() defer s.mu.Unlock() @@ -145,69 +145,99 @@ func (s *Store) LookupRefreshToken(_ context.Context, tokenHash string) (*oauth2 } cp := t + if cp.FamilyID == "" || s.isRevoked(cp.FamilyID) { + cp.Consumed = true + } return &cp, nil } -// RotateRefreshToken implements [oauth2.RefreshTokenStore]. The atomic -// sequence under the store's mutex is: -// -// 1. Look up the old token; if missing -> ErrInvalidGrant. -// 2. If the old token is already consumed -> revoke the entire family and -// return [oauth2.ErrRefreshTokenReused] (BCP §8.10.3). -// 3. Mark the old token as consumed, save the new token, register it in -// the same family. -func (s *Store) RotateRefreshToken(ctx context.Context, oldHash string, next *oauth2.RefreshToken) error { +// RotateRefreshToken implements [oauth2.RefreshTokenStore]. Under the +// store's mutex it decides, in the contract's order: unknown token +// (invalid_grant), next in another family (invalid_request), revoked +// family ([oauth2.ErrTokenFamilyRevoked]), consumed token (reuse: the +// family is revoked in the same critical section, which cannot fail, and +// [oauth2.ErrRefreshTokenReused] is returned). Otherwise it consumes the +// old token and persists next. +func (s *Store) RotateRefreshToken(_ context.Context, oldHash string, next *oauth2.TokenPair) error { + if err := validateRotation(next); err != nil { + return err + } + s.mu.Lock() + defer s.mu.Unlock() old, ok := s.refresh[oldHash] - if !ok { - s.mu.Unlock() + switch { + case !ok: return oauth2.ErrInvalidGrant.WithDescription("refresh token not found") - } - - if old.Consumed { - family := old.FamilyID - - s.mu.Unlock() - - _ = s.RevokeRefreshFamily(ctx, family) + case old.FamilyID != next.Access.FamilyID: + return oauth2.ErrInvalidRequest.WithDescription("storage: rotation into another token family") + case s.isRevoked(old.FamilyID): + return oauth2.ErrTokenFamilyRevoked + case old.Consumed: + s.revoked[old.FamilyID] = struct{}{} return oauth2.ErrRefreshTokenReused } old.Consumed = true s.refresh[oldHash] = old - s.refresh[next.TokenHash] = *next - - if next.FamilyID != "" { - s.families[next.FamilyID] = append(s.families[next.FamilyID], next.TokenHash) - } - - s.mu.Unlock() + s.persist(next) return nil } -// RevokeRefreshFamily implements [oauth2.RefreshTokenStore]. Every refresh -// token in the family is marked consumed and every access token whose -// FamilyID matches is removed. +// RevokeRefreshFamily implements [oauth2.RefreshTokenStore]: it records +// the family as revoked, whether or not any of its tokens exists. func (s *Store) RevokeRefreshFamily(_ context.Context, familyID string) error { + switch { + case familyID == "": + return oauth2.ErrInvalidRequest.WithDescription("storage: empty token family id") + case len(familyID) > oauth2.MaxFamilyIDLength: + return oauth2.ErrInvalidRequest.WithDescription("storage: token family id too long") + } + s.mu.Lock() defer s.mu.Unlock() - for _, hash := range s.families[familyID] { - if t, ok := s.refresh[hash]; ok { - t.Consumed = true - s.refresh[hash] = t - } + s.revoked[familyID] = struct{}{} + + return nil +} + +// isRevoked reports whether familyID is a revoked family; tokens outside +// any family never are. The caller holds the mutex. +func (s *Store) isRevoked(familyID string) bool { + if familyID == "" { + return false + } + + _, revoked := s.revoked[familyID] + + return revoked +} + +// persist writes the tokens of pair. The caller holds the mutex. +func (s *Store) persist(pair *oauth2.TokenPair) { + s.access[pair.Access.TokenHash] = pair.Access + + if pair.Refresh != nil { + s.refresh[pair.Refresh.TokenHash] = *pair.Refresh + } +} + +// validateRotation refuses a malformed rotation target before the store is +// touched: nil, failing [oauth2.TokenPair.Validate], or without the refresh +// token a rotation must issue. +func validateRotation(next *oauth2.TokenPair) error { + if err := next.Validate(); err != nil { + return fmt.Errorf("memory: rotate refresh token: %w", err) } - for hash, t := range s.access { - if t.FamilyID == familyID { - delete(s.access, hash) - } + if next.Refresh == nil { + return oauth2.ErrInvalidRequest.WithDescription("storage: rotation without a refresh token") } return nil diff --git a/oauth2/store/redis/codec.go b/oauth2/store/redis/codec.go index 8df83f9..d9f7d56 100644 --- a/oauth2/store/redis/codec.go +++ b/oauth2/store/redis/codec.go @@ -26,8 +26,9 @@ func marshalJSON(v any) ([]byte, error) { // The DTO types below are the on-wire JSON shapes persisted in Redis. They // deliberately omit the raw Token / Code fields — only hashes are keys, and // the raw secret is never stored. Timestamps are Unix seconds for compact, -// unambiguous encoding. The `consumed` field name is load-bearing: the -// rotate-refresh Lua script reads it via cjson. +// unambiguous encoding. The refresh token's `family_id` and `consumed` +// field names are load-bearing: the rotation Lua script reads both via +// cjson, and re-encodes the record when it consumes the token. type codeDTO struct { ClientID string `json:"client_id"` @@ -166,3 +167,20 @@ func ttlUntil(t time.Time) time.Duration { return d } + +// ttlMillis is ttlUntil in milliseconds, the unit of the scripts' PX TTLs. +func ttlMillis(t time.Time) int64 { + return ttlUntil(t).Milliseconds() +} + +// familyTTL is the TTL, in milliseconds, a family key needs to outlive +// every token of pair. +func familyTTL(pair *oauth2.TokenPair) int64 { + ttl := ttlMillis(pair.Access.ExpiresAt) + + if pair.Refresh != nil { + ttl = max(ttl, ttlMillis(pair.Refresh.ExpiresAt)) + } + + return ttl +} diff --git a/oauth2/store/redis/doc.go b/oauth2/store/redis/doc.go index 91c71df..d28a4e4 100644 --- a/oauth2/store/redis/doc.go +++ b/oauth2/store/redis/doc.go @@ -3,8 +3,48 @@ // license that can be found in the LICENSE file. // Package redisstore is a Redis implementation of oauth2.Storage. Atomicity -// of ConsumeAuthorizationCode and RotateRefreshToken is guaranteed by Lua -// scripts loaded via EVALSHA (with EVAL fallback). +// of ConsumeAuthorizationCode and of the token-family operations is +// guaranteed by Lua scripts loaded via EVALSHA (with EVAL fallback). +// +// # Key layout +// +// Every key starts with the store prefix (WithKeyPrefix, "oauth2:" by +// default): +// +// - code:{hash}: an authorization code (JSON), until it expires. +// - at:{hash}, rt:{hash}: an access or refresh token (JSON), until it +// expires. +// - fam:{id}: the state of a token family, "active" or "revoked", kept at +// least as long as every token of the family and, once revoked, for +// oauth2.FamilyRevocationRetention. +// +// # Deployment requirements +// +// The token-family guarantees of oauth2.Storage hold under these +// conditions: +// +// - Redis 6.0 or later: the rotation script uses SET … KEEPTTL, which an +// older server refuses at the script's first write — failing closed. +// - A standalone server, or the primary of a Sentinel deployment. Redis +// Cluster is not supported: the scripts touch keys of different hash +// slots (CROSSSLOT). +// - Every command reaches the primary: no replica reads, and no +// client-side caching on the store's client. Either could answer from +// before a revocation. +// - Durability is the server's: a revocation acknowledged by a primary +// that fails before persisting (AOF) or replicating it can be lost in a +// failover, reviving its tokens. Choose the persistence and failover +// settings accordingly. +// - maxmemory-policy noeviction. Every family key carries a TTL, so the +// volatile-* policies may evict it as well as the allkeys-* ones. A +// lost "active" key only logs users out — the store treats the tokens +// of an unknown family as revoked — but a lost "revoked" key, within +// its retention, is recorded active again by the next issuance into +// the family, reviving its tokens. +// +// go-redis resends a command whose reply was lost; the token-family +// scripts are safe to run twice, and a resent rotation is not mistaken for +// a reuse. // // Allowed dependencies: // - github.com/hyperscale-stack/security/oauth2 diff --git a/oauth2/store/redis/store.go b/oauth2/store/redis/store.go index f4b3114..9bd29df 100644 --- a/oauth2/store/redis/store.go +++ b/oauth2/store/redis/store.go @@ -14,9 +14,28 @@ import ( ) // Store is a Redis-backed [oauth2.Storage]. Single-use atomicity -// (ConsumeAuthorizationCode, RotateRefreshToken) is provided by Lua -// scripts: a Redis Lua script runs to completion without interleaving -// other commands, so the read-modify-write sequence is indivisible. +// (ConsumeAuthorizationCode) and the token-family contract of +// [oauth2.Storage] rest on Lua scripts: a Redis script runs to completion +// without interleaving other commands, so each read-modify-write sequence +// is indivisible. +// +// Each token family has one key, {prefix}fam:{id}, holding "active" or +// "revoked": the point every write into the family serializes on. +// +// - SaveTokenPair runs one script that refuses a family in any state but +// unknown or "active", records it active and writes both tokens. +// - RotateRefreshToken runs one script that checks the old token and the +// family — any state but "active" refuses the rotation — then consumes +// the old token and writes the new pair. On reuse the script writes +// nothing and the store runs the revocation script next: whatever +// commits in between is ordered before the revocation, which covers it. +// - RevokeRefreshFamily runs one script that writes that single key, +// whether or not any token of the family exists. +// - Lookups read the token, then its family key: unless that key holds +// exactly "active", the token is reported revoked. +// +// The family key lives at least as long as every token of the family, and +// for [oauth2.FamilyRevocationRetention] after a revocation. type Store struct { rdb redis.UniversalClient prefix string @@ -48,8 +67,20 @@ func New(rdb redis.UniversalClient, opts ...Option) (*Store, error) { func (s *Store) codeKey(hash string) string { return s.prefix + "code:" + hash } func (s *Store) atKey(hash string) string { return s.prefix + "at:" + hash } func (s *Store) rtKey(hash string) string { return s.prefix + "rt:" + hash } -func (s *Store) famRTKey(fam string) string { return s.prefix + "famrt:" + fam } -func (s *Store) famATKey(fam string) string { return s.prefix + "famat:" + fam } +func (s *Store) famKey(id string) string { return s.prefix + "fam:" + id } + +// familyActive is the state of a family key that lets its tokens be used; +// the scripts write it, and "revoked", as Lua literals. +const familyActive = "active" + +// The replies of the token scripts. +const ( + resultOK = "ok" + resultNotFound = "notfound" + resultMismatch = "mismatch" + resultRevoked = "revoked" + resultReused = "reused" +) // --- Lua scripts --------------------------------------------------------- @@ -62,31 +93,73 @@ redis.call('DEL', KEYS[1]) return v `) -// rotateRefreshScript atomically rotates a refresh token. +// issuePairScript persists a newly issued pair into a family. It refuses a +// family whose key holds anything but "active" — a missing key is a new +// family — records the family active for at least the pair's lifetime, +// then writes the tokens. // -// KEYS[1] old refresh-token key -// KEYS[2] new refresh-token key -// KEYS[3] family set of refresh-token hashes -// ARGV[1] new refresh-token JSON -// ARGV[2] new refresh-token TTL (seconds) -// ARGV[3] new refresh-token hash +// KEYS[1] family; KEYS[2] access token; KEYS[3] refresh token (optional) +// ARGV[1] family TTL (ms); ARGV[2] access JSON; ARGV[3] access TTL (ms); +// ARGV[4] refresh JSON; ARGV[5] refresh TTL (ms) — with KEYS[3] only // -// Returns: 'ok' on success, 'notfound' when the old key is absent, -// 'reused' when the old token was already consumed. -var rotateRefreshScript = redis.NewScript(` -local old = redis.call('GET', KEYS[1]) -if not old then return 'notfound' end -local decoded = cjson.decode(old) -if decoded.consumed then return 'reused' end -decoded.consumed = true -local ttl = redis.call('PTTL', KEYS[1]) -if ttl and ttl > 0 then - redis.call('SET', KEYS[1], cjson.encode(decoded), 'PX', ttl) -else - redis.call('SET', KEYS[1], cjson.encode(decoded)) +// Returns 'ok', or 'revoked' without writing anything. +var issuePairScript = redis.NewScript(` +local state = redis.call('GET', KEYS[1]) +if state and state ~= 'active' then return 'revoked' end +if redis.call('PTTL', KEYS[1]) < tonumber(ARGV[1]) then + redis.call('SET', KEYS[1], 'active', 'PX', ARGV[1]) +end +redis.call('SET', KEYS[2], ARGV[2], 'PX', ARGV[3]) +if KEYS[3] then redis.call('SET', KEYS[3], ARGV[4], 'PX', ARGV[5]) end +return 'ok' +`) + +// rotatePairScript atomically consumes a refresh token and persists the +// pair replacing it. Every check runs before the first write, so a refusal +// — or a corrupt old token, which aborts the script — writes nothing. +// +// KEYS[1] old refresh token; KEYS[2] family; +// KEYS[3] new refresh token; KEYS[4] new access token +// ARGV[1] family ID; ARGV[2] refresh JSON; ARGV[3] refresh TTL (ms); +// ARGV[4] access JSON; ARGV[5] access TTL (ms); ARGV[6] family TTL (ms) +// +// Returns 'ok', 'notfound', 'mismatch' (the old token is in another +// family), 'revoked' (the family key is not 'active': rotations never +// create a family) or 'reused'. A consumed old token whose replacement +// already exists is a resend of this very rotation — the new refresh key +// comes from 256 random bits, only an earlier run of the same call can +// have written it — so it answers 'ok' rather than 'reused': go-redis +// resends a command whose reply was lost, which must not revoke the family +// the rotation just extended. +var rotatePairScript = redis.NewScript(` +local raw = redis.call('GET', KEYS[1]) +if not raw then return 'notfound' end +local old = cjson.decode(raw) +if old.family_id ~= ARGV[1] then return 'mismatch' end +if redis.call('GET', KEYS[2]) ~= 'active' then return 'revoked' end +if old.consumed then + if redis.call('EXISTS', KEYS[3]) == 1 then return 'ok' end + return 'reused' +end +old.consumed = true +redis.call('SET', KEYS[1], cjson.encode(old), 'KEEPTTL') +redis.call('SET', KEYS[3], ARGV[2], 'PX', ARGV[3]) +redis.call('SET', KEYS[4], ARGV[4], 'PX', ARGV[5]) +if redis.call('PTTL', KEYS[2]) < tonumber(ARGV[6]) then + redis.call('SET', KEYS[2], 'active', 'PX', ARGV[6]) end -redis.call('SET', KEYS[2], ARGV[1], 'EX', tonumber(ARGV[2])) -redis.call('SADD', KEYS[3], ARGV[3]) +return 'ok' +`) + +// revokeFamilyScript records a family revoked: its only write sets the +// family key, keeping the longer of its current TTL and the retention. +// +// KEYS[1] family; ARGV[1] minimum retention (ms) +var revokeFamilyScript = redis.NewScript(` +local ttl = tonumber(ARGV[1]) +local current = redis.call('PTTL', KEYS[1]) +if current > ttl then ttl = current end +redis.call('SET', KEYS[1], 'revoked', 'PX', ttl) return 'ok' `) @@ -136,34 +209,58 @@ func (s *Store) ConsumeAuthorizationCode(ctx context.Context, codeHash string) ( return code, nil } -// --- access tokens ------------------------------------------------------- +// --- access and refresh tokens ------------------------------------------- -// SaveAccessToken implements [oauth2.AccessTokenStore]. -func (s *Store) SaveAccessToken(ctx context.Context, t *oauth2.AccessToken) error { - if t.TokenHash == "" { - return oauth2.ErrInvalidRequest.WithDescription("redisstore: empty access token hash") +// SaveTokenPair implements [oauth2.Storage]. A pair of a family goes +// through issuePairScript; a pair outside any family is one SET. +func (s *Store) SaveTokenPair(ctx context.Context, pair *oauth2.TokenPair) error { + if err := pair.Validate(); err != nil { + return fmt.Errorf("redisstore: save token pair: %w", err) } - payload, err := encodeAccess(t) + access, err := encodeAccess(&pair.Access) if err != nil { return fmt.Errorf("redisstore: encode access token: %w", err) } - pipe := s.rdb.TxPipeline() - pipe.Set(ctx, s.atKey(t.TokenHash), payload, ttlUntil(t.ExpiresAt)) + if pair.Access.FamilyID == "" { + if err := s.rdb.Set(ctx, s.atKey(pair.Access.TokenHash), access, ttlUntil(pair.Access.ExpiresAt)).Err(); err != nil { + return fmt.Errorf("redisstore: save access token: %w", err) + } - if t.FamilyID != "" { - pipe.SAdd(ctx, s.famATKey(t.FamilyID), t.TokenHash) + return nil } - if _, err := pipe.Exec(ctx); err != nil { - return fmt.Errorf("redisstore: save access token: %w", err) + keys := []string{s.famKey(pair.Access.FamilyID), s.atKey(pair.Access.TokenHash)} + args := []any{familyTTL(pair), access, ttlMillis(pair.Access.ExpiresAt)} + + if pair.Refresh != nil { + refresh, err := encodeRefresh(pair.Refresh) + if err != nil { + return fmt.Errorf("redisstore: encode refresh token: %w", err) + } + + keys = append(keys, s.rtKey(pair.Refresh.TokenHash)) + args = append(args, refresh, ttlMillis(pair.Refresh.ExpiresAt)) } - return nil + res, err := issuePairScript.Run(ctx, s.rdb, keys, args...).Text() + if err != nil { + return fmt.Errorf("redisstore: save token pair: %w", err) + } + + switch res { + case resultOK: + return nil + case resultRevoked: + return oauth2.ErrTokenFamilyRevoked + default: + return fmt.Errorf("redisstore: save token pair: unexpected script result %q", res) + } } -// LookupAccessToken implements [oauth2.AccessTokenStore]. +// LookupAccessToken implements [oauth2.AccessTokenStore]. A token whose +// family key is not "active" is reported as [oauth2.ErrTokenFamilyRevoked]. func (s *Store) LookupAccessToken(ctx context.Context, tokenHash string) (*oauth2.AccessToken, error) { raw, err := s.rdb.Get(ctx, s.atKey(tokenHash)).Bytes() if err != nil { @@ -179,6 +276,15 @@ func (s *Store) LookupAccessToken(ctx context.Context, tokenHash string) (*oauth return nil, fmt.Errorf("redisstore: decode access token: %w", err) } + active, err := s.familyActive(ctx, t.FamilyID) + if err != nil { + return nil, err + } + + if !active { + return nil, oauth2.ErrTokenFamilyRevoked + } + return t, nil } @@ -191,34 +297,9 @@ func (s *Store) RevokeAccessToken(ctx context.Context, tokenHash string) error { return nil } -// --- refresh tokens ------------------------------------------------------ - -// SaveRefreshToken implements [oauth2.RefreshTokenStore]. -func (s *Store) SaveRefreshToken(ctx context.Context, t *oauth2.RefreshToken) error { - if t.TokenHash == "" { - return oauth2.ErrInvalidRequest.WithDescription("redisstore: empty refresh token hash") - } - - payload, err := encodeRefresh(t) - if err != nil { - return fmt.Errorf("redisstore: encode refresh token: %w", err) - } - - pipe := s.rdb.TxPipeline() - pipe.Set(ctx, s.rtKey(t.TokenHash), payload, ttlUntil(t.ExpiresAt)) - - if t.FamilyID != "" { - pipe.SAdd(ctx, s.famRTKey(t.FamilyID), t.TokenHash) - } - - if _, err := pipe.Exec(ctx); err != nil { - return fmt.Errorf("redisstore: save refresh token: %w", err) - } - - return nil -} - -// LookupRefreshToken implements [oauth2.RefreshTokenStore]. +// LookupRefreshToken implements [oauth2.RefreshTokenStore]. A token whose +// family key is not "active" comes back consumed — and so does a token +// stored without a family, which no valid pair carries. func (s *Store) LookupRefreshToken(ctx context.Context, tokenHash string) (*oauth2.RefreshToken, error) { raw, err := s.rdb.Get(ctx, s.rtKey(tokenHash)).Bytes() if err != nil { @@ -234,109 +315,149 @@ func (s *Store) LookupRefreshToken(ctx context.Context, tokenHash string) (*oaut return nil, fmt.Errorf("redisstore: decode refresh token: %w", err) } + active, err := s.familyActive(ctx, t.FamilyID) + if err != nil { + return nil, err + } + + t.Consumed = t.Consumed || t.FamilyID == "" || !active + return t, nil } -// RotateRefreshToken implements [oauth2.RefreshTokenStore] via the -// rotateRefresh Lua script — the consumed-flag check and the new-token -// insert happen atomically. Reuse of a consumed token returns -// [oauth2.ErrRefreshTokenReused] and revokes the whole family. -func (s *Store) RotateRefreshToken(ctx context.Context, oldHash string, next *oauth2.RefreshToken) error { - payload, err := encodeRefresh(next) +// RotateRefreshToken implements [oauth2.RefreshTokenStore] via +// rotatePairScript. On reuse the script writes nothing and the store then +// revokes the family; a failure of that revocation is reported as +// [oauth2.ErrRefreshTokenReused] joined with +// [oauth2.ErrTokenFamilyRevocationFailed] and its cause. +func (s *Store) RotateRefreshToken(ctx context.Context, oldHash string, next *oauth2.TokenPair) error { + if err := validateRotation(next); err != nil { + return err + } + + refresh, err := encodeRefresh(next.Refresh) if err != nil { - return fmt.Errorf("redisstore: encode rotated refresh token: %w", err) + return fmt.Errorf("redisstore: encode refresh token: %w", err) + } + + access, err := encodeAccess(&next.Access) + if err != nil { + return fmt.Errorf("redisstore: encode access token: %w", err) } + familyID := next.Access.FamilyID keys := []string{ - s.rtKey(oldHash), - s.rtKey(next.TokenHash), - s.famRTKey(next.FamilyID), + s.rtKey(oldHash), s.famKey(familyID), + s.rtKey(next.Refresh.TokenHash), s.atKey(next.Access.TokenHash), + } + args := []any{ + familyID, + refresh, ttlMillis(next.Refresh.ExpiresAt), + access, ttlMillis(next.Access.ExpiresAt), + familyTTL(next), } - args := []any{payload, int64(ttlUntil(next.ExpiresAt).Seconds()), next.TokenHash} - res, err := rotateRefreshScript.Run(ctx, s.rdb, keys, args...).Result() + res, err := rotatePairScript.Run(ctx, s.rdb, keys, args...).Text() if err != nil { return fmt.Errorf("redisstore: rotate refresh token: %w", err) } switch res { - case "ok": + case resultOK: return nil - case "notfound": + case resultNotFound: return oauth2.ErrInvalidGrant.WithDescription("refresh token not found") - case "reused": - // Reuse detected — revoke the whole family per BCP §8.10.3. - _ = s.RevokeRefreshFamily(ctx, next.FamilyID) - - return oauth2.ErrRefreshTokenReused + case resultMismatch: + return oauth2.ErrInvalidRequest.WithDescription("redisstore: rotation into another token family") + case resultRevoked: + return oauth2.ErrTokenFamilyRevoked + case resultReused: + return s.revokeReused(ctx, familyID) default: - return fmt.Errorf("redisstore: rotate: unexpected script result %v", res) + return fmt.Errorf("redisstore: rotate refresh token: unexpected script result %q", res) } } -// RevokeRefreshFamily implements [oauth2.RefreshTokenStore]: every refresh -// token of the family is marked consumed, every access token of the -// family is deleted. +// RevokeRefreshFamily implements [oauth2.RefreshTokenStore] via +// revokeFamilyScript: one write, recorded whether or not any token of the +// family exists. A failure wraps [oauth2.ErrTokenFamilyRevocationFailed]. func (s *Store) RevokeRefreshFamily(ctx context.Context, familyID string) error { - rtHashes, err := s.rdb.SMembers(ctx, s.famRTKey(familyID)).Result() - if err != nil && !errors.Is(err, redis.Nil) { - return fmt.Errorf("redisstore: list family refresh tokens: %w", err) + if err := checkFamilyID(familyID); err != nil { + return err } - for _, h := range rtHashes { - if err := s.markConsumed(ctx, h); err != nil { - return err - } - } + return s.revokeFamily(ctx, familyID) +} - atHashes, err := s.rdb.SMembers(ctx, s.famATKey(familyID)).Result() - if err != nil && !errors.Is(err, redis.Nil) { - return fmt.Errorf("redisstore: list family access tokens: %w", err) +// revokeReused answers the reuse rotatePairScript detected by revoking the +// family, then reports the reuse. +func (s *Store) revokeReused(ctx context.Context, familyID string) error { + if err := s.revokeFamily(ctx, familyID); err != nil { + return fmt.Errorf("%w: %w", oauth2.ErrRefreshTokenReused, err) } - for _, h := range atHashes { - if err := s.rdb.Del(ctx, s.atKey(h)).Err(); err != nil { - return fmt.Errorf("redisstore: purge family access token: %w", err) - } + return oauth2.ErrRefreshTokenReused +} + +// revokeFamily runs revokeFamilyScript. +func (s *Store) revokeFamily(ctx context.Context, familyID string) error { + err := revokeFamilyScript.Run(ctx, s.rdb, []string{s.famKey(familyID)}, + oauth2.FamilyRevocationRetention.Milliseconds()).Err() + if err != nil { + return fmt.Errorf("redisstore: revoke family %q: %w: %w", familyID, oauth2.ErrTokenFamilyRevocationFailed, err) } return nil } -// markConsumed flips the consumed flag of a single refresh token, -// preserving its TTL. -func (s *Store) markConsumed(ctx context.Context, hash string) error { - raw, err := s.rdb.Get(ctx, s.rtKey(hash)).Bytes() - if err != nil { - if errors.Is(err, redis.Nil) { - return nil // already gone — nothing to revoke - } - - return fmt.Errorf("redisstore: get refresh token: %w", err) +// familyActive reports whether a token of familyID may be used: it is +// outside any family, or its family key holds exactly "active". A missing +// key — expired, evicted — or any other value reads as revoked; a key the +// store cannot read is an error, never an answer. +func (s *Store) familyActive(ctx context.Context, familyID string) (bool, error) { + if familyID == "" { + return true, nil } - rt, err := decodeRefresh(hash, raw) - if err != nil { - return fmt.Errorf("redisstore: decode refresh token: %w", err) - } + state, err := s.rdb.Get(ctx, s.famKey(familyID)).Result() - if rt.Consumed { - return nil + switch { + case errors.Is(err, redis.Nil): + return false, nil + case err != nil: + return false, fmt.Errorf("redisstore: lookup token family: %w", err) + default: + return state == familyActive, nil } +} - rt.Consumed = true - - payload, err := encodeRefresh(rt) - if err != nil { - return fmt.Errorf("redisstore: encode refresh token: %w", err) +// validateRotation refuses a malformed rotation target before any command +// runs: nil, failing [oauth2.TokenPair.Validate], or without the refresh +// token a rotation issues. +func validateRotation(next *oauth2.TokenPair) error { + if err := next.Validate(); err != nil { + return fmt.Errorf("redisstore: rotate refresh token: %w", err) } - if err := s.rdb.Set(ctx, s.rtKey(hash), payload, redis.KeepTTL).Err(); err != nil { - return fmt.Errorf("redisstore: mark refresh token consumed: %w", err) + if next.Refresh == nil { + return oauth2.ErrInvalidRequest.WithDescription("redisstore: rotation without a refresh token") } return nil } +// checkFamilyID refuses a family ID no family can have: empty, or longer +// than [oauth2.MaxFamilyIDLength]. +func checkFamilyID(familyID string) error { + switch { + case familyID == "": + return oauth2.ErrInvalidRequest.WithDescription("redisstore: empty token family id") + case len(familyID) > oauth2.MaxFamilyIDLength: + return oauth2.ErrInvalidRequest.WithDescription("redisstore: token family id too long") + default: + return nil + } +} + // Compile-time interface check. var _ oauth2.Storage = (*Store)(nil) diff --git a/oauth2/store/redis/store_error_test.go b/oauth2/store/redis/store_error_test.go index 2d5fe77..5745fdf 100644 --- a/oauth2/store/redis/store_error_test.go +++ b/oauth2/store/redis/store_error_test.go @@ -6,6 +6,7 @@ package redisstore_test import ( "context" + "strings" "testing" "time" @@ -17,15 +18,26 @@ import ( "github.com/stretchr/testify/require" ) -func TestRedisStoreRejectsEmptyHashes(t *testing.T) { +func TestRedisStoreRejectsInvalidInput(t *testing.T) { t.Parallel() store := newRedisStore(t) ctx := context.Background() + tooLong := strings.Repeat("f", oauth2.MaxFamilyIDLength+1) + bare := &oauth2.TokenPair{Access: oauth2.AccessToken{TokenHash: "h", FamilyID: "f"}} require.Error(t, store.SaveAuthorizationCode(ctx, &oauth2.AuthorizationCode{})) - require.Error(t, store.SaveAccessToken(ctx, &oauth2.AccessToken{})) - require.Error(t, store.SaveRefreshToken(ctx, &oauth2.RefreshToken{})) + + for _, err := range []error{ + store.SaveTokenPair(ctx, &oauth2.TokenPair{}), + store.SaveTokenPair(ctx, nil), + store.RotateRefreshToken(ctx, "h", nil), + store.RotateRefreshToken(ctx, "h", bare), + store.RevokeRefreshFamily(ctx, ""), + store.RevokeRefreshFamily(ctx, tooLong), + } { + assert.Equal(t, oauth2.CodeInvalidRequest, oauth2.IsCode(err), "%v", err) + } } // TestRedisStoreReportsBackendErrors closes the miniredis server so every @@ -34,7 +46,9 @@ func TestRedisStoreReportsBackendErrors(t *testing.T) { t.Parallel() mr := miniredis.RunT(t) - client := redis.NewClient(&redis.Options{Addr: mr.Addr()}) + // No retries: every command fails anyway, and retrying a dead server + // only slows the test down. + client := redis.NewClient(&redis.Options{Addr: mr.Addr(), MaxRetries: -1}) t.Cleanup(func() { _ = client.Close() }) store, err := redisstore.New(client) @@ -53,22 +67,21 @@ func TestRedisStoreReportsBackendErrors(t *testing.T) { _, err = store.ConsumeAuthorizationCode(ctx, "h") require.Error(t, err) - at := &oauth2.AccessToken{TokenHash: "h", FamilyID: "f", ExpiresAt: now.Add(time.Minute)} - require.Error(t, store.SaveAccessToken(ctx, at)) + require.Error(t, store.SaveTokenPair(ctx, testPair("h", "f", time.Minute))) + require.Error(t, store.SaveTokenPair(ctx, &oauth2.TokenPair{Access: oauth2.AccessToken{ + TokenHash: "h", ExpiresAt: now.Add(time.Minute), + }})) _, err = store.LookupAccessToken(ctx, "h") require.Error(t, err) require.Error(t, store.RevokeAccessToken(ctx, "h")) - rt := &oauth2.RefreshToken{TokenHash: "h", FamilyID: "f", ExpiresAt: now.Add(time.Minute)} - require.Error(t, store.SaveRefreshToken(ctx, rt)) - _, err = store.LookupRefreshToken(ctx, "h") require.Error(t, err) - require.Error(t, store.RotateRefreshToken(ctx, "h", rt)) - require.Error(t, store.RevokeRefreshFamily(ctx, "f")) + require.Error(t, store.RotateRefreshToken(ctx, "h", testPair("next", "f", time.Minute))) + require.ErrorIs(t, store.RevokeRefreshFamily(ctx, "f"), oauth2.ErrTokenFamilyRevocationFailed) } // TestRedisStoreDecodeErrors injects corrupt JSON directly into Redis and @@ -96,6 +109,13 @@ func TestRedisStoreDecodeErrors(t *testing.T) { require.NoError(t, mr.Set("oauth2:rt:bad", "{not-json")) _, err = store.LookupRefreshToken(ctx, "bad") require.Error(t, err) + + // A corrupt old token aborts the rotation before it writes anything. + next := testPair("after-bad", "f", time.Hour) + require.Error(t, store.RotateRefreshToken(ctx, "bad", next)) + assert.False(t, mr.Exists("oauth2:rt:rt-after-bad")) + assert.False(t, mr.Exists("oauth2:at:at-after-bad")) + assert.False(t, mr.Exists("oauth2:fam:f")) } // TestRedisStoreConsumeUnknownCode covers the "code already used / absent" @@ -109,63 +129,6 @@ func TestRedisStoreConsumeUnknownCode(t *testing.T) { require.ErrorIs(t, err, oauth2.ErrCodeAlreadyUsed) } -// TestRedisStoreRotateUnknownToken covers the script's "notfound" branch. -func TestRedisStoreRotateUnknownToken(t *testing.T) { - t.Parallel() - - store := newRedisStore(t) - - err := store.RotateRefreshToken(context.Background(), "never-saved", &oauth2.RefreshToken{ - TokenHash: "new", FamilyID: "fam", ExpiresAt: time.Now().Add(time.Hour), - }) - require.Error(t, err) -} - -// TestRedisStoreRevokeFamilyMarksRefreshTokens checks the family-revocation -// path: every refresh token of the family ends up consumed. -func TestRedisStoreRevokeFamilyMarksRefreshTokens(t *testing.T) { - t.Parallel() - - store := newRedisStore(t) - ctx := context.Background() - now := time.Now() - - rt := &oauth2.RefreshToken{ - Token: "raw", TokenHash: "rt-hash", ClientID: "c", Subject: "s", - Scope: "read", FamilyID: "fam-1", IssuedAt: now, ExpiresAt: now.Add(time.Hour), - } - require.NoError(t, store.SaveRefreshToken(ctx, rt)) - - require.NoError(t, store.RevokeRefreshFamily(ctx, "fam-1")) - - got, err := store.LookupRefreshToken(ctx, "rt-hash") - require.NoError(t, err) - assert.True(t, got.Consumed, "family revocation must mark the refresh token consumed") - - // Revoking again is idempotent — the token is already consumed. - require.NoError(t, store.RevokeRefreshFamily(ctx, "fam-1")) -} - -// TestRedisStoreRevokeFamilyWithCorruptMember exercises the markConsumed -// decode-error branch. -func TestRedisStoreRevokeFamilyWithCorruptMember(t *testing.T) { - t.Parallel() - - mr := miniredis.RunT(t) - client := redis.NewClient(&redis.Options{Addr: mr.Addr()}) - t.Cleanup(func() { _ = client.Close() }) - - store, err := redisstore.New(client) - require.NoError(t, err) - - // A family set referencing a refresh token whose payload is corrupt. - _, err = mr.SAdd("oauth2:famrt:fam-x", "corrupt-hash") - require.NoError(t, err) - require.NoError(t, mr.Set("oauth2:rt:corrupt-hash", "{not-json")) - - require.Error(t, store.RevokeRefreshFamily(context.Background(), "fam-x")) -} - // TestRedisStoreSavePastExpiryClampsTTL saves a token already past its // expiry; ttlUntil must clamp the key TTL to the 1-second floor. func TestRedisStoreSavePastExpiryClampsTTL(t *testing.T) { @@ -174,11 +137,11 @@ func TestRedisStoreSavePastExpiryClampsTTL(t *testing.T) { store := newRedisStore(t) ctx := context.Background() - at := &oauth2.AccessToken{ + at := oauth2.AccessToken{ TokenHash: "expired", ClientID: "c", Subject: "s", IssuedAt: time.Now().Add(-time.Hour), ExpiresAt: time.Now().Add(-time.Minute), } - require.NoError(t, store.SaveAccessToken(ctx, at)) + require.NoError(t, store.SaveTokenPair(ctx, &oauth2.TokenPair{Access: at})) got, err := store.LookupAccessToken(ctx, "expired") require.NoError(t, err) diff --git a/oauth2/store/redis/store_family_test.go b/oauth2/store/redis/store_family_test.go new file mode 100644 index 0000000..a870cc0 --- /dev/null +++ b/oauth2/store/redis/store_family_test.go @@ -0,0 +1,531 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package redisstore_test + +import ( + "context" + "errors" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/alicebob/miniredis/v2" + "github.com/hyperscale-stack/security/oauth2" + redisstore "github.com/hyperscale-stack/security/oauth2/store/redis" + "github.com/redis/go-redis/v9" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// errInjected is the backend failure the script hooks inject. +var errInjected = errors.New("injected failure") + +// Key layout of the default "oauth2:" prefix. +func famKey(id string) string { return "oauth2:fam:" + id } +func rtKey(hash string) string { return "oauth2:rt:" + hash } +func atKey(hash string) string { return "oauth2:at:" + hash } + +// testPair returns a pair in family whose tokens both expire after ttl. +func testPair(name, family string, ttl time.Duration) *oauth2.TokenPair { + now := time.Now() + + return &oauth2.TokenPair{ + Access: oauth2.AccessToken{ + TokenHash: "at-" + name, ClientID: "c", Subject: "s", Scope: "r", Audience: "api", + FamilyID: family, IssuedAt: now, ExpiresAt: now.Add(ttl), + }, + Refresh: &oauth2.RefreshToken{ + TokenHash: "rt-" + name, ClientID: "c", Subject: "s", Scope: "r", + FamilyID: family, IssuedAt: now, ExpiresAt: now.Add(ttl), + }, + } +} + +// scriptHook intercepts the store's Lua scripts, matched by their key +// count and first key. Scripts run through EVALSHA, then EVAL after a +// NOSCRIPT reply, so the hook acts BEFORE a matching command and never +// inspects its result. While armed it either runs before once — a +// competing operation landing right before the script — or fails the +// script without sending it. +type scriptHook struct { + numKeys int + key string + armed atomic.Bool + before func() + fail error +} + +func (h *scriptHook) DialHook(next redis.DialHook) redis.DialHook { return next } + +func (h *scriptHook) ProcessPipelineHook(next redis.ProcessPipelineHook) redis.ProcessPipelineHook { + return next +} + +func (h *scriptHook) ProcessHook(next redis.ProcessHook) redis.ProcessHook { + return func(ctx context.Context, cmd redis.Cmder) error { + if !h.matches(cmd) { + return next(ctx, cmd) + } + + if h.fail != nil && h.armed.Load() { + return h.fail + } + + if h.before != nil && h.armed.CompareAndSwap(true, false) { + h.before() + } + + return next(ctx, cmd) + } +} + +func (h *scriptHook) matches(cmd redis.Cmder) bool { + args := cmd.Args() + + return (cmd.Name() == "evalsha" || cmd.Name() == "eval") && + len(args) > 3 && args[2] == h.numKeys && args[3] == h.key +} + +// The scripts the hooks target, by key count and first key. +func revokeScript(family string) (int, string) { return 1, famKey(family) } +func rotateScript(oldHash string) (int, string) { return 4, rtKey(oldHash) } +func issueScript(family string) (int, string) { return 3, famKey(family) } + +// hookedStores returns two stores over one miniredis: hooked, whose client +// carries hook, and plain, the one competing operations go through. +func hookedStores(t *testing.T, hook *scriptHook) (hooked, plain *redisstore.Store, mr *miniredis.Miniredis) { + t.Helper() + + mr = miniredis.RunT(t) + + hookedClient := redis.NewClient(&redis.Options{Addr: mr.Addr()}) + hookedClient.AddHook(hook) + plainClient := redis.NewClient(&redis.Options{Addr: mr.Addr()}) + + t.Cleanup(func() { + _ = hookedClient.Close() + _ = plainClient.Close() + }) + + hooked, err := redisstore.New(hookedClient) + require.NoError(t, err) + + plain, err = redisstore.New(plainClient) + require.NoError(t, err) + + return hooked, plain, mr +} + +func requireUsable(t *testing.T, s oauth2.Storage, pair *oauth2.TokenPair) { + t.Helper() + + _, err := s.LookupAccessToken(t.Context(), pair.Access.TokenHash) + require.NoError(t, err) + + rt, err := s.LookupRefreshToken(t.Context(), pair.Refresh.TokenHash) + require.NoError(t, err) + assert.False(t, rt.Consumed, "refresh token %s", pair.Refresh.TokenHash) +} + +func requireUnusable(t *testing.T, s oauth2.Storage, pair *oauth2.TokenPair) { + t.Helper() + + _, err := s.LookupAccessToken(t.Context(), pair.Access.TokenHash) + assert.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked) + + rt, err := s.LookupRefreshToken(t.Context(), pair.Refresh.TokenHash) + require.NoError(t, err) + assert.True(t, rt.Consumed, "refresh token %s", pair.Refresh.TokenHash) +} + +// requireNoKeys asserts that none of pair's tokens reached Redis. +func requireNoKeys(t *testing.T, mr *miniredis.Miniredis, pair *oauth2.TokenPair) { + t.Helper() + + assert.False(t, mr.Exists(atKey(pair.Access.TokenHash)), "access token %s", pair.Access.TokenHash) + assert.False(t, mr.Exists(rtKey(pair.Refresh.TokenHash)), "refresh token %s", pair.Refresh.TokenHash) +} + +func requireFamily(t *testing.T, mr *miniredis.Miniredis, family, want string) { + t.Helper() + + got, err := mr.Get(famKey(family)) + require.NoError(t, err) + assert.Equal(t, want, got, "family %s", family) +} + +func TestRedisRevokeFamilyFailureIsReported(t *testing.T) { + t.Parallel() + + hook := &scriptHook{fail: errInjected} + hook.numKeys, hook.key = revokeScript("fam-revoke") + store, _, mr := hookedStores(t, hook) + + pair := testPair("revoke", "fam-revoke", time.Hour) + require.NoError(t, store.SaveTokenPair(t.Context(), pair)) + + hook.armed.Store(true) + + err := store.RevokeRefreshFamily(t.Context(), "fam-revoke") + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevocationFailed) + require.ErrorIs(t, err, errInjected) + assert.Empty(t, oauth2.IsCode(err), "not an OAuth2 error: a caller answers server_error") + assert.ErrorContains(t, err, `"fam-revoke"`) + + requireFamily(t, mr, "fam-revoke", "active") + requireUsable(t, store, pair) + + hook.armed.Store(false) + require.NoError(t, store.RevokeRefreshFamily(t.Context(), "fam-revoke")) + requireFamily(t, mr, "fam-revoke", "revoked") + requireUnusable(t, store, pair) +} + +func TestRedisReuseRevocationFailureIsReported(t *testing.T) { + t.Parallel() + + hook := &scriptHook{fail: errInjected} + hook.numKeys, hook.key = revokeScript("fam-reuse") + store, _, mr := hookedStores(t, hook) + + first := testPair("reuse-0", "fam-reuse", time.Hour) + second := testPair("reuse-1", "fam-reuse", time.Hour) + replay := testPair("reuse-2", "fam-reuse", time.Hour) + + require.NoError(t, store.SaveTokenPair(t.Context(), first)) + require.NoError(t, store.RotateRefreshToken(t.Context(), first.Refresh.TokenHash, second)) + + hook.armed.Store(true) + + err := store.RotateRefreshToken(t.Context(), first.Refresh.TokenHash, replay) + require.ErrorIs(t, err, oauth2.ErrRefreshTokenReused) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevocationFailed) + require.ErrorIs(t, err, errInjected) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + assert.ErrorContains(t, err, `"fam-reuse"`) + + // The store says the family is NOT revoked, and it is not. + requireFamily(t, mr, "fam-reuse", "active") + requireUsable(t, store, second) + requireNoKeys(t, mr, replay) + + hook.armed.Store(false) + require.NoError(t, store.RevokeRefreshFamily(t.Context(), "fam-reuse")) + requireUnusable(t, store, second) +} + +// TestRedisRotationJustBeforeRevocation lands a rotation right before the +// revocation script: the rotation is ordered before the revocation, which +// then covers its tokens. +func TestRedisRotationJustBeforeRevocation(t *testing.T) { + t.Parallel() + + hook := &scriptHook{} + hook.numKeys, hook.key = revokeScript("fam-race") + store, plain, _ := hookedStores(t, hook) + + first := testPair("race-0", "fam-race", time.Hour) + next := testPair("race-1", "fam-race", time.Hour) + require.NoError(t, store.SaveTokenPair(t.Context(), first)) + + hook.before = func() { + require.NoError(t, plain.RotateRefreshToken(t.Context(), first.Refresh.TokenHash, next)) + } + hook.armed.Store(true) + + require.NoError(t, store.RevokeRefreshFamily(t.Context(), "fam-race")) + assert.False(t, hook.armed.Load(), "the rotation ran") + + requireUnusable(t, store, first) + requireUnusable(t, store, next) +} + +// TestRedisRevocationJustBeforeRotation lands a revocation right before the +// rotation script: the rotation is refused and writes nothing. +func TestRedisRevocationJustBeforeRotation(t *testing.T) { + t.Parallel() + + first := testPair("late-0", "fam-late", time.Hour) + next := testPair("late-1", "fam-late", time.Hour) + + hook := &scriptHook{} + hook.numKeys, hook.key = rotateScript(first.Refresh.TokenHash) + store, plain, mr := hookedStores(t, hook) + + require.NoError(t, store.SaveTokenPair(t.Context(), first)) + + hook.before = func() { + require.NoError(t, plain.RevokeRefreshFamily(t.Context(), "fam-late")) + } + hook.armed.Store(true) + + err := store.RotateRefreshToken(t.Context(), first.Refresh.TokenHash, next) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked) + requireNoKeys(t, mr, next) + + raw, err := mr.Get(rtKey(first.Refresh.TokenHash)) + require.NoError(t, err) + assert.Contains(t, raw, `"consumed":false`, "the refused rotation consumed nothing") + requireUnusable(t, store, first) +} + +// TestRedisRotationBetweenReuseAndRevocation lets an attacker rotate the +// family between the script that detects a reuse and the revocation the +// store then runs: the revocation covers the attacker's tokens too. +func TestRedisRotationBetweenReuseAndRevocation(t *testing.T) { + t.Parallel() + + hook := &scriptHook{} + hook.numKeys, hook.key = revokeScript("fam-gap") + store, plain, _ := hookedStores(t, hook) + + first := testPair("gap-0", "fam-gap", time.Hour) + second := testPair("gap-1", "fam-gap", time.Hour) + attacker := testPair("gap-attacker", "fam-gap", time.Hour) + + require.NoError(t, store.SaveTokenPair(t.Context(), first)) + require.NoError(t, store.RotateRefreshToken(t.Context(), first.Refresh.TokenHash, second)) + + hook.before = func() { + require.NoError(t, plain.RotateRefreshToken(t.Context(), second.Refresh.TokenHash, attacker)) + } + hook.armed.Store(true) + + err := store.RotateRefreshToken(t.Context(), first.Refresh.TokenHash, testPair("gap-2", "fam-gap", time.Hour)) + assert.Same(t, oauth2.ErrRefreshTokenReused, err) + assert.False(t, hook.armed.Load(), "the attacker's rotation ran") + + requireUnusable(t, store, second) + requireUnusable(t, store, attacker) +} + +// TestRedisIssuanceAroundRevocation orders an issuance and a revocation +// both ways: an issuance landing right before the revocation script is +// covered by the revocation, one landing right after it is refused. +func TestRedisIssuanceAroundRevocation(t *testing.T) { + t.Parallel() + + t.Run("issuance just before the revocation", func(t *testing.T) { + t.Parallel() + + hook := &scriptHook{} + hook.numKeys, hook.key = revokeScript("fam-issue") + store, plain, _ := hookedStores(t, hook) + + pair := testPair("issue-before", "fam-issue", time.Hour) + hook.before = func() { require.NoError(t, plain.SaveTokenPair(t.Context(), pair)) } + hook.armed.Store(true) + + require.NoError(t, store.RevokeRefreshFamily(t.Context(), "fam-issue")) + requireUnusable(t, store, pair) + }) + + t.Run("revocation just before the issuance", func(t *testing.T) { + t.Parallel() + + hook := &scriptHook{} + hook.numKeys, hook.key = issueScript("fam-issue") + store, plain, mr := hookedStores(t, hook) + + hook.before = func() { require.NoError(t, plain.RevokeRefreshFamily(t.Context(), "fam-issue")) } + hook.armed.Store(true) + + pair := testPair("issue-after", "fam-issue", time.Hour) + require.ErrorIs(t, store.SaveTokenPair(t.Context(), pair), oauth2.ErrTokenFamilyRevoked) + requireNoKeys(t, mr, pair) + }) +} + +// resendHook runs the rotation script twice, the way go-redis resends a +// command whose reply was lost: the store must see the rotation succeed, +// not mistake its own first run for a reuse. +type resendHook struct{ scriptHook } + +func (h *resendHook) ProcessHook(next redis.ProcessHook) redis.ProcessHook { + return func(ctx context.Context, cmd redis.Cmder) error { + if h.matches(cmd) { + if err := next(ctx, cmd); err != nil { + return err // NOSCRIPT: go-redis falls back to EVAL + } + } + + return next(ctx, cmd) + } +} + +func TestRedisResentRotationIsNotReuse(t *testing.T) { + t.Parallel() + + first := testPair("resend-0", "fam-resend", time.Hour) + next := testPair("resend-1", "fam-resend", time.Hour) + + hook := &resendHook{} + hook.numKeys, hook.key = rotateScript(first.Refresh.TokenHash) + + mr := miniredis.RunT(t) + client := redis.NewClient(&redis.Options{Addr: mr.Addr()}) + client.AddHook(hook) + t.Cleanup(func() { _ = client.Close() }) + + store, err := redisstore.New(client) + require.NoError(t, err) + + require.NoError(t, store.SaveTokenPair(t.Context(), first)) + require.NoError(t, store.RotateRefreshToken(t.Context(), first.Refresh.TokenHash, next)) + + requireFamily(t, mr, "fam-resend", "active") + requireUsable(t, store, next) +} + +func TestRedisFamilyKeyTTL(t *testing.T) { + t.Parallel() + + store, _, mr := hookedStores(t, &scriptHook{}) + ctx := t.Context() + day := oauth2.FamilyRevocationRetention + + // The family lives as long as its longest token... + first := testPair("ttl-0", "fam-ttl", time.Hour) + require.NoError(t, store.SaveTokenPair(ctx, first)) + assert.InDelta(t, time.Hour, mr.TTL(famKey("fam-ttl")), float64(time.Second)) + + long := testPair("ttl-1", "fam-ttl", 48*time.Hour) + require.NoError(t, store.RotateRefreshToken(ctx, first.Refresh.TokenHash, long)) + assert.InDelta(t, 48*time.Hour, mr.TTL(famKey("fam-ttl")), float64(time.Second)) + + // ...never shrinks with a shorter one... + short := testPair("ttl-2", "fam-ttl", time.Minute) + require.NoError(t, store.RotateRefreshToken(ctx, long.Refresh.TokenHash, short)) + assert.InDelta(t, 48*time.Hour, mr.TTL(famKey("fam-ttl")), float64(time.Second)) + + // ...and rotating keeps the TTL of the consumed token. + assert.InDelta(t, time.Hour, mr.TTL(rtKey(first.Refresh.TokenHash)), float64(time.Second)) + + // A revocation keeps the longer of the family's TTL and the retention. + require.NoError(t, store.RevokeRefreshFamily(ctx, "fam-ttl")) + assert.InDelta(t, 48*time.Hour, mr.TTL(famKey("fam-ttl")), float64(time.Second)) + + brief := testPair("ttl-brief", "fam-brief", time.Minute) + require.NoError(t, store.SaveTokenPair(ctx, brief)) + require.NoError(t, store.RevokeRefreshFamily(ctx, "fam-brief")) + assert.InDelta(t, day, mr.TTL(famKey("fam-brief")), float64(time.Second)) + + // A revocation before issuance is retained for the retention too, and + // outlives the tokens it refuses. + require.NoError(t, store.RevokeRefreshFamily(ctx, "fam-unknown")) + assert.InDelta(t, day, mr.TTL(famKey("fam-unknown")), float64(time.Second)) + + mr.FastForward(2 * time.Minute) + assert.False(t, mr.Exists(rtKey(brief.Refresh.TokenHash)), "the tokens expired") + requireFamily(t, mr, "fam-brief", "revoked") + require.ErrorIs(t, store.SaveTokenPair(ctx, testPair("ttl-late", "fam-brief", time.Hour)), oauth2.ErrTokenFamilyRevoked) +} + +// TestRedisLookupFailsClosed covers family state that is missing or not +// what the store wrote: the family's tokens are never usable, and an +// unreadable state is an error, not an answer. +func TestRedisLookupFailsClosed(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + state func(mr *miniredis.Miniredis, key string) + }{ + {"missing", func(mr *miniredis.Miniredis, key string) { mr.Del(key) }}, + {"unexpected value", func(mr *miniredis.Miniredis, key string) { _ = mr.Set(key, "bogus") }}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + store, _, mr := hookedStores(t, &scriptHook{}) + pair := testPair("closed", "fam-closed", time.Hour) + require.NoError(t, store.SaveTokenPair(t.Context(), pair)) + + tc.state(mr, famKey("fam-closed")) + + requireUnusable(t, store, pair) + + next := testPair("closed-next", "fam-closed", time.Hour) + err := store.RotateRefreshToken(t.Context(), pair.Refresh.TokenHash, next) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked) + requireNoKeys(t, mr, next) + + // A rotation never creates the family. + state, _ := mr.Get(famKey("fam-closed")) + assert.NotEqual(t, "active", state) + }) + } + + t.Run("unreadable", func(t *testing.T) { + t.Parallel() + + store, _, mr := hookedStores(t, &scriptHook{}) + pair := testPair("wrongtype", "fam-wrongtype", time.Hour) + require.NoError(t, store.SaveTokenPair(t.Context(), pair)) + + mr.Del(famKey("fam-wrongtype")) + _, err := mr.Lpush(famKey("fam-wrongtype"), "x") + require.NoError(t, err) + + at, err := store.LookupAccessToken(t.Context(), pair.Access.TokenHash) + assert.Nil(t, at) + require.ErrorContains(t, err, "WRONGTYPE") + + rt, err := store.LookupRefreshToken(t.Context(), pair.Refresh.TokenHash) + assert.Nil(t, rt) + require.ErrorContains(t, err, "WRONGTYPE") + + require.Error(t, store.SaveTokenPair(t.Context(), testPair("wrongtype-2", "fam-wrongtype", time.Hour))) + }) +} + +// TestRedisIssuanceRefusesUnexpectedFamilyState refuses to join a family +// whose state is neither unknown nor "active". +func TestRedisIssuanceRefusesUnexpectedFamilyState(t *testing.T) { + t.Parallel() + + store, _, mr := hookedStores(t, &scriptHook{}) + require.NoError(t, mr.Set(famKey("fam-odd"), "bogus")) + + pair := testPair("odd", "fam-odd", time.Hour) + require.ErrorIs(t, store.SaveTokenPair(t.Context(), pair), oauth2.ErrTokenFamilyRevoked) + requireNoKeys(t, mr, pair) +} + +// TestRedisRefreshTokenWithoutFamilyIsConsumed covers a refresh token +// stored without a family — only a build predating #82 could write one: it +// can no longer be exchanged. +func TestRedisRefreshTokenWithoutFamilyIsConsumed(t *testing.T) { + t.Parallel() + + store, _, mr := hookedStores(t, &scriptHook{}) + require.NoError(t, mr.Set(rtKey("legacy"), + `{"client_id":"c","subject":"s","scope":"r","family_id":"","consumed":false,"issued_at":0,"expires_at":0}`)) + + rt, err := store.LookupRefreshToken(t.Context(), "legacy") + require.NoError(t, err) + assert.True(t, rt.Consumed) +} + +// TestRedisNoFamilySets checks that the store no longer keeps the per-family +// member sets of earlier builds: nothing enumerates a family any more. +func TestRedisNoFamilySets(t *testing.T) { + t.Parallel() + + store, _, mr := hookedStores(t, &scriptHook{}) + ctx := t.Context() + + first := testPair("sets-0", "fam-sets", time.Hour) + require.NoError(t, store.SaveTokenPair(ctx, first)) + require.NoError(t, store.RotateRefreshToken(ctx, first.Refresh.TokenHash, testPair("sets-1", "fam-sets", time.Hour))) + require.NoError(t, store.RevokeRefreshFamily(ctx, "fam-sets")) + + for _, key := range mr.Keys() { + assert.False(t, strings.Contains(key, "famrt:") || strings.Contains(key, "famat:"), key) + } +} diff --git a/oauth2/store/redis/store_test.go b/oauth2/store/redis/store_test.go index ed06106..d4a70f1 100644 --- a/oauth2/store/redis/store_test.go +++ b/oauth2/store/redis/store_test.go @@ -36,8 +36,8 @@ func newRedisStore(t *testing.T) oauth2.Storage { } // TestRedisStoreConformance runs the shared storage contract against the -// Redis implementation. The same 11-case suite the memory and SQL stores -// pass — concurrency races included — exercising the Lua-script atomicity. +// Redis implementation. The same suite the memory and SQL stores pass — +// concurrency races included — exercising the Lua-script atomicity. func TestRedisStoreConformance(t *testing.T) { t.Parallel() diff --git a/oauth2/store/sql/dialect.go b/oauth2/store/sql/dialect.go index 7d06905..8a93041 100644 --- a/oauth2/store/sql/dialect.go +++ b/oauth2/store/sql/dialect.go @@ -5,14 +5,16 @@ package sqlstore import ( + "database/sql" "strconv" "strings" ) -// Dialect abstracts the few SQL syntax differences the store cares about: -// the parameter-placeholder style and the boolean literals. The store -// writes every query with "?" placeholders and rebinds them through the -// dialect, so query strings stay readable. +// Dialect abstracts the few SQL differences the store cares about: the +// parameter-placeholder style, the insert-if-absent syntax, row locking and +// the transaction isolation level. The store writes every query with "?" +// placeholders and rebinds them through the dialect, so query strings stay +// readable. type Dialect interface { // Name returns a stable identifier ("postgres", "mysql", "sqlite") // used in error messages and OTel attributes. @@ -20,15 +22,40 @@ type Dialect interface { // rebind rewrites a "?"-placeholder query into the dialect's native // placeholder style. Postgres needs $1,$2,…; MySQL and SQLite keep ?. rebind(query string) string + // insertFamily inserts a token-family record (family_id, revoked_at, + // expires_at) unless one already exists, keeping the existing one. + insertFamily() string + // lockFamily reads a family's revoked_at under a row lock. + lockFamily() string + // lockRefresh reads a refresh token's family_id and consumed flag + // under a row lock. + lockRefresh() string + // txOptions are the options of the token-family transactions. + txOptions() *sql.TxOptions } -// Postgres is the PostgreSQL dialect ($1,$2,… placeholders). +// The token-family statements several dialects share, each a full literal. +const ( + insertFamilyOnConflict = `INSERT INTO oauth2_token_families (family_id, revoked_at, expires_at) VALUES (?, ?, ?) + ON CONFLICT (family_id) DO NOTHING` + lockFamilyForUpdate = `SELECT revoked_at FROM oauth2_token_families WHERE family_id = ? FOR UPDATE` + lockRefreshForUpdate = `SELECT family_id, consumed FROM oauth2_refresh_tokens WHERE token_hash = ? FOR UPDATE` +) + +// Postgres is the PostgreSQL dialect ($1,$2,… placeholders). Its +// token-family transactions run at READ COMMITTED whatever the server +// default: the family lock relies on a locking read re-reading the latest +// committed row, where REPEATABLE READ or SERIALIZABLE would abort the +// transaction with a serialization failure instead. var Postgres Dialect = postgres{} -// MySQL is the MySQL / MariaDB dialect (? placeholders). +// MySQL is the MySQL / MariaDB dialect (? placeholders). The tables must be +// InnoDB: the store relies on its row locks. var MySQL Dialect = mysql{} -// SQLite is the SQLite dialect (? placeholders). +// SQLite is the SQLite dialect (? placeholders). SQLite has no row locks: +// its single writer serializes the store's write transactions, each of +// which starts with a write. var SQLite Dialect = sqlite{} type postgres struct{} @@ -59,16 +86,50 @@ func (postgres) rebind(query string) string { return b.String() } +func (postgres) insertFamily() string { return insertFamilyOnConflict } +func (postgres) lockFamily() string { return lockFamilyForUpdate } +func (postgres) lockRefresh() string { return lockRefreshForUpdate } + +func (postgres) txOptions() *sql.TxOptions { + return &sql.TxOptions{Isolation: sql.LevelReadCommitted} +} + type mysql struct{} func (mysql) Name() string { return "mysql" } func (mysql) rebind(q string) string { return q } -// dialectSQLite is the dialect identifier for SQLite, kept as a constant so -// it can be referenced from Name() and from schema generation. +// insertFamily uses ON DUPLICATE KEY UPDATE rather than INSERT IGNORE, +// which would also silence errors such as a truncated value. +func (mysql) insertFamily() string { + return `INSERT INTO oauth2_token_families (family_id, revoked_at, expires_at) VALUES (?, ?, ?) + ON DUPLICATE KEY UPDATE family_id = family_id` +} + +func (mysql) lockFamily() string { return lockFamilyForUpdate } +func (mysql) lockRefresh() string { return lockRefreshForUpdate } + +func (mysql) txOptions() *sql.TxOptions { return nil } + +// dialectSQLite is the dialect identifier for SQLite. const dialectSQLite = "sqlite" type sqlite struct{} func (sqlite) Name() string { return dialectSQLite } func (sqlite) rebind(q string) string { return q } + +func (sqlite) insertFamily() string { return insertFamilyOnConflict } + +// lockFamily and lockRefresh are plain reads: SQLite has no FOR UPDATE, +// and the write that opens every store transaction already holds the +// database's single write lock. +func (sqlite) lockFamily() string { + return `SELECT revoked_at FROM oauth2_token_families WHERE family_id = ?` +} + +func (sqlite) lockRefresh() string { + return `SELECT family_id, consumed FROM oauth2_refresh_tokens WHERE token_hash = ?` +} + +func (sqlite) txOptions() *sql.TxOptions { return nil } diff --git a/oauth2/store/sql/dialect_family_internal_test.go b/oauth2/store/sql/dialect_family_internal_test.go new file mode 100644 index 0000000..cd793bc --- /dev/null +++ b/oauth2/store/sql/dialect_family_internal_test.go @@ -0,0 +1,66 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "database/sql" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestDialectFamilyStatements(t *testing.T) { + t.Parallel() + + const ( + onConflict = `INSERT INTO oauth2_token_families (family_id, revoked_at, expires_at) VALUES (?, ?, ?) + ON CONFLICT (family_id) DO NOTHING` + onDuplicate = `INSERT INTO oauth2_token_families (family_id, revoked_at, expires_at) VALUES (?, ?, ?) + ON DUPLICATE KEY UPDATE family_id = family_id` + familyForUpdate = `SELECT revoked_at FROM oauth2_token_families WHERE family_id = ? FOR UPDATE` + refreshForUpdate = `SELECT family_id, consumed FROM oauth2_refresh_tokens WHERE token_hash = ? FOR UPDATE` + ) + + cases := []struct { + dialect Dialect + insertFamily, lockFamily, lockRefresh string + txOptions *sql.TxOptions + }{ + // PostgreSQL pins READ COMMITTED: a locking read then re-reads the + // latest committed row instead of failing on a concurrent update. + { + postgres{}, onConflict, familyForUpdate, refreshForUpdate, + &sql.TxOptions{Isolation: sql.LevelReadCommitted}, + }, + {mysql{}, onDuplicate, familyForUpdate, refreshForUpdate, nil}, + // SQLite has no FOR UPDATE: its single writer serializes instead. + { + sqlite{}, onConflict, + `SELECT revoked_at FROM oauth2_token_families WHERE family_id = ?`, + `SELECT family_id, consumed FROM oauth2_refresh_tokens WHERE token_hash = ?`, + nil, + }, + } + + for _, tc := range cases { + t.Run(tc.dialect.Name(), func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tc.insertFamily, tc.dialect.insertFamily()) + assert.Equal(t, tc.lockFamily, tc.dialect.lockFamily()) + assert.Equal(t, tc.lockRefresh, tc.dialect.lockRefresh()) + assert.Equal(t, tc.txOptions, tc.dialect.txOptions()) + }) + } + + // PostgreSQL numbers the placeholders of the family statements too. + assert.Equal(t, + `INSERT INTO oauth2_token_families (family_id, revoked_at, expires_at) VALUES ($1, $2, $3) + ON CONFLICT (family_id) DO NOTHING`, + postgres{}.rebind(postgres{}.insertFamily())) + assert.Equal(t, + `SELECT family_id, consumed FROM oauth2_refresh_tokens WHERE token_hash = $1 FOR UPDATE`, + postgres{}.rebind(postgres{}.lockRefresh())) +} diff --git a/oauth2/store/sql/doc.go b/oauth2/store/sql/doc.go index caf771a..bf3ed8d 100644 --- a/oauth2/store/sql/doc.go +++ b/oauth2/store/sql/doc.go @@ -3,8 +3,51 @@ // license that can be found in the LICENSE file. // Package sqlstore is a database/sql implementation of oauth2.Storage with -// real atomicity (transactional ConsumeAuthorizationCode and -// RotateRefreshToken). Dialects supported: PostgreSQL, MySQL, SQLite. +// real atomicity: a transactional ConsumeAuthorizationCode, and one record +// per token family (oauth2_token_families) that every issuance, rotation, +// revocation and lookup goes through. Dialects supported: PostgreSQL, +// MySQL, SQLite. Schema returns the DDL; Store.Migrate applies it. +// +// # Deployment requirements +// +// The token-family guarantees of oauth2.Storage hold under these +// conditions; outside them a revocation can be lost or hidden. +// +// - PostgreSQL: the store runs its token-family transactions — issuance, +// rotation, revocation — at READ COMMITTED, whatever the server +// default, so that SELECT … FOR UPDATE re-reads the latest committed +// family row. synchronous_commit=off can lose an acknowledged +// revocation in a crash. +// - MySQL: the tables must use InnoDB (row locks), and lookups must run +// in autocommit mode — the default — so each one sees the latest +// commits. +// - SQLite: one writer at a time. Set a busy timeout (with +// modernc.org/sqlite, the _pragma=busy_timeout(5000) DSN option) so a +// concurrent writer waits instead of failing with SQLITE_BUSY; a busy +// error fails the operation, closed. +// - Lookups must reach the primary database: a replica DSN or a +// read/write-splitting proxy could answer from before a revocation. +// +// The CI of this module exercises SQLite only: the PostgreSQL and MySQL +// guarantees rest on the locking described in [Store]. +// +// # Cleanup +// +// A revocation never deletes tokens and the store ships no expiry job: +// rows stay until an operator deletes them. Every table carries expires_at +// (Unix seconds), and a family record outlives its tokens — and is kept for +// oauth2.FamilyRevocationRetention after a revocation. Delete expired rows +// ("expires_at < now") table by table, in small batches by primary key and +// outside long transactions (autocommit, or READ COMMITTED). Delete families +// in statements of their own: a transaction deleting tokens then families +// takes their locks in the reverse order of the store and can deadlock with +// it. On MySQL, a range delete on the unindexed expires_at column takes +// next-key locks: index it, or delete by primary key. +// +// Never delete a family row before its expires_at. A missing row fails +// closed for lookups — the store treats the tokens of an unknown family as +// revoked — but the next issuance into a family whose revoked row is gone +// records the family active again, reviving its tokens. // // Allowed dependencies: // - github.com/hyperscale-stack/security/oauth2 diff --git a/oauth2/store/sql/schema.go b/oauth2/store/sql/schema.go index 39d3dc2..5819cf7 100644 --- a/oauth2/store/sql/schema.go +++ b/oauth2/store/sql/schema.go @@ -9,20 +9,23 @@ import ( "fmt" ) -// Schema returns the DDL statements that create the three tables backing -// the store, for the given dialect. Timestamps are stored as BIGINT Unix -// seconds to dodge the TIMESTAMP / DATETIME portability minefield between -// engines. Token / code raw values are NEVER stored — only their hashes. +// Schema returns the DDL statements that create the four tables backing +// the store: authorization codes, access tokens, refresh tokens, and the +// token families — one record per family, active (revoked_at = 0) or +// revoked (revoked_at = Unix second of the first revocation), retained +// until expires_at. Timestamps are stored as BIGINT Unix seconds to dodge the +// TIMESTAMP / DATETIME portability minefield between engines, and flags as +// SMALLINT 0/1 since PostgreSQL has no implicit integer/boolean cast. Token +// / code raw values are NEVER stored — only their hashes. +// +// The DDL sticks to the subset every supported engine accepts, so it is +// the same for every [Dialect]: the argument is accepted for API stability +// and ignored. // // The statements are idempotent (CREATE TABLE IF NOT EXISTS). Production // deployments typically run them through a migration tool rather than via // [Store.Migrate], but Migrate is offered for tests and small setups. -func Schema(d Dialect) []string { - boolType := "BOOLEAN" - if d.Name() == dialectSQLite { - boolType = "INTEGER" - } - +func Schema(_ Dialect) []string { return []string{ `CREATE TABLE IF NOT EXISTS oauth2_auth_codes ( code_hash VARCHAR(128) PRIMARY KEY, @@ -46,18 +49,21 @@ func Schema(d Dialect) []string { issued_at BIGINT NOT NULL, expires_at BIGINT NOT NULL )`, - `CREATE INDEX IF NOT EXISTS idx_oauth2_access_family ON oauth2_access_tokens (family_id)`, - fmt.Sprintf(`CREATE TABLE IF NOT EXISTS oauth2_refresh_tokens ( + `CREATE TABLE IF NOT EXISTS oauth2_refresh_tokens ( token_hash VARCHAR(128) PRIMARY KEY, client_id VARCHAR(255) NOT NULL, subject VARCHAR(255) NOT NULL, scope TEXT NOT NULL, family_id VARCHAR(64) NOT NULL, - consumed %s NOT NULL DEFAULT 0, + consumed SMALLINT NOT NULL DEFAULT 0, issued_at BIGINT NOT NULL, expires_at BIGINT NOT NULL - )`, boolType), - `CREATE INDEX IF NOT EXISTS idx_oauth2_refresh_family ON oauth2_refresh_tokens (family_id)`, + )`, + `CREATE TABLE IF NOT EXISTS oauth2_token_families ( + family_id VARCHAR(64) PRIMARY KEY, + revoked_at BIGINT NOT NULL DEFAULT 0, + expires_at BIGINT NOT NULL + )`, } } diff --git a/oauth2/store/sql/schema_test.go b/oauth2/store/sql/schema_test.go new file mode 100644 index 0000000..bf0721a --- /dev/null +++ b/oauth2/store/sql/schema_test.go @@ -0,0 +1,34 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore_test + +import ( + "strings" + "testing" + + sqlstore "github.com/hyperscale-stack/security/oauth2/store/sql" + "github.com/stretchr/testify/assert" +) + +// TestSchemaIsPortable pins a DDL every supported engine accepts: +// PostgreSQL has no implicit integer/boolean cast (BOOLEAN DEFAULT 0 and +// consumed = 1 are rejected) and MySQL has no CREATE INDEX IF NOT EXISTS, +// so the schema sticks to the portable subset and is the same everywhere. +func TestSchemaIsPortable(t *testing.T) { + t.Parallel() + + want := sqlstore.Schema(sqlstore.SQLite) + + for _, d := range []sqlstore.Dialect{sqlstore.Postgres, sqlstore.MySQL} { + assert.Equal(t, want, sqlstore.Schema(d), d.Name()) + } + + for _, stmt := range want { + assert.NotContains(t, strings.ToUpper(stmt), "BOOLEAN", stmt) + assert.NotContains(t, strings.ToUpper(stmt), "CREATE INDEX", stmt) + } + + assert.Contains(t, strings.Join(want, "\n"), "consumed SMALLINT") +} diff --git a/oauth2/store/sql/store.go b/oauth2/store/sql/store.go index 61c59b8..48dcc83 100644 --- a/oauth2/store/sql/store.go +++ b/oauth2/store/sql/store.go @@ -14,11 +14,31 @@ import ( "github.com/hyperscale-stack/security/oauth2" ) -// Store is a database/sql-backed [oauth2.Storage]. Atomicity of the -// single-use operations (ConsumeAuthorizationCode, RotateRefreshToken) is -// guaranteed by transactions plus affected-row checks — no SELECT…FOR -// UPDATE is needed because the winning DELETE / UPDATE is the one that -// reports RowsAffected()==1. +// Store is a database/sql-backed [oauth2.Storage]. +// +// ConsumeAuthorizationCode runs in a transaction whose DELETE decides the +// winner when two callers race (RowsAffected()==1). +// +// The token operations implement the token-family contract of +// [oauth2.Storage] on one record per family (oauth2_token_families), the +// point every write into the family serializes on: +// +// - SaveTokenPair and RotateRefreshToken each run in one transaction that +// starts with a write — recording the family unless it exists — then +// locks the family row (SELECT … FOR UPDATE; SQLite's single writer +// serializes the transactions instead), refuses a revoked family, and +// only then writes the tokens. A rotation records an unknown family as +// revoked: only an issuance creates an active family. +// - RevokeRefreshFamily records the family revoked in one transaction, +// whether or not any of its tokens exists. On reuse, RotateRefreshToken +// does the same inside its own transaction, which already holds the +// family lock, and commits it. +// - Lookups are single statements joining the family record: a token of +// a revoked family — or of a family the table no longer holds — is +// reported revoked, a refresh token as consumed. +// +// Revocation therefore never touches token rows: they stay until an +// operator deletes expired rows (see the package documentation). type Store struct { db *sql.DB dialect Dialect @@ -128,41 +148,47 @@ func (s *Store) ConsumeAuthorizationCode(ctx context.Context, codeHash string) ( return code, nil } -// --- access tokens ------------------------------------------------------- +// --- access and refresh tokens ------------------------------------------- -// SaveAccessToken implements [oauth2.AccessTokenStore]. -func (s *Store) SaveAccessToken(ctx context.Context, t *oauth2.AccessToken) error { - if t.TokenHash == "" { - return oauth2.ErrInvalidRequest.WithDescription("sqlstore: empty access token hash") +// SaveTokenPair implements [oauth2.Storage]: one transaction joins the +// pair's family — recording it active when unknown, refusing it once +// revoked — and inserts both tokens. A pair outside any family is inserted +// without a family record. +func (s *Store) SaveTokenPair(ctx context.Context, pair *oauth2.TokenPair) error { + if err := pair.Validate(); err != nil { + return fmt.Errorf("sqlstore: save token pair: %w", err) } - const q = `INSERT INTO oauth2_access_tokens - (token_hash, client_id, subject, scope, family_id, audience, issued_at, expires_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?)` - - _, err := s.exec(ctx, q, - t.TokenHash, t.ClientID, t.Subject, t.Scope, t.FamilyID, t.Audience, - t.IssuedAt.Unix(), t.ExpiresAt.Unix()) - if err != nil { - return fmt.Errorf("sqlstore: save access token: %w", err) - } + return s.inTx(ctx, func(tx *sql.Tx) error { + if familyID := pair.Access.FamilyID; familyID != "" { + if err := s.joinFamily(ctx, tx, familyID, pairExpiry(pair)); err != nil { + return err + } + } - return nil + return s.insertPair(ctx, tx, pair) + }) } -// LookupAccessToken implements [oauth2.AccessTokenStore]. +// LookupAccessToken implements [oauth2.AccessTokenStore]. One statement +// reads the token and its family record: a token of a revoked or unknown +// family is reported as [oauth2.ErrTokenFamilyRevoked]. func (s *Store) LookupAccessToken(ctx context.Context, tokenHash string) (*oauth2.AccessToken, error) { - const q = `SELECT client_id, subject, scope, family_id, audience, issued_at, expires_at - FROM oauth2_access_tokens WHERE token_hash = ?` + const q = `SELECT t.client_id, t.subject, t.scope, t.family_id, t.audience, t.issued_at, t.expires_at, + f.revoked_at + FROM oauth2_access_tokens t + LEFT JOIN oauth2_token_families f ON f.family_id = t.family_id + WHERE t.token_hash = ?` var ( t = &oauth2.AccessToken{TokenHash: tokenHash} issuedAt, expires int64 + revokedAt sql.NullInt64 ) row := s.db.QueryRowContext(ctx, s.dialect.rebind(q), tokenHash) if err := row.Scan(&t.ClientID, &t.Subject, &t.Scope, &t.FamilyID, &t.Audience, - &issuedAt, &expires); err != nil { + &issuedAt, &expires, &revokedAt); err != nil { if errors.Is(err, sql.ErrNoRows) { return nil, oauth2.ErrInvalidGrant.WithDescription("access token not found") } @@ -170,6 +196,10 @@ func (s *Store) LookupAccessToken(ctx context.Context, tokenHash string) (*oauth return nil, fmt.Errorf("sqlstore: lookup access token: %w", err) } + if !familyActive(t.FamilyID, revokedAt) { + return nil, oauth2.ErrTokenFamilyRevoked + } + t.IssuedAt = time.Unix(issuedAt, 0) t.ExpiresAt = time.Unix(expires, 0) @@ -185,75 +215,224 @@ func (s *Store) RevokeAccessToken(ctx context.Context, tokenHash string) error { return nil } -// --- refresh tokens ------------------------------------------------------ +// LookupRefreshToken implements [oauth2.RefreshTokenStore]. One statement +// reads the token and its family record: a token of a revoked or unknown +// family comes back consumed — and so does a token stored without a +// family, which no valid pair carries. +func (s *Store) LookupRefreshToken(ctx context.Context, tokenHash string) (*oauth2.RefreshToken, error) { + const q = `SELECT t.client_id, t.subject, t.scope, t.family_id, t.consumed, t.issued_at, t.expires_at, + f.revoked_at + FROM oauth2_refresh_tokens t + LEFT JOIN oauth2_token_families f ON f.family_id = t.family_id + WHERE t.token_hash = ?` + + var ( + t = &oauth2.RefreshToken{TokenHash: tokenHash} + consumed int64 + issuedAt, expires int64 + revokedAt sql.NullInt64 + ) + + row := s.db.QueryRowContext(ctx, s.dialect.rebind(q), tokenHash) + if err := row.Scan(&t.ClientID, &t.Subject, &t.Scope, &t.FamilyID, + &consumed, &issuedAt, &expires, &revokedAt); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, oauth2.ErrInvalidGrant.WithDescription("refresh token not found") + } -// SaveRefreshToken implements [oauth2.RefreshTokenStore]. -func (s *Store) SaveRefreshToken(ctx context.Context, t *oauth2.RefreshToken) error { - if t.TokenHash == "" { - return oauth2.ErrInvalidRequest.WithDescription("sqlstore: empty refresh token hash") + return nil, fmt.Errorf("sqlstore: lookup refresh token: %w", err) } - const q = `INSERT INTO oauth2_refresh_tokens - (token_hash, client_id, subject, scope, family_id, consumed, issued_at, expires_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?)` + t.Consumed = consumed != 0 || t.FamilyID == "" || !familyActive(t.FamilyID, revokedAt) + t.IssuedAt = time.Unix(issuedAt, 0) + t.ExpiresAt = time.Unix(expires, 0) - _, err := s.exec(ctx, q, - t.TokenHash, t.ClientID, t.Subject, t.Scope, t.FamilyID, boolToInt(t.Consumed), - t.IssuedAt.Unix(), t.ExpiresAt.Unix()) + return t, nil +} + +// RotateRefreshToken implements [oauth2.RefreshTokenStore] in one +// transaction: +// +// 1. Lock next's family — recording it revoked if unknown, since a +// rotation never creates an active family — then read the old token +// under lock. +// 2. Decide in the contract's order: unknown old token (invalid_grant), +// next in another family (invalid_request), revoked family +// ([oauth2.ErrTokenFamilyRevoked]), consumed old token (reuse). +// 3. On reuse, record the family revoked and commit. A failure there is +// reported as [oauth2.ErrRefreshTokenReused] joined with +// [oauth2.ErrTokenFamilyRevocationFailed] and its cause. +// 4. Otherwise consume the old token, extend the family's retention, +// insert next and commit. +func (s *Store) RotateRefreshToken(ctx context.Context, oldHash string, next *oauth2.TokenPair) error { + if err := validateRotation(next); err != nil { + return err + } + + tx, err := s.db.BeginTx(ctx, s.dialect.txOptions()) if err != nil { - return fmt.Errorf("sqlstore: save refresh token: %w", err) + return fmt.Errorf("sqlstore: begin: %w", err) + } + + defer func() { _ = tx.Rollback() }() // a no-op once committed + + reused, err := s.rotateTx(ctx, tx, oldHash, next) + if err != nil { + return err + } + + if reused { + return s.revokeReused(ctx, tx, next.Access.FamilyID) + } + + if err := tx.Commit(); err != nil { + return fmt.Errorf("sqlstore: commit: %w", err) } return nil } -// LookupRefreshToken implements [oauth2.RefreshTokenStore]. -func (s *Store) LookupRefreshToken(ctx context.Context, tokenHash string) (*oauth2.RefreshToken, error) { - const q = `SELECT client_id, subject, scope, family_id, consumed, issued_at, expires_at - FROM oauth2_refresh_tokens WHERE token_hash = ?` +// RevokeRefreshFamily implements [oauth2.RefreshTokenStore]: one +// transaction records the family revoked — inserting the record if the +// family is unknown, keeping the time of a first revocation otherwise — +// and extends its retention to [oauth2.FamilyRevocationRetention] from +// now. A failure wraps [oauth2.ErrTokenFamilyRevocationFailed]. +func (s *Store) RevokeRefreshFamily(ctx context.Context, familyID string) error { + if err := checkFamilyID(familyID); err != nil { + return err + } - t, err := scanRefresh(s.db.QueryRowContext(ctx, s.dialect.rebind(q), tokenHash), tokenHash) - if err != nil { - if errors.Is(err, sql.ErrNoRows) { - return nil, oauth2.ErrInvalidGrant.WithDescription("refresh token not found") + err := s.inTx(ctx, func(tx *sql.Tx) error { + now := time.Now() + + if err := s.recordFamily(ctx, tx, familyID, now.Unix(), retentionFloor(now)); err != nil { + return err } - return nil, fmt.Errorf("sqlstore: lookup refresh token: %w", err) + return s.markRevoked(ctx, tx, familyID, now) + }) + if err != nil { + return revocationFailed(familyID, err) } - return t, nil + return nil } -// RotateRefreshToken implements [oauth2.RefreshTokenStore]. The whole -// sequence runs in one transaction: -// -// 1. UPDATE the old token to consumed=1 WHERE consumed=0. The -// RowsAffected()==1 check is the atomic gate — a concurrent rotation -// that already flipped the row gets 0. -// 2. On 0 rows, the token was reused: revoke the family and return -// ErrRefreshTokenReused. -// 3. On 1 row, INSERT the new token and commit. -func (s *Store) RotateRefreshToken(ctx context.Context, oldHash string, next *oauth2.RefreshToken) error { - tx, err := s.db.BeginTx(ctx, nil) +// --- transaction steps ----------------------------------------------------- + +// inTx runs fn in a transaction with the dialect's options, committing when +// fn succeeds and rolling back otherwise. fn uses tx only: with a +// single-connection pool, touching s.db while the transaction is open +// would deadlock. +func (s *Store) inTx(ctx context.Context, fn func(*sql.Tx) error) error { + tx, err := s.db.BeginTx(ctx, s.dialect.txOptions()) if err != nil { return fmt.Errorf("sqlstore: begin: %w", err) } - defer func() { _ = tx.Rollback() }() + if err := fn(tx); err != nil { + _ = tx.Rollback() - // Fetch family id (needed for the reuse-revocation path). - var familyID string + return err + } - famRow := tx.QueryRowContext(ctx, s.dialect.rebind( - `SELECT family_id FROM oauth2_refresh_tokens WHERE token_hash = ?`), oldHash) - if err := famRow.Scan(&familyID); err != nil { - if errors.Is(err, sql.ErrNoRows) { - return oauth2.ErrInvalidGrant.WithDescription("refresh token not found") - } + if err := tx.Commit(); err != nil { + return fmt.Errorf("sqlstore: commit: %w", err) + } + + return nil +} - return fmt.Errorf("sqlstore: select refresh token: %w", err) +// recordFamily inserts the record (familyID, revokedAt, expiresAt) unless +// the family already has one. It is the first statement of every write +// transaction on a family: a write first takes SQLite's write lock before +// anything is read. +func (s *Store) recordFamily(ctx context.Context, tx *sql.Tx, familyID string, revokedAt, expiresAt int64) error { + if _, err := tx.ExecContext(ctx, s.dialect.rebind(s.dialect.insertFamily()), + familyID, revokedAt, expiresAt); err != nil { + return fmt.Errorf("sqlstore: record token family: %w", err) } + return nil +} + +// lockFamily records familyID with revokedAt and expiresAt unless it +// exists, then reads whether it is revoked under a row lock. A record +// missing even then reads as revoked. +func (s *Store) lockFamily(ctx context.Context, tx *sql.Tx, familyID string, revokedAt, expiresAt int64) (bool, error) { + if err := s.recordFamily(ctx, tx, familyID, revokedAt, expiresAt); err != nil { + return false, err + } + + var current int64 + + err := tx.QueryRowContext(ctx, s.dialect.rebind(s.dialect.lockFamily()), familyID).Scan(¤t) + + switch { + case errors.Is(err, sql.ErrNoRows): + return true, nil + case err != nil: + return false, fmt.Errorf("sqlstore: lock token family: %w", err) + default: + return current != 0, nil + } +} + +// joinFamily locks familyID for an issuance, recording it active until +// expiresAt when unknown: a revoked family refuses the issuance, an active +// one has its retention extended to expiresAt. +func (s *Store) joinFamily(ctx context.Context, tx *sql.Tx, familyID string, expiresAt int64) error { + revoked, err := s.lockFamily(ctx, tx, familyID, 0, expiresAt) + if err != nil { + return err + } + + if revoked { + return oauth2.ErrTokenFamilyRevoked + } + + return s.extendFamily(ctx, tx, familyID, expiresAt) +} + +// rotateTx runs the reads of a rotation and, unless the old token was +// already consumed, its writes; it reports a reuse to the caller, which +// owns the transaction. +func (s *Store) rotateTx(ctx context.Context, tx *sql.Tx, oldHash string, next *oauth2.TokenPair) (bool, error) { + familyID := next.Access.FamilyID + now := time.Now() + + revoked, err := s.lockFamily(ctx, tx, familyID, now.Unix(), retentionFloor(now)) + if err != nil { + return false, err + } + + var ( + oldFamily string + consumed int64 + ) + + err = tx.QueryRowContext(ctx, s.dialect.rebind(s.dialect.lockRefresh()), oldHash).Scan(&oldFamily, &consumed) + + switch { + case errors.Is(err, sql.ErrNoRows): + return false, oauth2.ErrInvalidGrant.WithDescription("refresh token not found") + case err != nil: + return false, fmt.Errorf("sqlstore: lock refresh token: %w", err) + case oldFamily != familyID: + return false, oauth2.ErrInvalidRequest.WithDescription("sqlstore: rotation into another token family") + case revoked: + return false, oauth2.ErrTokenFamilyRevoked + case consumed != 0: + return true, nil + } + + return false, s.consumeAndInsert(ctx, tx, oldHash, next) +} + +// consumeAndInsert applies a rotation rotateTx cleared: it consumes the +// old token, extends the family's retention to next's tokens and inserts +// them. +func (s *Store) consumeAndInsert(ctx context.Context, tx *sql.Tx, oldHash string, next *oauth2.TokenPair) error { res, err := tx.ExecContext(ctx, s.dialect.rebind( `UPDATE oauth2_refresh_tokens SET consumed = 1 WHERE token_hash = ? AND consumed = 0`), oldHash) if err != nil { @@ -265,80 +444,153 @@ func (s *Store) RotateRefreshToken(ctx context.Context, oldHash string, next *oa return fmt.Errorf("sqlstore: rows affected: %w", err) } + // The token was read unconsumed under lock: any other count means the + // lock did not hold, and the rotation must not go on. if affected != 1 { - // Reuse: the token was already consumed. Revoke the family in a - // separate transaction after rolling this one back. - _ = tx.Rollback() - _ = s.RevokeRefreshFamily(ctx, familyID) + return fmt.Errorf("sqlstore: consume refresh token: %d rows affected", affected) + } - return oauth2.ErrRefreshTokenReused + if err := s.extendFamily(ctx, tx, next.Access.FamilyID, pairExpiry(next)); err != nil { + return err } + return s.insertPair(ctx, tx, next) +} + +// revokeReused answers a reuse rotateTx detected: within the rotation's +// transaction, which holds the family lock, it records the family revoked +// and commits, then reports the reuse. +func (s *Store) revokeReused(ctx context.Context, tx *sql.Tx, familyID string) error { + err := s.markRevoked(ctx, tx, familyID, time.Now()) + if err == nil { + err = tx.Commit() + } + + if err != nil { + return fmt.Errorf("%w: %w", oauth2.ErrRefreshTokenReused, revocationFailed(familyID, err)) + } + + return oauth2.ErrRefreshTokenReused +} + +// markRevoked records familyID revoked — keeping the time of a first +// revocation — and extends its retention to the floor a revocation +// guarantees. +func (s *Store) markRevoked(ctx context.Context, tx *sql.Tx, familyID string, now time.Time) error { if _, err := tx.ExecContext(ctx, s.dialect.rebind( - `INSERT INTO oauth2_refresh_tokens - (token_hash, client_id, subject, scope, family_id, consumed, issued_at, expires_at) - VALUES (?, ?, ?, ?, ?, 0, ?, ?)`), - next.TokenHash, next.ClientID, next.Subject, next.Scope, next.FamilyID, - next.IssuedAt.Unix(), next.ExpiresAt.Unix(), - ); err != nil { - return fmt.Errorf("sqlstore: insert rotated refresh token: %w", err) + `UPDATE oauth2_token_families SET revoked_at = ? WHERE family_id = ? AND revoked_at = 0`), + now.Unix(), familyID); err != nil { + return fmt.Errorf("sqlstore: mark token family revoked: %w", err) } - if err := tx.Commit(); err != nil { - return fmt.Errorf("sqlstore: commit: %w", err) + return s.extendFamily(ctx, tx, familyID, retentionFloor(now)) +} + +// extendFamily raises familyID's retention to until; it never lowers it. +func (s *Store) extendFamily(ctx context.Context, tx *sql.Tx, familyID string, until int64) error { + if _, err := tx.ExecContext(ctx, s.dialect.rebind( + `UPDATE oauth2_token_families SET expires_at = ? WHERE family_id = ? AND expires_at < ?`), + until, familyID, until); err != nil { + return fmt.Errorf("sqlstore: extend token family: %w", err) } return nil } -// RevokeRefreshFamily implements [oauth2.RefreshTokenStore]: every refresh -// token of the family is marked consumed and every access token of the -// family is deleted. -func (s *Store) RevokeRefreshFamily(ctx context.Context, familyID string) error { - if _, err := s.exec(ctx, - `UPDATE oauth2_refresh_tokens SET consumed = 1 WHERE family_id = ?`, familyID); err != nil { - return fmt.Errorf("sqlstore: revoke refresh family: %w", err) +// insertPair inserts the tokens of pair. +func (s *Store) insertPair(ctx context.Context, tx *sql.Tx, pair *oauth2.TokenPair) error { + at := &pair.Access + + if _, err := tx.ExecContext(ctx, s.dialect.rebind(`INSERT INTO oauth2_access_tokens + (token_hash, client_id, subject, scope, family_id, audience, issued_at, expires_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?)`), + at.TokenHash, at.ClientID, at.Subject, at.Scope, at.FamilyID, at.Audience, + at.IssuedAt.Unix(), at.ExpiresAt.Unix()); err != nil { + return fmt.Errorf("sqlstore: insert access token: %w", err) } - if _, err := s.exec(ctx, - `DELETE FROM oauth2_access_tokens WHERE family_id = ?`, familyID); err != nil { - return fmt.Errorf("sqlstore: purge family access tokens: %w", err) + rt := pair.Refresh + if rt == nil { + return nil + } + + if _, err := tx.ExecContext(ctx, s.dialect.rebind(`INSERT INTO oauth2_refresh_tokens + (token_hash, client_id, subject, scope, family_id, consumed, issued_at, expires_at) + VALUES (?, ?, ?, ?, ?, 0, ?, ?)`), + rt.TokenHash, rt.ClientID, rt.Subject, rt.Scope, rt.FamilyID, + rt.IssuedAt.Unix(), rt.ExpiresAt.Unix()); err != nil { + return fmt.Errorf("sqlstore: insert refresh token: %w", err) } return nil } -// rowScanner abstracts *sql.Row so scanRefresh works with QueryRow results. -type rowScanner interface { - Scan(dest ...any) error +// --- helpers --------------------------------------------------------------- + +// familyActive reports whether a token of familyID may be used: it is +// outside any family, or its family's record says active. A NULL revokedAt +// — no record joined — reads as revoked: a lookup never takes a lost family +// for an active one. +func familyActive(familyID string, revokedAt sql.NullInt64) bool { + if familyID == "" { + return true + } + + return revokedAt.Valid && revokedAt.Int64 == 0 } -// scanRefresh decodes a refresh-token row. -func scanRefresh(row rowScanner, hash string) (*oauth2.RefreshToken, error) { - var ( - t = &oauth2.RefreshToken{TokenHash: hash} - consumed int64 - issuedAt, expires int64 - ) +// pairExpiry is the Unix second the family of pair must be retained +// until: the later of the pair's token expiries. +func pairExpiry(pair *oauth2.TokenPair) int64 { + until := pair.Access.ExpiresAt.Unix() - if err := row.Scan(&t.ClientID, &t.Subject, &t.Scope, &t.FamilyID, - &consumed, &issuedAt, &expires); err != nil { - return nil, err //nolint:wrapcheck // caller classifies sql.ErrNoRows + if pair.Refresh != nil && pair.Refresh.ExpiresAt.Unix() > until { + until = pair.Refresh.ExpiresAt.Unix() } - t.Consumed = consumed != 0 - t.IssuedAt = time.Unix(issuedAt, 0) - t.ExpiresAt = time.Unix(expires, 0) + return until +} - return t, nil +// retentionFloor is the Unix second a family revoked at now is retained +// until, at least. +func retentionFloor(now time.Time) int64 { + return now.Add(oauth2.FamilyRevocationRetention).Unix() } -func boolToInt(b bool) int { - if b { - return 1 +// validateRotation refuses a malformed rotation target before anything +// runs: nil, failing [oauth2.TokenPair.Validate], or without the refresh +// token a rotation issues. +func validateRotation(next *oauth2.TokenPair) error { + if err := next.Validate(); err != nil { + return fmt.Errorf("sqlstore: rotate refresh token: %w", err) } - return 0 + if next.Refresh == nil { + return oauth2.ErrInvalidRequest.WithDescription("sqlstore: rotation without a refresh token") + } + + return nil +} + +// checkFamilyID refuses a family ID no family can have: empty, which would +// reach every token outside a family, or longer than +// [oauth2.MaxFamilyIDLength]. +func checkFamilyID(familyID string) error { + switch { + case familyID == "": + return oauth2.ErrInvalidRequest.WithDescription("sqlstore: empty token family id") + case len(familyID) > oauth2.MaxFamilyIDLength: + return oauth2.ErrInvalidRequest.WithDescription("sqlstore: token family id too long") + default: + return nil + } +} + +// revocationFailed reports a family revocation that could not be recorded: +// the family ID, then [oauth2.ErrTokenFamilyRevocationFailed], then the +// cause. +func revocationFailed(familyID string, err error) error { + return fmt.Errorf("sqlstore: revoke family %q: %w: %w", familyID, oauth2.ErrTokenFamilyRevocationFailed, err) } // Compile-time interface check. diff --git a/oauth2/store/sql/store_error_test.go b/oauth2/store/sql/store_error_test.go index f66c853..3973095 100644 --- a/oauth2/store/sql/store_error_test.go +++ b/oauth2/store/sql/store_error_test.go @@ -7,11 +7,13 @@ package sqlstore_test import ( "context" "database/sql" + "strings" "testing" "time" "github.com/hyperscale-stack/security/oauth2" sqlstore "github.com/hyperscale-stack/security/oauth2/store/sql" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" _ "modernc.org/sqlite" ) @@ -51,40 +53,47 @@ func TestSQLStoreReportsBackendErrors(t *testing.T) { _, err := store.ConsumeAuthorizationCode(ctx, "h") require.Error(t, err) - at := &oauth2.AccessToken{ - TokenHash: "h", ClientID: "c", Subject: "s", Scope: "r", - IssuedAt: now, ExpiresAt: now.Add(time.Minute), - } - require.Error(t, store.SaveAccessToken(ctx, at)) + pair := testPair("h", "f", time.Minute) + require.Error(t, store.SaveTokenPair(ctx, pair)) + + // Outside any family the pair skips the family record: the access + // token insert is the failing statement. + require.Error(t, store.SaveTokenPair(ctx, &oauth2.TokenPair{Access: oauth2.AccessToken{ + TokenHash: "h", ClientID: "c", Subject: "s", Scope: "r", IssuedAt: now, ExpiresAt: now.Add(time.Minute), + }})) _, err = store.LookupAccessToken(ctx, "h") require.Error(t, err) require.Error(t, store.RevokeAccessToken(ctx, "h")) - // Consumed=true also exercises the boolToInt true branch. - rt := &oauth2.RefreshToken{ - TokenHash: "h", ClientID: "c", Subject: "s", Scope: "r", FamilyID: "f", - Consumed: true, IssuedAt: now, ExpiresAt: now.Add(time.Minute), - } - require.Error(t, store.SaveRefreshToken(ctx, rt)) - _, err = store.LookupRefreshToken(ctx, "h") require.Error(t, err) - require.Error(t, store.RotateRefreshToken(ctx, "h", rt)) - require.Error(t, store.RevokeRefreshFamily(ctx, "f")) + require.Error(t, store.RotateRefreshToken(ctx, "h", testPair("next", "f", time.Minute))) + require.ErrorIs(t, store.RevokeRefreshFamily(ctx, "f"), oauth2.ErrTokenFamilyRevocationFailed) } -func TestSQLStoreRejectsEmptyHashes(t *testing.T) { +func TestSQLStoreRejectsInvalidInput(t *testing.T) { t.Parallel() store := unmigratedStore(t) ctx := context.Background() - - require.Error(t, store.SaveAuthorizationCode(ctx, &oauth2.AuthorizationCode{})) - require.Error(t, store.SaveAccessToken(ctx, &oauth2.AccessToken{})) - require.Error(t, store.SaveRefreshToken(ctx, &oauth2.RefreshToken{})) + tooLong := strings.Repeat("f", oauth2.MaxFamilyIDLength+1) + bare := &oauth2.TokenPair{Access: oauth2.AccessToken{TokenHash: "h", FamilyID: "f"}} + + // Refused before any statement runs: the database has no table at all. + for _, err := range []error{ + store.SaveAuthorizationCode(ctx, &oauth2.AuthorizationCode{}), + store.SaveTokenPair(ctx, &oauth2.TokenPair{}), + store.SaveTokenPair(ctx, nil), + store.RotateRefreshToken(ctx, "h", nil), + store.RotateRefreshToken(ctx, "h", bare), + store.RevokeRefreshFamily(ctx, ""), + store.RevokeRefreshFamily(ctx, tooLong), + } { + assert.Equal(t, oauth2.CodeInvalidRequest, oauth2.IsCode(err), "%v", err) + } } func TestSQLStoreReportsErrorsOnClosedDB(t *testing.T) { @@ -105,5 +114,7 @@ func TestSQLStoreReportsErrorsOnClosedDB(t *testing.T) { _, err = store.ConsumeAuthorizationCode(ctx, "h") require.Error(t, err) - require.Error(t, store.RotateRefreshToken(ctx, "h", &oauth2.RefreshToken{})) + require.Error(t, store.SaveTokenPair(ctx, testPair("h", "f", time.Minute))) + require.Error(t, store.RotateRefreshToken(ctx, "h", testPair("next", "f", time.Minute))) + require.ErrorIs(t, store.RevokeRefreshFamily(ctx, "f"), oauth2.ErrTokenFamilyRevocationFailed) } diff --git a/oauth2/store/sql/store_family_test.go b/oauth2/store/sql/store_family_test.go new file mode 100644 index 0000000..f6226e1 --- /dev/null +++ b/oauth2/store/sql/store_family_test.go @@ -0,0 +1,419 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore_test + +import ( + "context" + "database/sql" + "fmt" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// injected is the message of the failures the SQLite triggers inject. +const injected = "injected failure" + +// testPair returns a pair in family whose tokens both expire after ttl. +func testPair(name, family string, ttl time.Duration) *oauth2.TokenPair { + now := time.Now() + + return &oauth2.TokenPair{ + Access: oauth2.AccessToken{ + TokenHash: "at-" + name, ClientID: "c", Subject: "s", Scope: "r", Audience: "api", + FamilyID: family, IssuedAt: now, ExpiresAt: now.Add(ttl), + }, + Refresh: &oauth2.RefreshToken{ + TokenHash: "rt-" + name, ClientID: "c", Subject: "s", Scope: "r", + FamilyID: family, IssuedAt: now, ExpiresAt: now.Add(ttl), + }, + } +} + +// inject makes every statement matching spec — a SQLite trigger event +// clause such as "BEFORE INSERT ON t" — fail, until the test drops the +// trigger with the returned function. +func inject(t *testing.T, db *sql.DB, spec string) func() { + t.Helper() + + _, err := db.ExecContext(t.Context(), fmt.Sprintf( + "CREATE TRIGGER injected %s BEGIN SELECT RAISE(ABORT, '%s'); END", spec, injected)) + require.NoError(t, err) + + return func() { + _, err := db.ExecContext(t.Context(), "DROP TRIGGER injected") + require.NoError(t, err) + } +} + +// count runs a SELECT COUNT(*) query. +func count(t *testing.T, db *sql.DB, query string, args ...any) int { + t.Helper() + + var n int + require.NoError(t, db.QueryRowContext(t.Context(), query, args...).Scan(&n)) + + return n +} + +// requirePersisted asserts how many rows of pair's tokens the tables hold. +func requirePersisted(t *testing.T, db *sql.DB, pair *oauth2.TokenPair, want int) { + t.Helper() + + assert.Equal(t, want, count(t, db, + "SELECT COUNT(*) FROM oauth2_access_tokens WHERE token_hash = ?", pair.Access.TokenHash), + "access token %s", pair.Access.TokenHash) + assert.Equal(t, want, count(t, db, + "SELECT COUNT(*) FROM oauth2_refresh_tokens WHERE token_hash = ?", pair.Refresh.TokenHash), + "refresh token %s", pair.Refresh.TokenHash) +} + +// family reads the record of family straight from the table. +func family(t *testing.T, db *sql.DB, id string) (revokedAt, expiresAt int64) { + t.Helper() + + require.NoError(t, db.QueryRowContext(t.Context(), + "SELECT revoked_at, expires_at FROM oauth2_token_families WHERE family_id = ?", id, + ).Scan(&revokedAt, &expiresAt)) + + return revokedAt, expiresAt +} + +// requireUsable asserts that both tokens of pair can still be used. +func requireUsable(t *testing.T, s oauth2.Storage, pair *oauth2.TokenPair) { + t.Helper() + + _, err := s.LookupAccessToken(t.Context(), pair.Access.TokenHash) + require.NoError(t, err) + + rt, err := s.LookupRefreshToken(t.Context(), pair.Refresh.TokenHash) + require.NoError(t, err) + assert.False(t, rt.Consumed, "refresh token %s", pair.Refresh.TokenHash) +} + +// requireUnusable asserts that no token of pair can be used any more. +func requireUnusable(t *testing.T, s oauth2.Storage, pair *oauth2.TokenPair) { + t.Helper() + + _, err := s.LookupAccessToken(t.Context(), pair.Access.TokenHash) + assert.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked) + + rt, err := s.LookupRefreshToken(t.Context(), pair.Refresh.TokenHash) + require.NoError(t, err) + assert.True(t, rt.Consumed, "refresh token %s", pair.Refresh.TokenHash) +} + +func TestSQLRevokeFamilyFailureIsReported(t *testing.T) { + t.Parallel() + + // One failing statement per subtest — SQLite fires a BEFORE INSERT + // trigger even when ON CONFLICT DO NOTHING then skips the row. + cases := []struct{ name, trigger string }{ + {"recording the family", "BEFORE INSERT ON oauth2_token_families WHEN NEW.revoked_at <> 0"}, + {"marking it revoked", "BEFORE UPDATE OF revoked_at ON oauth2_token_families"}, + {"extending its retention", "BEFORE UPDATE OF expires_at ON oauth2_token_families"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + pair := testPair("revoke", "fam-revoke", time.Hour) + require.NoError(t, store.SaveTokenPair(t.Context(), pair)) + + drop := inject(t, db, tc.trigger) + + err := store.RevokeRefreshFamily(t.Context(), "fam-revoke") + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevocationFailed) + assert.Empty(t, oauth2.IsCode(err), "not an OAuth2 error: a caller answers server_error") + assert.ErrorContains(t, err, `"fam-revoke"`) + assert.ErrorContains(t, err, injected) + + // The failed revocation applied nothing... + requireUsable(t, store, pair) + + // ...and a retry, once the backend recovered, revokes. + drop() + require.NoError(t, store.RevokeRefreshFamily(t.Context(), "fam-revoke")) + requireUnusable(t, store, pair) + }) + } +} + +func TestSQLReuseRevocationFailureIsReported(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + first := testPair("reuse-0", "fam-reuse", time.Hour) + second := testPair("reuse-1", "fam-reuse", time.Hour) + replay := testPair("reuse-2", "fam-reuse", time.Hour) + + require.NoError(t, store.SaveTokenPair(t.Context(), first)) + require.NoError(t, store.RotateRefreshToken(t.Context(), first.Refresh.TokenHash, second)) + + drop := inject(t, db, "BEFORE UPDATE OF revoked_at ON oauth2_token_families") + + err := store.RotateRefreshToken(t.Context(), first.Refresh.TokenHash, replay) + require.ErrorIs(t, err, oauth2.ErrRefreshTokenReused) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevocationFailed) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + assert.ErrorContains(t, err, `"fam-reuse"`) + assert.ErrorContains(t, err, injected) + + // The store says the family is NOT revoked, and it is not. + requireUsable(t, store, second) + requirePersisted(t, db, replay, 0) + + drop() + require.NoError(t, store.RevokeRefreshFamily(t.Context(), "fam-reuse")) + requireUnusable(t, store, second) +} + +func TestSQLRotationIsAtomic(t *testing.T) { + t.Parallel() + + cases := []struct{ name, trigger string }{ + {"access token insert", "BEFORE INSERT ON oauth2_access_tokens"}, + {"refresh token insert", "BEFORE INSERT ON oauth2_refresh_tokens"}, + {"family retention", "BEFORE UPDATE OF expires_at ON oauth2_token_families"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + first := testPair("atomic-0", "fam-atomic", time.Hour) + require.NoError(t, store.SaveTokenPair(t.Context(), first)) + + _, before := family(t, db, "fam-atomic") + drop := inject(t, db, tc.trigger) + + // The replacement outlives the family's retention, so the + // rotation has to extend it too. + next := testPair("atomic-1", "fam-atomic", 48*time.Hour) + err := store.RotateRefreshToken(t.Context(), first.Refresh.TokenHash, next) + require.ErrorContains(t, err, injected) + assert.Empty(t, oauth2.IsCode(err), "a backend failure is no OAuth2 error") + + drop() + requirePersisted(t, db, next, 0) + requireUsable(t, store, first) + + _, after := family(t, db, "fam-atomic") + assert.Equal(t, before, after, "the failed rotation did not extend the family") + }) + } +} + +func TestSQLSaveTokenPairIsAtomic(t *testing.T) { + t.Parallel() + + for _, trigger := range []string{ + "BEFORE INSERT ON oauth2_access_tokens", + "BEFORE INSERT ON oauth2_refresh_tokens", + } { + t.Run(trigger, func(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + inject(t, db, trigger) + + pair := testPair("save", "fam-save", time.Hour) + require.ErrorContains(t, store.SaveTokenPair(t.Context(), pair), injected) + + requirePersisted(t, db, pair, 0) + assert.Zero(t, count(t, db, + "SELECT COUNT(*) FROM oauth2_token_families WHERE family_id = ?", "fam-save"), + "the failed issuance did not create the family") + }) + } +} + +func TestSQLFamilyRetention(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + ctx := t.Context() + before := time.Now() + floor := before.Add(oauth2.FamilyRevocationRetention).Unix() + + // Issuance records the family active, retained as long as its tokens. + first := testPair("ret-0", "fam-ret", time.Hour) + require.NoError(t, store.SaveTokenPair(ctx, first)) + + revokedAt, expiresAt := family(t, db, "fam-ret") + assert.Zero(t, revokedAt) + assert.Equal(t, first.Refresh.ExpiresAt.Unix(), expiresAt) + + // A rotation extends the retention to its tokens. + next := testPair("ret-1", "fam-ret", 48*time.Hour) + require.NoError(t, store.RotateRefreshToken(ctx, first.Refresh.TokenHash, next)) + + _, expiresAt = family(t, db, "fam-ret") + assert.Equal(t, next.Refresh.ExpiresAt.Unix(), expiresAt) + + // The revocation keeps the longer of the tokens and the retention floor. + require.NoError(t, store.RevokeRefreshFamily(ctx, "fam-ret")) + + revokedAt, expiresAt = family(t, db, "fam-ret") + assert.GreaterOrEqual(t, revokedAt, before.Unix()) + assert.Equal(t, next.Refresh.ExpiresAt.Unix(), expiresAt) + + short := testPair("ret-short", "fam-short", time.Minute) + require.NoError(t, store.SaveTokenPair(ctx, short)) + require.NoError(t, store.RevokeRefreshFamily(ctx, "fam-short")) + + _, expiresAt = family(t, db, "fam-short") + assert.GreaterOrEqual(t, expiresAt, floor) + + // A revocation before issuance is retained for the floor too. + require.NoError(t, store.RevokeRefreshFamily(ctx, "fam-unknown")) + + revokedAt, expiresAt = family(t, db, "fam-unknown") + assert.GreaterOrEqual(t, revokedAt, before.Unix()) + assert.GreaterOrEqual(t, expiresAt, floor) + + // Revoking again keeps the first revocation time and never shortens + // the retention. + far := time.Now().Add(100 * time.Hour).Unix() + _, err := db.ExecContext(ctx, + "UPDATE oauth2_token_families SET revoked_at = 1, expires_at = ? WHERE family_id = ?", far, "fam-short") + require.NoError(t, err) + require.NoError(t, store.RevokeRefreshFamily(ctx, "fam-short")) + + revokedAt, expiresAt = family(t, db, "fam-short") + assert.Equal(t, int64(1), revokedAt) + assert.Equal(t, far, expiresAt) +} + +// TestSQLMissingFamilyRecordFailsClosed covers a family record that is gone +// — deleted by a premature cleanup, or never written by a build predating +// the family table: its tokens are unusable, and a rotation neither +// succeeds nor records the family. +func TestSQLMissingFamilyRecordFailsClosed(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + ctx := t.Context() + pair := testPair("lost", "fam-lost", time.Hour) + require.NoError(t, store.SaveTokenPair(ctx, pair)) + + _, err := db.ExecContext(ctx, "DELETE FROM oauth2_token_families WHERE family_id = ?", "fam-lost") + require.NoError(t, err) + + requireUnusable(t, store, pair) + + next := testPair("lost-next", "fam-lost", time.Hour) + require.ErrorIs(t, store.RotateRefreshToken(ctx, pair.Refresh.TokenHash, next), oauth2.ErrTokenFamilyRevoked) + requirePersisted(t, db, next, 0) + assert.Zero(t, count(t, db, + "SELECT COUNT(*) FROM oauth2_token_families WHERE family_id = ?", "fam-lost"), + "a rotation never records a family") + + rt, err := store.LookupRefreshToken(ctx, pair.Refresh.TokenHash) + require.NoError(t, err) + assert.True(t, rt.Consumed, "the presented token was not consumed, it reads consumed for its lost family") +} + +// TestSQLRefreshTokenWithoutFamilyIsConsumed covers a refresh token stored +// without a family — only a build predating #82 could write one: it can no +// longer be exchanged, while a token outside any family by design (a +// client_credentials access token) stays usable. +func TestSQLRefreshTokenWithoutFamilyIsConsumed(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + ctx := t.Context() + exp := time.Now().Add(time.Hour).Unix() + + _, err := db.ExecContext(ctx, `INSERT INTO oauth2_refresh_tokens + (token_hash, client_id, subject, scope, family_id, consumed, issued_at, expires_at) + VALUES ('rt-legacy', 'c', 's', 'r', '', 0, 0, ?)`, exp) + require.NoError(t, err) + + rt, err := store.LookupRefreshToken(ctx, "rt-legacy") + require.NoError(t, err) + assert.True(t, rt.Consumed) + + cc := &oauth2.TokenPair{Access: oauth2.AccessToken{ + TokenHash: "at-cc", ClientID: "c", Subject: "c", ExpiresAt: time.Now().Add(time.Hour), + }} + require.NoError(t, store.SaveTokenPair(ctx, cc)) + + got, err := store.LookupAccessToken(ctx, "at-cc") + require.NoError(t, err) + assert.Equal(t, "c", got.ClientID) +} + +// TestSQLStoreHonorsContext checks that a canceled context reaches the +// driver: the store does not swallow it into a success. +func TestSQLStoreHonorsContext(t *testing.T) { + t.Parallel() + + store, _ := openSQLite(t) + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + require.ErrorIs(t, store.RevokeRefreshFamily(ctx, "fam-ctx"), context.Canceled) + require.ErrorIs(t, store.SaveTokenPair(ctx, testPair("ctx", "fam-ctx", time.Hour)), context.Canceled) +} + +// TestSQLRotationReportsTokenReadFailure fails the locking read of the old +// token: a backend failure, reported as such and not as an OAuth2 refusal. +func TestSQLRotationReportsTokenReadFailure(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + first := testPair("read-0", "fam-read", time.Hour) + require.NoError(t, store.SaveTokenPair(t.Context(), first)) + + _, err := db.ExecContext(t.Context(), "DROP TABLE oauth2_refresh_tokens") + require.NoError(t, err) + + err = store.RotateRefreshToken(t.Context(), first.Refresh.TokenHash, testPair("read-1", "fam-read", time.Hour)) + require.ErrorContains(t, err, "lock refresh token") + assert.Empty(t, oauth2.IsCode(err)) +} + +// TestSQLFamilyLockWithoutRecordFailsClosed covers the family record that +// is still missing after the insert-if-absent, the race a concurrent +// cleanup could cause on PostgreSQL: a trigger turns the insert into a +// silent no-op. The locking read then finds no row, which reads as a +// revoked family: neither an issuance nor a rotation goes through, and +// nothing is persisted. +func TestSQLFamilyLockWithoutRecordFailsClosed(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + ctx := t.Context() + + first := testPair("norow-0", "fam-norow", time.Hour) + require.NoError(t, store.SaveTokenPair(ctx, first)) + + _, err := db.ExecContext(ctx, "DELETE FROM oauth2_token_families WHERE family_id = ?", "fam-norow") + require.NoError(t, err) + + _, err = db.ExecContext(ctx, + "CREATE TRIGGER skip BEFORE INSERT ON oauth2_token_families BEGIN SELECT RAISE(IGNORE); END") + require.NoError(t, err) + + fresh := testPair("norow-fresh", "fam-norow-fresh", time.Hour) + require.ErrorIs(t, store.SaveTokenPair(ctx, fresh), oauth2.ErrTokenFamilyRevoked) + requirePersisted(t, db, fresh, 0) + + next := testPair("norow-1", "fam-norow", time.Hour) + require.ErrorIs(t, store.RotateRefreshToken(ctx, first.Refresh.TokenHash, next), oauth2.ErrTokenFamilyRevoked) + requirePersisted(t, db, next, 0) + + consumed := count(t, db, + "SELECT COUNT(*) FROM oauth2_refresh_tokens WHERE token_hash = ? AND consumed = 0", first.Refresh.TokenHash) + assert.Equal(t, 1, consumed, "the refused rotation consumed nothing") +} diff --git a/oauth2/store/sql/store_test.go b/oauth2/store/sql/store_test.go index a19436c..343839d 100644 --- a/oauth2/store/sql/store_test.go +++ b/oauth2/store/sql/store_test.go @@ -16,19 +16,19 @@ import ( _ "modernc.org/sqlite" // pure-Go SQLite driver — no cgo, no Docker ) -// newSQLiteStore opens a fresh in-memory SQLite database, migrates it, and -// returns a ready oauth2.Storage. Each call gets an isolated database -// (the "file::memory:" + unique cache prevents sharing across calls). -func newSQLiteStore(t *testing.T) oauth2.Storage { +// openSQLite opens a private in-memory SQLite database, migrates it, and +// returns the store over it along with the raw handle, which tests use to +// inspect the tables and inject failures with triggers. +// +// A plain ":memory:" database lives in its connection, so the pool is +// capped at one connection: every call gets a database of its own, which +// survives as long as the handle, and the store's transactions serialize +// on it (SQLite is single-writer anyway). +func openSQLite(t *testing.T) (*sqlstore.Store, *sql.DB) { t.Helper() - // A private in-memory database, scoped to this *sql.DB handle. - db, err := sql.Open("sqlite", "file::memory:?cache=shared&_pragma=foreign_keys(1)") + db, err := sql.Open("sqlite", ":memory:") require.NoError(t, err) - - // SQLite in-memory shared-cache stays alive while at least one - // connection is open; cap the pool at 1 so the schema persists and - // writes serialise (SQLite is single-writer anyway). db.SetMaxOpenConns(1) t.Cleanup(func() { _ = db.Close() }) @@ -38,6 +38,15 @@ func newSQLiteStore(t *testing.T) oauth2.Storage { require.NoError(t, store.Migrate(context.Background())) + return store, db +} + +// newSQLiteStore returns a ready oauth2.Storage over a private database. +func newSQLiteStore(t *testing.T) oauth2.Storage { + t.Helper() + + store, _ := openSQLite(t) + return store } @@ -57,16 +66,8 @@ func TestSQLiteStoreConformance(t *testing.T) { func TestMigrateIsIdempotent(t *testing.T) { t.Parallel() - db, err := sql.Open("sqlite", "file::memory:?cache=shared") - require.NoError(t, err) - db.SetMaxOpenConns(1) - - t.Cleanup(func() { _ = db.Close() }) + store, _ := openSQLite(t) - store, err := sqlstore.New(db, sqlstore.SQLite) - require.NoError(t, err) - - require.NoError(t, store.Migrate(context.Background())) require.NoError(t, store.Migrate(context.Background()), "second Migrate must be a no-op") } diff --git a/oauth2/storetest/conformance.go b/oauth2/storetest/conformance.go index 1393d29..8e47d3c 100644 --- a/oauth2/storetest/conformance.go +++ b/oauth2/storetest/conformance.go @@ -33,6 +33,13 @@ type Factory func() oauth2.Storage // func TestMyStoreConformance(t *testing.T) { // storetest.RunConformance(t, func() oauth2.Storage { return New(...) }) // } +// +// Besides single-use codes and atomic rotation, it checks the token-family +// invariants the [oauth2.Storage] godoc states — visibility, gating, atomic +// writes, reuse, idempotence, isolation — sequentially and under races, +// through the public API only. What it cannot observe stays with each +// store's own tests: an injected backend failure (I6, and a reuse whose +// revocation fails), and family state lost by the backend (I8). func RunConformance(t *testing.T, newStore Factory) { t.Helper() @@ -51,6 +58,18 @@ func RunConformance(t *testing.T, newStore Factory) { {"RefreshTokenReuseRevokesFamily", testRefreshReuse}, {"RefreshTokenConcurrentRotation", testRefreshConcurrentRotation}, {"RevokeRefreshFamily", testRevokeFamily}, + {"FamilyRevokeBeforeIssuance", testFamilyRevokeBeforeIssuance}, + {"FamilyRotateThenRevoke", testFamilyRotateThenRevoke}, + {"FamilyRevokeThenRotate", testFamilyRevokeThenRotate}, + {"FamilyRevokeIsIdempotent", testFamilyRevokeIsIdempotent}, + {"FamilyRevokeIsolation", testFamilyRevokeIsolation}, + {"FamilyRevokeRejectsEmptyID", testFamilyRevokeRejectsEmptyID}, + {"FamilyIDLengthLimit", testFamilyIDLengthLimit}, + {"FamilyFailedRotationPersistsNothing", testFamilyFailedRotationPersistsNothing}, + {"FamilySaveTokenPairRejectsInvalidPair", testFamilySaveTokenPairRejectsInvalidPair}, + {"FamilyAccessOnlyIssuanceIsGated", testFamilyAccessOnlyIssuanceIsGated}, + {"FamilyRevokeRacesRotationChain", testFamilyRevokeRacesRotationChain}, + {"FamilyRevokeRacesIssuance", testFamilyRevokeRacesIssuance}, } for _, c := range cases { @@ -71,6 +90,9 @@ const testScope = "read" func ctx() context.Context { return context.Background() } +// The assertion helpers below hold every t.Fatalf of the suite, so the +// cases themselves stay free of branches a passing store never takes. + func mustNoError(t *testing.T, err error, msg string) { t.Helper() @@ -79,6 +101,42 @@ func mustNoError(t *testing.T, err error, msg string) { } } +// mustErrorIs asserts that err matches target. +func mustErrorIs(t *testing.T, err, target error, msg string) { + t.Helper() + + if !errors.Is(err, target) { + t.Fatalf("%s: got error %v, want one matching %v", msg, err, target) + } +} + +// mustNotErrorIs asserts that err does not match target. +func mustNotErrorIs(t *testing.T, err, target error, msg string) { + t.Helper() + + if errors.Is(err, target) { + t.Fatalf("%s: got error %v, must not match %v", msg, err, target) + } +} + +// mustCode asserts that err is an error carrying the OAuth2 code. +func mustCode(t *testing.T, err error, code, msg string) { + t.Helper() + + if got := oauth2.IsCode(err); err == nil || got != code { + t.Fatalf("%s: got error %v (code %q), want an error with code %s", msg, err, got, code) + } +} + +// mustEqual asserts that got equals want. +func mustEqual[T comparable](t *testing.T, got, want T, msg string) { + t.Helper() + + if got != want { + t.Fatalf("%s: got %v, want %v", msg, got, want) + } +} + // --- authorization codes ------------------------------------------------- func sampleCode(hash string) *oauth2.AuthorizationCode { @@ -101,34 +159,24 @@ func testCodeSaveConsume(t *testing.T, s oauth2.Storage) { got, err := s.ConsumeAuthorizationCode(ctx(), "code-1") mustNoError(t, err, "consume") - - if got.ClientID != testClientID || got.Subject != testSubject || got.Scope != testScope { - t.Fatalf("consumed code lost fields: %+v", got) - } + mustEqual(t, got.ClientID, testClientID, "consumed code lost the client") + mustEqual(t, got.Subject, testSubject, "consumed code lost the subject") + mustEqual(t, got.Scope, testScope, "consumed code lost the scope") } func testCodeSingleUse(t *testing.T, s oauth2.Storage) { mustNoError(t, s.SaveAuthorizationCode(ctx(), sampleCode("code-2")), "save") - if _, err := s.ConsumeAuthorizationCode(ctx(), "code-2"); err != nil { - t.Fatalf("first consume failed: %v", err) - } - _, err := s.ConsumeAuthorizationCode(ctx(), "code-2") - if err == nil { - t.Fatal("second consume MUST fail (single-use)") - } + mustNoError(t, err, "first consume") - if !errors.Is(err, oauth2.ErrCodeAlreadyUsed) && - oauth2.IsCode(err) != oauth2.CodeInvalidGrant { - t.Fatalf("reuse error should be ErrCodeAlreadyUsed/invalid_grant, got %v", err) - } + _, err = s.ConsumeAuthorizationCode(ctx(), "code-2") + mustCode(t, err, oauth2.CodeInvalidGrant, "second consume (single-use)") } func testCodeUnknown(t *testing.T, s oauth2.Storage) { - if _, err := s.ConsumeAuthorizationCode(ctx(), "never-saved"); err == nil { - t.Fatal("consuming an unknown code MUST fail") - } + _, err := s.ConsumeAuthorizationCode(ctx(), "never-saved") + mustCode(t, err, oauth2.CodeInvalidGrant, "consume an unknown code") } // testCodeConcurrentConsume asserts the single-use guarantee under @@ -145,203 +193,158 @@ func testCodeConcurrentConsume(t *testing.T, s oauth2.Storage) { ) for range n { - wg.Add(1) - - go func() { - defer wg.Done() - + wg.Go(func() { if _, err := s.ConsumeAuthorizationCode(ctx(), "code-race"); err == nil { mu.Lock() wins++ mu.Unlock() } - }() + }) } wg.Wait() - if wins != 1 { - t.Fatalf("expected exactly 1 successful consume, got %d", wins) - } + mustEqual(t, wins, 1, "successful consumes") } // --- access tokens ------------------------------------------------------- -func sampleAccess(hash, family string) *oauth2.AccessToken { - now := time.Now() - - return &oauth2.AccessToken{ - Token: "raw-" + hash, - TokenHash: hash, - ClientID: testClientID, - Subject: testSubject, - Scope: testScope, - IssuedAt: now, - ExpiresAt: now.Add(time.Hour), - FamilyID: family, - Audience: "api", - } -} - func testAccessLifecycle(t *testing.T, s oauth2.Storage) { - mustNoError(t, s.SaveAccessToken(ctx(), sampleAccess("at-1", "")), "save") + cc := accessOnly("lifecycle", "") // a client_credentials token + mustSave(t, s, cc) - got, err := s.LookupAccessToken(ctx(), "at-1") + got, err := s.LookupAccessToken(ctx(), cc.Access.TokenHash) mustNoError(t, err, "lookup") + mustEqual(t, got.Subject, testSubject, "lookup lost the subject") + mustEqual(t, got.Audience, cc.Access.Audience, "lookup lost the audience") - if got.Subject != testSubject { - t.Fatalf("lookup lost fields: %+v", got) - } - - mustNoError(t, s.RevokeAccessToken(ctx(), "at-1"), "revoke") - - if _, err := s.LookupAccessToken(ctx(), "at-1"); err == nil { - t.Fatal("lookup after revoke MUST fail") - } + mustNoError(t, s.RevokeAccessToken(ctx(), cc.Access.TokenHash), "revoke") + requireUnusable(t, s, cc) } func testAccessUnknown(t *testing.T, s oauth2.Storage) { - if _, err := s.LookupAccessToken(ctx(), "missing"); err == nil { - t.Fatal("lookup of unknown access token MUST fail") - } + _, err := s.LookupAccessToken(ctx(), "missing") + mustCode(t, err, oauth2.CodeInvalidGrant, "lookup of an unknown access token") } // --- refresh tokens ------------------------------------------------------ -func sampleRefresh(hash, family string) *oauth2.RefreshToken { - now := time.Now() - - return &oauth2.RefreshToken{ - Token: "raw-" + hash, - TokenHash: hash, - ClientID: testClientID, - Subject: testSubject, - Scope: testScope, - IssuedAt: now, - ExpiresAt: now.Add(24 * time.Hour), - FamilyID: family, - } -} - func testRefreshSaveLookup(t *testing.T, s oauth2.Storage) { - mustNoError(t, s.SaveRefreshToken(ctx(), sampleRefresh("rt-1", "fam-1")), "save") + pair := newPair("save", "fam-save") + mustSave(t, s, pair) - got, err := s.LookupRefreshToken(ctx(), "rt-1") - mustNoError(t, err, "lookup") + requireUsable(t, s, pair) - if got.Consumed { - t.Fatal("freshly saved refresh token must not be consumed") - } + got, err := s.LookupRefreshToken(ctx(), pair.Refresh.TokenHash) + mustNoError(t, err, "lookup refresh token") + mustEqual(t, got.FamilyID, "fam-save", "lookup lost the family") + mustEqual(t, got.Scope, testScope, "lookup lost the scope") } func testRefreshRotation(t *testing.T, s oauth2.Storage) { - mustNoError(t, s.SaveRefreshToken(ctx(), sampleRefresh("rt-old", "fam-2")), "save old") - - next := sampleRefresh("rt-new", "fam-2") - mustNoError(t, s.RotateRefreshToken(ctx(), "rt-old", next), "rotate") + first := newPair("rot-0", "fam-rot") + mustSave(t, s, first) - old, err := s.LookupRefreshToken(ctx(), "rt-old") - mustNoError(t, err, "lookup old") + next := newPair("rot-1", "fam-rot") + mustRotate(t, s, first, next) - if !old.Consumed { - t.Fatal("rotated old token MUST be marked consumed") - } - - fresh, err := s.LookupRefreshToken(ctx(), "rt-new") - mustNoError(t, err, "lookup new") + requireConsumed(t, s, first.Refresh.TokenHash) + requireUsable(t, s, next) - if fresh.Consumed { - t.Fatal("new token must not be consumed") - } + // Rotation consumes the presented refresh token only: the access token + // issued alongside it lives on until it expires. + _, err := s.LookupAccessToken(ctx(), first.Access.TokenHash) + mustNoError(t, err, "lookup the previous access token") } +// testRefreshReuse replays a rotated token: the store reports reuse — with +// the family revoked, so not as a revocation failure — and nothing of the +// family stays usable or can be issued any more. func testRefreshReuse(t *testing.T, s oauth2.Storage) { - mustNoError(t, s.SaveRefreshToken(ctx(), sampleRefresh("rt-r1", "fam-3")), "save") - - next1 := sampleRefresh("rt-r2", "fam-3") - mustNoError(t, s.RotateRefreshToken(ctx(), "rt-r1", next1), "first rotate") - - // Replaying rt-r1 (already consumed) MUST fail and revoke the family. - next2 := sampleRefresh("rt-r3", "fam-3") - - err := s.RotateRefreshToken(ctx(), "rt-r1", next2) - if err == nil { - t.Fatal("rotating a consumed token MUST fail") - } - - if !errors.Is(err, oauth2.ErrRefreshTokenReused) { - t.Fatalf("expected ErrRefreshTokenReused, got %v", err) - } - - // The whole family must now be consumed. - for _, h := range []string{"rt-r1", "rt-r2"} { - rt, lookupErr := s.LookupRefreshToken(ctx(), h) - if lookupErr != nil { - continue // some backends delete revoked tokens — acceptable - } - - if !rt.Consumed { - t.Fatalf("token %s should be consumed after family revocation", h) - } - } + first := newPair("reuse-0", "fam-reuse") + mustSave(t, s, first) + + second := newPair("reuse-1", "fam-reuse") + mustRotate(t, s, first, second) + + replay := newPair("reuse-2", "fam-reuse") + err := s.RotateRefreshToken(ctx(), first.Refresh.TokenHash, replay) + mustErrorIs(t, err, oauth2.ErrRefreshTokenReused, "replay a rotated token") + mustCode(t, err, oauth2.CodeInvalidGrant, "replay a rotated token") + mustNotErrorIs(t, err, oauth2.ErrTokenFamilyRevocationFailed, "replay a rotated token") + + requireUnusable(t, s, first) + requireUnusable(t, s, second) + requireAbsent(t, s, replay) + + // Once the family is revoked, the revoked state decides before the + // consumed one: the next replay is refused as a revoked family. + again := newPair("reuse-again", "fam-reuse") + err = s.RotateRefreshToken(ctx(), first.Refresh.TokenHash, again) + mustErrorIs(t, err, oauth2.ErrTokenFamilyRevoked, "replay into the revoked family") + requireAbsent(t, s, again) + + late := newPair("reuse-3", "fam-reuse") + mustErrorIs(t, s.SaveTokenPair(ctx(), late), oauth2.ErrTokenFamilyRevoked, "issue into the revoked family") + requireAbsent(t, s, late) } // testRefreshConcurrentRotation asserts atomic rotation: 30 goroutines race -// to rotate the same token; exactly one succeeds, the rest see reuse. +// to rotate the same token and exactly one succeeds. Every loser runs after +// the winner, so the first of them detects the reuse and revokes the +// family: the winner's tokens end up unusable and no loser persists +// anything. func testRefreshConcurrentRotation(t *testing.T, s oauth2.Storage) { - mustNoError(t, s.SaveRefreshToken(ctx(), sampleRefresh("rt-c0", "fam-c")), "save") + first := newPair("race-0", "fam-race") + mustSave(t, s, first) const n = 30 - var ( - wg sync.WaitGroup - mu sync.Mutex - wins int - ) + nexts := make([]*oauth2.TokenPair, n) + errs := make([]error, n) - for i := range n { - wg.Add(1) + for i := range nexts { + nexts[i] = newPair(fmt.Sprintf("race-%d", i+1), "fam-race") + } - go func(i int) { - defer wg.Done() + var wg sync.WaitGroup - next := sampleRefresh(fmt.Sprintf("rt-c%d", i+1), "fam-c") - if err := s.RotateRefreshToken(ctx(), "rt-c0", next); err == nil { - mu.Lock() - wins++ - mu.Unlock() - } - }(i) - } + start := make(chan struct{}) - wg.Wait() + for i := range nexts { + wg.Go(func() { + <-start - if wins != 1 { - t.Fatalf("expected exactly 1 successful rotation, got %d", wins) + errs[i] = s.RotateRefreshToken(ctx(), first.Refresh.TokenHash, nexts[i]) + }) } -} -func testRevokeFamily(t *testing.T, s oauth2.Storage) { - mustNoError(t, s.SaveRefreshToken(ctx(), sampleRefresh("rt-f1", "fam-x")), "save rt1") - mustNoError(t, s.SaveRefreshToken(ctx(), sampleRefresh("rt-f2", "fam-x")), "save rt2") - mustNoError(t, s.SaveAccessToken(ctx(), sampleAccess("at-f1", "fam-x")), "save at") + close(start) + wg.Wait() - mustNoError(t, s.RevokeRefreshFamily(ctx(), "fam-x"), "revoke family") + wins := 0 - // Access tokens of the family must be gone. - if _, err := s.LookupAccessToken(ctx(), "at-f1"); err == nil { - t.Fatal("access token of revoked family MUST be gone") - } + for i, err := range errs { + if err == nil { + wins++ + + requireUnusable(t, s, nexts[i]) - // Refresh tokens of the family must be consumed (or gone). - for _, h := range []string{"rt-f1", "rt-f2"} { - rt, err := s.LookupRefreshToken(ctx(), h) - if err != nil { continue } - if !rt.Consumed { - t.Fatalf("refresh token %s should be consumed after family revocation", h) - } + mustCode(t, err, oauth2.CodeInvalidGrant, "losing rotation") + requireAbsent(t, s, nexts[i]) } + + mustEqual(t, wins, 1, "successful rotations") +} + +func testRevokeFamily(t *testing.T, s oauth2.Storage) { + pair := newPair("revoke", "fam-revoke") + mustSave(t, s, pair) + + mustNoError(t, s.RevokeRefreshFamily(ctx(), "fam-revoke"), "revoke family") + + requireUnusable(t, s, pair) } diff --git a/oauth2/storetest/family.go b/oauth2/storetest/family.go new file mode 100644 index 0000000..3d2a232 --- /dev/null +++ b/oauth2/storetest/family.go @@ -0,0 +1,455 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package storetest + +import ( + "fmt" + "strings" + "sync" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" +) + +// The token-family cases of the suite: they check the invariants the +// [oauth2.Storage] godoc states, through the public API only. + +// newPair returns a fresh pair in family: access token "at-" and +// refresh token "rt-", both valid for a while. +func newPair(name, family string) *oauth2.TokenPair { + now := time.Now() + + return &oauth2.TokenPair{ + Access: oauth2.AccessToken{ + Token: "raw-at-" + name, + TokenHash: "at-" + name, + ClientID: testClientID, + Subject: testSubject, + Scope: testScope, + IssuedAt: now, + ExpiresAt: now.Add(time.Hour), + FamilyID: family, + Audience: "api", + }, + Refresh: &oauth2.RefreshToken{ + Token: "raw-rt-" + name, + TokenHash: "rt-" + name, + ClientID: testClientID, + Subject: testSubject, + Scope: testScope, + IssuedAt: now, + ExpiresAt: now.Add(24 * time.Hour), + FamilyID: family, + }, + } +} + +// accessOnly returns a pair carrying an access token alone: a +// client_credentials token when family is empty, a non-rotating Profile20 +// refresh otherwise. +func accessOnly(name, family string) *oauth2.TokenPair { + pair := newPair(name, family) + pair.Refresh = nil + + return pair +} + +func mustSave(t *testing.T, s oauth2.Storage, pair *oauth2.TokenPair) { + t.Helper() + + mustNoError(t, s.SaveTokenPair(ctx(), pair), "save pair "+pair.Access.TokenHash) +} + +func mustRotate(t *testing.T, s oauth2.Storage, from, next *oauth2.TokenPair) { + t.Helper() + + mustNoError(t, s.RotateRefreshToken(ctx(), from.Refresh.TokenHash, next), "rotate "+from.Refresh.TokenHash) +} + +// requireUsable asserts that pair can still be used: its access token is +// found, and its refresh token, if any, is found and not consumed. +func requireUsable(t *testing.T, s oauth2.Storage, pair *oauth2.TokenPair) { + t.Helper() + + _, err := s.LookupAccessToken(ctx(), pair.Access.TokenHash) + mustNoError(t, err, "lookup access token "+pair.Access.TokenHash) + + if pair.Refresh == nil { + return + } + + rt, err := s.LookupRefreshToken(ctx(), pair.Refresh.TokenHash) + mustNoError(t, err, "lookup refresh token "+pair.Refresh.TokenHash) + mustEqual(t, rt.Consumed, false, "refresh token "+pair.Refresh.TokenHash+" consumed") +} + +// requireUnusable asserts that no token of pair can be used any more: its +// access token lookup fails with invalid_grant, and its refresh token, if +// any, is gone or consumed. +func requireUnusable(t *testing.T, s oauth2.Storage, pair *oauth2.TokenPair) { + t.Helper() + + _, err := s.LookupAccessToken(ctx(), pair.Access.TokenHash) + mustCode(t, err, oauth2.CodeInvalidGrant, "lookup access token "+pair.Access.TokenHash) + + if pair.Refresh == nil { + return + } + + rt, err := s.LookupRefreshToken(ctx(), pair.Refresh.TokenHash) + if err != nil { + mustCode(t, err, oauth2.CodeInvalidGrant, "lookup refresh token "+pair.Refresh.TokenHash) + + return + } + + mustEqual(t, rt.Consumed, true, "refresh token "+pair.Refresh.TokenHash+" consumed") +} + +// requireAbsent asserts that nothing of pair was persisted. A refresh +// token persisted into a revoked family would come back consumed, so its +// lookup must fail. +func requireAbsent(t *testing.T, s oauth2.Storage, pair *oauth2.TokenPair) { + t.Helper() + + _, err := s.LookupAccessToken(ctx(), pair.Access.TokenHash) + mustCode(t, err, oauth2.CodeInvalidGrant, "lookup access token "+pair.Access.TokenHash) + + if pair.Refresh == nil { + return + } + + _, err = s.LookupRefreshToken(ctx(), pair.Refresh.TokenHash) + mustCode(t, err, oauth2.CodeInvalidGrant, "lookup refresh token "+pair.Refresh.TokenHash) +} + +// requireConsumed asserts that the refresh token hash is found, consumed. +func requireConsumed(t *testing.T, s oauth2.Storage, hash string) { + t.Helper() + + rt, err := s.LookupRefreshToken(ctx(), hash) + mustNoError(t, err, "lookup refresh token "+hash) + mustEqual(t, rt.Consumed, true, "refresh token "+hash+" consumed") +} + +// testFamilyRevokeBeforeIssuance revokes a family no token belongs to yet: +// the revocation is recorded all the same and refuses the first issuance. +func testFamilyRevokeBeforeIssuance(t *testing.T, s oauth2.Storage) { + mustNoError(t, s.RevokeRefreshFamily(ctx(), "fam-early"), "revoke an unknown family") + + pair := newPair("early", "fam-early") + err := s.SaveTokenPair(ctx(), pair) + mustErrorIs(t, err, oauth2.ErrTokenFamilyRevoked, "issue into a family revoked beforehand") + mustCode(t, err, oauth2.CodeInvalidGrant, "issue into a family revoked beforehand") + requireAbsent(t, s, pair) +} + +// testFamilyRotateThenRevoke revokes a family after two rotations: every +// ancestor and descendant becomes unusable. +func testFamilyRotateThenRevoke(t *testing.T, s oauth2.Storage) { + chain := []*oauth2.TokenPair{ + newPair("chain-0", "fam-chain"), + newPair("chain-1", "fam-chain"), + newPair("chain-2", "fam-chain"), + } + + mustSave(t, s, chain[0]) + mustRotate(t, s, chain[0], chain[1]) + mustRotate(t, s, chain[1], chain[2]) + + mustNoError(t, s.RevokeRefreshFamily(ctx(), "fam-chain"), "revoke family") + + for _, pair := range chain { + requireUnusable(t, s, pair) + } +} + +// testFamilyRevokeThenRotate rotates a token of a revoked family: the +// rotation fails and persists nothing. +func testFamilyRevokeThenRotate(t *testing.T, s oauth2.Storage) { + first := newPair("late-0", "fam-late") + mustSave(t, s, first) + + mustNoError(t, s.RevokeRefreshFamily(ctx(), "fam-late"), "revoke family") + + next := newPair("late-1", "fam-late") + err := s.RotateRefreshToken(ctx(), first.Refresh.TokenHash, next) + mustCode(t, err, oauth2.CodeInvalidGrant, "rotate in a revoked family") + requireAbsent(t, s, next) + requireUnusable(t, s, first) +} + +// testFamilyRevokeIsIdempotent revokes a family twice: both calls succeed +// and the family stays revoked. +func testFamilyRevokeIsIdempotent(t *testing.T, s oauth2.Storage) { + pair := newPair("twice", "fam-twice") + mustSave(t, s, pair) + + mustNoError(t, s.RevokeRefreshFamily(ctx(), "fam-twice"), "first revocation") + mustNoError(t, s.RevokeRefreshFamily(ctx(), "fam-twice"), "second revocation") + + requireUnusable(t, s, pair) + + late := newPair("twice-late", "fam-twice") + mustErrorIs(t, s.SaveTokenPair(ctx(), late), oauth2.ErrTokenFamilyRevoked, "issue into the revoked family") + requireAbsent(t, s, late) +} + +// testFamilyRevokeIsolation revokes one family: another family and a token +// outside any family stay usable. +func testFamilyRevokeIsolation(t *testing.T, s oauth2.Storage) { + revoked := newPair("iso-a", "fam-iso-a") + other := newPair("iso-b", "fam-iso-b") + cc := accessOnly("iso-cc", "") + + mustSave(t, s, revoked) + mustSave(t, s, other) + mustSave(t, s, cc) + + mustNoError(t, s.RevokeRefreshFamily(ctx(), "fam-iso-a"), "revoke family") + + requireUnusable(t, s, revoked) + requireUsable(t, s, other) + requireUsable(t, s, cc) +} + +// testFamilyRevokeRejectsEmptyID refuses to revoke the empty family ID, +// which would otherwise reach every token outside a family. +func testFamilyRevokeRejectsEmptyID(t *testing.T, s oauth2.Storage) { + cc := accessOnly("empty-id", "") + mustSave(t, s, cc) + + mustCode(t, s.RevokeRefreshFamily(ctx(), ""), oauth2.CodeInvalidRequest, "revoke the empty family id") + requireUsable(t, s, cc) +} + +// testFamilyIDLengthLimit accepts a family ID of exactly +// oauth2.MaxFamilyIDLength bytes everywhere and refuses a longer one with +// invalid_request everywhere, so no backend can truncate two IDs into one. +func testFamilyIDLengthLimit(t *testing.T, s oauth2.Storage) { + longest := strings.Repeat("f", oauth2.MaxFamilyIDLength) + tooLong := longest + "f" + + first := newPair("len-0", longest) + mustSave(t, s, first) + + next := newPair("len-1", longest) + mustRotate(t, s, first, next) + requireUsable(t, s, next) + + over := newPair("len-over", tooLong) + mustCode(t, s.SaveTokenPair(ctx(), over), oauth2.CodeInvalidRequest, "issue with an over-long family id") + requireAbsent(t, s, over) + + overNext := newPair("len-over-next", tooLong) + err := s.RotateRefreshToken(ctx(), next.Refresh.TokenHash, overNext) + mustCode(t, err, oauth2.CodeInvalidRequest, "rotate into an over-long family id") + requireAbsent(t, s, overNext) + requireUsable(t, s, next) + + mustCode(t, s.RevokeRefreshFamily(ctx(), tooLong), oauth2.CodeInvalidRequest, "revoke an over-long family id") + mustNoError(t, s.RevokeRefreshFamily(ctx(), longest), "revoke the longest family id") + requireUnusable(t, s, next) +} + +// testFamilyFailedRotationPersistsNothing makes rotations fail for every +// reason the contract lists and checks that none of them persisted its +// target or consumed the old token. The targets declare an existing active +// family wherever the call allows it, so a wrongly persisted token would +// be found. +func testFamilyFailedRotationPersistsNothing(t *testing.T, s oauth2.Storage) { + mustSave(t, s, newPair("fail-active", "fam-fail-active")) + + first := newPair("fail-0", "fam-fail") + mustSave(t, s, first) + + unknown := newPair("fail-unknown", "fam-fail-active") + err := s.RotateRefreshToken(ctx(), "never-issued", unknown) + mustCode(t, err, oauth2.CodeInvalidGrant, "rotate an unknown token") + requireAbsent(t, s, unknown) + + stray := newPair("fail-stray", "fam-fail-active") + err = s.RotateRefreshToken(ctx(), first.Refresh.TokenHash, stray) + mustCode(t, err, oauth2.CodeInvalidRequest, "rotate into another family") + requireAbsent(t, s, stray) + + bare := accessOnly("fail-bare", "fam-fail") + err = s.RotateRefreshToken(ctx(), first.Refresh.TokenHash, bare) + mustCode(t, err, oauth2.CodeInvalidRequest, "rotate without a refresh token") + requireAbsent(t, s, bare) + + err = s.RotateRefreshToken(ctx(), first.Refresh.TokenHash, nil) + mustCode(t, err, oauth2.CodeInvalidRequest, "rotate into nothing") + + // None of these consumed the old token: it still rotates. + requireUsable(t, s, first) + + second := newPair("fail-1", "fam-fail") + mustRotate(t, s, first, second) + + mustNoError(t, s.RevokeRefreshFamily(ctx(), "fam-fail"), "revoke family") + + third := newPair("fail-2", "fam-fail") + err = s.RotateRefreshToken(ctx(), second.Refresh.TokenHash, third) + mustErrorIs(t, err, oauth2.ErrTokenFamilyRevoked, "rotate in a revoked family") + requireAbsent(t, s, third) +} + +// testFamilySaveTokenPairRejectsInvalidPair refuses every pair +// oauth2.TokenPair.Validate rejects, persisting nothing of it. +func testFamilySaveTokenPairRejectsInvalidPair(t *testing.T, s oauth2.Storage) { + mustCode(t, s.SaveTokenPair(ctx(), nil), oauth2.CodeInvalidRequest, "save a nil pair") + + split := newPair("invalid-split", "fam-invalid-a") + split.Refresh.FamilyID = "fam-invalid-b" + + orphan := newPair("invalid-orphan", "") + + consumed := newPair("invalid-consumed", "fam-invalid-c") + consumed.Refresh.Consumed = true + + for _, pair := range []*oauth2.TokenPair{split, orphan, consumed} { + mustCode(t, s.SaveTokenPair(ctx(), pair), oauth2.CodeInvalidRequest, "save invalid pair "+pair.Access.TokenHash) + requireAbsent(t, s, pair) + } +} + +// testFamilyAccessOnlyIssuanceIsGated covers the non-rotating Profile20 +// refresh, which issues an access token alone into the family: accepted +// while the family is active, refused once it is revoked. +func testFamilyAccessOnlyIssuanceIsGated(t *testing.T, s oauth2.Storage) { + pair := newPair("static", "fam-static") + mustSave(t, s, pair) + + refreshed := accessOnly("static-1", "fam-static") + mustSave(t, s, refreshed) + requireUsable(t, s, refreshed) + requireUsable(t, s, pair) + + mustNoError(t, s.RevokeRefreshFamily(ctx(), "fam-static"), "revoke family") + + late := accessOnly("static-2", "fam-static") + mustErrorIs(t, s.SaveTokenPair(ctx(), late), oauth2.ErrTokenFamilyRevoked, "issue into the revoked family") + requireAbsent(t, s, late) + requireUnusable(t, s, refreshed) + requireUnusable(t, s, pair) +} + +// testFamilyRevokeRacesRotationChain rotates a chain of tokens while the +// family is revoked concurrently. Whatever the schedule, once the +// revocation returned no pair of the chain is usable, the rotation that +// lost the race persisted nothing, and a further rotation fails. +func testFamilyRevokeRacesRotationChain(t *testing.T, s oauth2.Storage) { + const n = 20 + + chain := make([]*oauth2.TokenPair, n+1) + for i := range chain { + chain[i] = newPair(fmt.Sprintf("racing-%d", i), "fam-racing") + } + + mustSave(t, s, chain[0]) + + var ( + wg sync.WaitGroup + rotated int + rotateErr error + revokeErr error + ) + + start := make(chan struct{}) + + wg.Go(func() { + <-start + + for rotated < n { + rotateErr = s.RotateRefreshToken(ctx(), chain[rotated].Refresh.TokenHash, chain[rotated+1]) + if rotateErr != nil { + return + } + + rotated++ + } + }) + wg.Go(func() { + <-start + + revokeErr = s.RevokeRefreshFamily(ctx(), "fam-racing") + }) + + close(start) + wg.Wait() + + mustNoError(t, revokeErr, "revoke family") + + if rotated < n { + mustCode(t, rotateErr, oauth2.CodeInvalidGrant, "rotation that lost the race") + } + + for _, pair := range chain[:rotated+1] { + requireUnusable(t, s, pair) + } + + for _, pair := range chain[rotated+1:] { + requireAbsent(t, s, pair) + } + + extra := newPair("racing-extra", "fam-racing") + err := s.RotateRefreshToken(ctx(), chain[rotated].Refresh.TokenHash, extra) + mustCode(t, err, oauth2.CodeInvalidGrant, "rotate after the revocation") + requireAbsent(t, s, extra) +} + +// testFamilyRevokeRacesIssuance issues pairs into a family while it is +// revoked concurrently: once the revocation returned, an issued pair is +// unusable, and a refused one reports ErrTokenFamilyRevoked and persisted +// nothing. +func testFamilyRevokeRacesIssuance(t *testing.T, s oauth2.Storage) { + const n = 20 + + pairs := make([]*oauth2.TokenPair, n) + errs := make([]error, n) + + for i := range pairs { + pairs[i] = newPair(fmt.Sprintf("issuing-%d", i), "fam-issuing") + } + + var ( + wg sync.WaitGroup + revokeErr error + ) + + start := make(chan struct{}) + + for i := range pairs { + wg.Go(func() { + <-start + + errs[i] = s.SaveTokenPair(ctx(), pairs[i]) + }) + } + + wg.Go(func() { + <-start + + revokeErr = s.RevokeRefreshFamily(ctx(), "fam-issuing") + }) + + close(start) + wg.Wait() + + mustNoError(t, revokeErr, "revoke family") + + for i, pair := range pairs { + if errs[i] == nil { + requireUnusable(t, s, pair) + + continue + } + + mustErrorIs(t, errs[i], oauth2.ErrTokenFamilyRevoked, "issuance that lost the race") + requireAbsent(t, s, pair) + } +} diff --git a/oauth2/token/generator.go b/oauth2/token/generator.go index df576e1..337c7a4 100644 --- a/oauth2/token/generator.go +++ b/oauth2/token/generator.go @@ -32,7 +32,8 @@ type AccessTokenClaims struct { ClientID string // Scope is the granted scope. Scope string - // FamilyID is the rotation family identifier (refresh-token family). + // FamilyID is the rotation family identifier (refresh-token family). At + // most [oauth2.MaxFamilyIDLength] bytes. FamilyID string // IssuedAt is the issuance time. IssuedAt time.Time diff --git a/oauth2/token/jwt.go b/oauth2/token/jwt.go index 9e776ae..62caa58 100644 --- a/oauth2/token/jwt.go +++ b/oauth2/token/jwt.go @@ -30,6 +30,14 @@ type AccessTokenSigner interface { // introspection locate the AccessToken record without persisting the raw // JWT (the JWS is large; storing only the hash keeps the table compact and // removes the leak window). Every lookup path hashes the same way. +// +// Revoking such a token — or its family — takes effect in the store only: +// a resource server verifying the JWT offline, from its signature and +// claims, keeps accepting it until it expires, and the RFC 9068 payload +// carries no family or token identifier to deny-list. Keep the access-token +// lifetime short, or validate high-value requests against the store +// ([oauth2.AccessTokenStore.LookupAccessToken]) or through RFC 7662 +// introspection. type JWTAccessTokenGenerator struct { signer AccessTokenSigner } diff --git a/oauth2/token_endpoint.go b/oauth2/token_endpoint.go index 7b87041..4805902 100644 --- a/oauth2/token_endpoint.go +++ b/oauth2/token_endpoint.go @@ -163,7 +163,13 @@ func (s *Server) writeOAuthError(ctx context.Context, w http.ResponseWriter, err } s.notifyError(ctx, oe) + writeErrorResponse(w, oe) +} +// writeErrorResponse writes oe as the RFC 6749 §5.2 JSON body, with the +// status code it maps to. It does not notify the [ErrorHook]: callers that +// already reported the failure behind oe use it directly. +func writeErrorResponse(w http.ResponseWriter, oe *Error) { body := errorResponse{ Error: oe.Code, ErrorDescription: oe.Description,