From 503cdf8dd7038c3e6cac2c993836144d2df9c286 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Tue, 13 Oct 2026 09:02:08 +0200 Subject: [PATCH 1/9] fix: serialize concurrent Migrate of the SQL stores on PostgreSQL Every instance of a deployment typically calls Migrate at boot. On PostgreSQL, concurrent CREATE TABLE IF NOT EXISTS of one table all find it missing, and the losers then fail on the unique index of the catalog's row types ("duplicate key value violates unique constraint pg_type_typname_nsp_index", SQLSTATE 23505): 90 of 120 calls failed in a race of 12 instances over 10 rounds. The godoc promised "safe to call repeatedly". The three SQL stores (oauth2/store/sql, oidc/client/store/sql, saml/sp/store/sql) give their sealed Dialect an unexported migrationLock(). On PostgreSQL, Migrate runs in one transaction that first takes the transaction-level advisory lock pg_advisory_xact_lock(): - the key is the first eight bytes of SHA-256(".Migrate"); - it is distinct per module, since advisory locks are per database and the stores may share one, and it is documented; - the lock is released by COMMIT or ROLLBACK, whatever becomes of the pooled connection, PgBouncer transaction pooling included. Concurrent calls run one after the other. A waiting call honours its context: the driver cancels the server-side wait. A failed call creates nothing, since PostgreSQL DDL is transactional. MySQL and MariaDB serialize CREATE TABLE on the metadata lock of the table name; SQLite does it with its single writer and re-prepared statements, given the busy timeout it requires. Both keep their per-statement path: the same race showed 0 failures on MySQL 8.0, MySQL 8.4 and MariaDB 11.4. A new private, test-only module, internal/interop/sqlstores, runs Migrate of the three stores together: - on SQLite, in CI; - opt-in through its compose.yaml, on PostgreSQL 16 (server default and SERIALIZABLE default isolation), MySQL 8.0, MySQL 8.4 and MariaDB 11.4. It also checks that Migrate waits for its documented lock and leaves no table behind when the DDL fails. In-module tests drive the locked path on SQLite through the generated MockDialect: lock first, rollback on a failing statement, commit and begin failures. Audit: STORE-04. --- CHANGELOG.md | 15 + LIMITATIONS.md | 8 +- MIGRATION.md | 19 + docs/architecture.md | 3 + go.work | 1 + internal/interop/sqlstores/backends_test.go | 249 +++++++++++++ internal/interop/sqlstores/compose.yaml | 69 ++++ internal/interop/sqlstores/doc_test.go | 12 + internal/interop/sqlstores/go.mod | 64 ++++ internal/interop/sqlstores/go.sum | 111 ++++++ internal/interop/sqlstores/migrate_test.go | 351 ++++++++++++++++++ oauth2/store/sql/dialect.go | 43 ++- oauth2/store/sql/doc.go | 3 +- oauth2/store/sql/migrate_internal_test.go | 163 ++++++++ oauth2/store/sql/schema.go | 55 ++- oidc/client/store/sql/dialect.go | 40 +- oidc/client/store/sql/doc.go | 2 +- .../client/store/sql/migrate_internal_test.go | 163 ++++++++ oidc/client/store/sql/schema.go | 55 ++- saml/sp/store/sql/dialect.go | 38 +- saml/sp/store/sql/doc.go | 2 +- saml/sp/store/sql/faultydriver_test.go | 13 +- saml/sp/store/sql/migrate_internal_test.go | 163 ++++++++ saml/sp/store/sql/schema.go | 51 ++- 24 files changed, 1662 insertions(+), 31 deletions(-) create mode 100644 internal/interop/sqlstores/backends_test.go create mode 100644 internal/interop/sqlstores/compose.yaml create mode 100644 internal/interop/sqlstores/doc_test.go create mode 100644 internal/interop/sqlstores/go.mod create mode 100644 internal/interop/sqlstores/go.sum create mode 100644 internal/interop/sqlstores/migrate_test.go create mode 100644 oauth2/store/sql/migrate_internal_test.go create mode 100644 oidc/client/store/sql/migrate_internal_test.go create mode 100644 saml/sp/store/sql/migrate_internal_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ceab215..f87347ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -947,6 +947,21 @@ legacy packages (`authentication/`, `authorization/`, the in-tree 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. +- 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`). - 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 diff --git a/LIMITATIONS.md b/LIMITATIONS.md index 85147723..7a948b71 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -53,10 +53,12 @@ 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 `Migrate` concurrently 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' diff --git a/MIGRATION.md b/MIGRATION.md index 3c13a207..4ce47f49 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,17 @@ 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. 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/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..df8d0341 --- /dev/null +++ b/internal/interop/sqlstores/backends_test.go @@ -0,0 +1,249 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package 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 run-time parameters every PostgreSQL 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 + + 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..f74d2683 --- /dev/null +++ b/internal/interop/sqlstores/migrate_test.go @@ -0,0 +1,351 @@ +// 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() + } +} + +// 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, s.migrate(ctx, db, enginePostgres), 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, other.migrate(otherCtx, db, enginePostgres), "%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. +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) + db := open(t, driver, dsn, 2) + + _, err := db.ExecContext(t.Context(), "CREATE TYPE "+s.tables[len(s.tables)-1]+" AS ENUM ('taken')") + require.NoError(t, err) + + require.Error(t, s.migrate(t.Context(), db, enginePostgres)) + requireTables(t, db, enginePostgres, s, false) + }) + } +} diff --git a/oauth2/store/sql/dialect.go b/oauth2/store/sql/dialect.go index 2b3ba9e6..bf1b2c77 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,21 @@ 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 } +// 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,7 +60,9 @@ 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 @@ -95,6 +110,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" } @@ -112,6 +136,10 @@ 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" @@ -135,3 +163,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/doc.go b/oauth2/store/sql/doc.go index 7ffb3b26..e1d20068 100644 --- a/oauth2/store/sql/doc.go +++ b/oauth2/store/sql/doc.go @@ -38,7 +38,8 @@ // // # 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, diff --git a/oauth2/store/sql/migrate_internal_test.go b/oauth2/store/sql/migrate_internal_test.go new file mode 100644 index 00000000..c8d451b1 --- /dev/null +++ b/oauth2/store/sql/migrate_internal_test.go @@ -0,0 +1,163 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "crypto/sha256" + "database/sql" + "encoding/binary" + "path/filepath" + "strconv" + "testing" + + "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{"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) + + 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)) +} + +// 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..440208c7 100644 --- a/oauth2/store/sql/schema.go +++ b/oauth2/store/sql/schema.go @@ -33,9 +33,11 @@ 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. +// 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(_ Dialect) []string { return []string{ `CREATE TABLE IF NOT EXISTS oauth2_auth_codes ( @@ -87,12 +89,31 @@ func Schema(_ Dialect) []string { } } -// 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 +122,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/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..086c09ce 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 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..5698a22e --- /dev/null +++ b/oidc/client/store/sql/migrate_internal_test.go @@ -0,0 +1,163 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "crypto/sha256" + "database/sql" + "encoding/binary" + "path/filepath" + "strconv" + "testing" + + "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)) +} + +// 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..9fc5242a 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 // diff --git a/saml/sp/store/sql/faultydriver_test.go b/saml/sp/store/sql/faultydriver_test.go index 84538d49..5710d5c8 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 { 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..05769ec7 --- /dev/null +++ b/saml/sp/store/sql/migrate_internal_test.go @@ -0,0 +1,163 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package sqlstore + +import ( + "crypto/sha256" + "database/sql" + "encoding/binary" + "path/filepath" + "strconv" + "testing" + + "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)) +} + +// 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 +} From 79da177f51ac2125c2b35e98d338b4fd05dab078 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Tue, 13 Oct 2026 09:11:27 +0200 Subject: [PATCH 2/9] fix(oauth2): compare the SQL store's keys byte for byte on MySQL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MySQL and MariaDB compare text under the collation of its column: - the defaults (utf8mb4_0900_ai_ci on MySQL 8, utf8mb4_uca1400_ai_ci on MariaDB 11) ignore letter case and accents, and take canonically equivalent forms and ß / ss for one character; - MariaDB's, like every PAD SPACE collation (utf8mb4_bin and ascii_bin included), also ignore trailing spaces. The SQL store kept its keys (token-family IDs, code and token hashes) in VARCHAR columns, so 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: this breaks the family isolation (I7) of oauth2.Storage. The package documentation claimed text was "compared exactly". The library's own family IDs (random or derived base64url) and hashes (lowercase hex) never collide this way; custom family IDs can. Schema(MySQL) now declares the six key columns VARBINARY, which MySQL and MariaDB compare byte for byte (MySQL 8.0 §13.3.3, §12.8.5). A new unexported keyType dialect method gives VARCHAR on PostgreSQL (deterministic default collation) and SQLite (BINARY collation): their DDL is unchanged, and Schema(nil) keeps the portable DDL. utf8mb4_bin would still merge trailing spaces, and utf8mb4_0900_bin is missing on older MariaDB. Migrate stays create-only. MIGRATION.md gives the MySQL upgrade: - preflight queries: tokens merged into a look-alike's record, and key lengths; - the ALTERs, in strict mode; - an INSERT that records revoked every family left without a record of its own, so that no look-alike of a revoked family is revived by a later issuance (I5, I8); - an optional conversion of the codes' table. The opt-in suite runs those very statements, extracted from MIGRATION.md, on MySQL 8.0, MySQL 8.4 and MariaDB 11.4, then the conformance suite on the upgraded tables. storetest gains KeysAreComparedExactly, which fails on MySQL with the former DDL: - look-alike family IDs (case, accent, Unicode form, ß / ss, trailing space) stay isolated; - case-variant token and code hashes stay distinct. The OIDC and SAML stores, whose keys are validated lowercase hex, document why they need no binary collation. Audit: STORE-I1. --- CHANGELOG.md | 17 +- LIMITATIONS.md | 8 +- MIGRATION.md | 104 +++++ internal/interop/sqlstores/oauth2_test.go | 366 ++++++++++++++++++ oauth2/storage.go | 2 +- oauth2/store/sql/dialect.go | 27 +- .../store/sql/dialect_family_internal_test.go | 12 + oauth2/store/sql/doc.go | 25 +- oauth2/store/sql/migrate_internal_test.go | 2 + oauth2/store/sql/schema.go | 39 +- oauth2/store/sql/schema_test.go | 49 ++- oauth2/storetest/conformance.go | 4 +- oauth2/storetest/family.go | 70 ++++ oidc/client/store/sql/doc.go | 6 +- saml/sp/store/sql/doc.go | 4 +- 15 files changed, 696 insertions(+), 39 deletions(-) create mode 100644 internal/interop/sqlstores/oauth2_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f87347ff..83c9e894 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -946,7 +946,7 @@ 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, @@ -1817,3 +1817,18 @@ 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 `_bin` collation, 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/LIMITATIONS.md b/LIMITATIONS.md index 7a948b71..55f36978 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -58,12 +58,14 @@ future refactor phases. READ COMMITTED on PostgreSQL, InnoDB on MySQL) are argued from their 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 `Migrate` concurrently on them, locally only. + 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 4ce47f49..d846e294 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -2268,3 +2268,107 @@ 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 `_bin` +collation, 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, run steps 2 and 3 + with token writes stopped: between the `ALTER` of + `oauth2_token_families` and step 3, an issuance into such a family would + record it active and revive the tokens it shares with a revoked family. A + key longer than its column makes step 2 fail (it runs in strict mode): + delete those rows first — the store refuses such keys since #82. +2. **Convert the key columns and record the orphaned families revoked**, in + one session, before traffic resumes: + + ```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; until it is done, comparisons stay as they + were. +5. **Read the keys** with `CAST(family_id AS CHAR)`: the `mysql` client + shows binary columns in hexadecimal (`--binary-as-hex`, its default + 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/internal/interop/sqlstores/oauth2_test.go b/internal/interop/sqlstores/oauth2_test.go new file mode 100644 index 00000000..ffb8e900 --- /dev/null +++ b/internal/interop/sqlstores/oauth2_test.go @@ -0,0 +1,366 @@ +// 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 a look-alike — revoked, and active — it checks +// what the upgrade keeps: the preflight counts the merged tokens, the +// key columns become binary, a revoked family and its look-alike stay +// revoked even through a new issuance into the look-alike (the orphaned +// family is recorded revoked), the look-alike of an active family reads +// revoked, other families and codes are untouched. 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("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() + + 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 both. + var ( + alice = tokenPair("alice", "User-Alice") + aliceAlike = tokenPair("alice-alike", "user-alice") + bob = tokenPair("bob", "Team-Bob") + bobAlike = tokenPair("bob-alike", "team-bob") + plain = tokenPair("plain", "plain-family") + ) + + for _, p := range []*oauth2.TokenPair{alice, aliceAlike, bob, bobAlike, plain} { + require.NoError(t, s.SaveTokenPair(t.Context(), p)) + } + + require.NoError(t, s.RevokeRefreshFamily(t.Context(), "User-Alice")) + requireUnusable(t, s, aliceAlike) + + 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{2, 2}, 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) + requireUnusable(t, s, aliceAlike) + require.ErrorIs(t, s.SaveTokenPair(t.Context(), tokenPair("alice-alike-new", "user-alice")), oauth2.ErrTokenFamilyRevoked, + "an issuance into the look-alike of a revoked family") + requireUnusable(t, s, aliceAlike) + + requireUsable(t, s, bob) + requireUnusable(t, s, bobAlike) + requireUsable(t, s, plain) + + _, err = s.ConsumeAuthorizationCode(t.Context(), code.CodeHash) + require.NoError(t, err) +} + +// 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..e2847cbb 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 diff --git a/oauth2/store/sql/dialect.go b/oauth2/store/sql/dialect.go index bf1b2c77..76a6ea4b 100644 --- a/oauth2/store/sql/dialect.go +++ b/oauth2/store/sql/dialect.go @@ -37,6 +37,10 @@ type Dialect interface { // "" 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: @@ -66,7 +70,8 @@ const ( 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: @@ -119,6 +124,13 @@ 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" } @@ -131,6 +143,12 @@ 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 } @@ -150,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. 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 e1d20068..145246c6 100644 --- a/oauth2/store/sql/doc.go +++ b/oauth2/store/sql/doc.go @@ -28,13 +28,26 @@ // 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 mysql client shows them in +// hexadecimal: 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 // diff --git a/oauth2/store/sql/migrate_internal_test.go b/oauth2/store/sql/migrate_internal_test.go index c8d451b1..fe5efb23 100644 --- a/oauth2/store/sql/migrate_internal_test.go +++ b/oauth2/store/sql/migrate_internal_test.go @@ -13,6 +13,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" _ "modernc.org/sqlite" ) @@ -56,6 +57,7 @@ func lockingStore(t *testing.T, lock string) (*Store, *sql.DB) { d := NewMockDialect(t) d.EXPECT().migrationLock().Return(lock) + d.EXPECT().keyType(mock.Anything).RunAndReturn(sqlite{}.keyType).Maybe() return &Store{db: db, dialect: d}, db } diff --git a/oauth2/store/sql/schema.go b/oauth2/store/sql/schema.go index 440208c7..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 @@ -38,10 +46,17 @@ import ( // 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(_ Dialect) []string { +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, @@ -57,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, @@ -69,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, @@ -82,7 +97,7 @@ 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 )`, 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/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..38109b72 100644 --- a/oauth2/storetest/family.go +++ b/oauth2/storetest/family.go @@ -268,6 +268,76 @@ 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 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) + } + + upper, lower := newPair("Alike-Hash", "fam-alike-upper"), newPair("aLIKE-hASH", "fam-alike-lower") + mustSave(t, s, upper) + mustSave(t, s, lower) + requireOwnFamily(t, s, upper) + requireOwnFamily(t, s, lower) + + codes := []*oauth2.AuthorizationCode{sampleCode("Alike-Code"), sampleCode("aLIKE-cODE")} + 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/doc.go b/oidc/client/store/sql/doc.go index 086c09ce..d9d030b5 100644 --- a/oidc/client/store/sql/doc.go +++ b/oidc/client/store/sql/doc.go @@ -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 diff --git a/saml/sp/store/sql/doc.go b/saml/sp/store/sql/doc.go index 9fc5242a..526b0ca9 100644 --- a/saml/sp/store/sql/doc.go +++ b/saml/sp/store/sql/doc.go @@ -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. From 90a51fdd75485963f341ddad466cabdb0f649594 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Tue, 13 Oct 2026 09:13:44 +0200 Subject: [PATCH 3/9] docs(oauth2): state unknown write outcomes and lost consumed tokens The I3 invariant of oauth2.Storage said that a failed SaveTokenPair or RotateRefreshToken "persists no token and consumes nothing". That is true of a call the store refuses. It is not true of a backend failure reported after the commit was sent: a dropped connection, or a context ending while PostgreSQL commits, leaves the outcome unknown. All the writes may have been applied, never a part of them; the audit measured 59 such outcomes under tight deadlines. I3, the Atomicity bullet of docs/security-considerations.md and the ConsumeAuthorizationCode godoc now say so. A client retrying the refresh then presents a consumed token: a reuse, which revokes the family, as "Lost responses look like theft" already states. A SQL store test pins it with a driver that commits, then reports a lost acknowledgement. The documented limitations of the stores were re-checked; all hold. One case was missing: a consumed refresh token whose Redis key is evicted, or whose SQL row is deleted before its expires_at, reads as an unknown token when presented again. Its reuse is refused but no longer detected, and its family is not revoked. The Redis and SQL package documentation and the "Fail closed" bullet now say so. The statements about what CI exercises now name the opt-in suites that run the SQL stores on PostgreSQL, MySQL and MariaDB. Audit: STORE-I4, STORE-DL. --- CHANGELOG.md | 10 ++ docs/security-considerations.md | 20 +++- oauth2/storage.go | 18 +++- oauth2/store/redis/doc.go | 5 +- oauth2/store/sql/doc.go | 5 +- oauth2/store/sql/store_lostack_test.go | 134 +++++++++++++++++++++++++ oidc/client/store/sql/doc.go | 4 +- saml/sp/store/sql/doc.go | 4 +- 8 files changed, 187 insertions(+), 13 deletions(-) create mode 100644 oauth2/store/sql/store_lostack_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 83c9e894..400dbd8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -962,6 +962,16 @@ legacy packages (`authentication/`, `authorization/`, the in-tree 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 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/oauth2/storage.go b/oauth2/storage.go index e2847cbb..b2db50a1 100644 --- a/oauth2/storage.go +++ b/oauth2/storage.go @@ -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/doc.go b/oauth2/store/sql/doc.go index 145246c6..5623634b 100644 --- a/oauth2/store/sql/doc.go +++ b/oauth2/store/sql/doc.go @@ -80,7 +80,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/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/oidc/client/store/sql/doc.go b/oidc/client/store/sql/doc.go index d9d030b5..c0abb35a 100644 --- a/oidc/client/store/sql/doc.go +++ b/oidc/client/store/sql/doc.go @@ -39,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/saml/sp/store/sql/doc.go b/saml/sp/store/sql/doc.go index 526b0ca9..d7dacd59 100644 --- a/saml/sp/store/sql/doc.go +++ b/saml/sp/store/sql/doc.go @@ -55,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 // From 4ef5baeab621d7fdca5a3a3d0c37f5208e510e98 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Tue, 13 Oct 2026 09:13:44 +0200 Subject: [PATCH 4/9] docs: list jq among the release-check prerequisites scripts/release-check.sh stops with "jq is required" when jq is missing (it reads `go mod edit -json` with it), but docs/releasing.md did not list it next to git and go as a prerequisite of `make release-check`. GitHub runners have it; a maintainer checking a tag locally may not. Audit: CI-02. --- docs/releasing.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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`); From 51d54cc7bae595e11737d4da2baa52ee080650be Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Tue, 13 Oct 2026 09:23:23 +0200 Subject: [PATCH 5/9] docs(oauth2): name the PAD SPACE collations and the key bounds The MySQL upgrade notes said every _bin collation ignores trailing spaces: utf8mb4_0900_bin does not, it is NO PAD. The collations that do are the PAD SPACE ones, utf8mb4_bin included. The preflight also said the store refuses every over-long key since #82. That is true of family IDs (64 bytes); the store does not bound hashes, and its own are 64 characters. Audit: STORE-I1. --- CHANGELOG.md | 17 +++++++++-------- MIGRATION.md | 23 ++++++++++++----------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 400dbd8c..916815a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1832,13 +1832,14 @@ legacy packages (`authentication/`, `authorization/`, the in-tree 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 `_bin` collation, 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. + 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/MIGRATION.md b/MIGRATION.md index d846e294..a378f5e2 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -2275,15 +2275,15 @@ 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 `_bin` -collation, 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. +`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 | | -------------------------------------------------------------- | -------------------------------------------- | ------------------------------------------------ | @@ -2322,8 +2322,9 @@ To migrate (MySQL and MariaDB only; PostgreSQL and SQLite need nothing): with token writes stopped: between the `ALTER` of `oauth2_token_families` and step 3, an issuance into such a family would record it active and revive the tokens it shares with a revoked family. A - key longer than its column makes step 2 fail (it runs in strict mode): - delete those rows first — the store refuses such keys since #82. + 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, before traffic resumes: From 026c6603cc2777a231f75fddeaed010f57ee307d Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Tue, 13 Oct 2026 09:24:02 +0200 Subject: [PATCH 6/9] test: cover the per-statement Migrate failure of the SQL stores On MySQL and SQLite, Migrate runs its statements one by one. Its error branch was no longer reached once the faulty-driver test of the SAML store went through the PostgreSQL path, and no test of the OAuth2 store reached it. Both stores now fail a Migrate on that path and check the wrapped error. Audit: STORE-04. --- oauth2/store/sql/store_error_test.go | 3 +++ saml/sp/store/sql/faultydriver_test.go | 17 ++++++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) 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/saml/sp/store/sql/faultydriver_test.go b/saml/sp/store/sql/faultydriver_test.go index 5710d5c8..3c797519 100644 --- a/saml/sp/store/sql/faultydriver_test.go +++ b/saml/sp/store/sql/faultydriver_test.go @@ -68,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) { From fe3906b161c42d2a3f04fbf8f04bd6e4e8716531 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Tue, 13 Oct 2026 09:30:45 +0200 Subject: [PATCH 7/9] docs: bring CLAUDE.md up to date with the SAML SP and the SQL store tests The SAML section still described the pipeline before the audit fixes (consumption after verification), omitted the required Config.TransactionKeys and gave stale corpus facts. It now gives the validation order the SP enforces (transaction lookup with its integrity MAC, status and InResponseTo, atomic consumption, then verification and the processing rules), the required server-held transaction keys, the corpus size (390 entries over 360 responses) and the toolchain condition for a byte-identical regeneration. The module table lists the new private test module internal/interop/sqlstores, which runs the three SQL stores on real servers. Audit: SAML-01, STORE-02, STORE-04. --- CLAUDE.md | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) 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 From 608a75c40e3b958620138b94297963dd8607aff1 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Tue, 13 Oct 2026 10:30:56 +0200 Subject: [PATCH 8/9] test: pin the end of a failed Migrate transaction and its context Removing the deferred Rollback of the locked Migrate survived every test. The tests passed t.Context(), which the testing package cancels before cleanup, so database/sql rolled the transaction back for them. A Migrate failing under a context that never ends (the usual boot call) would keep its connection checked out, "idle in transaction (aborted)" on PostgreSQL, one more at every retry. Each store now has an in-module test: a one-connection pool, context.Background(), the connection back after the failure, and the next Migrate going through. The PostgreSQL failure test checks the same, plus no aborted backend. A Migrate that ignored its context made the lock test hang until go test timed out. The waits now run in a goroutine bounded by a timer, so such a regression fails the test. Audit: STORE-04. --- internal/interop/sqlstores/migrate_test.go | 55 ++++++++++++++++--- oauth2/store/sql/migrate_internal_test.go | 29 ++++++++++ .../client/store/sql/migrate_internal_test.go | 29 ++++++++++ saml/sp/store/sql/migrate_internal_test.go | 29 ++++++++++ 4 files changed, 135 insertions(+), 7 deletions(-) diff --git a/internal/interop/sqlstores/migrate_test.go b/internal/interop/sqlstores/migrate_test.go index f74d2683..d7c93d08 100644 --- a/internal/interop/sqlstores/migrate_test.go +++ b/internal/interop/sqlstores/migrate_test.go @@ -277,6 +277,26 @@ func migrateTogether(t *testing.T, b backend) { } } +// 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 { @@ -309,7 +329,7 @@ func TestMigrateWaitsForItsLock(t *testing.T) { ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) defer cancel() - require.ErrorIs(t, s.migrate(ctx, db, enginePostgres), context.DeadlineExceeded) + 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. @@ -317,7 +337,7 @@ func TestMigrateWaitsForItsLock(t *testing.T) { defer cancelOther() other := sqlStores[(i+1)%len(sqlStores)] - require.NoError(t, other.migrate(otherCtx, db, enginePostgres), "%s's lock is not %s's", other.name, s.name) + 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()) @@ -332,20 +352,41 @@ func TestMigrateWaitsForItsLock(t *testing.T) { // 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. +// 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) - db := open(t, driver, dsn, 2) + admin := open(t, driver, dsn, 1) + db := open(t, driver, dsn, 1) - _, err := db.ExecContext(t.Context(), "CREATE TYPE "+s.tables[len(s.tables)-1]+" AS ENUM ('taken')") + 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(t.Context(), db, enginePostgres)) - requireTables(t, db, enginePostgres, s, false) + 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/oauth2/store/sql/migrate_internal_test.go b/oauth2/store/sql/migrate_internal_test.go index fe5efb23..8954d65f 100644 --- a/oauth2/store/sql/migrate_internal_test.go +++ b/oauth2/store/sql/migrate_internal_test.go @@ -5,12 +5,14 @@ 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" @@ -138,6 +140,33 @@ func TestMigrateRollsBackAFailedSchema(t *testing.T) { 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) { diff --git a/oidc/client/store/sql/migrate_internal_test.go b/oidc/client/store/sql/migrate_internal_test.go index 5698a22e..5bfa166e 100644 --- a/oidc/client/store/sql/migrate_internal_test.go +++ b/oidc/client/store/sql/migrate_internal_test.go @@ -5,12 +5,14 @@ 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" @@ -136,6 +138,33 @@ func TestMigrateRollsBackAFailedSchema(t *testing.T) { 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) { diff --git a/saml/sp/store/sql/migrate_internal_test.go b/saml/sp/store/sql/migrate_internal_test.go index 05769ec7..82d21027 100644 --- a/saml/sp/store/sql/migrate_internal_test.go +++ b/saml/sp/store/sql/migrate_internal_test.go @@ -5,12 +5,14 @@ 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" @@ -136,6 +138,33 @@ func TestMigrateRollsBackAFailedSchema(t *testing.T) { 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) { From f4bdbb177c4e65df4b86dfc85a604f885f28cfa8 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Tue, 13 Oct 2026 10:30:56 +0200 Subject: [PATCH 9/9] fix(oauth2): pin and correct the MySQL key upgrade The upgrade test could not tell the documented orphan INSERT from one missing either half of its UNION: - without the refresh half, a revoked look-alike whose access token was gone is revived by a new issuance; - without the access half, an access-only look-alike is revived. It also missed the retention floor and the strict sql_mode. The test now seeds both shapes and checks the inserted records (revoked, kept for FamilyRevocationRetention at least). On a connection whose own sql_mode is lax, it checks that a 66-byte family ID stops the ALTER instead of being truncated. The conformance case also tells apart token and code hashes that differ by a trailing space. MIGRATION.md: the instruction to stop token writes named the optional step 3. It now targets step 2, between its families ALTER and its INSERT, and only when the preflight counted merged tokens. It also says that comparisons become exact table by table, and that the hex display is the interactive mysql client's default. Audit: STORE-I1. --- MIGRATION.md | 28 ++-- internal/interop/sqlstores/backends_test.go | 7 +- internal/interop/sqlstores/oauth2_test.go | 148 +++++++++++++++++--- oauth2/store/sql/doc.go | 5 +- oauth2/storetest/family.go | 23 ++- 5 files changed, 168 insertions(+), 43 deletions(-) diff --git a/MIGRATION.md b/MIGRATION.md index a378f5e2..464ff440 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -2318,15 +2318,16 @@ To migrate (MySQL and MariaDB only; PostgreSQL and SQLite need nothing): ``` The counts are 0 unless custom family IDs differ only by case, accents, - Unicode form or trailing spaces. When they are not, run steps 2 and 3 - with token writes stopped: between the `ALTER` of - `oauth2_token_families` and step 3, 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. + 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, before traffic resumes: + 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'); @@ -2366,10 +2367,11 @@ To migrate (MySQL and MariaDB only; PostgreSQL and SQLite need nothing): `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; until it is done, comparisons stay as they - were. -5. **Read the keys** with `CAST(family_id AS CHAR)`: the `mysql` client - shows binary columns in hexadecimal (`--binary-as-hex`, its default - since 8.0.19). + 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/internal/interop/sqlstores/backends_test.go b/internal/interop/sqlstores/backends_test.go index df8d0341..30294e53 100644 --- a/internal/interop/sqlstores/backends_test.go +++ b/internal/interop/sqlstores/backends_test.go @@ -58,7 +58,8 @@ type backend struct { engine engine // env names the variable holding the server's DSN; SQLite needs none. env string - // params are run-time parameters every PostgreSQL namespace DSN sets. + // 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. @@ -179,6 +180,10 @@ func (b backend) mysqlNamespace(t *testing.T) string { cfg.DBName = database cfg.ClientFoundRows = b.clientFoundRows + if len(b.params) > 0 { + cfg.Params = b.params + } + return cfg.FormatDSN() } diff --git a/internal/interop/sqlstores/oauth2_test.go b/internal/interop/sqlstores/oauth2_test.go index ffb8e900..4a3cd88b 100644 --- a/internal/interop/sqlstores/oauth2_test.go +++ b/internal/interop/sqlstores/oauth2_test.go @@ -246,13 +246,16 @@ func requireBinaryKeys(t *testing.T, db *sql.DB) { // TestOAuth2MySQLUpgrade runs the upgrade statements of MIGRATION.md on // tables created by the former MySQL DDL. Seeded with families the former -// collation merged with a look-alike — revoked, and active — it checks -// what the upgrade keeps: the preflight counts the merged tokens, the -// key columns become binary, a revoked family and its look-alike stay -// revoked even through a new issuance into the look-alike (the orphaned -// family is recorded revoked), the look-alike of an active family reads -// revoked, other families and codes are untouched. The conformance suite -// then passes on upgraded tables. +// 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") @@ -268,6 +271,10 @@ func TestOAuth2MySQLUpgrade(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) @@ -289,6 +296,8 @@ func TestOAuth2MySQLUpgrade(t *testing.T) { 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...) @@ -297,26 +306,41 @@ func upgradeSeededDatabase(t *testing.T, b backend, preflight, upgrade []string) 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 both. + // family joins that family's record: revoking one revokes all of them. var ( - alice = tokenPair("alice", "User-Alice") - aliceAlike = tokenPair("alice-alike", "user-alice") - bob = tokenPair("bob", "Team-Bob") - bobAlike = tokenPair("bob-alike", "team-bob") - plain = tokenPair("plain", "plain-family") + 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") ) - for _, p := range []*oauth2.TokenPair{alice, aliceAlike, bob, bobAlike, plain} { + // 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")) - requireUnusable(t, s, aliceAlike) + + 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{2, 2}, queryRow(t, db, preflight[0]), "access and refresh tokens merged with a look-alike") + 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) @@ -326,19 +350,103 @@ func upgradeSeededDatabase(t *testing.T, b backend, preflight, upgrade []string) requireBinaryKeys(t, db) requireUnusable(t, s, alice) - requireUnusable(t, s, aliceAlike) - require.ErrorIs(t, s.SaveTokenPair(t.Context(), tokenPair("alice-alike-new", "user-alice")), oauth2.ErrTokenFamilyRevoked, - "an issuance into the look-alike of a revoked family") - requireUnusable(t, s, aliceAlike) + + 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) { diff --git a/oauth2/store/sql/doc.go b/oauth2/store/sql/doc.go index 5623634b..8f23a14e 100644 --- a/oauth2/store/sql/doc.go +++ b/oauth2/store/sql/doc.go @@ -34,8 +34,9 @@ // 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 mysql client shows them in -// hexadecimal: read them with CAST(family_id AS CHAR). PostgreSQL +// 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; diff --git a/oauth2/storetest/family.go b/oauth2/storetest/family.go index 38109b72..38575dfd 100644 --- a/oauth2/storetest/family.go +++ b/oauth2/storetest/family.go @@ -287,7 +287,8 @@ var lookAlikeKeys = [][2]string{ // 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 are records of their own, each consumed on its own. +// 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]) @@ -306,13 +307,21 @@ func testKeysAreComparedExactly(t *testing.T, s oauth2.Storage) { requireUsable(t, s, first) } - upper, lower := newPair("Alike-Hash", "fam-alike-upper"), newPair("aLIKE-hASH", "fam-alike-lower") - mustSave(t, s, upper) - mustSave(t, s, lower) - requireOwnFamily(t, s, upper) - requireOwnFamily(t, s, lower) + 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) + } - codes := []*oauth2.AuthorizationCode{sampleCode("Alike-Code"), sampleCode("aLIKE-cODE")} + 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) }