Repository navigation
fix(stores)!: close the 2026-10-21 security audit findings of the stores - #137
Merged
Merged
Conversation
euskadi31
force-pushed
the
fix/audit2-stores
branch
from
October 21, 2026 20:58
2b8349c to
16805fa
Compare
The real-backend oauth2 conformance runs gave each case a pool with a single warm connection: the 50 racers of the concurrent code consumption and the 30 of the concurrent rotation queued on connection setup and never overlapped. A check-then-act code consumption (the previous audit's M19) and a rotation without either row lock passed on PostgreSQL. Each case now gets a pool whose eight connections are open and idle before it starts; the factory closes the previous case's pool, so a run holds eight connections, not one set per case. Both mutants are now killed on PostgreSQL (M19 on every MySQL too; the rotation mutant is equivalent on MySQL, where the family upsert already locks the row). Audit: F-24.
oauth2.Storage says every store MUST pass the storetest suite, but the suite spot-checked a few fields of token lookups and checked retention only on the next call. A store whose lookups returned another client ID, a broader scope, another subject or an expiry a century later, or one keeping a consumed code, a rotated refresh token or a revoked family for a second only, passed every case: such a store silently loses client binding, expiry, code-replay revocation (RFC 6749 section 4.1.2) and refresh-token reuse detection (RFC 9700 section 4.14.2). Every token lookup of the suite now compares the full record — every field, timestamps to the second, the consumed state the case expects, never the raw token — and names the fields that differ. Two cases join: TokenRecordsRoundTrip, on pairs whose fields all differ, and RetainedOverTime, which checks a consumed code, a rotated refresh token, families revoked before and after issuance and a live pair again 1.5 s later. A self-test runs the suite, in a process of its own, against in-memory stores that lie about one field or forget what they must retain, and asserts that the case pinning it fails. Audit: F-23.
The SAML Redis store set each key's expiry with PEXPIREAT, an instant computed on the SP's clock. Redis compares it with its own clock: when Redis runs ahead of the SP by more than a replay mark's retention (the SP keeps marks at least 60 s past an assertion's validity), the instant is already past and Redis deletes the mark as it is written, so the same assertion is accepted again. SAML 2.0 Profiles section 4.1.4.5 requires the SP to keep the used assertion IDs for as long as the assertions are valid. The store now sends durations: a replay mark is one SET ... NX PX, a transaction gets PEXPIRE, each with the time left until the record's expiry on the SP's clock, at least a second. The offset of Redis's clock no longer matters. WithClock names the clock the lifetimes are measured on, for an SP whose samlsp.Config.Now is injected (default time.Now). The recorded expiry, which the MAC covers, is unchanged. Tests reproduce the skew on miniredis (its clock set two minutes ahead and behind) and on a real Redis (the SP clock injected two minutes off the server's TIME); scripts/test-backends.sh runs the package. Audit: F-19.
The TransactionStore contract says Save and Consume refuse, before any I/O, what Transaction.Validate refuses, but no store validated the keys given to Consume. On MySQL, whose VARCHAR key is compared under a case-insensitive, space-padding collation, an upper-case state hash consumed the transaction saved under the canonical one. Client.Complete always passes canonical hex, so a login could not be hit this way, but a third-party caller could, and the contract was not met. oidcclient.ValidateStoreKey refuses anything but 64 lowercase hex digits with ErrInvalidTransaction, mirroring samlsp.ValidateStoreKey. The memory, SQL and Redis stores call it on both keys first. A conformance case, ConsumeRefusesInvalidKeys, pins the contract for every store. Audit: I-23.
The MySQL DDL of the three SQL stores named no storage engine. On a server whose default engine is MyISAM, Migrate silently created tables without transactions or row locks: the oauth2 store then lost the atomic writes of its contract (I3) — a rotation cut short left the presented refresh token consumed without its replacement — and a consumption acknowledged before a crash could be lost. The documented requirements (InnoDB, a strict sql_mode, binary key columns, Redis noeviction, no Redis Cluster) were checked nowhere: a non-strict MySQL session truncates an over-long subject or client ID, and a Cluster client was accepted and failed at runtime. - The MySQL DDL names ENGINE=InnoDB DEFAULT CHARSET=utf8mb4. - The three SQL stores gain CheckDeployment and ErrDeploymentRequirement. On MySQL it reports every store table that is missing or not InnoDB and, for oauth2, each key column that is not VARBINARY and a non-strict sql_mode. PostgreSQL and SQLite report nothing it checks. - Migrate stays create-only but ends with CheckDeployment, so such a deployment is refused at boot. MIGRATION.md gives the statements that convert existing tables; a real-backend test runs them. - oauth2/store/redis: New refuses a *redis.ClusterClient or *redis.Ring, whose sharding the multi-key scripts cannot work with, and CheckDeployment verifies through INFO Redis 6.0 or later, the noeviction policy and no cluster mode. An INFO the server refuses (NOPERM without +info) is returned as such, a field it does not report counts as unmet. BREAKING CHANGE: on MySQL, Migrate fails with ErrDeploymentRequirement on MyISAM tables, on legacy VARCHAR key columns of the oauth2 store and on a non-strict sql_mode for the oauth2 store; oauth2/store/redis.New refuses Cluster and Ring clients. Audit: F-18, I-24, D-25.
A rotation of the SQL token store rests on three layers: the family row lock, the presented refresh token's row lock and the consuming UPDATE that must affect exactly one row. Each alone keeps two rotations of one token from both succeeding, so the real-backend suites passed with any single layer removed and nothing pinned any of them. They stay — the last one is what kept rotations single when the locks were no-ops on MyISAM tables — and each gets a deterministic test. On PostgreSQL, MySQL and MariaDB a test transaction holds the family row, or the token row, while a rotation starts; the server is polled for a session blocked by that very transaction, then the holder revokes the family, or consumes the token, and commits: the rotation must report the revoked family, or the reuse. On SQLite a trigger makes the consuming UPDATE affect no row: the rotation must fail and persist nothing. The Store godoc names the layers. Audit: I-25.
The oauth2 and SAML SQL stores shipped no expiry job: codes, tokens, families, login transactions and replay marks stayed until an operator wrote a delete job, which the package documentation only described. PurgeExpired(ctx, now) deletes the rows expired before now, in batches of 1000, each its own transaction — at READ COMMITTED on PostgreSQL and MySQL — oauth2 codes and tokens before families. Each batch re-checks the expiry of the rows it deletes on their latest version (PostgreSQL and SQLite repeat it on the outer DELETE: a batch picking a family in its subquery, then waiting for a revocation extending that family, would otherwise delete a revoked family before its new expiry, and the next issuance would revive it). On MySQL, READ COMMITTED keeps an unindexed batch from locking the gaps it scans, which would block the inserts of /authorize, /token and the SAML ACS. The OIDC store's existing PurgeExpired runs at READ COMMITTED for the same reason. The package documentation gives the expires_at indexes to add. Real-backend tests run the purges on every server, hold a revocation across a purge, and insert while a purge waits for a locked row. Audit: D-25.
CHANGELOG entries for the store fixes and additions; MIGRATION.md sections for the Redis client refusal and the Consume key validation; LIMITATIONS.md: Redis Cluster now refused or detected, expired-row cleanup through PurgeExpired, the reach of the deployment checks (synchronous_commit stays unchecked: it can be set per role, session or transaction); the security considerations' store assumptions and operator checklist call CheckDeployment at boot and schedule the purges. Audit: F-18, F-19, I-23, I-24, D-25.
On MySQL and MariaDB, Migrate ran its CREATE TABLE IF NOT EXISTS statements, then its deployment check, with nothing serializing the instances of a deployment booting at once: concurrent creations of one table can fail with a metadata-lock deadlock (1213), as the concurrent Migrate test hit once. MySQL commits every DDL statement on its own, so the transaction-level advisory lock Migrate takes on PostgreSQL has no counterpart in a transaction. Migrate now creates the tables and runs the check on one connection holding a named lock of its module (GET_LOCK "hyperscale-stack/security/<module>.Migrate"), waiting as long as its context allows — the server's wait follows the deadline, and the driver cuts it when the context ends — and releases it before the connection goes back to the pool; a release the server refuses discards the connection. The deployment check takes a querier, so that it runs on the locked connection and a one-connection pool cannot deadlock. Tests: on MySQL 8.0/8.4 and MariaDB, a held lock makes Migrate wait until its deadline or its cancellation, having created nothing, while another store's Migrate goes through; the lock is free after a Migrate, failed or not. On SQLite, fake GET_LOCK and RELEASE_LOCK functions pin the acquisition, the wait bound, the release and the refusals. Audit: F-18, I-24.
The MySQL deployment checks read information_schema for the tables of the connection's database only, but nothing pinned that scope: every real-backend test ran alone on its server, so a check reading every database's tables passed them all. On a server holding one database per environment or tenant, such a check would let a compliant copy elsewhere hide a missing, MyISAM or text-keyed table, and Migrate let through what it must refuse. A test now keeps a compliant copy of every store in one database and checks, each in a database of its own, missing tables, MyISAM tables, an oauth2 families table without its key column and the former text keys: each is reported, whatever order information_schema returns its rows in. Audit: F-18, I-24.
The warmed conformance pool opened its eight connections under one 30-second context: a connect that stalled once, through Docker Desktop's port forwarding, failed the case after 30 s. Each connection is now opened with up to three attempts of 10 s, and a server that never answers fails the case with an error naming the connection and the attempts. Audit: F-24.
RetainedOverTime re-checks, after 1.5 s, both a family revoked before any issuance and one revoked after tokens were issued into it, but the self-test's liar forgot every revoked family and failed on the first check: the second never ran under any liar. A store keeping the revocation of a family with tokens under another lifetime than that of an unknown one — the Redis revocation script has such a branch — would lose exactly those, reviving their tokens (I5). A liar forgetting only the families revoked after issuance now fails RetainedOverTime on "issue into the revoked family, later". The re-checks the self-test does not pin on their own are documented as sanity checks. Audit: F-23.
When the GET_LOCK query of Migrate failed, the connection went back to the pool. The driver drops a connection whose query was cancelled, but a lost reply after the server granted the lock would have pooled a session holding the server-wide migration lock, blocking every later Migrate. Such a connection is now discarded, as one whose release fails already was; a lock refused for sure (0) keeps its connection. The Migrate godoc says how to find and end a holder whose client vanished (IS_USED_LOCK, KILL), and that Galera and Percona XtraDB clusters keep named locks per node. Audit: F-18, I-24.
- security considerations: separate checklist items for SQL stores whose schema a migration tool applies and for oauth2/store/redis (+info); the SAML item mentions CheckDeployment. - MIGRATION.md: a DSN sql_mode of TRADITIONAL, which keeps NO_ENGINE_SUBSTITUTION; how to end a stuck migration-lock holder; named locks are per node on Galera and Percona XtraDB clusters. - CHANGELOG: the OIDC purge is refused under binlog_format=STATEMENT. - LIMITATIONS: the OIDC purge runs at READ COMMITTED. Audit: F-18, I-24, D-25.
Rebasing over other entries left blank lines inside the Added, Changed, Fixed and Security lists. Audit: F-18, F-19, I-23, I-24, D-25.
euskadi31
force-pushed
the
fix/audit2-stores
branch
from
October 21, 2026 21:20
16805fa to
0cc9fa4
Compare
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
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-21 (commit 1551aa6): F-18, F-19, F-23, F-24 (
oauth2/store/sqlpart), I-23, I-24, I-25, D-25.F-18 — MySQL tables created MyISAM; atomic writes lost
Problem. The MySQL DDL of
oauth2/store/sql,oidc/client/store/sqlandsaml/sp/store/sqlnamed no storage engine. On a server whosedefault_storage_engineis MyISAM,Migratesilently created tables without transactions or row locks. The oauth2 store then lost the atomic writes ofoauth2.Storage(I3): a rotation cut short — cancelled context, dropped connection — left the presented refresh token consumed without its replacement (reproduced on MySQL 8.4 and MariaDB 11.4: 134 and 113 partial rotations out of 400 before, 0 after). MyISAM is not crash safe either.Fix.
ENGINE=InnoDB DEFAULT CHARSET=utf8mb4.CheckDeployment(ctx)/ErrDeploymentRequirementon the three SQL stores: on MySQL it reports every store table that is missing or not InnoDB.Migratestays create-only but ends withCheckDeployment.Migratecreates and checks under a per-module named lock (GET_LOCK "hyperscale-stack/security/<module>.Migrate"), like the PostgreSQL advisory lock: concurrentCREATE TABLE IF NOT EXISTSof one table can fail with a metadata-lock deadlock (1213). The wait is bounded by the context; a connection whose lock state is unknown is discarded.Breaking.
Migraterefuses a MySQL database with MyISAM or missing tables. MIGRATION.md gives theALTER TABLE … ENGINE=InnoDB(and optional utf8mb4) statements; a real-backend test runs them.F-19 — SAML Redis replay marks expired on Redis's clock
Problem.
saml/sp/store/redisset key expiries withPEXPIREAT, an instant computed on the SP's clock. With Redis ahead of the SP by more than a mark's retention, the mark was deleted as it was written and the same assertion was accepted again (SAML 2.0 Profiles §4.1.4.5: the SP MUST keep used IDs while the assertion is valid).Fix. Lifetimes are sent as durations (
SET … NX PX,PEXPIRE),max(1 s, expiresAt − now())on the SP's clock; newWithClockoption (give itsamlsp.Config.Nowwhen injected). No migration.F-23 — the oauth2 conformance suite did not pin token records or retention
Problem. A store whose lookups returned another client ID, a broader scope, another subject or an expiry a century later, or one keeping a consumed code, a rotated refresh token or a revoked family for one second, passed every case.
Fix. Every token lookup of
oauth2/storetestcompares the full record (field diff named on failure); new casesTokenRecordsRoundTripandRetainedOverTime(rechecks after 1.5 s; needs a backend whose time moves — miniredis does not). A committed self-test re-runs the suite in a subprocess against 14 lying in-memory stores (and an honest control) and asserts the case pinning each lie fails.F-24 (
oauth2/store/sql) — concurrency cases never raced on real backendsProblem. The real-backend conformance pool kept one warm connection, so the 50/30 racers never overlapped: a check-then-act code consumption passed on PostgreSQL.
Fix. Each case gets a pool whose 8 connections are opened beforehand (bounded retries for a stalled connect); the mutant is now killed on PostgreSQL and every MySQL server.
I-23 — OIDC
TransactionStore.Consumedid not validate its keysProblem. The interface says Consume refuses, before any I/O, what
Transaction.Validaterefuses; no store did. On MySQL an upper-case (or space-padded) state hash matched the canonical one under the column collation. Not reachable throughClient.Complete.Fix. New
oidcclient.ValidateStoreKey(mirrorssamlsp.ValidateStoreKey); the memory, SQL and Redis stores call it on both keys first; conformance caseConsumeRefusesInvalidKeys. Custom stores: see MIGRATION.md.I-24 — deployment requirements not checked at runtime
Fix (scope decided by the maintainer):
CheckDeploymentonoauth2/store/sql— MySQL: InnoDB tables, VARBINARY key columns, strictsql_mode(a non-strict session truncates an over-long subject or client ID), all scoped to the connection's database;oidc/client/store/sql,saml/sp/store/sql— MySQL: InnoDB;oauth2/store/redis— throughINFO: Redis ≥ 6.0,maxmemory-policy noeviction, no cluster mode; a refusedINFO(NOPERM without+info) is returned as such, a field the server does not report counts as unmet.Newrefuses*redis.ClusterClientand*redis.Ring(breaking; MIGRATION.md).PostgreSQL/SQLite: nothing checked (
synchronous_commitis per role/session, documented). OIDC/SAML Redis: no check (documented recommendations).I-25 — redundant locking layers untested
Decision: keep the three layers of a SQL rotation and test each: the family row lock and the presented token's row lock with holder-keyed lock-wait tests on PostgreSQL, MySQL 8.0/8.4 and MariaDB; the single-row consumption with a SQLite trigger. The
Storegodoc names them.D-25 — stores' operational gaps
PurgeExpired(ctx, now)onoauth2/store/sqlandsaml/sp/store/sql: batches of 1000, each a READ COMMITTED transaction (PostgreSQL, MySQL), oauth2 families last; the PostgreSQL/SQLite outerDELETEre-checksexpires_atso a family a concurrent revocation extends is kept. The existing OIDC purge runs at READ COMMITTED (MySQL's default isolation locked the gaps of an unindexed scan, blocking inserts).New), eviction policy, forgotten MySQL engine/key upgrades.expires_atindexes are applied by your tool (statements in the package docs); in-memory store growth; manual schema upgrades.Tests
TestKeyLifetimesIgnoreTheRedisClock/2m0s"… got nil" (replay accepted),TestMySQLMigrateCreatesInnoDBTables/*"expected InnoDB, actual MyISAM",TestMySQLMigrateWaitsForItsLock/*"… got nil",ConsumeRefusesInvalidKeyson every store,TestSQLPurgeExpired"expected 8, actual 0".FOR UPDATEand both row locks together on MySQL, where the family upsert already locks the row; the statement order of the purge, deadlock-only). MySQL-only mutants are killed by the real-backend suites.Checks
make generate sync build test lint check-modulesgreen (make syncleaves no diff);make test-backendsgreen (sqlstores 532, oidcstack 159, saml 286, oauth2/store/redis 112, saml/sp/store/redis 37; none skipped).Uncovered: MySQL-only
information_schemareaders and purge error branches, run bymake test-backends.Follow-ups
internal/interop/oidcstackpools keep one idle connection (the same weakness F-24 fixed for the oauth2 suite); warming them needs a pool-lifetime rework.Notes for the maintainer
scripts/test-backends.shsuite line:saml/sp/store/redis(real-Redis clock-skew test; CI already setsSAMLSP_REDIS_ADDR).Migrateon MySQL is serialized server-wide per module; on Galera/PXC named locks are node-local (documented).