diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dad9d01..51eb34e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -102,6 +102,11 @@ legacy packages (`authentication/`, `authorization/`, the in-tree consumed); `oauth2.MaxFamilyIDLength` (64 bytes) and `oauth2.FamilyRevocationRetention` (24h) bound family identifiers and how long a revoked family is remembered. +- **OAuth2 code replay error**: `oauth2.CodeReplayError`, the error an + `AuthorizationCodeStore` returns for a replayed code it still retains. It + matches `oauth2.ErrCodeAlreadyUsed` (`invalid_grant`) for `errors.Is`, + `oauth2.IsCode` and the wire, and carries the retained record for + `errors.As`. ### Changed @@ -140,6 +145,18 @@ legacy packages (`authentication/`, `authorization/`, the in-tree presenting such a refresh token answers `invalid_grant`; `client_credentials` and implicit tokens keep working. See [MIGRATION.md](MIGRATION.md). +- **OAuth2 authorization-code storage (breaking).** + `ConsumeAuthorizationCode` no longer deletes the code it consumes: every + store keeps the record, flagged consumed and without the raw code, at + least until the code expires, and answers a replay with an + `*oauth2.CodeReplayError` carrying it; an unknown code still gets + `ErrCodeAlreadyUsed`. `store/sql` adds a `consumed` column to + `oauth2_auth_codes` — existing tables need an `ALTER TABLE` — and keeps + consumed rows until the operator's cleanup; `store/redis` flags the + code's key and keeps its TTL, and a consumption resent by go-redis after + a lost reply no longer fails the exchange; the memory store no longer + keeps the raw code. Every instance must be upgraded at once. See + [MIGRATION.md](MIGRATION.md). ### Fixed @@ -276,3 +293,22 @@ legacy packages (`authentication/`, `authorization/`, the in-tree codes; their `error_description` no longer contains a double quote, which RFC 6749 §4.1.2.1 and §5.2 forbid, and no longer echoes an unsupported `code_challenge_method`. +- A replayed OAuth2 authorization code now revokes every token issued from + it — access and refresh tokens, and every later rotation — in every store + (RFC 6749 §4.1.2, §10.5; RFC 9700 §4.2.4). The `authorization_code` grant + derives the token family from the code before consuming it, so a replay + racing the first exchange cannot leave its tokens active: one landing + before that exchange persists its tokens makes it fail with + `invalid_grant` — as long as the store keeps the revoked family record + (see the deployment requirements in + [docs/security-considerations.md](docs/security-considerations.md)) — and + one landing after revokes them. Only a replay that would itself have been + accepted revokes: the code has not expired, and the client it was issued + to presents it with the same `redirect_uri` and PKCE verifier and may + still use `authorization_code` (OAuth 2.1 §4.1.3, §7.5.3). A public + `client_id` is no secret, so unknown codes, other clients and missing or + wrong verifiers revoke nothing. The replay is still refused with + `invalid_grant`, and a failed revocation is reported through + `grant.Config.OnError`. JWT access tokens validated offline stay valid + until they expire. Previously the tokens issued from a replayed code + stayed valid. diff --git a/LIMITATIONS.md b/LIMITATIONS.md index d2601d54..75cf81a7 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -24,8 +24,13 @@ than future refactor phases. 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). + rows — consumed and abandoned authorization codes included — stay until + an operator deletes those past `expires_at` (see the package + documentation for a safe order). +- **In-memory store growth** — `oauth2/storage/memory` never purges + anything: consumed and expired codes, expired tokens and revoked + families stay for the process lifetime. It is meant for development and + tests only. - **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 diff --git a/MIGRATION.md b/MIGRATION.md index c0ba4c33..76ccf991 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -227,6 +227,54 @@ To migrate: implicit) keep working until they expire. Flushing the old token rows or keys is good hygiene: they can no longer be used. +### `oauth2`: consumed authorization codes are retained (#80) + +`ConsumeAuthorizationCode` deleted a code as it consumed it, so a replay +looked exactly like an unknown code and nothing was left to tie the code +to the tokens its first exchange issued — while RFC 6749 §4.1.2 says those +tokens SHOULD be revoked. Stores now keep a consumed code — flagged +consumed, without the raw code — at least until it expires, and answer a +replay with an `*oauth2.CodeReplayError` carrying the retained record. + +| Before | After | +| ----------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------- | +| `ConsumeAuthorizationCode` deletes the code | flags it consumed and keeps it at least until `ExpiresAt`: Redis until the key expires, SQL until your cleanup, memory for the process lifetime | +| a replay returns `ErrCodeAlreadyUsed`, like an unknown code | a replay returns an `*oauth2.CodeReplayError` (it matches `ErrCodeAlreadyUsed`) carrying the record | +| the memory store keeps the raw `Code` | no store keeps it | + +To migrate: + +1. **Custom stores.** Keep a consumed code, without its raw value, at + least until its `ExpiresAt`. Return the record, with a `nil` error, to + one caller only; return `nil` and `&oauth2.CodeReplayError{Record: rec}` + to every later or concurrent caller, and `oauth2.ErrCodeAlreadyUsed` for + an unknown code. Pass `storetest.RunConformance`. +2. **Code calling `ConsumeAuthorizationCode`** — decorators, custom + grants — tests `err` first and never issues tokens from a record that + comes with an error. A wrapper must propagate `err` unchanged or + wrapped with `%w`, which keeps the replay record reachable through + `errors.As`: normalizing it to `ErrCodeAlreadyUsed` silently disables + the revocation a replay triggers. +3. **SQL.** `oauth2_auth_codes` gains a `consumed` column, which `Migrate` + does not add to an existing table (it only creates missing tables). + Until it exists, every code exchange fails with `server_error`. Run, on + PostgreSQL, MySQL or SQLite: + + ```sql + ALTER TABLE oauth2_auth_codes ADD COLUMN consumed SMALLINT NOT NULL DEFAULT 0; + ``` + + or drop the table and run `Migrate` again: a code only lives + `AuthorizeConfig.CodeTTL` (10 minutes by default). Consumed codes now + stay until your cleanup deletes the rows past `expires_at` (see the + `store/sql` package documentation); index `expires_at` through your + migration tool. +4. **Redis and memory** need nothing: a code payload without the new + `consumed` flag is an unconsumed code. +5. **Upgrade every instance at once.** An instance still running an + earlier build ignores the flag and would accept a code an upgraded + instance already consumed — a second use of the code. + ### `oauth2`: `/revoke` answers 503 when a revocation may have failed (#82) `/revoke` answered `200 OK` whatever happened, telling the client its token diff --git a/docs/architecture.md b/docs/architecture.md index 6205d471..fcc44820 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -158,15 +158,17 @@ The `oauth2` module is an authorization server, not just a provider: prefix used in the metadata document is configurable via `ServerConfig.RoutePrefix`. - **`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. + single-use `ConsumeAuthorizationCode` — consumed codes are retained until + they expire, so that a replayed code revokes the tokens issued from it — + 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 9fd8ed5e..1c89d01b 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -79,15 +79,18 @@ The hook fires for: `grant.Config.OnError` is the same hook for the errors a grant swallows 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. +presented token consumed or the store reported the reuse while rotating, +or after an authorization-code replay (RFC 6749 §4.1.2). These revocations +run on a context detached from the request, and the `refresh_token` grant +retries one the store could not record. The hook gets one `server_error` +per revocation that still failed — `revoke refresh family failed after +reuse detection`, or `revoke token family failed after authorization code +replay` — whose cause matches `oauth2.ErrTokenFamilyRevocationFailed` and +names the family with a conforming store. It does not match +`oauth2.ErrRefreshTokenReused` or `oauth2.ErrCodeAlreadyUsed`: the reuse or +replay 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 a20c5c6e..0cd2f79a 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -88,6 +88,36 @@ configurable clock skew, and resolves keys by `kid` against a JWKS provider 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. +- **Authorization-code replay** — exchanging a consumed code is refused with + `invalid_grant`, and the tokens its first exchange issued are revoked: + access and refresh tokens, and every later rotation (RFC 6749 §4.1.2, + §10.5; RFC 9700 §4.2.4). Only a replay that would itself have been + accepted revokes: the code has not expired, and the client it was issued + to presents it with the same `redirect_uri` and PKCE verifier and may + still use `authorization_code` (OAuth 2.1 §4.1.3, §7.5.3). Anything else — + another client, a missing or wrong verifier, an unknown code — is refused + and revokes nothing: a public `client_id` is no secret, and revoking on + any replay would let whoever holds a leaked code cut the client off. The + grant derives the code's token family from the code before consuming it, + so the link takes no storage and a replay racing the first exchange cannot + leave its tokens active: one landing before that exchange persists its + tokens makes it answer `invalid_grant` and issue nothing; one landing + after revokes them. The first outcome holds while the store keeps the + family's revoked record (`oauth2.FamilyRevocationRetention` at least), + i.e. under the [deployment + requirements](#token-family-revocation-storage-guarantees): an evicted + Redis family key, or a SQL family row deleted before its `expires_at`, + lets the in-flight exchange recreate the family active. Stores keep a + consumed code, hashed and flagged, at least until it expires — Redis + expires the key with the code, SQL rows stay until your cleanup, the + memory store keeps them for good — so a replay is recognized for the + code's lifetime (`AuthorizeConfig.CodeTTL`, 10 minutes by default); a + later one is refused like any replay, and revokes nothing. A failed + revocation reaches `grant.Config.OnError`, and the next proven replay + retries it. A legitimate client retrying a lost response, or submitting a + code twice, revokes its own tokens: a code MUST NOT be reused (RFC 6749 + §4.1.2). Access tokens validated offline are out of reach, see [Offline + JWT access tokens](#offline-jwt-access-tokens). - **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 @@ -181,7 +211,7 @@ 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 +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 @@ -196,7 +226,9 @@ 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 +until its `exp`, whatever happened to its family — including the tokens +revoked after an authorization-code replay: deleting or flagging store +records never invalidates a signature. 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 @@ -258,12 +290,12 @@ span catalog. - [ ] 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. +- [ ] With `store/sql`, periodically delete rows past `expires_at` — + consumed authorization codes included, 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. diff --git a/examples/oauth2/README.md b/examples/oauth2/README.md index 1b7b22e2..a9801efb 100644 --- a/examples/oauth2/README.md +++ b/examples/oauth2/README.md @@ -76,6 +76,9 @@ curl -i -u 5cc06c3b-5755-4229-958c-a515a245aaeb:WTvuAztPD2XBauomleRzGFYuZawS07Ym http://localhost:1337/oauth2/token ``` +A code is single-use: running the same exchange again is refused with +`invalid_grant` and revokes the tokens the first exchange issued. + ## What this example does NOT cover - JWT-formatted access tokens (`jwt.OAuth2AccessTokenSigner` adapter wires diff --git a/oauth2/code_replay_error_test.go b/oauth2/code_replay_error_test.go new file mode 100644 index 00000000..8dcbfebf --- /dev/null +++ b/oauth2/code_replay_error_test.go @@ -0,0 +1,59 @@ +// 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 ( + "errors" + "fmt" + "testing" + + "github.com/hyperscale-stack/security" + "github.com/hyperscale-stack/security/oauth2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCodeReplayErrorMatchesCodeAlreadyUsed pins what a store's replay +// error looks like to its callers, bare or wrapped with %w: the +// invalid_grant ErrCodeAlreadyUsed for errors.Is, IsCode and the error +// text, and the retained record for errors.As. +func TestCodeReplayErrorMatchesCodeAlreadyUsed(t *testing.T) { + t.Parallel() + + record := &oauth2.AuthorizationCode{CodeHash: "hash", ClientID: "client"} + replay := &oauth2.CodeReplayError{Record: record} + + for name, err := range map[string]error{ + "bare": replay, + "wrapped": fmt.Errorf("store: consume: %w", replay), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + require.ErrorIs(t, err, oauth2.ErrCodeAlreadyUsed) + require.ErrorIs(t, err, security.ErrInvalidCredentials, "the core sentinel stays reachable") + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + assert.Contains(t, err.Error(), oauth2.ErrCodeAlreadyUsed.Error()) + + var got *oauth2.CodeReplayError + + require.ErrorAs(t, err, &got) + assert.Same(t, record, got.Record) + }) + } + + assert.Equal(t, oauth2.ErrCodeAlreadyUsed.Error(), replay.Error()) + assert.Same(t, oauth2.ErrCodeAlreadyUsed, errors.Unwrap(replay)) +} + +// TestCodeAlreadyUsedIsNoReplay: the bare sentinel — what a store returns +// for an unknown code — carries no record to revoke from. +func TestCodeAlreadyUsedIsNoReplay(t *testing.T) { + t.Parallel() + + var replay *oauth2.CodeReplayError + + assert.False(t, errors.As(oauth2.ErrCodeAlreadyUsed, &replay)) +} diff --git a/oauth2/code_replay_test.go b/oauth2/code_replay_test.go new file mode 100644 index 00000000..dbba76c2 --- /dev/null +++ b/oauth2/code_replay_test.go @@ -0,0 +1,253 @@ +// 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" + "net/http" + "net/http/httptest" + "net/url" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" + "github.com/hyperscale-stack/security/oauth2/clientauth" + "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" +) + +// The extra clients of the code-replay tests. +const ( + replayOtherClientID = "other-client" + replayOtherSecret = "other-secret" + replaySecondSPAID = "spa-2" + // replayWrongVerifier is a well-formed code_verifier that does not + // match pkceChallenge. + replayWrongVerifier = "a-well-formed-verifier-that-does-not-match-the-challenge" +) + +// newCodeReplayServer builds a BCP server over store serving +// authorization_code and rotating refresh_token. Its clients are client-abc +// and other-client (confidential, client_secret_basic) and two public +// SPAs ("none"), all registered with redirectURI; both error hooks report +// to sink. +func newCodeReplayServer(t *testing.T, store oauth2.Storage, sink *errorSink) *oauth2.Server { + t.Helper() + + gcfg := grant.Config{ + Storage: store, + AccessTokens: token.NewOpaque(32), + RefreshTokens: token.OpaqueRefreshAdapter{Opaque: token.NewOpaque(32)}, + AccessTTL: time.Hour, + RefreshTTL: 24 * time.Hour, + OnError: sink.hook, + } + + client := func(id, secret string, typ oauth2.ClientType) oauth2.Client { + return &oauth2.DefaultClient{ + IDValue: id, Secret: secret, TypeValue: typ, + RedirectURIValues: []string{redirectURI}, ScopeValues: []string{"read"}, + } + } + + srv, err := oauth2.NewServer(oauth2.ServerConfig{ + Profile: oauth2.Profile20BCP, + Storage: store, + ClientStore: &staticClientStore{clients: map[string]oauth2.Client{ + testClientID: client(testClientID, testClientSecret, oauth2.ClientConfidential), + replayOtherClientID: client(replayOtherClientID, replayOtherSecret, oauth2.ClientConfidential), + spaClientID: client(spaClientID, "", oauth2.ClientPublic), + replaySecondSPAID: client(replaySecondSPAID, "", oauth2.ClientPublic), + }}, + IssuerResolver: oauth2.StaticIssuer("https://auth.example", "api"), + Grants: []oauth2.Grant{grant.NewAuthorizationCode(gcfg), grant.NewRefreshToken(gcfg)}, + ClientAuth: []oauth2.ClientAuthenticator{clientauth.NewBasic(), clientauth.NewNone()}, + OnError: sink.hook, + }) + require.NoError(t, err) + + return srv +} + +// issueCode runs /authorize for clientID — S256 PKCE with pkceChallenge — +// and returns the code it redirects with. +func issueCode(t *testing.T, srv *oauth2.Server, clientID string) string { + t.Helper() + + q := authorizeQuery() + q.Set("client_id", clientID) + + rec := runAuthorize(srv, q, approve) + require.Equal(t, http.StatusFound, rec.Code) + + code := mustLocation(t, rec).Query().Get("code") + require.NotEmpty(t, code) + + return code +} + +// codeForm is the /token form redeeming code with verifier; an empty +// verifier is left out. +func codeForm(code, verifier string) url.Values { + form := url.Values{"grant_type": {"authorization_code"}, "code": {code}, "redirect_uri": {redirectURI}} + if verifier != "" { + form.Set("code_verifier", verifier) + } + + return form +} + +// asPublicClient builds the /token request posting form as the public +// client id ("none": client_id only). +func asPublicClient(id string, form url.Values) *http.Request { + form.Set("client_id", id) + + return formRequest("/oauth2/token", form, false) +} + +// TestTokenEndpointCodeReplayRevokesIssuedTokens is the reproduction of +// issue #80: a code is exchanged, then replayed. The replay is refused +// with invalid_grant (RFC 6749 §4.1.2), and the tokens the first exchange +// issued are revoked — for /introspect, for the store, and for a refresh. +func TestTokenEndpointCodeReplayRevokesIssuedTokens(t *testing.T) { + t.Parallel() + + store := memory.New() + sink := &errorSink{} + srv := newCodeReplayServer(t, store, sink) + code := issueCode(t, srv, testClientID) + + issued := decodeTokens(t, exchangeCode(srv, code, pkceVerifier)) + require.Equal(t, true, introspect(t, srv, issued.AccessToken)["active"]) + + rec := exchangeCode(srv, code, pkceVerifier) + require.Equal(t, http.StatusBadRequest, rec.Code) + + errCode, desc := tokenError(t, rec) + assert.Equal(t, oauth2.CodeInvalidGrant, errCode) + assert.Equal(t, "authorization code already consumed", desc) + + inactive := map[string]any{"active": false} + assert.Equal(t, inactive, introspect(t, srv, issued.AccessToken), "access token") + assert.Equal(t, inactive, introspect(t, srv, issued.RefreshToken), "refresh token") + + _, err := store.LookupAccessToken(t.Context(), oauth2.HashToken(nil, issued.AccessToken)) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked) + + rec = refreshExchange(t, srv, issued.RefreshToken) + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Equal(t, oauth2.CodeInvalidGrant, decodeError(t, rec)) + + assert.NotContains(t, sink.codes(), oauth2.CodeServerError) +} + +// TestTokenEndpointUnprovenCodeReplayRevokesNothing replays a public +// client's consumed code with requests that could not have redeemed it: +// each one gets the very answer of any replay — nothing tells which check +// failed — and the tokens stay active, until a replay with the code's +// client, redirect_uri and PKCE verifier revokes them. A public client_id +// is no secret; the verifier is the proof. +func TestTokenEndpointUnprovenCodeReplayRevokesNothing(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + srv := newCodeReplayServer(t, memory.New(), sink) + code := issueCode(t, srv, spaClientID) + + issued := decodeTokens(t, serve(srv.TokenHandler(), asPublicClient(spaClientID, codeForm(code, pkceVerifier)))) + + cases := []struct { + name string + req *http.Request + }{ + {"another public client", asPublicClient(replaySecondSPAID, codeForm(code, pkceVerifier))}, + {"no code_verifier", asPublicClient(spaClientID, codeForm(code, ""))}, + {"a wrong code_verifier", asPublicClient(spaClientID, codeForm(code, replayWrongVerifier))}, + {"a confidential client", basicForm("/oauth2/token", codeForm(code, pkceVerifier), replayOtherClientID, replayOtherSecret)}, + {"an unknown code", asPublicClient(spaClientID, codeForm("never-issued", pkceVerifier))}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rec := serve(srv.TokenHandler(), tc.req) + assert.Equal(t, http.StatusBadRequest, rec.Code) + + errCode, desc := tokenError(t, rec) + assert.Equal(t, oauth2.CodeInvalidGrant, errCode) + assert.Equal(t, "authorization code already consumed", desc, "the answer of any replay: no oracle") + assert.Equal(t, true, introspect(t, srv, issued.AccessToken)["active"]) + }) + } + + rec := serve(srv.TokenHandler(), asPublicClient(spaClientID, codeForm(code, pkceVerifier))) + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Equal(t, map[string]any{"active": false}, introspect(t, srv, issued.AccessToken)) + assert.NotContains(t, sink.codes(), oauth2.CodeServerError) +} + +// codeRaceStore runs beforeSave once, right before the first token pair is +// persisted, and records the pairs it is given. +type codeRaceStore struct { + *memory.Store + + beforeSave func() + saved []oauth2.TokenPair +} + +func (s *codeRaceStore) SaveTokenPair(ctx context.Context, pair *oauth2.TokenPair) error { + s.saved = append(s.saved, *pair) + + if hook := s.beforeSave; hook != nil { + s.beforeSave = nil + hook() + } + + return s.Store.SaveTokenPair(ctx, pair) +} + +// TestTokenEndpointCodeExchangeLosingReplayRaceIsInvalidGrant serves a +// proven replay of a code while its first exchange sits between the +// consumption of the code and the issuance of its tokens: the replay +// revokes the code's family first, so the first exchange issues nothing +// and answers invalid_grant. +func TestTokenEndpointCodeExchangeLosingReplayRaceIsInvalidGrant(t *testing.T) { + t.Parallel() + + store := &codeRaceStore{Store: memory.New()} + sink := &errorSink{} + srv := newCodeReplayServer(t, store, sink) + code := issueCode(t, srv, testClientID) + + var replay *httptest.ResponseRecorder + + store.beforeSave = func() { replay = exchangeCode(srv, code, pkceVerifier) } + + rec := exchangeCode(srv, code, pkceVerifier) + require.Equal(t, http.StatusBadRequest, rec.Code) + + errCode, desc := tokenError(t, rec) + assert.Equal(t, oauth2.CodeInvalidGrant, errCode) + assert.Equal(t, "token family revoked", desc) + assert.NotContains(t, rec.Body.String(), "access_token") + + require.NotNil(t, replay, "the replay ran") + assert.Equal(t, http.StatusBadRequest, replay.Code) + assert.Equal(t, oauth2.CodeInvalidGrant, decodeError(t, replay)) + + assert.Equal(t, []string{oauth2.CodeInvalidGrant, oauth2.CodeInvalidGrant}, sink.codes()) + + require.Len(t, store.saved, 1) + require.NotNil(t, store.saved[0].Refresh) + + _, err := store.LookupAccessToken(t.Context(), store.saved[0].Access.TokenHash) + require.Error(t, err) + assert.NotErrorIs(t, err, oauth2.ErrTokenFamilyRevoked, "the access token was never persisted") + + _, err = store.LookupRefreshToken(t.Context(), store.saved[0].Refresh.TokenHash) + require.Error(t, err, "the refresh token was never persisted") +} diff --git a/oauth2/doc.go b/oauth2/doc.go index ab339c02..9994aecc 100644 --- a/oauth2/doc.go +++ b/oauth2/doc.go @@ -25,17 +25,18 @@ // - 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 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). +// - Stores expose an atomic ConsumeAuthorizationCode (single use; +// consumed codes are kept until they expire, so a replayed code revokes +// the tokens issued from it), 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 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. +// failed after a refresh-token reuse or an authorization-code replay, +// 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 e9222f53..18212c21 100644 --- a/oauth2/errors.go +++ b/oauth2/errors.go @@ -94,8 +94,10 @@ var ( ErrAccessDenied = newCoded(CodeAccessDenied, "the resource owner denied the request", security.ErrAccessDenied) // ErrServerError -> 500 server_error. ErrServerError = newCoded(CodeServerError, "internal server error", nil) - // ErrCodeAlreadyUsed signals authorization-code reuse — surfaced as - // invalid_grant per RFC 6749 §4.1.2. + // ErrCodeAlreadyUsed signals an unknown or already consumed + // authorization code — surfaced as invalid_grant per RFC 6749 §4.1.2. + // On a replay, stores return it wrapped in a [*CodeReplayError] + // carrying the retained record. ErrCodeAlreadyUsed = newCoded(CodeInvalidGrant, "authorization code already consumed", security.ErrInvalidCredentials) // ErrRefreshTokenReused signals refresh-token reuse — surfaced as // invalid_grant per RFC 9700 §4.14.2. Storage implementations MUST also @@ -118,6 +120,25 @@ var ( // triggers fails; match it with errors.Is. var ErrTokenFamilyRevocationFailed = errors.New("oauth2: token family revocation failed") +// CodeReplayError is the error an [AuthorizationCodeStore] returns for a +// code it consumed before and still retains: a replay (RFC 6749 §4.1.2). +// It reads and matches as [ErrCodeAlreadyUsed] — errors.Is, [IsCode] and +// the RFC 6749 §5.2 response all see invalid_grant — and carries the +// retained record, so the authorization_code grant can revoke the tokens +// the first exchange issued. The record MUST NOT be used to issue tokens. +// Wrapping it with %w keeps it reachable through errors.As. +type CodeReplayError struct { + // Record is the replayed code as it was saved, without the raw Code. + Record *AuthorizationCode +} + +// Error implements the error interface with the text of +// [ErrCodeAlreadyUsed]. +func (e *CodeReplayError) Error() string { return ErrCodeAlreadyUsed.Error() } + +// Unwrap returns [ErrCodeAlreadyUsed]. +func (e *CodeReplayError) Unwrap() error { return ErrCodeAlreadyUsed } + // 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/grant/authorization_code.go b/oauth2/grant/authorization_code.go index c8ae6082..5f968130 100644 --- a/oauth2/grant/authorization_code.go +++ b/oauth2/grant/authorization_code.go @@ -7,7 +7,9 @@ package grant import ( "context" "crypto/rand" + "crypto/sha256" "encoding/base64" + "errors" "fmt" "slices" @@ -18,12 +20,26 @@ import ( // AuthorizationCode implements RFC 6749 §4.1.3 with the RFC 7636 PKCE // extension. The flow: // -// 1. Pop the code from storage atomically (single-use enforcement). -// 2. Re-bind client (code.ClientID MUST match the authenticated client). -// 3. Re-bind redirect_uri (RFC 6749 §4.1.3 paragraph 7). -// 4. Verify PKCE when present / required. -// 5. Generate access token (+ optional refresh token). -// 6. Persist both and return the pair. +// 1. Derive the code's token family from its hash, then consume the code +// atomically (single-use enforcement). +// 2. Refuse an expired code. +// 3. Re-bind client (code.ClientID MUST match the authenticated client). +// 4. Re-bind redirect_uri (RFC 6749 §4.1.3 paragraph 7). +// 5. Verify PKCE when present / required, then check that the client may +// use the grant. +// 6. Generate the access token (+ optional refresh token) in the code's +// family, persist both atomically and return the pair. +// +// A replay — the store still retains the consumed code — is refused with +// invalid_grant. When the replay passes steps 2-5 itself, proving +// possession of the code the way its first exchange had to, the code's +// family is revoked (RFC 6749 §4.1.2, RFC 9700 §4.2.4): every token the +// code issued stops working, and an exchange of the code still in flight +// fails at step 6 with invalid_grant instead of issuing, while the store +// keeps the family's revoked record (see [oauth2.Storage]). A replay +// failing those steps — another client, a missing or wrong verifier, +// another redirect_uri, an expired code — revokes nothing (OAuth 2.1 +// §4.1.3, §7.5.3). type AuthorizationCode struct { cfg Config } @@ -48,33 +64,79 @@ func (g *AuthorizationCode) Handle(ctx context.Context, req Request) (*Response, } hash := oauth2.HashToken(nil, rawCode) // pepper-free: code only lives in storage briefly + // Fixed before the code is consumed: a replay can revoke the family + // even while the first exchange is still issuing into it. + familyID := codeFamilyID(hash) code, err := g.cfg.Storage.ConsumeAuthorizationCode(ctx, hash) + + // A replay without its record — a typed nil included — cannot be + // validated: it is refused like an unknown code. + var replay *oauth2.CodeReplayError + if errors.As(err, &replay) && replay != nil && replay.Record != nil { + g.revokeReplayed(ctx, req, replay.Record, familyID) + + return nil, oauth2.ErrCodeAlreadyUsed + } + if err != nil { return nil, err //nolint:wrapcheck // oauth2.* sentinels pass through } + if err := g.validateExchange(req, code); err != nil { + return nil, err + } + + return issueTokenPair(ctx, g.cfg, req, code.Subject, code.Scope, familyID) +} + +// validateExchange checks that req may redeem code, in order: the code has +// not expired, was issued to the authenticated client for the same +// redirect_uri (RFC 6749 §4.1.3), its PKCE verifier matches when present or +// required (RFC 7636 §4.6), and the client may use the grant. A replay +// passing it proves possession of the code. +func (g *AuthorizationCode) validateExchange(req Request, code *oauth2.AuthorizationCode) error { if code.IsExpired(req.Now) { - return nil, oauth2.ErrInvalidGrant.WithDescription("authorization code expired") + return oauth2.ErrInvalidGrant.WithDescription("authorization code expired") } if code.ClientID != req.Client.ID() { - return nil, oauth2.ErrInvalidGrant.WithDescription("code issued for a different client") + return oauth2.ErrInvalidGrant.WithDescription("code issued for a different client") } if redirect := req.Form.Get("redirect_uri"); redirect != code.RedirectURI { - return nil, oauth2.ErrInvalidGrant.WithDescription("redirect_uri mismatch") + return oauth2.ErrInvalidGrant.WithDescription("redirect_uri mismatch") } if err := g.verifyPKCE(req, code); err != nil { - return nil, err + return err } if !grantTypeAllowed(req.Client, "authorization_code") { - return nil, oauth2.ErrUnauthorizedClient.WithDescription("client cannot use authorization_code") + return oauth2.ErrUnauthorizedClient.WithDescription("client cannot use authorization_code") } - return issueTokenPair(ctx, g.cfg, req, code.Subject, code.Scope) + return nil +} + +// revokeReplayed answers the replay of a consumed code (RFC 6749 §4.1.2). +// When req would have been accepted for code, it revokes the code'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. Any other replay revokes +// nothing: it could not have redeemed the code itself, and revoking on its +// say-so would let anyone holding a leaked code, or naming a public +// client_id, cut the client off. +func (g *AuthorizationCode) revokeReplayed(ctx context.Context, req Request, code *oauth2.AuthorizationCode, familyID string) { + if g.validateExchange(req, code) != nil { + return + } + + if err := g.cfg.revokeFamilyDetached(ctx, familyID); err != nil { + g.cfg.notifyError(ctx, oauth2.ErrServerError. + WithDescription("revoke token family failed after authorization code replay"). + WithCause(err)) + } } func (g *AuthorizationCode) verifyPKCE(req Request, code *oauth2.AuthorizationCode) error { @@ -134,9 +196,26 @@ func grantTypeAllowed(c oauth2.Client, grant string) bool { return slices.Contains(all, grant) } -// newFamilyID returns a 16-byte random identifier used to group every -// access / refresh token issued from the same original authorization. -// base64url -> 22 chars without padding. +// codeFamilyPrefix domain-separates the token family derived from a code +// hash from any other use of the hash. +const codeFamilyPrefix = "oauth2/code-family:" + +// codeFamilyID derives the token family of the tokens a code issues from +// the code's storage hash: 16 bytes of a domain-separated SHA-256, +// base64url -> 22 chars without padding, like [newFamilyID]. The exchange +// and every replay of the code compute it before consuming the code, so +// linking a code to its tokens takes no storage, and a replay can revoke +// the family before the first exchange has issued into it. +func codeFamilyID(codeHash string) string { + sum := sha256.Sum256([]byte(codeFamilyPrefix + codeHash)) + + return base64.RawURLEncoding.EncodeToString(sum[:16]) +} + +// newFamilyID returns a 16-byte random identifier grouping every access / +// refresh token issued from one authorization that has no code to derive +// it from (the legacy password grant). base64url -> 22 chars without +// padding. func newFamilyID() (string, error) { b := make([]byte, 16) if _, err := rand.Read(b); err != nil { diff --git a/oauth2/grant/authorization_code_replay_test.go b/oauth2/grant/authorization_code_replay_test.go new file mode 100644 index 00000000..8a4aa373 --- /dev/null +++ b/oauth2/grant/authorization_code_replay_test.go @@ -0,0 +1,578 @@ +// 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" + "maps" + "net/url" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" + "github.com/hyperscale-stack/security/oauth2/grant" + "github.com/hyperscale-stack/security/oauth2/pkce" + "github.com/hyperscale-stack/security/oauth2/storage/memory" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// replayStore decorates the memory store for the code-replay tests. It +// counts the family revocations, records the deadline of the context each +// one runs on, can fail them, honors that context like a real backend, can +// force the result of a code consumption, runs afterConsume after every +// one, and runs beforeSave once, right before the first SaveTokenPair it +// records — a replay landing between a code's consumption and the +// issuance of its tokens. It is safe for concurrent use once configured. +type replayStore struct { + *memory.Store + + consumeErr error + afterConsume func() + revokeErr error + revokes atomic.Int32 + + mu sync.Mutex + beforeSave func() + saved []oauth2.TokenPair + deadlines []time.Time // zero when a revocation context had none +} + +func (s *replayStore) ConsumeAuthorizationCode(ctx context.Context, hash string) (*oauth2.AuthorizationCode, error) { + if s.consumeErr != nil { + return nil, s.consumeErr + } + + code, err := s.Store.ConsumeAuthorizationCode(ctx, hash) + + if s.afterConsume != nil { + s.afterConsume() + } + + return code, err +} + +func (s *replayStore) SaveTokenPair(ctx context.Context, pair *oauth2.TokenPair) error { + s.mu.Lock() + s.saved = append(s.saved, *pair) + hook := s.beforeSave + s.beforeSave = nil + s.mu.Unlock() + + if hook != nil { + hook() + } + + return s.Store.SaveTokenPair(ctx, pair) +} + +func (s *replayStore) RevokeRefreshFamily(ctx context.Context, familyID string) error { + s.revokes.Add(1) + + deadline, _ := ctx.Deadline() + + s.mu.Lock() + s.deadlines = append(s.deadlines, deadline) + s.mu.Unlock() + + if err := ctx.Err(); err != nil { + return err + } + + if s.revokeErr != nil { + return s.revokeErr + } + + return s.Store.RevokeRefreshFamily(ctx, familyID) +} + +// revocationDeadlines returns the deadline of each family revocation's +// context, zero for none. +func (s *replayStore) revocationDeadlines() []time.Time { + s.mu.Lock() + defer s.mu.Unlock() + + return append([]time.Time(nil), s.deadlines...) +} + +// savedPairs returns the pairs the grant tried to persist. +func (s *replayStore) savedPairs() []oauth2.TokenPair { + s.mu.Lock() + defer s.mu.Unlock() + + return append([]oauth2.TokenPair(nil), s.saved...) +} + +// codeRequest saves the authorization code raw — S256 PKCE, issued to the +// test client, valid for 10 minutes — into store, and returns the /token +// request redeeming it under the BCP profile. +func codeRequest(t *testing.T, store oauth2.Storage, raw string) grant.Request { + t.Helper() + + now := time.Now() + + require.NoError(t, store.SaveAuthorizationCode(t.Context(), &oauth2.AuthorizationCode{ + Code: raw, CodeHash: oauth2.HashToken(nil, raw), + ClientID: clientID, Subject: subject, RedirectURI: redirectURI, Scope: "read:mail", + CodeChallenge: codeChallenge, CodeChallengeMethod: string(pkce.MethodS256), + IssuedAt: now, ExpiresAt: now.Add(10 * time.Minute), + })) + + return grant.Request{ + Client: newClient(), + Form: url.Values{"code": {raw}, "redirect_uri": {redirectURI}, "code_verifier": {codeVerifier}}, + Issuer: "https://auth.example", + Audience: "api", + Now: now, + Profile: oauth2.Profile20BCP, + } +} + +// withForm returns a request change editing a copy of the form. +func withForm(edit func(url.Values)) func(grant.Request) grant.Request { + return func(req grant.Request) grant.Request { + req.Form = maps.Clone(req.Form) + edit(req.Form) + + return req + } +} + +// requirePairUsable asserts that both tokens of pair still work. +func requirePairUsable(t *testing.T, store oauth2.Storage, pair oauth2.TokenPair) { + t.Helper() + + _, err := store.LookupAccessToken(t.Context(), pair.Access.TokenHash) + require.NoError(t, err, "access token") + + if pair.Refresh == nil { + return + } + + rt, err := store.LookupRefreshToken(t.Context(), pair.Refresh.TokenHash) + require.NoError(t, err, "refresh token") + assert.False(t, rt.Consumed, "refresh token") +} + +// requirePairRevoked asserts that no token of pair works any more: its +// family is revoked. +func requirePairRevoked(t *testing.T, store oauth2.Storage, pair oauth2.TokenPair) { + t.Helper() + + _, err := store.LookupAccessToken(t.Context(), pair.Access.TokenHash) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked, "access token") + + if pair.Refresh == nil { + return + } + + rt, err := store.LookupRefreshToken(t.Context(), pair.Refresh.TokenHash) + require.NoError(t, err, "refresh token") + assert.True(t, rt.Consumed, "refresh token") +} + +// requirePairAbsent asserts that nothing of pair was persisted: both +// lookups fail as for tokens never issued, not as for revoked ones. +func requirePairAbsent(t *testing.T, store oauth2.Storage, pair oauth2.TokenPair) { + t.Helper() + + _, err := store.LookupAccessToken(t.Context(), pair.Access.TokenHash) + require.Error(t, err, "access token") + assert.NotErrorIs(t, err, oauth2.ErrTokenFamilyRevoked, "access token never persisted") + + if pair.Refresh == nil { + return + } + + _, err = store.LookupRefreshToken(t.Context(), pair.Refresh.TokenHash) + require.Error(t, err, "refresh token never persisted") +} + +// TestAuthorizationCodeReplayRevokesIssuedTokens is the reproduction of +// issue #80: a code is exchanged, then replayed with the same proof. The +// replay is refused, and the tokens the first exchange issued are revoked +// (RFC 6749 §4.1.2). +func TestAuthorizationCodeReplayRevokesIssuedTokens(t *testing.T) { + t.Parallel() + + store := &replayStore{Store: memory.New()} + hooks := &hookRecorder{} + cfg := rotatingConfig(store, hooks) + g := grant.NewAuthorizationCode(cfg) + req := codeRequest(t, store, "replayed-code") + + first, err := g.Handle(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, first.Pair.Refresh) + requirePairUsable(t, store, first.Pair) + + resp, err := g.Handle(t.Context(), req) + assert.Nil(t, resp) + assert.Same(t, oauth2.ErrCodeAlreadyUsed, err, "the replay gets the bare refusal") + + assert.Equal(t, int32(1), store.revokes.Load()) + assert.Empty(t, hooks.errs) + requirePairRevoked(t, store, first.Pair) + + // The refresh token cannot be exchanged any more either. + _, err = grant.NewRefreshToken(cfg).Handle(t.Context(), refreshReq(first.Pair.Refresh.Token, oauth2.Profile20BCP)) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) +} + +// TestAuthorizationCodeReplayRevokesRotatedTokens replays a code after its +// refresh token was rotated: the rotation stayed in the code's family, so +// the replay revokes it too. +func TestAuthorizationCodeReplayRevokesRotatedTokens(t *testing.T) { + t.Parallel() + + store := &replayStore{Store: memory.New()} + cfg := rotatingConfig(store, &hookRecorder{}) + g := grant.NewAuthorizationCode(cfg) + req := codeRequest(t, store, "rotated-code") + + first, err := g.Handle(t.Context(), req) + require.NoError(t, err) + + rotated, err := grant.NewRefreshToken(cfg).Handle(t.Context(), refreshReq(first.Pair.Refresh.Token, oauth2.Profile20BCP)) + require.NoError(t, err) + require.NotNil(t, rotated.Pair.Refresh, "the BCP profile rotates") + requirePairUsable(t, store, rotated.Pair) + + _, err = g.Handle(t.Context(), req) + assert.Same(t, oauth2.ErrCodeAlreadyUsed, err) + + requirePairRevoked(t, store, first.Pair) + requirePairRevoked(t, store, rotated.Pair) +} + +// TestAuthorizationCodeReplayBeforeIssuanceRefusesTheExchange lands a +// proven replay between the first exchange's consumption of the code and +// the issuance of its tokens: the family, derived from the code before it +// was consumed, is revoked first, so the first exchange issues nothing and +// answers invalid_grant. +func TestAuthorizationCodeReplayBeforeIssuanceRefusesTheExchange(t *testing.T) { + t.Parallel() + + store := &replayStore{Store: memory.New()} + hooks := &hookRecorder{} + g := grant.NewAuthorizationCode(rotatingConfig(store, hooks)) + req := codeRequest(t, store, "raced-code") + + var replayErr error + + store.beforeSave = func() { _, replayErr = g.Handle(t.Context(), req) } + + resp, err := g.Handle(t.Context(), req) + assert.Nil(t, resp) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + + assert.Same(t, oauth2.ErrCodeAlreadyUsed, replayErr) + assert.Equal(t, int32(1), store.revokes.Load()) + assert.Empty(t, hooks.errs, "the refusals are no server errors") + + saved := store.savedPairs() + require.Len(t, saved, 1, "only the first exchange reached issuance") + requirePairAbsent(t, store, saved[0]) +} + +// TestAuthorizationCodeReplayRevokesAccessOnlyIssuance covers a grant +// without a refresh-token generator: the lone access token is in the +// code's family and revoked with it. +func TestAuthorizationCodeReplayRevokesAccessOnlyIssuance(t *testing.T) { + t.Parallel() + + store := &replayStore{Store: memory.New()} + g := grant.NewAuthorizationCode(grant.Config{Storage: store, AccessTokens: newAccessGen(), AccessTTL: time.Hour}) + req := codeRequest(t, store, "access-only-code") + + first, err := g.Handle(t.Context(), req) + require.NoError(t, err) + require.Nil(t, first.Pair.Refresh) + assert.NotEmpty(t, first.Pair.Access.FamilyID) + + _, err = g.Handle(t.Context(), req) + assert.Same(t, oauth2.ErrCodeAlreadyUsed, err) + requirePairRevoked(t, store, first.Pair) +} + +// TestAuthorizationCodeUnprovenReplayRevokesNothing replays a consumed code +// with requests that could not have redeemed it — another client, a +// missing or wrong proof, another redirect_uri, an expired code, a client +// no longer allowed the grant — and an unknown code: each one revokes +// nothing (OAuth 2.1 §4.1.3, §7.5.3) and gets the very answer of any +// replay, so the response tells nothing about which check failed. A +// request that would have been accepted revokes. +func TestAuthorizationCodeUnprovenReplayRevokesNothing(t *testing.T) { + t.Parallel() + + store := &replayStore{Store: memory.New()} + hooks := &hookRecorder{} + g := grant.NewAuthorizationCode(rotatingConfig(store, hooks)) + req := codeRequest(t, store, "unproven-code") + + first, err := g.Handle(t.Context(), req) + require.NoError(t, err) + + cases := []struct { + name string + replay func(grant.Request) grant.Request + }{ + {"another confidential client", func(r grant.Request) grant.Request { + r.Client = &oauth2.DefaultClient{ + IDValue: "client-2", Secret: "secret-2", TypeValue: oauth2.ClientConfidential, + RedirectURIValues: []string{redirectURI}, + } + + return r + }}, + {"no code_verifier", withForm(func(f url.Values) { f.Del("code_verifier") })}, + {"a well-formed wrong code_verifier", withForm(func(f url.Values) { f.Set("code_verifier", wellFormedWrongVerifier) })}, + {"a malformed code_verifier", withForm(func(f url.Values) { f.Set("code_verifier", "x") })}, + {"another redirect_uri", withForm(func(f url.Values) { f.Set("redirect_uri", "https://attacker.example/cb") })}, + {"an expired code", func(r grant.Request) grant.Request { + r.Now = r.Now.Add(time.Hour) + + return r + }}, + {"a client no longer allowed authorization_code", func(r grant.Request) grant.Request { + r.Client = &oauth2.DefaultClient{ + IDValue: clientID, Secret: clientSecret, TypeValue: oauth2.ClientConfidential, + RedirectURIValues: []string{redirectURI}, GrantTypeValues: []string{"client_credentials"}, + } + + return r + }}, + {"an unknown code", withForm(func(f url.Values) { f.Set("code", "never-issued") })}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resp, err := g.Handle(t.Context(), tc.replay(req)) + assert.Nil(t, resp) + assert.Same(t, oauth2.ErrCodeAlreadyUsed, err, "the answer of any replay: no oracle") + + assert.Zero(t, store.revokes.Load()) + assert.Empty(t, hooks.errs) + requirePairUsable(t, store, first.Pair) + }) + } + + // The positive control: the same request as the first exchange. + _, err = g.Handle(t.Context(), req) + assert.Same(t, oauth2.ErrCodeAlreadyUsed, err) + assert.Equal(t, int32(1), store.revokes.Load()) + requirePairRevoked(t, store, first.Pair) +} + +// TestAuthorizationCodeReplayWithoutRecordRevokesNothing covers a store +// reporting a replay without the retained record — a CodeReplayError +// without one, or a typed nil: the replay cannot be validated, so the +// grant refuses it like an unknown code and revokes nothing. +func TestAuthorizationCodeReplayWithoutRecordRevokesNothing(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + err error + }{ + {"no record", &oauth2.CodeReplayError{}}, + {"typed nil", (*oauth2.CodeReplayError)(nil)}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + store := &replayStore{Store: memory.New(), consumeErr: tc.err} + g := grant.NewAuthorizationCode(rotatingConfig(store, &hookRecorder{})) + + resp, err := g.Handle(t.Context(), codeRequest(t, store, "recordless-code")) + assert.Nil(t, resp) + require.ErrorIs(t, err, oauth2.ErrCodeAlreadyUsed) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + assert.Zero(t, store.revokes.Load()) + }) + } +} + +// TestAuthorizationCodeReplayRevocationFailureIsReported fails the +// revocation a proven replay triggers: the client still gets the refusal, +// the failure reaches the grant's error hook once, and the retained code +// lets the next proven replay revoke the family. +func TestAuthorizationCodeReplayRevocationFailureIsReported(t *testing.T) { + t.Parallel() + + store := &replayStore{Store: memory.New()} + hooks := &hookRecorder{} + g := grant.NewAuthorizationCode(rotatingConfig(store, hooks)) + req := codeRequest(t, store, "unrevoked-code") + + first, err := g.Handle(t.Context(), req) + require.NoError(t, err) + + family := first.Pair.Access.FamilyID + store.revokeErr = revocationFailure(family) + + _, err = g.Handle(t.Context(), req) + assert.Same(t, oauth2.ErrCodeAlreadyUsed, err, "the client gets the refusal either way") + + 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 token family failed after authorization code replay") + assert.ErrorIs(t, got, oauth2.ErrTokenFamilyRevocationFailed) + assert.ErrorIs(t, got, errBackendDown) + assert.ErrorContains(t, errors.Unwrap(got), family, "the cause names the family") + assert.NotErrorIs(t, got, oauth2.ErrCodeAlreadyUsed, "the report counts the revocation, not the replay") + requirePairUsable(t, store, first.Pair) + + // The backend recovers: the next proven replay revokes, quietly. + store.revokeErr = nil + + _, err = g.Handle(t.Context(), req) + assert.Same(t, oauth2.ErrCodeAlreadyUsed, err) + requirePairRevoked(t, store, first.Pair) + assert.Len(t, hooks.errs, 1, "a successful revocation is not reported") +} + +// TestAuthorizationCodeReplayRevocationDetachedFromRequest cancels the +// replaying request right after its code is consumed — a client hanging +// up: the revocation still runs, on a context detached from the request +// and bounded by the detached revocation timeout (10s). +func TestAuthorizationCodeReplayRevocationDetachedFromRequest(t *testing.T) { + t.Parallel() + + store := &replayStore{Store: memory.New()} + hooks := &hookRecorder{} + g := grant.NewAuthorizationCode(rotatingConfig(store, hooks)) + req := codeRequest(t, store, "hung-up-code") + + first, err := g.Handle(t.Context(), req) + require.NoError(t, err) + + ctx, cancel := context.WithCancel(t.Context()) + store.afterConsume = cancel + + _, err = g.Handle(ctx, req) + assert.Same(t, oauth2.ErrCodeAlreadyUsed, err) + require.Error(t, ctx.Err(), "the request was canceled") + + assert.Equal(t, int32(1), store.revokes.Load()) + + deadlines := store.revocationDeadlines() + require.Len(t, deadlines, 1) + require.False(t, deadlines[0].IsZero(), "the detached revocation is bounded") + assert.WithinDuration(t, time.Now(), deadlines[0], 10*time.Second, "by the detached revocation timeout") + assert.Empty(t, hooks.errs) + requirePairRevoked(t, store, first.Pair) +} + +// TestAuthorizationCodeReplayOnlyRevokesItsOwnFamily exchanges two codes of +// the same client: each opens a family of its own, and replaying one +// leaves the other untouched. +func TestAuthorizationCodeReplayOnlyRevokesItsOwnFamily(t *testing.T) { + t.Parallel() + + store := &replayStore{Store: memory.New()} + g := grant.NewAuthorizationCode(rotatingConfig(store, &hookRecorder{})) + reqA := codeRequest(t, store, "code-a") + reqB := codeRequest(t, store, "code-b") + + a, err := g.Handle(t.Context(), reqA) + require.NoError(t, err) + + b, err := g.Handle(t.Context(), reqB) + require.NoError(t, err) + + assert.Len(t, a.Pair.Access.FamilyID, 22) + assert.Len(t, b.Pair.Access.FamilyID, 22) + assert.NotEqual(t, a.Pair.Access.FamilyID, b.Pair.Access.FamilyID) + + _, err = g.Handle(t.Context(), reqA) + assert.Same(t, oauth2.ErrCodeAlreadyUsed, err) + + requirePairRevoked(t, store, a.Pair) + requirePairUsable(t, store, b.Pair) +} + +// TestAuthorizationCodeConcurrentRedemption exchanges one code from 16 +// goroutines at once. Whatever the schedule, at most one exchange +// succeeds, every other one is an invalid_grant, every loser revokes the +// code's family, and a winner's tokens end up revoked. +func TestAuthorizationCodeConcurrentRedemption(t *testing.T) { + t.Parallel() + + store := &replayStore{Store: memory.New()} + g := grant.NewAuthorizationCode(grant.Config{ + Storage: store, AccessTokens: newAccessGen(), RefreshTokens: newRefreshGen(), + AccessTTL: time.Hour, RefreshTTL: 24 * time.Hour, + }) + req := codeRequest(t, store, "contended-code") + + const n = 16 + + resps := make([]*grant.Response, n) + errs := make([]error, n) + + var wg sync.WaitGroup + + start := make(chan struct{}) + + for i := range n { + wg.Go(func() { + <-start + + resps[i], errs[i] = g.Handle(context.Background(), req) + }) + } + + close(start) + wg.Wait() + + wins := 0 + + for i, err := range errs { + if err == nil { + wins++ + + requirePairRevoked(t, store, resps[i].Pair) + + continue + } + + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err), "%v", err) + } + + assert.LessOrEqual(t, wins, 1) + assert.Equal(t, int32(n-1), store.revokes.Load(), "every replay proved possession") +} + +// TestLegacyPasswordIssuesFreshFamilies: the password grant has no code to +// derive a family from, so each exchange opens a new random one. +func TestLegacyPasswordIssuesFreshFamilies(t *testing.T) { + t.Parallel() + + g := grant.NewLegacyPassword(grant.Config{ + Storage: newStore(), AccessTokens: newAccessGen(), + RefreshTokens: newRefreshGen(), AccessTTL: time.Hour, RefreshTTL: time.Hour, + }, fakeROVerifier{subject: subject}) + + req := grant.Request{Client: newClient(), Form: passwordForm("alice", "s3cr3t", ""), Now: time.Now()} + + a, err := g.Handle(t.Context(), req) + require.NoError(t, err) + + b, err := g.Handle(t.Context(), req) + require.NoError(t, err) + + assert.NotEmpty(t, a.Pair.Access.FamilyID) + assert.NotEqual(t, a.Pair.Access.FamilyID, b.Pair.Access.FamilyID) + assert.Equal(t, a.Pair.Access.FamilyID, a.Pair.Refresh.FamilyID) +} diff --git a/oauth2/grant/grant.go b/oauth2/grant/grant.go index fd0a4aa0..d4944f58 100644 --- a/oauth2/grant/grant.go +++ b/oauth2/grant/grant.go @@ -9,7 +9,8 @@ // Three grants are shipped: // // - authorization_code (with PKCE; PKCE is mandatory in -// [oauth2.Profile20BCP] and [oauth2.Profile21Draft]) +// [oauth2.Profile20BCP] and [oauth2.Profile21Draft]); a replayed code +// revokes the tokens it issued // - client_credentials // - refresh_token (rotation + reuse detection; rotation is mandatory in // [oauth2.Profile20BCP] and [oauth2.Profile21Draft]) @@ -59,8 +60,9 @@ type Config struct { // OnError, when set, observes the errors a grant swallows to keep the // protocol response intact — today, a family revocation that failed // 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 + // consumed or the store reported the reuse while rotating, or after an + // authorization-code replay (RFC 6749 §4.1.2): 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 diff --git a/oauth2/grant/issue.go b/oauth2/grant/issue.go index bbcd6869..089ff936 100644 --- a/oauth2/grant/issue.go +++ b/oauth2/grant/issue.go @@ -12,16 +12,14 @@ import ( ) // issueTokenPair mints an access token — and, when the config carries a -// 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) - } - +// refresh-token generator, a companion refresh token — in the family +// familyID for the given subject and scope, persists them atomically, and +// returns the grant response. It is the shared issuance path of the +// authorization_code grant, whose family is derived from the code, and of +// the legacy password grant, whose family is new. A family revoked in the +// meantime — by a replay of the code — refuses the pair with invalid_grant, +// and nothing is issued. +func issueTokenPair(ctx context.Context, cfg Config, req Request, subject, scope, familyID string) (*Response, error) { access, err := mintAccess(ctx, cfg, req, subject, scope, familyID) if err != nil { return nil, err diff --git a/oauth2/grant/legacy_password.go b/oauth2/grant/legacy_password.go index e56493cd..cfd127f7 100644 --- a/oauth2/grant/legacy_password.go +++ b/oauth2/grant/legacy_password.go @@ -82,7 +82,12 @@ func (g *LegacyPassword) Handle(ctx context.Context, req Request) (*Response, er return nil, err } - return issueTokenPair(ctx, g.cfg, req, subject, scope) + familyID, err := newFamilyID() + if err != nil { + return nil, oauth2.ErrServerError.WithCause(err) + } + + return issueTokenPair(ctx, g.cfg, req, subject, scope, familyID) } // Compile-time interface check. diff --git a/oauth2/models.go b/oauth2/models.go index 6536f20e..5212136b 100644 --- a/oauth2/models.go +++ b/oauth2/models.go @@ -8,16 +8,18 @@ import "time" // AuthorizationCode is the record persisted between the /authorize call and // the matching /token call. The Code field carries the raw, single-use value -// returned to the user-agent; storage implementations MUST hash it before -// persisting and consume atomically (RFC 6749 §4.1.2 + OAuth 2.0 BCP §4.5). +// returned to the user-agent; storage implementations key the record by +// CodeHash, never persist the raw value, consume it atomically, and keep +// the consumed record until ExpiresAt so that a replay is recognized (RFC +// 6749 §4.1.2, RFC 9700 §4.2.4; see [AuthorizationCodeStore]). type AuthorizationCode struct { // Code is the raw, single-use authorization code as issued to the // user-agent. Storage implementations persist its hash, never the raw // value. The struct carries the raw form because the issuance flow // needs to redirect it. Code string - // CodeHash is the storage-side hash of Code. Filled in by the storage - // layer; the issuance flow leaves it empty. + // CodeHash is the storage key: HashToken(nil, Code), set by the issuer + // (the authorization endpoint). Stores reject an empty one. CodeHash string // ClientID is the requesting client's identifier. ClientID string diff --git a/oauth2/storage.go b/oauth2/storage.go index 82a5db2c..144d7388 100644 --- a/oauth2/storage.go +++ b/oauth2/storage.go @@ -22,19 +22,40 @@ const FamilyRevocationRetention = 24 * time.Hour // 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 -// to AT MOST one caller. +// AuthorizationCodeStore persists single-use authorization codes (RFC 6749 +// §4.1.2, §10.5). A code is stored under its hash, +// [AuthorizationCode.CodeHash], which the issuer sets: implementations MUST +// NOT persist the raw [AuthorizationCode.Code]. Consuming a code does not +// delete it: the record is kept, flagged consumed, at least until its +// ExpiresAt, so that a replay can be told from an unknown code and the +// server can revoke what the first exchange issued. Afterwards an +// implementation MAY drop it — the Redis store expires the key, the SQL +// store documents a cleanup, the memory store keeps it for its lifetime. type AuthorizationCodeStore interface { - // SaveAuthorizationCode persists the code. The storage layer hashes - // code.Code into code.CodeHash before persisting. + // SaveAuthorizationCode persists code under code.CodeHash; an empty + // hash is rejected with invalid_request. Callers MUST save a given + // CodeHash at most once: a store may overwrite the record, and + // re-saving a consumed code can make it redeemable again (the Redis + // store does). SaveAuthorizationCode(ctx context.Context, code *AuthorizationCode) error - // ConsumeAuthorizationCode atomically reads-and-deletes the code - // identified by codeHash. Returns [ErrCodeAlreadyUsed] when the code - // was previously consumed (allowing the server to reject reuse with - // invalid_grant and revoke the resulting access token per RFC 6749 - // §4.1.2). + // ConsumeAuthorizationCode atomically marks the code codeHash consumed + // and returns its record to AT MOST one caller, even under + // concurrency. Every other call returns a nil code and an error: + // + // - a [*CodeReplayError] carrying the retained record when the code + // was already consumed — every concurrent loser gets one. The + // record only lets the caller revoke what the first exchange + // issued: it MUST NOT be used to issue tokens; + // - [ErrCodeAlreadyUsed] when the code is unknown: never saved, or + // no longer retained; + // - a wrapped error on a backend failure. + // + // Both refusals match [ErrCodeAlreadyUsed] (invalid_grant). A decorator + // or wrapper MUST return the store's error unchanged or wrapped with + // %w: normalizing it to ErrCodeAlreadyUsed silently disables the + // revocation a replay triggers. Expiry is not checked: the caller + // compares ExpiresAt with its own clock. ConsumeAuthorizationCode(ctx context.Context, codeHash string) (*AuthorizationCode, error) } @@ -153,6 +174,13 @@ type RefreshTokenStore interface { // active again, reviving its tokens, so a store MUST NOT lose a // revoked family's state within its retention (I5). // +// The authorization_code grant derives the family of the tokens a code +// issues from the code hash, before consuming the code. A replay of the +// code that proves possession revokes that family: by I1 and I2, the +// tokens the code issued stop working, and an exchange of the code still +// in flight fails instead of issuing — one more reason a store MUST keep a +// revoked family's state for its whole retention (I5, I8). +// // 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 diff --git a/oauth2/storage/memory/memory.go b/oauth2/storage/memory/memory.go index 212f63f1..24144cee 100644 --- a/oauth2/storage/memory/memory.go +++ b/oauth2/storage/memory/memory.go @@ -2,15 +2,16 @@ // Use of this source code is governed by a MIT // license that can be found in the LICENSE file. -// Package memory ships an in-process [oauth2.Storage] implementation -// suitable for tests, examples and small single-instance deployments. -// Production deployments MUST use the SQL or Redis implementations -// instead — the in-memory store loses all state on restart. +// Package memory ships an in-process [oauth2.Storage] implementation for +// development and tests. Production deployments MUST use the SQL or Redis +// implementations instead — the in-memory store loses all state on +// restart, and never purges anything: it grows with every code and token +// it is given, since consumed and expired codes, expired tokens and +// revoked families stay in memory for the process lifetime. // // 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. Nothing is ever purged: expired -// tokens and revoked families stay in memory for the process lifetime. +// optimized for clarity, not for scale. package memory import ( @@ -30,24 +31,27 @@ import ( // 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 - revoked map[string]struct{} // revoked token families + mu sync.Mutex + codes map[string]oauth2.AuthorizationCode + usedCodes map[string]struct{} // consumed codes, kept in codes + 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), - revoked: make(map[string]struct{}), + codes: make(map[string]oauth2.AuthorizationCode), + usedCodes: make(map[string]struct{}), + access: make(map[string]oauth2.AccessToken), + refresh: make(map[string]oauth2.RefreshToken), + revoked: make(map[string]struct{}), } } -// SaveAuthorizationCode implements [oauth2.AuthorizationCodeStore]. +// SaveAuthorizationCode implements [oauth2.AuthorizationCodeStore]. The +// record is kept without the raw code. func (s *Store) SaveAuthorizationCode(_ context.Context, code *oauth2.AuthorizationCode) error { s.mu.Lock() defer s.mu.Unlock() @@ -56,13 +60,17 @@ func (s *Store) SaveAuthorizationCode(_ context.Context, code *oauth2.Authorizat return oauth2.ErrInvalidRequest.WithDescription("storage: empty code hash") } - s.codes[code.CodeHash] = *code + record := *code + record.Code = "" + s.codes[code.CodeHash] = record return nil } -// ConsumeAuthorizationCode implements [oauth2.AuthorizationCodeStore]. The -// operation is atomic under the store's mutex. +// ConsumeAuthorizationCode implements [oauth2.AuthorizationCodeStore]. +// Under the store's mutex it marks the code consumed; the record stays for +// the store's lifetime, and every later call gets an +// [*oauth2.CodeReplayError] carrying it. func (s *Store) ConsumeAuthorizationCode(_ context.Context, codeHash string) (*oauth2.AuthorizationCode, error) { s.mu.Lock() defer s.mu.Unlock() @@ -72,15 +80,17 @@ func (s *Store) ConsumeAuthorizationCode(_ context.Context, codeHash string) (*o return nil, oauth2.ErrCodeAlreadyUsed } - delete(s.codes, codeHash) + if _, used := s.usedCodes[codeHash]; used { + return nil, &oauth2.CodeReplayError{Record: &c} + } - // Expiry is NOT checked here: the store only guarantees atomic - // single-use read+delete. The grant handler validates IsExpired with - // its injected clock — keeping the check in one place avoids the - // store and the grant disagreeing on "now". - cp := c + s.usedCodes[codeHash] = struct{}{} - return &cp, nil + // Expiry is NOT checked here: the store only guarantees atomic single + // use. The grant handler validates IsExpired with its injected clock — + // keeping the check in one place avoids the store and the grant + // disagreeing on "now". + return &c, nil } // SaveTokenPair implements [oauth2.Storage]: under the store's mutex it diff --git a/oauth2/store/redis/codec.go b/oauth2/store/redis/codec.go index d9f7d567..f2f129ba 100644 --- a/oauth2/store/redis/codec.go +++ b/oauth2/store/redis/codec.go @@ -26,9 +26,12 @@ 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 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. +// unambiguous encoding. Some field names are load-bearing: the rotation Lua +// script reads the refresh token's `family_id` and `consumed` via cjson, +// and the code-consumption script reads the code's `consumed` and writes +// it with `consumed_by`. Both scripts re-encode the record they consume +// with cjson, which may escape "/" as "\/" and prints numbers with 14 +// significant digits: Go decodes either unchanged. type codeDTO struct { ClientID string `json:"client_id"` @@ -40,6 +43,11 @@ type codeDTO struct { Nonce string `json:"nonce"` IssuedAt int64 `json:"issued_at"` ExpiresAt int64 `json:"expires_at"` + // Consumed and ConsumedBy are only written by consumeCodeScript: the + // flag of a consumed code and the nonce of the call that consumed it. + // A payload without them — or with nulls — is an unconsumed code. + Consumed bool `json:"consumed,omitempty"` + ConsumedBy string `json:"consumed_by,omitempty"` } func encodeCode(c *oauth2.AuthorizationCode) ([]byte, error) { @@ -56,10 +64,12 @@ func encodeCode(c *oauth2.AuthorizationCode) ([]byte, error) { }) } -func decodeCode(hash string, raw []byte) (*oauth2.AuthorizationCode, error) { +// decodeCode decodes a code payload into its record, and reports whether +// the code is consumed and the nonce of the call that consumed it. +func decodeCode(hash string, raw []byte) (*oauth2.AuthorizationCode, bool, string, error) { var d codeDTO if err := json.Unmarshal(raw, &d); err != nil { - return nil, err //nolint:wrapcheck // caller wraps + return nil, false, "", err //nolint:wrapcheck // caller wraps } return &oauth2.AuthorizationCode{ @@ -73,7 +83,7 @@ func decodeCode(hash string, raw []byte) (*oauth2.AuthorizationCode, error) { Nonce: d.Nonce, IssuedAt: time.Unix(d.IssuedAt, 0), ExpiresAt: time.Unix(d.ExpiresAt, 0), - }, nil + }, d.Consumed, d.ConsumedBy, nil } type accessDTO struct { diff --git a/oauth2/store/redis/doc.go b/oauth2/store/redis/doc.go index d28a4e47..1262310f 100644 --- a/oauth2/store/redis/doc.go +++ b/oauth2/store/redis/doc.go @@ -11,7 +11,9 @@ // Every key starts with the store prefix (WithKeyPrefix, "oauth2:" by // default): // -// - code:{hash}: an authorization code (JSON), until it expires. +// - code:{hash}: an authorization code (JSON), until it expires — +// exchanged or not: an exchanged code stays, flagged consumed, so that +// a replay of it is recognized. // - 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 @@ -23,8 +25,9 @@ // 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. +// - Redis 6.0 or later: the rotation and code-consumption scripts use +// 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). @@ -40,11 +43,13 @@ // 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. +// the family, reviving its tokens. An evicted code key reads as an +// unknown code: a replay of it is still refused, but no longer +// recognized as one. // -// 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. +// go-redis resends a command whose reply was lost; every script is safe to +// run twice: a resent rotation is not mistaken for a reuse, nor a resent +// code consumption for a replay. // // Allowed dependencies: // - github.com/hyperscale-stack/security/oauth2 diff --git a/oauth2/store/redis/store.go b/oauth2/store/redis/store.go index 9bd29df4..d379984f 100644 --- a/oauth2/store/redis/store.go +++ b/oauth2/store/redis/store.go @@ -6,6 +6,7 @@ package redisstore import ( "context" + "crypto/rand" "errors" "fmt" @@ -19,6 +20,10 @@ import ( // without interleaving other commands, so each read-modify-write sequence // is indivisible. // +// ConsumeAuthorizationCode runs one script that flags the code consumed in +// its key, which keeps its TTL: a replay is recognized until the code +// expires, and the key expires with it. +// // Each token family has one key, {prefix}fam:{id}, holding "active" or // "revoked": the point every write into the family serializes on. // @@ -84,12 +89,22 @@ const ( // --- Lua scripts --------------------------------------------------------- -// consumeCodeScript atomically reads-and-deletes an authorization code. -// Returns the JSON value, or false when the key is absent. +// consumeCodeScript atomically flags an authorization code consumed, +// keeping the key's TTL, and returns the payload as it found it — false +// when the key is absent. A payload already flagged is returned unchanged: +// it is a replay, unless this very call flagged it, which the caller +// tells from the consumed_by nonce the script records. cjson decodes a +// JSON null as a truthy value, hence the explicit comparison. +// +// KEYS[1] code; ARGV[1] the nonce of the call var consumeCodeScript = redis.NewScript(` local v = redis.call('GET', KEYS[1]) if not v then return false end -redis.call('DEL', KEYS[1]) +local c = cjson.decode(v) +if c.consumed == true then return v end +c.consumed = true +c.consumed_by = ARGV[1] +redis.call('SET', KEYS[1], cjson.encode(c), 'KEEPTTL') return v `) @@ -165,7 +180,10 @@ return 'ok' // --- authorization codes ------------------------------------------------- -// SaveAuthorizationCode implements [oauth2.AuthorizationCodeStore]. +// SaveAuthorizationCode implements [oauth2.AuthorizationCodeStore] with a +// plain SET, which overwrites any record under the same hash: a consumed +// code saved again becomes redeemable again, so callers MUST save a given +// CodeHash at most once. func (s *Store) SaveAuthorizationCode(ctx context.Context, code *oauth2.AuthorizationCode) error { if code.CodeHash == "" { return oauth2.ErrInvalidRequest.WithDescription("redisstore: empty code hash") @@ -184,9 +202,18 @@ func (s *Store) SaveAuthorizationCode(ctx context.Context, code *oauth2.Authoriz } // ConsumeAuthorizationCode implements [oauth2.AuthorizationCodeStore] via -// the consumeCode Lua script — atomic read+delete. +// consumeCodeScript: one atomic step flags the code consumed, and a replay +// gets an [*oauth2.CodeReplayError] carrying the record until the key +// expires with the code. +// +// Each call sends the script a fresh nonce, which the script records when +// it flags the code: go-redis resends a script whose reply was lost, and +// the resend — finding the code flagged under its own nonce — must not +// read as a replay of the exchange it is part of. func (s *Store) ConsumeAuthorizationCode(ctx context.Context, codeHash string) (*oauth2.AuthorizationCode, error) { - res, err := consumeCodeScript.Run(ctx, s.rdb, []string{s.codeKey(codeHash)}).Result() + nonce := rand.Text() + + res, err := consumeCodeScript.Run(ctx, s.rdb, []string{s.codeKey(codeHash)}, nonce).Result() if err != nil { if errors.Is(err, redis.Nil) { return nil, oauth2.ErrCodeAlreadyUsed @@ -197,15 +224,22 @@ func (s *Store) ConsumeAuthorizationCode(ctx context.Context, codeHash string) ( str, ok := res.(string) if !ok { - // The script returned false: key absent / already consumed. + // Any other reply type: treat as an unknown code (a Lua false + // arrives as redis.Nil, handled above). return nil, oauth2.ErrCodeAlreadyUsed } - code, err := decodeCode(codeHash, []byte(str)) + code, consumed, consumedBy, err := decodeCode(codeHash, []byte(str)) if err != nil { return nil, fmt.Errorf("redisstore: decode code: %w", err) } + // The payload is the one the script found: unconsumed when this call + // consumed the code, or flagged by this very call when it was resent. + if won := !consumed || (nonce != "" && consumedBy == nonce); !won { + return nil, &oauth2.CodeReplayError{Record: code} + } + return code, nil } diff --git a/oauth2/store/redis/store_code_test.go b/oauth2/store/redis/store_code_test.go new file mode 100644 index 00000000..15da131f --- /dev/null +++ b/oauth2/store/redis/store_code_test.go @@ -0,0 +1,160 @@ +// 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 ( + "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" +) + +// codeKey is the key of an authorization code under the default prefix. +func codeKey(hash string) string { return "oauth2:code:" + hash } + +// testCode returns an authorization code stored under hash that expires +// after ttl. +func testCode(hash string, ttl time.Duration) *oauth2.AuthorizationCode { + now := time.Now() + + return &oauth2.AuthorizationCode{ + Code: "raw-" + hash, CodeHash: hash, ClientID: "c", Subject: "s", + RedirectURI: "https://app.example/cb", Scope: "r", + CodeChallenge: "challenge", CodeChallengeMethod: "S256", Nonce: "n", + IssuedAt: now, ExpiresAt: now.Add(ttl), + } +} + +// requireCodeReplay asserts that got and err report a replay of the code +// stored under hash: no code, and the retained record in the error. +func requireCodeReplay(t *testing.T, got *oauth2.AuthorizationCode, err error, hash string) { + t.Helper() + + assert.Nil(t, got) + + var replay *oauth2.CodeReplayError + + require.ErrorAs(t, err, &replay) + require.NotNil(t, replay.Record) + assert.Equal(t, hash, replay.Record.CodeHash) + assert.Equal(t, "c", replay.Record.ClientID) +} + +// TestRedisConsumedCodeKeepsItsTTL: consuming a code flags its key without +// touching its TTL, so the record stays recognizable exactly until the code +// expires, and no longer. +func TestRedisConsumedCodeKeepsItsTTL(t *testing.T) { + t.Parallel() + + store, _, mr := hookedStores(t, &scriptHook{}) + ctx := t.Context() + + require.NoError(t, store.SaveAuthorizationCode(ctx, testCode("ttl", 10*time.Minute))) + + mr.FastForward(4 * time.Minute) + + got, err := store.ConsumeAuthorizationCode(ctx, "ttl") + require.NoError(t, err) + assert.Equal(t, "c", got.ClientID) + assert.InDelta(t, 6*time.Minute, mr.TTL(codeKey("ttl")), float64(time.Second), "neither reset nor cleared") + + got, err = store.ConsumeAuthorizationCode(ctx, "ttl") + requireCodeReplay(t, got, err, "ttl") + + mr.FastForward(7 * time.Minute) + assert.False(t, mr.Exists(codeKey("ttl")), "the record expires with the code") + + got, err = store.ConsumeAuthorizationCode(ctx, "ttl") + assert.Nil(t, got) + require.ErrorIs(t, err, oauth2.ErrCodeAlreadyUsed) + + var replay *oauth2.CodeReplayError + + assert.NotErrorAs(t, err, &replay, "an expired record reads as an unknown code") +} + +// TestRedisCodePayloadWithoutFlagIsConsumable covers payloads without a +// consumed flag — written before the flag existed — and with a null one: +// both are unconsumed codes, consumed once. miniredis decodes a JSON null +// as nil, so the null case only checks the Go side; on a real Redis, +// cjson.null is truthy, and the script's == true comparison is what keeps +// such a payload single-use. +func TestRedisCodePayloadWithoutFlagIsConsumable(t *testing.T) { + t.Parallel() + + cases := []struct{ name, payload string }{ + {"no flag", `{"client_id":"c","subject":"s","redirect_uri":"https://app.example/cb","scope":"r",` + + `"code_challenge":"","code_challenge_method":"","nonce":"","issued_at":0,"expires_at":0}`}, + {"null flag", `{"client_id":"c","subject":"s","redirect_uri":"https://app.example/cb","scope":"r",` + + `"code_challenge":"","code_challenge_method":"","nonce":"","issued_at":0,"expires_at":0,` + + `"consumed":null,"consumed_by":null}`}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + store, _, mr := hookedStores(t, &scriptHook{}) + require.NoError(t, mr.Set(codeKey("legacy"), tc.payload)) + mr.SetTTL(codeKey("legacy"), 10*time.Minute) + + got, err := store.ConsumeAuthorizationCode(t.Context(), "legacy") + require.NoError(t, err) + assert.Equal(t, "https://app.example/cb", got.RedirectURI) + assert.InDelta(t, 10*time.Minute, mr.TTL(codeKey("legacy")), float64(time.Second)) + + got, err = store.ConsumeAuthorizationCode(t.Context(), "legacy") + requireCodeReplay(t, got, err, "legacy") + }) + } +} + +// TestRedisConsumeCodeDecodeError covers a payload the script can decode +// but the store cannot: the consume fails with an error, not a code. +func TestRedisConsumeCodeDecodeError(t *testing.T) { + t.Parallel() + + store, _, mr := hookedStores(t, &scriptHook{}) + require.NoError(t, mr.Set(codeKey("odd"), `{"issued_at":"x"}`)) + + got, err := store.ConsumeAuthorizationCode(t.Context(), "odd") + assert.Nil(t, got) + require.ErrorContains(t, err, "redisstore: decode code") + assert.NotErrorIs(t, err, oauth2.ErrCodeAlreadyUsed) +} + +// TestRedisResentConsumeIsNotReplay runs the consume script twice, the way +// go-redis resends a command whose reply was lost: the second run finds the +// code consumed by the first, and the store must still see its own win — +// not a replay that refuses the exchange. +func TestRedisResentConsumeIsNotReplay(t *testing.T) { + t.Parallel() + + hook := &resendHook{} + hook.numKeys, hook.key = 1, codeKey("resent") + + 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.SaveAuthorizationCode(t.Context(), testCode("resent", time.Minute))) + + got, err := store.ConsumeAuthorizationCode(t.Context(), "resent") + require.NoError(t, err) + assert.Equal(t, "c", got.ClientID) + + // Another call — another nonce — sees a replay. + got, err = store.ConsumeAuthorizationCode(t.Context(), "resent") + requireCodeReplay(t, got, err, "resent") +} diff --git a/oauth2/store/sql/dialect.go b/oauth2/store/sql/dialect.go index 8a93041f..0af964ec 100644 --- a/oauth2/store/sql/dialect.go +++ b/oauth2/store/sql/dialect.go @@ -30,7 +30,7 @@ type Dialect interface { // 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 are the options of every store transaction. txOptions() *sql.TxOptions } @@ -42,9 +42,9 @@ const ( 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 +// Postgres is the PostgreSQL dialect ($1,$2,… placeholders). Its store +// transactions run at READ COMMITTED whatever the server default: the +// family lock and the consumption of a code rely on re-reading the latest // committed row, where REPEATABLE READ or SERIALIZABLE would abort the // transaction with a serialization failure instead. var Postgres Dialect = postgres{} diff --git a/oauth2/store/sql/doc.go b/oauth2/store/sql/doc.go index bf3ed8d1..35a87fb5 100644 --- a/oauth2/store/sql/doc.go +++ b/oauth2/store/sql/doc.go @@ -13,10 +13,11 @@ // 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 +// - PostgreSQL: the store runs every transaction at READ COMMITTED, +// whatever the server default, so that its locking reads and writes act +// on the latest committed row: SELECT … FOR UPDATE re-reads a family +// row, and the UPDATE consuming a code finds a concurrent consumption +// and reports a replay. 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 @@ -33,16 +34,21 @@ // // # 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 +// The store ships no expiry job: rows stay until an operator deletes them. +// A revocation never deletes tokens, and an exchanged authorization code +// keeps its row, flagged consumed, so that a replay of the code is +// recognized; a code never exchanged stays too. 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"; a code past it can no longer be +// exchanged) 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. +// it. The DDL creates no index — MySQL has no CREATE INDEX IF NOT EXISTS — +// so add one on expires_at through your migration tool: on MySQL, a range +// delete on an unindexed expires_at takes next-key locks that block the +// code and token writes of /authorize and /token. // // 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 diff --git a/oauth2/store/sql/schema.go b/oauth2/store/sql/schema.go index 5819cf73..1a1c15cd 100644 --- a/oauth2/store/sql/schema.go +++ b/oauth2/store/sql/schema.go @@ -10,9 +10,10 @@ import ( ) // 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 +// the store: authorization codes — a consumed one keeps its row, flagged +// consumed, until an operator deletes it — 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 @@ -36,6 +37,7 @@ func Schema(_ Dialect) []string { code_challenge TEXT NOT NULL, code_challenge_method VARCHAR(16) NOT NULL, nonce TEXT NOT NULL, + consumed SMALLINT NOT NULL DEFAULT 0, issued_at BIGINT NOT NULL, expires_at BIGINT NOT NULL )`, @@ -68,7 +70,8 @@ func Schema(_ Dialect) []string { } // Migrate applies [Schema] to the store's database. It is safe to call -// repeatedly (every statement is IF NOT EXISTS). +// repeatedly (every statement is IF NOT EXISTS): it creates the missing +// tables and never alters an existing one. func (s *Store) Migrate(ctx context.Context) error { for _, stmt := range Schema(s.dialect) { if _, err := s.db.ExecContext(ctx, stmt); err != nil { diff --git a/oauth2/store/sql/store.go b/oauth2/store/sql/store.go index 48dcc836..3d98e4d2 100644 --- a/oauth2/store/sql/store.go +++ b/oauth2/store/sql/store.go @@ -16,8 +16,11 @@ import ( // Store is a database/sql-backed [oauth2.Storage]. // -// ConsumeAuthorizationCode runs in a transaction whose DELETE decides the -// winner when two callers race (RowsAffected()==1). +// ConsumeAuthorizationCode runs in a transaction whose first statement, an +// UPDATE flagging the code consumed, decides the winner when two callers +// race (RowsAffected()==1). The consumed row stays, so a replay is +// recognized, until an operator deletes it (see the package +// documentation). // // The token operations implement the token-family contract of // [oauth2.Storage] on one record per family (oauth2_token_families), the @@ -88,18 +91,54 @@ func (s *Store) SaveAuthorizationCode(ctx context.Context, code *oauth2.Authoriz return nil } -// ConsumeAuthorizationCode implements [oauth2.AuthorizationCodeStore]. The -// SELECT + DELETE run in one transaction; the DELETE's RowsAffected() -// decides the winner when two callers race, so the operation is atomic -// without SELECT…FOR UPDATE. +// ConsumeAuthorizationCode implements [oauth2.AuthorizationCodeStore] in +// one transaction that starts with its write: an UPDATE flags the code +// consumed unless it already is, and its RowsAffected() picks the one +// winner when callers race — a concurrent UPDATE waits on the row lock, +// then finds the flag set (SQLite's single writer serializes them +// instead). The record is then read in the same transaction: the winner +// commits and gets it, a loser rolls back and gets an +// [*oauth2.CodeReplayError] carrying it, and a hash without a row is an +// unknown code. func (s *Store) ConsumeAuthorizationCode(ctx context.Context, codeHash string) (*oauth2.AuthorizationCode, error) { - tx, err := s.db.BeginTx(ctx, nil) + var code *oauth2.AuthorizationCode + + err := s.inTx(ctx, func(tx *sql.Tx) error { + res, err := tx.ExecContext(ctx, s.dialect.rebind( + `UPDATE oauth2_auth_codes SET consumed = 1 WHERE code_hash = ? AND consumed = 0`), codeHash) + if err != nil { + return fmt.Errorf("sqlstore: consume authorization code: %w", err) + } + + won, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("sqlstore: rows affected: %w", err) + } + + if code, err = s.selectCode(ctx, tx, codeHash); err != nil { + return err + } + + if won != 1 { + // Consumed before: by an earlier call, or by a concurrent one + // that committed first. + return &oauth2.CodeReplayError{Record: code} + } + + return nil + }) if err != nil { - return nil, fmt.Errorf("sqlstore: begin: %w", err) + return nil, err } - defer func() { _ = tx.Rollback() }() + // Expiry is NOT checked here: the grant compares ExpiresAt with its + // own clock. + return code, nil +} +// selectCode reads the record of the code codeHash within tx. A hash +// without a row is an unknown code: [oauth2.ErrCodeAlreadyUsed]. +func (s *Store) selectCode(ctx context.Context, tx *sql.Tx, codeHash string) (*oauth2.AuthorizationCode, error) { const sel = `SELECT client_id, subject, redirect_uri, scope, code_challenge, code_challenge_method, nonce, issued_at, expires_at FROM oauth2_auth_codes WHERE code_hash = ?` @@ -109,37 +148,17 @@ func (s *Store) ConsumeAuthorizationCode(ctx context.Context, codeHash string) ( issuedAt, expires int64 ) - row := tx.QueryRowContext(ctx, s.dialect.rebind(sel), codeHash) - if err := row.Scan( + err := tx.QueryRowContext(ctx, s.dialect.rebind(sel), codeHash).Scan( &code.ClientID, &code.Subject, &code.RedirectURI, &code.Scope, &code.CodeChallenge, &code.CodeChallengeMethod, &code.Nonce, &issuedAt, &expires, - ); err != nil { - if errors.Is(err, sql.ErrNoRows) { - return nil, oauth2.ErrCodeAlreadyUsed - } - - return nil, fmt.Errorf("sqlstore: select authorization code: %w", err) - } - - res, err := tx.ExecContext(ctx, s.dialect.rebind( - `DELETE FROM oauth2_auth_codes WHERE code_hash = ?`), codeHash) - if err != nil { - return nil, fmt.Errorf("sqlstore: delete authorization code: %w", err) - } - - affected, err := res.RowsAffected() - if err != nil { - return nil, fmt.Errorf("sqlstore: rows affected: %w", err) - } + ) - if affected != 1 { - // A concurrent transaction consumed the code first. + switch { + case errors.Is(err, sql.ErrNoRows): return nil, oauth2.ErrCodeAlreadyUsed - } - - if err := tx.Commit(); err != nil { - return nil, fmt.Errorf("sqlstore: commit: %w", err) + case err != nil: + return nil, fmt.Errorf("sqlstore: select authorization code: %w", err) } code.IssuedAt = time.Unix(issuedAt, 0) diff --git a/oauth2/store/sql/store_code_test.go b/oauth2/store/sql/store_code_test.go new file mode 100644 index 00000000..16b3f1cf --- /dev/null +++ b/oauth2/store/sql/store_code_test.go @@ -0,0 +1,140 @@ +// 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" + "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" +) + +// testCode returns an authorization code stored under hash that expires +// at expiresAt. +func testCode(hash string, expiresAt time.Time) *oauth2.AuthorizationCode { + return &oauth2.AuthorizationCode{ + Code: "raw-" + hash, CodeHash: hash, ClientID: "c", Subject: "s", + RedirectURI: "https://app.example/cb", Scope: "r", + CodeChallenge: "challenge", CodeChallengeMethod: "S256", Nonce: "n", + IssuedAt: expiresAt.Add(-10 * time.Minute), ExpiresAt: expiresAt, + } +} + +// requireCodeReplay asserts that err reports a replay of the code stored +// under hash, carrying its retained record. +func requireCodeReplay(t *testing.T, err error, hash string) { + t.Helper() + + var replay *oauth2.CodeReplayError + + require.ErrorAs(t, err, &replay) + require.NotNil(t, replay.Record) + assert.Equal(t, hash, replay.Record.CodeHash) + assert.Equal(t, "c", replay.Record.ClientID) +} + +// TestSQLConsumedCodeRowIsKept: consuming a code flags its row instead of +// deleting it, so a replay can be recognized; the record stays intact. +func TestSQLConsumedCodeRowIsKept(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + code := testCode("kept", time.Now().Add(10*time.Minute)) + require.NoError(t, store.SaveAuthorizationCode(t.Context(), code)) + + _, err := store.ConsumeAuthorizationCode(t.Context(), "kept") + require.NoError(t, err) + + require.Equal(t, 1, count(t, db, "SELECT COUNT(*) FROM oauth2_auth_codes WHERE code_hash = ?", "kept"), + "the consumed code is retained") + + var ( + consumed int + clientID string + challenge string + expiresAt int64 + ) + + require.NoError(t, db.QueryRowContext(t.Context(), + "SELECT consumed, client_id, code_challenge, expires_at FROM oauth2_auth_codes WHERE code_hash = ?", "kept", + ).Scan(&consumed, &clientID, &challenge, &expiresAt)) + assert.Equal(t, 1, consumed) + assert.Equal(t, "c", clientID) + assert.Equal(t, "challenge", challenge) + assert.Equal(t, code.ExpiresAt.Unix(), expiresAt) +} + +// TestSQLCodeCleanupRule runs the cleanup the package documentation +// describes: it drops the codes past expires_at, whose replays then read as +// unknown codes, and keeps the others recognizable. +func TestSQLCodeCleanupRule(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + now := time.Now() + + require.NoError(t, store.SaveAuthorizationCode(t.Context(), testCode("short", now.Add(10*time.Minute)))) + require.NoError(t, store.SaveAuthorizationCode(t.Context(), testCode("long", now.Add(2*time.Hour)))) + + for _, hash := range []string{"short", "long"} { + _, err := store.ConsumeAuthorizationCode(t.Context(), hash) + require.NoError(t, err) + } + + // The cleanup, run an hour later. + _, err := db.ExecContext(t.Context(), "DELETE FROM oauth2_auth_codes WHERE expires_at < ?", now.Add(time.Hour).Unix()) + require.NoError(t, err) + + got, err := store.ConsumeAuthorizationCode(t.Context(), "short") + assert.Nil(t, got) + require.ErrorIs(t, err, oauth2.ErrCodeAlreadyUsed) + + var replay *oauth2.CodeReplayError + + assert.NotErrorAs(t, err, &replay, "a deleted code reads as unknown") + + got, err = store.ConsumeAuthorizationCode(t.Context(), "long") + assert.Nil(t, got) + requireCodeReplay(t, err, "long") +} + +// TestSQLConsumeReportsSelectError covers a consume whose read fails after +// its write: the error is reported, no code is returned, and the +// transaction leaves the code unconsumed. +func TestSQLConsumeReportsSelectError(t *testing.T) { + t.Parallel() + + db, err := sql.Open("sqlite", ":memory:") + require.NoError(t, err) + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + + ctx := context.Background() + + // A table with only the columns the write needs: the read that follows + // it fails. + _, err = db.ExecContext(ctx, "CREATE TABLE oauth2_auth_codes (code_hash VARCHAR(128) PRIMARY KEY, consumed SMALLINT NOT NULL DEFAULT 0)") + require.NoError(t, err) + _, err = db.ExecContext(ctx, "INSERT INTO oauth2_auth_codes (code_hash) VALUES ('h')") + require.NoError(t, err) + + store, err := sqlstore.New(db, sqlstore.SQLite) + require.NoError(t, err) + + got, err := store.ConsumeAuthorizationCode(ctx, "h") + assert.Nil(t, got) + require.ErrorContains(t, err, "sqlstore: select authorization code") + + var consumed int + + require.NoError(t, db.QueryRowContext(ctx, "SELECT consumed FROM oauth2_auth_codes WHERE code_hash = 'h'").Scan(&consumed)) + assert.Equal(t, 0, consumed, "the failed consume rolled back") +} diff --git a/oauth2/storetest/code_replay.go b/oauth2/storetest/code_replay.go new file mode 100644 index 00000000..a269f161 --- /dev/null +++ b/oauth2/storetest/code_replay.go @@ -0,0 +1,85 @@ +// 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 ( + "net/url" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" + "github.com/hyperscale-stack/security/oauth2/grant" + "github.com/hyperscale-stack/security/oauth2/pkce" + "github.com/hyperscale-stack/security/oauth2/token" +) + +// The RFC 7636 Appendix B PKCE pair the replay case redeems codes with. +const ( + replayVerifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" + replayChallenge = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" +) + +// redeemCode saves the authorization code raw — S256 PKCE, issued to the +// test client — into s, redeems it through g, and returns the /token +// request it sent and the pair the exchange issued. +func redeemCode(t *testing.T, s oauth2.Storage, g *grant.AuthorizationCode, raw string) (grant.Request, *oauth2.TokenPair) { + t.Helper() + + now := time.Now() + + mustNoError(t, s.SaveAuthorizationCode(ctx(), &oauth2.AuthorizationCode{ + Code: raw, CodeHash: oauth2.HashToken(nil, raw), + ClientID: testClientID, Subject: testSubject, RedirectURI: testRedirectURI, Scope: testScope, + CodeChallenge: replayChallenge, CodeChallengeMethod: string(pkce.MethodS256), + IssuedAt: now, ExpiresAt: now.Add(10 * time.Minute), + }), "save code "+raw) + + req := grant.Request{ + Client: &oauth2.DefaultClient{ + IDValue: testClientID, TypeValue: oauth2.ClientConfidential, + RedirectURIValues: []string{testRedirectURI}, + }, + Form: url.Values{"code": {raw}, "redirect_uri": {testRedirectURI}, "code_verifier": {replayVerifier}}, + Now: now, + Profile: oauth2.Profile20BCP, + } + + resp, err := g.Handle(ctx(), req) + mustNoError(t, err, "exchange code "+raw) + + return req, &resp.Pair +} + +// testCodeReplayRevokesIssuedTokens drives the real authorization_code +// grant over the store. Two codes of the same client are exchanged, then +// the first one is replayed with the same proof: the replay is refused, +// and the tokens the first exchange issued are no longer usable through +// the store (RFC 6749 §4.1.2), while those of the other code and a token +// outside any family are untouched. +func testCodeReplayRevokesIssuedTokens(t *testing.T, s oauth2.Storage) { + g := grant.NewAuthorizationCode(grant.Config{ + Storage: s, + AccessTokens: token.NewOpaque(32), + RefreshTokens: token.OpaqueRefreshAdapter{Opaque: token.NewOpaque(32)}, + AccessTTL: time.Hour, + RefreshTTL: 24 * time.Hour, + }) + + replayed, first := redeemCode(t, s, g, "replay-a") + _, other := redeemCode(t, s, g, "replay-b") + + cc := accessOnly("replay-cc", "") + mustSave(t, s, cc) + + requireUsable(t, s, first) + + resp, err := g.Handle(ctx(), replayed) + mustEqual(t, resp == nil, true, "replay: no tokens issued") + mustErrorIs(t, err, oauth2.ErrCodeAlreadyUsed, "replay") + + requireUnusable(t, s, first) + requireUsable(t, s, other) + requireUsable(t, s, cc) +} diff --git a/oauth2/storetest/conformance.go b/oauth2/storetest/conformance.go index 8e47d3c0..4c7a965b 100644 --- a/oauth2/storetest/conformance.go +++ b/oauth2/storetest/conformance.go @@ -34,12 +34,15 @@ type Factory func() oauth2.Storage // storetest.RunConformance(t, func() oauth2.Storage { return New(...) }) // } // -// Besides single-use codes and atomic rotation, it checks the token-family +// Besides single-use codes — retained once consumed, so a replay is told +// from an unknown code — 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). +// through the public API only, and drives the real authorization_code +// grant to check that a replayed code revokes the tokens it issued. 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 +54,7 @@ func RunConformance(t *testing.T, newStore Factory) { {"AuthorizationCodeSingleUse", testCodeSingleUse}, {"AuthorizationCodeUnknown", testCodeUnknown}, {"AuthorizationCodeConcurrentConsume", testCodeConcurrentConsume}, + {"AuthorizationCodeReplayRevokesIssuedTokens", testCodeReplayRevokesIssuedTokens}, {"AccessTokenSaveLookupRevoke", testAccessLifecycle}, {"AccessTokenLookupUnknown", testAccessUnknown}, {"RefreshTokenSaveLookup", testRefreshSaveLookup}, @@ -88,6 +92,9 @@ const testClientID = "client-1" // testScope is the fixed scope used across the suite. const testScope = "read" +// testRedirectURI is the fixed redirect URI of the suite's codes. +const testRedirectURI = "https://app.example/cb" + func ctx() context.Context { return context.Background() } // The assertion helpers below hold every t.Fatalf of the suite, so the @@ -143,67 +150,137 @@ func sampleCode(hash string) *oauth2.AuthorizationCode { now := time.Now() return &oauth2.AuthorizationCode{ - Code: "raw-" + hash, - CodeHash: hash, - ClientID: testClientID, - Subject: testSubject, - RedirectURI: "https://app.example/cb", - Scope: testScope, - IssuedAt: now, - ExpiresAt: now.Add(10 * time.Minute), + Code: "raw-" + hash, + CodeHash: hash, + ClientID: testClientID, + Subject: testSubject, + RedirectURI: testRedirectURI, + Scope: testScope, + CodeChallenge: "challenge-" + hash, + CodeChallengeMethod: "S256", + Nonce: "nonce-" + hash, + IssuedAt: now, + ExpiresAt: now.Add(10 * time.Minute), } } +// storedCode is code as a store returns it: without the raw value, with +// its timestamps at the second. +func storedCode(code oauth2.AuthorizationCode) oauth2.AuthorizationCode { + code.Code = "" + code.IssuedAt = time.Unix(code.IssuedAt.Unix(), 0) + code.ExpiresAt = time.Unix(code.ExpiresAt.Unix(), 0) + + return code +} + +// requireRecord asserts that got is the record of saved: every field a +// store keeps, and never the raw code. +func requireRecord(t *testing.T, got, saved *oauth2.AuthorizationCode, msg string) { + t.Helper() + + mustEqual(t, got != nil, true, msg+": a record") + mustEqual(t, got.Code, "", msg+": the raw code is never kept") + mustEqual(t, storedCode(*got), storedCode(*saved), msg) +} + +// requireReplay asserts that got and err report a replay of saved: no +// code to issue tokens from, and an invalid_grant matching +// oauth2.ErrCodeAlreadyUsed that carries the retained record. +func requireReplay(t *testing.T, got *oauth2.AuthorizationCode, err error, saved *oauth2.AuthorizationCode, msg string) { + t.Helper() + + mustEqual(t, got == nil, true, msg+": no code returned") + mustErrorIs(t, err, oauth2.ErrCodeAlreadyUsed, msg) + mustCode(t, err, oauth2.CodeInvalidGrant, msg) + + var replay *oauth2.CodeReplayError + + mustEqual(t, errors.As(err, &replay), true, msg+": a CodeReplayError") + requireRecord(t, replay.Record, saved, msg+": the retained record") +} + func testCodeSaveConsume(t *testing.T, s oauth2.Storage) { - mustNoError(t, s.SaveAuthorizationCode(ctx(), sampleCode("code-1")), "save") + mustCode(t, s.SaveAuthorizationCode(ctx(), &oauth2.AuthorizationCode{}), oauth2.CodeInvalidRequest, "save a code without hash") + + saved := sampleCode("code-1") + mustNoError(t, s.SaveAuthorizationCode(ctx(), saved), "save") got, err := s.ConsumeAuthorizationCode(ctx(), "code-1") mustNoError(t, err, "consume") - 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") + requireRecord(t, got, saved, "consumed code") } +// testCodeSingleUse replays a consumed code: every replay is refused with +// a CodeReplayError carrying the retained record, so the server can tell +// it from an unknown code and revoke what the first exchange issued. func testCodeSingleUse(t *testing.T, s oauth2.Storage) { - mustNoError(t, s.SaveAuthorizationCode(ctx(), sampleCode("code-2")), "save") + saved := sampleCode("code-2") + mustNoError(t, s.SaveAuthorizationCode(ctx(), saved), "save") _, err := s.ConsumeAuthorizationCode(ctx(), "code-2") mustNoError(t, err, "first consume") - _, err = s.ConsumeAuthorizationCode(ctx(), "code-2") - mustCode(t, err, oauth2.CodeInvalidGrant, "second consume (single-use)") + for _, attempt := range []string{"second consume", "third consume"} { + got, err := s.ConsumeAuthorizationCode(ctx(), "code-2") + requireReplay(t, got, err, saved, attempt) + } } +// testCodeUnknown consumes a code never saved: an invalid_grant matching +// oauth2.ErrCodeAlreadyUsed, but no replay — there is nothing to revoke. func testCodeUnknown(t *testing.T, s oauth2.Storage) { - _, err := s.ConsumeAuthorizationCode(ctx(), "never-saved") + got, err := s.ConsumeAuthorizationCode(ctx(), "never-saved") + mustEqual(t, got == nil, true, "consume an unknown code: no code returned") + mustErrorIs(t, err, oauth2.ErrCodeAlreadyUsed, "consume an unknown code") mustCode(t, err, oauth2.CodeInvalidGrant, "consume an unknown code") + + var replay *oauth2.CodeReplayError + + mustEqual(t, errors.As(err, &replay), false, "an unknown code is no replay") } // testCodeConcurrentConsume asserts the single-use guarantee under -// concurrency: 50 goroutines race to consume one code, exactly one wins. +// concurrency: 50 goroutines race to consume one code, exactly one wins, +// and every other one sees a replay of the code. func testCodeConcurrentConsume(t *testing.T, s oauth2.Storage) { - mustNoError(t, s.SaveAuthorizationCode(ctx(), sampleCode("code-race")), "save") + saved := sampleCode("code-race") + mustNoError(t, s.SaveAuthorizationCode(ctx(), saved), "save") const n = 50 - var ( - wg sync.WaitGroup - mu sync.Mutex - wins int - ) + codes := make([]*oauth2.AuthorizationCode, n) + errs := make([]error, n) + + var wg sync.WaitGroup - for range n { + start := make(chan struct{}) + + for i := range n { wg.Go(func() { - if _, err := s.ConsumeAuthorizationCode(ctx(), "code-race"); err == nil { - mu.Lock() - wins++ - mu.Unlock() - } + <-start + + codes[i], errs[i] = s.ConsumeAuthorizationCode(ctx(), "code-race") }) } + close(start) wg.Wait() + wins := 0 + + for i, err := range errs { + if err == nil { + wins++ + + requireRecord(t, codes[i], saved, "winning consume") + + continue + } + + requireReplay(t, codes[i], err, saved, "losing consume") + } + mustEqual(t, wins, 1, "successful consumes") }