Repository navigation
fix(stores)!: close the security audit findings of the SQL stores - #121
Merged
Merged
Conversation
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(<key>):
- the key is the first eight bytes of SHA-256("<module path>.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.
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.
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.
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.
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.
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.
…ests 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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the following findings of the internal security audit of 2026-10-12 (commit c5c2ac3): STORE-04, STORE-I1, STORE-I4, STORE-DL. Also closes the CI-02 review nit left by #112 (
jqprerequisite).Important
Operator step for MySQL / MariaDB deployments of
oauth2/store/sql.Migratestill never alters a table.Existing MySQL and MariaDB databases get byte-exact keys only through the manual upgrade in
MIGRATION.md → "
oauth2/store/sql: MySQL compares keys byte for byte":ALTERs, then anINSERTthat records orphaned look-alike families as revoked, with token writesstopped if the preflight counted merged tokens;
PostgreSQL and SQLite need nothing. The store runs before, during and after the conversion. Each
ALTERrebuildsits table: MySQL 8 blocks that table's writes meanwhile.
STORE-04 — concurrent
Migratefailed on PostgreSQL (all three SQL stores)Problem.
Migrateat boot, and the godoc said "safe to call repeatedly".CREATE TABLE IF NOT EXISTSof one table all find it missing. Thelosers then fail on the catalog's row-type index:
duplicate key value violates unique constraint "pg_type_typname_nsp_index"(SQLSTATE 23505).TestMigrateConcurrently/postgres, red phase): 90 of 120 calls failed over 10 rounds of 12 instancesmigrating the three stores at once.
re-prepared statements), showed 0 failures.
Fix (
oauth2/store/sql,oidc/client/store/sql,saml/sp/store/sql).Dialectgets an unexportedmigrationLock().Migrateruns in one transaction that first takes the transaction-level advisory lockpg_advisory_xact_lock(<key>):"<module path>.Migrate"). Advisory locks are per database (PostgreSQL 16§13.3.5, §54.12), so each module gets its own key, and each key is documented so that a migration tool can take
the same lock:
-2450120788798466590-33465659682724687581266919442082856213implicitly (MySQL 8.0 §15.3.3).
Breaking change: none (behaviour only). MIGRATION.md notes the lock for migration tools. An engine without
pg_advisory_xact_lockappliesSchemaitself.STORE-I1 — MySQL collations made key comparisons inexact (
oauth2/store/sql)Problem.
utf8mb4_0900_ai_ci,utf8mb4_uca1400_ai_ci) fold case, accents, canonically equivalentforms and
ß/ss;utf8mb4_binincluded) also ignores trailing spaces.one joined the other's record. This breaks I7 of
oauth2.Storage, and the docs claimed text was "comparedexactly".
KeysAreComparedExactlyfailed on MySQL 8.0, MySQL 8.4 (with andwithout CLIENT_FOUND_ROWS) and MariaDB 11.4: revoking
fAM-cASE-aZ09revokedFam-Case-Az09.Fix.
Schema(MySQL)declares the six key columns (code hash, token hashes, family IDs)VARBINARY, through anunexported
keyTypedialect method. MySQL and MariaDB compare binary strings byte for byte (MySQL 8.0 §13.3.3,§12.8.5).
utf8mb4_binis rejected: it still merges trailing spaces;utf8mb4_0900_bin(8.0.17+) is absent from older MariaDB.Breaking change: the MySQL upgrade above. The upgrade revives no revoked family:
INSERTrecords revoked every family the collation had merged and that has no byte-exact record of its own;The opt-in suite runs the very SQL blocks of MIGRATION.md on MySQL 8.0, MySQL 8.4 and MariaDB 11.4:
Custom stores must compare keys byte for byte:
storetest.RunConformancenow checks it.STORE-I4 — the I3 contract ignored lost commit acknowledgements
The I3 invariant said a failed
SaveTokenPair/RotateRefreshToken"persists no token and consumes nothing". Thatholds for a call the store refuses. It does not hold for a backend failure reported after the commit was sent: the
outcome is unknown, every write may have been applied, never a part of them.
What changed:
ConsumeAuthorizationCodegodoc and the security considerations now say so.theft".
TestRotationWithALostCommitAcknowledgementpins this. It uses a SQLite driver whose COMMIT succeeds and thenreports a lost acknowledgement.
STORE-DL — documented limitations re-checked
Every listed limitation is accurately documented: Redis eviction, Redis < 6.0, the SQLite busy timeout, no expiry job
and index, Redis Cluster, CI coverage.
What changed:
expires_at, readsas an unknown token. Its reuse is refused but no longer detected, and its family is not revoked. Added to the
Redis and SQL package docs and to "Fail closed".
Tests
internal/interop/sqlstores, withcompose.yamlpinning postgres:16,mysql:8.0, mysql:8.4 and mariadb:11.4 by digest, and opt-in
SQLSTORES_*_DSNvariables:TestMigrateConcurrently: the 3 stores at once, 5 × 12 instances, on SQLite (also in CI), PostgreSQL (defaultand SERIALIZABLE default isolation), MySQL 8.0, MySQL 8.4 and MariaDB.
TestMigrateWaitsForItsLock: the documented key holdsMigrateback, keys are distinct, and aMigratethatignores its context fails instead of hanging.
TestMigrateFailureCreatesNothing: no table left, connection released, no "idle in transaction (aborted)".TestOAuth2Conformanceon every backend.TestOAuth2MySQLUpgrade.MockDialect: lock first, rollback, commit and begin failures,connection released after a failure;
TestSchemaPerDialect,TestDialectKeyType;KeysAreComparedExactly: case, accent, Unicode form, ß/ss and trailing-space family IDs, plus case andtrailing-space token and code hashes, on memory, SQLite and miniredis.
KeysAreComparedExactlyon MySQL/MariaDB, missing upgrade section).override. A mutant counts as killed only by a failing test.
deferred
Rollback, a context-ignoringMigrate, PAD SPACE hash columns, trimmed hashes, either half of theINSERT'sUNION, the retention floor, strict mode.utf8mb4_0900_binkeys (NO PAD, byte-exact).Migratepath. With one table alreadypresent, 300 of 320 concurrent calls fail with
SQLITE_BUSY/SQLITE_BUSY_SNAPSHOT(0 of 320 on theshipped per-statement path); the lock pin test catches it.
Checks
make generate sync build test lint check-modules vuln. All pass: lint 0 issues,check-modules ok, govulncheck no finding.
0 SKIP.
oauth2/store/sqloidc/client/store/sqlsaml/sp/store/sqloauth2/storetestoauth2oauth2/storage/memoryoauth2/store/redis(doc only)internal/interop/sqlstoresEvery new function is at 100 %.
Follow-ups (out of scope)
ci:PR, as decided.internal/interop/samlonly (alsoused by oidcstack and sqlstores). The CLAUDE.md change in this PR is limited to the SAML text and the new module's
row.
Notes for the maintainer
internal/interop/sqlstoresrow) is the orchestrator's, asapproved.
SQLSTORES_*_DSNvariables listed in the compose header, and rungo test -race -count=1 ./...ininternal/interop/sqlstores.