Repository navigation
fix(sqlstore)!: harden sealing keys, SQLite mode, resource server and client key wiring - #778
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
AESGCMSealer checked only a key's length, so an all-zero placeholder, a passphrase or a hex or base64 key passed undecoded sealed every record under a key anyone could guess: no confidentiality, and anyone who could write to the database could forge sealed records. FAPIgo's other sealers refuse such keys; AESGCMSealer now makes the same check (copied, since sqlstore uses FAPIgo's public API only), and RotatingSealer refuses two AESGCMSealers over the same key. BREAKING CHANGE: AESGCMSealer returns an error for a key that is one byte repeated or made entirely of printable text, which v0.3 accepted. Generate the key with crypto/rand and decode an encoded key before passing it. The README's Upgrading from v0.3 section shows how to keep reading the records sealed under the old key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SQLite opened without the documented pragmas passed every check, then failed most concurrent requests with server_error: outside WAL mode, a read made while a write commits fails at once unless its connection has a busy timeout, and Options.SQLiteBusyTimeout only ever applied to sqlstore's own writes. Migrate and CheckSchema now read PRAGMA journal_mode and refuse anything but wal, quoting the DSN to open the file with, and the docs say which busy timeout covers reads. BREAKING CHANGE: on SQLite, Migrate and CheckSchema return an error unless the database is in WAL mode, which v0.3 accepted. Open the file with both pragmas in its DSN, busy_timeout first, as in file:fapigo.db?_pragma=busy_timeout(10000)&_pragma=journal_mode(WAL). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… stores A resource server running apart from the authorization server needs two DBs: the server's, for Revocation, Replay and AccessTokens, and one with a TablePrefix of its own, for Nonces. Both have every store method, so the shortest wiring, everything from the resource server's own DB, type-checked and passed production checks while its Revocation read a table the server never writes: a revoked grant's tokens kept working until they expired. ResourceServer(server, own) returns the four stores, each from the right DB, and refuses a nil DB or two DBs on one TablePrefix. The package documentation and README show it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The clients table had no place for a client's keys, so every client was registered twice: its configuration in the table, its keys in a keys.ClientKeySource built elsewhere, typically at startup, so a client added with Put couldn't authenticate until a restart, and the two registries could drift. The client configuration document gains optional jwks_uri and jwks members (ClientKeyRegistration; version 1 takes new optional members), written with ClientRepository.PutWithKeys or MarshalClientConfigWithKeys and read with ParseClientKeys. Both refuse a registration with both members (RFC 7591 §2), a jwks_uri that isn't https, and a jwks that isn't a JWK Set with a usable key, or that the document's strict decoding would refuse. DB.ClientKeys returns a keys.ClientJWKSSource whose lookup reads the client's row on every resolution, fetching a jwks_uri through the given fapihttp.Client, so it declares LiveFetchHardened exactly as that source does. A row whose key members are malformed is an outage (ErrClientConfig wrapping storage.ErrStoreUnavailable), as for its other members. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ClientRepository.Put checks a client only as storage.NewRegisteredClient does, and the operator tooling writing the clients table often holds no *server.Server, so a client the server can't honour (an unregistered RAR type, AllowedResources without resource indicators, CIBA ping without a notifier) went unnoticed until its first request failed. The package documentation and README now say to run server.Server.CheckClientRegistration on every client at startup and after each Put, and ClientRepository.All lists the clients for that sweep, yielding a bad row's error in its place. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eckClientRegistration GETTING_STARTED's durable-storage section now says where a sqlstore server's ClientKeys come from (db.ClientKeys) and that a standalone resource server takes its stores from sqlstore.ResourceServer, so its Revocation can't come from its own prefix. The production checklist's sqlstore section gains running CheckClientRegistration on every client at startup and after each Put. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
osanderson
force-pushed
the
fix/sqlstore-hardening
branch
from
November 2, 2026 07:16
8c8787f to
1674aea
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.
sqlstore changes for its v0.4.0 release, from a developer-experience review of an integrator building an authorization server, a standalone resource server and a relying party on sqlstore.
Breaking (README "Upgrading from v0.3")
AESGCMSealerrefuses a key that is one byte repeated or all printable text (a placeholder, a passphrase, or a hex/base64 key not yet decoded), as FAPIgo's other sealers do;RotatingSealerrefuses twoAESGCMSealers over the same key. The check is copied frominternal/sealkeysrather than imported, since CI requires sqlstore to use FAPIgo's public API only (a released sqlstore must build against any later FAPIgo). The upgrade note shows a smallSealerfor reading records sealed under an old weak key while rotating.MigrateandCheckSchemarefuse a database whosePRAGMA journal_modeisn'twal, quoting the DSN to use. Without WAL, reads made while a write committed failed at once (the busy timeout only covered sqlstore's own writes), and most concurrent requests failed asserver_error.Options.SQLiteBusyTimeout's doc now says it covers writes, and reads take the DSN'sbusy_timeout.New
sqlstore.ResourceServer(server, own)returns a standalone resource server'sRevocation,ReplayandAccessTokensfrom the server's DB andNoncesfrom its own, refusing a nil DB or one sharedTablePrefix. Taking everything from the resource server's own DB used to type-check and pass production checks while its revocation table was never written: revoked grants' tokens kept working.jwks_uri/jwksmembers (ClientKeyRegistration,PutWithKeys,MarshalClientConfigWithKeys,ParseClientKeys), anddb.ClientKeys(fetcher)returns akeys.ClientJWKSSourcereading them from the client's row, so the clients table is the one registry and a client added at runtime authenticates without a restart. Malformed key members are refused on write and treated as an outage on read; the source'sLiveFetchHardenedfollows the fetcher.Clients().All(ctx)iterates every client; the docs say to runServer.CheckClientRegistrationon every client at startup and after eachPut.Docs outside sqlstore (one
docs:commit): GETTING_STARTED's durable-storage section namesdb.ClientKeysandsqlstore.ResourceServer; PRODUCTION_CHECKLIST's sqlstore section gains theCheckClientRegistrationitem.Not done: detecting a resource server that reads revocations from a prefix the server never writes. A fresh deployment's tables are empty too, so there is no reliable signal.
Tests cover the weak-key and WAL refusals (both fail on main),
ResourceServer,keys.TestClientKeySourceContractoverClientKeys, runtime rekeying, jwks_uri fetching, malformed rows,All, and production acceptance with a hardened fetcher and refusal without one.🤖 Generated with Claude Code