diff --git a/CHANGELOG.md b/CHANGELOG.md index cd354d78..68cd634f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -579,6 +579,29 @@ legacy packages (`authentication/`, `authorization/`, the in-tree loopback-issuer setting to both configurations, sets `AdvertiseJWKSURI` and gives the server its OpenID Connect scopes followed by the deployment's own. +- `CheckDeployment(ctx)` and `ErrDeploymentRequirement` on + `oauth2/store/sql`, `oidc/client/store/sql`, `saml/sp/store/sql` and + `oauth2/store/redis`: the deployment requirements of each package + documentation that the server reports — on MySQL, InnoDB tables, binary + key columns and a strict `sql_mode` (oauth2); on Redis, version 6.0 or + later, `maxmemory-policy noeviction` and no cluster mode (through `INFO`, + which needs `+info` in a restricted ACL). Every requirement not met is + reported; a server error (NOPERM) is returned as such. Call it at boot + when the schema is applied by your own migration tool. +- `PurgeExpired(ctx, now)` on `oauth2/store/sql` and `saml/sp/store/sql`: + deletes the rows expired before `now` in batches of 1000, each a READ + COMMITTED transaction (PostgreSQL, MySQL), oauth2 families last, never a + family a concurrent revocation extends. Add the `expires_at` indexes the + package documentation lists. +- `oidcclient.ValidateStoreKey`, the check every `TransactionStore.Consume` + runs on its keys before any I/O (64 lowercase hex digits). +- `saml/sp/store/redis.WithClock`: the clock the store measures its key + lifetimes on (give it `samlsp.Config.Now` when that one is injected). +- `storetest.RunConformance` (oauth2) compares the full token records of + every lookup and rechecks, 1.5 s later, a consumed code, a rotated refresh + token and revoked families (`TokenRecordsRoundTrip`, `RetainedOverTime`); + the OIDC suite checks that `Consume` refuses malformed keys + (`ConsumeRefusesInvalidKeys`). ### Changed @@ -1148,6 +1171,26 @@ legacy packages (`authentication/`, `authorization/`, the in-tree and validates its configured redirect URI with `ValidateRedirectURI` at startup, and `e2e/oidc` runs the relying party on `127.0.0.1` and the provider on `localhost`. +- **The SQL stores' `Migrate` checks the deployment (breaking on + non-compliant MySQL deployments)**: it still only creates missing tables, + then runs `CheckDeployment` and fails with `ErrDeploymentRequirement` on + a MyISAM or missing table, on `VARCHAR` key columns of the oauth2 store + and on a non-strict `sql_mode` for the oauth2 store. The MySQL DDL names + `ENGINE=InnoDB DEFAULT CHARSET=utf8mb4`. On MySQL and MariaDB, `Migrate` + creates the tables and checks them under a named lock per store module + (`GET_LOCK`, released before it returns, waited for as long as its context + allows), like the advisory lock it takes on PostgreSQL: concurrent + `CREATE TABLE IF NOT EXISTS` of one table could fail with a metadata-lock + deadlock. See [MIGRATION.md](MIGRATION.md). +- **`oauth2/store/redis.New` refuses a `*redis.ClusterClient` or a + `*redis.Ring`** (breaking): the store's multi-key scripts cannot work + with their sharding; Cluster clients used to fail at runtime (CROSSSLOT) + and Ring clients intermittently. See [MIGRATION.md](MIGRATION.md). +- `oidc/client/store/sql.PurgeExpired` runs its DELETE in a READ COMMITTED + transaction on PostgreSQL and MySQL: at MySQL's default isolation an + unindexed purge locked the gaps it scanned, blocking the logins starting + meanwhile. A MySQL server logging statements (`binlog_format=STATEMENT`) + now refuses it: use ROW or MIXED. ### Fixed @@ -1436,6 +1479,15 @@ legacy packages (`authentication/`, `authorization/`, the in-tree `unsupported_response_type`; RFC 6749 §4.1.1 makes the parameter REQUIRED, and §4.1.2.1 answers a missing one with `invalid_request` — an empty occurrence included (§3.1). +- `oidcclient.TransactionStore.Consume` validates its keys in the memory, + SQL and Redis stores, as the interface documents: on MySQL a malformed + state hash — upper-case, space-padded — matched the canonical one under + the column's collation. +- Test gaps of the stores: the real-backend oauth2 conformance runs warm + eight connections before each case, so that the concurrency cases race + for real (a check-then-act code consumption passed on PostgreSQL), and + each locking layer of the SQL token rotation — family lock, token lock, + single-row consumption — has a test of its own. ### Removed @@ -2295,3 +2347,15 @@ legacy packages (`authentication/`, `authorization/`, the in-tree encodings, so a non-canonical encoding (y ≥ p, whose decoding RFC 8032 §5.1.3 makes fail) was used as a key. PS* signatures are now verified with a hash-size salt and such keys are ignored, as in the `jwt` module. +- **SQL stores on MySQL use InnoDB**: the MySQL DDL of the three SQL + stores named no storage engine, so on a server whose default engine is + MyISAM `Migrate` created tables without transactions or row locks. The + oauth2 store then lost the atomicity of its writes (`oauth2.Storage` I3): + a rotation cut short left the presented refresh token consumed without + its replacement, and a consumption acknowledged before a crash could be + lost. The DDL now names InnoDB, and `Migrate` refuses other engines. +- **The SAML Redis store's replay marks no longer depend on Redis's + clock**: it set key expiries as absolute instants computed on the SP's + clock, so with Redis ahead of the SP by more than a mark's retention the + mark was deleted as written and the same assertion accepted again (SAML + 2.0 Profiles §4.1.4.5). Key lifetimes are now sent as durations. diff --git a/LIMITATIONS.md b/LIMITATIONS.md index 4fbf4fac..f3391f04 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -54,12 +54,18 @@ future refactor phases. their lifetime short. See [docs/security-considerations.md](docs/security-considerations.md#offline-jwt-access-tokens). - **Redis Cluster** — not supported by `oauth2/store/redis`: its Lua - scripts touch keys of several hash slots. Use a standalone server or a - Sentinel primary. -- **SQL expired-row cleanup** — `oauth2/store/sql` ships no expiry job: - rows — consumed and abandoned authorization codes included — stay until - an operator deletes those past `expires_at` (see the package - documentation for a safe order). + scripts touch keys of several hash slots (a token, its family, its + replacement), and lookups by token hash cannot place them in one slot. + Use a standalone server or a Sentinel primary: `New` refuses a Cluster or + Ring client, and `Store.CheckDeployment` a server in cluster mode; a + proxy sharding keys behind one endpoint is not detected and fails + closed at runtime. +- **SQL expired-row cleanup is scheduled by you** — `oauth2/store/sql` + runs no job of its own: rows — consumed and abandoned authorization codes + included — stay until `Store.PurgeExpired` deletes those past + `expires_at`. Schedule it, and add the `expires_at` indexes the package + documentation lists: `Schema` creates none (MySQL has no `CREATE INDEX IF + NOT EXISTS`, and building one at boot would block a populated table). - **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 @@ -76,9 +82,20 @@ future refactor phases. - **SQL schema upgrades are manual** — `Store.Migrate` creates missing tables and never alters an existing one: the columns a release adds (#80's `consumed`, #83's `issuer` and `extension_data`, the codes' - `redirect_uri_omitted`, the refresh tokens' `family_issued_at`) and the - binary key columns of MySQL are added or converted with the statements - MIGRATION.md lists, through your migration tool. + `redirect_uri_omitted`, the refresh tokens' `family_issued_at`), the + binary key columns and the InnoDB engine of MySQL are added or converted + with the statements MIGRATION.md lists, through your migration tool. + `Migrate` — and `Store.CheckDeployment`, for a schema applied by your own + tool — refuses a MySQL database still missing the engine or the binary + keys; a missing column fails the statements that use it, closed. +- **Deployment checks see what the server reports** — `CheckDeployment` + verifies on MySQL the engine, the key columns and the `sql_mode` of one + pooled connection (set the strict mode server-wide or in the DSN), and on + Redis what `INFO` reports. It checks nothing on PostgreSQL: + `synchronous_commit=off` can lose an acknowledged revocation in a crash, + but it can be set per role, session or transaction, so one connection's + value proves nothing — keep it on. Replicas, proxies and eviction set + after boot are not seen. - **A leaked code can be burned** — codes are consumed before they are validated, so a party holding a code (but not its client's credentials or PKCE verifier) can make the legitimate exchange fail with @@ -349,8 +366,8 @@ PKCE. Not supported, and refused where a provider could select them: login starts, since a full store refuses every new login (`memory.ErrFull`) until transactions expire or are consumed. `oidc/client/store/sql` ships `PurgeExpired`, a single - unbatched `DELETE` the operator schedules (index `expires_at`, see its - package documentation). CI (`make test-backends`) runs the conformance + unbatched `DELETE` at READ COMMITTED the operator schedules (index + `expires_at`, see its package documentation). CI (`make test-backends`) runs the conformance suite and the replica races of the SQL and Redis stores on PostgreSQL 16.15, MySQL 8.0.46 (end of life since April 2026) and Redis 7.4.11 (`internal/interop/oidcstack`, `TestRealBackend*`), and the concurrent @@ -569,8 +586,8 @@ PKCE. Not supported, and refused where a provider could select them: - **Stores** — `saml/sp/storage/memory` (development and tests) sweeps its expired records every 64 writes on its own clock (`memory.WithClock`) and is otherwise unbounded: it holds the login rate times `TransactionTTL`. - `saml/sp/store/sql` ships no expiry job (its package documentation gives - the cleanup statements). CI runs the stores on + `saml/sp/store/sql` deletes expired rows only when its `PurgeExpired` + runs: schedule it. CI runs the stores on SQLite and miniredis, and (`make test-backends`) their races, conformance suite and payload round trip on PostgreSQL 16.15, MySQL 8.0.46 (end of life since April 2026) and Redis 7.4.11 diff --git a/MIGRATION.md b/MIGRATION.md index 740282bc..17314f9d 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -3225,3 +3225,132 @@ To migrate: 2. A provider whose PS* ID Tokens now fail with `ErrIDTokenSignature` must sign with a salt as long as the hash (RFC 7518 §3.5), or the client use another algorithm it offers (`Config.Algorithms`). + +### SQL stores: MySQL tables are InnoDB, checked at boot + +`Schema(sqlstore.MySQL)` of `oauth2/store/sql`, `oidc/client/store/sql` and +`saml/sp/store/sql` named no storage engine: on a server whose default +engine is MyISAM (`default_storage_engine`), `Migrate` created tables +without transactions or row locks. The oauth2 store then lost the atomic +writes its contract promises (I3): a rotation cut short — a context +cancelled, a lost connection — left the presented refresh token consumed +without its replacement. MyISAM is not crash safe either: a consumption +acknowledged before a crash could be lost. The DDL now names +`ENGINE=InnoDB DEFAULT CHARSET=utf8mb4`, and every SQL store gains +`CheckDeployment`, which `Migrate` runs once the tables exist. + +| | Before | After | +| --------------------------------------------------- | ------------------------------------ | --------------------------------------------------------------------- | +| MySQL `CREATE TABLE` of the three SQL stores | the server's default engine and charset | `ENGINE=InnoDB DEFAULT CHARSET=utf8mb4` | +| `Migrate` on MySQL with a MyISAM (or missing) table | succeeds | fails with `ErrDeploymentRequirement`, naming the table | +| `oauth2/store/sql` `Migrate` with legacy `VARCHAR` key columns | succeeds | fails with `ErrDeploymentRequirement`, naming each column | +| `oauth2/store/sql` `Migrate` on a non-strict `sql_mode` | succeeds (over-long values truncated) | fails with `ErrDeploymentRequirement` | +| `Store.CheckDeployment(ctx)` | — | new, on the three SQL stores (no-op on PostgreSQL and SQLite) | +| Concurrent `Migrate` on MySQL | concurrent `CREATE TABLE IF NOT EXISTS` (could fail with a metadata-lock deadlock, 1213) | one after the other, under the named lock `hyperscale-stack/security/.Migrate` (`GET_LOCK`) | +| PostgreSQL and SQLite | — | unchanged | + +`Migrate` still never alters a table. To migrate (MySQL and MariaDB only): + +1. **Find the tables to convert**: + + ```sql + SELECT TABLE_NAME, ENGINE, TABLE_COLLATION FROM information_schema.TABLES + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME IN ( + 'oauth2_auth_codes', 'oauth2_access_tokens', 'oauth2_refresh_tokens', 'oauth2_token_families', + 'oidc_client_transactions', 'samlsp_transactions', 'samlsp_used_assertions'); + ``` + +2. **Convert every table that is not InnoDB** — keep the lines of the + tables the query listed: + + ```sql + ALTER TABLE oauth2_auth_codes ENGINE=InnoDB; + ALTER TABLE oauth2_access_tokens ENGINE=InnoDB; + ALTER TABLE oauth2_refresh_tokens ENGINE=InnoDB; + ALTER TABLE oauth2_token_families ENGINE=InnoDB; + ALTER TABLE oidc_client_transactions ENGINE=InnoDB; + ALTER TABLE samlsp_transactions ENGINE=InnoDB; + ALTER TABLE samlsp_used_assertions ENGINE=InnoDB; + ``` + +3. **Optionally, convert the oauth2 tables to utf8mb4** when the query + listed another character set: subjects, scopes and extension data are + UTF-8. The OIDC and SAML tables hold ASCII only. `CONVERT TO` turns a + `TEXT` column into `MEDIUMTEXT`, which changes nothing for the store; + the binary key columns are left as they are. + + ```sql + ALTER TABLE oauth2_auth_codes CONVERT TO CHARACTER SET utf8mb4; + ALTER TABLE oauth2_access_tokens CONVERT TO CHARACTER SET utf8mb4; + ALTER TABLE oauth2_refresh_tokens CONVERT TO CHARACTER SET utf8mb4; + ALTER TABLE oauth2_token_families CONVERT TO CHARACTER SET utf8mb4; + ``` + +4. **Plan the window.** Each `ALTER` copies its table under a metadata + lock: run it at a quiet time, or with an online schema-change tool. +5. **Make `sql_mode` strict** for the oauth2 store's connections — + `STRICT_TRANS_TABLES` or `STRICT_ALL_TABLES`, the default of MySQL 8 and + MariaDB — in the server's configuration, or in the DSN with + `?sql_mode='TRADITIONAL'` (go-sql-driver/mysql): strict, and it keeps + `NO_ENGINE_SUBSTITUTION`; a DSN mode replaces the whole session mode. A + database still on `VARCHAR` key columns first needs the upgrade of the + section "`oauth2/store/sql`: MySQL compares keys byte for byte". +6. **Check at boot.** `Migrate` now fails, with an error matching the + store's `ErrDeploymentRequirement`, until the deployment meets the + requirements; it lists every one it finds unmet. When you apply + `Schema` with your own migration tool, call `store.CheckDeployment(ctx)` + at boot, before serving traffic. +7. **Concurrent boots.** On MySQL and MariaDB, `Migrate` holds the named + lock its package documentation names (`GET_LOCK`, server-wide) while it + creates the tables and checks them, and waits for it as long as its + context allows: pass a deadline. A migration tool applying `Schema` + while instances run `Migrate` should take the same lock. A holder whose + client vanished without closing its connection keeps the lock until the + server notices (`wait_timeout`, 8 hours by default): find it with + `SELECT IS_USED_LOCK('hyperscale-stack/security/oauth2/store/sql.Migrate')` + and end it with `KILL `. On a Galera or Percona XtraDB cluster named + locks are local to a node (some versions refuse `GET_LOCK`): instances + migrating through different nodes are not serialized — run `Migrate` + through one node, or apply `Schema` with your own tool. + +### `oauth2/store/redis`: `New` refuses Cluster and Ring clients + +The store's scripts touch several keys at once — a token, its family, its +replacement. A Redis Cluster puts them in different hash slots, and a +client-side `redis.Ring` on different shards: a Cluster client failed every +script with CROSSSLOT, a Ring client found a token only when it shared its +family's shard. `New` now refuses both, with an error matching +`redisstore.ErrDeploymentRequirement`, and `Store.CheckDeployment` checks +the server (Redis 6.0 or later, `maxmemory-policy noeviction`, no cluster +mode). + +| | Before | After | +| ------------------------------------ | ------------------------------ | ------------------------------------------------- | +| `New(*redis.ClusterClient)` | accepted, scripts fail at runtime | error matching `ErrDeploymentRequirement` | +| `New(*redis.Ring)` | accepted, lookups fail at random | error matching `ErrDeploymentRequirement` | +| `New(*redis.Client)` (standalone, Sentinel failover client) | accepted | unchanged | +| `Store.CheckDeployment(ctx)` | — | new: checks the server through `INFO` | + +To migrate: + +1. Run the store on a standalone Redis or the primary of a Sentinel + deployment (`redis.NewClient`, `redis.NewFailoverClient`). +2. Call `store.CheckDeployment(ctx)` at boot. A restricted ACL user needs + `+info` for it (`INFO` is in the `@dangerous` category); without it the + call returns the server's NOPERM error — verify the requirements by hand + and skip the call. A `maxmemory-policy` other than `noeviction` is + refused: a "revoked" family key evicted within its retention lets the + next issuance revive the family. + +### `oidc/client`: `TransactionStore.Consume` validates its keys + +`Consume` of the memory, SQL and Redis stores now refuses, before any I/O, a +state hash or binding hash that is not 64 lowercase hex digits, with an +error matching `oidcclient.ErrInvalidTransaction`, as the +`TransactionStore` contract states. `Client.Complete` always passes +canonical keys: nothing changes for it. + +To migrate a `TransactionStore` of your own: call +`oidcclient.ValidateStoreKey` on both keys first in `Consume`, and run +`storetest.RunConformance`, which now checks it +(`ConsumeRefusesInvalidKeys`). diff --git a/docs/architecture.md b/docs/architecture.md index f1f54631..8e6b2cf7 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -332,8 +332,10 @@ StartLogin / Begin HandleCallback / Complete completes a login; altering a transaction fails it, whoever runs it. - **`TransactionStore`** — `Save` never overwrites, `Consume` removes a transaction atomically in a pinned decision order (unknown, expired, - binding mismatch — which keeps it). In-memory, SQL and Redis - implementations pass `oidc/client/storetest`. + binding mismatch — which keeps it). Both refuse malformed input before + any I/O: `Save` what `Transaction.Validate` refuses, `Consume` keys + `ValidateStoreKey` refuses. In-memory, SQL and Redis implementations pass + `oidc/client/storetest`. - **ID Token validation** — OpenID Connect Core §3.1.3.7 on top of go-jose: asymmetric algorithms only, keys from the issuer's `jwks_uri` through a bounded, rate-limited cache, and every claim check including nonce, diff --git a/docs/saml-sp.md b/docs/saml-sp.md index 7c88bfbf..2096501c 100644 --- a/docs/saml-sp.md +++ b/docs/saml-sp.md @@ -242,8 +242,9 @@ Keycloak run hit this with a freshly generated realm key. it the SP's with `memory.WithClock` when that is not `time.Now`; it holds the login rate times `TransactionTTL`, so rate-limit `BeginLogin`; - - `saml/sp/store/sql` ships no job, and its package documentation gives - the cleanup statements; + - `saml/sp/store/sql` deletes expired rows when its `PurgeExpired` runs: + schedule it, and add the `expires_at` indexes its package documentation + lists; - `saml/sp/store/redis` uses key expiry, so Redis must run with `noeviction`. diff --git a/docs/security-considerations.md b/docs/security-considerations.md index eec90d61..d6014b55 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -395,7 +395,7 @@ keys by `kid` against a JWKS provider (static or remote). 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 + expires the key with the code, SQL rows stay until `PurgeExpired`, 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 @@ -788,7 +788,11 @@ MySQL tables use InnoDB, Redis is a standalone server or a Sentinel primary (not Cluster) running 6.0 or later with `maxmemory-policy noeviction`, no family row or key is deleted before it expires, and durability is the backend's: a revocation acknowledged by a primary that fails before -persisting or replicating it can be lost in a failover. CI checks these +persisting or replicating it can be lost in a failover. `CheckDeployment` +checks what the server reports of them — on MySQL the engine, the binary +key columns and a strict `sql_mode` (`Migrate` runs it), on Redis the +version, the eviction policy and cluster mode — and the Redis store's +`New` refuses a Cluster or Ring client. CI checks these guarantees on SQLite and, in its "Store suites (real backends)" job (`make test-backends`), on PostgreSQL 16, MySQL 8.0 and 8.4, MariaDB 11.4 (`internal/interop/sqlstores`) and Redis 7.4 (the conformance suite of @@ -1917,12 +1921,19 @@ 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` — - 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. +- [ ] With `store/sql`, schedule `Store.PurgeExpired` with the current + time — every few minutes — after adding the `expires_at` indexes the + package documentation lists; 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. +- [ ] SQL stores (oauth2, OpenID Connect, SAML) whose schema your + migration tool applies, not `Migrate` (which runs it): call + `CheckDeployment` at boot and treat its error as fatal — on MySQL, + InnoDB tables, binary oauth2 key columns and a strict `sql_mode`. +- [ ] `oauth2/store/redis`: call `CheckDeployment` at boot and treat its + error as fatal — version 6.0+, `noeviction`, no cluster mode; the + user needs `+info` for it. - [ ] Keep JWT access tokens short-lived: a store revocation does not reach resource servers that verify them offline. - [ ] Bound refresh-token families with `grant.Config.RefreshFamilyTTL` — @@ -1967,10 +1978,11 @@ span catalog. `CheckURL`, `RootCAs`). Enable `InsecureAllowSHA1` only for an IdP that cannot do better, with a plan to remove it. - [ ] SAML: share the transaction store and the replay cache between - instances (`saml/sp/store/sql` or `saml/sp/store/redis`) and purge - expired SQL rows. Give the SP least-privilege access to them: a Redis - ACL limited to its key prefix, a database user limited to its two - tables. + instances (`saml/sp/store/sql` or `saml/sp/store/redis`); with the + SQL store, schedule `PurgeExpired`, and call `CheckDeployment` at boot + when your migration tool applies its schema. Give the SP + least-privilege access to them: a Redis ACL limited to its key prefix, + a database user limited to its two tables. - [ ] SAML: load `samlsp.Config.TransactionKeys` from your secret store — the same keys on every instance, distinct from the OpenID Connect client's and the session keys — and rotate them by adding the new key diff --git a/internal/interop/sqlstores/deployment_test.go b/internal/interop/sqlstores/deployment_test.go new file mode 100644 index 00000000..43661e0d --- /dev/null +++ b/internal/interop/sqlstores/deployment_test.go @@ -0,0 +1,353 @@ +// 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 sqlstores_test + +import ( + "database/sql" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/hyperscale-stack/security/oauth2" + oauth2sql "github.com/hyperscale-stack/security/oauth2/store/sql" + "github.com/hyperscale-stack/security/oauth2/storetest" + "github.com/stretchr/testify/require" +) + +// mysqlServers are the MySQL-family servers the deployment checks run on. +var mysqlServers = []backend{mysql80, mysql84, mariadb} + +// withParams returns b whose namespaces set the given system variables on +// every connection, under a name of its own. +func withParams(b backend, suffix string, params map[string]string) backend { + b.name += "-" + suffix + b.params = params + + return b +} + +// myisamDefault makes MyISAM the storage engine of every CREATE TABLE that +// names none: a server started with --default-storage-engine=MyISAM. +var myisamDefault = map[string]string{"default_storage_engine": "'MyISAM'"} + +// tableStatus returns the storage engine and collation of table in the +// namespace db works in, "" for a missing table. +func tableStatus(t *testing.T, db *sql.DB, table string) (engine, collation string) { + t.Helper() + + err := db.QueryRowContext(t.Context(), `SELECT ENGINE, TABLE_COLLATION FROM information_schema.TABLES + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = ?`, table).Scan(&engine, &collation) + if err == sql.ErrNoRows { + return "", "" + } + + require.NoError(t, err) + + return engine, collation +} + +// requireInnoDB asserts that every table of s is InnoDB, in utf8mb4. +func requireInnoDB(t *testing.T, db *sql.DB, s sqlStore) { + t.Helper() + + for _, table := range s.tables { + engine, collation := tableStatus(t, db, table) + require.Equal(t, "InnoDB", engine, "engine of %s", table) + require.True(t, strings.HasPrefix(collation, "utf8mb4"), "collation of %s: %s", table, collation) + } +} + +// TestMySQLMigrateCreatesInnoDBTables runs Migrate on servers whose +// default storage engine is MyISAM: the DDL names InnoDB and utf8mb4, so +// every table is created InnoDB — the engine whose transactions and row +// locks the stores rest on — and Migrate's own check passes. +func TestMySQLMigrateCreatesInnoDBTables(t *testing.T) { + for _, base := range mysqlServers { + b := withParams(base, "myisam-default", myisamDefault) + + t.Run(b.name, func(t *testing.T) { + b.server(t) + + for _, s := range sqlStores { + t.Run(s.name, func(t *testing.T) { + driver, dsn := b.namespace(t) + db := open(t, driver, dsn, 4) + + require.NoError(t, s.migrate(t.Context(), db, engineMySQL)) + requireInnoDB(t, db, s) + s.use(t, db, engineMySQL) + }) + } + }) + } +} + +// engineSection is the heading of the MIGRATION.md section whose SQL +// blocks — the preflight query, the conversion to InnoDB, the conversion +// to utf8mb4 — TestMySQLCheckDeploymentRefusesMyISAM runs. +const engineSection = "### SQL stores: MySQL tables are InnoDB, checked at boot" + +// sectionBlocks returns the SQL blocks of the MIGRATION.md section +// heading, each as its statements. +func sectionBlocks(t *testing.T, heading string) [][]string { + t.Helper() + + raw, err := os.ReadFile(filepath.Join("..", "..", "..", "MIGRATION.md")) + require.NoError(t, err) + + doc := string(raw) + start := strings.Index(doc, "\n"+heading+"\n") + require.GreaterOrEqual(t, start, 0, "MIGRATION.md has no section %q", heading) + + return sqlBlocks(t, doc[start+1:], heading) +} + +// TestMySQLCheckDeploymentRefusesMyISAM: tables a MyISAM-default server +// created before the DDL named an engine stay MyISAM — CREATE TABLE IF NOT +// EXISTS leaves them — so CheckDeployment, and Migrate which ends with it, +// refuse them, naming each table. The statements MIGRATION.md gives +// convert them; then both pass and the stores work, the oauth2 one passing +// its conformance suite. +func TestMySQLCheckDeploymentRefusesMyISAM(t *testing.T) { + blocks := sectionBlocks(t, engineSection) + require.Len(t, blocks, 3, "the preflight, the engine conversion and the character-set conversion") + + for _, base := range mysqlServers { + b := withParams(base, "myisam", myisamDefault) + + t.Run(b.name, func(t *testing.T) { + b.server(t) + + driver, dsn := b.namespace(t) + db := open(t, driver, dsn, 4) + createMyISAM(t, db) + + for _, s := range sqlStores { + err := s.check(t.Context(), db, engineMySQL) + require.ErrorIs(t, err, s.requirement, "CheckDeployment of %s", s.name) + + for _, table := range s.tables { + require.Contains(t, err.Error(), "table "+table+" uses the MyISAM engine, not InnoDB") + } + + require.ErrorIs(t, s.migrate(t.Context(), db, engineMySQL), s.requirement, "Migrate of %s", s.name) + } + + execAll(t, db, blocks[0]...) + execAll(t, db, blocks[1]...) + execAll(t, db, blocks[2]...) + + for _, s := range sqlStores { + require.NoError(t, s.check(t.Context(), db, engineMySQL), "CheckDeployment of %s", s.name) + require.NoError(t, s.migrate(t.Context(), db, engineMySQL), "Migrate of %s", s.name) + requireInnoDB(t, db, s) + s.use(t, db, engineMySQL) + } + + storetest.RunConformance(t, func() oauth2.Storage { + store, err := oauth2sql.New(db, oauth2sql.MySQL) + require.NoError(t, err) + + return store + }) + }) + } +} + +// createMyISAM creates the tables of every store as MyISAM tables. +func createMyISAM(t *testing.T, db *sql.DB) { + t.Helper() + + for _, s := range sqlStores { + for _, stmt := range s.schema(engineMySQL) { + execAll(t, db, strings.Replace(stmt, "ENGINE=InnoDB", "ENGINE=MyISAM", 1)) + } + + for _, table := range s.tables { + engine, _ := tableStatus(t, db, table) + require.Equal(t, "MyISAM", engine, "engine of %s", table) + } + } +} + +// TestMySQLCheckDeploymentRefusesNonStrictSQLMode: without a strict +// sql_mode, MySQL truncates an over-long subject or client ID instead of +// refusing it; the oauth2 store, which carries no integrity check of its +// own, refuses such a session. The OIDC and SAML records are bounded and +// MAC-protected: they have no sql_mode requirement. +func TestMySQLCheckDeploymentRefusesNonStrictSQLMode(t *testing.T) { + for _, base := range mysqlServers { + for _, mode := range []string{"''", "'NO_ENGINE_SUBSTITUTION'", "'ANSI_QUOTES,NO_ZERO_DATE'"} { + b := withParams(base, "sql-mode-"+strings.Trim(mode, "'"), map[string]string{"sql_mode": mode}) + + t.Run(b.name, func(t *testing.T) { + b.server(t) + + driver, dsn := b.namespace(t) + db := open(t, driver, dsn, 2) + + for _, s := range sqlStores { + err := s.migrate(t.Context(), db, engineMySQL) + if s.name != "oauth2" { + require.NoError(t, err, "Migrate of %s", s.name) + + continue + } + + require.ErrorIs(t, err, s.requirement) + require.Contains(t, err.Error(), "sql_mode") + require.ErrorIs(t, s.check(t.Context(), db, engineMySQL), s.requirement) + } + }) + } + + for _, mode := range []string{"'TRADITIONAL'", "'STRICT_ALL_TABLES'", "'STRICT_TRANS_TABLES'"} { + b := withParams(base, "sql-mode-"+strings.Trim(mode, "'"), map[string]string{"sql_mode": mode}) + + t.Run(b.name, func(t *testing.T) { + b.server(t) + + driver, dsn := b.namespace(t) + db := open(t, driver, dsn, 2) + + for _, s := range sqlStores { + require.NoError(t, s.migrate(t.Context(), db, engineMySQL), "Migrate of %s", s.name) + } + }) + } + } +} + +// TestMySQLCheckDeploymentRefusesLegacyKeys: an oauth2 database created +// before its key columns were binary compares keys under a collation that +// may ignore case, accents or trailing spaces; CheckDeployment names each +// such column, and passes once MIGRATION.md's upgrade is applied. +func TestMySQLCheckDeploymentRefusesLegacyKeys(t *testing.T) { + blocks := upgradeBlocks(t) + upgrade := append(blocks[1], blocks[2]...) + + for _, b := range mysqlServers { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + driver, dsn := b.namespace(t) + db := open(t, driver, dsn, 2) + execAll(t, db, legacyMySQLSchema...) + + s := sqlStores[0] + err := s.check(t.Context(), db, engineMySQL) + require.ErrorIs(t, err, oauth2sql.ErrDeploymentRequirement) + + for _, column := range []string{ + "oauth2_auth_codes.code_hash", "oauth2_access_tokens.token_hash", "oauth2_access_tokens.family_id", + "oauth2_refresh_tokens.token_hash", "oauth2_refresh_tokens.family_id", "oauth2_token_families.family_id", + } { + require.Contains(t, err.Error(), "column "+column+" is varchar, not varbinary") + } + + execAll(t, db, upgrade...) + require.NoError(t, s.check(t.Context(), db, engineMySQL)) + }) + } +} + +// TestMySQLCheckDeploymentReportsMissingTables: a table the database does +// not hold — a migration not applied, another database selected — is +// reported, never taken for a compliant one. +func TestMySQLCheckDeploymentReportsMissingTables(t *testing.T) { + for _, b := range mysqlServers { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + driver, dsn := b.namespace(t) + db := open(t, driver, dsn, 2) + + for _, s := range sqlStores { + require.NoError(t, s.migrate(t.Context(), db, engineMySQL)) + + missing := s.tables[len(s.tables)-1] + execAll(t, db, "DROP TABLE "+missing) + + err := s.check(t.Context(), db, engineMySQL) + require.ErrorIs(t, err, s.requirement, "CheckDeployment of %s", s.name) + require.Contains(t, err.Error(), "table "+missing+" not found") + } + }) + } +} + +// TestMySQLCheckDeploymentIsScopedToItsDatabase: a MySQL server often holds +// the store's tables in several databases — one per environment or +// tenant. CheckDeployment reads the tables of the connection's database +// only: a compliant copy in another database never hides a table missing, +// on another engine or with text keys in this one. +func TestMySQLCheckDeploymentIsScopedToItsDatabase(t *testing.T) { + for _, b := range mysqlServers { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + ctx := t.Context() + + // The compliant copy, in a database of its own. + driver, dsn := b.namespace(t) + compliant := open(t, driver, dsn, 2) + + for _, s := range sqlStores { + require.NoError(t, s.migrate(ctx, compliant, engineMySQL), "Migrate of %s", s.name) + } + + // A database without the tables. + driver, dsn = b.namespace(t) + empty := open(t, driver, dsn, 2) + + for _, s := range sqlStores { + err := s.check(ctx, empty, engineMySQL) + require.ErrorIs(t, err, s.requirement, "CheckDeployment of %s", s.name) + + for _, table := range s.tables { + require.Contains(t, err.Error(), "table "+table+" not found in the connection's database") + } + } + + // A database with MyISAM tables. + driver, dsn = b.namespace(t) + myisam := open(t, driver, dsn, 2) + createMyISAM(t, myisam) + + for _, s := range sqlStores { + err := s.check(ctx, myisam, engineMySQL) + require.ErrorIs(t, err, s.requirement, "CheckDeployment of %s", s.name) + + for _, table := range s.tables { + require.Contains(t, err.Error(), "table "+table+" uses the MyISAM engine, not InnoDB") + } + } + + // A database whose oauth2 families table lacks its key column, + // and one with the former text keys. + driver, dsn = b.namespace(t) + keyless := open(t, driver, dsn, 2) + oauth2 := sqlStores[0] + schema := oauth2.schema(engineMySQL) + execAll(t, keyless, schema[:len(schema)-1]...) + execAll(t, keyless, `CREATE TABLE oauth2_token_families ( + fid VARBINARY(64) PRIMARY KEY, revoked_at BIGINT NOT NULL DEFAULT 0, expires_at BIGINT NOT NULL + ) ENGINE=InnoDB`) + + err := oauth2.check(ctx, keyless, engineMySQL) + require.ErrorIs(t, err, oauth2.requirement) + require.Contains(t, err.Error(), "column oauth2_token_families.family_id not found") + + driver, dsn = b.namespace(t) + legacy := open(t, driver, dsn, 2) + execAll(t, legacy, legacyMySQLSchema...) + + err = oauth2.check(ctx, legacy, engineMySQL) + require.ErrorIs(t, err, oauth2.requirement) + require.Contains(t, err.Error(), "column oauth2_auth_codes.code_hash is varchar, not varbinary") + }) + } +} diff --git a/internal/interop/sqlstores/locking_test.go b/internal/interop/sqlstores/locking_test.go new file mode 100644 index 00000000..6a98b962 --- /dev/null +++ b/internal/interop/sqlstores/locking_test.go @@ -0,0 +1,269 @@ +// 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 sqlstores_test + +import ( + "context" + "database/sql" + "strconv" + "strings" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" + oauth2sql "github.com/hyperscale-stack/security/oauth2/store/sql" + "github.com/stretchr/testify/require" +) + +// lockingServers are the servers with row locks: SQLite's single writer +// serializes the store's transactions instead. +var lockingServers = []backend{postgres, mysql80, mysql84, mariadb} + +// placeholders rewrites the ? of query into PostgreSQL's $1, $2, … on e. +func placeholders(e engine, query string) string { + if e != enginePostgres { + return query + } + + var b strings.Builder + + n := 0 + + for _, r := range query { + if r != '?' { + b.WriteRune(r) + + continue + } + + n++ + + b.WriteString("$" + strconv.Itoa(n)) + } + + return b.String() +} + +// lockHolder is a transaction of the test holding row locks, on a +// connection of its own whose server-side ID — the backend PID on +// PostgreSQL, CONNECTION_ID() on MySQL — identifies the sessions it blocks. +type lockHolder struct { + t *testing.T + engine engine + conn *sql.Conn + tx *sql.Tx + id int64 +} + +// holdLocks opens a transaction on db and runs each statement in it — a +// SELECT … FOR UPDATE, an UPDATE — so that it holds their row locks. +func holdLocks(t *testing.T, db *sql.DB, e engine, stmts ...stmt) *lockHolder { + t.Helper() + + conn, err := db.Conn(t.Context()) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + + h := &lockHolder{t: t, engine: e, conn: conn} + + idQuery := "SELECT CONNECTION_ID()" + if e == enginePostgres { + idQuery = "SELECT pg_backend_pid()" + } + + require.NoError(t, conn.QueryRowContext(t.Context(), idQuery).Scan(&h.id)) + + h.tx, err = conn.BeginTx(t.Context(), nil) + require.NoError(t, err) + t.Cleanup(func() { _ = h.tx.Rollback() }) + + h.exec(stmts...) + + return h +} + +// stmt is a statement with its arguments, written with ? placeholders. +type stmt struct { + query string + args []any +} + +// exec runs stmts in the holder's transaction. +func (h *lockHolder) exec(stmts ...stmt) { + h.t.Helper() + + for _, s := range stmts { + _, err := h.tx.ExecContext(h.t.Context(), placeholders(h.engine, s.query), s.args...) + require.NoError(h.t, err, s.query) + } +} + +// commit ends the holder's transaction, releasing its locks. +func (h *lockHolder) commit() { + h.t.Helper() + + require.NoError(h.t, h.tx.Commit()) +} + +// lockWaitDeadline bounds the wait for a session to block on a holder, +// polled every lockWaitPoll: a MySQL sys.innodb_lock_waits query latches +// the lock system, so polling it in a tight loop keeps the waiting session +// from enqueuing its wait. +const ( + lockWaitDeadline = 10 * time.Second + lockWaitPoll = 100 * time.Millisecond +) + +// waitForWaiter returns once the server reports a session waiting for one +// of the holder's locks — that session, not another test's: the query +// names the holder. It reads through admin, another pool on the server. +func (h *lockHolder) waitForWaiter(admin *sql.DB) { + h.t.Helper() + + query := `SELECT COUNT(*) FROM sys.innodb_lock_waits WHERE blocking_pid = ?` + if h.engine == enginePostgres { + query = `SELECT COUNT(*) FROM pg_stat_activity WHERE $1 = ANY(pg_blocking_pids(pid))` + } + + deadline := time.Now().Add(lockWaitDeadline) + + for { + var waiters int + require.NoError(h.t, admin.QueryRowContext(h.t.Context(), query, h.id).Scan(&waiters)) + + if waiters > 0 { + return + } + + require.True(h.t, time.Now().Before(deadline), "no session waited for the holder's locks within %s", lockWaitDeadline) + time.Sleep(lockWaitPoll) + } +} + +// lockingStore returns a migrated oauth2 store on a new namespace of b, its +// pool, and an admin pool for the lock-wait queries. +func lockingStore(t *testing.T, b backend) (store *oauth2sql.Store, db, admin *sql.DB) { + t.Helper() + + driver, dsn := b.namespace(t) + db = open(t, driver, dsn, 4) + admin = open(t, driver, dsn, 1) + + store, err := oauth2sql.New(db, oauth2Dialect(b.engine)) + require.NoError(t, err) + require.NoError(t, store.Migrate(t.Context())) + + return store, db, admin +} + +// rotateInBackground starts the rotation of from into next and returns the +// channel its result arrives on. +func rotateInBackground(store *oauth2sql.Store, from, next *oauth2.TokenPair) <-chan error { + done := make(chan error, 1) + + go func() { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + done <- store.RotateRefreshToken(ctx, from.Refresh.TokenHash, next) + }() + + return done +} + +// countRows returns the number of rows of table whose key column holds key. +func countRows(t *testing.T, db *sql.DB, e engine, table, column, key string) int { + t.Helper() + + var n int + require.NoError(t, db.QueryRowContext(t.Context(), + placeholders(e, "SELECT COUNT(*) FROM "+table+" WHERE "+column+" = ?"), key).Scan(&n)) + + return n +} + +// requireNotPersisted asserts that no token of pair was written. +func requireNotPersisted(t *testing.T, db *sql.DB, e engine, pair *oauth2.TokenPair) { + t.Helper() + + require.Zero(t, countRows(t, db, e, "oauth2_access_tokens", "token_hash", pair.Access.TokenHash), "access token persisted") + require.Zero(t, countRows(t, db, e, "oauth2_refresh_tokens", "token_hash", pair.Refresh.TokenHash), "refresh token persisted") +} + +// TestOAuth2RotationWaitsForTheFamilyLock pins the family row lock (I-25, +// the first locking layer): a revocation holds the family's row while a +// rotation of one of its tokens starts. The rotation waits for it, then +// finds the family revoked: it fails with ErrTokenFamilyRevoked and writes +// nothing. A rotation reading the family without the lock would not wait: +// it would read the family active and write its tokens into a family being +// revoked (on PostgreSQL; on MySQL the family upsert, INSERT … ON +// DUPLICATE KEY UPDATE, takes the row lock first). +func TestOAuth2RotationWaitsForTheFamilyLock(t *testing.T) { + for _, b := range lockingServers { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + store, db, admin := lockingStore(t, b) + + first, next := tokenPair("lock-family-0", "fam-lock"), tokenPair("lock-family-1", "fam-lock") + require.NoError(t, store.SaveTokenPair(t.Context(), first)) + + holder := holdLocks(t, db, b.engine, + stmt{`SELECT revoked_at FROM oauth2_token_families WHERE family_id = ? FOR UPDATE`, []any{"fam-lock"}}) + + done := rotateInBackground(store, first, next) + holder.waitForWaiter(admin) + + holder.exec(stmt{`UPDATE oauth2_token_families SET revoked_at = ? WHERE family_id = ?`, + []any{time.Now().Unix(), "fam-lock"}}) + holder.commit() + + require.ErrorIs(t, <-done, oauth2.ErrTokenFamilyRevoked) + requireNotPersisted(t, db, b.engine, next) + + var consumed int + require.NoError(t, db.QueryRowContext(t.Context(), placeholders(b.engine, + `SELECT consumed FROM oauth2_refresh_tokens WHERE token_hash = ?`), first.Refresh.TokenHash).Scan(&consumed)) + require.Zero(t, consumed, "the presented token is not consumed") + }) + } +} + +// TestOAuth2RotationLocksThePresentedToken pins the refresh-token row lock +// (I-25, the second locking layer): another writer holds the presented +// token's row and consumes it while a rotation starts. The rotation waits +// for it, then finds the token consumed: a reuse, which revokes the +// family. A rotation reading the token without the lock would see it +// unconsumed and stop on the third layer — its UPDATE consuming no row — as +// a backend error, not as the reuse it is. +func TestOAuth2RotationLocksThePresentedToken(t *testing.T) { + for _, b := range lockingServers { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + store, db, admin := lockingStore(t, b) + + first, next := tokenPair("lock-token-0", "fam-lock-token"), tokenPair("lock-token-1", "fam-lock-token") + require.NoError(t, store.SaveTokenPair(t.Context(), first)) + + holder := holdLocks(t, db, b.engine, + stmt{`SELECT consumed FROM oauth2_refresh_tokens WHERE token_hash = ? FOR UPDATE`, []any{first.Refresh.TokenHash}}) + + done := rotateInBackground(store, first, next) + holder.waitForWaiter(admin) + + holder.exec(stmt{`UPDATE oauth2_refresh_tokens SET consumed = 1 WHERE token_hash = ?`, []any{first.Refresh.TokenHash}}) + holder.commit() + + err := <-done + require.ErrorIs(t, err, oauth2.ErrRefreshTokenReused) + require.NotErrorIs(t, err, oauth2.ErrTokenFamilyRevocationFailed) + requireNotPersisted(t, db, b.engine, next) + + _, err = store.LookupAccessToken(t.Context(), first.Access.TokenHash) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked, "the reuse revoked the family") + }) + } +} diff --git a/internal/interop/sqlstores/migrate_test.go b/internal/interop/sqlstores/migrate_test.go index d7c93d08..d4f816be 100644 --- a/internal/interop/sqlstores/migrate_test.go +++ b/internal/interop/sqlstores/migrate_test.go @@ -11,6 +11,7 @@ import ( "encoding/binary" "encoding/hex" "strconv" + "strings" "sync" "testing" "time" @@ -34,6 +35,12 @@ type sqlStore struct { tables []string // migrate runs the store's Migrate on db. migrate func(ctx context.Context, db *sql.DB, e engine) error + // check runs the store's CheckDeployment on db. + check func(ctx context.Context, db *sql.DB, e engine) error + // schema is the store's Schema for e. + schema func(e engine) []string + // requirement is the store's ErrDeploymentRequirement. + requirement error // use writes and reads a record, proving the tables serve the store. use func(t *testing.T, db *sql.DB, e engine) } @@ -51,7 +58,17 @@ var sqlStores = []sqlStore{ return s.Migrate(ctx) }, - use: useOAuth2, + check: func(ctx context.Context, db *sql.DB, e engine) error { + s, err := oauth2sql.New(db, oauth2Dialect(e)) + if err != nil { + return err + } + + return s.CheckDeployment(ctx) + }, + schema: func(e engine) []string { return oauth2sql.Schema(oauth2Dialect(e)) }, + requirement: oauth2sql.ErrDeploymentRequirement, + use: useOAuth2, }, { name: "oidc", @@ -65,7 +82,17 @@ var sqlStores = []sqlStore{ return s.Migrate(ctx) }, - use: useOIDC, + check: func(ctx context.Context, db *sql.DB, e engine) error { + s, err := oidcsql.New(db, oidcDialect(e)) + if err != nil { + return err + } + + return s.CheckDeployment(ctx) + }, + schema: func(e engine) []string { return oidcsql.Schema(oidcDialect(e)) }, + requirement: oidcsql.ErrDeploymentRequirement, + use: useOIDC, }, { name: "saml", @@ -79,7 +106,17 @@ var sqlStores = []sqlStore{ return s.Migrate(ctx) }, - use: useSAML, + check: func(ctx context.Context, db *sql.DB, e engine) error { + s, err := samlsql.New(db, samlDialect(e)) + if err != nil { + return err + } + + return s.CheckDeployment(ctx) + }, + schema: func(e engine) []string { return samlsql.Schema(samlDialect(e)) }, + requirement: samlsql.ErrDeploymentRequirement, + use: useSAML, }, } @@ -283,9 +320,16 @@ func migrateTogether(t *testing.T, b backend) { func migrateWithin(t *testing.T, ctx context.Context, limit time.Duration, s sqlStore, db *sql.DB) error { t.Helper() + return migrateOnWithin(t, ctx, limit, s, db, enginePostgres) +} + +// migrateOnWithin is migrateWithin on the engine e. +func migrateOnWithin(t *testing.T, ctx context.Context, limit time.Duration, s sqlStore, db *sql.DB, e engine) error { + t.Helper() + done := make(chan error, 1) - go func() { done <- s.migrate(ctx, db, enginePostgres) }() + go func() { done <- s.migrate(ctx, db, e) }() select { case err := <-done: @@ -390,3 +434,96 @@ func TestMigrateFailureCreatesNothing(t *testing.T) { }) } } + +// mysqlMigrationLock is the GET_LOCK name the Migrate of the store module +// takes on MySQL and MariaDB, as its package documentation names it: the +// module path without its host, then ".Migrate". +func mysqlMigrationLock(module string) string { + return strings.TrimPrefix(module, "github.com/") + ".Migrate" +} + +// holdNamedLock takes the MySQL named lock name on a connection of db of +// its own, which keeps it until released or closed. +func holdNamedLock(t *testing.T, db *sql.DB, name string) *sql.Conn { + t.Helper() + + conn, err := db.Conn(t.Context()) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + + var got int + require.NoError(t, conn.QueryRowContext(t.Context(), "SELECT GET_LOCK(?, 0)", name).Scan(&got)) + require.Equal(t, 1, got, "lock %s taken", name) + + return conn +} + +// requireLockFree asserts that no session holds the named lock. +func requireLockFree(t *testing.T, db *sql.DB, name string) { + t.Helper() + + var free int + require.NoError(t, db.QueryRowContext(t.Context(), "SELECT IS_FREE_LOCK(?)", name).Scan(&free)) + require.Equal(t, 1, free, "lock %s released", name) +} + +// TestMySQLMigrateWaitsForItsLock: on MySQL and MariaDB, Migrate creates +// its tables and checks them under the named lock its package +// documentation names (GET_LOCK), so that the Migrates of a deployment's +// instances run one after the other — concurrent CREATE TABLE IF NOT EXISTS +// of one table can fail with a metadata-lock deadlock. Held by another +// session, the lock makes Migrate wait — until its context's deadline, or +// its cancellation, having created nothing — while the Migrate of another +// store, whose lock is distinct, goes through. Once the holder releases +// it, Migrate creates the tables; after Migrate, failed or not, the lock +// is free again. +func TestMySQLMigrateWaitsForItsLock(t *testing.T) { + for _, b := range mysqlServers { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + for i, s := range sqlStores { + t.Run(s.name, func(t *testing.T) { + driver, dsn := b.namespace(t) + db := open(t, driver, dsn, 2) + admin := open(t, driver, dsn, 2) + name := mysqlMigrationLock(s.module) + holder := holdNamedLock(t, admin, name) + + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + + require.ErrorIs(t, migrateOnWithin(t, ctx, 10*time.Second, s, db, engineMySQL), context.DeadlineExceeded) + requireTables(t, db, engineMySQL, s, false) + + cancelled, cancelNow := context.WithCancel(t.Context()) + time.AfterFunc(500*time.Millisecond, cancelNow) + require.ErrorIs(t, migrateOnWithin(t, cancelled, 10*time.Second, s, db, engineMySQL), context.Canceled) + requireTables(t, db, engineMySQL, s, false) + + otherCtx, cancelOther := context.WithTimeout(t.Context(), 10*time.Second) + defer cancelOther() + + other := sqlStores[(i+1)%len(sqlStores)] + require.NoError(t, migrateOnWithin(t, otherCtx, 15*time.Second, other, db, engineMySQL), + "%s's lock is not %s's", other.name, s.name) + requireTables(t, db, engineMySQL, other, true) + + var released int + require.NoError(t, holder.QueryRowContext(t.Context(), "SELECT RELEASE_LOCK(?)", name).Scan(&released)) + require.Equal(t, 1, released) + + require.NoError(t, s.migrate(t.Context(), db, engineMySQL)) + requireTables(t, db, engineMySQL, s, true) + requireLockFree(t, admin, name) + + // A Migrate failing its check releases the lock too. + execAll(t, db, "DROP TABLE "+s.tables[0]) + execAll(t, db, strings.Replace(s.schema(engineMySQL)[0], "ENGINE=InnoDB", "ENGINE=MyISAM", 1)) + require.ErrorIs(t, s.migrate(t.Context(), db, engineMySQL), s.requirement) + requireLockFree(t, admin, name) + }) + } + }) + } +} diff --git a/internal/interop/sqlstores/oauth2_test.go b/internal/interop/sqlstores/oauth2_test.go index 4a3cd88b..bac30e3e 100644 --- a/internal/interop/sqlstores/oauth2_test.go +++ b/internal/interop/sqlstores/oauth2_test.go @@ -5,9 +5,8 @@ package sqlstores_test import ( + "context" "database/sql" - "os" - "path/filepath" "regexp" "strings" "testing" @@ -25,14 +24,90 @@ var mysql84FoundRows = backend{ name: "mysql84-client-found-rows", engine: engineMySQL, env: envMySQL84DSN, clientFoundRows: true, } -// conformancePool returns a pool on dsn sized for one conformance case: -// RunConformance builds a store per case on the parent test, so each pool -// keeps a single idle connection once its case is over. -func conformancePool(t *testing.T, driver, dsn string) *sql.DB { +// conformancePoolSize is the number of connections a conformance case +// races on: every one is open before the case starts. +const conformancePoolSize = 8 + +// conformancePools returns the pool opener of a RunConformance run: each +// call returns a pool on dsn whose conformancePoolSize connections are open +// and idle before the case starts, so the racers of the concurrency cases +// overlap for real instead of queuing on connection setup (with one warm +// connection, a check-then-act consumption passes them). RunConformance +// builds every case's store on the parent test, which keeps the pools +// until it ends: each call closes the pool of the previous case, so a run +// holds conformancePoolSize connections, not one set per case. That relies +// on RunConformance running its cases one after the other, as it does. +func conformancePools(t *testing.T) func(driver, dsn string) *sql.DB { t.Helper() - db := open(t, driver, dsn, 8) - db.SetMaxIdleConns(1) + var previous *sql.DB + + return func(driver, dsn string) *sql.DB { + if previous != nil { + _ = previous.Close() + } + + previous = warmPool(t, driver, dsn, conformancePoolSize) + + return previous + } +} + +// warmConnAttempts and warmConnTimeout bound the opening of each connection +// of a warmed pool: a connect that stalls — seen once through Docker +// Desktop's port forwarding — is tried again, and a server that never +// answers fails the case with a clear error instead of a hang. +const ( + warmConnAttempts = 3 + warmConnTimeout = 10 * time.Second +) + +// openConn opens a connection of db, each attempt bounded by +// warmConnTimeout, and returns the last error after warmConnAttempts. +func openConn(t *testing.T, db *sql.DB) (*sql.Conn, error) { + t.Helper() + + var err error + + for range warmConnAttempts { + ctx, cancel := context.WithTimeout(t.Context(), warmConnTimeout) + + var c *sql.Conn + + c, err = db.Conn(ctx) + + cancel() + + if err == nil { + return c, nil + } + } + + return nil, err +} + +// warmPool returns a pool of size connections on dsn, every one of them +// opened — each held until all are, so that they are distinct — and +// returned to the pool idle. +func warmPool(t *testing.T, driver, dsn string, size int) *sql.DB { + t.Helper() + + db := open(t, driver, dsn, size) + conns := make([]*sql.Conn, 0, size) + + for i := range size { + c, err := openConn(t, db) + require.NoError(t, err, "opening connection %d of %d of the warmed pool (%d attempts of %s each)", + i+1, size, warmConnAttempts, warmConnTimeout) + + conns = append(conns, c) + } + + for _, c := range conns { + require.NoError(t, c.Close()) + } + + require.Equal(t, size, db.Stats().Idle, "idle connections of the warmed pool") return db } @@ -46,10 +121,12 @@ func TestOAuth2Conformance(t *testing.T) { t.Run(b.name, func(t *testing.T) { b.server(t) + pool := conformancePools(t) + storetest.RunConformance(t, func() oauth2.Storage { driver, dsn := b.namespace(t) - s, err := oauth2sql.New(conformancePool(t, driver, dsn), oauth2Dialect(b.engine)) + s, err := oauth2sql.New(pool(driver, dsn), oauth2Dialect(b.engine)) require.NoError(t, err) require.NoError(t, s.Migrate(t.Context())) @@ -120,21 +197,21 @@ const upgradeSection = "### `oauth2/store/sql`: MySQL compares keys byte for byt var sqlBlock = regexp.MustCompile("(?s)```sql\n(.*?)```") // upgradeBlocks returns the SQL blocks of the upgrade section of -// MIGRATION.md, each as its statements: comment lines dropped, split at -// the semicolons ending a line. +// MIGRATION.md, each as its statements. func upgradeBlocks(t *testing.T) [][]string { t.Helper() - raw, err := os.ReadFile(filepath.Join("..", "..", "..", "MIGRATION.md")) - require.NoError(t, err) + return sectionBlocks(t, upgradeSection) +} - doc := string(raw) - start := strings.Index(doc, "\n"+upgradeSection+"\n") - require.GreaterOrEqual(t, start, 0, "MIGRATION.md has no section %q", upgradeSection) +// sqlBlocks returns the SQL blocks of section — MIGRATION.md from the +// heading on, up to the next heading of its level — each as its +// statements: comment lines dropped, split at the semicolons ending a line. +func sqlBlocks(t *testing.T, section, heading string) [][]string { + t.Helper() - section := doc[start+1:] - if end := strings.Index(section[len(upgradeSection):], "\n### "); end >= 0 { - section = section[:len(upgradeSection)+end] + if end := strings.Index(section[len(heading):], "\n### "); end >= 0 { + section = section[:len(heading)+end] } var blocks [][]string @@ -276,9 +353,11 @@ func TestOAuth2MySQLUpgrade(t *testing.T) { }) t.Run("conformance", func(t *testing.T) { + pool := conformancePools(t) + storetest.RunConformance(t, func() oauth2.Storage { driver, dsn := b.namespace(t) - db := conformancePool(t, driver, dsn) + db := pool(driver, dsn) execAll(t, db, legacyMySQLSchema...) execAll(t, db, upgrade...) diff --git a/internal/interop/sqlstores/purge_test.go b/internal/interop/sqlstores/purge_test.go new file mode 100644 index 00000000..d6bd79c1 --- /dev/null +++ b/internal/interop/sqlstores/purge_test.go @@ -0,0 +1,359 @@ +// 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 sqlstores_test + +import ( + "context" + "database/sql" + "strings" + "testing" + "time" + + mysqldriver "github.com/go-sql-driver/mysql" + "github.com/hyperscale-stack/security/oauth2" + oauth2sql "github.com/hyperscale-stack/security/oauth2/store/sql" + oidcclient "github.com/hyperscale-stack/security/oidc/client" + oidcsql "github.com/hyperscale-stack/security/oidc/client/store/sql" + samlsp "github.com/hyperscale-stack/security/saml/sp" + samlsql "github.com/hyperscale-stack/security/saml/sp/store/sql" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// allServers are every backend the purges run on. +var allServers = []backend{sqliteFile, postgres, mysql80, mysql84, mariadb} + +// expiringPair returns a pair of family whose tokens expire at expiresAt. +func expiringPair(name, family string, expiresAt time.Time) *oauth2.TokenPair { + pair := tokenPair(name, family) + pair.Access.ExpiresAt, pair.Refresh.ExpiresAt = expiresAt, expiresAt + + return pair +} + +// expiringCode returns a code expiring at expiresAt. +func expiringCode(hash string, expiresAt time.Time) *oauth2.AuthorizationCode { + return &oauth2.AuthorizationCode{ + CodeHash: hash, ClientID: "client-1", Subject: "alice", RedirectURI: "https://app.example/cb", Scope: "read", + IssuedAt: expiresAt.Add(-time.Minute), ExpiresAt: expiresAt, + } +} + +// tableRows returns the number of rows of table. +func tableRows(t *testing.T, db *sql.DB, table string) int { + t.Helper() + + var n int + require.NoError(t, db.QueryRowContext(t.Context(), "SELECT COUNT(*) FROM "+table).Scan(&n)) + + return n +} + +// TestOAuth2PurgeExpired runs PurgeExpired on every server: the rows +// expired before now go — two codes, three access tokens, two refresh +// tokens, one family — and the rows expiring at now, the live ones and the +// record of a revoked family within its retention stay; the live pair is +// still usable, the revoked family still refuses issuance. +func TestOAuth2PurgeExpired(t *testing.T) { + for _, b := range allServers { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + ctx := t.Context() + driver, dsn := b.namespace(t) + db := open(t, driver, dsn, 4) + + s, err := oauth2sql.New(db, oauth2Dialect(b.engine)) + require.NoError(t, err) + require.NoError(t, s.Migrate(ctx)) + + now := time.Unix(time.Now().Unix(), 0) + past := now.Add(-time.Hour) + + for _, code := range []*oauth2.AuthorizationCode{ + expiringCode("code-expired", past), expiringCode("code-consumed", past), + expiringCode("code-boundary", now), expiringCode("code-live", now.Add(time.Hour)), + } { + require.NoError(t, s.SaveAuthorizationCode(ctx, code)) + } + + _, err = s.ConsumeAuthorizationCode(ctx, "code-consumed") + require.NoError(t, err) + + cc := expiringPair("purge-cc", "", past) + cc.Refresh = nil + live := expiringPair("purge-live", "fam-purge-live", now.Add(time.Hour)) + + for _, pair := range []*oauth2.TokenPair{ + expiringPair("purge-expired", "fam-purge-expired", past), cc, live, + expiringPair("purge-revoked", "fam-purge-revoked", past), + expiringPair("purge-boundary", "fam-purge-boundary", now), + } { + require.NoError(t, s.SaveTokenPair(ctx, pair)) + } + + require.NoError(t, s.RevokeRefreshFamily(ctx, "fam-purge-revoked")) + + n, err := s.PurgeExpired(ctx, now) + require.NoError(t, err) + assert.Equal(t, int64(8), n) + + for table, rows := range map[string]int{ + "oauth2_auth_codes": 2, "oauth2_access_tokens": 2, "oauth2_refresh_tokens": 2, "oauth2_token_families": 3, + } { + assert.Equal(t, rows, tableRows(t, db, table), table) + } + + _, err = s.LookupAccessToken(ctx, live.Access.TokenHash) + require.NoError(t, err) + require.ErrorIs(t, s.SaveTokenPair(ctx, expiringPair("purge-again", "fam-purge-revoked", now.Add(time.Hour))), + oauth2.ErrTokenFamilyRevoked) + + n, err = s.PurgeExpired(ctx, now) + require.NoError(t, err) + assert.Zero(t, n) + }) + } +} + +// TestSAMLPurgeExpired runs the SAML store's PurgeExpired on every server: +// the transaction and the replay mark expired before now go, those +// expiring at now and the live ones stay. +func TestSAMLPurgeExpired(t *testing.T) { + for _, b := range allServers { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + ctx := t.Context() + driver, dsn := b.namespace(t) + db := open(t, driver, dsn, 2) + + s, err := samlsql.New(db, samlDialect(b.engine)) + require.NoError(t, err) + require.NoError(t, s.Migrate(ctx)) + + now := time.Unix(time.Now().Unix(), 0) + + for label, expires := range map[string]time.Time{ + "expired": now.Add(-time.Hour), "boundary": now, "live": now.Add(time.Hour), + } { + require.NoError(t, s.SaveTransaction(ctx, &samlsp.TransactionRecord{ + ID: sha256Hex("tx/" + label), BindingHash: sha256Hex("b/" + label), Payload: `{"v":1}`, + ExpiresAt: expires, Integrity: sha256Hex("i/" + label), + })) + require.NoError(t, s.MarkAssertionUsed(ctx, sha256Hex("as/"+label), expires)) + } + + n, err := s.PurgeExpired(ctx, now) + require.NoError(t, err) + assert.Equal(t, int64(2), n) + assert.Equal(t, 2, tableRows(t, db, "samlsp_transactions")) + assert.Equal(t, 2, tableRows(t, db, "samlsp_used_assertions")) + }) + } +} + +// TestOAuth2PurgeKeepsAFamilyExtendedMeanwhile: a revocation extends the +// retention of an expired family while a purge runs. The purge waits for +// the family's row, then re-checks its expiry on the committed row and +// keeps it: a revoked family deleted before its new expiry would be +// recorded active again by the next issuance into it. (On PostgreSQL the +// batch picks its rows in a subquery, which a wait does not re-run: the +// outer DELETE repeats the expiry.) +func TestOAuth2PurgeKeepsAFamilyExtendedMeanwhile(t *testing.T) { + for _, b := range lockingServers { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + store, db, admin := lockingStore(t, b) + now := time.Unix(time.Now().Unix(), 0) + + require.NoError(t, store.SaveTokenPair(t.Context(), expiringPair("extended", "fam-extended", now.Add(-time.Hour)))) + + holder := holdLocks(t, db, b.engine, stmt{ + `UPDATE oauth2_token_families SET revoked_at = ?, expires_at = ? WHERE family_id = ?`, + []any{now.Unix(), now.Add(oauth2.FamilyRevocationRetention).Unix(), "fam-extended"}, + }) + + done := make(chan int64, 1) + + go func() { + n, err := store.PurgeExpired(context.Background(), now) + assert.NoError(t, err) + + done <- n + }() + + holder.waitForWaiter(admin) + holder.commit() + + assert.Equal(t, int64(2), <-done, "the expired tokens go, the family stays") + require.Equal(t, 1, countRows(t, db, b.engine, "oauth2_token_families", "family_id", "fam-extended")) + require.ErrorIs(t, store.SaveTokenPair(t.Context(), expiringPair("extended-again", "fam-extended", now.Add(time.Hour))), + oauth2.ErrTokenFamilyRevoked) + }) + } +} + +// purgeTarget is a store whose purge TestPurgeDoesNotBlockInserts runs: it +// stores records expired under keys sorting around a live one, then must +// insert a record under a key sorting before all of them while a purge +// waits for the live one's row. +type purgeTarget struct { + name string + // table and column hold the records and their keys. + table, column string + // setup migrates the store on db and returns its purge and its + // insert, the latter bound to the pool insertDB. + setup func(t *testing.T, db, insertDB *sql.DB) (purge func(now time.Time) error, insert func(key string) error) + // save stores a record under key, expiring at expiresAt. + save func(t *testing.T, db *sql.DB, key string, expiresAt time.Time) +} + +// hexKey is a 64-hex-digit store key starting with digit, so that keys +// sort by it. +func hexKey(digit string) string { return digit + strings.Repeat("0", 63) } + +var purgeTargets = []purgeTarget{ + { + name: "oauth2", table: "oauth2_auth_codes", column: "code_hash", + setup: func(t *testing.T, db, insertDB *sql.DB) (func(time.Time) error, func(string) error) { + s, err := oauth2sql.New(db, oauth2sql.MySQL) + require.NoError(t, err) + require.NoError(t, s.Migrate(t.Context())) + + inserter, err := oauth2sql.New(insertDB, oauth2sql.MySQL) + require.NoError(t, err) + + return func(now time.Time) error { + _, err := s.PurgeExpired(context.Background(), now) + + return err + }, func(key string) error { + return inserter.SaveAuthorizationCode(context.Background(), expiringCode(key, time.Now().Add(time.Hour))) + } + }, + save: func(t *testing.T, db *sql.DB, key string, expiresAt time.Time) { + s, err := oauth2sql.New(db, oauth2sql.MySQL) + require.NoError(t, err) + require.NoError(t, s.SaveAuthorizationCode(t.Context(), expiringCode(key, expiresAt))) + }, + }, + { + name: "oidc", table: "oidc_client_transactions", column: "state_hash", + setup: func(t *testing.T, db, insertDB *sql.DB) (func(time.Time) error, func(string) error) { + s, err := oidcsql.New(db, oidcsql.MySQL) + require.NoError(t, err) + require.NoError(t, s.Migrate(t.Context())) + + inserter, err := oidcsql.New(insertDB, oidcsql.MySQL) + require.NoError(t, err) + + return func(now time.Time) error { + _, err := s.PurgeExpired(context.Background(), now) + + return err + }, func(key string) error { + return inserter.Save(context.Background(), oidcTransaction(key, time.Now().Add(time.Hour))) + } + }, + save: func(t *testing.T, db *sql.DB, key string, expiresAt time.Time) { + s, err := oidcsql.New(db, oidcsql.MySQL) + require.NoError(t, err) + require.NoError(t, s.Save(t.Context(), oidcTransaction(key, expiresAt))) + }, + }, + { + name: "saml", table: "samlsp_used_assertions", column: "assertion_key", + setup: func(t *testing.T, db, insertDB *sql.DB) (func(time.Time) error, func(string) error) { + s, err := samlsql.New(db, samlsql.MySQL) + require.NoError(t, err) + require.NoError(t, s.Migrate(t.Context())) + + inserter, err := samlsql.New(insertDB, samlsql.MySQL) + require.NoError(t, err) + + return func(now time.Time) error { + _, err := s.PurgeExpired(context.Background(), now) + + return err + }, func(key string) error { + return inserter.MarkAssertionUsed(context.Background(), key, + time.Unix(time.Now().Add(time.Hour).Unix(), 0)) + } + }, + save: func(t *testing.T, db *sql.DB, key string, expiresAt time.Time) { + s, err := samlsql.New(db, samlsql.MySQL) + require.NoError(t, err) + require.NoError(t, s.MarkAssertionUsed(t.Context(), key, expiresAt)) + }, + }, +} + +// oidcTransaction returns a transaction stored under stateHash, expiring +// at expiresAt. +func oidcTransaction(stateHash string, expiresAt time.Time) *oidcclient.Transaction { + expires := time.Unix(expiresAt.Unix(), 0).UTC() + + return &oidcclient.Transaction{ + StateHash: stateHash, BindingHash: sha256Hex("binding-" + stateHash), + ConnectionID: "acme", Issuer: "https://idp.example.com", ClientID: "client-1", + RedirectURI: "https://rp.example.com/oidc/acme/callback", ResponseMode: oidcclient.ResponseModeQuery, + MaxAge: oidcclient.NoMaxAge, CreatedAt: expires.Add(-time.Hour), ExpiresAt: expires, + Integrity: sha256Hex("integrity-" + stateHash), + } +} + +// TestPurgeDoesNotBlockInserts: on MySQL, a purge — which scans its table +// when expires_at has no index, as MySQL creates none — waits for a live +// row another transaction holds. Meanwhile a new record must still be +// inserted, under a key sorting before every scanned one: at READ +// COMMITTED the purge locks only the rows it deletes, where at the +// server's REPEATABLE READ it would also lock the gaps it scanned, and the +// insert would time out (innodb_lock_wait_timeout, here 1 s). +func TestPurgeDoesNotBlockInserts(t *testing.T) { + for _, b := range mysqlServers { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + for _, target := range purgeTargets { + t.Run(target.name, func(t *testing.T) { + driver, dsn := b.namespace(t) + db := open(t, driver, dsn, 4) + admin := open(t, driver, dsn, 1) + + cfg, err := mysqldriver.ParseDSN(dsn) + require.NoError(t, err) + + cfg.Params = map[string]string{"innodb_lock_wait_timeout": "1"} + insertDB := open(t, driver, cfg.FormatDSN(), 1) + + purge, insert := target.setup(t, db, insertDB) + + now := time.Unix(time.Now().Unix(), 0) + target.save(t, db, hexKey("2"), now.Add(-time.Hour)) + target.save(t, db, hexKey("8"), now.Add(time.Hour)) + target.save(t, db, hexKey("e"), now.Add(-time.Hour)) + + holder := holdLocks(t, db, engineMySQL, stmt{ + "SELECT " + target.column + " FROM " + target.table + " WHERE " + target.column + " = ? FOR UPDATE", + []any{hexKey("8")}, + }) + + done := make(chan error, 1) + + go func() { done <- purge(now) }() + + holder.waitForWaiter(admin) + require.NoError(t, insert(hexKey("0")), "an insert while the purge waits") + + holder.commit() + require.NoError(t, <-done) + require.Equal(t, 2, tableRows(t, db, target.table), "the live and the new record stay") + }) + } + }) + } +} diff --git a/oauth2/store/redis/deployment.go b/oauth2/store/redis/deployment.go new file mode 100644 index 00000000..f11d6922 --- /dev/null +++ b/oauth2/store/redis/deployment.go @@ -0,0 +1,138 @@ +// 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 + +import ( + "context" + "errors" + "fmt" + "strconv" + "strings" + + "github.com/redis/go-redis/v9" +) + +// ErrDeploymentRequirement reports a deployment requirement of the package +// documentation that the Redis client or server does not meet: [New] +// returns it for a client that spreads keys over several servers, +// [Store.CheckDeployment] for each requirement the server does not meet. +var ErrDeploymentRequirement = errors.New("redisstore: deployment requirement not met") + +// shardedClient reports why rdb cannot serve the store, or nil: the +// scripts touch several keys — a token, its family, its replacement — that +// a Redis Cluster puts in different hash slots (CROSSSLOT), and a Ring on +// different shards, where a token is found only when it shares its +// family's shard. +func shardedClient(rdb redis.UniversalClient) error { + switch rdb.(type) { + case *redis.ClusterClient: + return fmt.Errorf("%w: a Redis Cluster client: the scripts touch keys of several hash slots", ErrDeploymentRequirement) + case *redis.Ring: + return fmt.Errorf("%w: a Redis Ring client: the scripts touch keys of several shards", ErrDeploymentRequirement) + default: + return nil + } +} + +// infoSections are the INFO sections CheckDeployment reads, one at a time: +// several sections in one INFO need Redis 7. +var infoSections = []string{"server", "memory", "cluster"} + +// CheckDeployment verifies, through INFO, the deployment requirements of +// the package documentation that the server reports: +// +// - Redis 6.0 or later (redis_version): the scripts write with SET … +// KEEPTTL; +// - maxmemory-policy noeviction (maxmemory_policy): every other policy +// may evict a family key, and a "revoked" one lost within its retention +// lets the next issuance revive the family; +// - no cluster mode (cluster_enabled 0). +// +// It returns every requirement not met, each matching +// [ErrDeploymentRequirement] — a field the server does not report counts +// as not met: verify that requirement by hand and skip the call. When INFO +// itself fails it returns that error, which does not match the sentinel: +// INFO is in the @dangerous ACL category, so a least-privilege user needs +// +info to run the check (NOPERM otherwise). Call it at boot, before +// serving traffic. A proxy sharding keys behind one endpoint (twemproxy, a +// non-OSS cluster policy) reports cluster_enabled 0: the scripts then fail +// at runtime, closed. +func (s *Store) CheckDeployment(ctx context.Context) error { + fields := map[string]string{} + + for _, section := range infoSections { + reply, err := s.rdb.Info(ctx, section).Result() + if err != nil { + return fmt.Errorf("redisstore: check deployment: INFO %s: %w", section, err) + } + + parseInfo(reply, fields) + } + + return checkInfo(fields) +} + +// parseInfo adds the "field:value" lines of an INFO reply to fields. +func parseInfo(reply string, fields map[string]string) { + for line := range strings.Lines(reply) { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + + if field, value, ok := strings.Cut(line, ":"); ok { + fields[field] = value + } + } +} + +// checkInfo checks the INFO fields against the requirements and returns +// every one not met. +func checkInfo(fields map[string]string) error { + var errs []error + + version, ok := fields["redis_version"] + major, valid := versionMajor(version) + + switch { + case !ok: + errs = append(errs, notReported("redis_version")) + case !valid: + errs = append(errs, fmt.Errorf("%w: redis_version %s is not a version", ErrDeploymentRequirement, version)) + case major < 6: + errs = append(errs, fmt.Errorf("%w: Redis %s is older than 6.0 (SET … KEEPTTL)", ErrDeploymentRequirement, version)) + } + + switch policy, ok := fields["maxmemory_policy"]; { + case !ok: + errs = append(errs, notReported("maxmemory_policy")) + case policy != "noeviction": + errs = append(errs, fmt.Errorf("%w: maxmemory_policy is %s, not noeviction: a revoked token family may be evicted", + ErrDeploymentRequirement, policy)) + } + + switch cluster, ok := fields["cluster_enabled"]; { + case !ok: + errs = append(errs, notReported("cluster_enabled")) + case cluster != "0": + errs = append(errs, fmt.Errorf("%w: cluster_enabled is %s: Redis Cluster is not supported", ErrDeploymentRequirement, cluster)) + } + + return errors.Join(errs...) +} + +// notReported is the requirement on field, which the server did not report. +func notReported(field string) error { + return fmt.Errorf("%w: %s not reported by the server", ErrDeploymentRequirement, field) +} + +// versionMajor returns the major number of a version — "7.2.4", +// "8.0.0-rc1" — and whether it has one. +func versionMajor(version string) (int, bool) { + major, _, _ := strings.Cut(version, ".") + n, err := strconv.Atoi(major) + + return n, err == nil +} diff --git a/oauth2/store/redis/deployment_test.go b/oauth2/store/redis/deployment_test.go new file mode 100644 index 00000000..70c5b87d --- /dev/null +++ b/oauth2/store/redis/deployment_test.go @@ -0,0 +1,202 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package redisstore_test + +import ( + "context" + "crypto/rand" + "encoding/hex" + "errors" + "os" + "testing" + + 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" +) + +// TestNewRefusesShardedClients: the scripts touch several keys — a token, +// its family, its replacement — that a Redis Cluster puts in different hash +// slots (CROSSSLOT) and a Ring on different shards (a token found only when +// it shares its family's shard): New refuses both clients, without I/O. +func TestNewRefusesShardedClients(t *testing.T) { + t.Parallel() + + cluster := redis.NewClusterClient(&redis.ClusterOptions{Addrs: []string{"127.0.0.1:1"}}) + t.Cleanup(func() { _ = cluster.Close() }) + + ring := redis.NewRing(&redis.RingOptions{Addrs: map[string]string{"a": "127.0.0.1:1", "b": "127.0.0.1:2"}}) + t.Cleanup(func() { _ = ring.Close() }) + + for name, client := range map[string]redis.UniversalClient{"cluster": cluster, "ring": ring} { + store, err := redisstore.New(client) + require.ErrorIs(t, err, redisstore.ErrDeploymentRequirement, name) + assert.Nil(t, store, name) + } + + // A standalone or Sentinel (failover) client is a *redis.Client. + failover := redis.NewFailoverClient(&redis.FailoverOptions{MasterName: "m", SentinelAddrs: []string{"127.0.0.1:1"}}) + t.Cleanup(func() { _ = failover.Close() }) + + _, err := redisstore.New(failover) + require.NoError(t, err) +} + +// infoHook answers INFO from canned sections, or with err, without +// reaching a server: the INFO fields miniredis does not report. +type infoHook struct { + sections map[string]string + err error +} + +func (h infoHook) DialHook(next redis.DialHook) redis.DialHook { return next } + +func (h infoHook) ProcessPipelineHook(next redis.ProcessPipelineHook) redis.ProcessPipelineHook { + return next +} + +func (h infoHook) ProcessHook(next redis.ProcessHook) redis.ProcessHook { + return func(ctx context.Context, cmd redis.Cmder) error { + if cmd.Name() != "info" { + return next(ctx, cmd) + } + + info, ok := cmd.(*redis.StringCmd) + if !ok { + return next(ctx, cmd) + } + + if h.err != nil { + info.SetErr(h.err) + + return h.err + } + + section, _ := cmd.Args()[1].(string) + info.SetVal(h.sections[section]) + + return nil + } +} + +// infoStore returns a store whose INFO replies are sections. +func infoStore(t *testing.T, sections map[string]string, err error) *redisstore.Store { + t.Helper() + + client := redis.NewClient(&redis.Options{Addr: "127.0.0.1:1"}) + client.AddHook(infoHook{sections: sections, err: err}) + t.Cleanup(func() { _ = client.Close() }) + + store, nerr := redisstore.New(client) + require.NoError(t, nerr) + + return store +} + +// compliantInfo returns the INFO sections of a compliant server: Redis +// version, the noeviction policy, no cluster mode. +func compliantInfo(version, policy, cluster string) map[string]string { + return map[string]string{ + "server": "# Server\r\nredis_version:" + version + "\r\nredis_mode:standalone\r\n", + "memory": "# Memory\r\nused_memory:1000\r\nmaxmemory:0\r\nmaxmemory_policy:" + policy + "\r\n", + "cluster": "# Cluster\r\ncluster_enabled:" + cluster + "\r\n", + } +} + +// TestCheckDeployment checks the INFO fields against the requirements: +// Redis 6.0 or later, maxmemory-policy noeviction, no cluster mode; every +// requirement not met is reported, each matching ErrDeploymentRequirement. +func TestCheckDeployment(t *testing.T) { + t.Parallel() + + for _, version := range []string{"6.0.0", "6.2.14", "7.2.4", "7.4.11", "10.0.1", "8.0.0-rc1", "7"} { + require.NoError(t, infoStore(t, compliantInfo(version, "noeviction", "0"), nil).CheckDeployment(t.Context()), version) + } + + for name, c := range map[string]struct { + sections map[string]string + want []string + }{ + "Redis 5": {compliantInfo("5.0.14", "noeviction", "0"), []string{"Redis 5.0.14 is older than 6.0"}}, + "Redis 4.0": {compliantInfo("4.0.0", "noeviction", "0"), []string{"Redis 4.0.0 is older than 6.0"}}, + "no version": {compliantInfo("unknown", "noeviction", "0"), []string{"redis_version unknown is not a version"}}, + "no major": {compliantInfo(".5", "noeviction", "0"), []string{"redis_version .5 is not a version"}}, + "allkeys-lru": {compliantInfo("7.4.11", "allkeys-lru", "0"), []string{"maxmemory_policy is allkeys-lru, not noeviction"}}, + "volatile-lru": {compliantInfo("7.4.11", "volatile-lru", "0"), []string{"maxmemory_policy is volatile-lru, not noeviction"}}, + "cluster": {compliantInfo("7.4.11", "noeviction", "1"), []string{"cluster_enabled is 1"}}, + "all three": {compliantInfo("5.0.14", "volatile-ttl", "1"), []string{ + "older than 6.0", "maxmemory_policy is volatile-ttl", "cluster_enabled is 1", + }}, + "not reported": {map[string]string{"server": "# Server\r\n", "memory": "", "cluster": "# Cluster\r\n"}, []string{ + "redis_version not reported", "maxmemory_policy not reported", "cluster_enabled not reported", + }}, + } { + err := infoStore(t, c.sections, nil).CheckDeployment(t.Context()) + require.ErrorIs(t, err, redisstore.ErrDeploymentRequirement, name) + + for _, want := range c.want { + assert.Contains(t, err.Error(), want, name) + } + } +} + +// TestCheckDeploymentReturnsBackendErrors: when INFO fails — a user +// without +info gets NOPERM, a server reports no such section — the error +// is the backend's, not a requirement found unmet. +func TestCheckDeploymentReturnsBackendErrors(t *testing.T) { + t.Parallel() + + noperm := errors.New("NOPERM User app has no permissions to run the 'info' command") + + err := infoStore(t, nil, noperm).CheckDeployment(t.Context()) + require.ErrorIs(t, err, noperm) + require.NotErrorIs(t, err, redisstore.ErrDeploymentRequirement) + + // miniredis answers INFO for its clients and stats sections only. + store, ok := newRedisStore(t).(*redisstore.Store) + require.True(t, ok) + + err = store.CheckDeployment(t.Context()) + require.Error(t, err) + require.NotErrorIs(t, err, redisstore.ErrDeploymentRequirement) +} + +// TestCheckDeploymentRealServer runs the check on a real server +// (OAUTH2_TEST_REDIS_ADDR), which meets the requirements, and as a +// least-privilege ACL user without +info, which gets NOPERM. +func TestCheckDeploymentRealServer(t *testing.T) { + addr := os.Getenv(envRedisAddr) + if addr == "" { + t.Skipf("set %s to run against a real Redis (opt-in)", envRedisAddr) + } + + ctx := context.Background() + admin := redis.NewClient(&redis.Options{Addr: addr}) + t.Cleanup(func() { _ = admin.Close() }) + + store, err := redisstore.New(admin) + require.NoError(t, err) + require.NoError(t, store.CheckDeployment(ctx)) + + b := make([]byte, 8) + _, _ = rand.Read(b) + user, password := "oauth2-check-"+hex.EncodeToString(b), hex.EncodeToString(b) + + require.NoError(t, admin.Do(ctx, "ACL", "SETUSER", user, "on", ">"+password, "~oauth2:*", + "+@read", "+@write", "+@scripting", "+@connection").Err()) + t.Cleanup(func() { _ = admin.Do(context.Background(), "ACL", "DELUSER", user).Err() }) + + limited := redis.NewClient(&redis.Options{Addr: addr, Username: user, Password: password}) + t.Cleanup(func() { _ = limited.Close() }) + + store, err = redisstore.New(limited) + require.NoError(t, err) + + err = store.CheckDeployment(ctx) + require.Error(t, err) + require.NotErrorIs(t, err, redisstore.ErrDeploymentRequirement) + require.Contains(t, err.Error(), "NOPERM") +} diff --git a/oauth2/store/redis/doc.go b/oauth2/store/redis/doc.go index de438f7f..54eee360 100644 --- a/oauth2/store/redis/doc.go +++ b/oauth2/store/redis/doc.go @@ -29,14 +29,18 @@ // # Deployment requirements // // The token-family guarantees of oauth2.Storage hold under these -// conditions: +// conditions. New refuses a Redis Cluster or Ring client; Store.CheckDeployment +// verifies, through INFO, the version, the eviction policy and the absence +// of cluster mode — call it at boot (the user needs +info). // // - 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). +// slots (CROSSSLOT); nor is a client-side Ring, which would spread them +// over shards. New refuses both clients; a proxy sharding keys behind +// one endpoint reports no cluster mode, and fails closed at runtime. // - Every command reaches the primary: no replica reads, and no // client-side caching on the store's client. Either could answer from // before a revocation. diff --git a/oauth2/store/redis/store.go b/oauth2/store/redis/store.go index cf1d371a..f2fffa1f 100644 --- a/oauth2/store/redis/store.go +++ b/oauth2/store/redis/store.go @@ -55,12 +55,20 @@ func WithKeyPrefix(prefix string) Option { } // New returns a [Store] bound to the given Redis client. The caller owns -// the client's lifecycle. +// the client's lifecycle. It refuses, with an error matching +// [ErrDeploymentRequirement], a *redis.ClusterClient or a *redis.Ring: the +// scripts touch keys those clients would spread over several hash slots or +// shards. A standalone server, or the primary of a Sentinel deployment +// (redis.NewFailoverClient), is served by a *redis.Client. func New(rdb redis.UniversalClient, opts ...Option) (*Store, error) { if rdb == nil { return nil, errors.New("redisstore: New: nil redis client") } + if err := shardedClient(rdb); err != nil { + return nil, fmt.Errorf("redisstore: New: %w", err) + } + s := &Store{rdb: rdb, prefix: "oauth2:"} for _, o := range opts { o(s) diff --git a/oauth2/store/sql/deployment.go b/oauth2/store/sql/deployment.go new file mode 100644 index 00000000..76420ba7 --- /dev/null +++ b/oauth2/store/sql/deployment.go @@ -0,0 +1,175 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "context" + "database/sql" + "errors" + "fmt" + "strings" +) + +// ErrDeploymentRequirement reports a deployment requirement of the package +// documentation that the database does not meet. [Store.CheckDeployment] +// and [Store.Migrate] wrap it with the requirement and the table, column +// or setting at fault. +var ErrDeploymentRequirement = errors.New("sqlstore: deployment requirement not met") + +// CheckDeployment verifies the deployment requirements of the package +// documentation that the database reports, and returns every one it finds +// unmet — each matching [ErrDeploymentRequirement] — or the error of the +// database. On MySQL and MariaDB: +// +// - every table of the store exists in the connection's database and +// uses InnoDB, whose transactions and row locks the token-family +// contract of [oauth2.Storage] rests on (atomic writes, I3); +// - the six key columns are VARBINARY, compared byte for byte; +// - the connection's sql_mode is strict (STRICT_TRANS_TABLES or +// STRICT_ALL_TABLES): a non-strict session truncates an over-long +// subject, client ID or extension data instead of refusing it. +// +// PostgreSQL and SQLite report nothing it checks (see [Postgres] and +// [SQLite]). [Store.Migrate] calls it once the tables exist; when you apply +// [Schema] with your own migration tool, call it at boot, before serving +// traffic. It reads information_schema and the session's sql_mode, on one +// pooled connection: set the strict mode for every connection — the +// server's default, or the DSN — never per session. +func (s *Store) CheckDeployment(ctx context.Context) error { + return s.dialect.checkDeployment(ctx, s.db) +} + +// querier runs the queries of a deployment check: the store's *sql.DB, or +// the *sql.Conn of a Migrate holding its named lock. +type querier interface { + QueryContext(ctx context.Context, query string, args ...any) (*sql.Rows, error) + QueryRowContext(ctx context.Context, query string, args ...any) *sql.Row +} + +// storeTables are the tables of the store, in [Schema]'s order. +var storeTables = []string{"oauth2_auth_codes", "oauth2_access_tokens", "oauth2_refresh_tokens", "oauth2_token_families"} + +// keyColumns are the columns the statements compare, as table.column: the +// columns [Schema] declares VARBINARY on MySQL. +var keyColumns = []string{ + "oauth2_auth_codes.code_hash", + "oauth2_access_tokens.token_hash", "oauth2_access_tokens.family_id", + "oauth2_refresh_tokens.token_hash", "oauth2_refresh_tokens.family_id", + "oauth2_token_families.family_id", +} + +// The statements of the MySQL check, on the tables of storeTables. +const ( + selectEngines = `SELECT TABLE_NAME, COALESCE(ENGINE, '') FROM information_schema.TABLES + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME IN (?, ?, ?, ?)` + selectColumnTypes = `SELECT CONCAT(TABLE_NAME, '.', COLUMN_NAME), DATA_TYPE FROM information_schema.COLUMNS + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME IN (?, ?, ?, ?)` + selectSQLMode = `SELECT @@SESSION.sql_mode` +) + +// checkMySQL reads what the requirements of a MySQL deployment depend on, +// then checks it. +func checkMySQL(ctx context.Context, q querier) error { + tables := make([]any, len(storeTables)) + for i, table := range storeTables { + tables[i] = table + } + + engines, err := queryPairs(ctx, q, selectEngines, tables...) + if err != nil { + return fmt.Errorf("sqlstore: check deployment: storage engines: %w", err) + } + + types, err := queryPairs(ctx, q, selectColumnTypes, tables...) + if err != nil { + return fmt.Errorf("sqlstore: check deployment: column types: %w", err) + } + + var mode string + if err := q.QueryRowContext(ctx, selectSQLMode).Scan(&mode); err != nil { + return fmt.Errorf("sqlstore: check deployment: sql_mode: %w", err) + } + + return errors.Join(checkEngines(storeTables, engines), checkKeyColumns(engines, types), checkSQLMode(mode)) +} + +// queryPairs runs query and returns its two-column rows as a map. +func queryPairs(ctx context.Context, q querier, query string, args ...any) (map[string]string, error) { + rows, err := q.QueryContext(ctx, query, args...) + if err != nil { + return nil, err //nolint:wrapcheck // wrapped by the caller + } + + defer func() { _ = rows.Close() }() + + pairs := map[string]string{} + + for rows.Next() { + var key, value string + if err := rows.Scan(&key, &value); err != nil { + return nil, err //nolint:wrapcheck // wrapped by the caller + } + + pairs[key] = value + } + + return pairs, rows.Err() //nolint:wrapcheck // wrapped by the caller +} + +// checkEngines reports each of tables that engines — table name → storage +// engine — does not list, or lists with another engine than InnoDB. +func checkEngines(tables []string, engines map[string]string) error { + var errs []error + + for _, table := range tables { + engine, ok := engines[table] + + switch { + case !ok: + errs = append(errs, fmt.Errorf("%w: table %s not found in the connection's database", ErrDeploymentRequirement, table)) + case !strings.EqualFold(engine, "InnoDB"): + errs = append(errs, fmt.Errorf("%w: table %s uses the %s engine, not InnoDB", ErrDeploymentRequirement, table, engine)) + } + } + + return errors.Join(errs...) +} + +// checkKeyColumns reports each key column of a table engines lists whose +// type — in types, table.column → type — is not varbinary. +func checkKeyColumns(engines, types map[string]string) error { + var errs []error + + for _, column := range keyColumns { + table, _, _ := strings.Cut(column, ".") + if _, ok := engines[table]; !ok { + continue // reported by checkEngines + } + + switch dataType, ok := types[column]; { + case !ok: + errs = append(errs, fmt.Errorf("%w: column %s not found", ErrDeploymentRequirement, column)) + case !strings.EqualFold(dataType, "varbinary"): + errs = append(errs, fmt.Errorf("%w: column %s is %s, not varbinary: keys would be compared under a collation", + ErrDeploymentRequirement, column, strings.ToLower(dataType))) + } + } + + return errors.Join(errs...) +} + +// checkSQLMode reports a sql_mode without STRICT_TRANS_TABLES nor +// STRICT_ALL_TABLES. +func checkSQLMode(mode string) error { + for flag := range strings.SplitSeq(mode, ",") { + switch strings.ToUpper(strings.TrimSpace(flag)) { + case "STRICT_TRANS_TABLES", "STRICT_ALL_TABLES": + return nil + } + } + + return fmt.Errorf("%w: sql_mode %q is not strict (STRICT_TRANS_TABLES or STRICT_ALL_TABLES): an over-long value would be truncated", + ErrDeploymentRequirement, mode) +} diff --git a/oauth2/store/sql/deployment_internal_test.go b/oauth2/store/sql/deployment_internal_test.go new file mode 100644 index 00000000..02a987e3 --- /dev/null +++ b/oauth2/store/sql/deployment_internal_test.go @@ -0,0 +1,154 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "database/sql" + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + _ "modernc.org/sqlite" +) + +// innoDB is the engine of every table in a compliant MySQL database. +func innoDB() map[string]string { + engines := map[string]string{} + for _, table := range storeTables { + engines[table] = "InnoDB" + } + + return engines +} + +// varbinary is the type of every key column in a compliant database. +func varbinary() map[string]string { + types := map[string]string{"oauth2_auth_codes.client_id": "varchar"} + for _, column := range keyColumns { + types[column] = "varbinary" + } + + return types +} + +// requireUnmet asserts that err reports requirements not met, each of want +// matching ErrDeploymentRequirement on a line of its own. +func requireUnmet(t *testing.T, err error, want ...string) { + t.Helper() + + require.ErrorIs(t, err, ErrDeploymentRequirement) + + lines := strings.Split(err.Error(), "\n") + require.Len(t, lines, len(want), err.Error()) + + for i, w := range want { + assert.Equal(t, "sqlstore: deployment requirement not met: "+w, lines[i]) + } +} + +func TestCheckEngines(t *testing.T) { + t.Parallel() + + require.NoError(t, checkEngines(storeTables, innoDB())) + + engines := innoDB() + engines["oauth2_auth_codes"] = "innodb" // as a server may spell it + require.NoError(t, checkEngines(storeTables, engines)) + + engines["oauth2_access_tokens"] = "MyISAM" + engines["oauth2_token_families"] = "MEMORY" + delete(engines, "oauth2_refresh_tokens") + + requireUnmet(t, checkEngines(storeTables, engines), + "table oauth2_access_tokens uses the MyISAM engine, not InnoDB", + "table oauth2_refresh_tokens not found in the connection's database", + "table oauth2_token_families uses the MEMORY engine, not InnoDB") + + requireUnmet(t, checkEngines(storeTables, map[string]string{}), + "table oauth2_auth_codes not found in the connection's database", + "table oauth2_access_tokens not found in the connection's database", + "table oauth2_refresh_tokens not found in the connection's database", + "table oauth2_token_families not found in the connection's database") +} + +func TestCheckKeyColumns(t *testing.T) { + t.Parallel() + + require.NoError(t, checkKeyColumns(innoDB(), varbinary())) + + types := varbinary() + types["oauth2_auth_codes.code_hash"] = "VARCHAR" + types["oauth2_token_families.family_id"] = "char" + delete(types, "oauth2_refresh_tokens.family_id") + + requireUnmet(t, checkKeyColumns(innoDB(), types), + "column oauth2_auth_codes.code_hash is varchar, not varbinary: keys would be compared under a collation", + "column oauth2_refresh_tokens.family_id not found", + "column oauth2_token_families.family_id is char, not varbinary: keys would be compared under a collation") + + // The columns of a missing table are reported with the table, once. + engines := innoDB() + delete(engines, "oauth2_access_tokens") + require.NoError(t, checkKeyColumns(engines, map[string]string{ + "oauth2_auth_codes.code_hash": "varbinary", + "oauth2_refresh_tokens.token_hash": "varbinary", + "oauth2_refresh_tokens.family_id": "varbinary", + "oauth2_token_families.family_id": "varbinary", + "oauth2_access_tokens.token_hash": "varchar", + "unrelated_table.unrelated_column": "varchar", + "oauth2_access_tokens.family_id": "varchar", + "oauth2_auth_codes.redirect_uri": "text", + "oauth2_refresh_tokens.extra_field": "int", + })) +} + +func TestCheckSQLMode(t *testing.T) { + t.Parallel() + + for _, mode := range []string{ + "STRICT_TRANS_TABLES", + "STRICT_ALL_TABLES", + "ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION", + "STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,TRADITIONAL,NO_ENGINE_SUBSTITUTION", + "strict_all_tables", + "ANSI_QUOTES, STRICT_TRANS_TABLES", + } { + require.NoError(t, checkSQLMode(mode), mode) + } + + for _, mode := range []string{"", "NO_ENGINE_SUBSTITUTION", "ANSI_QUOTES,NO_ZERO_DATE", "STRICT", "NOT_STRICT_TRANS_TABLES"} { + err := checkSQLMode(mode) + requireUnmet(t, err, "sql_mode \""+mode+ + "\" is not strict (STRICT_TRANS_TABLES or STRICT_ALL_TABLES): an over-long value would be truncated") + } +} + +// TestCheckDeploymentPerDialect: PostgreSQL and SQLite report nothing the +// store checks; MySQL's check reaches the database, whose failure is +// returned as such, not as a requirement found unmet. +func TestCheckDeploymentPerDialect(t *testing.T) { + t.Parallel() + + db, err := sql.Open("sqlite", ":memory:") + require.NoError(t, err) + require.NoError(t, db.Close()) + + for _, d := range []Dialect{Postgres, SQLite} { + store, err := New(db, d) + require.NoError(t, err) + require.NoError(t, store.CheckDeployment(t.Context()), d.Name()) + assert.Empty(t, d.tableOptions(), d.Name()) + } + + store, err := New(db, MySQL) + require.NoError(t, err) + + err = store.CheckDeployment(t.Context()) + require.ErrorContains(t, err, "sqlstore: check deployment: storage engines: ") + require.False(t, errors.Is(err, ErrDeploymentRequirement), "a database failure is not a requirement found unmet") + assert.Equal(t, " ENGINE=InnoDB DEFAULT CHARSET=utf8mb4", MySQL.tableOptions()) +} diff --git a/oauth2/store/sql/dialect.go b/oauth2/store/sql/dialect.go index 76a6ea4b..37750c6b 100644 --- a/oauth2/store/sql/dialect.go +++ b/oauth2/store/sql/dialect.go @@ -5,6 +5,7 @@ package sqlstore import ( + "context" "database/sql" "strconv" "strings" @@ -41,6 +42,26 @@ type Dialect interface { // bytes — a code or token hash, a token-family ID: the only columns // the statements compare. It must compare them byte for byte. keyType(length int) string + // tableOptions follows the column list of every CREATE TABLE of + // [Schema]: the storage engine and character set the dialect needs, + // "" when it needs none. + tableOptions() string + // namedMigrationLock is the session-level named lock [Store.Migrate] + // takes around the creation of the tables and its deployment check, on + // an engine whose DDL cannot run in a transaction; "" for none. + namedMigrationLock() string + // checkDeployment verifies the requirements of the package + // documentation the database reports (see [Store.CheckDeployment]). + checkDeployment(ctx context.Context, q querier) error + // purgeStatements are the statements [Store.PurgeExpired] runs, one + // per table — codes, access tokens, refresh tokens, then families — + // each deleting at most a batch of the rows expired before a second. + purgeStatements() []string + // purgeArgs are the arguments of a purge statement: the second rows + // must have expired before, and the batch size. + purgeArgs(before int64, limit int) []any + // purgeTxOptions are the options of a purge batch's transaction. + purgeTxOptions() *sql.TxOptions } // migrationLockKey is the PostgreSQL advisory lock [Store.Migrate] takes: @@ -70,8 +91,10 @@ const ( var Postgres Dialect = postgres{} // MySQL is the MySQL / MariaDB dialect (? placeholders). The tables must be -// InnoDB: the store relies on its row locks. Its key columns are -// VARBINARY (see [Schema]). +// InnoDB — the store relies on its transactions and row locks — and +// [Schema] creates them so (ENGINE=InnoDB DEFAULT CHARSET=utf8mb4); their +// key columns are VARBINARY. [Store.CheckDeployment] verifies both, and +// a strict sql_mode. var MySQL Dialect = mysql{} // SQLite is the SQLite dialect (? placeholders). SQLite has no row locks: @@ -131,6 +154,26 @@ func (postgres) migrationLock() string { // hashes are 64 characters. func (postgres) keyType(length int) string { return varchar(length) } +func (postgres) tableOptions() string { return "" } + +func (postgres) purgeStatements() []string { return purgeSubqueries } +func (postgres) purgeArgs(before int64, limit int) []any { return subqueryArgs(before, limit) } + +// purgeTxOptions pins READ COMMITTED: the outer DELETE then re-checks the +// expiry of a row a concurrent transaction changed, where REPEATABLE READ +// or SERIALIZABLE would abort the batch with a serialization failure. +func (postgres) purgeTxOptions() *sql.TxOptions { + return &sql.TxOptions{Isolation: sql.LevelReadCommitted} +} + +// checkDeployment checks nothing: the store pins READ COMMITTED itself, +// and synchronous_commit — off, it can lose an acknowledged revocation in a +// crash — can be set per role, session or transaction, so the value one +// pooled connection reports proves nothing about the others. +func (postgres) checkDeployment(context.Context, querier) error { return nil } + +func (postgres) namedMigrationLock() string { return "" } + type mysql struct{} func (mysql) Name() string { return "mysql" } @@ -152,10 +195,44 @@ func (mysql) keyType(length int) string { return "VARBINARY(" + strconv.Itoa(len func (mysql) lockFamily() string { return lockFamilyForUpdate } func (mysql) lockRefresh() string { return lockRefreshForUpdate } +// tableOptions names the engine and the character set: a table created +// with the server's default engine would be MyISAM on a server configured +// so, without transactions or row locks. +func (mysql) tableOptions() string { return " ENGINE=InnoDB DEFAULT CHARSET=utf8mb4" } + +func (mysql) checkDeployment(ctx context.Context, q querier) error { return checkMySQL(ctx, q) } + +// namedMigrationLock serializes the Migrate calls of a deployment: MySQL +// commits every DDL statement on its own, so no transaction can hold a +// lock across them, and concurrent CREATE TABLE IF NOT EXISTS of one +// table can fail with a metadata-lock deadlock (1213). +func (mysql) namedMigrationLock() string { return migrationLockName } + +// purgeStatements are single-table DELETEs with LIMIT, which MySQL accepts +// (unlike LIMIT in an IN subquery): the statement locks and re-checks each +// row it deletes on its latest version. +func (mysql) purgeStatements() []string { + return []string{ + `DELETE FROM oauth2_auth_codes WHERE expires_at < ? LIMIT ?`, + `DELETE FROM oauth2_access_tokens WHERE expires_at < ? LIMIT ?`, + `DELETE FROM oauth2_refresh_tokens WHERE expires_at < ? LIMIT ?`, + `DELETE FROM oauth2_token_families WHERE expires_at < ? LIMIT ?`, + } +} + +func (mysql) purgeArgs(before int64, limit int) []any { return []any{before, limit} } + +// purgeTxOptions pins READ COMMITTED: at the server's default, REPEATABLE +// READ, a batch locks every record it scans and the gaps between them, +// blocking the inserts of /authorize and /token until it commits. +func (mysql) purgeTxOptions() *sql.TxOptions { + return &sql.TxOptions{Isolation: sql.LevelReadCommitted} +} + func (mysql) txOptions() *sql.TxOptions { return nil } -// migrationLock is empty: MySQL commits every DDL statement on its own, -// and serializes the creation of a table on the metadata lock of its name. +// migrationLock is empty: MySQL commits every DDL statement on its own, so +// Migrate takes a named lock instead (see namedMigrationLock). func (mysql) migrationLock() string { return "" } // dialectSQLite is the dialect identifier for SQLite. @@ -172,6 +249,21 @@ func (sqlite) insertFamily() string { return insertFamilyOnConflict } // BINARY collation. func (sqlite) keyType(length int) string { return varchar(length) } +func (sqlite) tableOptions() string { return "" } + +func (sqlite) purgeStatements() []string { return purgeSubqueries } +func (sqlite) purgeArgs(before int64, limit int) []any { return subqueryArgs(before, limit) } + +// purgeTxOptions are the defaults: SQLite's single writer serializes the +// batches with every other write. +func (sqlite) purgeTxOptions() *sql.TxOptions { return nil } + +// checkDeployment checks nothing: SQLite has one engine, and a missing busy +// timeout fails operations closed (SQLITE_BUSY). +func (sqlite) checkDeployment(context.Context, querier) error { return nil } + +func (sqlite) namedMigrationLock() string { return "" } + // varchar is the VARCHAR(length) column type. func varchar(length int) string { return "VARCHAR(" + strconv.Itoa(length) + ")" } diff --git a/oauth2/store/sql/doc.go b/oauth2/store/sql/doc.go index 09eb5a3c..c4fe6307 100644 --- a/oauth2/store/sql/doc.go +++ b/oauth2/store/sql/doc.go @@ -12,6 +12,8 @@ // // The token-family guarantees of oauth2.Storage hold under these // conditions; outside them a revocation can be lost or hidden. +// [Store.CheckDeployment] — which [Store.Migrate] runs — verifies the MySQL +// ones the server reports: the engine, the key columns and the sql_mode. // // - PostgreSQL: the store runs every transaction at READ COMMITTED, // whatever the server default, so that its locking reads and writes act @@ -19,9 +21,10 @@ // 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 -// commits. +// - MySQL: the tables must use InnoDB (transactions, row locks) — +// [Schema] names it, so a server whose default engine is MyISAM still +// gets InnoDB tables — and lookups must run in autocommit mode — the +// default — so each one sees the latest commits. // - SQLite: one writer at a time. Set a busy timeout (with // modernc.org/sqlite, the _pragma=busy_timeout(5000) DSN option) so a // concurrent writer waits instead of failing with SQLITE_BUSY; a busy @@ -62,21 +65,33 @@ // // # Cleanup // -// 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. 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. +// Rows stay until [Store.PurgeExpired] deletes them: schedule it, every few +// minutes for instance. 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. PurgeExpired deletes the rows expired before the instant it +// is given — a code past expires_at can no longer be exchanged — table by +// table, families last, in batches of its own transactions at READ +// COMMITTED; it re-checks each row's expiry as it deletes it, so a family a +// concurrent revocation extends is kept. A job of your own follows the same +// rules: small batches, outside long transactions, 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. +// +// The DDL creates no index — MySQL has no CREATE INDEX IF NOT EXISTS, and +// Migrate building one on a populated table would block its writes at boot +// — so add one on expires_at to each table through your migration tool: +// +// CREATE INDEX oauth2_auth_codes_expires_at ON oauth2_auth_codes (expires_at); +// CREATE INDEX oauth2_access_tokens_expires_at ON oauth2_access_tokens (expires_at); +// CREATE INDEX oauth2_refresh_tokens_expires_at ON oauth2_refresh_tokens (expires_at); +// CREATE INDEX oauth2_token_families_expires_at ON oauth2_token_families (expires_at); +// +// (CREATE INDEX CONCURRENTLY on a busy PostgreSQL database). Without it each +// purge batch scans its table and, on MySQL, waits for every row a +// concurrent /token request holds. // // 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/migrate_internal_test.go b/oauth2/store/sql/migrate_internal_test.go index 8954d65f..c6016555 100644 --- a/oauth2/store/sql/migrate_internal_test.go +++ b/oauth2/store/sql/migrate_internal_test.go @@ -9,6 +9,7 @@ import ( "crypto/sha256" "database/sql" "encoding/binary" + "fmt" "path/filepath" "strconv" "testing" @@ -60,6 +61,9 @@ func lockingStore(t *testing.T, lock string) (*Store, *sql.DB) { d := NewMockDialect(t) d.EXPECT().migrationLock().Return(lock) d.EXPECT().keyType(mock.Anything).RunAndReturn(sqlite{}.keyType).Maybe() + d.EXPECT().tableOptions().Return("").Maybe() + d.EXPECT().namedMigrationLock().Return("") + d.EXPECT().checkDeployment(mock.Anything, db).Return(nil).Maybe() return &Store{db: db, dialect: d}, db } @@ -192,3 +196,28 @@ func TestMigrateReportsAFailedBegin(t *testing.T) { require.ErrorContains(t, store.Migrate(t.Context()), "sqlstore: migrate: begin: ") } + +// TestMigrateEndsWithTheDeploymentCheck: once the tables exist, Migrate +// runs the dialect's deployment check and fails with what it reports — +// after the tables were created, which it never undoes. +func TestMigrateEndsWithTheDeploymentCheck(t *testing.T) { + t.Parallel() + + db, err := sql.Open("sqlite", "file:"+filepath.Join(t.TempDir(), "check.db")) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + unmet := fmt.Errorf("%w: table oauth2_auth_codes uses the MyISAM engine, not InnoDB", ErrDeploymentRequirement) + + d := NewMockDialect(t) + d.EXPECT().namedMigrationLock().Return("") + d.EXPECT().migrationLock().Return("") + d.EXPECT().keyType(mock.Anything).RunAndReturn(sqlite{}.keyType) + d.EXPECT().tableOptions().Return("") + d.EXPECT().checkDeployment(mock.Anything, db).Return(unmet).Once() + + store := &Store{db: db, dialect: d} + + require.ErrorIs(t, store.Migrate(t.Context()), unmet) + assert.Equal(t, schemaTables, tables(t, db)) +} diff --git a/oauth2/store/sql/namedlock.go b/oauth2/store/sql/namedlock.go new file mode 100644 index 00000000..bc5826a0 --- /dev/null +++ b/oauth2/store/sql/namedlock.go @@ -0,0 +1,113 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "context" + "database/sql" + "database/sql/driver" + "fmt" + "math" + "time" +) + +// migrationLockName is the named lock [Store.Migrate] takes on MySQL and +// MariaDB (GET_LOCK): the module path without its host, then ".Migrate". +// Named locks are server-wide and each store module has its own, so that +// their migrations never wait for one another. +const migrationLockName = "hyperscale-stack/security/oauth2/store/sql.Migrate" + +// lockWaitForever is the GET_LOCK timeout, in seconds, under a context +// without deadline: a year — the wait lasts as long as the context does. +const lockWaitForever = 365 * 24 * 60 * 60 + +// releaseTimeout bounds the release of a named lock, which runs even once +// the context of Migrate has ended. +const releaseTimeout = 10 * time.Second + +// lockWaitSeconds is the GET_LOCK timeout under ctx: the seconds left until +// its deadline, rounded up and at least one, or lockWaitForever. +func lockWaitSeconds(ctx context.Context) int64 { + deadline, ok := ctx.Deadline() + if !ok { + return lockWaitForever + } + + return max(1, int64(math.Ceil(time.Until(deadline).Seconds()))) +} + +// migrateUnderNamedLock applies [Schema] and runs the deployment check on +// one connection holding the named lock name, which it releases before the +// connection goes back to the pool: concurrent calls run one after the +// other. +func (s *Store) migrateUnderNamedLock(ctx context.Context, name string) error { + conn, err := s.db.Conn(ctx) + if err != nil { + return fmt.Errorf("sqlstore: migrate: connection: %w", err) + } + + defer func() { _ = conn.Close() }() + + if err := acquireNamedLock(ctx, conn, name); err != nil { + return err + } + + defer releaseNamedLock(ctx, conn, name) + + for _, stmt := range Schema(s.dialect) { + if _, err := conn.ExecContext(ctx, stmt); err != nil { + return fmt.Errorf("sqlstore: migrate: %w", err) + } + } + + return s.dialect.checkDeployment(ctx, conn) +} + +// acquireNamedLock takes the named lock name on conn, waiting as long as +// ctx allows: the server's wait is bounded by its deadline, and the driver +// cancels it when ctx ends. +func acquireNamedLock(ctx context.Context, conn *sql.Conn, name string) error { + var granted sql.NullInt64 + + if err := conn.QueryRowContext(ctx, `SELECT GET_LOCK(?, ?)`, name, lockWaitSeconds(ctx)).Scan(&granted); err != nil { + // The server may have granted the lock before the reply was lost: + // never pool a session that may hold it. + discard(conn) + + return fmt.Errorf("sqlstore: migrate: lock %s: %w", name, err) + } + + switch { + case !granted.Valid: + return fmt.Errorf("sqlstore: migrate: lock %s: not granted", name) + case granted.Int64 != 1: + return fmt.Errorf("sqlstore: migrate: lock %s: %w", name, context.DeadlineExceeded) + default: + return nil + } +} + +// releaseNamedLock releases the named lock name held on conn, even once +// ctx has ended. A release that fails discards the connection: its session +// would otherwise keep the lock from the pool. +func releaseNamedLock(ctx context.Context, conn *sql.Conn, name string) { + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), releaseTimeout) + defer cancel() + + var released sql.NullInt64 + + err := conn.QueryRowContext(ctx, `SELECT RELEASE_LOCK(?)`, name).Scan(&released) + if err == nil && released.Valid && released.Int64 == 1 { + return + } + + discard(conn) +} + +// discard closes conn instead of returning it to the pool when it is +// closed. +func discard(conn *sql.Conn) { + _ = conn.Raw(func(any) error { return driver.ErrBadConn }) +} diff --git a/oauth2/store/sql/namedlock_internal_test.go b/oauth2/store/sql/namedlock_internal_test.go new file mode 100644 index 00000000..1e4784b8 --- /dev/null +++ b/oauth2/store/sql/namedlock_internal_test.go @@ -0,0 +1,223 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "context" + "database/sql" + "database/sql/driver" + "errors" + "fmt" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + moderncsqlite "modernc.org/sqlite" +) + +// fakeLocks stands in for MySQL's GET_LOCK and RELEASE_LOCK on SQLite: the +// lock name picks the server's answer, and every call is recorded. +var fakeLocks = struct { + sync.Mutex + + calls []string +}{} + +// The lock names of the fake and the answer they get. +const ( + lockGranted = "granted" // GET_LOCK 1, RELEASE_LOCK 1 + lockTimedOut = "timed-out" // GET_LOCK 0 + lockNull = "null" // GET_LOCK NULL + lockFails = "fails" // GET_LOCK errors + lockReleaseFails = "release-fails" // GET_LOCK 1, RELEASE_LOCK 0 +) + +func init() { + moderncsqlite.MustRegisterScalarFunction("GET_LOCK", 2, func(_ *moderncsqlite.FunctionContext, args []driver.Value) (driver.Value, error) { + name, _ := args[0].(string) + record(fmt.Sprintf("GET_LOCK(%s, %v)", name, args[1])) + + switch name { + case lockTimedOut: + return int64(0), nil + case lockNull: + return nil, nil + case lockFails: + return nil, errors.New("lock service down") + default: + return int64(1), nil + } + }) + moderncsqlite.MustRegisterScalarFunction("RELEASE_LOCK", 1, func(_ *moderncsqlite.FunctionContext, args []driver.Value) (driver.Value, error) { + name, _ := args[0].(string) + record("RELEASE_LOCK(" + name + ")") + + if name == lockReleaseFails { + return int64(0), nil + } + + return int64(1), nil + }) +} + +func record(call string) { + fakeLocks.Lock() + defer fakeLocks.Unlock() + + fakeLocks.calls = append(fakeLocks.calls, call) +} + +// lockCalls returns the recorded calls naming lock. +func lockCalls(lock string) []string { + fakeLocks.Lock() + defer fakeLocks.Unlock() + + var calls []string + + for _, c := range fakeLocks.calls { + if strings.HasPrefix(c, "GET_LOCK("+lock+", ") || c == "RELEASE_LOCK("+lock+")" { + calls = append(calls, c) + } + } + + return calls +} + +// namedLockStore returns a store over a new SQLite database file whose +// dialect — a mock — names lock as its named migration lock, so that +// Migrate takes the MySQL path: lock, tables, check, release, on one +// connection. check is what the deployment check returns. +func namedLockStore(t *testing.T, lock string, check error) (*Store, *sql.DB) { + t.Helper() + + db, err := sql.Open("sqlite", "file:"+filepath.Join(t.TempDir(), "named.db")) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + d := NewMockDialect(t) + d.EXPECT().namedMigrationLock().Return(lock) + d.EXPECT().keyType(mock.Anything).RunAndReturn(sqlite{}.keyType).Maybe() + d.EXPECT().tableOptions().Return("").Maybe() + d.EXPECT().checkDeployment(mock.Anything, mock.AnythingOfType("*sql.Conn")).Return(check).Maybe() + + return &Store{db: db, dialect: d}, db +} + +// TestMigrateUnderNamedLock: Migrate takes the named lock, waiting as long +// as its context allows, creates the tables, runs the deployment check on +// the connection holding the lock, and releases it — after a failed check +// too. +func TestMigrateUnderNamedLock(t *testing.T) { + store, db := namedLockStore(t, lockGranted, nil) + + require.NoError(t, store.Migrate(context.Background())) + assert.Equal(t, schemaTables, tables(t, db)) + assert.Equal(t, []string{ + fmt.Sprintf("GET_LOCK(%s, %d)", lockGranted, lockWaitForever), "RELEASE_LOCK(" + lockGranted + ")", + }, lockCalls(lockGranted)) + assert.Equal(t, 1, db.Stats().Idle, "the connection goes back to the pool") + + unmet := fmt.Errorf("%w: table oauth2_auth_codes uses the MyISAM engine, not InnoDB", ErrDeploymentRequirement) + failing, _ := namedLockStore(t, lockGranted+"-check", unmet) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + require.ErrorIs(t, failing.Migrate(ctx), unmet) + assert.Equal(t, []string{ + "GET_LOCK(" + lockGranted + "-check, 30)", "RELEASE_LOCK(" + lockGranted + "-check)", + }, lockCalls(lockGranted+"-check"), "the wait follows the deadline; a failed check releases the lock") +} + +// TestMigrateNamedLockRefused: a lock the server does not grant — timed +// out, NULL, an error — stops Migrate before any table is created, and +// nothing is released. +func TestMigrateNamedLockRefused(t *testing.T) { + for lock, want := range map[string]string{ + lockTimedOut: "sqlstore: migrate: lock timed-out: context deadline exceeded", + lockNull: "sqlstore: migrate: lock null: not granted", + lockFails: "sqlstore: migrate: lock fails: SQL logic error: lock service down (1)", + } { + store, db := namedLockStore(t, lock, nil) + + err := store.Migrate(context.Background()) + require.EqualError(t, err, want) + assert.Empty(t, tables(t, db), lock) + assert.NotContains(t, lockCalls(lock), "RELEASE_LOCK("+lock+")", lock) + } + + store, _ := namedLockStore(t, lockTimedOut, nil) + require.ErrorIs(t, store.Migrate(context.Background()), context.DeadlineExceeded) +} + +// TestMigrateDiscardsAConnectionWhoseLockIsUnknown: a GET_LOCK that fails +// leaves the lock's state unknown — the server may have granted it before +// the reply was lost — so the connection is discarded, never pooled with +// the server-wide migration lock possibly held. +func TestMigrateDiscardsAConnectionWhoseLockIsUnknown(t *testing.T) { + store, db := namedLockStore(t, lockFails, nil) + + require.Error(t, store.Migrate(context.Background())) + assert.Zero(t, db.Stats().OpenConnections, "the connection is closed, not pooled") + + timedOut, timedOutDB := namedLockStore(t, lockTimedOut, nil) + + require.Error(t, timedOut.Migrate(context.Background())) + assert.Equal(t, 1, timedOutDB.Stats().Idle, "a lock refused for sure keeps its connection") +} + +// TestMigrateNamedLockFailures: no connection, or a failing statement, +// fails Migrate; a failing statement releases the lock first. +func TestMigrateNamedLockFailures(t *testing.T) { + store, db := namedLockStore(t, lockGranted+"-closed", nil) + require.NoError(t, db.Close()) + require.ErrorContains(t, store.Migrate(context.Background()), "sqlstore: migrate: connection: ") + + store, db = namedLockStore(t, lockGranted+"-blocked", nil) + last := schemaTables[len(schemaTables)-1] + execAll(t, db, `CREATE TABLE blocker (x INTEGER)`, `CREATE INDEX `+last+` ON blocker (x)`) + + require.ErrorContains(t, store.Migrate(context.Background()), "already an index named "+last) + assert.Contains(t, lockCalls(lockGranted+"-blocked"), "RELEASE_LOCK("+lockGranted+"-blocked)") +} + +// TestMigrateDiscardsAConnectionKeepingTheLock: a release the server +// refuses discards the connection, whose session would keep the lock. +func TestMigrateDiscardsAConnectionKeepingTheLock(t *testing.T) { + store, db := namedLockStore(t, lockReleaseFails, nil) + + require.NoError(t, store.Migrate(context.Background())) + assert.Contains(t, lockCalls(lockReleaseFails), "RELEASE_LOCK("+lockReleaseFails+")") + assert.Zero(t, db.Stats().OpenConnections, "the connection is closed, not pooled") +} + +func TestLockWaitSeconds(t *testing.T) { + t.Parallel() + + assert.Equal(t, int64(lockWaitForever), lockWaitSeconds(context.Background())) + + ctx, cancel := context.WithTimeout(context.Background(), 1500*time.Millisecond) + defer cancel() + + assert.Equal(t, int64(2), lockWaitSeconds(ctx), "rounded up") + + expired, cancelExpired := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancelExpired() + + assert.Equal(t, int64(1), lockWaitSeconds(expired), "at least one second") +} + +func TestNamedMigrationLock(t *testing.T) { + t.Parallel() + + assert.Equal(t, "hyperscale-stack/security/oauth2/store/sql.Migrate", MySQL.namedMigrationLock()) + assert.Empty(t, Postgres.namedMigrationLock()) + assert.Empty(t, SQLite.namedMigrationLock()) +} diff --git a/oauth2/store/sql/purge.go b/oauth2/store/sql/purge.go new file mode 100644 index 00000000..08db5880 --- /dev/null +++ b/oauth2/store/sql/purge.go @@ -0,0 +1,118 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "context" + "fmt" + "time" +) + +// purgeBatch is the most rows one purge statement deletes: a batch keeps +// the rows it deletes locked until it commits, so a backlog goes in short +// transactions. A variable, for the tests. +var purgeBatch = 1000 + +// The purge statements of PostgreSQL and SQLite, one per table in purge +// order — codes and tokens, then families — with ? for (before, limit, +// before). The outer DELETE repeats the expiry: a row the subquery picked, +// then extended by a concurrent transaction — a revocation extending its +// family's retention — is re-checked on its latest version and kept. +const ( + purgeCodesSubquery = `DELETE FROM oauth2_auth_codes WHERE code_hash IN + (SELECT code_hash FROM oauth2_auth_codes WHERE expires_at < ? LIMIT ?) AND expires_at < ?` + purgeAccessSubquery = `DELETE FROM oauth2_access_tokens WHERE token_hash IN + (SELECT token_hash FROM oauth2_access_tokens WHERE expires_at < ? LIMIT ?) AND expires_at < ?` + purgeRefreshSubquery = `DELETE FROM oauth2_refresh_tokens WHERE token_hash IN + (SELECT token_hash FROM oauth2_refresh_tokens WHERE expires_at < ? LIMIT ?) AND expires_at < ?` + purgeFamiliesSubquery = `DELETE FROM oauth2_token_families WHERE family_id IN + (SELECT family_id FROM oauth2_token_families WHERE expires_at < ? LIMIT ?) AND expires_at < ?` +) + +// purgeSubqueries are the purge statements of PostgreSQL and SQLite. +var purgeSubqueries = []string{purgeCodesSubquery, purgeAccessSubquery, purgeRefreshSubquery, purgeFamiliesSubquery} + +// subqueryArgs are the arguments of a purge statement of purgeSubqueries. +func subqueryArgs(before int64, limit int) []any { return []any{before, limit, before} } + +// PurgeExpired deletes the rows whose expires_at is before now — codes, +// access and refresh tokens, then token families past their retention — +// and returns how many it deleted. Nothing else deletes them: schedule it, +// every few minutes for instance. Concurrent purges are safe. +// +// Pass the current time: a later instant deletes live rows, and a token +// family deleted before its expires_at is recorded active again by the +// next issuance into it, reviving the tokens a revocation covered (see the +// package documentation). +// +// Each table is purged in batches of at most 1000 rows, each a transaction +// of its own — at READ COMMITTED on PostgreSQL and MySQL — codes and tokens +// before families, the order in which no batch can deadlock with the +// store's transactions. A batch re-checks the expiry of each row it +// deletes on its latest version, so a family whose retention a concurrent +// revocation extends is kept. On MySQL, READ COMMITTED keeps a batch from +// locking the gaps it scans, which would block the inserts of /authorize +// and /token; a server logging statements (binlog_format=STATEMENT) +// refuses it, and the purge fails, closed. Add an index on expires_at to +// each table (see the package documentation): without it every batch +// scans its table. +// +// A failure returns the rows deleted until then and the error; the +// batches committed before it stay committed. +func (s *Store) PurgeExpired(ctx context.Context, now time.Time) (int64, error) { + var total int64 + + for _, stmt := range s.dialect.purgeStatements() { + n, err := s.purgeTable(ctx, stmt, now.Unix()) + total += n + + if err != nil { + return total, err + } + } + + return total, nil +} + +// purgeTable runs the purge statement of one table, batch after batch, +// until a batch deletes fewer rows than purgeBatch. +func (s *Store) purgeTable(ctx context.Context, stmt string, before int64) (int64, error) { + var total int64 + + for { + n, err := s.purgeOnce(ctx, stmt, before) + total += n + + if err != nil || n < int64(purgeBatch) { + return total, err + } + } +} + +// purgeOnce runs one batch of stmt in a transaction of its own. +func (s *Store) purgeOnce(ctx context.Context, stmt string, before int64) (int64, error) { + tx, err := s.db.BeginTx(ctx, s.dialect.purgeTxOptions()) + if err != nil { + return 0, fmt.Errorf("sqlstore: purge expired rows: begin: %w", err) + } + + defer func() { _ = tx.Rollback() }() // a no-op once committed + + res, err := tx.ExecContext(ctx, s.dialect.rebind(stmt), s.dialect.purgeArgs(before, purgeBatch)...) + if err != nil { + return 0, fmt.Errorf("sqlstore: purge expired rows: %w", err) + } + + n, err := res.RowsAffected() + if err != nil { + return 0, fmt.Errorf("sqlstore: purge expired rows: rows affected: %w", err) + } + + if err := tx.Commit(); err != nil { + return 0, fmt.Errorf("sqlstore: purge expired rows: commit: %w", err) + } + + return n, nil +} diff --git a/oauth2/store/sql/purge_internal_test.go b/oauth2/store/sql/purge_internal_test.go new file mode 100644 index 00000000..e91e9d64 --- /dev/null +++ b/oauth2/store/sql/purge_internal_test.go @@ -0,0 +1,51 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "database/sql" + "strconv" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestPurgeExpiredInBatches deletes a backlog larger than a batch, batch +// after batch, until one comes back short. It changes purgeBatch, so it +// does not run in parallel. +func TestPurgeExpiredInBatches(t *testing.T) { + defer func(batch int) { purgeBatch = batch }(purgeBatch) + + purgeBatch = 2 + + db, err := sql.Open("sqlite", ":memory:") + require.NoError(t, err) + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + + store, err := New(db, SQLite) + require.NoError(t, err) + require.NoError(t, store.Migrate(t.Context())) + + now := time.Unix(time.Now().Unix(), 0) + + for i := range 5 { + require.NoError(t, store.SaveAuthorizationCode(t.Context(), &oauth2.AuthorizationCode{ + CodeHash: "expired-" + strconv.Itoa(i), ClientID: "c", Subject: "s", RedirectURI: "https://app.example/cb", + Scope: "r", IssuedAt: now.Add(-2 * time.Hour), ExpiresAt: now.Add(-time.Hour), + })) + } + + n, err := store.PurgeExpired(t.Context(), now) + require.NoError(t, err) + assert.Equal(t, int64(5), n) + + var left int + require.NoError(t, db.QueryRowContext(t.Context(), `SELECT COUNT(*) FROM oauth2_auth_codes`).Scan(&left)) + assert.Zero(t, left) +} diff --git a/oauth2/store/sql/purge_test.go b/oauth2/store/sql/purge_test.go new file mode 100644 index 00000000..4eeffef2 --- /dev/null +++ b/oauth2/store/sql/purge_test.go @@ -0,0 +1,136 @@ +// 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" +) + +// purgeSeed is what seedPurge stored: the records PurgeExpired must keep. +type purgeSeed struct { + live, revoked *oauth2.TokenPair +} + +// pairExpiring returns a pair of family whose tokens expire at expiresAt. +func pairExpiring(name, family string, expiresAt time.Time) *oauth2.TokenPair { + pair := testPair(name, family, time.Hour) + pair.Access.ExpiresAt, pair.Refresh.ExpiresAt = expiresAt, expiresAt + + return pair +} + +// seedPurge stores, as of now, eight rows expired before it — two codes, +// three access tokens, two refresh tokens, one family — and rows that must +// stay: codes and a family expiring at now exactly, a live code and pair, +// and the record of a family revoked, whose expired tokens go. +func seedPurge(t *testing.T, s *sqlstore.Store, db *sql.DB, now time.Time) purgeSeed { + t.Helper() + + ctx := t.Context() + past := now.Add(-time.Hour) + + for hash, expires := range map[string]time.Time{ + "code-expired": past, "code-consumed": past, "code-boundary": now, "code-live": now.Add(time.Hour), + } { + require.NoError(t, s.SaveAuthorizationCode(ctx, &oauth2.AuthorizationCode{ + CodeHash: hash, ClientID: "c", Subject: "s", RedirectURI: "https://app.example/cb", Scope: "r", + IssuedAt: past.Add(-time.Minute), ExpiresAt: expires, + })) + } + + _, err := s.ConsumeAuthorizationCode(ctx, "code-consumed") + require.NoError(t, err) + + require.NoError(t, s.SaveTokenPair(ctx, pairExpiring("purge-expired", "fam-purge-expired", past))) + + cc := pairExpiring("purge-cc", "", past) + cc.Refresh = nil + require.NoError(t, s.SaveTokenPair(ctx, cc)) + + seed := purgeSeed{ + live: pairExpiring("purge-live", "fam-purge-live", now.Add(time.Hour)), + revoked: pairExpiring("purge-revoked", "fam-purge-revoked", past), + } + require.NoError(t, s.SaveTokenPair(ctx, seed.live)) + require.NoError(t, s.SaveTokenPair(ctx, seed.revoked)) + require.NoError(t, s.RevokeRefreshFamily(ctx, "fam-purge-revoked")) + + require.NoError(t, s.SaveTokenPair(ctx, pairExpiring("purge-boundary", "fam-purge-boundary", now))) + + return seed +} + +// purgedSeed is the number of rows seedPurge stores expired. +const purgedSeed = 8 + +// rowCount returns the number of rows of table. +func rowCount(t *testing.T, db *sql.DB, table string) int { + t.Helper() + + var n int + require.NoError(t, db.QueryRowContext(t.Context(), "SELECT COUNT(*) FROM "+table).Scan(&n)) + + return n +} + +// TestSQLPurgeExpired deletes every row expired before now, table by +// table, and keeps the rows expiring at now, the live ones and the record +// of a revoked family within its retention: once purged, the live pair is +// still usable and the revoked family still refuses issuance. A second +// purge deletes nothing. +func TestSQLPurgeExpired(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + now := time.Unix(time.Now().Unix(), 0) + seed := seedPurge(t, store, db, now) + + n, err := store.PurgeExpired(t.Context(), now) + require.NoError(t, err) + assert.Equal(t, int64(purgedSeed), n) + + for table, rows := range map[string]int{ + "oauth2_auth_codes": 2, "oauth2_access_tokens": 2, "oauth2_refresh_tokens": 2, "oauth2_token_families": 3, + } { + assert.Equal(t, rows, rowCount(t, db, table), table) + } + + _, err = store.LookupAccessToken(t.Context(), seed.live.Access.TokenHash) + require.NoError(t, err, "the live pair stays") + + again := pairExpiring("purge-revoked-again", "fam-purge-revoked", now.Add(time.Hour)) + require.ErrorIs(t, store.SaveTokenPair(t.Context(), again), oauth2.ErrTokenFamilyRevoked, + "the revoked family's record stays") + + n, err = store.PurgeExpired(t.Context(), now) + require.NoError(t, err) + assert.Zero(t, n) +} + +// TestSQLPurgeExpiredFailures: a statement or a transaction that fails +// fails the purge, with the rows deleted until then. +func TestSQLPurgeExpiredFailures(t *testing.T) { + t.Parallel() + + n, err := unmigratedStore(t).PurgeExpired(t.Context(), time.Now()) + require.ErrorContains(t, err, "sqlstore: purge expired rows: ") + assert.Zero(t, n) + + store, _ := openSQLite(t) + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + _, err = store.PurgeExpired(ctx, time.Now()) + require.ErrorIs(t, err, context.Canceled) +} diff --git a/oauth2/store/sql/schema.go b/oauth2/store/sql/schema.go index 4712a7c5..941a08de 100644 --- a/oauth2/store/sql/schema.go +++ b/oauth2/store/sql/schema.go @@ -24,10 +24,13 @@ import ( // and token hashes and the token-family IDs, the only columns the // statements compare: VARCHAR on PostgreSQL and SQLite, which compare it // byte for byte, VARBINARY on MySQL, whose text comparisons follow a -// collation that may ignore case, accents or trailing spaces. A nil -// Dialect gets the PostgreSQL / SQLite DDL. A MySQL database created -// before the key columns were binary is upgraded with the statements -// MIGRATION.md lists. +// collation that may ignore case, accents or trailing spaces — and for the +// table options of MySQL: ENGINE=InnoDB DEFAULT CHARSET=utf8mb4, since a +// server whose default engine is MyISAM would otherwise create tables +// without transactions or row locks, losing the atomic writes (I3) of +// [oauth2.Storage]. A nil Dialect gets the PostgreSQL / SQLite DDL. A MySQL +// database created before the key columns were binary, or with MyISAM +// tables, is upgraded with the statements MIGRATION.md lists. // // The issuer and extension_data columns of codes and tokens — the realm a // grant is bound to and the opaque state an oauth2.Extension recorded for @@ -52,7 +55,7 @@ func Schema(d Dialect) []string { } // 64: oauth2.MaxFamilyIDLength, in bytes. - hash, family := d.keyType(128), d.keyType(64) + hash, family, options := d.keyType(128), d.keyType(64), d.tableOptions() return []string{ `CREATE TABLE IF NOT EXISTS oauth2_auth_codes ( @@ -70,7 +73,7 @@ func Schema(d Dialect) []string { issuer TEXT, extension_data TEXT, redirect_uri_omitted SMALLINT NOT NULL DEFAULT 0 - )`, + )` + options, `CREATE TABLE IF NOT EXISTS oauth2_access_tokens ( token_hash ` + hash + ` PRIMARY KEY, client_id VARCHAR(255) NOT NULL, @@ -82,7 +85,7 @@ func Schema(d Dialect) []string { expires_at BIGINT NOT NULL, issuer TEXT, extension_data TEXT - )`, + )` + options, `CREATE TABLE IF NOT EXISTS oauth2_refresh_tokens ( token_hash ` + hash + ` PRIMARY KEY, client_id VARCHAR(255) NOT NULL, @@ -95,19 +98,22 @@ func Schema(d Dialect) []string { issuer TEXT, extension_data TEXT, family_issued_at BIGINT NOT NULL DEFAULT 0 - )`, + )` + options, `CREATE TABLE IF NOT EXISTS oauth2_token_families ( family_id ` + family + ` PRIMARY KEY, revoked_at BIGINT NOT NULL DEFAULT 0, expires_at BIGINT NOT NULL - )`, + )` + options, } } // Migrate applies [Schema] to the store's database: it creates the missing // tables and never alters an existing one — the columns a release adds to // an existing table are added with the statements MIGRATION.md lists, -// through your migration tool, before the release serves traffic. +// through your migration tool, before the release serves traffic. It then +// runs [Store.CheckDeployment], and fails with its error: a table that +// exists already keeps its engine and its column types, so a deployment +// that does not meet the requirements is refused at boot. // // It is safe to call repeatedly, and concurrently — every instance of a // deployment starting at once. On PostgreSQL, where concurrent CREATE @@ -120,11 +126,34 @@ func Schema(d Dialect) []string { // server-side wait when ctx ends. A migration tool applying [Schema] // concurrently with Migrate should take the same lock. An engine without // pg_advisory_xact_lock cannot use Migrate: apply Schema with your own -// tool. MySQL serializes the creation of a table on the metadata lock of -// its name, and SQLite on its write lock — given the busy timeout the -// package documentation requires, without which a concurrent call fails -// with SQLITE_BUSY: there each statement runs on its own. +// tool. On MySQL and MariaDB, where concurrent CREATE TABLE IF NOT EXISTS +// of one table can fail with a metadata-lock deadlock, Migrate creates the +// tables and runs the check on one connection holding the named lock +// "hyperscale-stack/security/oauth2/store/sql.Migrate" (GET_LOCK; named +// locks are server-wide), which it releases before returning: concurrent +// calls run one after the other, waiting as long as ctx allows. A holder +// whose client vanished keeps the lock until the server notices +// (wait_timeout): find it with SELECT IS_USED_LOCK('') and KILL it. +// On a Galera or Percona XtraDB cluster named locks are local to a node, so +// Migrate calls through different nodes are not serialized. SQLite +// serializes the creation of a table on its write lock — given the busy +// timeout the package documentation requires, without which a concurrent +// call fails with SQLITE_BUSY: there each statement runs on its own. func (s *Store) Migrate(ctx context.Context) error { + if name := s.dialect.namedMigrationLock(); name != "" { + return s.migrateUnderNamedLock(ctx, name) + } + + if err := s.createTables(ctx); err != nil { + return err + } + + return s.CheckDeployment(ctx) +} + +// createTables applies [Schema]: under the dialect's migration lock when it +// has one, statement by statement otherwise. +func (s *Store) createTables(ctx context.Context) error { if lock := s.dialect.migrationLock(); lock != "" { return s.migrateLocked(ctx, lock) } diff --git a/oauth2/store/sql/schema_test.go b/oauth2/store/sql/schema_test.go index b3a9e680..baf426fd 100644 --- a/oauth2/store/sql/schema_test.go +++ b/oauth2/store/sql/schema_test.go @@ -21,7 +21,8 @@ import ( // hashes and the token-family IDs, the only columns the statements // compare — which are VARBINARY: MySQL and MariaDB compare binary strings // byte for byte, text under a collation that may ignore case, accents or -// trailing spaces. +// trailing spaces; and for the options of every table, which name InnoDB +// and utf8mb4 rather than leave them to the server's defaults. func TestSchemaPerDialect(t *testing.T) { t.Parallel() @@ -41,7 +42,12 @@ func TestSchemaPerDialect(t *testing.T) { require.Len(t, mysql, len(portable)) for i, stmt := range mysql { - assert.Equal(t, portable[i], strings.ReplaceAll(stmt, "VARBINARY(", "VARCHAR("), "only the key columns differ") + const options = ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4" + + require.True(t, strings.HasSuffix(stmt, options), "MySQL tables name their engine: %s", stmt) + assert.Equal(t, portable[i], strings.ReplaceAll(strings.TrimSuffix(stmt, options)+")", "VARBINARY(", "VARCHAR("), + "only the key columns and the table options differ") + assert.NotContains(t, portable[i], "ENGINE") } ddl := strings.Join(mysql, "\n") diff --git a/oauth2/store/sql/store.go b/oauth2/store/sql/store.go index 44c75fe3..05d8e53c 100644 --- a/oauth2/store/sql/store.go +++ b/oauth2/store/sql/store.go @@ -42,6 +42,14 @@ import ( // // Revocation therefore never touches token rows: they stay until an // operator deletes expired rows (see the package documentation). +// +// A rotation rests on three layers, each enough to keep two rotations of +// one token from both succeeding, and each pinned by a test of its own: +// the family row lock (the family upsert takes it already on MySQL, the +// SELECT … FOR UPDATE on PostgreSQL), the presented token's row lock, +// which makes a concurrent consumption a detected reuse, and the UPDATE +// consuming the token, which must affect exactly that row — the last line +// should a lock not hold. type Store struct { db *sql.DB dialect Dialect diff --git a/oauth2/store/sql/store_family_test.go b/oauth2/store/sql/store_family_test.go index f6226e17..324d0f47 100644 --- a/oauth2/store/sql/store_family_test.go +++ b/oauth2/store/sql/store_family_test.go @@ -417,3 +417,37 @@ func TestSQLFamilyLockWithoutRecordFailsClosed(t *testing.T) { "SELECT COUNT(*) FROM oauth2_refresh_tokens WHERE token_hash = ? AND consumed = 0", first.Refresh.TokenHash) assert.Equal(t, 1, consumed, "the refused rotation consumed nothing") } + +// TestSQLRotationStopsWhenItConsumesNoRow pins the last locking layer of a +// rotation (I-25): the UPDATE consuming the presented token must affect +// exactly the row read unconsumed under lock. A trigger silently skipping +// that UPDATE — what a lock that did not hold would leave, a concurrent +// rotation having consumed the token in between — makes it affect no row: +// the rotation fails, its transaction rolls back, nothing is persisted and +// the presented token stays as it was. +func TestSQLRotationStopsWhenItConsumesNoRow(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + ctx := t.Context() + + first, next := testPair("noop-0", "fam-noop", time.Hour), testPair("noop-1", "fam-noop", time.Hour) + require.NoError(t, store.SaveTokenPair(ctx, first)) + + _, err := db.ExecContext(ctx, `CREATE TRIGGER lose_consumption BEFORE UPDATE OF consumed ON oauth2_refresh_tokens + BEGIN SELECT RAISE(IGNORE); END`) + require.NoError(t, err) + + err = store.RotateRefreshToken(ctx, first.Refresh.TokenHash, next) + require.EqualError(t, err, "sqlstore: consume refresh token: 0 rows affected") + + _, err = store.LookupAccessToken(ctx, next.Access.TokenHash) + require.EqualError(t, err, "oauth2: invalid_grant: access token not found", "the replacement is not persisted") + + _, err = store.LookupRefreshToken(ctx, next.Refresh.TokenHash) + require.EqualError(t, err, "oauth2: invalid_grant: refresh token not found", "the replacement is not persisted") + + rt, err := store.LookupRefreshToken(ctx, first.Refresh.TokenHash) + require.NoError(t, err) + require.False(t, rt.Consumed, "the presented token is not consumed, its family not revoked") +} diff --git a/oauth2/storetest/conformance.go b/oauth2/storetest/conformance.go index 78c21178..fab871e9 100644 --- a/oauth2/storetest/conformance.go +++ b/oauth2/storetest/conformance.go @@ -5,7 +5,9 @@ // Package storetest provides a black-box conformance suite that every // [oauth2.Storage] implementation MUST pass. The in-memory, SQL and Redis // stores all run RunConformance against a fresh instance so behavioral -// drift between backends is caught at test time. +// drift between backends is caught at test time. Retention is checked over +// time, not only on the next call: a run waits about 1.5 s, and the case +// needs a backend whose time moves on its own (miniredis does not). // // The package imports "testing" deliberately: it is a test helper in the // spirit of net/http/httptest and testing/fstest, meant to be called from @@ -40,7 +42,11 @@ type Factory func() oauth2.Storage // writes, reuse, idempotence, isolation, keys compared byte for byte — // sequentially and under races, // through the public API only, and drives the real authorization_code -// grant to check that a replayed code revokes the tokens it issued. What +// grant to check that a replayed code revokes the tokens it issued. Every +// token lookup must return the full record it was saved with — client, +// subject, scope, audience, issuer, extension data, family, timestamps to +// the second, consumed state — and a consumed code, a rotated refresh token +// and a revoked family must still be recognized about 1.5 s later. 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). @@ -82,6 +88,8 @@ func RunConformance(t *testing.T, newStore Factory) { {"GrantContextLongestData", testGrantContextLongestData}, {"RotationKeepsGrantContext", testRotationKeepsGrantContext}, {"RefreshTokenFamilyIssuedAt", testRefreshFamilyIssuedAt}, + {"TokenRecordsRoundTrip", testTokenRecordsRoundTrip}, + {"RetainedOverTime", testRetainedOverTime}, } for _, c := range cases { @@ -340,7 +348,7 @@ func testAccessLifecycle(t *testing.T, s oauth2.Storage) { mustNoError(t, err, "lookup") mustEqual(t, got.Subject, testSubject, "lookup lost the subject") mustEqual(t, got.Audience, cc.Access.Audience, "lookup lost the audience") - requireContext(t, s, cc) + requireRecords(t, s, cc, false) mustNoError(t, s.RevokeAccessToken(ctx(), cc.Access.TokenHash), "revoke") requireUnusable(t, s, cc) @@ -414,7 +422,7 @@ func testRefreshSaveLookup(t *testing.T, s oauth2.Storage) { mustNoError(t, err, "lookup refresh token") mustEqual(t, got.FamilyID, "fam-save", "lookup lost the family") mustEqual(t, got.Scope, testScope, "lookup lost the scope") - requireContext(t, s, pair) + requireRecords(t, s, pair, false) } func testRefreshRotation(t *testing.T, s oauth2.Storage) { @@ -426,10 +434,10 @@ func testRefreshRotation(t *testing.T, s oauth2.Storage) { requireConsumed(t, s, first.Refresh.TokenHash) requireUsable(t, s, next) - requireContext(t, s, next) + requireRecords(t, s, next, false) // The consumed token keeps its own record: the Redis rotation script // re-encodes it with cjson, the context must survive that byte-exact. - requireContext(t, s, first) + requireRecords(t, s, first, true) // Rotation consumes the presented refresh token only: the access token // issued alongside it lives on until it expires. diff --git a/oauth2/storetest/family.go b/oauth2/storetest/family.go index 38575dfd..a92f1039 100644 --- a/oauth2/storetest/family.go +++ b/oauth2/storetest/family.go @@ -67,38 +67,6 @@ func familyContext(family string) (issuer, data string) { return testIssuer, trickyData + family } -// requireContext asserts that the lookups of pair's tokens return the -// issuer and extension data it was saved with, byte-exact. -func requireContext(t *testing.T, s oauth2.Storage, pair *oauth2.TokenPair) { - t.Helper() - - at, err := s.LookupAccessToken(ctx(), pair.Access.TokenHash) - mustNoError(t, err, "lookup access token "+pair.Access.TokenHash) - mustEqual(t, at.Issuer, pair.Access.Issuer, "access token issuer") - mustEqual(t, at.ExtensionData, pair.Access.ExtensionData, "access token extension data") - mustEqual(t, at.Token, "", "the raw access token is never kept") - - if pair.Refresh == nil { - return - } - - rt, err := s.LookupRefreshToken(ctx(), pair.Refresh.TokenHash) - mustNoError(t, err, "lookup refresh token "+pair.Refresh.TokenHash) - mustEqual(t, rt.Issuer, pair.Refresh.Issuer, "refresh token issuer") - mustEqual(t, rt.ExtensionData, pair.Refresh.ExtensionData, "refresh token extension data") - mustEqual(t, rt.Token, "", "the raw refresh token is never kept") - requireFamilyIssuedAt(t, rt, pair.Refresh.FamilyIssuedAt) -} - -// requireFamilyIssuedAt asserts that rt carries want as FamilyIssuedAt, to -// the second — the zero time as the zero time, never as the Unix epoch. -func requireFamilyIssuedAt(t *testing.T, rt *oauth2.RefreshToken, want time.Time) { - t.Helper() - - mustEqual(t, rt.FamilyIssuedAt.IsZero(), want.IsZero(), "refresh token FamilyIssuedAt set") - mustEqual(t, rt.FamilyIssuedAt.Unix(), want.Unix(), "refresh token FamilyIssuedAt") -} - // accessOnly returns a pair carrying an access token alone: a // client_credentials token when family is empty, a non-rotating Profile20 // refresh otherwise. diff --git a/oauth2/storetest/grant_context.go b/oauth2/storetest/grant_context.go index 3f2d82e8..e8b292d9 100644 --- a/oauth2/storetest/grant_context.go +++ b/oauth2/storetest/grant_context.go @@ -50,8 +50,8 @@ func testGrantContextRoundTrip(t *testing.T, s oauth2.Storage) { mustRotate(t, s, chain[0], chain[1]) mustRotate(t, s, chain[1], chain[2]) - for _, pair := range chain { - requireContext(t, s, pair) + for i, pair := range chain { + requireRecords(t, s, pair, i < len(chain)-1) } bare := sampleCode("ctx-bare") @@ -66,7 +66,7 @@ func testGrantContextRoundTrip(t *testing.T, s oauth2.Storage) { empty.Access.Issuer, empty.Access.ExtensionData = "", "" empty.Refresh.Issuer, empty.Refresh.ExtensionData = "", "" mustSave(t, s, empty) - requireContext(t, s, empty) + requireRecords(t, s, empty, false) } // testGrantContextLongestData stores the longest ExtensionData the server @@ -92,8 +92,8 @@ func testGrantContextLongestData(t *testing.T, s oauth2.Storage) { mustSave(t, s, first) mustRotate(t, s, first, next) - requireContext(t, s, first) - requireContext(t, s, next) + requireRecords(t, s, first, true) + requireRecords(t, s, next, false) } // testRotationKeepsGrantContext rotates a token into a pair carrying @@ -123,7 +123,7 @@ func testRotationKeepsGrantContext(t *testing.T, s oauth2.Storage) { same := newPair("keep-1", "fam-keep") mustRotate(t, s, first, same) - requireContext(t, s, same) + requireRecords(t, s, same, false) // The context check comes right after the family check, before the // revoked and the consumed ones (oauth2.RefreshTokenStore): a consumed @@ -158,13 +158,13 @@ func testRefreshFamilyIssuedAt(t *testing.T, s oauth2.Storage) { unset := newPair("start-unset", "fam-start-unset") unset.Refresh.FamilyIssuedAt = time.Time{} mustSave(t, s, unset) - requireContext(t, s, unset) + requireRecords(t, s, unset, false) first, next := newPair("start-0", "fam-start"), newPair("start-1", "fam-start") next.Refresh.FamilyIssuedAt = first.Refresh.FamilyIssuedAt mustSave(t, s, first) mustRotate(t, s, first, next) - requireContext(t, s, next) - requireContext(t, s, first) + requireRecords(t, s, next, false) + requireRecords(t, s, first, true) } diff --git a/oauth2/storetest/records.go b/oauth2/storetest/records.go new file mode 100644 index 00000000..c084bb2e --- /dev/null +++ b/oauth2/storetest/records.go @@ -0,0 +1,198 @@ +// 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 ( + "reflect" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" +) + +// The token-record cases of the suite: a lookup returns every field a +// store keeps — the client binding, the subject, the scope, the audience, +// the expiry the grants and endpoints check — exactly as saved, and a store +// keeps what the token-family contract needs for as long as it needs it. + +// retentionWait is how long RetainedOverTime lets pass before checking +// again: over a second, so a store keeping a record for a second only — a +// TTL unit mistake, a clamped expiry — is caught. It needs a backend whose +// time moves on its own: miniredis expires keys only when its clock is +// moved, so there the case proves nothing. +const retentionWait = 1500 * time.Millisecond + +// atSecond is t as a store keeps it: whole Unix seconds, the zero time kept +// zero — never the Unix epoch. +func atSecond(t time.Time) time.Time { + if t.IsZero() { + return time.Time{} + } + + return time.Unix(t.Unix(), 0) +} + +// storedAccess is at as a store returns it: without the raw token, with +// its timestamps at the second. +func storedAccess(at oauth2.AccessToken) oauth2.AccessToken { + at.Token = "" + at.IssuedAt = atSecond(at.IssuedAt) + at.ExpiresAt = atSecond(at.ExpiresAt) + + return at +} + +// storedRefresh is rt as a store returns it: without the raw token, with +// its timestamps at the second. +func storedRefresh(rt oauth2.RefreshToken) oauth2.RefreshToken { + rt.Token = "" + rt.IssuedAt = atSecond(rt.IssuedAt) + rt.ExpiresAt = atSecond(rt.ExpiresAt) + rt.FamilyIssuedAt = atSecond(rt.FamilyIssuedAt) + + return rt +} + +// mustEqualRecord asserts that the records got and want are equal in every +// field — a field added to the model is compared too — and names the +// fields that differ. +func mustEqualRecord[T any](t *testing.T, got, want T, msg string) { + t.Helper() + + g, w := reflect.ValueOf(got), reflect.ValueOf(want) + + var differ []string + + for i := range g.NumField() { + if !reflect.DeepEqual(g.Field(i).Interface(), w.Field(i).Interface()) { + differ = append(differ, g.Type().Field(i).Name) + } + } + + if len(differ) > 0 { + t.Fatalf("%s: fields %v differ: got %+v, want %+v", msg, differ, got, want) + } +} + +// requireRecords asserts that the lookups of pair's tokens return their +// full records — every field a store keeps, timestamps to the second, +// never the raw token — the refresh token consumed as told. +func requireRecords(t *testing.T, s oauth2.Storage, pair *oauth2.TokenPair, consumed bool) { + t.Helper() + + at, err := s.LookupAccessToken(ctx(), pair.Access.TokenHash) + mustNoError(t, err, "lookup access token "+pair.Access.TokenHash) + mustEqual(t, at.Token, "", "the raw access token is never kept") + mustEqualRecord(t, storedAccess(*at), storedAccess(pair.Access), "record of access token "+pair.Access.TokenHash) + + if pair.Refresh == nil { + return + } + + rt, err := s.LookupRefreshToken(ctx(), pair.Refresh.TokenHash) + mustNoError(t, err, "lookup refresh token "+pair.Refresh.TokenHash) + mustEqual(t, rt.Token, "", "the raw refresh token is never kept") + + want := storedRefresh(*pair.Refresh) + want.Consumed = consumed + + mustEqualRecord(t, storedRefresh(*rt), want, "record of refresh token "+pair.Refresh.TokenHash) +} + +// distinctPair is newPair with a value of its own in every field a lookup +// returns, so that a store returning one field for another is caught. +func distinctPair(name, family string) *oauth2.TokenPair { + pair := newPair(name, family) + now := time.Now() + + pair.Access.ClientID, pair.Refresh.ClientID = "client-"+name, "client-"+name + pair.Access.Subject, pair.Refresh.Subject = "subject-"+name, "subject-"+name + pair.Access.Scope, pair.Refresh.Scope = "read", "read write offline_access" + pair.Access.Audience = "https://api.example/" + name + pair.Access.IssuedAt, pair.Refresh.IssuedAt = now.Add(-time.Minute), now.Add(-2*time.Minute) + pair.Access.ExpiresAt, pair.Refresh.ExpiresAt = now.Add(15*time.Minute), now.Add(30*24*time.Hour) + + return pair +} + +// testTokenRecordsRoundTrip saves, rotates and looks up token pairs whose +// fields all differ: each lookup returns the full record it was saved with +// — the consumed ancestor too, with Consumed set — and an access token +// outside any family alike. +func testTokenRecordsRoundTrip(t *testing.T, s oauth2.Storage) { + first := distinctPair("records-0", "fam-records") + mustSave(t, s, first) + requireRecords(t, s, first, false) + + next := distinctPair("records-1", "fam-records") + next.Refresh.FamilyIssuedAt = first.Refresh.FamilyIssuedAt + mustRotate(t, s, first, next) + requireRecords(t, s, next, false) + requireRecords(t, s, first, true) + + cc := distinctPair("records-cc", "") + cc.Refresh = nil + mustSave(t, s, cc) + requireRecords(t, s, cc, false) +} + +// testRetainedOverTime checks again, retentionWait later, what a store +// must keep while it lasts — not only on the next call: a consumed code +// is still a replay (RFC 6749 §4.1.2), a rotated refresh token still a +// reuse that revokes its family (RFC 9700 §4.14.2), a family revoked +// before or after issuance still refuses new tokens (I2, I5), and a live +// pair is still found intact. +func testRetainedOverTime(t *testing.T, s oauth2.Storage) { + code := sampleCode("retained-code") + mustNoError(t, s.SaveAuthorizationCode(ctx(), code), "save code") + + _, err := s.ConsumeAuthorizationCode(ctx(), code.CodeHash) + mustNoError(t, err, "consume code") + + first, second := newPair("retained-0", "fam-retained"), newPair("retained-1", "fam-retained") + mustSave(t, s, first) + mustRotate(t, s, first, second) + + mustNoError(t, s.RevokeRefreshFamily(ctx(), "fam-retained-early"), "revoke a family before issuance") + + revoked := newPair("retained-revoked", "fam-retained-revoked") + mustSave(t, s, revoked) + mustNoError(t, s.RevokeRefreshFamily(ctx(), "fam-retained-revoked"), "revoke a family") + + live := newPair("retained-live", "fam-retained-live") + mustSave(t, s, live) + + time.Sleep(retentionWait) + + got, err := s.ConsumeAuthorizationCode(ctx(), code.CodeHash) + requireReplay(t, got, err, code, "replay the consumed code later") + + // The live pair and the rotated chain are still found intact. The live + // pair's check doubles that of second, and requireUnusable(revoked) + // below holds even for a store that lost the family (I8 fails its + // lookups closed): sanity checks, which the self-test does not pin on + // their own. + requireRecords(t, s, live, false) + requireRecords(t, s, second, false) + requireRecords(t, s, first, true) + + early := newPair("retained-early", "fam-retained-early") + mustErrorIs(t, s.SaveTokenPair(ctx(), early), oauth2.ErrTokenFamilyRevoked, + "issue into the family revoked before issuance, later") + requireAbsent(t, s, early) + + requireUnusable(t, s, revoked) + + late := newPair("retained-revoked-late", "fam-retained-revoked") + mustErrorIs(t, s.SaveTokenPair(ctx(), late), oauth2.ErrTokenFamilyRevoked, "issue into the revoked family, later") + requireAbsent(t, s, late) + + reuse := newPair("retained-2", "fam-retained") + err = s.RotateRefreshToken(ctx(), first.Refresh.TokenHash, reuse) + mustErrorIs(t, err, oauth2.ErrRefreshTokenReused, "reuse the rotated refresh token later") + mustNotErrorIs(t, err, oauth2.ErrTokenFamilyRevocationFailed, "reuse the rotated refresh token later") + requireAbsent(t, s, reuse) + requireUnusable(t, s, second) +} diff --git a/oauth2/storetest/selftest_test.go b/oauth2/storetest/selftest_test.go new file mode 100644 index 00000000..4d83ab10 --- /dev/null +++ b/oauth2/storetest/selftest_test.go @@ -0,0 +1,269 @@ +// 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_test + +import ( + "context" + "errors" + "os" + "os/exec" + "strings" + "sync" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" + "github.com/hyperscale-stack/security/oauth2/storage/memory" + "github.com/hyperscale-stack/security/oauth2/storetest" + "github.com/stretchr/testify/require" +) + +// lieEnv names the lie TestHelperLyingStore runs the suite against; the +// helper skips while it is unset. +const lieEnv = "STORETEST_HELPER_LIE" + +// forgetAfter is how long a forgetting liar keeps what it forgets: a +// second, as a store whose TTLs are a second — or a thousand times too +// short — would. +const forgetAfter = time.Second + +// liar is the in-memory store with one deliberate defect, named by lie: +// its lookups misreport one field of a token record, or it forgets a +// consumed code, a rotated refresh token (at rotation, or at lookup) or a +// revoked family — any, or only one revoked after issuance — after +// forgetAfter. The conformance suite must fail on every one of them. +type liar struct { + *memory.Store + + lie string + mu sync.Mutex + since map[string]time.Time // when a code was consumed, a token rotated, a family revoked + issued map[string]bool // the families tokens were issued into +} + +func newLiar(lie string) *liar { + return &liar{Store: memory.New(), lie: lie, since: map[string]time.Time{}, issued: map[string]bool{}} +} + +// record notes that key happened now; forgotten reports whether key +// happened more than forgetAfter ago, for the lie that forgets it. +func (l *liar) record(lie, key string) { + if l.lie != lie { + return + } + + l.mu.Lock() + defer l.mu.Unlock() + + l.since[key] = time.Now() +} + +func (l *liar) forgotten(lie, key string) bool { + if l.lie != lie { + return false + } + + l.mu.Lock() + defer l.mu.Unlock() + + at, ok := l.since[key] + + return ok && time.Since(at) > forgetAfter +} + +func (l *liar) ConsumeAuthorizationCode(ctx context.Context, hash string) (*oauth2.AuthorizationCode, error) { + if l.forgotten("forget-code", hash) { + return nil, oauth2.ErrCodeAlreadyUsed // an unknown code: no replay to report + } + + code, err := l.Store.ConsumeAuthorizationCode(ctx, hash) + if err == nil { + l.record("forget-code", hash) + } + + return code, err +} + +func (l *liar) RotateRefreshToken(ctx context.Context, oldHash string, next *oauth2.TokenPair) error { + if l.forgotten("forget-rotated", oldHash) { + return oauth2.ErrInvalidGrant.WithDescription("refresh token not found") + } + + err := l.Store.RotateRefreshToken(ctx, oldHash, next) + if err == nil { + l.record("forget-rotated", oldHash) + l.record("forget-consumed-token", oldHash) + } + + return err +} + +func (l *liar) RevokeRefreshFamily(ctx context.Context, familyID string) error { + err := l.Store.RevokeRefreshFamily(ctx, familyID) + if err == nil { + l.record("forget-revoked", familyID) + + l.mu.Lock() + issued := l.issued[familyID] + l.mu.Unlock() + + if issued { + l.record("forget-revoked-issued", familyID) + } + } + + return err +} + +// SaveTokenPair issues a pair of a forgotten family into a fresh one: what +// a store that lost the family's revoked state does. forget-revoked forgets +// every revoked family, forget-revoked-issued only those revoked after +// tokens were issued into them — a store keeping the revocation of a +// family with tokens under another TTL than that of an unknown one. +func (l *liar) SaveTokenPair(ctx context.Context, pair *oauth2.TokenPair) error { + if pair != nil { + l.mu.Lock() + l.issued[pair.Access.FamilyID] = true + l.mu.Unlock() + } + + if pair != nil && (l.forgotten("forget-revoked", pair.Access.FamilyID) || + l.forgotten("forget-revoked-issued", pair.Access.FamilyID)) { + reborn := *pair + reborn.Access.FamilyID += "/forgotten" + + if pair.Refresh != nil { + refresh := *pair.Refresh + refresh.FamilyID = reborn.Access.FamilyID + reborn.Refresh = &refresh + } + + return l.Store.SaveTokenPair(ctx, &reborn) + } + + return l.Store.SaveTokenPair(ctx, pair) +} + +func (l *liar) LookupAccessToken(ctx context.Context, hash string) (*oauth2.AccessToken, error) { + at, err := l.Store.LookupAccessToken(ctx, hash) + if err != nil { + return at, err + } + + switch l.lie { + case "expires": + at.ExpiresAt = at.ExpiresAt.Add(100 * 365 * 24 * time.Hour) + case "issued-at": + at.IssuedAt = at.IssuedAt.Add(-time.Hour) + case "client": + at.ClientID = "another-client" + case "scope": + at.Scope += " admin" + case "audience": + at.Audience = "another-api" + } + + return at, nil +} + +func (l *liar) LookupRefreshToken(ctx context.Context, hash string) (*oauth2.RefreshToken, error) { + if l.forgotten("forget-consumed-token", hash) { + return nil, oauth2.ErrInvalidGrant.WithDescription("refresh token not found") + } + + rt, err := l.Store.LookupRefreshToken(ctx, hash) + if err != nil { + return rt, err + } + + switch l.lie { + case "expires": + rt.ExpiresAt = rt.ExpiresAt.Add(100 * 365 * 24 * time.Hour) + case "client": + rt.ClientID = "another-client" + case "subject": + rt.Subject = "mallory" + case "consumed": + rt.Consumed = !rt.Consumed + case "family-issued-at": + rt.FamilyIssuedAt = rt.IssuedAt + } + + return rt, nil +} + +// TestHelperLyingStore is the process TestConformanceCatchesLyingStores +// runs: the conformance suite against the liar its environment names. +func TestHelperLyingStore(t *testing.T) { + lie := os.Getenv(lieEnv) + if lie == "" { + t.Skip("the helper process of TestConformanceCatchesLyingStores") + } + + storetest.RunConformance(t, func() oauth2.Storage { return newLiar(lie) }) +} + +// TestConformanceCatchesLyingStores runs the suite, in a process of its +// own, against stores lying about one security field of a token record or +// forgetting what they must retain (the gaps of the audit's F-23), and +// asserts that the case pinning it fails, naming the defect. The honest +// store passes, so every failure is the lie's. +func TestConformanceCatchesLyingStores(t *testing.T) { + t.Parallel() + + const ( + records = "TokenRecordsRoundTrip" + retention = "RetainedOverTime" + ) + + for _, c := range []struct { + lie, caseName, want string + }{ + {"none", records, ""}, + {"none", retention, ""}, + {"expires", records, "fields [ExpiresAt] differ"}, + {"issued-at", records, "fields [IssuedAt] differ"}, + {"client", records, "fields [ClientID] differ"}, + {"scope", records, "fields [Scope] differ"}, + {"audience", records, "fields [Audience] differ"}, + {"subject", records, "fields [Subject] differ"}, + {"consumed", records, "fields [Consumed] differ"}, + {"family-issued-at", records, "fields [FamilyIssuedAt] differ"}, + {"forget-code", retention, "replay the consumed code later"}, + {"forget-rotated", retention, "reuse the rotated refresh token later"}, + {"forget-consumed-token", retention, "lookup refresh token rt-retained-0"}, + {"forget-revoked", retention, "issue into the family revoked before issuance, later"}, + {"forget-revoked-issued", retention, "issue into the revoked family, later"}, + } { + t.Run(c.lie+"/"+c.caseName, func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Minute) + defer cancel() + + cmd := exec.CommandContext(ctx, os.Args[0], //nolint:gosec // the test binary itself + "-test.run=^TestHelperLyingStore$/^"+c.caseName+"$", "-test.v", "-test.count=1") + cmd.Env = append(os.Environ(), lieEnv+"="+c.lie) + + out, err := cmd.CombinedOutput() + output := string(out) + + require.True(t, strings.Contains(output, "=== RUN TestHelperLyingStore/"+c.caseName), + "the helper must run the case %s:\n%s", c.caseName, output) + + if c.want == "" { + require.NoError(t, err, "the honest store must pass %s:\n%s", c.caseName, output) + + return + } + + var exit *exec.ExitError + + require.True(t, errors.As(err, &exit), "the suite passed a store that lies (%s):\n%s", c.lie, output) + require.Contains(t, output, "--- FAIL: TestHelperLyingStore/"+c.caseName) + require.Contains(t, output, c.want) + }) + } +} diff --git a/oidc/client/storage/memory/memory.go b/oidc/client/storage/memory/memory.go index 64ad7bb3..02f2a575 100644 --- a/oidc/client/storage/memory/memory.go +++ b/oidc/client/storage/memory/memory.go @@ -130,6 +130,10 @@ func (s *Store) Save(_ context.Context, tx *oidcclient.Transaction) error { // Consume implements oidcclient.TransactionStore under the store's mutex. func (s *Store) Consume(_ context.Context, stateHash, bindingHash string, now time.Time) (*oidcclient.Transaction, error) { + if err := validateKeys(stateHash, bindingHash); err != nil { + return nil, err + } + s.mu.Lock() defer s.mu.Unlock() @@ -155,6 +159,19 @@ func (s *Store) Consume(_ context.Context, stateHash, bindingHash string, now ti return &tx, nil } +// validateKeys refuses the keys oidcclient.ValidateStoreKey refuses. +func validateKeys(stateHash, bindingHash string) error { + if err := oidcclient.ValidateStoreKey(stateHash); err != nil { + return fmt.Errorf("memory: consume: state hash: %w", err) + } + + if err := oidcclient.ValidateStoreKey(bindingHash); err != nil { + return fmt.Errorf("memory: consume: binding hash: %w", err) + } + + return nil +} + // Len returns the number of transactions the store retains, expired ones // not yet swept included. func (s *Store) Len() int { diff --git a/oidc/client/store/redis/store.go b/oidc/client/store/redis/store.go index d32b747b..3edc1d09 100644 --- a/oidc/client/store/redis/store.go +++ b/oidc/client/store/redis/store.go @@ -113,8 +113,17 @@ func (s *Store) Save(ctx context.Context, tx *oidcclient.Transaction) error { } // Consume implements [oidcclient.TransactionStore] with one script run -// (see [Store]). +// (see [Store]), after refusing, before any I/O, keys +// [oidcclient.ValidateStoreKey] refuses. func (s *Store) Consume(ctx context.Context, stateHash, bindingHash string, now time.Time) (*oidcclient.Transaction, error) { + if err := oidcclient.ValidateStoreKey(stateHash); err != nil { + return nil, fmt.Errorf("redisstore: consume transaction: state hash: %w", err) + } + + if err := oidcclient.ValidateStoreKey(bindingHash); err != nil { + return nil, fmt.Errorf("redisstore: consume transaction: binding hash: %w", err) + } + res, err := consumeScript.Run(ctx, s.rdb, []string{s.key(stateHash)}, bindingHash, now.Unix(), rand.Text()).Text() switch { diff --git a/oidc/client/store/redis/store_test.go b/oidc/client/store/redis/store_test.go index ee12fcfe..d0c9de1e 100644 --- a/oidc/client/store/redis/store_test.go +++ b/oidc/client/store/redis/store_test.go @@ -9,6 +9,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "strings" "sync/atomic" "testing" "time" @@ -341,3 +342,28 @@ func (h replyHook) ProcessHook(next redis.ProcessHook) redis.ProcessHook { return next(ctx, cmd) } } + +// TestConsumeValidatesKeysBeforeIO: with a closed client, a malformed key +// is refused as invalid — the refusal comes before any I/O — while a +// well-formed one reaches the client and fails there. +func TestConsumeValidatesKeysBeforeIO(t *testing.T) { + t.Parallel() + + client := redis.NewClient(&redis.Options{Addr: miniredis.RunT(t).Addr()}) + require.NoError(t, client.Close()) + + store, err := redisstore.New(client) + require.NoError(t, err) + + now := time.Now() + good := hash("state") + + _, err = store.Consume(t.Context(), strings.ToUpper(good), hash("binding"), now) + require.ErrorIs(t, err, oidcclient.ErrInvalidTransaction, "malformed state hash") + + _, err = store.Consume(t.Context(), good, good[:63], now) + require.ErrorIs(t, err, oidcclient.ErrInvalidTransaction, "malformed binding hash") + + _, err = store.Consume(t.Context(), good, hash("binding"), now) + require.ErrorIs(t, err, redis.ErrClosed, "well-formed keys reach the closed client") +} diff --git a/oidc/client/store/sql/deployment.go b/oidc/client/store/sql/deployment.go new file mode 100644 index 00000000..0df2d4be --- /dev/null +++ b/oidc/client/store/sql/deployment.go @@ -0,0 +1,74 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "context" + "database/sql" + "errors" + "fmt" + "strings" +) + +// ErrDeploymentRequirement reports a deployment requirement of the package +// documentation that the database does not meet. [Store.CheckDeployment] +// and [Store.Migrate] wrap it with the requirement and the table at fault. +var ErrDeploymentRequirement = errors.New("sqlstore: deployment requirement not met") + +// CheckDeployment verifies the deployment requirement of the package +// documentation that the database reports, and returns an error matching +// [ErrDeploymentRequirement] when it is not met — or the error of the +// database. On MySQL and MariaDB, oidc_client_transactions exists in the +// connection's database and uses InnoDB, whose row locks Consume takes and +// whose crash safety keeps a consumption once acknowledged. PostgreSQL and +// SQLite report nothing it checks. [Store.Migrate] calls it once the table +// exists; when you apply [Schema] with your own migration tool, call it at +// boot, before serving traffic. +func (s *Store) CheckDeployment(ctx context.Context) error { + return s.dialect.checkDeployment(ctx, s.db) +} + +// querier runs the queries of a deployment check: the store's *sql.DB, or +// the *sql.Conn of a Migrate holding its named lock. +type querier interface { + QueryContext(ctx context.Context, query string, args ...any) (*sql.Rows, error) + QueryRowContext(ctx context.Context, query string, args ...any) *sql.Row +} + +// storeTable is the table of the store. +const storeTable = "oidc_client_transactions" + +// selectEngine reads the storage engine of storeTable. +const selectEngine = `SELECT COALESCE(ENGINE, '') FROM information_schema.TABLES + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = ?` + +// checkMySQL reads the storage engine of the store's table, then checks it. +func checkMySQL(ctx context.Context, q querier) error { + var engine string + + err := q.QueryRowContext(ctx, selectEngine, storeTable).Scan(&engine) + + switch { + case errors.Is(err, sql.ErrNoRows): + return checkEngine("", false) + case err != nil: + return fmt.Errorf("sqlstore: check deployment: storage engine: %w", err) + default: + return checkEngine(engine, true) + } +} + +// checkEngine reports a store table that is missing, or whose engine is +// not InnoDB. +func checkEngine(engine string, found bool) error { + switch { + case !found: + return fmt.Errorf("%w: table %s not found in the connection's database", ErrDeploymentRequirement, storeTable) + case !strings.EqualFold(engine, "InnoDB"): + return fmt.Errorf("%w: table %s uses the %s engine, not InnoDB", ErrDeploymentRequirement, storeTable, engine) + default: + return nil + } +} diff --git a/oidc/client/store/sql/dialect.go b/oidc/client/store/sql/dialect.go index 0c5ce8fe..3375742c 100644 --- a/oidc/client/store/sql/dialect.go +++ b/oidc/client/store/sql/dialect.go @@ -5,6 +5,7 @@ package sqlstore import ( + "context" "database/sql" "strconv" "strings" @@ -30,6 +31,20 @@ type Dialect interface { // "" when the engine serializes the creation of a table itself and // Migrate runs its statement on its own. migrationLock() string + // tableOptions follows the column list of the CREATE TABLE of + // [Schema]: the storage engine and character set the dialect needs, + // "" when it needs none. + tableOptions() string + // namedMigrationLock is the session-level named lock [Store.Migrate] + // takes around the creation of the tables and its deployment check, on + // an engine whose DDL cannot run in a transaction; "" for none. + namedMigrationLock() string + // checkDeployment verifies the requirements of the package + // documentation the database reports (see [Store.CheckDeployment]). + checkDeployment(ctx context.Context, q querier) error + // purgeTxOptions are the options of the [Store.PurgeExpired] + // transaction. + purgeTxOptions() *sql.TxOptions } // migrationLockKey is the PostgreSQL advisory lock [Store.Migrate] takes: @@ -54,7 +69,9 @@ const selectTransaction = `SELECT state_hash, binding_hash, connection_id, issue var Postgres Dialect = postgres{} // MySQL is the MySQL / MariaDB dialect (? placeholders). The table must be -// InnoDB: Consume relies on its row locks. +// InnoDB — Consume relies on its row locks — and [Schema] creates it so +// (ENGINE=InnoDB DEFAULT CHARSET=utf8mb4); [Store.CheckDeployment] +// verifies it. var MySQL Dialect = mysql{} // SQLite is the SQLite dialect (? placeholders). SQLite has no row locks: @@ -105,6 +122,17 @@ func (postgres) migrationLock() string { return "SELECT pg_advisory_xact_lock(" + migrationLockKey + ")" } +func (postgres) tableOptions() string { return "" } + +func (postgres) purgeTxOptions() *sql.TxOptions { + return &sql.TxOptions{Isolation: sql.LevelReadCommitted} +} + +// checkDeployment checks nothing: the store pins READ COMMITTED itself. +func (postgres) checkDeployment(context.Context, querier) error { return nil } + +func (postgres) namedMigrationLock() string { return "" } + type mysql struct{} func (mysql) Name() string { return "mysql" } @@ -112,10 +140,29 @@ func (mysql) rebind(q string) string { return q } func (mysql) lockRow() string { return selectTransaction + " FOR UPDATE" } func (mysql) txOptions() *sql.TxOptions { return nil } -// migrationLock is empty: MySQL commits every DDL statement on its own, -// and serializes the creation of a table on the metadata lock of its name. +// migrationLock is empty: MySQL commits every DDL statement on its own, so +// Migrate takes a named lock instead (see namedMigrationLock). func (mysql) migrationLock() string { return "" } +// tableOptions names the engine and the character set: a table created +// with the server's default engine would be MyISAM on a server configured +// so, without transactions, row locks or crash safety. +func (mysql) tableOptions() string { return " ENGINE=InnoDB DEFAULT CHARSET=utf8mb4" } + +func (mysql) checkDeployment(ctx context.Context, q querier) error { return checkMySQL(ctx, q) } + +// namedMigrationLock serializes the Migrate calls of a deployment: MySQL +// commits every DDL statement on its own, so no transaction can hold a +// lock across them, and concurrent CREATE TABLE IF NOT EXISTS of one +// table can fail with a metadata-lock deadlock (1213). +func (mysql) namedMigrationLock() string { return migrationLockName } + +// purgeTxOptions pins READ COMMITTED: at REPEATABLE READ the purge locks +// the gaps it scans, blocking the inserts of Save until it commits. +func (mysql) purgeTxOptions() *sql.TxOptions { + return &sql.TxOptions{Isolation: sql.LevelReadCommitted} +} + type sqlite struct{} func (sqlite) Name() string { return "sqlite" } @@ -131,3 +178,12 @@ func (sqlite) txOptions() *sql.TxOptions { return nil } // of the table, and a statement prepared before a concurrent change of the // schema is prepared again. func (sqlite) migrationLock() string { return "" } + +func (sqlite) tableOptions() string { return "" } + +func (sqlite) purgeTxOptions() *sql.TxOptions { return nil } + +// checkDeployment checks nothing: SQLite has one engine. +func (sqlite) checkDeployment(context.Context, querier) error { return nil } + +func (sqlite) namedMigrationLock() string { return "" } diff --git a/oidc/client/store/sql/doc.go b/oidc/client/store/sql/doc.go index 99917d6d..c53ebddf 100644 --- a/oidc/client/store/sql/doc.go +++ b/oidc/client/store/sql/doc.go @@ -24,12 +24,13 @@ // // - PostgreSQL: Consume runs at READ COMMITTED whatever the server // default (see [Postgres]). -// - MySQL: the table must use InnoDB (row locks); utf8mb4 is not +// - MySQL: the table must use InnoDB (row locks, crash safety) — +// [Schema] names it, and [Store.CheckDeployment], which +// [Store.Migrate] runs, refuses another engine; utf8mb4 is not // required — every column holds ASCII. Nor is a binary collation: the -// keys are 64 lowercase hex digits — validated at Save; oidcclient -// passes the canonical hex(SHA-256(state)) to Consume, and the binding -// hash is compared in Go, in constant time — so no collation takes one -// key for another. +// keys are 64 lowercase hex digits — validated by Save and by Consume +// (oidcclient.ValidateStoreKey), and the binding hash is compared in +// Go, in constant time — so no collation takes one key for another. // - SQLite: one writer at a time. Set a busy timeout (with // modernc.org/sqlite, the _pragma=busy_timeout(5000) DSN option) so a // concurrent consumer waits instead of failing with SQLITE_BUSY; a busy @@ -48,11 +49,15 @@ // Consume deletes the transactions it reaches; an abandoned login keeps // its row until [Store.PurgeExpired] deletes it, so schedule that call. // The DDL creates no index — MySQL has no CREATE INDEX IF NOT EXISTS — so -// add one on expires_at through your migration tool: without it, the purge -// scans the table and, on MySQL, locks every row it scans, holding back the -// logins starting or completing meanwhile. A transaction lives minutes, -// so the table stays small when the purge runs every few minutes; the -// purge is one DELETE, not batched. +// add one on expires_at through your migration tool: +// +// CREATE INDEX oidc_client_transactions_expires_at ON oidc_client_transactions (expires_at); +// +// Without it the purge scans the table and, on MySQL, waits for every row +// a login completing meanwhile holds; it runs at READ COMMITTED, so it does +// not lock the gaps it scans, which would hold back the logins starting. A +// transaction lives minutes, so the table stays small when the purge runs +// every few minutes; the purge is one DELETE, not batched. // // Allowed dependencies: // - github.com/hyperscale-stack/security/oidc/client diff --git a/oidc/client/store/sql/migrate_internal_test.go b/oidc/client/store/sql/migrate_internal_test.go index 5bfa166e..112e1219 100644 --- a/oidc/client/store/sql/migrate_internal_test.go +++ b/oidc/client/store/sql/migrate_internal_test.go @@ -15,6 +15,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" _ "modernc.org/sqlite" ) @@ -57,7 +58,10 @@ func lockingStore(t *testing.T, lock string) (*Store, *sql.DB) { t.Cleanup(func() { _ = db.Close() }) d := NewMockDialect(t) + d.EXPECT().namedMigrationLock().Return("") d.EXPECT().migrationLock().Return(lock) + d.EXPECT().tableOptions().Return("").Maybe() + d.EXPECT().checkDeployment(mock.Anything, db).Return(nil).Maybe() return &Store{db: db, dialect: d}, db } @@ -190,3 +194,68 @@ func TestMigrateReportsAFailedBegin(t *testing.T) { require.ErrorContains(t, store.Migrate(t.Context()), "sqlstore: migrate: begin: ") } + +// TestMigrateEndsWithTheDeploymentCheck: once the table exists, Migrate +// runs the dialect's deployment check and fails with what it reports. +func TestMigrateEndsWithTheDeploymentCheck(t *testing.T) { + t.Parallel() + + db, err := sql.Open("sqlite", "file:"+filepath.Join(t.TempDir(), "check.db")) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + unmet := checkEngine("MyISAM", true) + + d := NewMockDialect(t) + d.EXPECT().namedMigrationLock().Return("") + d.EXPECT().migrationLock().Return("") + d.EXPECT().tableOptions().Return("") + d.EXPECT().checkDeployment(mock.Anything, db).Return(unmet).Once() + + require.ErrorIs(t, (&Store{db: db, dialect: d}).Migrate(t.Context()), unmet) +} + +func TestCheckEngine(t *testing.T) { + t.Parallel() + + require.NoError(t, checkEngine("InnoDB", true)) + require.NoError(t, checkEngine("innodb", true)) + + for _, c := range []struct { + engine string + found bool + want string + }{ + {"MyISAM", true, "table oidc_client_transactions uses the MyISAM engine, not InnoDB"}, + {"", true, "table oidc_client_transactions uses the engine, not InnoDB"}, + {"", false, "table oidc_client_transactions not found in the connection's database"}, + } { + err := checkEngine(c.engine, c.found) + require.ErrorIs(t, err, ErrDeploymentRequirement) + assert.Equal(t, "sqlstore: deployment requirement not met: "+c.want, err.Error()) + } +} + +// TestCheckDeploymentPerDialect: PostgreSQL and SQLite report nothing the +// store checks; MySQL's check reaches the database, whose failure is +// returned as such, not as a requirement found unmet. +func TestCheckDeploymentPerDialect(t *testing.T) { + t.Parallel() + + db, err := sql.Open("sqlite", ":memory:") + require.NoError(t, err) + require.NoError(t, db.Close()) + + for _, d := range []Dialect{Postgres, SQLite} { + store, err := New(db, d) + require.NoError(t, err) + require.NoError(t, store.CheckDeployment(t.Context()), d.Name()) + } + + store, err := New(db, MySQL) + require.NoError(t, err) + + err = store.CheckDeployment(t.Context()) + require.ErrorContains(t, err, "sqlstore: check deployment: storage engine: ") + require.NotErrorIs(t, err, ErrDeploymentRequirement) +} diff --git a/oidc/client/store/sql/namedlock.go b/oidc/client/store/sql/namedlock.go new file mode 100644 index 00000000..6e8a9646 --- /dev/null +++ b/oidc/client/store/sql/namedlock.go @@ -0,0 +1,113 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "context" + "database/sql" + "database/sql/driver" + "fmt" + "math" + "time" +) + +// migrationLockName is the named lock [Store.Migrate] takes on MySQL and +// MariaDB (GET_LOCK): the module path without its host, then ".Migrate". +// Named locks are server-wide and each store module has its own, so that +// their migrations never wait for one another. +const migrationLockName = "hyperscale-stack/security/oidc/client/store/sql.Migrate" + +// lockWaitForever is the GET_LOCK timeout, in seconds, under a context +// without deadline: a year — the wait lasts as long as the context does. +const lockWaitForever = 365 * 24 * 60 * 60 + +// releaseTimeout bounds the release of a named lock, which runs even once +// the context of Migrate has ended. +const releaseTimeout = 10 * time.Second + +// lockWaitSeconds is the GET_LOCK timeout under ctx: the seconds left until +// its deadline, rounded up and at least one, or lockWaitForever. +func lockWaitSeconds(ctx context.Context) int64 { + deadline, ok := ctx.Deadline() + if !ok { + return lockWaitForever + } + + return max(1, int64(math.Ceil(time.Until(deadline).Seconds()))) +} + +// migrateUnderNamedLock applies [Schema] and runs the deployment check on +// one connection holding the named lock name, which it releases before the +// connection goes back to the pool: concurrent calls run one after the +// other. +func (s *Store) migrateUnderNamedLock(ctx context.Context, name string) error { + conn, err := s.db.Conn(ctx) + if err != nil { + return fmt.Errorf("sqlstore: migrate: connection: %w", err) + } + + defer func() { _ = conn.Close() }() + + if err := acquireNamedLock(ctx, conn, name); err != nil { + return err + } + + defer releaseNamedLock(ctx, conn, name) + + for _, stmt := range Schema(s.dialect) { + if _, err := conn.ExecContext(ctx, stmt); err != nil { + return fmt.Errorf("sqlstore: migrate: %w", err) + } + } + + return s.dialect.checkDeployment(ctx, conn) +} + +// acquireNamedLock takes the named lock name on conn, waiting as long as +// ctx allows: the server's wait is bounded by its deadline, and the driver +// cancels it when ctx ends. +func acquireNamedLock(ctx context.Context, conn *sql.Conn, name string) error { + var granted sql.NullInt64 + + if err := conn.QueryRowContext(ctx, `SELECT GET_LOCK(?, ?)`, name, lockWaitSeconds(ctx)).Scan(&granted); err != nil { + // The server may have granted the lock before the reply was lost: + // never pool a session that may hold it. + discard(conn) + + return fmt.Errorf("sqlstore: migrate: lock %s: %w", name, err) + } + + switch { + case !granted.Valid: + return fmt.Errorf("sqlstore: migrate: lock %s: not granted", name) + case granted.Int64 != 1: + return fmt.Errorf("sqlstore: migrate: lock %s: %w", name, context.DeadlineExceeded) + default: + return nil + } +} + +// releaseNamedLock releases the named lock name held on conn, even once +// ctx has ended. A release that fails discards the connection: its session +// would otherwise keep the lock from the pool. +func releaseNamedLock(ctx context.Context, conn *sql.Conn, name string) { + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), releaseTimeout) + defer cancel() + + var released sql.NullInt64 + + err := conn.QueryRowContext(ctx, `SELECT RELEASE_LOCK(?)`, name).Scan(&released) + if err == nil && released.Valid && released.Int64 == 1 { + return + } + + discard(conn) +} + +// discard closes conn instead of returning it to the pool when it is +// closed. +func discard(conn *sql.Conn) { + _ = conn.Raw(func(any) error { return driver.ErrBadConn }) +} diff --git a/oidc/client/store/sql/namedlock_internal_test.go b/oidc/client/store/sql/namedlock_internal_test.go new file mode 100644 index 00000000..fc07eed3 --- /dev/null +++ b/oidc/client/store/sql/namedlock_internal_test.go @@ -0,0 +1,222 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "context" + "database/sql" + "database/sql/driver" + "errors" + "fmt" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + moderncsqlite "modernc.org/sqlite" +) + +// fakeLocks stands in for MySQL's GET_LOCK and RELEASE_LOCK on SQLite: the +// lock name picks the server's answer, and every call is recorded. +var fakeLocks = struct { + sync.Mutex + + calls []string +}{} + +// The lock names of the fake and the answer they get. +const ( + lockGranted = "granted" // GET_LOCK 1, RELEASE_LOCK 1 + lockTimedOut = "timed-out" // GET_LOCK 0 + lockNull = "null" // GET_LOCK NULL + lockFails = "fails" // GET_LOCK errors + lockReleaseFails = "release-fails" // GET_LOCK 1, RELEASE_LOCK 0 +) + +func init() { + moderncsqlite.MustRegisterScalarFunction("GET_LOCK", 2, func(_ *moderncsqlite.FunctionContext, args []driver.Value) (driver.Value, error) { + name, _ := args[0].(string) + record(fmt.Sprintf("GET_LOCK(%s, %v)", name, args[1])) + + switch name { + case lockTimedOut: + return int64(0), nil + case lockNull: + return nil, nil + case lockFails: + return nil, errors.New("lock service down") + default: + return int64(1), nil + } + }) + moderncsqlite.MustRegisterScalarFunction("RELEASE_LOCK", 1, func(_ *moderncsqlite.FunctionContext, args []driver.Value) (driver.Value, error) { + name, _ := args[0].(string) + record("RELEASE_LOCK(" + name + ")") + + if name == lockReleaseFails { + return int64(0), nil + } + + return int64(1), nil + }) +} + +func record(call string) { + fakeLocks.Lock() + defer fakeLocks.Unlock() + + fakeLocks.calls = append(fakeLocks.calls, call) +} + +// lockCalls returns the recorded calls naming lock. +func lockCalls(lock string) []string { + fakeLocks.Lock() + defer fakeLocks.Unlock() + + var calls []string + + for _, c := range fakeLocks.calls { + if strings.HasPrefix(c, "GET_LOCK("+lock+", ") || c == "RELEASE_LOCK("+lock+")" { + calls = append(calls, c) + } + } + + return calls +} + +// namedLockStore returns a store over a new SQLite database file whose +// dialect — a mock — names lock as its named migration lock, so that +// Migrate takes the MySQL path: lock, tables, check, release, on one +// connection. check is what the deployment check returns. +func namedLockStore(t *testing.T, lock string, check error) (*Store, *sql.DB) { + t.Helper() + + db, err := sql.Open("sqlite", "file:"+filepath.Join(t.TempDir(), "named.db")) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + d := NewMockDialect(t) + d.EXPECT().namedMigrationLock().Return(lock) + d.EXPECT().tableOptions().Return("").Maybe() + d.EXPECT().checkDeployment(mock.Anything, mock.AnythingOfType("*sql.Conn")).Return(check).Maybe() + + return &Store{db: db, dialect: d}, db +} + +// TestMigrateUnderNamedLock: Migrate takes the named lock, waiting as long +// as its context allows, creates the tables, runs the deployment check on +// the connection holding the lock, and releases it — after a failed check +// too. +func TestMigrateUnderNamedLock(t *testing.T) { + store, db := namedLockStore(t, lockGranted, nil) + + require.NoError(t, store.Migrate(context.Background())) + assert.Equal(t, schemaTables, tables(t, db)) + assert.Equal(t, []string{ + fmt.Sprintf("GET_LOCK(%s, %d)", lockGranted, lockWaitForever), "RELEASE_LOCK(" + lockGranted + ")", + }, lockCalls(lockGranted)) + assert.Equal(t, 1, db.Stats().Idle, "the connection goes back to the pool") + + unmet := fmt.Errorf("%w: table oidc_client_transactions uses the MyISAM engine, not InnoDB", ErrDeploymentRequirement) + failing, _ := namedLockStore(t, lockGranted+"-check", unmet) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + require.ErrorIs(t, failing.Migrate(ctx), unmet) + assert.Equal(t, []string{ + "GET_LOCK(" + lockGranted + "-check, 30)", "RELEASE_LOCK(" + lockGranted + "-check)", + }, lockCalls(lockGranted+"-check"), "the wait follows the deadline; a failed check releases the lock") +} + +// TestMigrateNamedLockRefused: a lock the server does not grant — timed +// out, NULL, an error — stops Migrate before any table is created, and +// nothing is released. +func TestMigrateNamedLockRefused(t *testing.T) { + for lock, want := range map[string]string{ + lockTimedOut: "sqlstore: migrate: lock timed-out: context deadline exceeded", + lockNull: "sqlstore: migrate: lock null: not granted", + lockFails: "sqlstore: migrate: lock fails: SQL logic error: lock service down (1)", + } { + store, db := namedLockStore(t, lock, nil) + + err := store.Migrate(context.Background()) + require.EqualError(t, err, want) + assert.Empty(t, tables(t, db), lock) + assert.NotContains(t, lockCalls(lock), "RELEASE_LOCK("+lock+")", lock) + } + + store, _ := namedLockStore(t, lockTimedOut, nil) + require.ErrorIs(t, store.Migrate(context.Background()), context.DeadlineExceeded) +} + +// TestMigrateDiscardsAConnectionWhoseLockIsUnknown: a GET_LOCK that fails +// leaves the lock's state unknown — the server may have granted it before +// the reply was lost — so the connection is discarded, never pooled with +// the server-wide migration lock possibly held. +func TestMigrateDiscardsAConnectionWhoseLockIsUnknown(t *testing.T) { + store, db := namedLockStore(t, lockFails, nil) + + require.Error(t, store.Migrate(context.Background())) + assert.Zero(t, db.Stats().OpenConnections, "the connection is closed, not pooled") + + timedOut, timedOutDB := namedLockStore(t, lockTimedOut, nil) + + require.Error(t, timedOut.Migrate(context.Background())) + assert.Equal(t, 1, timedOutDB.Stats().Idle, "a lock refused for sure keeps its connection") +} + +// TestMigrateNamedLockFailures: no connection, or a failing statement, +// fails Migrate; a failing statement releases the lock first. +func TestMigrateNamedLockFailures(t *testing.T) { + store, db := namedLockStore(t, lockGranted+"-closed", nil) + require.NoError(t, db.Close()) + require.ErrorContains(t, store.Migrate(context.Background()), "sqlstore: migrate: connection: ") + + store, db = namedLockStore(t, lockGranted+"-blocked", nil) + last := schemaTables[len(schemaTables)-1] + execAll(t, db, `CREATE TABLE blocker (x INTEGER)`, `CREATE INDEX `+last+` ON blocker (x)`) + + require.ErrorContains(t, store.Migrate(context.Background()), "already an index named "+last) + assert.Contains(t, lockCalls(lockGranted+"-blocked"), "RELEASE_LOCK("+lockGranted+"-blocked)") +} + +// TestMigrateDiscardsAConnectionKeepingTheLock: a release the server +// refuses discards the connection, whose session would keep the lock. +func TestMigrateDiscardsAConnectionKeepingTheLock(t *testing.T) { + store, db := namedLockStore(t, lockReleaseFails, nil) + + require.NoError(t, store.Migrate(context.Background())) + assert.Contains(t, lockCalls(lockReleaseFails), "RELEASE_LOCK("+lockReleaseFails+")") + assert.Zero(t, db.Stats().OpenConnections, "the connection is closed, not pooled") +} + +func TestLockWaitSeconds(t *testing.T) { + t.Parallel() + + assert.Equal(t, int64(lockWaitForever), lockWaitSeconds(context.Background())) + + ctx, cancel := context.WithTimeout(context.Background(), 1500*time.Millisecond) + defer cancel() + + assert.Equal(t, int64(2), lockWaitSeconds(ctx), "rounded up") + + expired, cancelExpired := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancelExpired() + + assert.Equal(t, int64(1), lockWaitSeconds(expired), "at least one second") +} + +func TestNamedMigrationLock(t *testing.T) { + t.Parallel() + + assert.Equal(t, "hyperscale-stack/security/oidc/client/store/sql.Migrate", MySQL.namedMigrationLock()) + assert.Empty(t, Postgres.namedMigrationLock()) + assert.Empty(t, SQLite.namedMigrationLock()) +} diff --git a/oidc/client/store/sql/schema.go b/oidc/client/store/sql/schema.go index a88e8fbd..8dfbcaf9 100644 --- a/oidc/client/store/sql/schema.go +++ b/oidc/client/store/sql/schema.go @@ -18,15 +18,22 @@ import ( // holds a secret (see oidcclient.Transaction). // // The DDL sticks to the subset every supported engine accepts, so it is -// the same for every [Dialect]: the argument is accepted for API stability -// and ignored. It creates no index — MySQL has no CREATE INDEX IF NOT -// EXISTS — see the package documentation for the one on expires_at. +// the same for every [Dialect] but for the table options of MySQL: +// ENGINE=InnoDB DEFAULT CHARSET=utf8mb4, since a server whose default +// engine is MyISAM would otherwise create a table without row locks or +// crash safety. A nil Dialect gets the PostgreSQL / SQLite DDL. It creates +// no index — MySQL has no CREATE INDEX IF NOT EXISTS — see the package +// documentation for the one on expires_at. // // The statement is idempotent (CREATE TABLE IF NOT EXISTS) and valid // inside a PostgreSQL transaction block, where [Store.Migrate] runs it. // Production deployments typically run it through a migration tool rather // than via Migrate. -func Schema(_ Dialect) []string { +func Schema(d Dialect) []string { + if d == nil { + d = sqlite{} + } + return []string{ `CREATE TABLE IF NOT EXISTS oidc_client_transactions ( state_hash VARCHAR(64) PRIMARY KEY, @@ -42,12 +49,14 @@ func Schema(_ Dialect) []string { created_at BIGINT NOT NULL, expires_at BIGINT NOT NULL, integrity VARCHAR(64) NOT NULL - )`, + )` + d.tableOptions(), } } // Migrate applies [Schema] to the store's database: it creates the table -// when missing and never alters it. +// when missing and never alters it. It then runs [Store.CheckDeployment], +// and fails with its error: a table that exists already keeps its engine, +// so a deployment that does not meet the requirements is refused at boot. // // It is safe to call repeatedly, and concurrently — every instance of a // deployment starting at once. On PostgreSQL, where concurrent CREATE @@ -60,11 +69,34 @@ func Schema(_ Dialect) []string { // server-side wait when ctx ends. A migration tool applying [Schema] // concurrently with Migrate should take the same lock. An engine without // pg_advisory_xact_lock cannot use Migrate: apply Schema with your own -// tool. MySQL serializes the creation of a table on the metadata lock of -// its name, and SQLite on its write lock — given the busy timeout the -// package documentation requires, without which a concurrent call fails -// with SQLITE_BUSY. +// tool. On MySQL and MariaDB, where concurrent CREATE TABLE IF NOT EXISTS +// of one table can fail with a metadata-lock deadlock, Migrate creates the +// tables and runs the check on one connection holding the named lock +// "hyperscale-stack/security/oidc/client/store/sql.Migrate" (GET_LOCK; named +// locks are server-wide), which it releases before returning: concurrent +// calls run one after the other, waiting as long as ctx allows. A holder +// whose client vanished keeps the lock until the server notices +// (wait_timeout): find it with SELECT IS_USED_LOCK('') and KILL it. +// On a Galera or Percona XtraDB cluster named locks are local to a node, so +// Migrate calls through different nodes are not serialized. SQLite +// serializes the creation of a table on its write lock — given the busy +// timeout the package documentation requires, without which a concurrent +// call fails with SQLITE_BUSY. func (s *Store) Migrate(ctx context.Context) error { + if name := s.dialect.namedMigrationLock(); name != "" { + return s.migrateUnderNamedLock(ctx, name) + } + + if err := s.createTable(ctx); err != nil { + return err + } + + return s.CheckDeployment(ctx) +} + +// createTable applies [Schema]: under the dialect's migration lock when it +// has one, on its own otherwise. +func (s *Store) createTable(ctx context.Context) error { if lock := s.dialect.migrationLock(); lock != "" { return s.migrateLocked(ctx, lock) } diff --git a/oidc/client/store/sql/store.go b/oidc/client/store/sql/store.go index 4ac6c88c..9638cfd6 100644 --- a/oidc/client/store/sql/store.go +++ b/oidc/client/store/sql/store.go @@ -92,8 +92,15 @@ func (s *Store) Save(ctx context.Context, tx *oidcclient.Transaction) error { return fmt.Errorf("sqlstore: save transaction: %w", err) } -// Consume implements [oidcclient.TransactionStore] (see [Store]). +// Consume implements [oidcclient.TransactionStore] (see [Store]). It +// refuses, before any I/O, keys [oidcclient.ValidateStoreKey] refuses: a +// MySQL VARCHAR key compares under a case-insensitive, space-padding +// collation, which would otherwise take a malformed key for a stored one. func (s *Store) Consume(ctx context.Context, stateHash, bindingHash string, now time.Time) (*oidcclient.Transaction, error) { + if err := validateKeys(stateHash, bindingHash); err != nil { + return nil, err + } + tx, err := s.db.BeginTx(ctx, s.dialect.txOptions()) if err != nil { return nil, fmt.Errorf("sqlstore: begin: %w", err) @@ -115,6 +122,19 @@ func (s *Store) Consume(ctx context.Context, stateHash, bindingHash string, now return record, err } +// validateKeys refuses the keys oidcclient.ValidateStoreKey refuses. +func validateKeys(stateHash, bindingHash string) error { + if err := oidcclient.ValidateStoreKey(stateHash); err != nil { + return fmt.Errorf("sqlstore: consume transaction: state hash: %w", err) + } + + if err := oidcclient.ValidateStoreKey(bindingHash); err != nil { + return fmt.Errorf("sqlstore: consume transaction: binding hash: %w", err) + } + + return nil +} + // take decides Consume within tx, in the order the contract pins. func (s *Store) take(ctx context.Context, tx *sql.Tx, stateHash, bindingHash string, now time.Time) (*oidcclient.Transaction, error) { if _, err := tx.ExecContext(ctx, s.dialect.rebind(deleteIfExpired), stateHash, now.Unix()); err != nil { @@ -182,9 +202,20 @@ func (s *Store) lockRow(ctx context.Context, tx *sql.Tx, stateHash string) (*oid // PurgeExpired deletes the transactions expired at now — abandoned logins, // which Consume never reaches — and returns how many it deleted. Schedule // it, every few minutes for instance: nothing else deletes them. It is one -// DELETE statement (see the package documentation for its index). +// DELETE statement (see the package documentation for its index), in a +// transaction at READ COMMITTED on PostgreSQL and MySQL. On MySQL, READ +// COMMITTED keeps it from locking the gaps it scans, which would block the +// logins starting meanwhile; a server logging statements +// (binlog_format=STATEMENT) refuses it, and the purge fails, closed. func (s *Store) PurgeExpired(ctx context.Context, now time.Time) (int64, error) { - res, err := s.db.ExecContext(ctx, s.dialect.rebind(deleteExpired), now.Unix()) + tx, err := s.db.BeginTx(ctx, s.dialect.purgeTxOptions()) + if err != nil { + return 0, fmt.Errorf("sqlstore: purge expired transactions: begin: %w", err) + } + + defer func() { _ = tx.Rollback() }() // a no-op once committed + + res, err := tx.ExecContext(ctx, s.dialect.rebind(deleteExpired), now.Unix()) if err != nil { return 0, fmt.Errorf("sqlstore: purge expired transactions: %w", err) } @@ -194,5 +225,9 @@ func (s *Store) PurgeExpired(ctx context.Context, now time.Time) (int64, error) return 0, fmt.Errorf(errAffectedContext, err) } + if err := tx.Commit(); err != nil { + return 0, fmt.Errorf("sqlstore: purge expired transactions: commit: %w", err) + } + return n, nil } diff --git a/oidc/client/store/sql/store_error_test.go b/oidc/client/store/sql/store_error_test.go index 83cb092b..c80d8d3a 100644 --- a/oidc/client/store/sql/store_error_test.go +++ b/oidc/client/store/sql/store_error_test.go @@ -194,6 +194,10 @@ func TestBackendErrors(t *testing.T) { _, err = store.Consume(t.Context(), tx.StateHash, tx.BindingHash, at) requireBackendError(t, err) requireBackendError(t, store.Migrate(t.Context())) + + _, err = store.PurgeExpired(t.Context(), at) + requireBackendError(t, err) + require.ErrorContains(t, err, "sqlstore: purge expired transactions: begin: ") } // TestSaveRefusesInvalidBeforeIO checks Validate runs before any diff --git a/oidc/client/store/sql/store_test.go b/oidc/client/store/sql/store_test.go index f3deaf85..595e27e8 100644 --- a/oidc/client/store/sql/store_test.go +++ b/oidc/client/store/sql/store_test.go @@ -159,16 +159,18 @@ func TestDialectNames(t *testing.T) { // TestSchemaIsPortable pins a DDL every supported engine accepts: // PostgreSQL has no implicit integer/boolean cast and MySQL has no CREATE -// INDEX IF NOT EXISTS, so the schema is the same for every dialect. +// INDEX IF NOT EXISTS, so the schema is the same for every dialect but for +// MySQL's table options, which name InnoDB and utf8mb4 rather than leave +// them to the server's defaults. func TestSchemaIsPortable(t *testing.T) { t.Parallel() want := sqlstore.Schema(sqlstore.SQLite) require.Len(t, want, 1) - - for _, d := range []sqlstore.Dialect{sqlstore.Postgres, sqlstore.MySQL} { - assert.Equal(t, want, sqlstore.Schema(d), d.Name()) - } + assert.Equal(t, want, sqlstore.Schema(sqlstore.Postgres)) + assert.Equal(t, want, sqlstore.Schema(nil)) + assert.NotContains(t, want[0], "ENGINE") + assert.Equal(t, []string{want[0] + " ENGINE=InnoDB DEFAULT CHARSET=utf8mb4"}, sqlstore.Schema(sqlstore.MySQL)) assert.NotContains(t, strings.ToUpper(want[0]), "BOOLEAN") assert.NotContains(t, strings.ToUpper(want[0]), "CREATE INDEX") @@ -242,3 +244,26 @@ func TestPurgeExpired(t *testing.T) { require.NoError(t, err) assert.Zero(t, n) } + +// TestConsumeValidatesKeysBeforeIO: on a closed database, a malformed key +// is refused as invalid — the refusal comes before any I/O — while a +// well-formed one reaches the database and fails there. +func TestConsumeValidatesKeysBeforeIO(t *testing.T) { + t.Parallel() + + store, db := open(t, "file:"+filepath.Join(t.TempDir(), "closed.db"), 1) + require.NoError(t, db.Close()) + + now := time.Now() + good := hash("state") + + _, err := store.Consume(t.Context(), strings.ToUpper(good), hash("binding"), now) + require.ErrorIs(t, err, oidcclient.ErrInvalidTransaction, "malformed state hash") + + _, err = store.Consume(t.Context(), good, "", now) + require.ErrorIs(t, err, oidcclient.ErrInvalidTransaction, "malformed binding hash") + + _, err = store.Consume(t.Context(), good, hash("binding"), now) + require.Error(t, err) + require.NotErrorIs(t, err, oidcclient.ErrInvalidTransaction, "well-formed keys reach the closed database") +} diff --git a/oidc/client/storetest/conformance.go b/oidc/client/storetest/conformance.go index b3a8a2aa..e0b2ba15 100644 --- a/oidc/client/storetest/conformance.go +++ b/oidc/client/storetest/conformance.go @@ -55,6 +55,7 @@ func RunConformance(t *testing.T, newStore Factory) { {"ExpiryDecidedBeforeBinding", testExpiredWrongBinding}, {"SaveNeverOverwrites", testSaveNeverOverwrites}, {"SaveRefusesInvalid", testSaveInvalid}, + {"ConsumeRefusesInvalidKeys", testConsumeInvalidKeys}, {"TransactionsAreIsolated", testIsolation}, {"ConcurrentConsume", testConcurrentConsume}, {"ConcurrentConsumeMixedBindings", testConcurrentMixedBindings}, @@ -287,6 +288,45 @@ func testSaveInvalid(t *testing.T, s oidcclient.TransactionStore) { mustErrorIs(t, s.Save(ctx(), nil), oidcclient.ErrInvalidTransaction, "Save(nil)") } +// malformedKeys returns variants of the store key key that +// Transaction.Validate refuses, by the defect they carry. +func malformedKeys(key string) map[string]string { + return map[string]string{ + "empty": "", + "short": key[:63], + "long": key + "0", + "upper-case": strings.ToUpper(key), + "non-hex": "g" + key[1:], + "trailing space": key[:63] + " ", + "NUL": key[:63] + "\x00", + "multi-byte": key[:62] + "é", + } +} + +// testConsumeInvalidKeys calls Consume with keys Transaction.Validate +// refuses: the store refuses them with ErrInvalidTransaction before any +// I/O — no store answers a key it would never have saved, even one a +// case-insensitive collation would match — and the stored transaction +// stays consumable with its own keys. +func testConsumeInvalidKeys(t *testing.T, s oidcclient.TransactionStore) { + tx := sample("invalid-keys") + mustSave(t, s, tx) + + for name, bad := range malformedKeys(tx.StateHash) { + got, err := s.Consume(ctx(), bad, tx.BindingHash, tx.CreatedAt) + mustErrorIs(t, err, oidcclient.ErrInvalidTransaction, "Consume with a state hash "+name) + mustTrue(t, got == nil, "Consume with a state hash "+name+": no transaction returned") + } + + for name, bad := range malformedKeys(tx.BindingHash) { + got, err := s.Consume(ctx(), tx.StateHash, bad, tx.CreatedAt) + mustErrorIs(t, err, oidcclient.ErrInvalidTransaction, "Consume with a binding hash "+name) + mustTrue(t, got == nil, "Consume with a binding hash "+name+": no transaction returned") + } + + mustConsume(t, s, tx) +} + func testIsolation(t *testing.T, s oidcclient.TransactionStore) { a, b := sample("isolation-a"), sample("isolation-b") mustSave(t, s, a) diff --git a/oidc/client/transaction.go b/oidc/client/transaction.go index e6fe5f9f..d3934a7d 100644 --- a/oidc/client/transaction.go +++ b/oidc/client/transaction.go @@ -152,6 +152,20 @@ func (t *Transaction) validateTimes() error { return nil } +// ValidateStoreKey reports whether key is a store key: a StateHash or a +// BindingHash, 64 lowercase hex digits. Every error matches +// ErrInvalidTransaction. Consume calls it on both its keys before any I/O, +// so that a store never answers a key it would never have saved — not even +// one a case-insensitive or space-padding collation would take for a saved +// one. +func ValidateStoreKey(key string) error { + if !isLowerHex(key, hashHexLength) { + return fmt.Errorf("%w: a store key must be 64 lowercase hex digits", ErrInvalidTransaction) + } + + return nil +} + // TransactionStore persists pending logins. An implementation MUST be safe // for concurrent use and shared by every instance that may serve the // callback. Two operations make the protocol guarantees: @@ -162,7 +176,8 @@ func (t *Transaction) validateTimes() error { // receives it, even under concurrency and across instances. // // Both refuse, before any I/O, what Transaction.Validate refuses, with an -// error matching ErrInvalidTransaction. +// error matching ErrInvalidTransaction: Save the transaction, Consume a +// stateHash or a bindingHash ValidateStoreKey refuses. type TransactionStore interface { // Save persists tx under tx.StateHash. Save(ctx context.Context, tx *Transaction) error diff --git a/oidc/client/transaction_test.go b/oidc/client/transaction_test.go index 2ae61fa0..555b063d 100644 --- a/oidc/client/transaction_test.go +++ b/oidc/client/transaction_test.go @@ -106,3 +106,24 @@ func TestTransactionValidate(t *testing.T) { assert.NoError(t, tx.Validate()) }) } + +// TestValidateStoreKey accepts exactly 64 lowercase hex digits; every +// refusal matches ErrInvalidTransaction. +func TestValidateStoreKey(t *testing.T) { + t.Parallel() + + require.NoError(t, oidcclient.ValidateStoreKey(strings.Repeat("0123456789abcdef", 4))) + + for name, key := range map[string]string{ + "empty": "", + "63 digits": strings.Repeat("a", 63), + "65 digits": strings.Repeat("a", 65), + "upper-case": strings.Repeat("A", 64), + "non-hex": strings.Repeat("g", 64), + "trailing space": strings.Repeat("a", 63) + " ", + "NUL": strings.Repeat("a", 63) + "\x00", + "multi-byte": strings.Repeat("a", 62) + "é", + } { + assert.ErrorIs(t, oidcclient.ValidateStoreKey(key), oidcclient.ErrInvalidTransaction, name) + } +} diff --git a/saml/sp/store/redis/clock_test.go b/saml/sp/store/redis/clock_test.go new file mode 100644 index 00000000..02aaa75e --- /dev/null +++ b/saml/sp/store/redis/clock_test.go @@ -0,0 +1,165 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package redisstore_test + +import ( + "context" + "crypto/rand" + "encoding/hex" + "os" + "testing" + "time" + + samlsp "github.com/hyperscale-stack/security/saml/sp" + redisstore "github.com/hyperscale-stack/security/saml/sp/store/redis" + "github.com/redis/go-redis/v9" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// envRedisAddr names a disposable real Redis server (scripts/test-backends.sh). +const envRedisAddr = "SAMLSP_REDIS_ADDR" + +// TestKeyLifetimesIgnoreTheRedisClock: the store measures how long a key +// must live on the SP's clock and sends that duration, so the offset of +// Redis's clock plays no part. With Redis two minutes ahead, an absolute +// expiry one minute away on the SP's clock is already past for Redis: the +// replay mark would vanish as it is written, and the same assertion be +// accepted again (SAML 2.0 Profiles §4.1.4.5). With Redis behind, the keys +// would outlive their retention. +func TestKeyLifetimesIgnoreTheRedisClock(t *testing.T) { + t.Parallel() + + for _, offset := range []time.Duration{2 * time.Minute, -2 * time.Minute} { + t.Run(offset.String(), func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + store, mr := newStore(t) + mr.SetTime(time.Now().Add(offset)) + + mark := key("skewed mark") + expiry := time.Now().Add(time.Minute).Truncate(time.Second) + + require.NoError(t, store.MarkAssertionUsed(ctx, mark, expiry), "first mark") + require.ErrorIs(t, store.MarkAssertionUsed(ctx, mark, expiry), samlsp.ErrAssertionReplayed, + "the mark is kept until its expiry on the SP's clock") + assert.InDelta(t, time.Minute, mr.TTL("samlsp:as:"+mark), float64(2*time.Second), "the mark's lifetime") + + rec := record("skewed transaction", time.Minute) + require.NoError(t, store.SaveTransaction(ctx, rec)) + + got, err := store.LookupTransaction(ctx, rec.ID) + require.NoError(t, err, "the transaction is kept until its expiry on the SP's clock") + assert.Equal(t, rec.ExpiresAt.Unix(), got.ExpiresAt.Unix(), "the recorded expiry is the SP's") + assert.InDelta(t, time.Minute, mr.TTL("samlsp:tx:"+rec.ID), float64(2*time.Second), "the transaction's lifetime") + }) + } +} + +// TestWithClockMeasuresKeyLifetimes: the lifetimes are measured on the +// clock WithClock names — the SP's samlsp.Config.Now — not on the host's. +func TestWithClockMeasuresKeyLifetimes(t *testing.T) { + t.Parallel() + + ctx := context.Background() + spNow := time.Now().Add(-10 * time.Minute) + store, mr := newStore(t, redisstore.WithClock(func() time.Time { return spNow })) + + mark := key("injected clock") + expiry := spNow.Add(time.Minute).Truncate(time.Second) + + require.NoError(t, store.MarkAssertionUsed(ctx, mark, expiry)) + require.ErrorIs(t, store.MarkAssertionUsed(ctx, mark, expiry), samlsp.ErrAssertionReplayed) + assert.InDelta(t, expiry.Sub(spNow), mr.TTL("samlsp:as:"+mark), float64(time.Second)) + + rec := record("injected clock", 0) + rec.ExpiresAt = spNow.Add(5 * time.Minute).Truncate(time.Second) + require.NoError(t, store.SaveTransaction(ctx, rec)) + assert.InDelta(t, rec.ExpiresAt.Sub(spNow), mr.TTL("samlsp:tx:"+rec.ID), float64(time.Second)) + + // A nil clock keeps the default one. + defaulted, mr2 := newStore(t, redisstore.WithClock(nil)) + require.NoError(t, defaulted.MarkAssertionUsed(ctx, mark, time.Now().Add(time.Minute).Truncate(time.Second))) + assert.InDelta(t, time.Minute, mr2.TTL("samlsp:as:"+mark), float64(2*time.Second)) +} + +// TestPastExpiryKeepsKeysBriefly: an expiry already past on the SP's clock +// still gets a key, for a second — Redis refuses a lifetime that is not +// positive — rather than an error or no key at all. +func TestPastExpiryKeepsKeysBriefly(t *testing.T) { + t.Parallel() + + ctx := context.Background() + store, mr := newStore(t) + past := time.Now().Add(-time.Hour).Truncate(time.Second) + + mark := key("past mark") + require.NoError(t, store.MarkAssertionUsed(ctx, mark, past)) + assert.Equal(t, time.Second, mr.TTL("samlsp:as:"+mark)) + + rec := record("past transaction", 0) + rec.ExpiresAt = past + require.NoError(t, store.SaveTransaction(ctx, rec)) + assert.Equal(t, time.Second, mr.TTL("samlsp:tx:"+rec.ID)) +} + +// TestRealRedisClockSkew runs the clock-skew check on a real server +// (SAMLSP_REDIS_ADDR): the SP's clock, injected, is set two minutes behind +// and then ahead of the Redis TIME, and every key lives the minute the SP +// asked for. +func TestRealRedisClockSkew(t *testing.T) { + addr := os.Getenv(envRedisAddr) + if addr == "" { + t.Skipf("set %s to run against a real Redis (opt-in)", envRedisAddr) + } + + ctx := context.Background() + rdb := redis.NewClient(&redis.Options{Addr: addr}) + t.Cleanup(func() { _ = rdb.Close() }) + + b := make([]byte, 6) + _, _ = rand.Read(b) + prefix := "samlsp-skew-" + hex.EncodeToString(b) + ":" + + t.Cleanup(func() { + keys, _ := rdb.Keys(context.Background(), prefix+"*").Result() + if len(keys) > 0 { + _ = rdb.Del(context.Background(), keys...).Err() + } + }) + + redisNow, err := rdb.Time(ctx).Result() + require.NoError(t, err) + + for _, offset := range []time.Duration{-2 * time.Minute, 2 * time.Minute} { + spNow := redisNow.Add(offset) + store, err := redisstore.New(rdb, redisstore.WithKeyPrefix(prefix+offset.String()+":"), + redisstore.WithClock(func() time.Time { return spNow })) + require.NoError(t, err) + + mark := key("real skew") + expiry := spNow.Add(time.Minute).Truncate(time.Second) + + require.NoError(t, store.MarkAssertionUsed(ctx, mark, expiry), "first mark, SP clock %s", offset) + require.ErrorIs(t, store.MarkAssertionUsed(ctx, mark, expiry), samlsp.ErrAssertionReplayed, + "second mark, SP clock %s", offset) + + ttl, err := rdb.PTTL(ctx, prefix+offset.String()+":as:"+mark).Result() + require.NoError(t, err) + assert.InDelta(t, expiry.Sub(spNow), ttl, float64(2*time.Second), "the mark's lifetime, SP clock %s", offset) + + rec := record("real skew", 0) + rec.ExpiresAt = spNow.Add(time.Minute).Truncate(time.Second) + require.NoError(t, store.SaveTransaction(ctx, rec)) + + _, err = store.LookupTransaction(ctx, rec.ID) + require.NoError(t, err, "the transaction, SP clock %s", offset) + + ttl, err = rdb.PTTL(ctx, prefix+offset.String()+":tx:"+rec.ID).Result() + require.NoError(t, err) + assert.InDelta(t, rec.ExpiresAt.Sub(spNow), ttl, float64(2*time.Second), "the transaction's lifetime, SP clock %s", offset) + } +} diff --git a/saml/sp/store/redis/doc.go b/saml/sp/store/redis/doc.go index 9b03d88a..738ad242 100644 --- a/saml/sp/store/redis/doc.go +++ b/saml/sp/store/redis/doc.go @@ -16,11 +16,17 @@ // - tx:{id}: a transaction, as a hash with the fields b (binding digest), // p (opaque payload), e (expiry, Unix seconds), i (integrity MAC), n // (the nonce of the saving call) and, once consumed, c (the nonce of the -// consuming call). The key expires at the transaction expiry -// (PEXPIREAT: the Redis clock decides). +// consuming call). The key lives until the transaction expiry. // - as:{key}: a consumed assertion, holding the nonce of the recording // call, until its retention expiry. // +// A key's lifetime is sent to Redis as a duration (PEXPIRE, SET … PX): +// the time left until the record's expiry on the SP's clock — the one +// WithClock names, time.Now by default — and at least a second. Redis's +// clock never decides whether a replay mark is still due: with absolute +// instants, a Redis clock ahead of the SP's by more than a mark's +// retention would delete the mark as it is written. +// // Every script touches a single key, so Redis Cluster is supported. Values // are never re-encoded by the scripts (no cjson round trip). // @@ -41,8 +47,13 @@ // - Durability is the server's: a consumption acknowledged by a primary // that fails before persisting or replicating it can be lost in a // failover. Choose the persistence settings accordingly. -// - Clocks of the SP instances and of Redis should be synchronized: the -// expiries are absolute instants. +// - Clocks: the offset between the SP's clock and Redis's plays no part +// (lifetimes are durations), and the SP instances' mutual offset is +// covered by the replay retention margin. Redis's clock must advance +// steadily, and its replicas' clocks match the primary's: Redis turns +// a lifetime into an instant on its own clock and replicates expiries +// as instants, so a clock step or a skewed replica promoted in a +// failover still shortens retention. // - Give the SP an ACL limited to its key prefix: each transaction // carries a MAC under samlsp.Config.TransactionKeys, so Redis is trusted // for availability and single consumption only. A transaction saved diff --git a/saml/sp/store/redis/store.go b/saml/sp/store/redis/store.go index c443b7d4..6ce5b8be 100644 --- a/saml/sp/store/redis/store.go +++ b/saml/sp/store/redis/store.go @@ -22,6 +22,7 @@ import ( type Store struct { rdb redis.UniversalClient prefix string + now func() time.Time } // Option configures a Store. @@ -32,13 +33,26 @@ func WithKeyPrefix(prefix string) Option { return func(s *Store) { s.prefix = prefix } } +// WithClock sets the clock the store measures the lifetime of its keys on: +// a key whose record expires at t lives t − now() on Redis. Give it the +// clock of samlsp.Config.Now when that one is injected, so that a replay +// mark lives as long as the SP considers its assertion valid (SAML 2.0 +// Profiles §4.1.4.5). Default: time.Now; nil keeps the default. +func WithClock(now func() time.Time) Option { + return func(s *Store) { + if now != nil { + s.now = now + } + } +} + // New returns a Store bound to rdb; the caller owns its lifecycle. func New(rdb redis.UniversalClient, opts ...Option) (*Store, error) { if rdb == nil { return nil, errors.New("redisstore: New: nil redis client") } - s := &Store{rdb: rdb, prefix: "samlsp:"} + s := &Store{rdb: rdb, prefix: "samlsp:", now: time.Now} for _, o := range opts { o(s) } @@ -49,6 +63,19 @@ func New(rdb redis.UniversalClient, opts ...Option) (*Store, error) { func (s *Store) txKey(id string) string { return s.prefix + "tx:" + id } func (s *Store) assertionKey(key string) string { return s.prefix + "as:" + key } +// minKeyLifetime is the shortest lifetime the store gives a key: Redis +// refuses one that is not positive, and a record whose expiry has passed +// on the SP's clock is refused by the SP anyway. +const minKeyLifetime = time.Second + +// lifetime is the time-to-live, in milliseconds, of a key whose record +// expires at expiresAt: the time left until then on the store's clock, at +// least minKeyLifetime. Redis receives a duration, never an instant, so the +// offset of its clock from the SP's plays no part. +func (s *Store) lifetime(expiresAt time.Time) int64 { + return max(minKeyLifetime, expiresAt.Sub(s.now())).Milliseconds() +} + // Script replies. const ( replyOK = "ok" @@ -64,14 +91,14 @@ const ( // other call, even with identical content, finds the key taken. // // KEYS[1] tx; ARGV[1] binding; ARGV[2] payload; ARGV[3] expiry (Unix s); -// ARGV[4] expiry (Unix ms); ARGV[5] integrity; ARGV[6] nonce +// ARGV[4] lifetime (ms); ARGV[5] integrity; ARGV[6] nonce var saveScript = redis.NewScript(` if redis.call('EXISTS', KEYS[1]) == 1 then if redis.call('HGET', KEYS[1], 'n') == ARGV[6] then return 'ok' end return 'exists' end redis.call('HSET', KEYS[1], 'b', ARGV[1], 'p', ARGV[2], 'e', ARGV[3], 'i', ARGV[5], 'n', ARGV[6]) -redis.call('PEXPIREAT', KEYS[1], ARGV[4]) +redis.call('PEXPIRE', KEYS[1], ARGV[4]) return 'ok' `) @@ -92,15 +119,12 @@ redis.call('HSET', KEYS[1], 'c', ARGV[2]) return 'ok' `) -// markScript records an assertion unless present; a resent call is -// recognized by its nonce. +// markScript records an assertion unless present, in one SET … NX PX; a +// resent call is recognized by its nonce. // -// KEYS[1] assertion; ARGV[1] nonce; ARGV[2] expiry (Unix ms) +// KEYS[1] assertion; ARGV[1] nonce; ARGV[2] lifetime (ms) var markScript = redis.NewScript(` -if redis.call('SET', KEYS[1], ARGV[1], 'NX') then - redis.call('PEXPIREAT', KEYS[1], ARGV[2]) - return 'ok' -end +if redis.call('SET', KEYS[1], ARGV[1], 'NX', 'PX', ARGV[2]) then return 'ok' end if redis.call('GET', KEYS[1]) == ARGV[1] then return 'ok' end return 'replay' `) @@ -120,7 +144,7 @@ func (s *Store) SaveTransaction(ctx context.Context, rec *samlsp.TransactionReco } reply, err := saveScript.Run(ctx, s.rdb, []string{s.txKey(rec.ID)}, - rec.BindingHash, rec.Payload, rec.ExpiresAt.Unix(), rec.ExpiresAt.UnixMilli(), rec.Integrity, nonce()).Text() + rec.BindingHash, rec.Payload, rec.ExpiresAt.Unix(), s.lifetime(rec.ExpiresAt), rec.Integrity, nonce()).Text() if err != nil { return fmt.Errorf("redisstore: save transaction: %w", err) } @@ -207,7 +231,7 @@ func (s *Store) MarkAssertionUsed(ctx context.Context, key string, expiresAt tim return fmt.Errorf("redisstore: mark assertion: %w", err) } - reply, err := markScript.Run(ctx, s.rdb, []string{s.assertionKey(key)}, nonce(), expiresAt.UnixMilli()).Text() + reply, err := markScript.Run(ctx, s.rdb, []string{s.assertionKey(key)}, nonce(), s.lifetime(expiresAt)).Text() if err != nil { return fmt.Errorf("redisstore: mark assertion: %w", err) } diff --git a/saml/sp/store/sql/deployment.go b/saml/sp/store/sql/deployment.go new file mode 100644 index 00000000..22641e58 --- /dev/null +++ b/saml/sp/store/sql/deployment.go @@ -0,0 +1,93 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "context" + "database/sql" + "errors" + "fmt" + "strings" +) + +// ErrDeploymentRequirement reports a deployment requirement of the package +// documentation that the database does not meet. CheckDeployment and +// Migrate wrap it with the requirement and the table at fault. +var ErrDeploymentRequirement = errors.New("sqlstore: deployment requirement not met") + +// CheckDeployment verifies the deployment requirement of the package +// documentation that the database reports, and returns every table that +// does not meet it — each matching ErrDeploymentRequirement — or the error +// of the database. On MySQL and MariaDB, both tables exist in the +// connection's database and use InnoDB, whose crash safety keeps a +// consumption or a replay mark once acknowledged. PostgreSQL and SQLite +// report nothing it checks. Migrate calls it once the tables exist; when +// you apply Schema with your own migration tool, call it at boot, before +// serving traffic. +func (s *Store) CheckDeployment(ctx context.Context) error { + return s.dialect.checkDeployment(ctx, s.db) +} + +// querier runs the queries of a deployment check: the store's *sql.DB, or +// the *sql.Conn of a Migrate holding its named lock. +type querier interface { + QueryContext(ctx context.Context, query string, args ...any) (*sql.Rows, error) + QueryRowContext(ctx context.Context, query string, args ...any) *sql.Row +} + +// storeTables are the tables of the store, in Schema's order. +var storeTables = []string{"samlsp_transactions", "samlsp_used_assertions"} + +// selectEngines reads the storage engine of the tables of storeTables. +const selectEngines = `SELECT TABLE_NAME, COALESCE(ENGINE, '') FROM information_schema.TABLES + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME IN (?, ?)` + +// checkMySQL reads the storage engines of the store's tables, then checks +// them. +func checkMySQL(ctx context.Context, q querier) error { + rows, err := q.QueryContext(ctx, selectEngines, storeTables[0], storeTables[1]) + if err != nil { + return fmt.Errorf("sqlstore: check deployment: storage engines: %w", err) + } + + defer func() { _ = rows.Close() }() + + engines := map[string]string{} + + for rows.Next() { + var table, engine string + if err := rows.Scan(&table, &engine); err != nil { + return fmt.Errorf("sqlstore: check deployment: storage engines: %w", err) + } + + engines[table] = engine + } + + if err := rows.Err(); err != nil { + return fmt.Errorf("sqlstore: check deployment: storage engines: %w", err) + } + + return checkEngines(engines) +} + +// checkEngines reports each store table that engines — table name → +// storage engine — does not list, or lists with another engine than +// InnoDB. +func checkEngines(engines map[string]string) error { + var errs []error + + for _, table := range storeTables { + engine, ok := engines[table] + + switch { + case !ok: + errs = append(errs, fmt.Errorf("%w: table %s not found in the connection's database", ErrDeploymentRequirement, table)) + case !strings.EqualFold(engine, "InnoDB"): + errs = append(errs, fmt.Errorf("%w: table %s uses the %s engine, not InnoDB", ErrDeploymentRequirement, table, engine)) + } + } + + return errors.Join(errs...) +} diff --git a/saml/sp/store/sql/dialect.go b/saml/sp/store/sql/dialect.go index a21adbe2..8701422a 100644 --- a/saml/sp/store/sql/dialect.go +++ b/saml/sp/store/sql/dialect.go @@ -5,13 +5,16 @@ package sqlstore import ( + "context" + "database/sql" "strconv" "strings" ) // Dialect abstracts the SQL differences the store cares about: the -// parameter placeholders and the serialization of concurrent migrations. -// Every statement is written with "?" and rebound. +// parameter placeholders, the serialization of concurrent migrations, the +// table options and the deployment check. Every statement is written with +// "?" and rebound. type Dialect interface { // Name returns a stable identifier: "postgres", "mysql" or "sqlite". Name() string @@ -22,6 +25,26 @@ type Dialect interface { // "" when the engine serializes the creation of a table itself and // Migrate runs its statements one by one. migrationLock() string + // tableOptions follows the column list of every CREATE TABLE of + // [Schema]: the storage engine and character set the dialect needs, + // "" when it needs none. + tableOptions() string + // namedMigrationLock is the session-level named lock [Store.Migrate] + // takes around the creation of the tables and its deployment check, on + // an engine whose DDL cannot run in a transaction; "" for none. + namedMigrationLock() string + // checkDeployment verifies the requirements of the package + // documentation the database reports (see [Store.CheckDeployment]). + checkDeployment(ctx context.Context, q querier) error + // purgeStatements are the statements [Store.PurgeExpired] runs, one + // per table, each deleting at most a batch of the rows expired before + // a second. + purgeStatements() []string + // purgeArgs are the arguments of a purge statement: the second rows + // must have expired before, and the batch size. + purgeArgs(before int64, limit int) []any + // purgeTxOptions are the options of a purge batch's transaction. + purgeTxOptions() *sql.TxOptions } // migrationLockKey is the PostgreSQL advisory lock [Store.Migrate] takes: @@ -37,7 +60,9 @@ const migrationLockKey = "1266919442082856213" // 1266919442082856213 (pg_advisory_xact_lock) before creating the tables. var Postgres Dialect = postgres{} -// MySQL is the MySQL / MariaDB dialect (? placeholders, InnoDB tables). +// MySQL is the MySQL / MariaDB dialect (? placeholders). [Schema] creates +// its tables InnoDB (ENGINE=InnoDB DEFAULT CHARSET=utf8mb4), and +// [Store.CheckDeployment] verifies they are. var MySQL Dialect = mysql{} // SQLite is the SQLite dialect (? placeholders). @@ -80,15 +105,59 @@ func (postgres) migrationLock() string { return "SELECT pg_advisory_xact_lock(" + migrationLockKey + ")" } +func (postgres) tableOptions() string { return "" } + +func (postgres) purgeStatements() []string { return purgeSubqueries } + +func (postgres) purgeArgs(before int64, limit int) []any { return []any{before, limit, before} } + +// purgeTxOptions pins READ COMMITTED: a serialization failure would abort +// a batch under REPEATABLE READ or SERIALIZABLE. +func (postgres) purgeTxOptions() *sql.TxOptions { return readCommitted } + +// checkDeployment checks nothing PostgreSQL reports. +func (postgres) checkDeployment(context.Context, querier) error { return nil } + +func (postgres) namedMigrationLock() string { return "" } + type mysql struct{} func (mysql) Name() string { return "mysql" } func (mysql) rebind(q string) string { return q } -// migrationLock is empty: MySQL commits every DDL statement on its own, -// and serializes the creation of a table on the metadata lock of its name. +// migrationLock is empty: MySQL commits every DDL statement on its own, so +// Migrate takes a named lock instead (see namedMigrationLock). func (mysql) migrationLock() string { return "" } +// tableOptions names the engine and the character set: a table created +// with the server's default engine would be MyISAM on a server configured +// so, without crash safety. +func (mysql) tableOptions() string { return " ENGINE=InnoDB DEFAULT CHARSET=utf8mb4" } + +func (mysql) checkDeployment(ctx context.Context, q querier) error { return checkMySQL(ctx, q) } + +// namedMigrationLock serializes the Migrate calls of a deployment: MySQL +// commits every DDL statement on its own, so no transaction can hold a +// lock across them, and concurrent CREATE TABLE IF NOT EXISTS of one +// table can fail with a metadata-lock deadlock (1213). +func (mysql) namedMigrationLock() string { return migrationLockName } + +// purgeStatements are single-table DELETEs with LIMIT, which MySQL accepts +// (unlike LIMIT in an IN subquery). +func (mysql) purgeStatements() []string { + return []string{ + `DELETE FROM samlsp_transactions WHERE expires_at < ? LIMIT ?`, + `DELETE FROM samlsp_used_assertions WHERE expires_at < ? LIMIT ?`, + } +} + +func (mysql) purgeArgs(before int64, limit int) []any { return []any{before, limit} } + +// purgeTxOptions pins READ COMMITTED: at REPEATABLE READ a batch locks +// the gaps it scans, blocking the inserts of SaveTransaction and +// MarkAssertionUsed until it commits. +func (mysql) purgeTxOptions() *sql.TxOptions { return readCommitted } + type sqlite struct{} func (sqlite) Name() string { return "sqlite" } @@ -98,3 +167,18 @@ func (sqlite) rebind(q string) string { return q } // of the tables, and a statement prepared before a concurrent change of the // schema is prepared again. func (sqlite) migrationLock() string { return "" } + +func (sqlite) tableOptions() string { return "" } + +func (sqlite) purgeStatements() []string { return purgeSubqueries } + +func (sqlite) purgeArgs(before int64, limit int) []any { return []any{before, limit, before} } + +// purgeTxOptions are the defaults: SQLite's single writer serializes the +// batches with every other write. +func (sqlite) purgeTxOptions() *sql.TxOptions { return nil } + +// checkDeployment checks nothing: SQLite has one engine. +func (sqlite) checkDeployment(context.Context, querier) error { return nil } + +func (sqlite) namedMigrationLock() string { return "" } diff --git a/saml/sp/store/sql/doc.go b/saml/sp/store/sql/doc.go index ae0a792b..fe6d9e94 100644 --- a/saml/sp/store/sql/doc.go +++ b/saml/sp/store/sql/doc.go @@ -32,7 +32,8 @@ // - Every instance uses the same database, reaching the primary: a read // replica could report a consumed transaction or a recorded assertion as // absent. -// - MySQL tables must use InnoDB. +// - MySQL tables must use InnoDB (crash safety): Schema names it, and +// CheckDeployment, which Migrate runs, refuses another engine. // - SQLite: one writer at a time; set a busy timeout (with // modernc.org/sqlite, the _pragma=busy_timeout(5000) DSN option). // - Every column holds ASCII (hex digests, ASCII-only JSON, integers): no @@ -61,18 +62,21 @@ // // # Cleanup // -// The store ships no expiry job. Both tables carry expires_at (Unix -// seconds): delete the rows past it, in small batches by primary key, from -// a scheduled job: +// Both tables carry expires_at (Unix seconds), and rows stay until +// PurgeExpired deletes those expired before the instant it is given — in +// batches of their own transactions, at READ COMMITTED on PostgreSQL and +// MySQL: schedule it, every few minutes for instance. A row deleted after +// its expiry changes nothing: an expired transaction is refused anyway, and +// an assertion is never accepted after its replay entry's expiry. The DDL +// creates no index (MySQL has no CREATE INDEX IF NOT EXISTS): add one on +// expires_at through your migration tool, else each purge batch scans its +// table: // -// DELETE FROM samlsp_transactions WHERE expires_at < ? -// DELETE FROM samlsp_used_assertions WHERE expires_at < ? +// CREATE INDEX samlsp_transactions_expires_at ON samlsp_transactions (expires_at); +// CREATE INDEX samlsp_used_assertions_expires_at ON samlsp_used_assertions (expires_at); // -// A row deleted after its expiry changes nothing: an expired transaction is -// refused anyway, and an assertion is never accepted after its replay -// entry's expiry. The DDL creates no index (MySQL has no CREATE INDEX IF NOT -// EXISTS): add one on expires_at through your migration tool. BeginLogin -// inserts a row per unauthenticated login attempt, so rate-limit it. +// BeginLogin inserts a row per unauthenticated login attempt, so +// rate-limit it. // // Allowed dependencies: // - github.com/hyperscale-stack/security/saml/sp diff --git a/saml/sp/store/sql/migrate_internal_test.go b/saml/sp/store/sql/migrate_internal_test.go index 82d21027..5c826503 100644 --- a/saml/sp/store/sql/migrate_internal_test.go +++ b/saml/sp/store/sql/migrate_internal_test.go @@ -15,6 +15,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" _ "modernc.org/sqlite" ) @@ -57,7 +58,10 @@ func lockingStore(t *testing.T, lock string) (*Store, *sql.DB) { t.Cleanup(func() { _ = db.Close() }) d := NewMockDialect(t) + d.EXPECT().namedMigrationLock().Return("") d.EXPECT().migrationLock().Return(lock) + d.EXPECT().tableOptions().Return("").Maybe() + d.EXPECT().checkDeployment(mock.Anything, db).Return(nil).Maybe() return &Store{db: db, dialect: d}, db } @@ -190,3 +194,58 @@ func TestMigrateReportsAFailedBegin(t *testing.T) { require.ErrorContains(t, store.Migrate(t.Context()), "sqlstore: migrate: begin: ") } + +// TestMigrateEndsWithTheDeploymentCheck: once the tables exist, Migrate +// runs the dialect's deployment check and fails with what it reports. +func TestMigrateEndsWithTheDeploymentCheck(t *testing.T) { + t.Parallel() + + db, err := sql.Open("sqlite", "file:"+filepath.Join(t.TempDir(), "check.db")) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + unmet := checkEngines(map[string]string{}) + + d := NewMockDialect(t) + d.EXPECT().namedMigrationLock().Return("") + d.EXPECT().migrationLock().Return("") + d.EXPECT().tableOptions().Return("") + d.EXPECT().checkDeployment(mock.Anything, db).Return(unmet).Once() + + require.ErrorIs(t, (&Store{db: db, dialect: d}).Migrate(t.Context()), unmet) +} + +func TestCheckEngines(t *testing.T) { + t.Parallel() + + require.NoError(t, checkEngines(map[string]string{"samlsp_transactions": "InnoDB", "samlsp_used_assertions": "innodb"})) + + err := checkEngines(map[string]string{"samlsp_used_assertions": "MyISAM"}) + require.ErrorIs(t, err, ErrDeploymentRequirement) + assert.Equal(t, "sqlstore: deployment requirement not met: table samlsp_transactions not found in the connection's database\n"+ + "sqlstore: deployment requirement not met: table samlsp_used_assertions uses the MyISAM engine, not InnoDB", err.Error()) +} + +// TestCheckDeploymentPerDialect: PostgreSQL and SQLite report nothing the +// store checks; MySQL's check reaches the database, whose failure is +// returned as such, not as a requirement found unmet. +func TestCheckDeploymentPerDialect(t *testing.T) { + t.Parallel() + + db, err := sql.Open("sqlite", ":memory:") + require.NoError(t, err) + require.NoError(t, db.Close()) + + for _, d := range []Dialect{Postgres, SQLite} { + store, err := New(db, d) + require.NoError(t, err) + require.NoError(t, store.CheckDeployment(t.Context()), d.Name()) + } + + store, err := New(db, MySQL) + require.NoError(t, err) + + err = store.CheckDeployment(t.Context()) + require.ErrorContains(t, err, "sqlstore: check deployment: storage engines: ") + require.NotErrorIs(t, err, ErrDeploymentRequirement) +} diff --git a/saml/sp/store/sql/namedlock.go b/saml/sp/store/sql/namedlock.go new file mode 100644 index 00000000..c2042f46 --- /dev/null +++ b/saml/sp/store/sql/namedlock.go @@ -0,0 +1,113 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "context" + "database/sql" + "database/sql/driver" + "fmt" + "math" + "time" +) + +// migrationLockName is the named lock Migrate takes on MySQL and +// MariaDB (GET_LOCK): the module path without its host, then ".Migrate". +// Named locks are server-wide and each store module has its own, so that +// their migrations never wait for one another. +const migrationLockName = "hyperscale-stack/security/saml/sp/store/sql.Migrate" + +// lockWaitForever is the GET_LOCK timeout, in seconds, under a context +// without deadline: a year — the wait lasts as long as the context does. +const lockWaitForever = 365 * 24 * 60 * 60 + +// releaseTimeout bounds the release of a named lock, which runs even once +// the context of Migrate has ended. +const releaseTimeout = 10 * time.Second + +// lockWaitSeconds is the GET_LOCK timeout under ctx: the seconds left until +// its deadline, rounded up and at least one, or lockWaitForever. +func lockWaitSeconds(ctx context.Context) int64 { + deadline, ok := ctx.Deadline() + if !ok { + return lockWaitForever + } + + return max(1, int64(math.Ceil(time.Until(deadline).Seconds()))) +} + +// migrateUnderNamedLock applies Schema and runs the deployment check on +// one connection holding the named lock name, which it releases before the +// connection goes back to the pool: concurrent calls run one after the +// other. +func (s *Store) migrateUnderNamedLock(ctx context.Context, name string) error { + conn, err := s.db.Conn(ctx) + if err != nil { + return fmt.Errorf("sqlstore: migrate: connection: %w", err) + } + + defer func() { _ = conn.Close() }() + + if err := acquireNamedLock(ctx, conn, name); err != nil { + return err + } + + defer releaseNamedLock(ctx, conn, name) + + for _, stmt := range Schema(s.dialect) { + if _, err := conn.ExecContext(ctx, stmt); err != nil { + return fmt.Errorf("sqlstore: migrate: %w", err) + } + } + + return s.dialect.checkDeployment(ctx, conn) +} + +// acquireNamedLock takes the named lock name on conn, waiting as long as +// ctx allows: the server's wait is bounded by its deadline, and the driver +// cancels it when ctx ends. +func acquireNamedLock(ctx context.Context, conn *sql.Conn, name string) error { + var granted sql.NullInt64 + + if err := conn.QueryRowContext(ctx, `SELECT GET_LOCK(?, ?)`, name, lockWaitSeconds(ctx)).Scan(&granted); err != nil { + // The server may have granted the lock before the reply was lost: + // never pool a session that may hold it. + discard(conn) + + return fmt.Errorf("sqlstore: migrate: lock %s: %w", name, err) + } + + switch { + case !granted.Valid: + return fmt.Errorf("sqlstore: migrate: lock %s: not granted", name) + case granted.Int64 != 1: + return fmt.Errorf("sqlstore: migrate: lock %s: %w", name, context.DeadlineExceeded) + default: + return nil + } +} + +// releaseNamedLock releases the named lock name held on conn, even once +// ctx has ended. A release that fails discards the connection: its session +// would otherwise keep the lock from the pool. +func releaseNamedLock(ctx context.Context, conn *sql.Conn, name string) { + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), releaseTimeout) + defer cancel() + + var released sql.NullInt64 + + err := conn.QueryRowContext(ctx, `SELECT RELEASE_LOCK(?)`, name).Scan(&released) + if err == nil && released.Valid && released.Int64 == 1 { + return + } + + discard(conn) +} + +// discard closes conn instead of returning it to the pool when it is +// closed. +func discard(conn *sql.Conn) { + _ = conn.Raw(func(any) error { return driver.ErrBadConn }) +} diff --git a/saml/sp/store/sql/namedlock_internal_test.go b/saml/sp/store/sql/namedlock_internal_test.go new file mode 100644 index 00000000..d9e91b4b --- /dev/null +++ b/saml/sp/store/sql/namedlock_internal_test.go @@ -0,0 +1,222 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "context" + "database/sql" + "database/sql/driver" + "errors" + "fmt" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + moderncsqlite "modernc.org/sqlite" +) + +// fakeLocks stands in for MySQL's GET_LOCK and RELEASE_LOCK on SQLite: the +// lock name picks the server's answer, and every call is recorded. +var fakeLocks = struct { + sync.Mutex + + calls []string +}{} + +// The lock names of the fake and the answer they get. +const ( + lockGranted = "granted" // GET_LOCK 1, RELEASE_LOCK 1 + lockTimedOut = "timed-out" // GET_LOCK 0 + lockNull = "null" // GET_LOCK NULL + lockFails = "fails" // GET_LOCK errors + lockReleaseFails = "release-fails" // GET_LOCK 1, RELEASE_LOCK 0 +) + +func init() { + moderncsqlite.MustRegisterScalarFunction("GET_LOCK", 2, func(_ *moderncsqlite.FunctionContext, args []driver.Value) (driver.Value, error) { + name, _ := args[0].(string) + record(fmt.Sprintf("GET_LOCK(%s, %v)", name, args[1])) + + switch name { + case lockTimedOut: + return int64(0), nil + case lockNull: + return nil, nil + case lockFails: + return nil, errors.New("lock service down") + default: + return int64(1), nil + } + }) + moderncsqlite.MustRegisterScalarFunction("RELEASE_LOCK", 1, func(_ *moderncsqlite.FunctionContext, args []driver.Value) (driver.Value, error) { + name, _ := args[0].(string) + record("RELEASE_LOCK(" + name + ")") + + if name == lockReleaseFails { + return int64(0), nil + } + + return int64(1), nil + }) +} + +func record(call string) { + fakeLocks.Lock() + defer fakeLocks.Unlock() + + fakeLocks.calls = append(fakeLocks.calls, call) +} + +// lockCalls returns the recorded calls naming lock. +func lockCalls(lock string) []string { + fakeLocks.Lock() + defer fakeLocks.Unlock() + + var calls []string + + for _, c := range fakeLocks.calls { + if strings.HasPrefix(c, "GET_LOCK("+lock+", ") || c == "RELEASE_LOCK("+lock+")" { + calls = append(calls, c) + } + } + + return calls +} + +// namedLockStore returns a store over a new SQLite database file whose +// dialect — a mock — names lock as its named migration lock, so that +// Migrate takes the MySQL path: lock, tables, check, release, on one +// connection. check is what the deployment check returns. +func namedLockStore(t *testing.T, lock string, check error) (*Store, *sql.DB) { + t.Helper() + + db, err := sql.Open("sqlite", "file:"+filepath.Join(t.TempDir(), "named.db")) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + d := NewMockDialect(t) + d.EXPECT().namedMigrationLock().Return(lock) + d.EXPECT().tableOptions().Return("").Maybe() + d.EXPECT().checkDeployment(mock.Anything, mock.AnythingOfType("*sql.Conn")).Return(check).Maybe() + + return &Store{db: db, dialect: d}, db +} + +// TestMigrateUnderNamedLock: Migrate takes the named lock, waiting as long +// as its context allows, creates the tables, runs the deployment check on +// the connection holding the lock, and releases it — after a failed check +// too. +func TestMigrateUnderNamedLock(t *testing.T) { + store, db := namedLockStore(t, lockGranted, nil) + + require.NoError(t, store.Migrate(context.Background())) + assert.Equal(t, schemaTables, tables(t, db)) + assert.Equal(t, []string{ + fmt.Sprintf("GET_LOCK(%s, %d)", lockGranted, lockWaitForever), "RELEASE_LOCK(" + lockGranted + ")", + }, lockCalls(lockGranted)) + assert.Equal(t, 1, db.Stats().Idle, "the connection goes back to the pool") + + unmet := fmt.Errorf("%w: table samlsp_transactions uses the MyISAM engine, not InnoDB", ErrDeploymentRequirement) + failing, _ := namedLockStore(t, lockGranted+"-check", unmet) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + require.ErrorIs(t, failing.Migrate(ctx), unmet) + assert.Equal(t, []string{ + "GET_LOCK(" + lockGranted + "-check, 30)", "RELEASE_LOCK(" + lockGranted + "-check)", + }, lockCalls(lockGranted+"-check"), "the wait follows the deadline; a failed check releases the lock") +} + +// TestMigrateNamedLockRefused: a lock the server does not grant — timed +// out, NULL, an error — stops Migrate before any table is created, and +// nothing is released. +func TestMigrateNamedLockRefused(t *testing.T) { + for lock, want := range map[string]string{ + lockTimedOut: "sqlstore: migrate: lock timed-out: context deadline exceeded", + lockNull: "sqlstore: migrate: lock null: not granted", + lockFails: "sqlstore: migrate: lock fails: SQL logic error: lock service down (1)", + } { + store, db := namedLockStore(t, lock, nil) + + err := store.Migrate(context.Background()) + require.EqualError(t, err, want) + assert.Empty(t, tables(t, db), lock) + assert.NotContains(t, lockCalls(lock), "RELEASE_LOCK("+lock+")", lock) + } + + store, _ := namedLockStore(t, lockTimedOut, nil) + require.ErrorIs(t, store.Migrate(context.Background()), context.DeadlineExceeded) +} + +// TestMigrateDiscardsAConnectionWhoseLockIsUnknown: a GET_LOCK that fails +// leaves the lock's state unknown — the server may have granted it before +// the reply was lost — so the connection is discarded, never pooled with +// the server-wide migration lock possibly held. +func TestMigrateDiscardsAConnectionWhoseLockIsUnknown(t *testing.T) { + store, db := namedLockStore(t, lockFails, nil) + + require.Error(t, store.Migrate(context.Background())) + assert.Zero(t, db.Stats().OpenConnections, "the connection is closed, not pooled") + + timedOut, timedOutDB := namedLockStore(t, lockTimedOut, nil) + + require.Error(t, timedOut.Migrate(context.Background())) + assert.Equal(t, 1, timedOutDB.Stats().Idle, "a lock refused for sure keeps its connection") +} + +// TestMigrateNamedLockFailures: no connection, or a failing statement, +// fails Migrate; a failing statement releases the lock first. +func TestMigrateNamedLockFailures(t *testing.T) { + store, db := namedLockStore(t, lockGranted+"-closed", nil) + require.NoError(t, db.Close()) + require.ErrorContains(t, store.Migrate(context.Background()), "sqlstore: migrate: connection: ") + + store, db = namedLockStore(t, lockGranted+"-blocked", nil) + last := schemaTables[len(schemaTables)-1] + execAll(t, db, `CREATE TABLE blocker (x INTEGER)`, `CREATE INDEX `+last+` ON blocker (x)`) + + require.ErrorContains(t, store.Migrate(context.Background()), "already an index named "+last) + assert.Contains(t, lockCalls(lockGranted+"-blocked"), "RELEASE_LOCK("+lockGranted+"-blocked)") +} + +// TestMigrateDiscardsAConnectionKeepingTheLock: a release the server +// refuses discards the connection, whose session would keep the lock. +func TestMigrateDiscardsAConnectionKeepingTheLock(t *testing.T) { + store, db := namedLockStore(t, lockReleaseFails, nil) + + require.NoError(t, store.Migrate(context.Background())) + assert.Contains(t, lockCalls(lockReleaseFails), "RELEASE_LOCK("+lockReleaseFails+")") + assert.Zero(t, db.Stats().OpenConnections, "the connection is closed, not pooled") +} + +func TestLockWaitSeconds(t *testing.T) { + t.Parallel() + + assert.Equal(t, int64(lockWaitForever), lockWaitSeconds(context.Background())) + + ctx, cancel := context.WithTimeout(context.Background(), 1500*time.Millisecond) + defer cancel() + + assert.Equal(t, int64(2), lockWaitSeconds(ctx), "rounded up") + + expired, cancelExpired := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancelExpired() + + assert.Equal(t, int64(1), lockWaitSeconds(expired), "at least one second") +} + +func TestNamedMigrationLock(t *testing.T) { + t.Parallel() + + assert.Equal(t, "hyperscale-stack/security/saml/sp/store/sql.Migrate", MySQL.namedMigrationLock()) + assert.Empty(t, Postgres.namedMigrationLock()) + assert.Empty(t, SQLite.namedMigrationLock()) +} diff --git a/saml/sp/store/sql/purge.go b/saml/sp/store/sql/purge.go new file mode 100644 index 00000000..328c6891 --- /dev/null +++ b/saml/sp/store/sql/purge.go @@ -0,0 +1,93 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "context" + "database/sql" + "fmt" + "time" +) + +// purgeBatch is the most rows one purge statement deletes: a batch keeps +// the rows it deletes locked until it commits, so a backlog goes in short +// transactions. A variable, for the tests. +var purgeBatch = 1000 + +// The purge statements of PostgreSQL and SQLite, one per table, with ? for +// (before, limit, before): the outer DELETE re-checks the expiry of each +// row on its latest version. +var purgeSubqueries = []string{ + `DELETE FROM samlsp_transactions WHERE id IN + (SELECT id FROM samlsp_transactions WHERE expires_at < ? LIMIT ?) AND expires_at < ?`, + `DELETE FROM samlsp_used_assertions WHERE assertion_key IN + (SELECT assertion_key FROM samlsp_used_assertions WHERE expires_at < ? LIMIT ?) AND expires_at < ?`, +} + +// PurgeExpired deletes the transactions and replay marks whose expiry is +// before now, and returns how many it deleted. Nothing else deletes them: +// schedule it, every few minutes for instance. Concurrent purges are safe. +// A row is deleted only after its expiry — a replay mark is kept at least +// until then — which changes nothing: an expired transaction is refused +// anyway, and an assertion is never accepted past its mark's expiry. Pass +// the current time: a later instant would delete marks still due. +// +// Each table is purged in batches of at most 1000 rows, each a transaction +// of its own at READ COMMITTED on PostgreSQL and MySQL. On MySQL, READ +// COMMITTED keeps a batch from locking the gaps it scans, which would block +// the inserts of logins starting and assertions consumed meanwhile; a +// server logging statements (binlog_format=STATEMENT) refuses it, and the +// purge fails, closed. Add an index on expires_at to both tables (see the +// package documentation). A failure returns the rows deleted until then +// and the error. +func (s *Store) PurgeExpired(ctx context.Context, now time.Time) (int64, error) { + var total int64 + + for _, stmt := range s.dialect.purgeStatements() { + for { + n, err := s.purgeOnce(ctx, stmt, now.Unix()) + total += n + + if err != nil { + return total, err + } + + if n < int64(purgeBatch) { + break + } + } + } + + return total, nil +} + +// purgeOnce runs one batch of stmt in a transaction of its own. +func (s *Store) purgeOnce(ctx context.Context, stmt string, before int64) (int64, error) { + tx, err := s.db.BeginTx(ctx, s.dialect.purgeTxOptions()) + if err != nil { + return 0, fmt.Errorf("sqlstore: purge expired rows: begin: %w", err) + } + + defer func() { _ = tx.Rollback() }() // a no-op once committed + + res, err := tx.ExecContext(ctx, s.dialect.rebind(stmt), s.dialect.purgeArgs(before, purgeBatch)...) + if err != nil { + return 0, fmt.Errorf("sqlstore: purge expired rows: %w", err) + } + + n, err := res.RowsAffected() + if err != nil { + return 0, fmt.Errorf("sqlstore: purge expired rows: rows affected: %w", err) + } + + if err := tx.Commit(); err != nil { + return 0, fmt.Errorf("sqlstore: purge expired rows: commit: %w", err) + } + + return n, nil +} + +// readCommitted are the options of a purge batch on PostgreSQL and MySQL. +var readCommitted = &sql.TxOptions{Isolation: sql.LevelReadCommitted} diff --git a/saml/sp/store/sql/purge_internal_test.go b/saml/sp/store/sql/purge_internal_test.go new file mode 100644 index 00000000..334c9b7c --- /dev/null +++ b/saml/sp/store/sql/purge_internal_test.go @@ -0,0 +1,45 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "crypto/sha256" + "database/sql" + "encoding/hex" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestPurgeExpiredInBatches deletes a backlog larger than a batch, batch +// after batch. It changes purgeBatch, so it does not run in parallel. +func TestPurgeExpiredInBatches(t *testing.T) { + defer func(batch int) { purgeBatch = batch }(purgeBatch) + + purgeBatch = 2 + + db, err := sql.Open("sqlite", ":memory:") + require.NoError(t, err) + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + + store, err := New(db, SQLite) + require.NoError(t, err) + require.NoError(t, store.Migrate(t.Context())) + + now := time.Unix(time.Now().Unix(), 0) + + for i := range 5 { + sum := sha256.Sum256([]byte("expired-" + strconv.Itoa(i))) + require.NoError(t, store.MarkAssertionUsed(t.Context(), hex.EncodeToString(sum[:]), now.Add(-time.Hour))) + } + + n, err := store.PurgeExpired(t.Context(), now) + require.NoError(t, err) + assert.Equal(t, int64(5), n) +} diff --git a/saml/sp/store/sql/purge_test.go b/saml/sp/store/sql/purge_test.go new file mode 100644 index 00000000..9a0d6425 --- /dev/null +++ b/saml/sp/store/sql/purge_test.go @@ -0,0 +1,110 @@ +// 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" + "errors" + "testing" + "time" + + samlsp "github.com/hyperscale-stack/security/saml/sp" + sqlstore "github.com/hyperscale-stack/security/saml/sp/store/sql" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// seedSAMLPurge stores, as of now, a transaction and a replay mark expired +// before it, and of each one expiring at now and one live: PurgeExpired +// deletes the first two only. +func seedSAMLPurge(t *testing.T, s *sqlstore.Store, now time.Time) { + t.Helper() + + ctx := t.Context() + + for label, expires := range map[string]time.Time{ + "expired": now.Add(-time.Hour), "boundary": now, "live": now.Add(time.Hour), + } { + require.NoError(t, s.SaveTransaction(ctx, &samlsp.TransactionRecord{ + ID: key("tx/" + label), BindingHash: key("b/" + label), Payload: `{"v":1}`, + ExpiresAt: expires, Integrity: key("i/" + label), + })) + require.NoError(t, s.MarkAssertionUsed(ctx, key("as/"+label), expires)) + } +} + +func samlRows(t *testing.T, db *sql.DB, table string) int { + t.Helper() + + var n int + require.NoError(t, db.QueryRowContext(t.Context(), "SELECT COUNT(*) FROM "+table).Scan(&n)) + + return n +} + +// TestSQLPurgeExpired deletes the transactions and replay marks expired +// before now — never one expiring at now: a mark is kept at least until +// its expiry — and a second purge deletes nothing. +func TestSQLPurgeExpired(t *testing.T) { + t.Parallel() + + store, db := openSQLite(t) + now := time.Unix(time.Now().Unix(), 0) + seedSAMLPurge(t, store, now) + + n, err := store.PurgeExpired(t.Context(), now) + require.NoError(t, err) + assert.Equal(t, int64(2), n) + assert.Equal(t, 2, samlRows(t, db, "samlsp_transactions")) + assert.Equal(t, 2, samlRows(t, db, "samlsp_used_assertions")) + + require.ErrorIs(t, store.MarkAssertionUsed(t.Context(), key("as/boundary"), now), samlsp.ErrAssertionReplayed, + "the mark expiring at now is kept") + + n, err = store.PurgeExpired(t.Context(), now) + require.NoError(t, err) + assert.Zero(t, n) +} + +// TestSQLPurgeExpiredFailures: a failing statement or transaction fails +// the purge. +func TestSQLPurgeExpiredFailures(t *testing.T) { + t.Parallel() + + db, err := sql.Open("sqlite", ":memory:") + require.NoError(t, err) + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + + unmigrated, err := sqlstore.New(db, sqlstore.SQLite) + require.NoError(t, err) + + n, err := unmigrated.PurgeExpired(t.Context(), time.Now()) + require.ErrorContains(t, err, "sqlstore: purge expired rows: ") + assert.Zero(t, n) + + store, _ := openSQLite(t) + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + _, err = store.PurgeExpired(ctx, time.Now()) + require.ErrorIs(t, err, context.Canceled) +} + +// TestSQLPurgeExpiredDriverFailures: a row count the driver cannot give, +// and a transaction it cannot begin at READ COMMITTED, fail the purge. +func TestSQLPurgeExpiredDriverFailures(t *testing.T) { + t.Parallel() + + store := openFaultyDialect(t, &faultyConn{rowsAffectedErr: errors.New("no row count")}, sqlstore.SQLite) + _, err := store.PurgeExpired(t.Context(), time.Now()) + require.ErrorContains(t, err, "sqlstore: purge expired rows: rows affected: no row count") + + store = openFaultyDialect(t, &faultyConn{}, sqlstore.Postgres) + _, err = store.PurgeExpired(t.Context(), time.Now()) + require.ErrorContains(t, err, "sqlstore: purge expired rows: begin: ") +} diff --git a/saml/sp/store/sql/schema.go b/saml/sp/store/sql/schema.go index c7c3ef40..c059e40b 100644 --- a/saml/sp/store/sql/schema.go +++ b/saml/sp/store/sql/schema.go @@ -22,11 +22,19 @@ import ( // // No secret is stored: keys and binding values are SHA-256 digests, and // the MAC's key never reaches the database. Every column holds ASCII. The DDL -// is the same for every Dialect (portable types, no BOOLEAN, no index); -// the argument is accepted for API stability. The statements are -// idempotent (CREATE TABLE IF NOT EXISTS) and valid inside a PostgreSQL -// transaction block, where Migrate runs them. -func Schema(_ Dialect) []string { +// is the same for every Dialect (portable types, no BOOLEAN, no index) but +// for the table options of MySQL: ENGINE=InnoDB DEFAULT CHARSET=utf8mb4, +// since a server whose default engine is MyISAM would otherwise create +// tables without crash safety. A nil Dialect gets the PostgreSQL / SQLite +// DDL. The statements are idempotent (CREATE TABLE IF NOT EXISTS) and valid +// inside a PostgreSQL transaction block, where Migrate runs them. +func Schema(d Dialect) []string { + if d == nil { + d = sqlite{} + } + + options := d.tableOptions() + return []string{ `CREATE TABLE IF NOT EXISTS samlsp_transactions ( id VARCHAR(64) PRIMARY KEY, @@ -35,16 +43,18 @@ func Schema(_ Dialect) []string { consumed SMALLINT NOT NULL DEFAULT 0, expires_at BIGINT NOT NULL, integrity VARCHAR(64) NOT NULL - )`, + )` + options, `CREATE TABLE IF NOT EXISTS samlsp_used_assertions ( assertion_key VARCHAR(64) PRIMARY KEY, expires_at BIGINT NOT NULL - )`, + )` + options, } } // Migrate applies Schema: it creates the missing tables and never alters -// an existing one. +// an existing one. It then runs CheckDeployment, and fails with its error: +// a table that exists already keeps its engine, so a deployment that does +// not meet the requirements is refused at boot. // // It is safe to call repeatedly, and concurrently — every instance of a // deployment starting at once. On PostgreSQL, where concurrent CREATE @@ -57,11 +67,34 @@ func Schema(_ Dialect) []string { // server-side wait when ctx ends. A migration tool applying Schema // concurrently with Migrate should take the same lock. An engine without // pg_advisory_xact_lock cannot use Migrate: apply Schema with your own -// tool. MySQL serializes the creation of a table on the metadata lock of -// its name, and SQLite on its write lock — given the busy timeout the -// package documentation requires, without which a concurrent call fails -// with SQLITE_BUSY: there each statement runs on its own. +// tool. On MySQL and MariaDB, where concurrent CREATE TABLE IF NOT EXISTS +// of one table can fail with a metadata-lock deadlock, Migrate creates the +// tables and runs the check on one connection holding the named lock +// "hyperscale-stack/security/saml/sp/store/sql.Migrate" (GET_LOCK; named +// locks are server-wide), which it releases before returning: concurrent +// calls run one after the other, waiting as long as ctx allows. A holder +// whose client vanished keeps the lock until the server notices +// (wait_timeout): find it with SELECT IS_USED_LOCK('') and KILL it. +// On a Galera or Percona XtraDB cluster named locks are local to a node, so +// Migrate calls through different nodes are not serialized. SQLite +// serializes the creation of a table on its write lock — given the busy +// timeout the package documentation requires, without which a concurrent +// call fails with SQLITE_BUSY: there each statement runs on its own. func (s *Store) Migrate(ctx context.Context) error { + if name := s.dialect.namedMigrationLock(); name != "" { + return s.migrateUnderNamedLock(ctx, name) + } + + if err := s.createTables(ctx); err != nil { + return err + } + + return s.CheckDeployment(ctx) +} + +// createTables applies Schema: under the dialect's migration lock when it +// has one, statement by statement otherwise. +func (s *Store) createTables(ctx context.Context) error { if lock := s.dialect.migrationLock(); lock != "" { return s.migrateLocked(ctx, lock) } diff --git a/saml/sp/store/sql/store_test.go b/saml/sp/store/sql/store_test.go index 43563cc7..a66ad454 100644 --- a/saml/sp/store/sql/store_test.go +++ b/saml/sp/store/sql/store_test.go @@ -82,9 +82,20 @@ func TestDialects(t *testing.T) { func TestSchemaIsPortable(t *testing.T) { t.Parallel() - for _, d := range []sqlstore.Dialect{sqlstore.Postgres, sqlstore.MySQL, sqlstore.SQLite} { + portable := sqlstore.Schema(sqlstore.SQLite) + require.Equal(t, portable, sqlstore.Schema(nil)) + + mysql := sqlstore.Schema(sqlstore.MySQL) + require.Len(t, mysql, len(portable)) + + for i, stmt := range mysql { + assert.Equal(t, portable[i]+" ENGINE=InnoDB DEFAULT CHARSET=utf8mb4", stmt, "MySQL names the engine") + assert.NotContains(t, portable[i], "ENGINE") + } + + for _, d := range []sqlstore.Dialect{sqlstore.Postgres, sqlstore.SQLite} { stmts := sqlstore.Schema(d) - require.Equal(t, sqlstore.Schema(sqlstore.SQLite), stmts, "same DDL for %s", d.Name()) + require.Equal(t, portable, stmts, "same DDL for %s", d.Name()) for _, s := range stmts { upper := strings.ToUpper(s) diff --git a/scripts/test-backends.sh b/scripts/test-backends.sh index 6ed247e5..291c5c0a 100755 --- a/scripts/test-backends.sh +++ b/scripts/test-backends.sh @@ -16,6 +16,8 @@ # and browser-helper tests, opt-in on their # own) # oauth2/store/redis the oauth2 store on Redis +# saml/sp/store/redis the SAML Redis store's key lifetimes under +# clock skew, on Redis # # Every variable below must name a disposable server: the tests create a # namespace of their own (schema, database or key prefix) and drop it @@ -109,6 +111,7 @@ suite internal/interop/sqlstores ./... suite internal/interop/oidcstack . suite internal/interop/saml . '^(TestKeycloakLive|TestCaptureKeycloakFixture|TestE2EHelperProcess)$' suite oauth2/store/redis ./... +suite saml/sp/store/redis ./... if [ -n "$failed" ]; then echo "test-backends: failed:$failed" >&2