Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -946,7 +946,32 @@ legacy packages (`authentication/`, `authorization/`, the in-tree
the former `BOOLEAN NOT NULL DEFAULT 0` column and the store's integer
comparisons — and the token-family indexes are gone, since MySQL rejects
their `CREATE INDEX IF NOT EXISTS`. `Schema` returns the same DDL for
every dialect.
every dialect but for MySQL's key columns (see Security).
- The `Migrate` of the SQL stores (`oauth2/store/sql`,
`oidc/client/store/sql`, `saml/sp/store/sql`) can run concurrently, as
every instance of a deployment starting at once does. On PostgreSQL,
concurrent `CREATE TABLE IF NOT EXISTS` of one table found it missing in
every session, and all but one then failed (`duplicate key value violates
unique constraint "pg_type_typname_nsp_index"`, SQLSTATE 23505). There
`Migrate` now runs in one transaction that first takes a transaction-level
advisory lock (`pg_advisory_xact_lock`), one per store module, documented
in each package: concurrent calls run one after the other, a waiting call
waits as long as its context allows, and a failed call creates nothing.
MySQL, MariaDB and SQLite (with the busy timeout the stores require)
already serialized the creation of a table, and run as before. The opt-in
suite of the new private module `internal/interop/sqlstores` checks the
four engines (PostgreSQL 16, MySQL 8.0 and 8.4, MariaDB 11.4, from its
`compose.yaml`).
- The `oauth2.Storage` contract no longer says that a failed
`SaveTokenPair` or `RotateRefreshToken` persists nothing (I3). When the
acknowledgement of a commit is lost, the call fails although all its
writes may have been applied — never a part of them — and a client
retrying the refresh then presents a consumed token: a reuse, which
revokes the family (`docs/security-considerations.md`, "Lost responses
look like theft"). A test of the SQL store pins it. The documentation now
also says that a consumed refresh token evicted from Redis, or deleted
from SQL before its `expires_at`, loses its reuse detection: presented
again, it reads as an unknown token and revokes nothing.
- A failed family revocation after a refresh-token reuse the store detects
is no longer swallowed: the SQL and Redis stores returned the bare
`ErrRefreshTokenReused` whatever the revocation did. The error now also
Expand Down Expand Up @@ -1802,3 +1827,19 @@ legacy packages (`authentication/`, `authorization/`, the in-tree
for it on a `Manager` of its own (cookie `__Host-signin`), cross-origin
posts are refused (`http.CrossOriginProtection`) and the credentials go
through `basic.Authenticator`.
- The OAuth2 SQL store (`oauth2/store/sql`) compares its keys byte for byte
on MySQL and MariaDB. Those engines compared the token-family IDs and the
code and token hashes under the collation of their columns, and the
default collations ignore letter case and accents and take canonically
equivalent forms (and `ß` / `ss`) for one character; MariaDB's, like
every PAD SPACE collation (`utf8mb4_bin` included), also ignore trailing
spaces. Two families whose IDs differed only that way were one: revoking
one revoked the other, and a pair issued into one joined the other's
record (the family isolation, I7, of `oauth2.Storage`). The library's own
family IDs and hashes never differ that way; those of custom grants can.
`Schema(sqlstore.MySQL)` declares the six key columns `VARBINARY`; an
existing MySQL database is converted with the statements of MIGRATION.md,
which also record revoked the families such a merge left without a
record, so that none is revived.
`storetest.RunConformance` checks that every store compares its keys byte
for byte (`KeysAreComparedExactly`).
26 changes: 17 additions & 9 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down Expand Up @@ -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
Expand All @@ -412,23 +415,28 @@ 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
`security.ErrInvalidCredentials`); store/resolver failures do not.
- `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
Expand Down
14 changes: 9 additions & 5 deletions LIMITATIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,19 @@ future refactor phases.
anything: consumed and expired codes, expired tokens and revoked
families stay for the process lifetime. It is meant for development and
tests only.
- **PostgreSQL and MySQL are not exercised in CI** — the SQL store's tests
run on SQLite only; its PostgreSQL and MySQL guarantees (row locks,
- **PostgreSQL and MySQL are not exercised in CI** — CI runs the SQL
store on SQLite only; its PostgreSQL and MySQL guarantees (row locks,
READ COMMITTED on PostgreSQL, InnoDB on MySQL) are argued from their
locking semantics, not tested.
locking semantics. The opt-in suite of `internal/interop/sqlstores`
(PostgreSQL 16, MySQL 8.0 and 8.4, MariaDB 11.4, from its `compose.yaml`)
runs the conformance suite, `Migrate` concurrently and the MySQL upgrade
statements on them, locally only.
- **SQL schema upgrades are manual** — `Store.Migrate` creates missing
tables and never alters an existing one: the columns a release adds (#80's
`consumed`, #83's `issuer` and `extension_data`, the codes'
`redirect_uri_omitted`, the refresh tokens' `family_issued_at`) are added
with the statements MIGRATION.md lists, through your migration tool.
`redirect_uri_omitted`, the refresh tokens' `family_issued_at`) and the
binary key columns of MySQL are added or converted with the statements
MIGRATION.md lists, through your migration tool.
- **A leaked code can be burned** — codes are consumed before they are
validated, so a party holding a code (but not its client's credentials or
PKCE verifier) can make the legitimate exchange fail with
Expand Down
126 changes: 126 additions & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -2249,3 +2254,124 @@ To migrate:
relay the token must read the request's `Authorization` field. A custom
`TokenVerifier` should build its result with
`bearer.New(token).WithAuthenticated(...)`, which drops the token.

### SQL stores: `Migrate` takes an advisory lock on PostgreSQL

Nothing to migrate. On PostgreSQL, the `Migrate` of `oauth2/store/sql`,
`oidc/client/store/sql` and `saml/sp/store/sql` now runs in one transaction
that first takes a transaction-level advisory lock, so that every instance
of a deployment can call it at boot: concurrent `CREATE TABLE IF NOT EXISTS`
of one table used to fail (SQLSTATE 23505). A migration tool that applies
`Schema` while instances may run `Migrate` should take the same lock, named
in each package's documentation (`pg_advisory_xact_lock(-2450120788798466590)`
for `oauth2/store/sql`, `(-3346565968272468758)` for `oidc/client/store/sql`,
`(1266919442082856213)` for `saml/sp/store/sql`). An engine without
`pg_advisory_xact_lock` cannot use `Migrate`: apply `Schema` with your own
tool.

### `oauth2/store/sql`: MySQL compares keys byte for byte

On MySQL and MariaDB the SQL store compared its keys — token-family IDs,
code and token hashes — under the collation of their columns. The default
ones ignore letter case and accents and take canonically equivalent Unicode
forms (and `ß` / `ss`) for one character: `utf8mb4_0900_ai_ci` on MySQL 8,
`utf8mb4_uca1400_ai_ci` on MariaDB 11. Those of MariaDB, like every PAD
SPACE collation (`utf8mb4_bin` included), also ignore trailing spaces. Two
families whose IDs differed only that way were one family: revoking one
revoked the other, and a pair issued into one joined the other's record —
against the isolation the `oauth2.Storage` contract promises (I7). The
library's own family IDs (random or derived, base64url) and its hashes
(lowercase hex) never differ that way; the family IDs of custom grants can.
`Schema(sqlstore.MySQL)` now declares the six key columns `VARBINARY`, which
MySQL and MariaDB compare byte for byte; PostgreSQL and SQLite already did.

| | Before | After |
| -------------------------------------------------------------- | -------------------------------------------- | ------------------------------------------------ |
| MySQL key columns (`code_hash`, `token_hash` ×2, `family_id` ×3) | `VARCHAR(n)`, compared under the collation | `VARBINARY(n)`, compared byte for byte |
| PostgreSQL and SQLite tables | `VARCHAR(n)`, compared byte for byte | unchanged |
| `Migrate` on an existing MySQL database | creates the missing tables | unchanged: it never alters a table (steps below) |
| A token merged into a look-alike family's record | shares that family's state | reads revoked once upgraded |
| `storetest.RunConformance` | — | checks that a store compares keys byte for byte |

To migrate (MySQL and MariaDB only; PostgreSQL and SQLite need nothing):

1. **Preflight**, on the tables as they are, before any `ALTER`:

```sql
-- Tokens merged into the record of a look-alike family:
-- access tokens, refresh tokens (expected: 0, 0).
SELECT
(SELECT COUNT(*) FROM oauth2_access_tokens t
JOIN oauth2_token_families f ON f.family_id = t.family_id
WHERE CAST(t.family_id AS BINARY) <> CAST(f.family_id AS BINARY)),
(SELECT COUNT(*) FROM oauth2_refresh_tokens t
JOIN oauth2_token_families f ON f.family_id = t.family_id
WHERE CAST(t.family_id AS BINARY) <> CAST(f.family_id AS BINARY));
-- The longest keys, in bytes: family IDs at most 64, hashes at most 128.
SELECT
(SELECT COALESCE(MAX(LENGTH(family_id)), 0) FROM oauth2_token_families),
(SELECT COALESCE(MAX(LENGTH(family_id)), 0) FROM oauth2_access_tokens),
(SELECT COALESCE(MAX(LENGTH(family_id)), 0) FROM oauth2_refresh_tokens),
(SELECT COALESCE(MAX(LENGTH(code_hash)), 0) FROM oauth2_auth_codes),
(SELECT COALESCE(MAX(LENGTH(token_hash)), 0) FROM oauth2_access_tokens),
(SELECT COALESCE(MAX(LENGTH(token_hash)), 0) FROM oauth2_refresh_tokens);
```

The counts are 0 unless custom family IDs differ only by case, accents,
Unicode form or trailing spaces. When they are not, stop token writes —
issuance, rotation, revocation — during step 2: between its `ALTER` of
`oauth2_token_families` and its `INSERT`, an issuance into such a family
would record it active and revive the tokens it shares with a revoked
family. A family ID over 64 bytes or a hash over 128 makes step 2 fail
(it runs in strict mode): delete those rows first. The store refuses
such family IDs since #82, and its own hashes are 64 characters.
2. **Convert the key columns and record the orphaned families revoked**, in
one session — with token writes stopped when step 1 counted merged
tokens:

```sql
SET SESSION sql_mode = CONCAT_WS(',', NULLIF(@@SESSION.sql_mode, ''), 'STRICT_ALL_TABLES');
ALTER TABLE oauth2_access_tokens MODIFY token_hash VARBINARY(128) NOT NULL, MODIFY family_id VARBINARY(64) NOT NULL;
ALTER TABLE oauth2_refresh_tokens MODIFY token_hash VARBINARY(128) NOT NULL, MODIFY family_id VARBINARY(64) NOT NULL;
ALTER TABLE oauth2_token_families MODIFY family_id VARBINARY(64) NOT NULL;
INSERT INTO oauth2_token_families (family_id, revoked_at, expires_at)
SELECT o.family_id, UNIX_TIMESTAMP(), GREATEST(MAX(o.expires_at), UNIX_TIMESTAMP() + 86400)
FROM (SELECT family_id, expires_at FROM oauth2_access_tokens WHERE family_id <> ''
UNION ALL
SELECT family_id, expires_at FROM oauth2_refresh_tokens WHERE family_id <> '') o
WHERE NOT EXISTS (SELECT 1 FROM oauth2_token_families f WHERE f.family_id = o.family_id)
GROUP BY o.family_id;
```

The `INSERT` runs after the `ALTER`s — before them, the look-alike would
collide with its family's key. It records revoked every family that
tokens name but that has no record of its own once keys compare byte for
byte: the look-alikes the collation merged, and families whose record
was lost (tokens issued before #82, a premature cleanup). Their tokens
already read revoked (I8); the record keeps a later issuance from
recording them active, for `oauth2.FamilyRevocationRetention` (86400 s)
at least. A look-alike that shared an *active* family's record therefore
reads revoked: its users sign in again.
3. **Optionally, convert the codes' table.** The library's code hashes are
lowercase hex, which no collation confuses, so this only matters for
hashes of your own. It rebuilds a table `/authorize` writes to: run it at
a quiet time (`Migrate` creates it binary on a new database).

```sql
ALTER TABLE oauth2_auth_codes MODIFY code_hash VARBINARY(128) NOT NULL;
```

4. **Plan the window.** Each `ALTER` rebuilds its table. MySQL 8.0 and 8.4
copy it and block its writes meanwhile (`ALGORITHM=COPY`, `LOCK=SHARED`).
MariaDB 11.2 and later can rebuild it with `LOCK=NONE`. While
`oauth2_token_families` is rebuilt, every issuance, rotation and
revocation waits. Run it at a quiet time, or use an online schema-change
tool with the same column definitions. The store works before, during
and after the conversion, and comparisons become exact table by table:
once a token table is converted, its tokens merged into a look-alike's
record already read revoked.
5. **Read the keys** with `CAST(family_id AS CHAR)`: the interactive
`mysql` client shows binary columns in hexadecimal (`--binary-as-hex`,
on by default in interactive mode since 8.0.19).
6. **Custom stores** compare family IDs and hashes byte for byte, and pass
`storetest.RunConformance`, which now checks it (`KeysAreComparedExactly`).
Loading
Loading