From da59f6cac9f0d82b57ed6d89914912dcd1fa5fa1 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Sat, 10 Oct 2026 03:34:22 +0200 Subject: [PATCH 1/3] fix(oauth2): retain consumed authorization codes until they expire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every store deleted an authorization code as it consumed it. A replay then looked exactly like an unknown code, and nothing was left to tell the server which client the code was issued to or what its first exchange had issued, although RFC 6749 §4.1.2 says the tokens issued from a replayed code SHOULD be revoked. The memory store also kept the raw code in its record, which the model says no store may persist. Consuming a code now flags it instead of deleting it. The record stays, without the raw code, at least until the code expires, and every later consumption gets nil and an oauth2.CodeReplayError carrying it: a new error type that reads and matches as ErrCodeAlreadyUsed, so callers that test err first, IsCode and the wire response are unchanged. An unknown code still gets the bare ErrCodeAlreadyUsed, and exactly one caller gets the record with a nil error, as before. The SQL store flags the row write-first in one transaction (inTx, so READ COMMITTED on PostgreSQL): an UPDATE ... WHERE consumed = 0 picks the winner through RowsAffected, the record is then read in the same transaction, and losers roll back. oauth2_auth_codes gains a consumed SMALLINT column, which Migrate does not add to an existing table. The Redis script flags the code's JSON with SET ... KEEPTTL, so the record expires with the code, and records a per-call nonce: a script go-redis resends after a lost reply finds its own mark and still wins, where the former GET+DEL script made such a resend fail the exchange with invalid_grant. The script compares the flag with true because cjson decodes a JSON null as a truthy value. The storage contract, the SQL cleanup guidance (consumed and abandoned codes stay until an operator deletes the rows past expires_at, which deserves an index), the PostgreSQL READ COMMITTED requirement, which now covers code consumption, and the memory store's growth are documented; MIGRATION.md lists what custom stores, callers, SQL deployments and rolling upgrades need. The grant still refuses a replay without revoking anything: that comes next. Refs #80. --- CHANGELOG.md | 17 +++ MIGRATION.md | 46 ++++++++ docs/security-considerations.md | 2 +- oauth2/code_replay_error_test.go | 59 ++++++++++ oauth2/errors.go | 25 +++- oauth2/models.go | 10 +- oauth2/storage.go | 36 ++++-- oauth2/storage/memory/memory.go | 62 +++++----- oauth2/store/redis/codec.go | 22 +++- oauth2/store/redis/doc.go | 19 ++-- oauth2/store/redis/store.go | 44 ++++++-- oauth2/store/redis/store_code_test.go | 157 ++++++++++++++++++++++++++ oauth2/store/sql/dialect.go | 8 +- oauth2/store/sql/doc.go | 32 +++--- oauth2/store/sql/schema.go | 11 +- oauth2/store/sql/store.go | 89 +++++++++------ oauth2/store/sql/store_code_test.go | 140 +++++++++++++++++++++++ oauth2/storetest/conformance.go | 128 ++++++++++++++++----- 18 files changed, 759 insertions(+), 148 deletions(-) create mode 100644 oauth2/code_replay_error_test.go create mode 100644 oauth2/store/redis/store_code_test.go create mode 100644 oauth2/store/sql/store_code_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dad9d01..38ab3641 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 diff --git a/MIGRATION.md b/MIGRATION.md index c0ba4c33..d49200b7 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -227,6 +227,52 @@ 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`; wrapping it with + `%w` keeps the replay record reachable through `errors.As`. +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/security-considerations.md b/docs/security-considerations.md index a20c5c6e..72d9c935 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -181,7 +181,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 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/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/models.go b/oauth2/models.go index 6536f20e..17bb5da0 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 + OAuth 2.0 BCP §4.5; 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..54a360b0 100644 --- a/oauth2/storage.go +++ b/oauth2/storage.go @@ -22,19 +22,35 @@ 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. The issuer + // saves each code once, under a fresh hash; an empty hash is rejected + // with invalid_request. 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). Expiry is + // not checked: the caller compares ExpiresAt with its own clock. ConsumeAuthorizationCode(ctx context.Context, codeHash string) (*AuthorizationCode, error) } 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..f9a9ee5a 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 `) @@ -184,9 +199,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 +221,21 @@ func (s *Store) ConsumeAuthorizationCode(ctx context.Context, codeHash string) ( str, ok := res.(string) if !ok { - // The script returned false: key absent / already consumed. + // The script returned false: the key is absent. 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..a6034c68 --- /dev/null +++ b/oauth2/store/redis/store_code_test.go @@ -0,0 +1,157 @@ +// 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, +// which cjson decodes as a truthy value: both are unconsumed codes. +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/conformance.go b/oauth2/storetest/conformance.go index 8e47d3c0..761798a6 100644 --- a/oauth2/storetest/conformance.go +++ b/oauth2/storetest/conformance.go @@ -143,67 +143,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: "https://app.example/cb", + 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) - for range n { + var wg sync.WaitGroup + + 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") } From c50326e94d447ab3724b8682d4164126f879dfbb Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Sat, 10 Oct 2026 03:48:17 +0200 Subject: [PATCH 2/3] fix(oauth2): revoke the tokens issued from a replayed authorization code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 6749 §4.1.2 says the tokens previously issued from a replayed authorization code SHOULD be revoked, and RFC 9700 §4.2.4 relies on it against stolen codes. The grant refused a replay but left the tokens of the first exchange alive: an attacker who redeemed an intercepted code first kept an access token and a refresh token it could rotate at will. Nothing linked a code to its tokens either: issuance picked a random token family after the code was consumed. The authorization_code grant now derives the token family from the code hash — a domain-separated SHA-256, 22 characters like a random family ID — before consuming the code, so the first exchange and every replay agree on it without storing the link. A replay the store reports with its retained record goes through the validation an exchange goes through (expiry, client, redirect_uri, PKCE, grant type, in their existing order). Only when it passes, i.e. when the request would itself have been accepted (OAuth 2.1 §4.1.3, §7.5.3), does the grant revoke the family, on a context detached from the request; a failure is reported to grant.Config.OnError ("revoke token family failed after authorization code replay"), and the next proven replay retries it. A public client_id is no secret, so unknown codes, other clients and missing, wrong or malformed verifiers revoke nothing. The replay still answers the bare ErrCodeAlreadyUsed. Revoking the family revokes every token the code issued and every later rotation. Since the family is known before the code is consumed, a proven replay landing between the first exchange's consumption and its issuance revokes the family first, and the family gate of SaveTokenPair then makes that exchange fail with invalid_grant ("token family revoked") instead of issuing — as long as the store keeps the revoked family record. The legacy password grant, which has no code, keeps a random family. storetest drives the real grant over every backend: a replayed code's tokens become unusable, while another code's tokens and a family-less token stay usable. The storage, grant and package docs, the security considerations (with the offline JWT caveat and the deployment requirements the race guarantee rests on), observability, architecture, LIMITATIONS and the example README describe the behavior. Refs #80. --- CHANGELOG.md | 17 + LIMITATIONS.md | 9 +- docs/architecture.md | 20 +- docs/observability.md | 21 +- docs/security-considerations.md | 45 +- examples/oauth2/README.md | 3 + oauth2/code_replay_test.go | 249 ++++++++ oauth2/doc.go | 15 +- oauth2/grant/authorization_code.go | 107 +++- .../grant/authorization_code_replay_test.go | 541 ++++++++++++++++++ oauth2/grant/grant.go | 8 +- oauth2/grant/issue.go | 18 +- oauth2/grant/legacy_password.go | 7 +- oauth2/storage.go | 7 + oauth2/storetest/code_replay.go | 85 +++ oauth2/storetest/conformance.go | 17 +- 16 files changed, 1101 insertions(+), 68 deletions(-) create mode 100644 oauth2/code_replay_test.go create mode 100644 oauth2/grant/authorization_code_replay_test.go create mode 100644 oauth2/storetest/code_replay.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 38ab3641..2aae1f51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -293,3 +293,20 @@ 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 makes that exchange fail with `invalid_grant` + instead of issuing, as long as the store keeps the revoked family record + (see the deployment requirements in + [docs/security-considerations.md](docs/security-considerations.md)). 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/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 72d9c935..1b89e950 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -88,6 +88,35 @@ 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 revokes the family before that exchange issues: the + exchange answers `invalid_grant` and issues nothing. That race guarantee + 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 as expired 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 @@ -196,7 +225,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 +289,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_test.go b/oauth2/code_replay_test.go new file mode 100644 index 00000000..5539103e --- /dev/null +++ b/oauth2/code_replay_test.go @@ -0,0 +1,249 @@ +// 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 is refused 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) + assert.Equal(t, oauth2.CodeInvalidGrant, decodeError(t, rec)) + 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) //nolint:wrapcheck // test double +} + +// 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/grant/authorization_code.go b/oauth2/grant/authorization_code.go index c8ae6082..a4f1f6a6 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,77 @@ 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) + + var replay *oauth2.CodeReplayError + if errors.As(err, &replay) && 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 +194,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..55de5c65 --- /dev/null +++ b/oauth2/grant/authorization_code_replay_test.go @@ -0,0 +1,541 @@ +// 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 and can fail them, honors the context it +// gets 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 +} + +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 //nolint:wrapcheck // test double +} + +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) //nolint:wrapcheck // test double +} + +func (s *replayStore) RevokeRefreshFamily(ctx context.Context, familyID string) error { + s.revokes.Add(1) + + if err := ctx.Err(); err != nil { + return err + } + + if s.revokeErr != nil { + return s.revokeErr + } + + return s.Store.RevokeRefreshFamily(ctx, familyID) //nolint:wrapcheck // test double +} + +// 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 is refused and +// revokes nothing (OAuth 2.1 §4.1.3, §7.5.3). 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.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + + 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: the replay cannot be +// validated, so the grant refuses it like an unknown code and revokes +// nothing. +func TestAuthorizationCodeReplayWithoutRecordRevokesNothing(t *testing.T) { + t.Parallel() + + store := &replayStore{Store: memory.New(), consumeErr: &oauth2.CodeReplayError{}} + 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. +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()) + 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/storage.go b/oauth2/storage.go index 54a360b0..d0470f3a 100644 --- a/oauth2/storage.go +++ b/oauth2/storage.go @@ -169,6 +169,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/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 761798a6..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 @@ -147,7 +154,7 @@ func sampleCode(hash string) *oauth2.AuthorizationCode { CodeHash: hash, ClientID: testClientID, Subject: testSubject, - RedirectURI: "https://app.example/cb", + RedirectURI: testRedirectURI, Scope: testScope, CodeChallenge: "challenge-" + hash, CodeChallengeMethod: "S256", From 8b032ff451f786c2d32a5c8ff296d76a09f81729 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Sat, 10 Oct 2026 04:22:29 +0200 Subject: [PATCH 3/3] test(oauth2): pin the replay wire answer and tighten the code-replay contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups for the authorization-code replay revocation. An unproven replay — another client, a missing, wrong or malformed verifier, another redirect_uri, an expired code, a client no longer allowed the grant — now has its answer pinned to the bare ErrCodeAlreadyUsed ("authorization code already consumed"), the answer of any replay and of an unknown code, so the response tells nothing about which check failed. The former pins only checked the invalid_grant code, which a grant returning the validation error would have passed. The revocation a replay triggers is now checked to be bounded by the detached revocation timeout, not only detached from the request, and the replay guard also refuses a typed-nil CodeReplayError, which used to panic. The storage contract now says that a given CodeHash is saved at most once — the Redis store overwrites, so a consumed code saved again becomes redeemable again — and that a decorator of ConsumeAuthorizationCode returns the store's error unchanged or wrapped with %w, since normalizing it to ErrCodeAlreadyUsed silently disables replay revocation. The docs describe both outcomes of a replay racing the first exchange — it fails that exchange or revokes its tokens — say that a late replay is refused like any replay, and cite RFC 9700 §4.2.4. The Redis comments explain what the null-flag test checks and the non-string script reply, and the test doubles lose their dead nolint directives. Refs #80. --- CHANGELOG.md | 26 +++--- MIGRATION.md | 6 +- docs/security-considerations.md | 57 +++++++------ oauth2/code_replay_test.go | 14 +-- oauth2/grant/authorization_code.go | 4 +- .../grant/authorization_code_replay_test.go | 85 +++++++++++++------ oauth2/models.go | 2 +- oauth2/storage.go | 15 ++-- oauth2/store/redis/store.go | 8 +- oauth2/store/redis/store_code_test.go | 7 +- 10 files changed, 142 insertions(+), 82 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2aae1f51..51eb34e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -297,16 +297,18 @@ legacy packages (`authentication/`, `authorization/`, the in-tree 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 makes that exchange fail with `invalid_grant` - instead of issuing, as long as the store keeps the revoked family record + 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)). 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. + [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/MIGRATION.md b/MIGRATION.md index d49200b7..76ccf991 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -251,8 +251,10 @@ To migrate: 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`; wrapping it with - `%w` keeps the replay record reachable through `errors.As`. + 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 diff --git a/docs/security-considerations.md b/docs/security-considerations.md index 1b89e950..0cd2f79a 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -88,35 +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 revokes the family before that exchange issues: the - exchange answers `invalid_grant` and issues nothing. That race guarantee - 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 as expired and revokes nothing. A failed +- **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). + 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 diff --git a/oauth2/code_replay_test.go b/oauth2/code_replay_test.go index 5539103e..dbba76c2 100644 --- a/oauth2/code_replay_test.go +++ b/oauth2/code_replay_test.go @@ -148,9 +148,10 @@ func TestTokenEndpointCodeReplayRevokesIssuedTokens(t *testing.T) { // TestTokenEndpointUnprovenCodeReplayRevokesNothing replays a public // client's consumed code with requests that could not have redeemed it: -// each one is refused 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. +// 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() @@ -175,7 +176,10 @@ func TestTokenEndpointUnprovenCodeReplayRevokesNothing(t *testing.T) { t.Run(tc.name, func(t *testing.T) { rec := serve(srv.TokenHandler(), tc.req) assert.Equal(t, http.StatusBadRequest, rec.Code) - assert.Equal(t, oauth2.CodeInvalidGrant, decodeError(t, rec)) + + 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"]) }) } @@ -203,7 +207,7 @@ func (s *codeRaceStore) SaveTokenPair(ctx context.Context, pair *oauth2.TokenPai hook() } - return s.Store.SaveTokenPair(ctx, pair) //nolint:wrapcheck // test double + return s.Store.SaveTokenPair(ctx, pair) } // TestTokenEndpointCodeExchangeLosingReplayRaceIsInvalidGrant serves a diff --git a/oauth2/grant/authorization_code.go b/oauth2/grant/authorization_code.go index a4f1f6a6..5f968130 100644 --- a/oauth2/grant/authorization_code.go +++ b/oauth2/grant/authorization_code.go @@ -70,8 +70,10 @@ func (g *AuthorizationCode) Handle(ctx context.Context, req Request) (*Response, 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.Record != nil { + if errors.As(err, &replay) && replay != nil && replay.Record != nil { g.revokeReplayed(ctx, req, replay.Record, familyID) return nil, oauth2.ErrCodeAlreadyUsed diff --git a/oauth2/grant/authorization_code_replay_test.go b/oauth2/grant/authorization_code_replay_test.go index 55de5c65..8a4aa373 100644 --- a/oauth2/grant/authorization_code_replay_test.go +++ b/oauth2/grant/authorization_code_replay_test.go @@ -23,12 +23,12 @@ import ( ) // replayStore decorates the memory store for the code-replay tests. It -// counts the family revocations and can fail them, honors the context it -// gets 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. +// 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 @@ -40,6 +40,7 @@ type replayStore struct { 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) { @@ -53,7 +54,7 @@ func (s *replayStore) ConsumeAuthorizationCode(ctx context.Context, hash string) s.afterConsume() } - return code, err //nolint:wrapcheck // test double + return code, err } func (s *replayStore) SaveTokenPair(ctx context.Context, pair *oauth2.TokenPair) error { @@ -67,12 +68,18 @@ func (s *replayStore) SaveTokenPair(ctx context.Context, pair *oauth2.TokenPair) hook() } - return s.Store.SaveTokenPair(ctx, pair) //nolint:wrapcheck // test double + 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 } @@ -81,7 +88,16 @@ func (s *replayStore) RevokeRefreshFamily(ctx context.Context, familyID string) return s.revokeErr } - return s.Store.RevokeRefreshFamily(ctx, familyID) //nolint:wrapcheck // test double + 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. @@ -288,9 +304,10 @@ func TestAuthorizationCodeReplayRevokesAccessOnlyIssuance(t *testing.T) { // 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 is refused and -// revokes nothing (OAuth 2.1 §4.1.3, §7.5.3). A request that would have -// been accepted revokes. +// 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() @@ -338,7 +355,7 @@ func TestAuthorizationCodeUnprovenReplayRevokesNothing(t *testing.T) { t.Run(tc.name, func(t *testing.T) { resp, err := g.Handle(t.Context(), tc.replay(req)) assert.Nil(t, resp) - assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + assert.Same(t, oauth2.ErrCodeAlreadyUsed, err, "the answer of any replay: no oracle") assert.Zero(t, store.revokes.Load()) assert.Empty(t, hooks.errs) @@ -354,20 +371,34 @@ func TestAuthorizationCodeUnprovenReplayRevokesNothing(t *testing.T) { } // TestAuthorizationCodeReplayWithoutRecordRevokesNothing covers a store -// reporting a replay without the retained record: the replay cannot be -// validated, so the grant refuses it like an unknown code and revokes -// nothing. +// 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() - store := &replayStore{Store: memory.New(), consumeErr: &oauth2.CodeReplayError{}} - g := grant.NewAuthorizationCode(rotatingConfig(store, &hookRecorder{})) + cases := []struct { + name string + err error + }{ + {"no record", &oauth2.CodeReplayError{}}, + {"typed nil", (*oauth2.CodeReplayError)(nil)}, + } - 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()) + 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 @@ -413,7 +444,8 @@ func TestAuthorizationCodeReplayRevocationFailureIsReported(t *testing.T) { // 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. +// 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() @@ -433,6 +465,11 @@ func TestAuthorizationCodeReplayRevocationDetachedFromRequest(t *testing.T) { 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) } diff --git a/oauth2/models.go b/oauth2/models.go index 17bb5da0..5212136b 100644 --- a/oauth2/models.go +++ b/oauth2/models.go @@ -11,7 +11,7 @@ import "time" // 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 + OAuth 2.0 BCP §4.5; see [AuthorizationCodeStore]). +// 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 diff --git a/oauth2/storage.go b/oauth2/storage.go index d0470f3a..144d7388 100644 --- a/oauth2/storage.go +++ b/oauth2/storage.go @@ -32,9 +32,11 @@ const MaxFamilyIDLength = 64 // 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 code under code.CodeHash. The issuer - // saves each code once, under a fresh hash; an empty hash is rejected - // with invalid_request. + // 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 marks the code codeHash consumed @@ -49,8 +51,11 @@ type AuthorizationCodeStore interface { // no longer retained; // - a wrapped error on a backend failure. // - // Both refusals match [ErrCodeAlreadyUsed] (invalid_grant). Expiry is - // not checked: the caller compares ExpiresAt with its own clock. + // 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) } diff --git a/oauth2/store/redis/store.go b/oauth2/store/redis/store.go index f9a9ee5a..d379984f 100644 --- a/oauth2/store/redis/store.go +++ b/oauth2/store/redis/store.go @@ -180,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") @@ -221,7 +224,8 @@ func (s *Store) ConsumeAuthorizationCode(ctx context.Context, codeHash string) ( str, ok := res.(string) if !ok { - // The script returned false: the key is absent. + // Any other reply type: treat as an unknown code (a Lua false + // arrives as redis.Nil, handled above). return nil, oauth2.ErrCodeAlreadyUsed } diff --git a/oauth2/store/redis/store_code_test.go b/oauth2/store/redis/store_code_test.go index a6034c68..15da131f 100644 --- a/oauth2/store/redis/store_code_test.go +++ b/oauth2/store/redis/store_code_test.go @@ -81,8 +81,11 @@ func TestRedisConsumedCodeKeepsItsTTL(t *testing.T) { } // TestRedisCodePayloadWithoutFlagIsConsumable covers payloads without a -// consumed flag — written before the flag existed — and with a null one, -// which cjson decodes as a truthy value: both are unconsumed codes. +// 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()