diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ceab215..916815a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -946,7 +946,32 @@ legacy packages (`authentication/`, `authorization/`, the in-tree the former `BOOLEAN NOT NULL DEFAULT 0` column and the store's integer comparisons — and the token-family indexes are gone, since MySQL rejects their `CREATE INDEX IF NOT EXISTS`. `Schema` returns the same DDL for - every dialect. + every dialect but for MySQL's key columns (see Security). +- The `Migrate` of the SQL stores (`oauth2/store/sql`, + `oidc/client/store/sql`, `saml/sp/store/sql`) can run concurrently, as + every instance of a deployment starting at once does. On PostgreSQL, + concurrent `CREATE TABLE IF NOT EXISTS` of one table found it missing in + every session, and all but one then failed (`duplicate key value violates + unique constraint "pg_type_typname_nsp_index"`, SQLSTATE 23505). There + `Migrate` now runs in one transaction that first takes a transaction-level + advisory lock (`pg_advisory_xact_lock`), one per store module, documented + in each package: concurrent calls run one after the other, a waiting call + waits as long as its context allows, and a failed call creates nothing. + MySQL, MariaDB and SQLite (with the busy timeout the stores require) + already serialized the creation of a table, and run as before. The opt-in + suite of the new private module `internal/interop/sqlstores` checks the + four engines (PostgreSQL 16, MySQL 8.0 and 8.4, MariaDB 11.4, from its + `compose.yaml`). +- The `oauth2.Storage` contract no longer says that a failed + `SaveTokenPair` or `RotateRefreshToken` persists nothing (I3). When the + acknowledgement of a commit is lost, the call fails although all its + writes may have been applied — never a part of them — and a client + retrying the refresh then presents a consumed token: a reuse, which + revokes the family (`docs/security-considerations.md`, "Lost responses + look like theft"). A test of the SQL store pins it. The documentation now + also says that a consumed refresh token evicted from Redis, or deleted + from SQL before its `expires_at`, loses its reuse detection: presented + again, it reads as an unknown token and revokes nothing. - A failed family revocation after a refresh-token reuse the store detects is no longer swallowed: the SQL and Redis stores returned the bare `ErrRefreshTokenReused` whatever the revocation did. The error now also @@ -1802,3 +1827,19 @@ legacy packages (`authentication/`, `authorization/`, the in-tree for it on a `Manager` of its own (cookie `__Host-signin`), cross-origin posts are refused (`http.CrossOriginProtection`) and the credentials go through `basic.Authenticator`. +- The OAuth2 SQL store (`oauth2/store/sql`) compares its keys byte for byte + on MySQL and MariaDB. Those engines compared the token-family IDs and the + code and token hashes under the collation of their columns, and the + default collations ignore letter case and accents and take canonically + equivalent forms (and `ß` / `ss`) for one character; MariaDB's, like + every PAD SPACE collation (`utf8mb4_bin` included), also ignore trailing + spaces. Two families whose IDs differed only that way were one: revoking + one revoked the other, and a pair issued into one joined the other's + record (the family isolation, I7, of `oauth2.Storage`). The library's own + family IDs and hashes never differ that way; those of custom grants can. + `Schema(sqlstore.MySQL)` declares the six key columns `VARBINARY`; an + existing MySQL database is converted with the statements of MIGRATION.md, + which also record revoked the families such a merge left without a + record, so that none is revived. + `storetest.RunConformance` checks that every store compares its keys byte + for byte (`KeysAreComparedExactly`). diff --git a/CLAUDE.md b/CLAUDE.md index 9928a02c..42f22480 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -138,6 +138,7 @@ persisted; keep it `actionlint`- and `zizmor`-clean): | `./internal/interop/oidcrp` | (private, tests only) | `oidc/client` against zitadel/oidc + opt-in Chrome test | | `./internal/interop/oidcstack` | (private, tests only) | our RP ↔ our OP end to end, attacks included; opt-in real-backend store runs | | `./internal/interop/saml` | (private, tests only) | `saml/sp` against crewjam's IdP, Keycloak (pinned + opt-in live), shared stores, e2e helper | +| `./internal/interop/sqlstores` | (private, tests only) | the three SQL stores on real servers (SQLite always; opt-in PostgreSQL, MySQL, MariaDB): racing `Migrate`, oauth2 conformance, MySQL upgrade statements | `oauth2/storage/memory` is **not** a standalone module — it is a sub-package of `oauth2`; likewise `oidc/client/storage/memory` and `oidc/client/storetest` @@ -403,7 +404,9 @@ cookie included). `UserInfo` fetches claims whose `sub` must match. `samlsp.New(Config{...})` takes a `ConnectionResolver` (each `Connection` has its own SP entity ID, ACS URL and trusted `IdentityProvider`), a -`TransactionStore` and a `ReplayCache`. `BeginLogin` (HTTP-Redirect +`TransactionStore`, a `ReplayCache` and the REQUIRED `TransactionKeys` (1 to +4 server-held keys of at least 32 bytes; every stored transaction carries +their HMAC, verified before any field is used). `BeginLogin` (HTTP-Redirect AuthnRequest + `__Host-samlsp` browser-binding cookie) and `CompleteLogin` (HTTP-POST ACS) are the flow; `Metadata`/`ServeMetadata` publish SP metadata; `ParseIdentityProviderMetadata` imports trusted IdP metadata, and @@ -412,10 +415,12 @@ URL through `outbound` (pinned entity ID, bounded refresh, never triggered by response content). - Validation is layered: streaming lexical gate (`xmlgate.go`) → structural - profile on the raw etree (`precheck*.go`) → gosaml2/goxmldsig verification - (`verify.go`) → processing rules on the verified decoded values - (`postcheck.go`) → atomic consumption → replay mark. Never read identity - data from the raw tree; never write XML-DSig/C14N code. + profile on the raw etree (`precheck*.go`) → transaction lookup: integrity + MAC (`integrity.go`), browser binding, expiry → status and Response + `InResponseTo` → atomic consumption (one verification per login) → + gosaml2/goxmldsig verification (`verify.go`) → processing rules on the + verified decoded values (`postcheck.go`) → replay mark. Never read + identity data from the raw tree; never write XML-DSig/C14N code. - Strict by default: SHA-1 only via `Connection.InsecureAllowSHA1`; ambiguous shapes and encrypted assertions refused with typed errors. Every refusal matches `ErrRejected` (wraps @@ -423,12 +428,15 @@ by response content). - `Storage` implementations: `saml/sp/storage/memory` (dev/tests), `saml/sp/store/sql`, `saml/sp/store/redis`; all must pass `saml/sp/storetest`. -- `saml/sp/testdata/corpus` pins 359 signed adversarial responses, stored - uncompressed (review them as text diffs); after a +- `saml/sp/testdata/corpus` pins 390 entries over 360 signed adversarial + responses, stored uncompressed (review them as text diffs); after a deliberate change regenerate it with `go test -run '^TestCorpusGenerate$' -count=1 . -args -corpus.update` (in `saml/sp`): regeneration is - byte-identical, and `TestCorpusCoversEveryRule` requires every refusal - rule of the sources in some entry (or in its documented allowlist). + byte-identical only with the toolchain recorded in the manifest + (`generatedWith`); with another Go version every file changes (new RSA + keys), so regenerate in a commit of its own. `TestCorpusCoversEveryRule` + requires every refusal rule of the sources in some entry (or in its + documented allowlist). - `internal/interop/saml` runs the SP against crewjam's IdP, replays the pinned Keycloak responses (`testdata/keycloak-*`), races instances on shared stores and tests metadata retrieval. Opt-in, with diff --git a/LIMITATIONS.md b/LIMITATIONS.md index 85147723..55f36978 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -53,15 +53,19 @@ future refactor phases. anything: consumed and expired codes, expired tokens and revoked families stay for the process lifetime. It is meant for development and tests only. -- **PostgreSQL and MySQL are not exercised in CI** — the SQL store's tests - run on SQLite only; its PostgreSQL and MySQL guarantees (row locks, +- **PostgreSQL and MySQL are not exercised in CI** — CI runs the SQL + store on SQLite only; its PostgreSQL and MySQL guarantees (row locks, READ COMMITTED on PostgreSQL, InnoDB on MySQL) are argued from their - locking semantics, not tested. + locking semantics. The opt-in suite of `internal/interop/sqlstores` + (PostgreSQL 16, MySQL 8.0 and 8.4, MariaDB 11.4, from its `compose.yaml`) + runs the conformance suite, `Migrate` concurrently and the MySQL upgrade + statements on them, locally only. - **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`) are added - with the statements MIGRATION.md lists, through your migration tool. + `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. - **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 diff --git a/MIGRATION.md b/MIGRATION.md index 3c13a207..464ff440 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -34,6 +34,7 @@ released on its own cadence. | `./internal/interop/oidcrp` | `github.com/hyperscale-stack/security/internal/interop/oidcrp` | OIDC client interoperability tests against zitadel/oidc (private, tests only) | | `./internal/interop/oidcstack` | `github.com/hyperscale-stack/security/internal/interop/oidcstack` | Our RP against our OP end to end, attacks included; opt-in real-backend runs of the client stores (private, tests only) | | `./internal/interop/saml` | `github.com/hyperscale-stack/security/internal/interop/saml` | SAML interoperability tests: crewjam/saml IdP, Keycloak, shared stores, e2e helper (private, tests only) | +| `./internal/interop/sqlstores` | `github.com/hyperscale-stack/security/internal/interop/sqlstores` | The three SQL stores on SQLite and, opt-in, real PostgreSQL / MySQL / MariaDB: concurrent `Migrate`, the oauth2 conformance suite, the MySQL upgrade statements (private, tests only) | `oauth2/storage/memory` is a sub-package of the `oauth2` module (not a standalone module): it ships the in-memory `oauth2.Storage` used for dev @@ -102,6 +103,10 @@ internal/interop/saml/ ← (tests only) saml/sp + saml/sp/store/{sql,redis} + github.com/go-sql-driver/mysql + github.com/redis/go-redis/v9 + github.com/alicebob/miniredis/v2 +internal/interop/sqlstores/ ← (tests only) oauth2/store/sql, oidc/client/store/sql, + saml/sp/store/sql + oauth2 + oidc/client + saml/sp + + modernc.org/sqlite + opt-in drivers + github.com/jackc/pgx/v5, github.com/go-sql-driver/mysql (`oauth2/storage/memory` is a sub-package of the `oauth2` module; `oidc/client/storage/memory` and `oidc/client/storetest` are sub-packages @@ -2249,3 +2254,124 @@ To migrate: relay the token must read the request's `Authorization` field. A custom `TokenVerifier` should build its result with `bearer.New(token).WithAuthenticated(...)`, which drops the token. + +### SQL stores: `Migrate` takes an advisory lock on PostgreSQL + +Nothing to migrate. On PostgreSQL, the `Migrate` of `oauth2/store/sql`, +`oidc/client/store/sql` and `saml/sp/store/sql` now runs in one transaction +that first takes a transaction-level advisory lock, so that every instance +of a deployment can call it at boot: concurrent `CREATE TABLE IF NOT EXISTS` +of one table used to fail (SQLSTATE 23505). A migration tool that applies +`Schema` while instances may run `Migrate` should take the same lock, named +in each package's documentation (`pg_advisory_xact_lock(-2450120788798466590)` +for `oauth2/store/sql`, `(-3346565968272468758)` for `oidc/client/store/sql`, +`(1266919442082856213)` for `saml/sp/store/sql`). An engine without +`pg_advisory_xact_lock` cannot use `Migrate`: apply `Schema` with your own +tool. + +### `oauth2/store/sql`: MySQL compares keys byte for byte + +On MySQL and MariaDB the SQL store compared its keys — token-family IDs, +code and token hashes — under the collation of their columns. The default +ones ignore letter case and accents and take canonically equivalent Unicode +forms (and `ß` / `ss`) for one character: `utf8mb4_0900_ai_ci` on MySQL 8, +`utf8mb4_uca1400_ai_ci` on MariaDB 11. Those of MariaDB, like every PAD +SPACE collation (`utf8mb4_bin` included), also ignore trailing spaces. Two +families whose IDs differed only that way were one family: revoking one +revoked the other, and a pair issued into one joined the other's record — +against the isolation the `oauth2.Storage` contract promises (I7). The +library's own family IDs (random or derived, base64url) and its hashes +(lowercase hex) never differ that way; the family IDs of custom grants can. +`Schema(sqlstore.MySQL)` now declares the six key columns `VARBINARY`, which +MySQL and MariaDB compare byte for byte; PostgreSQL and SQLite already did. + +| | Before | After | +| -------------------------------------------------------------- | -------------------------------------------- | ------------------------------------------------ | +| MySQL key columns (`code_hash`, `token_hash` ×2, `family_id` ×3) | `VARCHAR(n)`, compared under the collation | `VARBINARY(n)`, compared byte for byte | +| PostgreSQL and SQLite tables | `VARCHAR(n)`, compared byte for byte | unchanged | +| `Migrate` on an existing MySQL database | creates the missing tables | unchanged: it never alters a table (steps below) | +| A token merged into a look-alike family's record | shares that family's state | reads revoked once upgraded | +| `storetest.RunConformance` | — | checks that a store compares keys byte for byte | + +To migrate (MySQL and MariaDB only; PostgreSQL and SQLite need nothing): + +1. **Preflight**, on the tables as they are, before any `ALTER`: + + ```sql + -- Tokens merged into the record of a look-alike family: + -- access tokens, refresh tokens (expected: 0, 0). + SELECT + (SELECT COUNT(*) FROM oauth2_access_tokens t + JOIN oauth2_token_families f ON f.family_id = t.family_id + WHERE CAST(t.family_id AS BINARY) <> CAST(f.family_id AS BINARY)), + (SELECT COUNT(*) FROM oauth2_refresh_tokens t + JOIN oauth2_token_families f ON f.family_id = t.family_id + WHERE CAST(t.family_id AS BINARY) <> CAST(f.family_id AS BINARY)); + -- The longest keys, in bytes: family IDs at most 64, hashes at most 128. + SELECT + (SELECT COALESCE(MAX(LENGTH(family_id)), 0) FROM oauth2_token_families), + (SELECT COALESCE(MAX(LENGTH(family_id)), 0) FROM oauth2_access_tokens), + (SELECT COALESCE(MAX(LENGTH(family_id)), 0) FROM oauth2_refresh_tokens), + (SELECT COALESCE(MAX(LENGTH(code_hash)), 0) FROM oauth2_auth_codes), + (SELECT COALESCE(MAX(LENGTH(token_hash)), 0) FROM oauth2_access_tokens), + (SELECT COALESCE(MAX(LENGTH(token_hash)), 0) FROM oauth2_refresh_tokens); + ``` + + The counts are 0 unless custom family IDs differ only by case, accents, + Unicode form or trailing spaces. When they are not, stop token writes — + issuance, rotation, revocation — during step 2: between its `ALTER` of + `oauth2_token_families` and its `INSERT`, an issuance into such a family + would record it active and revive the tokens it shares with a revoked + family. A family ID over 64 bytes or a hash over 128 makes step 2 fail + (it runs in strict mode): delete those rows first. The store refuses + such family IDs since #82, and its own hashes are 64 characters. +2. **Convert the key columns and record the orphaned families revoked**, in + one session — with token writes stopped when step 1 counted merged + tokens: + + ```sql + SET SESSION sql_mode = CONCAT_WS(',', NULLIF(@@SESSION.sql_mode, ''), 'STRICT_ALL_TABLES'); + ALTER TABLE oauth2_access_tokens MODIFY token_hash VARBINARY(128) NOT NULL, MODIFY family_id VARBINARY(64) NOT NULL; + ALTER TABLE oauth2_refresh_tokens MODIFY token_hash VARBINARY(128) NOT NULL, MODIFY family_id VARBINARY(64) NOT NULL; + ALTER TABLE oauth2_token_families MODIFY family_id VARBINARY(64) NOT NULL; + INSERT INTO oauth2_token_families (family_id, revoked_at, expires_at) + SELECT o.family_id, UNIX_TIMESTAMP(), GREATEST(MAX(o.expires_at), UNIX_TIMESTAMP() + 86400) + FROM (SELECT family_id, expires_at FROM oauth2_access_tokens WHERE family_id <> '' + UNION ALL + SELECT family_id, expires_at FROM oauth2_refresh_tokens WHERE family_id <> '') o + WHERE NOT EXISTS (SELECT 1 FROM oauth2_token_families f WHERE f.family_id = o.family_id) + GROUP BY o.family_id; + ``` + + The `INSERT` runs after the `ALTER`s — before them, the look-alike would + collide with its family's key. It records revoked every family that + tokens name but that has no record of its own once keys compare byte for + byte: the look-alikes the collation merged, and families whose record + was lost (tokens issued before #82, a premature cleanup). Their tokens + already read revoked (I8); the record keeps a later issuance from + recording them active, for `oauth2.FamilyRevocationRetention` (86400 s) + at least. A look-alike that shared an *active* family's record therefore + reads revoked: its users sign in again. +3. **Optionally, convert the codes' table.** The library's code hashes are + lowercase hex, which no collation confuses, so this only matters for + hashes of your own. It rebuilds a table `/authorize` writes to: run it at + a quiet time (`Migrate` creates it binary on a new database). + + ```sql + ALTER TABLE oauth2_auth_codes MODIFY code_hash VARBINARY(128) NOT NULL; + ``` + +4. **Plan the window.** Each `ALTER` rebuilds its table. MySQL 8.0 and 8.4 + copy it and block its writes meanwhile (`ALGORITHM=COPY`, `LOCK=SHARED`). + MariaDB 11.2 and later can rebuild it with `LOCK=NONE`. While + `oauth2_token_families` is rebuilt, every issuance, rotation and + revocation waits. Run it at a quiet time, or use an online schema-change + tool with the same column definitions. The store works before, during + and after the conversion, and comparisons become exact table by table: + once a token table is converted, its tokens merged into a look-alike's + record already read revoked. +5. **Read the keys** with `CAST(family_id AS CHAR)`: the interactive + `mysql` client shows binary columns in hexadecimal (`--binary-as-hex`, + on by default in interactive mode since 8.0.19). +6. **Custom stores** compare family IDs and hashes byte for byte, and pass + `storetest.RunConformance`, which now checks it (`KeysAreComparedExactly`). diff --git a/docs/architecture.md b/docs/architecture.md index 31a5f257..11464322 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -48,6 +48,7 @@ transitive dependencies. | `./internal/interop/oidcrp` | (private) | OIDC client interoperability tests (zitadel/oidc, headless Chrome) | | `./internal/interop/oidcstack` | (private) | Our RP against our OP, attacks included; opt-in real PG/MySQL/Redis store runs | | `./internal/interop/saml` | (private) | SAML interoperability tests (crewjam/saml IdP, Keycloak, shared stores, e2e helper) | +| `./internal/interop/sqlstores` | (private) | The three SQL stores on SQLite and, opt-in, real PG/MySQL/MariaDB: concurrent `Migrate`, oauth2 conformance, MySQL upgrade | `oauth2/storage/memory` is a sub-package of the `oauth2` module (not a separate module) — it ships an in-memory `oauth2.Storage` for dev and tests. @@ -93,6 +94,8 @@ internal/interop/oidcstack/ ← (tests only) oidc/client + its stores internal/interop/saml/ ← (tests only) saml/sp + its stores + crewjam/saml + etree + modernc sqlite + pgx + go-sql-driver/mysql + go-redis + miniredis +internal/interop/sqlstores/ ← (tests only) the three SQL stores + oauth2 + oidc/client + + saml/sp + modernc sqlite + opt-in pgx, go-sql-driver/mysql ``` The core MUST NOT depend on gRPC, ConnectRPC, JOSE libraries, OAuth2, diff --git a/docs/releasing.md b/docs/releasing.md index 57e1c687..a1a78f31 100644 --- a/docs/releasing.md +++ b/docs/releasing.md @@ -65,7 +65,9 @@ requirements are the `github.com/hyperscale-stack/security…` lines of its git push origin oauth2/store/sql/v1.0.0 ``` - `make release-check` refuses: + `make release-check` runs `scripts/release-check.sh`, which needs `git`, + `go` and `jq` on the `PATH` (GitHub runners have them; without `jq` the + script stops with "jq is required"). It refuses: - a malformed or non-v1 tag; - a module that is never released; - a placeholder requirement (`v0.0.0-00010101000000-000000000000`); diff --git a/docs/security-considerations.md b/docs/security-considerations.md index 2cf2d011..dfb47f4c 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -651,9 +651,14 @@ never has to find the family's tokens: tokens exists; the revocation is kept for at least `oauth2.FamilyRevocationRetention` (24 hours), and never for less than the lifetime of the family's tokens, so a late issuance is still refused. -- **Atomicity.** A failed issuance or rotation persists no token and - consumes nothing: a failed exchange no longer leaves an access token - behind. +- **Atomicity.** An issuance or rotation applies all its writes or none, + never a part of them. One the store refuses — a revoked family, a reuse, + a malformed pair — persists no token and consumes nothing: a failed + exchange no longer leaves an access token behind. A backend failure can + leave the outcome unknown: when the acknowledgement of a commit is lost, + the call fails although all its writes may have been applied, and a + client retrying the refresh then presents a consumed token — a reuse + (see "Lost responses look like theft" below). - **Idempotence and isolation.** Revoking a revoked family changes nothing and succeeds; a revoked family never becomes active again. Revoking one family affects no other family and no token outside a family, and the @@ -676,7 +681,11 @@ never has to find the family's tokens: predating token families — is treated as revoked. This protects lookups only: the next issuance into a family whose revoked state was lost records it active again and revives its tokens, so a revoked family's state must - never be lost within its retention — no eviction, no early cleanup. + never be lost within its retention — no eviction, no early cleanup. A + consumed refresh token must not be lost either: one evicted from Redis, + or deleted from SQL before its `expires_at`, reads as an unknown token + when presented again — `invalid_grant`, revoking nothing — so a stolen + token's reuse goes undetected. - **Lost responses look like theft.** When the response to a successful rotation is lost, the client retries with the presented token, which is now a reuse: the family is revoked and the user re-authenticates. This is @@ -695,7 +704,8 @@ family row or key is deleted before it expires, and durability is the backend's: a revocation acknowledged by a primary that fails before persisting or replicating it can be lost in a failover. Only SQLite is exercised in CI; the PostgreSQL and MySQL guarantees rest on their locking -semantics. +semantics, which the opt-in suite of `internal/interop/sqlstores` checks on +PostgreSQL, MySQL and MariaDB servers. ### Offline JWT access tokens diff --git a/go.work b/go.work index d4db1718..3b0ed9d9 100644 --- a/go.work +++ b/go.work @@ -13,6 +13,7 @@ use ( ./internal/interop/oidcrp ./internal/interop/oidcstack ./internal/interop/saml + ./internal/interop/sqlstores ./jwt ./oauth2 ./oauth2/store/redis diff --git a/internal/interop/sqlstores/backends_test.go b/internal/interop/sqlstores/backends_test.go new file mode 100644 index 00000000..30294e53 --- /dev/null +++ b/internal/interop/sqlstores/backends_test.go @@ -0,0 +1,254 @@ +// 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" + "crypto/rand" + "database/sql" + "encoding/hex" + "net/url" + "os" + "path/filepath" + "testing" + "time" + + mysqldriver "github.com/go-sql-driver/mysql" + oauth2sql "github.com/hyperscale-stack/security/oauth2/store/sql" + oidcsql "github.com/hyperscale-stack/security/oidc/client/store/sql" + samlsql "github.com/hyperscale-stack/security/saml/sp/store/sql" + _ "github.com/jackc/pgx/v5/stdlib" // registers the "pgx" database/sql driver + "github.com/stretchr/testify/require" + _ "modernc.org/sqlite" // registers the "sqlite" database/sql driver +) + +// The real backends are opt-in — routine CI never sets these variables. +// Start them with compose.yaml (see its header), then: +// +// SQLSTORES_POSTGRES_DSN=postgres://sqlstores:$SQLSTORES_DB_PASSWORD@127.0.0.1:35432/sqlstores?sslmode=disable +// SQLSTORES_MYSQL80_DSN=root:$SQLSTORES_DB_PASSWORD@tcp(127.0.0.1:33306)/ +// SQLSTORES_MYSQL84_DSN=root:$SQLSTORES_DB_PASSWORD@tcp(127.0.0.1:33307)/ +// SQLSTORES_MARIADB_DSN=root:$SQLSTORES_DB_PASSWORD@tcp(127.0.0.1:33308)/ +// +// Every test works in a namespace it creates — a PostgreSQL schema, a +// MySQL database, a SQLite file — and drops the server ones afterwards: +// point the variables at disposable servers all the same. The real-backend +// tests run one after the other, to bound the connections. +const ( + envPostgresDSN = "SQLSTORES_POSTGRES_DSN" + envMySQL80DSN = "SQLSTORES_MYSQL80_DSN" + envMySQL84DSN = "SQLSTORES_MYSQL84_DSN" + envMariaDBDSN = "SQLSTORES_MARIADB_DSN" +) + +// engine is the SQL dialect a backend speaks. +type engine int + +const ( + engineSQLite engine = iota + enginePostgres + engineMySQL +) + +// backend is a database server the stores run on. +type backend struct { + name string + engine engine + // env names the variable holding the server's DSN; SQLite needs none. + env string + // params are the run-time parameters (PostgreSQL) or the system + // variables (MySQL) every namespace DSN sets. + params map[string]string + // clientFoundRows sets the MySQL CLIENT_FOUND_ROWS flag, under which + // an UPDATE reports the rows it matched rather than those it changed. + clientFoundRows bool +} + +var ( + sqliteFile = backend{name: "sqlite", engine: engineSQLite} + postgres = backend{name: "postgres", engine: enginePostgres, env: envPostgresDSN} + // postgresSerializable makes SERIALIZABLE the default isolation of + // every transaction that does not choose its own. + postgresSerializable = backend{ + name: "postgres-serializable", engine: enginePostgres, env: envPostgresDSN, + params: map[string]string{"default_transaction_isolation": "serializable"}, + } + mysql80 = backend{name: "mysql80", engine: engineMySQL, env: envMySQL80DSN} + mysql84 = backend{name: "mysql84", engine: engineMySQL, env: envMySQL84DSN} + mariadb = backend{name: "mariadb", engine: engineMySQL, env: envMariaDBDSN} +) + +// server returns the DSN of b's server, and skips t when b is an opt-in +// backend whose variable is not set. +func (b backend) server(t *testing.T) string { + t.Helper() + + if b.engine == engineSQLite { + return "" + } + + dsn := os.Getenv(b.env) + if dsn == "" { + t.Skipf("set %s to run against a real %s (opt-in)", b.env, b.name) + } + + return dsn +} + +// namespace creates an empty namespace on b and returns the driver and the +// DSN of connections into it. +func (b backend) namespace(t *testing.T) (driver, dsn string) { + t.Helper() + + switch b.engine { + case enginePostgres: + return "pgx", b.postgresNamespace(t) + case engineMySQL: + return "mysql", b.mysqlNamespace(t) + default: + return "sqlite", sqliteNamespace(t) + } +} + +// randomName returns prefix followed by 10 random hex digits. +func randomName(prefix string) string { + b := make([]byte, 5) + _, _ = rand.Read(b) + + return prefix + hex.EncodeToString(b) +} + +// admin runs one statement on the server dsn designates, on a connection +// of its own. +func admin(driver, dsn, stmt string) error { + db, err := sql.Open(driver, dsn) + if err != nil { + return err + } + + defer func() { _ = db.Close() }() + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + _, err = db.ExecContext(ctx, stmt) + + return err +} + +// postgresNamespace works in a new schema, selected through the +// search_path run-time parameter of a URL DSN. +func (b backend) postgresNamespace(t *testing.T) string { + t.Helper() + + server := b.server(t) + schema := randomName("sqlstores_") + + require.NoError(t, admin("pgx", server, "CREATE SCHEMA "+schema)) + t.Cleanup(func() { _ = admin("pgx", server, "DROP SCHEMA "+schema+" CASCADE") }) + + u, err := url.Parse(server) + require.NoError(t, err, "%s must be a URL (postgres://…)", b.env) + + q := u.Query() + q.Set("search_path", schema) + + for name, value := range b.params { + q.Set(name, value) + } + + u.RawQuery = q.Encode() + + return u.String() +} + +// mysqlNamespace works in a new database. +func (b backend) mysqlNamespace(t *testing.T) string { + t.Helper() + + server := b.server(t) + database := randomName("sqlstores_") + + require.NoError(t, admin("mysql", server, "CREATE DATABASE "+database)) + t.Cleanup(func() { _ = admin("mysql", server, "DROP DATABASE "+database) }) + + cfg, err := mysqldriver.ParseDSN(server) + require.NoError(t, err) + + cfg.DBName = database + cfg.ClientFoundRows = b.clientFoundRows + + if len(b.params) > 0 { + cfg.Params = b.params + } + + return cfg.FormatDSN() +} + +// sqliteNamespace works in a new database file in WAL mode, with the busy +// timeout the stores require: concurrent writers wait for the write lock +// instead of failing with SQLITE_BUSY. The file is created, and switched +// to WAL, before any test connection opens it. +func sqliteNamespace(t *testing.T) string { + t.Helper() + + dsn := "file:" + filepath.Join(t.TempDir(), "stores.db") + "?_pragma=busy_timeout(10000)&_pragma=journal_mode(WAL)" + open(t, "sqlite", dsn, 1) + + return dsn +} + +// open returns a pool of at most maxConns connections on dsn, closed when +// the test ends, after checking that it reaches the server. +func open(t *testing.T, driver, dsn string, maxConns int) *sql.DB { + t.Helper() + + db, err := sql.Open(driver, dsn) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + db.SetMaxOpenConns(maxConns) + db.SetMaxIdleConns(maxConns) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + require.NoError(t, db.PingContext(ctx), "reaching %s", driver) + + return db +} + +func oauth2Dialect(e engine) oauth2sql.Dialect { + switch e { + case enginePostgres: + return oauth2sql.Postgres + case engineMySQL: + return oauth2sql.MySQL + default: + return oauth2sql.SQLite + } +} + +func oidcDialect(e engine) oidcsql.Dialect { + switch e { + case enginePostgres: + return oidcsql.Postgres + case engineMySQL: + return oidcsql.MySQL + default: + return oidcsql.SQLite + } +} + +func samlDialect(e engine) samlsql.Dialect { + switch e { + case enginePostgres: + return samlsql.Postgres + case engineMySQL: + return samlsql.MySQL + default: + return samlsql.SQLite + } +} diff --git a/internal/interop/sqlstores/compose.yaml b/internal/interop/sqlstores/compose.yaml new file mode 100644 index 00000000..9b3d5182 --- /dev/null +++ b/internal/interop/sqlstores/compose.yaml @@ -0,0 +1,69 @@ +# Disposable databases for the opt-in real-backend tests of the SQL stores +# (oauth2/store/sql, oidc/client/store/sql, saml/sp/store/sql); routine CI +# runs the SQLite cases only. Images are pinned by digest — postgres:16 and +# mysql:8.0 are the ones the other interop projects use — and every port is +# bound to 127.0.0.1 on a non-default port, distinct from those projects', +# so all of them can run at once. Nothing is persisted. +# +# export SQLSTORES_DB_PASSWORD="$(openssl rand -hex 16)" +# docker compose -f internal/interop/sqlstores/compose.yaml up -d --wait +# cd internal/interop/sqlstores +# SQLSTORES_POSTGRES_DSN="postgres://sqlstores:$SQLSTORES_DB_PASSWORD@127.0.0.1:35432/sqlstores?sslmode=disable" \ +# SQLSTORES_MYSQL80_DSN="root:$SQLSTORES_DB_PASSWORD@tcp(127.0.0.1:33306)/" \ +# SQLSTORES_MYSQL84_DSN="root:$SQLSTORES_DB_PASSWORD@tcp(127.0.0.1:33307)/" \ +# SQLSTORES_MARIADB_DSN="root:$SQLSTORES_DB_PASSWORD@tcp(127.0.0.1:33308)/" \ +# go test -race -count=1 ./... +# docker compose -f internal/interop/sqlstores/compose.yaml down + +name: sqlstores-interop + +services: + postgres: + image: postgres:16@sha256:ca0bd484cb98bf4b24eb1010e73fb3fcbd6714d240fbc1a10eea5b7dbecb641d + environment: + POSTGRES_USER: sqlstores + POSTGRES_PASSWORD: ${SQLSTORES_DB_PASSWORD:?set SQLSTORES_DB_PASSWORD} + POSTGRES_DB: sqlstores + ports: + - "127.0.0.1:35432:5432" + healthcheck: + test: ["CMD-SHELL", "pg_isready -U sqlstores -d sqlstores"] + interval: 2s + timeout: 5s + retries: 30 + + mysql80: + image: mysql:8.0@sha256:7dcddc01f13bab2f15cde676d44d01f61fc9f99fe7785e86196dfc07d358ae2b + environment: + MYSQL_ROOT_PASSWORD: ${SQLSTORES_DB_PASSWORD:?set SQLSTORES_DB_PASSWORD} + ports: + - "127.0.0.1:33306:3306" + healthcheck: + test: ["CMD-SHELL", "mysqladmin ping -h 127.0.0.1 -uroot -p\"$$MYSQL_ROOT_PASSWORD\" --silent"] + interval: 2s + timeout: 5s + retries: 60 + + mysql84: + image: mysql:8.4@sha256:24989179f58856db763f418010c1169fecc944e3ec1ea7e921572d83eb070f70 + environment: + MYSQL_ROOT_PASSWORD: ${SQLSTORES_DB_PASSWORD:?set SQLSTORES_DB_PASSWORD} + ports: + - "127.0.0.1:33307:3306" + healthcheck: + test: ["CMD-SHELL", "mysqladmin ping -h 127.0.0.1 -uroot -p\"$$MYSQL_ROOT_PASSWORD\" --silent"] + interval: 2s + timeout: 5s + retries: 60 + + mariadb: + image: mariadb:11.4@sha256:1292844148b311e4ed4300022a996d39083f415a963e970cf47cad1b3b18e3a6 + environment: + MARIADB_ROOT_PASSWORD: ${SQLSTORES_DB_PASSWORD:?set SQLSTORES_DB_PASSWORD} + ports: + - "127.0.0.1:33308:3306" + healthcheck: + test: ["CMD", "healthcheck.sh", "--connect", "--innodb_initialized"] + interval: 2s + timeout: 5s + retries: 60 diff --git a/internal/interop/sqlstores/doc_test.go b/internal/interop/sqlstores/doc_test.go new file mode 100644 index 00000000..f6babd10 --- /dev/null +++ b/internal/interop/sqlstores/doc_test.go @@ -0,0 +1,12 @@ +// 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 runs the three SQL stores of the repository — +// oauth2/store/sql, oidc/client/store/sql and saml/sp/store/sql — on real +// database servers: their Migrate racing on one empty database, the +// advisory lock Migrate takes on PostgreSQL, the oauth2 storage +// conformance suite, and the MySQL upgrade statements of MIGRATION.md. The +// module holds tests only and is never released. SQLite runs everywhere; +// PostgreSQL, MySQL and MariaDB are opt-in (see compose.yaml). +package sqlstores_test diff --git a/internal/interop/sqlstores/go.mod b/internal/interop/sqlstores/go.mod new file mode 100644 index 00000000..847dbebb --- /dev/null +++ b/internal/interop/sqlstores/go.mod @@ -0,0 +1,64 @@ +module github.com/hyperscale-stack/security/internal/interop/sqlstores + +go 1.27.2 + +replace github.com/hyperscale-stack/security => ../../../ + +replace github.com/hyperscale-stack/security/oauth2 => ../../../oauth2 + +replace github.com/hyperscale-stack/security/oauth2/store/sql => ../../../oauth2/store/sql + +replace github.com/hyperscale-stack/security/oidc/client => ../../../oidc/client + +replace github.com/hyperscale-stack/security/oidc/client/store/sql => ../../../oidc/client/store/sql + +replace github.com/hyperscale-stack/security/saml/sp => ../../../saml/sp + +replace github.com/hyperscale-stack/security/saml/sp/store/sql => ../../../saml/sp/store/sql + +require ( + github.com/go-sql-driver/mysql v1.10.1 + github.com/hyperscale-stack/security/oauth2 v0.0.0-00010101000000-000000000000 + github.com/hyperscale-stack/security/oauth2/store/sql v0.0.0-00010101000000-000000000000 + github.com/hyperscale-stack/security/oidc/client v0.0.0-00010101000000-000000000000 + github.com/hyperscale-stack/security/oidc/client/store/sql v0.0.0-00010101000000-000000000000 + github.com/hyperscale-stack/security/saml/sp v0.0.0-00010101000000-000000000000 + github.com/hyperscale-stack/security/saml/sp/store/sql v0.0.0-00010101000000-000000000000 + github.com/jackc/pgx/v5 v5.11.0 + github.com/stretchr/testify v1.12.1 + modernc.org/sqlite v1.60.1 +) + +require ( + filippo.io/edwards25519 v1.2.0 // indirect + github.com/beevik/etree v1.8.1 // indirect + github.com/cespare/xxhash/v2 v2.3.0 // indirect + github.com/dustin/go-humanize v1.0.1 // indirect + github.com/go-jose/go-jose/v4 v4.1.5 // indirect + github.com/go-logr/logr v1.4.4 // indirect + github.com/go-logr/stdr v1.2.2 // indirect + github.com/google/uuid v1.6.0 // indirect + github.com/hyperscale-stack/security v0.0.0-00010101000000-000000000000 // indirect + github.com/jackc/pgpassfile v1.0.0 // indirect + github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect + github.com/jackc/puddle/v2 v2.2.2 // indirect + github.com/jonboulle/clockwork v0.5.0 // indirect + github.com/mattermost/xml-roundtrip-validator v0.1.0 // indirect + github.com/mattn/go-isatty v0.0.24 // indirect + github.com/ncruces/go-strftime v1.0.0 // indirect + github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect + github.com/russellhaering/gosaml2 v0.12.0 // indirect + github.com/russellhaering/goxmldsig v1.6.1 // indirect + go.opentelemetry.io/auto/sdk v1.2.1 // indirect + go.opentelemetry.io/otel v1.47.0 // indirect + go.opentelemetry.io/otel/log v1.47.0 // indirect + go.opentelemetry.io/otel/metric v1.47.0 // indirect + go.opentelemetry.io/otel/trace v1.47.0 // indirect + go.yaml.in/yaml/v3 v3.0.5 // indirect + golang.org/x/sync v0.24.0 // indirect + golang.org/x/sys v0.49.0 // indirect + golang.org/x/text v0.43.0 // indirect + modernc.org/libc v1.77.1 // indirect + modernc.org/mathutil v1.7.1 // indirect + modernc.org/memory v1.12.1 // indirect +) diff --git a/internal/interop/sqlstores/go.sum b/internal/interop/sqlstores/go.sum new file mode 100644 index 00000000..32d7a54e --- /dev/null +++ b/internal/interop/sqlstores/go.sum @@ -0,0 +1,111 @@ +filippo.io/edwards25519 v1.2.0 h1:crnVqOiS4jqYleHd9vaKZ+HKtHfllngJIiOpNpoJsjo= +filippo.io/edwards25519 v1.2.0/go.mod h1:xzAOLCNug/yB62zG1bQ8uziwrIqIuxhctzJT18Q77mc= +github.com/beevik/etree v1.8.1 h1:MchsAnqPGCGsfQezhwcouHPlAHlcAOqWpyCVZoyWfjU= +github.com/beevik/etree v1.8.1/go.mod h1:bh4zJxiIr62SOf9pRzN7UUYaEDa9HEKafK25+sLc0Gc= +github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= +github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY= +github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto= +github.com/go-jose/go-jose/v4 v4.1.5 h1:RjgjO2LOtWOJKUC5wpwY9LR3B3vwVAz6JS2YHfYU6eA= +github.com/go-jose/go-jose/v4 v4.1.5/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= +github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= +github.com/go-logr/logr v1.4.4 h1:tG4xh9yMsRCAiodLVTxyrkzSZ9+o0L1Kg/+cPVcbP/8= +github.com/go-logr/logr v1.4.4/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= +github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= +github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= +github.com/go-sql-driver/mysql v1.10.1 h1:arlSnNLq6a5yxGxV7qg9lF4j0C+KwD6NbQyKr9QL6ME= +github.com/go-sql-driver/mysql v1.10.1/go.mod h1:M+cqaI7+xxXGG9swrdeUIoPG3Y3KCkF0pZej+SK+nWk= +github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= +github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= +github.com/google/pprof v0.0.0-20260802141513-ef3492d7dac3 h1:LMLX+LgTNWpfvCBdFebv6EsYotImrt/Ppc5cXIriCSo= +github.com/google/pprof v0.0.0-20260802141513-ef3492d7dac3/go.mod h1:jl5iWTm0/hd5PjEYEOuwAJ57L/CibdZfrqZ5XA5GrCk= +github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= +github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs4luLUK2k= +github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM= +github.com/jackc/pgpassfile v1.0.0 h1:/6Hmqy13Ss2zCq62VdNG8tM1wchn8zjSGOBJ6icpsIM= +github.com/jackc/pgpassfile v1.0.0/go.mod h1:CEx0iS5ambNFdcRtxPj5JhEz+xB6uRky5eyVu/W2HEg= +github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 h1:iCEnooe7UlwOQYpKFhBabPMi4aNAfoODPEFNiAnClxo= +github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761/go.mod h1:5TJZWKEWniPve33vlWYSoGYefn3gLQRzjfDlhSJ9ZKM= +github.com/jackc/pgx/v5 v5.11.0 h1:IzBBtyK9AHqf98cctWFifYSci2hgQR/cd56wB4p+ogg= +github.com/jackc/pgx/v5 v5.11.0/go.mod h1:mal1tBGAFfLHvZzaYh77YS/eC6IX9OWbRV1QIIM0Jn4= +github.com/jackc/puddle/v2 v2.2.2 h1:PR8nw+E/1w0GLuRFSmiioY6UooMp6KJv0/61nB7icHo= +github.com/jackc/puddle/v2 v2.2.2/go.mod h1:vriiEXHvEE654aYKXXjOvZM39qJ0q+azkZFrfEOc3H4= +github.com/jonboulle/clockwork v0.5.0 h1:Hyh9A8u51kptdkR+cqRpT1EebBwTn1oK9YfGYbdFz6I= +github.com/jonboulle/clockwork v0.5.0/go.mod h1:3mZlmanh0g2NDKO5TWZVJAfofYk64M7XN3SzBPjZF60= +github.com/mattermost/xml-roundtrip-validator v0.1.0 h1:RXbVD2UAl7A7nOTR4u7E3ILa4IbtvKBHw64LDsmu9hU= +github.com/mattermost/xml-roundtrip-validator v0.1.0/go.mod h1:qccnGMcpgwcNaBnxqpJpWWUiPNr5H3O8eDgGV9gT5To= +github.com/mattn/go-isatty v0.0.24 h1:tGZZoVgT/KiqK1c8ocVLeDS8BSWMRd47J3Lbz7vsReI= +github.com/mattn/go-isatty v0.0.24/go.mod h1:nMCL3Zebbrt45jsMDgnfIwz6ydEQApk5oEI3HqDio6A= +github.com/ncruces/go-strftime v1.0.0 h1:HMFp8mLCTPp341M/ZnA4qaf7ZlsbTc+miZjCLOFAw7w= +github.com/ncruces/go-strftime v1.0.0/go.mod h1:Fwc5htZGVVkseilnfgOVb9mKy6w1naJmn9CehxcKcls= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec h1:W09IVJc94icq4NjY3clb7Lk8O1qJ8BdBEF8z0ibU0rE= +github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo= +github.com/russellhaering/gosaml2 v0.12.0 h1:/otu5cL+pAUnO41Mi2zAbOd6GP+pTqYCMBUlxL8yw4E= +github.com/russellhaering/gosaml2 v0.12.0/go.mod h1:txFBNgTGPBhdtNqMo+XR5YPqavMyqhks5GL0scTP+r4= +github.com/russellhaering/goxmldsig v1.6.1 h1:SB7R5ttvrGIDB2juJAK/i7DQ2Ivr7agG+ohfNJjwyYU= +github.com/russellhaering/goxmldsig v1.6.1/go.mod h1:haZkRcLs9W/Xp989fIjP3BrTdbFQveRF0QNZSYoH09w= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.5.3 h1:jmXUvGomnU1o3W/V5h2VEradbpJDwGrzugQQvL0POH4= +github.com/stretchr/objx v0.5.3/go.mod h1:rDQraq+vQZU7Fde9LOZLr8Tax6zZvy4kuNKF+QYS+U0= +github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= +github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.12.1 h1:EuwCh5fleGS7H32xRwO3wRGT7DxrDhLAT6FF8MpWDWE= +github.com/stretchr/testify v1.12.1/go.mod h1:MDEgiDPPsNp5cuIrHPPCyornHKgEVbtFUmoNlxoYthg= +go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= +go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= +go.opentelemetry.io/otel v1.47.0 h1:j7ALJ/zgkS7Z6aeJW09p8VC9804bC+PpeTfCD4XPnOM= +go.opentelemetry.io/otel v1.47.0/go.mod h1:8wS9O2qfXrYrzp6hIF/HOYJJf/wIhFPhR2xLuP+iXQU= +go.opentelemetry.io/otel/log v1.47.0 h1:cOTS1CcLbSQeZKanGJ+0JpF/+t4PELi3O3bbl2lqCcI= +go.opentelemetry.io/otel/log v1.47.0/go.mod h1:9byitSQ5pLC6PpqwGXjqdMKya6ZTswHRZh2vvXT33nw= +go.opentelemetry.io/otel/metric v1.47.0 h1:4PptaldXx3Eat1XjMZ68pPJEs5wrhlemctZE9a3UdWY= +go.opentelemetry.io/otel/metric v1.47.0/go.mod h1:ADGSXxRrXM6bjbvLo535EstVFlPpPYZm4LBKixjDHwU= +go.opentelemetry.io/otel/sdk v1.47.0 h1:zWXEr4j2lFefG87TU6Yg8a7ngfohIKFZHKp0Hf5hC6I= +go.opentelemetry.io/otel/sdk v1.47.0/go.mod h1:VUc24kiOeoGsxG8G9ULx3fWKvB7jMhnGE8Oi607lgR0= +go.opentelemetry.io/otel/trace v1.47.0 h1:JOjX/Oci8K94QHddo+bbfya/Ai/nf6/dt9ZfrFNWSrM= +go.opentelemetry.io/otel/trace v1.47.0/go.mod h1:jNaSLa2PZEYFG6fRjJABAu+bw4FS08uDmPg28lTghu0= +go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= +go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg= +golang.org/x/mod v0.41.0 h1:qJmnOUb4YB+FsEuM3HcWucdZASCPGhsX6uljO6pog0c= +golang.org/x/mod v0.41.0/go.mod h1:Ek9pY8RKWXwsWvd3rQiHYtMqkjSUV+s1Rj7j4H5Ur6o= +golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA= +golang.org/x/sync v0.24.0/go.mod h1:sUUOizhqBxiL6pEWpqNLUiaJn1ShEbZ6BBqskPbjZm0= +golang.org/x/sys v0.49.0 h1:XbzkgYJHdqh/8m2Uu0W/dQv8nktxx4BFHp1M0gROTXA= +golang.org/x/sys v0.49.0/go.mod h1:hNLxWAXmnKAxqDtdwIYC4bM9oQPEecfsnNMuSxOs3og= +golang.org/x/text v0.43.0 h1:1QivrlhwAsnMOcqgOdl9my7s9OyZl8O5/UGUTJVXoWc= +golang.org/x/text v0.43.0/go.mod h1:hbSIYA/amXcRXTFSZqh8tWHEoqRvvbKPJ2uz/KU+a+s= +golang.org/x/tools v0.51.0 h1:k4Xc/1Om9jwkBJBo4NVLMSARBoWtK10mx+W5BnXCeAI= +golang.org/x/tools v0.51.0/go.mod h1:9eEncMayCV6zRMGhR5eZEC2iBx98qWcF1HZ9Z7wJOoA= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +modernc.org/cc/v4 v4.29.7 h1:q+NXGJ0bK3b4TXFYQQVr9pYETGnmwFWkrUzJnMya/Tg= +modernc.org/cc/v4 v4.29.7/go.mod h1:OnovgIhbbMXMu1aISnJ0wvVD1KnW+cAUJkIrAWh+kVI= +modernc.org/ccgo/v4 v4.36.1 h1:ZNIUZAryN0UgnJwtyxrdEzcFc3yD4Cu4AzjfPXsLsIE= +modernc.org/ccgo/v4 v4.36.1/go.mod h1:rrtGc2QkS239nYb/mQNuBMyjq3/y3ZXWbBjPoV3wqzA= +modernc.org/fileutil v1.4.0 h1:j6ZzNTftVS054gi281TyLjHPp6CPHr2KCxEXjEbD6SM= +modernc.org/fileutil v1.4.0/go.mod h1:EqdKFDxiByqxLk8ozOxObDSfcVOv/54xDs/DUHdvCUU= +modernc.org/gc/v2 v2.6.5 h1:nyqdV8q46KvTpZlsw66kWqwXRHdjIlJOhG6kxiV/9xI= +modernc.org/gc/v2 v2.6.5/go.mod h1:YgIahr1ypgfe7chRuJi2gD7DBQiKSLMPgBQe9oIiito= +modernc.org/gc/v3 v3.1.5 h1:21ldfPfRYE31Tb7B3mwAK8gy1AxP4+dKjrOQPfqakoc= +modernc.org/gc/v3 v3.1.5/go.mod h1:HFK/6AGESC7Ex+EZJhJ2Gni6cTaYpSMmU/cT9RmlfYY= +modernc.org/goabi0 v0.2.0 h1:HvEowk7LxcPd0eq6mVOAEMai46V+i7Jrj13t4AzuNks= +modernc.org/goabi0 v0.2.0/go.mod h1:CEFRnnJhKvWT1c1JTI3Avm+tgOWbkOu5oPA8eH8LnMI= +modernc.org/libc v1.77.1 h1:Ct8j47QtiZ1Enj2DtFXQtUqrPCAjdCmPjtCuvrYQ0Hs= +modernc.org/libc v1.77.1/go.mod h1:87/pZ4L6nD1zqW4nItuS12YO7hN1igAah34xjnQo/W0= +modernc.org/mathutil v1.7.1 h1:GCZVGXdaN8gTqB1Mf/usp1Y/hSqgI2vAGGP4jZMCxOU= +modernc.org/mathutil v1.7.1/go.mod h1:4p5IwJITfppl0G4sUEDtCr4DthTaT47/N3aT6MhfgJg= +modernc.org/memory v1.12.1 h1:nFMiWrpStgZczNl6XI9GnIk/rWhYIyHGUaR04pGbp9g= +modernc.org/memory v1.12.1/go.mod h1:/JP4VbVC+K5sU2wZi9bHoq2MAkCnrt2r98UGeSK7Mjw= +modernc.org/opt v0.2.0 h1:tGyef5ApycA7FSEOMraay9SaTk5zmbx7Tu+cJs4QKZg= +modernc.org/opt v0.2.0/go.mod h1:03fq9lsNfvkYSfxrfUhZCWPk1lm4cq4N+Bh//bEtgns= +modernc.org/sortutil v1.2.1 h1:+xyoGf15mM3NMlPDnFqrteY07klSFxLElE2PVuWIJ7w= +modernc.org/sortutil v1.2.1/go.mod h1:7ZI3a3REbai7gzCLcotuw9AC4VZVpYMjDzETGsSMqJE= +modernc.org/sqlite v1.60.1 h1:/blz53O951KWFOso4QQvEs/Fq6cDBKLtMVrYNSeJVKw= +modernc.org/sqlite v1.60.1/go.mod h1:1dIoEagfDE72QytD5scH1lxARtaUgKgHC/NuApA27r0= +modernc.org/strutil v1.2.1 h1:UneZBkQA+DX2Rp35KcM69cSsNES9ly8mQWD71HKlOA0= +modernc.org/strutil v1.2.1/go.mod h1:EHkiggD70koQxjVdSBM3JKM7k6L0FbGE5eymy9i3B9A= +modernc.org/token v1.1.0 h1:Xl7Ap9dKaEs5kLoOQeQmPWevfnk/DM5qcLcYlA8ys6Y= +modernc.org/token v1.1.0/go.mod h1:UGzOrNV1mAFSEB63lOFHIpNRUVMvYTc6yu1SMY/XTDM= diff --git a/internal/interop/sqlstores/migrate_test.go b/internal/interop/sqlstores/migrate_test.go new file mode 100644 index 00000000..d7c93d08 --- /dev/null +++ b/internal/interop/sqlstores/migrate_test.go @@ -0,0 +1,392 @@ +// 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" + "crypto/sha256" + "database/sql" + "encoding/binary" + "encoding/hex" + "strconv" + "sync" + "testing" + "time" + + "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/require" +) + +// sqlStore is one of the SQL stores under test. +type sqlStore struct { + name string + // module is the store's module path, from which the documented + // PostgreSQL advisory lock of its Migrate is derived. + module string + // tables are the tables its Schema creates, in creation order. + tables []string + // migrate runs the store's Migrate on db. + migrate func(ctx context.Context, db *sql.DB, e engine) error + // use writes and reads a record, proving the tables serve the store. + use func(t *testing.T, db *sql.DB, e engine) +} + +var sqlStores = []sqlStore{ + { + name: "oauth2", + module: "github.com/hyperscale-stack/security/oauth2/store/sql", + tables: []string{"oauth2_auth_codes", "oauth2_access_tokens", "oauth2_refresh_tokens", "oauth2_token_families"}, + migrate: func(ctx context.Context, db *sql.DB, e engine) error { + s, err := oauth2sql.New(db, oauth2Dialect(e)) + if err != nil { + return err + } + + return s.Migrate(ctx) + }, + use: useOAuth2, + }, + { + name: "oidc", + module: "github.com/hyperscale-stack/security/oidc/client/store/sql", + tables: []string{"oidc_client_transactions"}, + migrate: func(ctx context.Context, db *sql.DB, e engine) error { + s, err := oidcsql.New(db, oidcDialect(e)) + if err != nil { + return err + } + + return s.Migrate(ctx) + }, + use: useOIDC, + }, + { + name: "saml", + module: "github.com/hyperscale-stack/security/saml/sp/store/sql", + tables: []string{"samlsp_transactions", "samlsp_used_assertions"}, + migrate: func(ctx context.Context, db *sql.DB, e engine) error { + s, err := samlsql.New(db, samlDialect(e)) + if err != nil { + return err + } + + return s.Migrate(ctx) + }, + use: useSAML, + }, +} + +// migrationLock is the PostgreSQL advisory lock the Migrate of the store +// module takes, as its package documentation derives it: the first eight +// bytes of SHA-256(".Migrate"), big-endian, as a bigint. +func migrationLock(module string) string { + sum := sha256.Sum256([]byte(module + ".Migrate")) + + return strconv.FormatInt(int64(binary.BigEndian.Uint64(sum[:8])), 10) //nolint:gosec // G115: the bigint key is these 64 bits +} + +// sha256Hex returns hex(SHA-256(s)): a store key of the OIDC and SAML +// stores. +func sha256Hex(s string) string { + sum := sha256.Sum256([]byte(s)) + + return hex.EncodeToString(sum[:]) +} + +func useOAuth2(t *testing.T, db *sql.DB, e engine) { + t.Helper() + + s, err := oauth2sql.New(db, oauth2Dialect(e)) + require.NoError(t, err) + + pair := tokenPair(randomName("use-"), randomName("fam-use-")) + require.NoError(t, s.SaveTokenPair(t.Context(), pair)) + + _, err = s.LookupAccessToken(t.Context(), pair.Access.TokenHash) + require.NoError(t, err) +} + +func useOIDC(t *testing.T, db *sql.DB, e engine) { + t.Helper() + + s, err := oidcsql.New(db, oidcDialect(e)) + require.NoError(t, err) + + seed := randomName("use-") + now := time.Unix(time.Now().Unix(), 0).UTC() + tx := &oidcclient.Transaction{ + StateHash: sha256Hex("state-" + seed), BindingHash: sha256Hex("binding-" + seed), + 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: now, ExpiresAt: now.Add(10 * time.Minute), + Integrity: sha256Hex("integrity-" + seed), + } + require.NoError(t, s.Save(t.Context(), tx)) + + got, err := s.Consume(t.Context(), tx.StateHash, tx.BindingHash, now) + require.NoError(t, err) + require.Equal(t, tx.StateHash, got.StateHash) +} + +func useSAML(t *testing.T, db *sql.DB, e engine) { + t.Helper() + + s, err := samlsql.New(db, samlDialect(e)) + require.NoError(t, err) + + seed := randomName("use-") + rec := &samlsp.TransactionRecord{ + ID: sha256Hex("id-" + seed), BindingHash: sha256Hex("binding-" + seed), Payload: `{"req":"x"}`, + ExpiresAt: time.Unix(time.Now().Add(10*time.Minute).Unix(), 0), Integrity: sha256Hex("integrity-" + seed), + } + require.NoError(t, s.SaveTransaction(t.Context(), rec)) + require.NoError(t, s.ConsumeTransaction(t.Context(), rec.ID, rec.BindingHash)) +} + +// tokenPair returns a pair in family: access token "at-" and refresh +// token "rt-". +func tokenPair(name, family string) *oauth2.TokenPair { + now := time.Now() + + return &oauth2.TokenPair{ + Access: oauth2.AccessToken{ + TokenHash: "at-" + name, ClientID: "client-1", Subject: "alice", Scope: "read", Audience: "api", + FamilyID: family, IssuedAt: now, ExpiresAt: now.Add(time.Hour), Issuer: "https://issuer.example", + }, + Refresh: &oauth2.RefreshToken{ + TokenHash: "rt-" + name, ClientID: "client-1", Subject: "alice", Scope: "read", + FamilyID: family, IssuedAt: now, ExpiresAt: now.Add(24 * time.Hour), Issuer: "https://issuer.example", + }, + } +} + +// tableExists reports whether the namespace db works in holds table. +func tableExists(t *testing.T, db *sql.DB, e engine, table string) bool { + t.Helper() + + q := `SELECT count(*) FROM information_schema.tables WHERE table_schema = current_schema() AND table_name = $1` + + switch e { + case engineMySQL: + q = `SELECT count(*) FROM information_schema.tables WHERE table_schema = DATABASE() AND table_name = ?` + case engineSQLite: + q = `SELECT count(*) FROM sqlite_master WHERE type = 'table' AND name = ?` + } + + var n int + require.NoError(t, db.QueryRowContext(t.Context(), q, table).Scan(&n)) + + return n == 1 +} + +// requireTables asserts that every table of s exists, or that none does. +func requireTables(t *testing.T, db *sql.DB, e engine, s sqlStore, exist bool) { + t.Helper() + + for _, table := range s.tables { + require.Equal(t, exist, tableExists(t, db, e, table), "table %s exists", table) + } +} + +// The size of TestMigrateConcurrently: rounds of instances booting at once +// on an empty database, each Migrate bounded by migrateDeadline, so that a +// Migrate waiting on a lock it never gets fails fast. +const ( + migrateRounds = 5 + migrateInstances = 12 + migrateDeadline = 15 * time.Second +) + +// TestMigrateConcurrently boots twelve instances at once on an empty +// database — four per store, each with its own connection pool, the three +// stores migrating together — round after round: every Migrate succeeds +// and the tables then serve their store. On PostgreSQL, concurrent CREATE +// TABLE IF NOT EXISTS of one table used to fail on the unique index of +// the catalog's row types (SQLSTATE 23505); the PostgreSQL rounds run at +// the server's default isolation and with SERIALIZABLE as the default. +// MySQL, MariaDB and SQLite (with its busy timeout) serialize the creation +// of a table themselves. +func TestMigrateConcurrently(t *testing.T) { + for _, b := range []backend{sqliteFile, postgres, postgresSerializable, mysql80, mysql84, mariadb} { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + for range migrateRounds { + migrateTogether(t, b) + } + }) + } +} + +// migrateTogether runs one round of TestMigrateConcurrently on a new +// namespace of b. +func migrateTogether(t *testing.T, b backend) { + t.Helper() + + driver, dsn := b.namespace(t) + + pools := make([]*sql.DB, migrateInstances) + for i := range pools { + pools[i] = open(t, driver, dsn, 2) + } + + if b.params != nil { + var isolation string + require.NoError(t, pools[0].QueryRowContext(t.Context(), `SHOW default_transaction_isolation`).Scan(&isolation)) + require.Equal(t, b.params["default_transaction_isolation"], isolation) + } + + var ( + wg sync.WaitGroup + start = make(chan struct{}) + errs = make([]error, migrateInstances) + ) + + for i := range migrateInstances { + wg.Go(func() { + <-start + + ctx, cancel := context.WithTimeout(context.Background(), migrateDeadline) + defer cancel() + + errs[i] = sqlStores[i%len(sqlStores)].migrate(ctx, pools[i], b.engine) + }) + } + + close(start) + wg.Wait() + + for i, err := range errs { + require.NoError(t, err, "Migrate of instance %d (%s)", i, sqlStores[i%len(sqlStores)].name) + } + + for i, s := range sqlStores { + requireTables(t, pools[i], b.engine, s, true) + s.use(t, pools[i], b.engine) + } + + for _, db := range pools { + _ = db.Close() + } +} + +// migrateWithin runs the Migrate of s under ctx, and fails t unless it +// returns within limit: a Migrate that ignores its context, waiting for a +// lock it never gets, fails the test instead of hanging it. +func migrateWithin(t *testing.T, ctx context.Context, limit time.Duration, s sqlStore, db *sql.DB) error { + t.Helper() + + done := make(chan error, 1) + + go func() { done <- s.migrate(ctx, db, enginePostgres) }() + + select { + case err := <-done: + return err + case <-time.After(limit): + t.Fatalf("%s: Migrate still running after %s: it ignores its context", s.name, limit) + + return nil + } +} + +// holdLock takes the PostgreSQL advisory lock key in a transaction of +// db, which the caller ends. +func holdLock(t *testing.T, db *sql.DB, key string) *sql.Tx { + t.Helper() + + tx, err := db.BeginTx(t.Context(), nil) + require.NoError(t, err) + t.Cleanup(func() { _ = tx.Rollback() }) + + _, err = tx.ExecContext(t.Context(), "SELECT pg_advisory_xact_lock("+key+")") + require.NoError(t, err) + + return tx +} + +// TestMigrateWaitsForItsLock holds, in another transaction, the advisory +// lock each store's package documentation names: the store's Migrate +// waits for it — until its context ends, having created nothing — while +// the Migrate of another store, whose lock is distinct, goes through; +// once the holder commits, Migrate creates its tables. +func TestMigrateWaitsForItsLock(t *testing.T) { + postgres.server(t) + + for i, s := range sqlStores { + t.Run(s.name, func(t *testing.T) { + driver, dsn := postgres.namespace(t) + db := open(t, driver, dsn, 2) + holder := holdLock(t, open(t, driver, dsn, 1), migrationLock(s.module)) + + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + + require.ErrorIs(t, migrateWithin(t, ctx, 10*time.Second, s, db), context.DeadlineExceeded) + requireTables(t, db, enginePostgres, s, false) + + // Bounded: a store sharing the held lock would wait for good. + otherCtx, cancelOther := context.WithTimeout(t.Context(), 10*time.Second) + defer cancelOther() + + other := sqlStores[(i+1)%len(sqlStores)] + require.NoError(t, migrateWithin(t, otherCtx, 15*time.Second, other, db), "%s's lock is not %s's", other.name, s.name) + requireTables(t, db, enginePostgres, other, true) + + require.NoError(t, holder.Commit()) + require.NoError(t, s.migrate(t.Context(), db, enginePostgres)) + requireTables(t, db, enginePostgres, s, true) + }) + } +} + +// TestMigrateFailureCreatesNothing makes the creation of each store's last +// table fail on PostgreSQL — an enum type named like the table already +// exists, so the table's row type cannot be created (a composite type +// would not do: it is a relation, which IF NOT EXISTS skips) — and checks +// that Migrate, which runs in one transaction there, leaves none of the +// store's tables behind. Run under a context that never ends, the usual +// call at boot, the failed Migrate also ends its transaction: its +// connection goes back to the pool — not left "idle in transaction +// (aborted)" — and, on a one-connection pool, the next Migrate goes +// through once the cause is gone. +func TestMigrateFailureCreatesNothing(t *testing.T) { + postgres.server(t) + + for _, s := range sqlStores { + t.Run(s.name, func(t *testing.T) { + driver, dsn := postgres.namespace(t) + admin := open(t, driver, dsn, 1) + db := open(t, driver, dsn, 1) + + blocker := s.tables[len(s.tables)-1] + _, err := admin.ExecContext(t.Context(), "CREATE TYPE "+blocker+" AS ENUM ('taken')") + require.NoError(t, err) + + require.Error(t, s.migrate(context.Background(), db, enginePostgres)) + requireTables(t, admin, enginePostgres, s, false) + require.Zero(t, db.Stats().InUse, "the failed Migrate gave its connection back") + + var aborted int + require.NoError(t, admin.QueryRowContext(t.Context(), `SELECT count(*) FROM pg_stat_activity + WHERE datname = current_database() AND state = 'idle in transaction (aborted)'`).Scan(&aborted)) + require.Zero(t, aborted, "backends left idle in transaction (aborted)") + + _, err = admin.ExecContext(t.Context(), "DROP TYPE "+blocker) + require.NoError(t, err) + + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + defer cancel() + + require.NoError(t, migrateWithin(t, ctx, 15*time.Second, s, db), "the next Migrate on the same pool") + requireTables(t, admin, enginePostgres, s, true) + }) + } +} diff --git a/internal/interop/sqlstores/oauth2_test.go b/internal/interop/sqlstores/oauth2_test.go new file mode 100644 index 00000000..4a3cd88b --- /dev/null +++ b/internal/interop/sqlstores/oauth2_test.go @@ -0,0 +1,474 @@ +// 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" + "regexp" + "strings" + "testing" + "time" + + "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" +) + +// mysql84FoundRows is MySQL 8.4 with the CLIENT_FOUND_ROWS flag, under +// which an UPDATE reports the rows it matched rather than those it changed. +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 { + t.Helper() + + db := open(t, driver, dsn, 8) + db.SetMaxIdleConns(1) + + return db +} + +// TestOAuth2Conformance runs the oauth2 storage conformance suite — code +// consumption and rotation under races, the token-family invariants, keys +// compared byte for byte — on every backend, each case in a new namespace +// that Migrate prepares. +func TestOAuth2Conformance(t *testing.T) { + for _, b := range []backend{sqliteFile, postgres, mysql80, mysql84, mysql84FoundRows, mariadb} { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + storetest.RunConformance(t, func() oauth2.Storage { + driver, dsn := b.namespace(t) + + s, err := oauth2sql.New(conformancePool(t, driver, dsn), oauth2Dialect(b.engine)) + require.NoError(t, err) + require.NoError(t, s.Migrate(t.Context())) + + return s + }) + }) + } +} + +// legacyMySQLSchema is the DDL Schema returned for MySQL before its key +// columns became binary strings: the tables a deployment upgrading from it +// still has. +var legacyMySQLSchema = []string{ + `CREATE TABLE oauth2_auth_codes ( + code_hash VARCHAR(128) PRIMARY KEY, + client_id VARCHAR(255) NOT NULL, + subject VARCHAR(255) NOT NULL, + redirect_uri TEXT NOT NULL, + scope TEXT NOT NULL, + code_challenge TEXT NOT NULL, + code_challenge_method VARCHAR(16) NOT NULL, + nonce TEXT NOT NULL, + consumed SMALLINT NOT NULL DEFAULT 0, + issued_at BIGINT NOT NULL, + expires_at BIGINT NOT NULL, + issuer TEXT, + extension_data TEXT, + redirect_uri_omitted SMALLINT NOT NULL DEFAULT 0 + )`, + `CREATE TABLE oauth2_access_tokens ( + token_hash VARCHAR(128) PRIMARY KEY, + client_id VARCHAR(255) NOT NULL, + subject VARCHAR(255) NOT NULL, + scope TEXT NOT NULL, + family_id VARCHAR(64) NOT NULL, + audience VARCHAR(255) NOT NULL, + issued_at BIGINT NOT NULL, + expires_at BIGINT NOT NULL, + issuer TEXT, + extension_data TEXT + )`, + `CREATE TABLE oauth2_refresh_tokens ( + token_hash VARCHAR(128) PRIMARY KEY, + client_id VARCHAR(255) NOT NULL, + subject VARCHAR(255) NOT NULL, + scope TEXT NOT NULL, + family_id VARCHAR(64) NOT NULL, + consumed SMALLINT NOT NULL DEFAULT 0, + issued_at BIGINT NOT NULL, + expires_at BIGINT NOT NULL, + issuer TEXT, + extension_data TEXT, + family_issued_at BIGINT NOT NULL DEFAULT 0 + )`, + `CREATE TABLE oauth2_token_families ( + family_id VARCHAR(64) PRIMARY KEY, + revoked_at BIGINT NOT NULL DEFAULT 0, + expires_at BIGINT NOT NULL + )`, +} + +// upgradeSection is the heading of the MIGRATION.md section whose SQL +// blocks — the preflight queries, the upgrade, the optional conversion of +// the codes' table — TestOAuth2MySQLUpgrade runs: the documented +// statements are the tested ones. +const upgradeSection = "### `oauth2/store/sql`: MySQL compares keys byte for byte" + +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. +func upgradeBlocks(t *testing.T) [][]string { + t.Helper() + + raw, err := os.ReadFile(filepath.Join("..", "..", "..", "MIGRATION.md")) + require.NoError(t, err) + + doc := string(raw) + start := strings.Index(doc, "\n"+upgradeSection+"\n") + require.GreaterOrEqual(t, start, 0, "MIGRATION.md has no section %q", upgradeSection) + + section := doc[start+1:] + if end := strings.Index(section[len(upgradeSection):], "\n### "); end >= 0 { + section = section[:len(upgradeSection)+end] + } + + var blocks [][]string + + for _, m := range sqlBlock.FindAllStringSubmatch(section, -1) { + var ( + stmts []string + stmt strings.Builder + ) + + for _, line := range strings.Split(m[1], "\n") { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "--") { + continue + } + + stmt.WriteString(line + "\n") + + if strings.HasSuffix(trimmed, ";") { + stmts = append(stmts, strings.TrimSuffix(strings.TrimSpace(stmt.String()), ";")) + stmt.Reset() + } + } + + require.Empty(t, strings.TrimSpace(stmt.String()), "a statement without its semicolon") + + blocks = append(blocks, stmts) + } + + return blocks +} + +// execAll runs stmts on one connection of db, so that a SET SESSION +// applies to the statements after it. +func execAll(t *testing.T, db *sql.DB, stmts ...string) { + t.Helper() + + conn, err := db.Conn(t.Context()) + require.NoError(t, err) + + defer func() { _ = conn.Close() }() + + for _, stmt := range stmts { + _, err := conn.ExecContext(t.Context(), stmt) + require.NoError(t, err, stmt) + } +} + +// queryRow returns the integers of the single row query returns. +func queryRow(t *testing.T, db *sql.DB, query string) []int64 { + t.Helper() + + rows, err := db.QueryContext(t.Context(), query) + require.NoError(t, err, query) + + defer func() { _ = rows.Close() }() + + columns, err := rows.Columns() + require.NoError(t, err) + require.True(t, rows.Next(), "a row") + + values := make([]int64, len(columns)) + targets := make([]any, len(columns)) + + for i := range values { + targets[i] = &values[i] + } + + require.NoError(t, rows.Scan(targets...)) + require.False(t, rows.Next(), "a single row") + + return values +} + +// requireBinaryKeys asserts that the six key columns of an upgraded +// database are binary strings of their length, NOT NULL, and that the +// primary keys survived the conversion. +func requireBinaryKeys(t *testing.T, db *sql.DB) { + t.Helper() + + for _, c := range []struct { + table, column string + length int64 + primary bool + }{ + {"oauth2_auth_codes", "code_hash", 128, true}, + {"oauth2_access_tokens", "token_hash", 128, true}, + {"oauth2_access_tokens", "family_id", 64, false}, + {"oauth2_refresh_tokens", "token_hash", 128, true}, + {"oauth2_refresh_tokens", "family_id", 64, false}, + {"oauth2_token_families", "family_id", 64, true}, + } { + var ( + dataType, nullable, key string + length int64 + ) + + require.NoError(t, db.QueryRowContext(t.Context(), `SELECT DATA_TYPE, CHARACTER_MAXIMUM_LENGTH, IS_NULLABLE, COLUMN_KEY + FROM information_schema.COLUMNS WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = ? AND COLUMN_NAME = ?`, + c.table, c.column).Scan(&dataType, &length, &nullable, &key)) + + name := c.table + "." + c.column + require.Equal(t, "varbinary", dataType, name) + require.Equal(t, c.length, length, name) + require.Equal(t, "NO", nullable, name) + require.Equal(t, c.primary, key == "PRI", name+" primary key") + } +} + +// TestOAuth2MySQLUpgrade runs the upgrade statements of MIGRATION.md on +// tables created by the former MySQL DDL. Seeded with families the former +// collation merged with look-alikes — revoked, and active; whole pairs, +// a refresh token whose access token is gone, an access token alone — it +// checks what the upgrade keeps: the preflight counts the merged tokens, +// the key columns become binary, a revoked family and its look-alikes stay +// revoked even through a new issuance into each look-alike (the orphaned +// families are recorded revoked, for FamilyRevocationRetention at least), +// the look-alike of an active family reads revoked, other families and +// codes are untouched. In strict mode, an over-long key stops the upgrade +// instead of being truncated. The conformance suite then passes on +// upgraded tables. +func TestOAuth2MySQLUpgrade(t *testing.T) { + blocks := upgradeBlocks(t) + require.Len(t, blocks, 3, "the preflight, the upgrade and the conversion of the codes' table") + + preflight, upgrade := blocks[0], append(blocks[1], blocks[2]...) + require.Len(t, preflight, 2) + + for _, b := range []backend{mysql80, mysql84, mariadb} { + t.Run(b.name, func(t *testing.T) { + b.server(t) + + t.Run("revocations survive", func(t *testing.T) { + upgradeSeededDatabase(t, b, preflight, upgrade) + }) + + t.Run("over-long keys stop it", func(t *testing.T) { + upgradeRefusesOverLongKeys(t, b, preflight, upgrade) + }) + + t.Run("conformance", func(t *testing.T) { + storetest.RunConformance(t, func() oauth2.Storage { + driver, dsn := b.namespace(t) + db := conformancePool(t, driver, dsn) + execAll(t, db, legacyMySQLSchema...) + execAll(t, db, upgrade...) + + s, err := oauth2sql.New(db, oauth2sql.MySQL) + require.NoError(t, err) + + return s + }) + }) + }) + } +} + +// upgradeSeededDatabase is the seeded half of TestOAuth2MySQLUpgrade. +func upgradeSeededDatabase(t *testing.T, b backend, preflight, upgrade []string) { + t.Helper() + + start := time.Now().Unix() + + driver, dsn := b.namespace(t) + db := open(t, driver, dsn, 4) + execAll(t, db, legacyMySQLSchema...) + + s, err := oauth2sql.New(db, oauth2sql.MySQL) + require.NoError(t, err) + + // Under the former collation a pair saved into a look-alike of a + // family joins that family's record: revoking one revokes all of them. + var ( + alice = tokenPair("alice", "User-Alice") + aliceAlike = tokenPair("alice-alike", "user-alice") + aliceRefreshOnly = tokenPair("alice-refresh-only", "USER-ALICE") + aliceAccessOnly = tokenPair("alice-access-only", "uSeR-aLiCe") + bob = tokenPair("bob", "Team-Bob") + bobAlike = tokenPair("bob-alike", "team-bob") + plain = tokenPair("plain", "plain-family") + ) + + // An access token alone: a non-rotating refresh under Profile20. + aliceAccessOnly.Refresh = nil + + for _, p := range []*oauth2.TokenPair{alice, aliceAlike, aliceRefreshOnly, aliceAccessOnly, bob, bobAlike, plain} { + require.NoError(t, s.SaveTokenPair(t.Context(), p)) + } + + // A refresh token whose access token is gone: revoked, or cleaned up. + require.NoError(t, s.RevokeAccessToken(t.Context(), aliceRefreshOnly.Access.TokenHash)) + require.NoError(t, s.RevokeRefreshFamily(t.Context(), "User-Alice")) + + lookAlikes := map[string]func(){ + "user-alice": func() { requireUnusable(t, s, aliceAlike) }, + "USER-ALICE": func() { requireRefreshConsumed(t, s, aliceRefreshOnly) }, + "uSeR-aLiCe": func() { requireAccessRevoked(t, s, aliceAccessOnly) }, + } + for _, unusable := range lookAlikes { + unusable() + } + + code := &oauth2.AuthorizationCode{CodeHash: sha256Hex("legacy-code"), ClientID: "client-1", ExpiresAt: time.Now().Add(time.Hour)} + require.NoError(t, s.SaveAuthorizationCode(t.Context(), code)) + + require.Equal(t, []int64{3, 3}, queryRow(t, db, preflight[0]), "access and refresh tokens merged with a look-alike") + + for i, length := range queryRow(t, db, preflight[1]) { + require.LessOrEqual(t, length, []int64{64, 64, 64, 128, 128, 128}[i], "longest key %d", i) + } + + execAll(t, db, upgrade...) + requireBinaryKeys(t, db) + + requireUnusable(t, s, alice) + + for family, unusable := range lookAlikes { + unusable() + require.ErrorIs(t, s.SaveTokenPair(t.Context(), tokenPair("new-"+family, family)), oauth2.ErrTokenFamilyRevoked, + "an issuance into %q, a look-alike of a revoked family", family) + unusable() + requireRevokedRecord(t, db, family, start) + } + + requireUsable(t, s, bob) + requireUnusable(t, s, bobAlike) + requireRevokedRecord(t, db, "team-bob", start) + requireUsable(t, s, plain) + + _, err = s.ConsumeAuthorizationCode(t.Context(), code.CodeHash) + require.NoError(t, err) +} + +// requireRevokedRecord asserts that the upgrade recorded family revoked — +// no earlier than start — and kept for oauth2.FamilyRevocationRetention +// at least: past every one of its tokens, and past start + 24 hours even +// for a family whose tokens expire sooner (an access token alone lives an +// hour here). +func requireRevokedRecord(t *testing.T, db *sql.DB, family string, start int64) { + t.Helper() + + var revokedAt, expiresAt int64 + require.NoError(t, db.QueryRowContext(t.Context(), + `SELECT revoked_at, expires_at FROM oauth2_token_families WHERE family_id = ?`, family).Scan(&revokedAt, &expiresAt), family) + + require.GreaterOrEqual(t, revokedAt, start, "%s revoked at the upgrade", family) + require.GreaterOrEqual(t, expiresAt, start+int64(oauth2.FamilyRevocationRetention/time.Second), "%s retained", family) +} + +// upgradeRefusesOverLongKeys seeds, behind the store's back, a token row +// whose family ID fits VARCHAR(64) — 33 characters — but not VARBINARY(64): +// 66 bytes. The preflight reports it, and the upgrade, which sets a strict +// sql_mode for its session, stops at the ALTER of that table instead of +// truncating the key, on a connection whose own sql_mode is not strict. +func upgradeRefusesOverLongKeys(t *testing.T, b backend, preflight, upgrade []string) { + t.Helper() + + lax := b + lax.params = map[string]string{"sql_mode": "''"} + + driver, dsn := lax.namespace(t) + db := open(t, driver, dsn, 1) + execAll(t, db, legacyMySQLSchema...) + + long := strings.Repeat("\u00e9", 33) + execAll(t, db, `INSERT INTO oauth2_access_tokens (token_hash, client_id, subject, scope, family_id, audience, issued_at, expires_at) + VALUES ('at-long', 'client-1', 'alice', 'read', '`+long+`', 'api', 0, 4102444800)`) + + require.Equal(t, int64(66), queryRow(t, db, preflight[1])[1], "the preflight reports the 66-byte family ID") + + // One session: the SET applies to the statements after it. + conn, err := db.Conn(t.Context()) + require.NoError(t, err) + + var failed error + + for _, stmt := range upgrade { + if _, failed = conn.ExecContext(t.Context(), stmt); failed != nil { + require.Contains(t, stmt, "ALTER TABLE oauth2_access_tokens", "the statement that fails") + + break + } + } + + require.NoError(t, conn.Close()) + require.Error(t, failed, "the upgrade truncated a 66-byte family ID into VARBINARY(64)") + + var familyID, dataType string + require.NoError(t, db.QueryRowContext(t.Context(), `SELECT family_id FROM oauth2_access_tokens WHERE token_hash = 'at-long'`).Scan(&familyID)) + require.Equal(t, long, familyID, "the key is intact") + require.NoError(t, db.QueryRowContext(t.Context(), `SELECT DATA_TYPE FROM information_schema.COLUMNS + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'oauth2_access_tokens' AND COLUMN_NAME = 'family_id'`).Scan(&dataType)) + require.Equal(t, "varchar", dataType, "the table is left as it was") +} + +// requireRefreshConsumed asserts that pair's refresh token reads consumed. +func requireRefreshConsumed(t *testing.T, s *oauth2sql.Store, pair *oauth2.TokenPair) { + t.Helper() + + rt, err := s.LookupRefreshToken(t.Context(), pair.Refresh.TokenHash) + require.NoError(t, err, pair.Refresh.TokenHash) + require.True(t, rt.Consumed, pair.Refresh.TokenHash) +} + +// requireAccessRevoked asserts that pair's access token reads revoked. +func requireAccessRevoked(t *testing.T, s *oauth2sql.Store, pair *oauth2.TokenPair) { + t.Helper() + + _, err := s.LookupAccessToken(t.Context(), pair.Access.TokenHash) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked, pair.Access.TokenHash) +} + +// requireUsable asserts that pair's access token is found and its refresh +// token is not consumed. +func requireUsable(t *testing.T, s *oauth2sql.Store, pair *oauth2.TokenPair) { + t.Helper() + + _, err := s.LookupAccessToken(t.Context(), pair.Access.TokenHash) + require.NoError(t, err, pair.Access.TokenHash) + + rt, err := s.LookupRefreshToken(t.Context(), pair.Refresh.TokenHash) + require.NoError(t, err, pair.Refresh.TokenHash) + require.False(t, rt.Consumed, pair.Refresh.TokenHash) +} + +// requireUnusable asserts that pair's access token reads revoked and its +// refresh token consumed. +func requireUnusable(t *testing.T, s *oauth2sql.Store, pair *oauth2.TokenPair) { + t.Helper() + + _, err := s.LookupAccessToken(t.Context(), pair.Access.TokenHash) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked, pair.Access.TokenHash) + + rt, err := s.LookupRefreshToken(t.Context(), pair.Refresh.TokenHash) + require.NoError(t, err, pair.Refresh.TokenHash) + require.True(t, rt.Consumed, pair.Refresh.TokenHash) +} diff --git a/oauth2/storage.go b/oauth2/storage.go index a70b6e57..b2db50a1 100644 --- a/oauth2/storage.go +++ b/oauth2/storage.go @@ -19,7 +19,7 @@ const FamilyRevocationRetention = 24 * time.Hour // [Storage] accepts: [TokenPair.Validate] and every RevokeRefreshFamily // implementation refuse a longer one with invalid_request, so no backend // can silently truncate two identifiers into the same family (the SQL -// store keeps them in a VARCHAR(64) column). +// store keeps them in a VARCHAR(64) column, VARBINARY(64) on MySQL). const MaxFamilyIDLength = 64 // AuthorizationCodeStore persists single-use authorization codes (RFC 6749 @@ -51,7 +51,10 @@ type AuthorizationCodeStore interface { // issued: it MUST NOT be used to issue tokens; // - [ErrCodeAlreadyUsed] when the code is unknown: never saved, or // no longer retained; - // - a wrapped error on a backend failure. + // - a wrapped error on a backend failure — whose outcome may be + // unknown: when the acknowledgement of the commit is lost, the code + // may be consumed all the same, and a retried exchange is then a + // replay. // // Both refusals match [ErrCodeAlreadyUsed] (invalid_grant). A decorator // or wrapper MUST return the store's error unchanged or wrapped with @@ -151,9 +154,16 @@ type RefreshTokenStore interface { // a consumed token) without persisting anything. A lookup concurrent // with R may still see F active: it is ordered before R. // - I3, atomic writes: SaveTokenPair and RotateRefreshToken apply all -// their writes or none. A failed call persists no token and consumes -// nothing; the one exception is reuse, which only records F as -// revoked. +// their writes or none, never a part of them. A call refused by the +// contract — invalid_request, invalid_grant, [ErrTokenFamilyRevoked] — +// persists no token and consumes nothing; the one exception is reuse, +// which only records F as revoked. A backend failure can leave the +// outcome unknown: when the acknowledgement of the commit is lost (the +// connection drops, or the context ends while the backend commits), +// the call fails although all its writes may have been applied. Callers +// never reuse the pair they passed — every call carries a freshly +// generated one — and a client retrying a refresh after such a failure +// presents a consumed token: a reuse, which revokes F. // - I4, reuse: RotateRefreshToken on a consumed token of a non-revoked // family fails with [ErrRefreshTokenReused]; one of an already revoked // family may fail with [ErrTokenFamilyRevoked] instead (both are diff --git a/oauth2/store/redis/doc.go b/oauth2/store/redis/doc.go index 00eefb18..de438f7f 100644 --- a/oauth2/store/redis/doc.go +++ b/oauth2/store/redis/doc.go @@ -51,7 +51,10 @@ // its retention, is recorded active again by the next issuance into // the family, reviving its tokens. An evicted code key reads as an // unknown code: a replay of it is still refused, but no longer -// recognized as one. +// recognized as one. An evicted refresh-token key — a consumed one +// included, within its lifetime — reads as an unknown token: a reuse +// of a rotated token is refused but no longer detected, and its family +// is not revoked. // // go-redis resends a command whose reply was lost; every script is safe to // run twice: a resent rotation is not mistaken for a reuse, nor a resent diff --git a/oauth2/store/sql/dialect.go b/oauth2/store/sql/dialect.go index 2b3ba9e6..76a6ea4b 100644 --- a/oauth2/store/sql/dialect.go +++ b/oauth2/store/sql/dialect.go @@ -11,10 +11,10 @@ import ( ) // Dialect abstracts the few SQL differences the store cares about: the -// parameter-placeholder style, the insert-if-absent syntax, row locking and -// the transaction isolation level. The store writes every query with "?" -// placeholders and rebinds them through the dialect, so query strings stay -// readable. +// parameter-placeholder style, the insert-if-absent syntax, row locking, +// the transaction isolation level and the serialization of concurrent +// migrations. The store writes every query with "?" placeholders and +// rebinds them through the dialect, so query strings stay readable. type Dialect interface { // Name returns a stable identifier ("postgres", "mysql", "sqlite") // used in error messages and OTel attributes. @@ -32,8 +32,25 @@ type Dialect interface { lockRefresh() string // txOptions are the options of every store transaction. txOptions() *sql.TxOptions + // migrationLock is the statement [Store.Migrate] runs first, in the + // transaction creating the tables, to serialize concurrent migrations; + // "" when the engine serializes the creation of a table itself and + // Migrate runs its statements one by one. + migrationLock() string + // keyType is the column type [Schema] gives a key of at most length + // 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 } +// migrationLockKey is the PostgreSQL advisory lock [Store.Migrate] takes: +// the first eight bytes of SHA-256 of +// "github.com/hyperscale-stack/security/oauth2/store/sql.Migrate", +// big-endian, as a bigint. Advisory locks are local to a database, which +// the stores of other modules may share: each store module has a key of +// its own, so that their migrations never wait for one another. +const migrationLockKey = "-2450120788798466590" + // The token-family statements several dialects share, each a full literal. const ( insertFamilyOnConflict = `INSERT INTO oauth2_token_families (family_id, revoked_at, expires_at) VALUES (?, ?, ?) @@ -47,11 +64,14 @@ const ( // transactions run at READ COMMITTED whatever the server default: the // family lock and the consumption of a code rely on re-reading the latest // committed row, where REPEATABLE READ or SERIALIZABLE would abort the -// transaction with a serialization failure instead. +// transaction with a serialization failure instead. [Store.Migrate] takes +// the transaction-level advisory lock -2450120788798466590 +// (pg_advisory_xact_lock) before creating the tables. var Postgres Dialect = postgres{} // MySQL is the MySQL / MariaDB dialect (? placeholders). The tables must be -// InnoDB: the store relies on its row locks. +// InnoDB: the store relies on its row locks. Its key columns are +// VARBINARY (see [Schema]). var MySQL Dialect = mysql{} // SQLite is the SQLite dialect (? placeholders). SQLite has no row locks: @@ -95,6 +115,22 @@ func (postgres) txOptions() *sql.TxOptions { return &sql.TxOptions{Isolation: sql.LevelReadCommitted} } +// migrationLock takes a transaction-level advisory lock: concurrent +// CREATE TABLE IF NOT EXISTS of one table can both find it missing, and +// the loser then fails on the unique index of the catalog's row types. A +// transaction-level lock is released by the COMMIT or ROLLBACK ending the +// migration, whatever becomes of the connection afterwards. +func (postgres) migrationLock() string { + return "SELECT pg_advisory_xact_lock(" + migrationLockKey + ")" +} + +// keyType is VARCHAR: PostgreSQL compares text byte for byte under a +// deterministic collation, which a database's default collation is. A +// VARCHAR(n) silently drops the spaces past n characters, which no key +// reaches: the store refuses a family ID over 64 bytes, and oauth2.HashToken +// hashes are 64 characters. +func (postgres) keyType(length int) string { return varchar(length) } + type mysql struct{} func (mysql) Name() string { return "mysql" } @@ -107,11 +143,21 @@ func (mysql) insertFamily() string { ON DUPLICATE KEY UPDATE family_id = family_id` } +// keyType is VARBINARY: MySQL and MariaDB compare text under the column's +// collation — case- and accent-insensitive by default, trailing spaces +// ignored by every PAD SPACE one, utf8mb4_bin included — and binary +// strings byte for byte. +func (mysql) keyType(length int) string { return "VARBINARY(" + strconv.Itoa(length) + ")" } + func (mysql) lockFamily() string { return lockFamilyForUpdate } func (mysql) lockRefresh() string { return lockRefreshForUpdate } 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. +func (mysql) migrationLock() string { return "" } + // dialectSQLite is the dialect identifier for SQLite. const dialectSQLite = "sqlite" @@ -122,6 +168,13 @@ func (sqlite) rebind(q string) string { return q } func (sqlite) insertFamily() string { return insertFamilyOnConflict } +// keyType is VARCHAR: SQLite compares text byte for byte under its default +// BINARY collation. +func (sqlite) keyType(length int) string { return varchar(length) } + +// varchar is the VARCHAR(length) column type. +func varchar(length int) string { return "VARCHAR(" + strconv.Itoa(length) + ")" } + // lockFamily and lockRefresh are plain reads: SQLite has no FOR UPDATE, // and the write that opens every store transaction already holds the // database's single write lock. @@ -135,3 +188,8 @@ func (sqlite) lockRefresh() string { } func (sqlite) txOptions() *sql.TxOptions { return nil } + +// migrationLock is empty: SQLite's single writer serializes the creation +// of the tables, and a statement prepared before a concurrent change of the +// schema is prepared again. +func (sqlite) migrationLock() string { return "" } diff --git a/oauth2/store/sql/dialect_family_internal_test.go b/oauth2/store/sql/dialect_family_internal_test.go index f8241cb0..ff4efb93 100644 --- a/oauth2/store/sql/dialect_family_internal_test.go +++ b/oauth2/store/sql/dialect_family_internal_test.go @@ -67,3 +67,15 @@ func TestDialectFamilyStatements(t *testing.T) { FROM oauth2_refresh_tokens WHERE token_hash = $1 FOR UPDATE`, postgres{}.rebind(postgres{}.lockRefresh())) } + +// TestDialectKeyType: the key columns are binary strings on MySQL, which +// compares them byte for byte whatever the table's collation, and VARCHAR +// elsewhere — PostgreSQL and SQLite compare those byte for byte already. +func TestDialectKeyType(t *testing.T) { + t.Parallel() + + assert.Equal(t, "VARCHAR(64)", postgres{}.keyType(64)) + assert.Equal(t, "VARCHAR(128)", sqlite{}.keyType(128)) + assert.Equal(t, "VARBINARY(64)", mysql{}.keyType(64)) + assert.Equal(t, "VARBINARY(128)", mysql{}.keyType(128)) +} diff --git a/oauth2/store/sql/doc.go b/oauth2/store/sql/doc.go index 7ffb3b26..8f23a14e 100644 --- a/oauth2/store/sql/doc.go +++ b/oauth2/store/sql/doc.go @@ -28,17 +28,32 @@ // error fails the operation, closed. // - Lookups must reach the primary database: a replica DSN or a // read/write-splitting proxy could answer from before a revocation. -// - Text is stored as UTF-8 and compared exactly: a PostgreSQL database -// in the UTF8 encoding; MySQL tables in utf8mb4 with a strict sql_mode, -// since a non-strict MySQL truncates an over-long value — a subject, an -// extension's data — instead of refusing it. +// - Keys are compared byte for byte. The statements compare the code and +// token hashes and the token-family IDs, and no other column. MySQL +// and MariaDB compare text under the column's collation — +// case- and accent-insensitive by default, trailing spaces ignored by +// every PAD SPACE one, utf8mb4_bin included — so [Schema] declares +// these columns VARBINARY there; a database created before needs the +// statements MIGRATION.md lists. The interactive mysql client shows +// them in hexadecimal (--binary-as-hex, on by default in interactive +// mode since 8.0.19): read them with CAST(family_id AS CHAR). PostgreSQL +// compares them byte for byte under the database's default collation, +// which is deterministic, and SQLite under its BINARY collation. +// - Text is stored as UTF-8: a PostgreSQL database in the UTF8 encoding; +// MySQL tables in utf8mb4 with a strict sql_mode, since a non-strict +// MySQL truncates an over-long value — a subject, an extension's data — +// instead of refusing it. // -// The CI of this module exercises SQLite only: the PostgreSQL and MySQL -// guarantees rest on the locking described in [Store]. +// The CI of this module exercises SQLite only. The PostgreSQL and MySQL +// guarantees rest on the locking described in [Store]; the opt-in suite of +// internal/interop/sqlstores runs the conformance suite, concurrent +// Migrate and the MySQL upgrade on PostgreSQL, MySQL and MariaDB servers +// (see its compose.yaml). // // # Upgrading // -// Migrate only creates the tables that are missing. When a release adds +// Migrate only creates the tables that are missing; every instance may run +// it at boot, concurrently (see [Store.Migrate]). When a release adds // columns to existing tables — the issuer and extension_data columns of // codes and tokens, or the redirect_uri_omitted flag of codes, for // instance — MIGRATION.md lists the ALTER TABLE statements to apply, @@ -66,7 +81,10 @@ // Never delete a family row before its expires_at. A missing row fails // closed for lookups — the store treats the tokens of an unknown family as // revoked — but the next issuance into a family whose revoked row is gone -// records the family active again, reviving its tokens. +// records the family active again, reviving its tokens. Nor a token row: +// a consumed refresh token deleted early reads as an unknown token when +// presented again, so its reuse is refused but no longer detected, and +// its family is not revoked. // // Allowed dependencies: // - github.com/hyperscale-stack/security/oauth2 diff --git a/oauth2/store/sql/migrate_internal_test.go b/oauth2/store/sql/migrate_internal_test.go new file mode 100644 index 00000000..8954d65f --- /dev/null +++ b/oauth2/store/sql/migrate_internal_test.go @@ -0,0 +1,194 @@ +// 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" + "crypto/sha256" + "database/sql" + "encoding/binary" + "path/filepath" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + _ "modernc.org/sqlite" +) + +// schemaTables are the tables of Schema, in the order it creates them. +var schemaTables = []string{"oauth2_auth_codes", "oauth2_access_tokens", "oauth2_refresh_tokens", "oauth2_token_families"} + +// TestMigrationLock pins the statement Migrate runs first on each engine: +// on PostgreSQL, the transaction-level advisory lock the package +// documentation names — the first eight bytes of SHA-256 of +// "github.com/hyperscale-stack/security/oauth2/store/sql.Migrate", +// big-endian, as a bigint — and none on MySQL and SQLite, which serialize +// the creation of a table themselves. +func TestMigrationLock(t *testing.T) { + t.Parallel() + + sum := sha256.Sum256([]byte("github.com/hyperscale-stack/security/oauth2/store/sql.Migrate")) + key := strconv.FormatInt(int64(binary.BigEndian.Uint64(sum[:8])), 10) + + assert.Equal(t, "-2450120788798466590", key) + assert.Equal(t, "SELECT pg_advisory_xact_lock("+key+")", Postgres.migrationLock()) + assert.Empty(t, MySQL.migrationLock()) + assert.Empty(t, SQLite.migrationLock()) +} + +// lockingStore returns a store over a new SQLite database file whose +// dialect — a mock — names lock as its migration lock, so that Migrate +// takes the path it takes on PostgreSQL: lock first, then the schema, in +// one transaction. The pool has several connections, so that a statement +// run outside the transaction waits for its write lock — and fails after +// the busy timeout — instead of deadlocking a single connection. +func lockingStore(t *testing.T, lock string) (*Store, *sql.DB) { + t.Helper() + + dsn := "file:" + filepath.Join(t.TempDir(), "migrate.db") + "?_pragma=busy_timeout(1000)&_pragma=foreign_keys(1)" + + db, err := sql.Open("sqlite", dsn) + require.NoError(t, err) + db.SetMaxOpenConns(4) + t.Cleanup(func() { _ = db.Close() }) + + d := NewMockDialect(t) + d.EXPECT().migrationLock().Return(lock) + d.EXPECT().keyType(mock.Anything).RunAndReturn(sqlite{}.keyType).Maybe() + + return &Store{db: db, dialect: d}, db +} + +// execAll runs every statement on db. +func execAll(t *testing.T, db *sql.DB, stmts ...string) { + t.Helper() + + for _, stmt := range stmts { + _, err := db.ExecContext(t.Context(), stmt) + require.NoError(t, err, stmt) + } +} + +// tables lists the tables of db in the order they were created. +func tables(t *testing.T, db *sql.DB) []string { + t.Helper() + + rows, err := db.QueryContext(t.Context(), `SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY rowid`) + require.NoError(t, err) + + defer func() { _ = rows.Close() }() + + var names []string + + for rows.Next() { + var name string + require.NoError(t, rows.Scan(&name)) + + names = append(names, name) + } + + require.NoError(t, rows.Err()) + + return names +} + +// TestMigrateTakesTheLockFirst runs the locked path: the lock statement — +// here one creating a probe table — runs before the schema, and the +// transaction commits. +func TestMigrateTakesTheLockFirst(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `CREATE TABLE migration_lock_probe (taken INTEGER)`) + + require.NoError(t, store.Migrate(t.Context())) + assert.Equal(t, append([]string{"migration_lock_probe"}, schemaTables...), tables(t, db)) +} + +// TestMigrateFailedLockCreatesNothing: a lock that cannot be taken stops +// Migrate before any table is created. +func TestMigrateFailedLockCreatesNothing(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `SELECT no_such_function()`) + + err := store.Migrate(t.Context()) + require.ErrorContains(t, err, "sqlstore: migrate: ") + require.ErrorContains(t, err, "no_such_function") + assert.Empty(t, tables(t, db)) +} + +// TestMigrateRollsBackAFailedSchema: when a statement of the schema fails, +// the whole transaction is rolled back — the lock statement's probe table +// and the tables created before it included. +func TestMigrateRollsBackAFailedSchema(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `CREATE TABLE migration_lock_probe (taken INTEGER)`) + + // An index named like the last table makes its CREATE TABLE fail. + last := schemaTables[len(schemaTables)-1] + execAll(t, db, `CREATE TABLE blocker (x INTEGER)`, `CREATE INDEX `+last+` ON blocker (x)`) + + err := store.Migrate(t.Context()) + require.ErrorContains(t, err, "sqlstore: migrate: ") + require.ErrorContains(t, err, "already an index named "+last) + assert.Equal(t, []string{"blocker"}, tables(t, db)) +} + +// TestMigrateReleasesAFailedTransaction: a Migrate that fails under a +// context that never ends — the usual call at boot — still ends its +// transaction and gives its connection back. Left open, the transaction +// would keep the connection checked out (on PostgreSQL, "idle in +// transaction (aborted)") for the life of the pool, one more at every +// retry. On a one-connection pool, the next Migrate, once the cause is +// gone, goes through. +func TestMigrateReleasesAFailedTransaction(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `CREATE TABLE migration_lock_probe (taken INTEGER)`) + db.SetMaxOpenConns(1) + + last := schemaTables[len(schemaTables)-1] + execAll(t, db, `CREATE TABLE blocker (x INTEGER)`, `CREATE INDEX `+last+` ON blocker (x)`) + + require.Error(t, store.Migrate(context.Background())) + require.Zero(t, db.Stats().InUse, "the failed Migrate gave its connection back") + + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + + _, err := db.ExecContext(ctx, `DROP INDEX `+last) + require.NoError(t, err) + require.NoError(t, store.Migrate(ctx)) +} + +// TestMigrateReportsAFailedCommit: a COMMIT that fails — here on a deferred +// foreign key the lock statement violates — fails Migrate. +func TestMigrateReportsAFailedCommit(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `INSERT INTO child (parent_id) VALUES (42)`) + execAll(t, db, + `CREATE TABLE parent (id INTEGER PRIMARY KEY)`, + `CREATE TABLE child (parent_id INTEGER REFERENCES parent (id) DEFERRABLE INITIALLY DEFERRED)`) + + err := store.Migrate(t.Context()) + require.ErrorContains(t, err, "sqlstore: migrate: commit: ") + require.ErrorContains(t, err, "FOREIGN KEY constraint failed") +} + +// TestMigrateReportsAFailedBegin: a transaction that cannot begin fails +// Migrate. +func TestMigrateReportsAFailedBegin(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `SELECT 1`) + require.NoError(t, db.Close()) + + require.ErrorContains(t, store.Migrate(t.Context()), "sqlstore: migrate: begin: ") +} diff --git a/oauth2/store/sql/schema.go b/oauth2/store/sql/schema.go index c19aa9e4..4712a7c5 100644 --- a/oauth2/store/sql/schema.go +++ b/oauth2/store/sql/schema.go @@ -19,11 +19,19 @@ import ( // SMALLINT 0/1 since PostgreSQL has no implicit integer/boolean cast. Token // / code raw values are NEVER stored — only their hashes. // -// The DDL sticks to the subset every supported engine accepts, so it is -// the same for every [Dialect]: the argument is accepted for API stability -// and ignored. The 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 it — are nullable TEXT, the only form every engine accepts +// The DDL sticks to the subset every supported engine accepts, and is the +// same for every [Dialect] but for the type of the key columns — the code +// 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. +// +// 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 +// it — are nullable TEXT, the only form every engine accepts // in an ALTER TABLE … ADD COLUMN on a populated table, so a database // created before them is upgraded with the statements MIGRATION.md lists // and ends up with the same columns as a new one. The store writes an @@ -33,13 +41,22 @@ import ( // recorded family start), which every engine also accepts on a populated // table: a row written before them reads 0. // -// The statements are idempotent (CREATE TABLE IF NOT EXISTS). Production -// deployments typically run them through a migration tool rather than via -// [Store.Migrate], but Migrate is offered for tests and small setups. -func Schema(_ Dialect) []string { +// The statements are idempotent (CREATE TABLE IF NOT EXISTS), and each is +// valid inside a PostgreSQL transaction block, where [Store.Migrate] runs +// them. Production deployments typically run them through a migration tool +// rather than via Migrate, but Migrate is offered for tests and small +// setups. +func Schema(d Dialect) []string { + if d == nil { + d = sqlite{} + } + + // 64: oauth2.MaxFamilyIDLength, in bytes. + hash, family := d.keyType(128), d.keyType(64) + return []string{ `CREATE TABLE IF NOT EXISTS oauth2_auth_codes ( - code_hash VARCHAR(128) PRIMARY KEY, + code_hash ` + hash + ` PRIMARY KEY, client_id VARCHAR(255) NOT NULL, subject VARCHAR(255) NOT NULL, redirect_uri TEXT NOT NULL, @@ -55,11 +72,11 @@ func Schema(_ Dialect) []string { redirect_uri_omitted SMALLINT NOT NULL DEFAULT 0 )`, `CREATE TABLE IF NOT EXISTS oauth2_access_tokens ( - token_hash VARCHAR(128) PRIMARY KEY, + token_hash ` + hash + ` PRIMARY KEY, client_id VARCHAR(255) NOT NULL, subject VARCHAR(255) NOT NULL, scope TEXT NOT NULL, - family_id VARCHAR(64) NOT NULL, + family_id ` + family + ` NOT NULL, audience VARCHAR(255) NOT NULL, issued_at BIGINT NOT NULL, expires_at BIGINT NOT NULL, @@ -67,11 +84,11 @@ func Schema(_ Dialect) []string { extension_data TEXT )`, `CREATE TABLE IF NOT EXISTS oauth2_refresh_tokens ( - token_hash VARCHAR(128) PRIMARY KEY, + token_hash ` + hash + ` PRIMARY KEY, client_id VARCHAR(255) NOT NULL, subject VARCHAR(255) NOT NULL, scope TEXT NOT NULL, - family_id VARCHAR(64) NOT NULL, + family_id ` + family + ` NOT NULL, consumed SMALLINT NOT NULL DEFAULT 0, issued_at BIGINT NOT NULL, expires_at BIGINT NOT NULL, @@ -80,19 +97,38 @@ func Schema(_ Dialect) []string { family_issued_at BIGINT NOT NULL DEFAULT 0 )`, `CREATE TABLE IF NOT EXISTS oauth2_token_families ( - family_id VARCHAR(64) PRIMARY KEY, + family_id ` + family + ` PRIMARY KEY, revoked_at BIGINT NOT NULL DEFAULT 0, expires_at BIGINT NOT NULL )`, } } -// Migrate applies [Schema] to the store's database. It is safe to call -// repeatedly (every statement is IF NOT EXISTS): it creates the missing +// 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. +// +// It is safe to call repeatedly, and concurrently — every instance of a +// deployment starting at once. On PostgreSQL, where concurrent CREATE +// TABLE IF NOT EXISTS of one table can fail, Migrate runs in one +// transaction that first takes the transaction-level advisory lock +// -2450120788798466590 (pg_advisory_xact_lock; see [Postgres]): concurrent +// calls run one after the other, and a failed call creates nothing. A +// waiting call waits as long as ctx — and the server's lock_timeout or +// statement_timeout — allows: pass a deadline; the driver cancels the +// 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. func (s *Store) Migrate(ctx context.Context) error { + if lock := s.dialect.migrationLock(); lock != "" { + return s.migrateLocked(ctx, lock) + } + for _, stmt := range Schema(s.dialect) { if _, err := s.db.ExecContext(ctx, stmt); err != nil { return fmt.Errorf("sqlstore: migrate: %w", err) @@ -101,3 +137,27 @@ func (s *Store) Migrate(ctx context.Context) error { return nil } + +// migrateLocked applies [Schema] in one transaction that first runs lock, +// the dialect's migration lock: the transaction holds the lock until it +// commits the tables, or rolls everything back. +func (s *Store) migrateLocked(ctx context.Context, lock string) error { + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("sqlstore: migrate: begin: %w", err) + } + + defer func() { _ = tx.Rollback() }() // a no-op once committed + + for _, stmt := range append([]string{lock}, Schema(s.dialect)...) { + if _, err := tx.ExecContext(ctx, stmt); err != nil { + return fmt.Errorf("sqlstore: migrate: %w", err) + } + } + + if err := tx.Commit(); err != nil { + return fmt.Errorf("sqlstore: migrate: commit: %w", err) + } + + return nil +} diff --git a/oauth2/store/sql/schema_test.go b/oauth2/store/sql/schema_test.go index bf0721a2..b3a9e680 100644 --- a/oauth2/store/sql/schema_test.go +++ b/oauth2/store/sql/schema_test.go @@ -10,25 +10,50 @@ import ( sqlstore "github.com/hyperscale-stack/security/oauth2/store/sql" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -// TestSchemaIsPortable pins a DDL every supported engine accepts: -// PostgreSQL has no implicit integer/boolean cast (BOOLEAN DEFAULT 0 and -// consumed = 1 are rejected) and MySQL has no CREATE INDEX IF NOT EXISTS, -// so the schema sticks to the portable subset and is the same everywhere. -func TestSchemaIsPortable(t *testing.T) { +// TestSchemaPerDialect pins the DDL of each engine. PostgreSQL and SQLite — +// and a nil Dialect — get one portable DDL: no BOOLEAN (PostgreSQL has no +// implicit integer/boolean cast, so BOOLEAN DEFAULT 0 and consumed = 1 are +// rejected) and no CREATE INDEX (MySQL has no CREATE INDEX IF NOT EXISTS). +// MySQL gets the same DDL but for its six key columns — the code and token +// 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. +func TestSchemaPerDialect(t *testing.T) { t.Parallel() - want := sqlstore.Schema(sqlstore.SQLite) + portable := sqlstore.Schema(sqlstore.SQLite) + assert.Equal(t, portable, sqlstore.Schema(sqlstore.Postgres)) + assert.Equal(t, portable, sqlstore.Schema(nil)) - for _, d := range []sqlstore.Dialect{sqlstore.Postgres, sqlstore.MySQL} { - assert.Equal(t, want, sqlstore.Schema(d), d.Name()) - } - - for _, stmt := range want { + for _, stmt := range portable { assert.NotContains(t, strings.ToUpper(stmt), "BOOLEAN", stmt) assert.NotContains(t, strings.ToUpper(stmt), "CREATE INDEX", stmt) + assert.NotContains(t, stmt, "VARBINARY", stmt) } - assert.Contains(t, strings.Join(want, "\n"), "consumed SMALLINT") + assert.Contains(t, strings.Join(portable, "\n"), "consumed SMALLINT") + + mysql := sqlstore.Schema(sqlstore.MySQL) + 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") + } + + ddl := strings.Join(mysql, "\n") + + assert.Equal(t, 6, strings.Count(ddl, "VARBINARY(")) + + for column, n := range map[string]int{ + "code_hash VARBINARY(128) PRIMARY KEY": 1, + "token_hash VARBINARY(128) PRIMARY KEY": 2, + "family_id VARBINARY(64) NOT NULL": 2, + "family_id VARBINARY(64) PRIMARY KEY": 1, + } { + assert.Equal(t, n, strings.Count(ddl, column), column) + } } diff --git a/oauth2/store/sql/store_error_test.go b/oauth2/store/sql/store_error_test.go index 3973095f..c05bee9e 100644 --- a/oauth2/store/sql/store_error_test.go +++ b/oauth2/store/sql/store_error_test.go @@ -114,6 +114,9 @@ func TestSQLStoreReportsErrorsOnClosedDB(t *testing.T) { _, err = store.ConsumeAuthorizationCode(ctx, "h") require.Error(t, err) + // Migrate runs its statements one by one on SQLite: the first fails. + require.ErrorContains(t, store.Migrate(ctx), "sqlstore: migrate: ") + require.Error(t, store.SaveTokenPair(ctx, testPair("h", "f", time.Minute))) require.Error(t, store.RotateRefreshToken(ctx, "h", testPair("next", "f", time.Minute))) require.ErrorIs(t, store.RevokeRefreshFamily(ctx, "f"), oauth2.ErrTokenFamilyRevocationFailed) diff --git a/oauth2/store/sql/store_lostack_test.go b/oauth2/store/sql/store_lostack_test.go new file mode 100644 index 00000000..faa5c432 --- /dev/null +++ b/oauth2/store/sql/store_lostack_test.go @@ -0,0 +1,134 @@ +// 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" + "database/sql/driver" + "errors" + "sync/atomic" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" + sqlstore "github.com/hyperscale-stack/security/oauth2/store/sql" + "github.com/stretchr/testify/require" + _ "modernc.org/sqlite" +) + +// errLostAcknowledgement is what a COMMIT whose acknowledgement was lost +// reports: the transaction is committed, the caller cannot tell. +var errLostAcknowledgement = errors.New("connection lost after COMMIT") + +// lostAckConnector serves connections of the SQLite driver whose next +// COMMIT, once lose is set, commits and then reports +// errLostAcknowledgement. +type lostAckConnector struct { + drv driver.Driver + dsn string + lose *atomic.Bool +} + +func (c lostAckConnector) Connect(context.Context) (driver.Conn, error) { + conn, err := c.drv.Open(c.dsn) + if err != nil { + return nil, err + } + + return lostAckConn{Conn: conn, lose: c.lose}, nil +} + +func (c lostAckConnector) Driver() driver.Driver { return c.drv } + +// lostAckConn wraps the transactions of its connection. +type lostAckConn struct { + driver.Conn + lose *atomic.Bool +} + +func (c lostAckConn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, error) { + beginner, ok := c.Conn.(driver.ConnBeginTx) + if !ok { + return nil, errors.New("lostack: the driver has no BeginTx") + } + + tx, err := beginner.BeginTx(ctx, opts) + if err != nil { + return nil, err + } + + return lostAckTx{Tx: tx, lose: c.lose}, nil +} + +// lostAckTx commits, then loses the acknowledgement when lose is set. +type lostAckTx struct { + driver.Tx + lose *atomic.Bool +} + +func (t lostAckTx) Commit() error { + if err := t.Tx.Commit(); err != nil { + return err + } + + if t.lose.CompareAndSwap(true, false) { + return errLostAcknowledgement + } + + return nil +} + +// TestRotationWithALostCommitAcknowledgement pins what the I3 wording of +// oauth2.Storage states: a rotation whose COMMIT succeeded but whose +// acknowledgement was lost fails although every write was applied — an +// unknown outcome is never a partial one — and the client's retry of the +// refresh, which presents the same refresh token, is then a reuse: it +// revokes the family, the replacement issued by the "failed" rotation +// included. +func TestRotationWithALostCommitAcknowledgement(t *testing.T) { + t.Parallel() + + base, err := sql.Open("sqlite", ":memory:") + require.NoError(t, err) + + lose := new(atomic.Bool) + db := sql.OpenDB(lostAckConnector{drv: base.Driver(), dsn: ":memory:", lose: lose}) + db.SetMaxOpenConns(1) + + t.Cleanup(func() { _ = db.Close(); _ = base.Close() }) + + store, err := sqlstore.New(db, sqlstore.SQLite) + require.NoError(t, err) + require.NoError(t, store.Migrate(t.Context())) + + ctx := t.Context() + first := testPair("lost-0", "fam-lost", time.Hour) + require.NoError(t, store.SaveTokenPair(ctx, first)) + + next := testPair("lost-1", "fam-lost", time.Hour) + + lose.Store(true) + require.ErrorIs(t, store.RotateRefreshToken(ctx, first.Refresh.TokenHash, next), errLostAcknowledgement) + + // Applied whole: the presented token consumed, its replacement stored. + rt, err := store.LookupRefreshToken(ctx, first.Refresh.TokenHash) + require.NoError(t, err) + require.True(t, rt.Consumed) + + _, err = store.LookupAccessToken(ctx, next.Access.TokenHash) + require.NoError(t, err) + + rt, err = store.LookupRefreshToken(ctx, next.Refresh.TokenHash) + require.NoError(t, err) + require.False(t, rt.Consumed) + + // The client retries with the token it presented: a reuse. + retry := testPair("lost-2", "fam-lost", time.Hour) + require.ErrorIs(t, store.RotateRefreshToken(ctx, first.Refresh.TokenHash, retry), oauth2.ErrRefreshTokenReused) + + _, err = store.LookupAccessToken(ctx, next.Access.TokenHash) + require.ErrorIs(t, err, oauth2.ErrTokenFamilyRevoked) +} diff --git a/oauth2/storetest/conformance.go b/oauth2/storetest/conformance.go index 70c72b21..78c21178 100644 --- a/oauth2/storetest/conformance.go +++ b/oauth2/storetest/conformance.go @@ -37,7 +37,8 @@ type Factory func() oauth2.Storage // Besides single-use codes — retained once consumed, so a replay is told // from an unknown code — and atomic rotation, it checks the token-family // invariants the [oauth2.Storage] godoc states — visibility, gating, atomic -// writes, reuse, idempotence, isolation — sequentially and under races, +// 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 // it cannot observe stays with each store's own tests: an injected backend @@ -69,6 +70,7 @@ func RunConformance(t *testing.T, newStore Factory) { {"FamilyRevokeThenRotate", testFamilyRevokeThenRotate}, {"FamilyRevokeIsIdempotent", testFamilyRevokeIsIdempotent}, {"FamilyRevokeIsolation", testFamilyRevokeIsolation}, + {"KeysAreComparedExactly", testKeysAreComparedExactly}, {"FamilyRevokeRejectsEmptyID", testFamilyRevokeRejectsEmptyID}, {"FamilyIDLengthLimit", testFamilyIDLengthLimit}, {"FamilyFailedRotationPersistsNothing", testFamilyFailedRotationPersistsNothing}, diff --git a/oauth2/storetest/family.go b/oauth2/storetest/family.go index ef96eecb..38575dfd 100644 --- a/oauth2/storetest/family.go +++ b/oauth2/storetest/family.go @@ -268,6 +268,85 @@ func testFamilyRevokeIsolation(t *testing.T, s oauth2.Storage) { requireUsable(t, s, cc) } +// lookAlikeKeys are pairs of keys that a comparison under a +// case-insensitive, accent-insensitive or space-padding collation — the +// default ones of MySQL and MariaDB, and every PAD SPACE one, utf8mb4_bin +// included — takes for one key. They differ only in letter case, an +// accent, the Unicode form of a character (é precomposed, and e followed +// by a combining acute accent), an expansion (ß and ss) or a trailing +// space. +var lookAlikeKeys = [][2]string{ + {"Fam-Case-Az09", "fAM-cASE-aZ09"}, + {"fam-accent-e", "fam-accent-\u00e9"}, + {"fam-form-\u00e9", "fam-form-e\u0301"}, + {"fam-strasse", "fam-stra\u00dfe"}, + {"fam-space", "fam-space "}, +} + +// testKeysAreComparedExactly checks that a store tells keys apart byte for +// byte (I7). Of two look-alike family IDs, revoking one leaves the other +// usable, and a revocation recorded for one before any issuance does not +// refuse the first pair of the other. Tokens and codes whose hashes differ +// only in letter case, or by a trailing space, are records of their own, +// each consumed on its own. +func testKeysAreComparedExactly(t *testing.T, s oauth2.Storage) { + for i, ids := range lookAlikeKeys { + kept := newPair(fmt.Sprintf("alike-kept-%d", i), ids[0]) + revoked := newPair(fmt.Sprintf("alike-revoked-%d", i), ids[1]) + + mustSave(t, s, kept) + mustSave(t, s, revoked) + mustNoError(t, s.RevokeRefreshFamily(ctx(), ids[1]), fmt.Sprintf("revoke family %q", ids[1])) + requireUnusable(t, s, revoked) + requireUsable(t, s, kept) + + mustNoError(t, s.RevokeRefreshFamily(ctx(), "early-"+ids[0]), fmt.Sprintf("revoke family %q", "early-"+ids[0])) + + first := newPair(fmt.Sprintf("alike-first-%d", i), "early-"+ids[1]) + mustSave(t, s, first) + requireUsable(t, s, first) + } + + pairs := []*oauth2.TokenPair{ + newPair("Alike-Hash", "fam-alike-upper"), newPair("aLIKE-hASH", "fam-alike-lower"), + newPair("alike-pad", "fam-alike-pad"), newPair("alike-pad ", "fam-alike-padded"), + } + for _, pair := range pairs { + mustSave(t, s, pair) + } + + for _, pair := range pairs { + requireOwnFamily(t, s, pair) + } + + codes := []*oauth2.AuthorizationCode{ + sampleCode("Alike-Code"), sampleCode("aLIKE-cODE"), sampleCode("alike-pad"), sampleCode("alike-pad "), + } + for _, code := range codes { + mustNoError(t, s.SaveAuthorizationCode(ctx(), code), "save code "+code.CodeHash) + } + + for _, code := range codes { + got, err := s.ConsumeAuthorizationCode(ctx(), code.CodeHash) + mustNoError(t, err, "consume code "+code.CodeHash) + requireRecord(t, got, code, "consume code "+code.CodeHash) + } +} + +// requireOwnFamily asserts that the lookups of pair's tokens find its own +// records: those of the family it was saved in. +func requireOwnFamily(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.FamilyID, pair.Access.FamilyID, "family of access token "+pair.Access.TokenHash) + + rt, err := s.LookupRefreshToken(ctx(), pair.Refresh.TokenHash) + mustNoError(t, err, "lookup refresh token "+pair.Refresh.TokenHash) + mustEqual(t, rt.FamilyID, pair.Refresh.FamilyID, "family of refresh token "+pair.Refresh.TokenHash) +} + // testFamilyRevokeRejectsEmptyID refuses to revoke the empty family ID, // which would otherwise reach every token outside a family. func testFamilyRevokeRejectsEmptyID(t *testing.T, s oauth2.Storage) { diff --git a/oidc/client/store/sql/dialect.go b/oidc/client/store/sql/dialect.go index 9082f75c..0c5ce8fe 100644 --- a/oidc/client/store/sql/dialect.go +++ b/oidc/client/store/sql/dialect.go @@ -11,9 +11,10 @@ import ( ) // Dialect abstracts the few SQL differences the store cares about: the -// parameter-placeholder style, the locking read of Consume and the -// transaction isolation level. The store writes every query with "?" -// placeholders and rebinds them through the dialect. +// parameter-placeholder style, the locking read of Consume, the +// transaction isolation level and the serialization of concurrent +// migrations. The store writes every query with "?" placeholders and +// rebinds them through the dialect. type Dialect interface { // Name returns a stable identifier ("postgres", "mysql", "sqlite"). Name() string @@ -24,8 +25,21 @@ type Dialect interface { lockRow() string // txOptions are the options of the Consume transaction. txOptions() *sql.TxOptions + // migrationLock is the statement [Store.Migrate] runs first, in the + // transaction creating the table, to serialize concurrent migrations; + // "" when the engine serializes the creation of a table itself and + // Migrate runs its statement on its own. + migrationLock() string } +// migrationLockKey is the PostgreSQL advisory lock [Store.Migrate] takes: +// the first eight bytes of SHA-256 of +// "github.com/hyperscale-stack/security/oidc/client/store/sql.Migrate", +// big-endian, as a bigint. Advisory locks are local to a database, which +// the stores of other modules may share: each store module has a key of +// its own, so that their migrations never wait for one another. +const migrationLockKey = "-3346565968272468758" + // selectTransaction reads a transaction by its state hash. const selectTransaction = `SELECT state_hash, binding_hash, connection_id, issuer, client_id, redirect_uri, response_mode, max_age, acr_values, app_state, created_at, expires_at, integrity @@ -35,6 +49,8 @@ const selectTransaction = `SELECT state_hash, binding_hash, connection_id, issue // at READ COMMITTED whatever the server default, so that its locking read // waits for a concurrent consumer and then sees its deletion, where // REPEATABLE READ or SERIALIZABLE would abort with a serialization failure. +// [Store.Migrate] takes the transaction-level advisory lock +// -3346565968272468758 (pg_advisory_xact_lock) before creating the table. var Postgres Dialect = postgres{} // MySQL is the MySQL / MariaDB dialect (? placeholders). The table must be @@ -80,6 +96,15 @@ func (postgres) txOptions() *sql.TxOptions { return &sql.TxOptions{Isolation: sql.LevelReadCommitted} } +// migrationLock takes a transaction-level advisory lock: concurrent +// CREATE TABLE IF NOT EXISTS of one table can both find it missing, and +// the loser then fails on the unique index of the catalog's row types. A +// transaction-level lock is released by the COMMIT or ROLLBACK ending the +// migration, whatever becomes of the connection afterwards. +func (postgres) migrationLock() string { + return "SELECT pg_advisory_xact_lock(" + migrationLockKey + ")" +} + type mysql struct{} func (mysql) Name() string { return "mysql" } @@ -87,6 +112,10 @@ 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. +func (mysql) migrationLock() string { return "" } + type sqlite struct{} func (sqlite) Name() string { return "sqlite" } @@ -97,3 +126,8 @@ func (sqlite) rebind(q string) string { return q } func (sqlite) lockRow() string { return selectTransaction } func (sqlite) txOptions() *sql.TxOptions { return nil } + +// migrationLock is empty: SQLite's single writer serializes the creation +// of the table, and a statement prepared before a concurrent change of the +// schema is prepared again. +func (sqlite) migrationLock() string { return "" } diff --git a/oidc/client/store/sql/doc.go b/oidc/client/store/sql/doc.go index 51d54244..c0abb35a 100644 --- a/oidc/client/store/sql/doc.go +++ b/oidc/client/store/sql/doc.go @@ -6,7 +6,7 @@ // oidcclient.TransactionStore, the store of pending OpenID Connect logins, // for deployments whose instances share a database. Dialects supported: // PostgreSQL, MySQL, SQLite. Schema returns the DDL; Store.Migrate applies -// it. +// it, and every instance may run it at boot, concurrently. // // The table holds no secret: transactions are keyed by a hash of the // state, the browser binding is a keyed hash, and the nonce and PKCE @@ -25,7 +25,11 @@ // - PostgreSQL: Consume runs at READ COMMITTED whatever the server // default (see [Postgres]). // - MySQL: the table must use InnoDB (row locks); utf8mb4 is not -// required — every column holds ASCII. +// 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. // - 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 @@ -35,7 +39,9 @@ // // The CI of this module exercises SQLite only, with several connections // racing on a database file; the PostgreSQL and MySQL guarantees rest on -// the locking described in [Store]. +// the locking described in [Store], which the opt-in suites of +// internal/interop/oidcstack and internal/interop/sqlstores check on real +// servers. // // # Cleanup // diff --git a/oidc/client/store/sql/migrate_internal_test.go b/oidc/client/store/sql/migrate_internal_test.go new file mode 100644 index 00000000..5bfa166e --- /dev/null +++ b/oidc/client/store/sql/migrate_internal_test.go @@ -0,0 +1,192 @@ +// 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" + "crypto/sha256" + "database/sql" + "encoding/binary" + "path/filepath" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + _ "modernc.org/sqlite" +) + +// schemaTables are the tables of Schema, in the order it creates them. +var schemaTables = []string{"oidc_client_transactions"} + +// TestMigrationLock pins the statement Migrate runs first on each engine: +// on PostgreSQL, the transaction-level advisory lock the package +// documentation names — the first eight bytes of SHA-256 of +// "github.com/hyperscale-stack/security/oidc/client/store/sql.Migrate", +// big-endian, as a bigint — and none on MySQL and SQLite, which serialize +// the creation of a table themselves. +func TestMigrationLock(t *testing.T) { + t.Parallel() + + sum := sha256.Sum256([]byte("github.com/hyperscale-stack/security/oidc/client/store/sql.Migrate")) + key := strconv.FormatInt(int64(binary.BigEndian.Uint64(sum[:8])), 10) + + assert.Equal(t, "-3346565968272468758", key) + assert.Equal(t, "SELECT pg_advisory_xact_lock("+key+")", Postgres.migrationLock()) + assert.Empty(t, MySQL.migrationLock()) + assert.Empty(t, SQLite.migrationLock()) +} + +// lockingStore returns a store over a new SQLite database file whose +// dialect — a mock — names lock as its migration lock, so that Migrate +// takes the path it takes on PostgreSQL: lock first, then the schema, in +// one transaction. The pool has several connections, so that a statement +// run outside the transaction waits for its write lock — and fails after +// the busy timeout — instead of deadlocking a single connection. +func lockingStore(t *testing.T, lock string) (*Store, *sql.DB) { + t.Helper() + + dsn := "file:" + filepath.Join(t.TempDir(), "migrate.db") + "?_pragma=busy_timeout(1000)&_pragma=foreign_keys(1)" + + db, err := sql.Open("sqlite", dsn) + require.NoError(t, err) + db.SetMaxOpenConns(4) + t.Cleanup(func() { _ = db.Close() }) + + d := NewMockDialect(t) + d.EXPECT().migrationLock().Return(lock) + + return &Store{db: db, dialect: d}, db +} + +// execAll runs every statement on db. +func execAll(t *testing.T, db *sql.DB, stmts ...string) { + t.Helper() + + for _, stmt := range stmts { + _, err := db.ExecContext(t.Context(), stmt) + require.NoError(t, err, stmt) + } +} + +// tables lists the tables of db in the order they were created. +func tables(t *testing.T, db *sql.DB) []string { + t.Helper() + + rows, err := db.QueryContext(t.Context(), `SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY rowid`) + require.NoError(t, err) + + defer func() { _ = rows.Close() }() + + var names []string + + for rows.Next() { + var name string + require.NoError(t, rows.Scan(&name)) + + names = append(names, name) + } + + require.NoError(t, rows.Err()) + + return names +} + +// TestMigrateTakesTheLockFirst runs the locked path: the lock statement — +// here one creating a probe table — runs before the schema, and the +// transaction commits. +func TestMigrateTakesTheLockFirst(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `CREATE TABLE migration_lock_probe (taken INTEGER)`) + + require.NoError(t, store.Migrate(t.Context())) + assert.Equal(t, append([]string{"migration_lock_probe"}, schemaTables...), tables(t, db)) +} + +// TestMigrateFailedLockCreatesNothing: a lock that cannot be taken stops +// Migrate before any table is created. +func TestMigrateFailedLockCreatesNothing(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `SELECT no_such_function()`) + + err := store.Migrate(t.Context()) + require.ErrorContains(t, err, "sqlstore: migrate: ") + require.ErrorContains(t, err, "no_such_function") + assert.Empty(t, tables(t, db)) +} + +// TestMigrateRollsBackAFailedSchema: when a statement of the schema fails, +// the whole transaction is rolled back — the lock statement's probe table +// and the tables created before it included. +func TestMigrateRollsBackAFailedSchema(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `CREATE TABLE migration_lock_probe (taken INTEGER)`) + + // An index named like the last table makes its CREATE TABLE fail. + last := schemaTables[len(schemaTables)-1] + execAll(t, db, `CREATE TABLE blocker (x INTEGER)`, `CREATE INDEX `+last+` ON blocker (x)`) + + err := store.Migrate(t.Context()) + require.ErrorContains(t, err, "sqlstore: migrate: ") + require.ErrorContains(t, err, "already an index named "+last) + assert.Equal(t, []string{"blocker"}, tables(t, db)) +} + +// TestMigrateReleasesAFailedTransaction: a Migrate that fails under a +// context that never ends — the usual call at boot — still ends its +// transaction and gives its connection back. Left open, the transaction +// would keep the connection checked out (on PostgreSQL, "idle in +// transaction (aborted)") for the life of the pool, one more at every +// retry. On a one-connection pool, the next Migrate, once the cause is +// gone, goes through. +func TestMigrateReleasesAFailedTransaction(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `CREATE TABLE migration_lock_probe (taken INTEGER)`) + db.SetMaxOpenConns(1) + + last := schemaTables[len(schemaTables)-1] + execAll(t, db, `CREATE TABLE blocker (x INTEGER)`, `CREATE INDEX `+last+` ON blocker (x)`) + + require.Error(t, store.Migrate(context.Background())) + require.Zero(t, db.Stats().InUse, "the failed Migrate gave its connection back") + + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + + _, err := db.ExecContext(ctx, `DROP INDEX `+last) + require.NoError(t, err) + require.NoError(t, store.Migrate(ctx)) +} + +// TestMigrateReportsAFailedCommit: a COMMIT that fails — here on a deferred +// foreign key the lock statement violates — fails Migrate. +func TestMigrateReportsAFailedCommit(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `INSERT INTO child (parent_id) VALUES (42)`) + execAll(t, db, + `CREATE TABLE parent (id INTEGER PRIMARY KEY)`, + `CREATE TABLE child (parent_id INTEGER REFERENCES parent (id) DEFERRABLE INITIALLY DEFERRED)`) + + err := store.Migrate(t.Context()) + require.ErrorContains(t, err, "sqlstore: migrate: commit: ") + require.ErrorContains(t, err, "FOREIGN KEY constraint failed") +} + +// TestMigrateReportsAFailedBegin: a transaction that cannot begin fails +// Migrate. +func TestMigrateReportsAFailedBegin(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `SELECT 1`) + require.NoError(t, db.Close()) + + require.ErrorContains(t, store.Migrate(t.Context()), "sqlstore: migrate: begin: ") +} diff --git a/oidc/client/store/sql/schema.go b/oidc/client/store/sql/schema.go index b45d6fa7..a88e8fbd 100644 --- a/oidc/client/store/sql/schema.go +++ b/oidc/client/store/sql/schema.go @@ -22,9 +22,10 @@ import ( // 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 statement is idempotent (CREATE TABLE IF NOT EXISTS). Production -// deployments typically run it through a migration tool rather than via -// [Store.Migrate]. +// 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 { return []string{ `CREATE TABLE IF NOT EXISTS oidc_client_transactions ( @@ -45,9 +46,29 @@ func Schema(_ Dialect) []string { } } -// Migrate applies [Schema] to the store's database. It is safe to call -// repeatedly: it creates the table when missing and never alters it. +// Migrate applies [Schema] to the store's database: it creates the table +// when missing and never alters it. +// +// It is safe to call repeatedly, and concurrently — every instance of a +// deployment starting at once. On PostgreSQL, where concurrent CREATE +// TABLE IF NOT EXISTS of one table can fail, Migrate runs in one +// transaction that first takes the transaction-level advisory lock +// -3346565968272468758 (pg_advisory_xact_lock; see [Postgres]): concurrent +// calls run one after the other, and a failed call creates nothing. A +// waiting call waits as long as ctx — and the server's lock_timeout or +// statement_timeout — allows: pass a deadline; the driver cancels the +// 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. func (s *Store) Migrate(ctx context.Context) error { + if lock := s.dialect.migrationLock(); lock != "" { + return s.migrateLocked(ctx, lock) + } + for _, stmt := range Schema(s.dialect) { if _, err := s.db.ExecContext(ctx, stmt); err != nil { return fmt.Errorf("sqlstore: migrate: %w", err) @@ -56,3 +77,27 @@ func (s *Store) Migrate(ctx context.Context) error { return nil } + +// migrateLocked applies [Schema] in one transaction that first runs lock, +// the dialect's migration lock: the transaction holds the lock until it +// commits the table, or rolls everything back. +func (s *Store) migrateLocked(ctx context.Context, lock string) error { + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("sqlstore: migrate: begin: %w", err) + } + + defer func() { _ = tx.Rollback() }() // a no-op once committed + + for _, stmt := range append([]string{lock}, Schema(s.dialect)...) { + if _, err := tx.ExecContext(ctx, stmt); err != nil { + return fmt.Errorf("sqlstore: migrate: %w", err) + } + } + + if err := tx.Commit(); err != nil { + return fmt.Errorf("sqlstore: migrate: commit: %w", err) + } + + return nil +} diff --git a/saml/sp/store/sql/dialect.go b/saml/sp/store/sql/dialect.go index 4dfa837b..a21adbe2 100644 --- a/saml/sp/store/sql/dialect.go +++ b/saml/sp/store/sql/dialect.go @@ -9,16 +9,32 @@ import ( "strings" ) -// Dialect abstracts the SQL differences the store cares about: only the -// parameter placeholders. Every statement is written with "?" and rebound. +// 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. type Dialect interface { // Name returns a stable identifier: "postgres", "mysql" or "sqlite". Name() string // rebind rewrites "?" placeholders into the dialect's style. rebind(query string) string + // migrationLock is the statement [Store.Migrate] runs first, in the + // transaction creating the tables, to serialize concurrent migrations; + // "" when the engine serializes the creation of a table itself and + // Migrate runs its statements one by one. + migrationLock() string } +// migrationLockKey is the PostgreSQL advisory lock [Store.Migrate] takes: +// the first eight bytes of SHA-256 of +// "github.com/hyperscale-stack/security/saml/sp/store/sql.Migrate", +// big-endian, as a bigint. Advisory locks are local to a database, which +// the stores of other modules may share: each store module has a key of +// its own, so that their migrations never wait for one another. +const migrationLockKey = "1266919442082856213" + // Postgres is the PostgreSQL dialect ($1, $2, … placeholders). +// [Store.Migrate] takes the transaction-level advisory lock +// 1266919442082856213 (pg_advisory_xact_lock) before creating the tables. var Postgres Dialect = postgres{} // MySQL is the MySQL / MariaDB dialect (? placeholders, InnoDB tables). @@ -55,12 +71,30 @@ func (postgres) rebind(query string) string { return b.String() } +// migrationLock takes a transaction-level advisory lock: concurrent +// CREATE TABLE IF NOT EXISTS of one table can both find it missing, and +// the loser then fails on the unique index of the catalog's row types. A +// transaction-level lock is released by the COMMIT or ROLLBACK ending the +// migration, whatever becomes of the connection afterwards. +func (postgres) migrationLock() string { + return "SELECT pg_advisory_xact_lock(" + migrationLockKey + ")" +} + 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. +func (mysql) migrationLock() string { return "" } + type sqlite struct{} func (sqlite) Name() string { return "sqlite" } func (sqlite) rebind(q string) string { return q } + +// migrationLock is empty: SQLite's single writer serializes the creation +// of the tables, and a statement prepared before a concurrent change of the +// schema is prepared again. +func (sqlite) migrationLock() string { return "" } diff --git a/saml/sp/store/sql/doc.go b/saml/sp/store/sql/doc.go index 98438f3e..d7dacd59 100644 --- a/saml/sp/store/sql/doc.go +++ b/saml/sp/store/sql/doc.go @@ -6,7 +6,7 @@ // login transactions of a SAML Service Provider and its assertion replay // cache, shared by every instance of a deployment. Dialects supported: // PostgreSQL, MySQL, SQLite. Schema returns the DDL; Store.Migrate applies -// it. +// it, and every instance may run it at boot, concurrently. // // # Atomicity // @@ -36,7 +36,9 @@ // - 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 -// character set, collation or sql_mode requirement. +// character set, collation or sql_mode requirement. The keys are 64 +// lowercase hex digits, validated at every call: no collation takes one +// key for another. // - Give the SP a database user limited to the two tables: each record // carries a MAC under samlsp.Config.TransactionKeys, so the database is // trusted for availability and single consumption only. @@ -53,7 +55,9 @@ // progress, which fail once either way. // // The CI of this module exercises SQLite only; the PostgreSQL and MySQL -// guarantees are argued from the statements above. +// guarantees are argued from the statements above, and checked by the +// opt-in suites of internal/interop/saml and internal/interop/sqlstores on +// real servers. // // # Cleanup // diff --git a/saml/sp/store/sql/faultydriver_test.go b/saml/sp/store/sql/faultydriver_test.go index 84538d49..3c797519 100644 --- a/saml/sp/store/sql/faultydriver_test.go +++ b/saml/sp/store/sql/faultydriver_test.go @@ -25,8 +25,17 @@ type faultyConn struct { func (c *faultyConn) Prepare(string) (driver.Stmt, error) { return nil, errors.New("faulty: no prepare") } -func (c *faultyConn) Close() error { return nil } -func (c *faultyConn) Begin() (driver.Tx, error) { return nil, errors.New("faulty: no transactions") } +func (c *faultyConn) Close() error { return nil } + +// Begin opens a transaction whose statements go through the connection, +// failing as configured. +func (c *faultyConn) Begin() (driver.Tx, error) { return faultyTx{}, nil } + +// faultyTx commits and rolls back nothing. +type faultyTx struct{} + +func (faultyTx) Commit() error { return nil } +func (faultyTx) Rollback() error { return nil } func (c *faultyConn) ExecContext(context.Context, string, []driver.NamedValue) (driver.Result, error) { if c.execErr != nil { @@ -59,20 +68,31 @@ func (d faultyDriver) Open(string) (driver.Conn, error) { return d.conn, nil } func openFaulty(t *testing.T, conn *faultyConn) *sqlstore.Store { t.Helper() + return openFaultyDialect(t, conn, sqlstore.Postgres) +} + +func openFaultyDialect(t *testing.T, conn *faultyConn, d sqlstore.Dialect) *sqlstore.Store { + t.Helper() + db := sql.OpenDB(faultyConnector{conn: conn}) t.Cleanup(func() { _ = db.Close() }) - store, err := sqlstore.New(db, sqlstore.Postgres) + store, err := sqlstore.New(db, d) require.NoError(t, err) return store } +// TestMigrateFailure: a failing statement fails Migrate, on the locked +// path (PostgreSQL: the advisory lock is the first statement) and on the +// per-statement one (SQLite). func TestMigrateFailure(t *testing.T) { t.Parallel() - store := openFaulty(t, &faultyConn{execErr: errors.New("permission denied")}) - require.ErrorContains(t, store.Migrate(context.Background()), "permission denied") + for _, d := range []sqlstore.Dialect{sqlstore.Postgres, sqlstore.SQLite} { + store := openFaultyDialect(t, &faultyConn{execErr: errors.New("permission denied")}, d) + require.ErrorContains(t, store.Migrate(context.Background()), "permission denied", d.Name()) + } } func TestConsumeClassificationFailures(t *testing.T) { diff --git a/saml/sp/store/sql/migrate_internal_test.go b/saml/sp/store/sql/migrate_internal_test.go new file mode 100644 index 00000000..82d21027 --- /dev/null +++ b/saml/sp/store/sql/migrate_internal_test.go @@ -0,0 +1,192 @@ +// 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" + "crypto/sha256" + "database/sql" + "encoding/binary" + "path/filepath" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + _ "modernc.org/sqlite" +) + +// schemaTables are the tables of Schema, in the order it creates them. +var schemaTables = []string{"samlsp_transactions", "samlsp_used_assertions"} + +// TestMigrationLock pins the statement Migrate runs first on each engine: +// on PostgreSQL, the transaction-level advisory lock the package +// documentation names — the first eight bytes of SHA-256 of +// "github.com/hyperscale-stack/security/saml/sp/store/sql.Migrate", +// big-endian, as a bigint — and none on MySQL and SQLite, which serialize +// the creation of a table themselves. +func TestMigrationLock(t *testing.T) { + t.Parallel() + + sum := sha256.Sum256([]byte("github.com/hyperscale-stack/security/saml/sp/store/sql.Migrate")) + key := strconv.FormatInt(int64(binary.BigEndian.Uint64(sum[:8])), 10) + + assert.Equal(t, "1266919442082856213", key) + assert.Equal(t, "SELECT pg_advisory_xact_lock("+key+")", Postgres.migrationLock()) + assert.Empty(t, MySQL.migrationLock()) + assert.Empty(t, SQLite.migrationLock()) +} + +// lockingStore returns a store over a new SQLite database file whose +// dialect — a mock — names lock as its migration lock, so that Migrate +// takes the path it takes on PostgreSQL: lock first, then the schema, in +// one transaction. The pool has several connections, so that a statement +// run outside the transaction waits for its write lock — and fails after +// the busy timeout — instead of deadlocking a single connection. +func lockingStore(t *testing.T, lock string) (*Store, *sql.DB) { + t.Helper() + + dsn := "file:" + filepath.Join(t.TempDir(), "migrate.db") + "?_pragma=busy_timeout(1000)&_pragma=foreign_keys(1)" + + db, err := sql.Open("sqlite", dsn) + require.NoError(t, err) + db.SetMaxOpenConns(4) + t.Cleanup(func() { _ = db.Close() }) + + d := NewMockDialect(t) + d.EXPECT().migrationLock().Return(lock) + + return &Store{db: db, dialect: d}, db +} + +// execAll runs every statement on db. +func execAll(t *testing.T, db *sql.DB, stmts ...string) { + t.Helper() + + for _, stmt := range stmts { + _, err := db.ExecContext(t.Context(), stmt) + require.NoError(t, err, stmt) + } +} + +// tables lists the tables of db in the order they were created. +func tables(t *testing.T, db *sql.DB) []string { + t.Helper() + + rows, err := db.QueryContext(t.Context(), `SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY rowid`) + require.NoError(t, err) + + defer func() { _ = rows.Close() }() + + var names []string + + for rows.Next() { + var name string + require.NoError(t, rows.Scan(&name)) + + names = append(names, name) + } + + require.NoError(t, rows.Err()) + + return names +} + +// TestMigrateTakesTheLockFirst runs the locked path: the lock statement — +// here one creating a probe table — runs before the schema, and the +// transaction commits. +func TestMigrateTakesTheLockFirst(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `CREATE TABLE migration_lock_probe (taken INTEGER)`) + + require.NoError(t, store.Migrate(t.Context())) + assert.Equal(t, append([]string{"migration_lock_probe"}, schemaTables...), tables(t, db)) +} + +// TestMigrateFailedLockCreatesNothing: a lock that cannot be taken stops +// Migrate before any table is created. +func TestMigrateFailedLockCreatesNothing(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `SELECT no_such_function()`) + + err := store.Migrate(t.Context()) + require.ErrorContains(t, err, "sqlstore: migrate: ") + require.ErrorContains(t, err, "no_such_function") + assert.Empty(t, tables(t, db)) +} + +// TestMigrateRollsBackAFailedSchema: when a statement of the schema fails, +// the whole transaction is rolled back — the lock statement's probe table +// and the tables created before it included. +func TestMigrateRollsBackAFailedSchema(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `CREATE TABLE migration_lock_probe (taken INTEGER)`) + + // An index named like the last table makes its CREATE TABLE fail. + last := schemaTables[len(schemaTables)-1] + execAll(t, db, `CREATE TABLE blocker (x INTEGER)`, `CREATE INDEX `+last+` ON blocker (x)`) + + err := store.Migrate(t.Context()) + require.ErrorContains(t, err, "sqlstore: migrate: ") + require.ErrorContains(t, err, "already an index named "+last) + assert.Equal(t, []string{"blocker"}, tables(t, db)) +} + +// TestMigrateReleasesAFailedTransaction: a Migrate that fails under a +// context that never ends — the usual call at boot — still ends its +// transaction and gives its connection back. Left open, the transaction +// would keep the connection checked out (on PostgreSQL, "idle in +// transaction (aborted)") for the life of the pool, one more at every +// retry. On a one-connection pool, the next Migrate, once the cause is +// gone, goes through. +func TestMigrateReleasesAFailedTransaction(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `CREATE TABLE migration_lock_probe (taken INTEGER)`) + db.SetMaxOpenConns(1) + + last := schemaTables[len(schemaTables)-1] + execAll(t, db, `CREATE TABLE blocker (x INTEGER)`, `CREATE INDEX `+last+` ON blocker (x)`) + + require.Error(t, store.Migrate(context.Background())) + require.Zero(t, db.Stats().InUse, "the failed Migrate gave its connection back") + + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + + _, err := db.ExecContext(ctx, `DROP INDEX `+last) + require.NoError(t, err) + require.NoError(t, store.Migrate(ctx)) +} + +// TestMigrateReportsAFailedCommit: a COMMIT that fails — here on a deferred +// foreign key the lock statement violates — fails Migrate. +func TestMigrateReportsAFailedCommit(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `INSERT INTO child (parent_id) VALUES (42)`) + execAll(t, db, + `CREATE TABLE parent (id INTEGER PRIMARY KEY)`, + `CREATE TABLE child (parent_id INTEGER REFERENCES parent (id) DEFERRABLE INITIALLY DEFERRED)`) + + err := store.Migrate(t.Context()) + require.ErrorContains(t, err, "sqlstore: migrate: commit: ") + require.ErrorContains(t, err, "FOREIGN KEY constraint failed") +} + +// TestMigrateReportsAFailedBegin: a transaction that cannot begin fails +// Migrate. +func TestMigrateReportsAFailedBegin(t *testing.T) { + t.Parallel() + + store, db := lockingStore(t, `SELECT 1`) + require.NoError(t, db.Close()) + + require.ErrorContains(t, store.Migrate(t.Context()), "sqlstore: migrate: begin: ") +} diff --git a/saml/sp/store/sql/schema.go b/saml/sp/store/sql/schema.go index 429b534c..c7c3ef40 100644 --- a/saml/sp/store/sql/schema.go +++ b/saml/sp/store/sql/schema.go @@ -24,7 +24,8 @@ import ( // 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). +// idempotent (CREATE TABLE IF NOT EXISTS) and valid inside a PostgreSQL +// transaction block, where Migrate runs them. func Schema(_ Dialect) []string { return []string{ `CREATE TABLE IF NOT EXISTS samlsp_transactions ( @@ -42,9 +43,29 @@ func Schema(_ Dialect) []string { } } -// Migrate applies Schema; it is safe to call repeatedly and never alters an -// existing table. +// Migrate applies Schema: it creates the missing tables and never alters +// an existing one. +// +// It is safe to call repeatedly, and concurrently — every instance of a +// deployment starting at once. On PostgreSQL, where concurrent CREATE +// TABLE IF NOT EXISTS of one table can fail, Migrate runs in one +// transaction that first takes the transaction-level advisory lock +// 1266919442082856213 (pg_advisory_xact_lock; see Postgres): concurrent +// calls run one after the other, and a failed call creates nothing. A +// waiting call waits as long as ctx — and the server's lock_timeout or +// statement_timeout — allows: pass a deadline; the driver cancels the +// 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. func (s *Store) Migrate(ctx context.Context) error { + if lock := s.dialect.migrationLock(); lock != "" { + return s.migrateLocked(ctx, lock) + } + for _, stmt := range Schema(s.dialect) { if _, err := s.db.ExecContext(ctx, stmt); err != nil { return fmt.Errorf("sqlstore: migrate: %w", err) @@ -53,3 +74,27 @@ func (s *Store) Migrate(ctx context.Context) error { return nil } + +// migrateLocked applies Schema in one transaction that first runs lock, +// the dialect's migration lock: the transaction holds the lock until it +// commits the tables, or rolls everything back. +func (s *Store) migrateLocked(ctx context.Context, lock string) error { + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("sqlstore: migrate: begin: %w", err) + } + + defer func() { _ = tx.Rollback() }() // a no-op once committed + + for _, stmt := range append([]string{lock}, Schema(s.dialect)...) { + if _, err := tx.ExecContext(ctx, stmt); err != nil { + return fmt.Errorf("sqlstore: migrate: %w", err) + } + } + + if err := tx.Commit(); err != nil { + return fmt.Errorf("sqlstore: migrate: commit: %w", err) + } + + return nil +}