From 934e4a2a59f04be8ef4416d6f8a96007694969fd Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Tue, 11 Aug 2026 21:34:42 -0500 Subject: [PATCH] appview: stop the crypto key first-boot race Two instances booting against a fresh database both find no key, both generate one, and both write. PutCryptoKey was last-writer-wins, so the loser kept its own key in memory while the database held the other's. It then signed OAuth client assertions with a key absent from the published JWKS, and issued registry JWTs that did not match the certificate written to disk. Every one of them fails verification, and nothing logs why. PutCryptoKey now keeps the first write, and both loaders re-read afterwards and use whatever is stored. Nothing in the codebase rotates a key through this function, so the update arm only ever fired on the race. Verified against the old behavior: with last-writer-wins restored, three of six concurrent loaders returned a key that was not the one in the database. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/appview/crypto_keys.go | 49 +++++++++- pkg/appview/crypto_keys_test.go | 156 ++++++++++++++++++++++++++++++++ pkg/appview/db/crypto_keys.go | 16 +++- 3 files changed, 215 insertions(+), 6 deletions(-) create mode 100644 pkg/appview/crypto_keys_test.go diff --git a/pkg/appview/crypto_keys.go b/pkg/appview/crypto_keys.go index 52519c1..b31e505 100644 --- a/pkg/appview/crypto_keys.go +++ b/pkg/appview/crypto_keys.go @@ -1,6 +1,7 @@ package appview import ( + "bytes" "crypto/rand" "crypto/rsa" "crypto/x509" @@ -42,9 +43,29 @@ func loadOAuthKey(database *sql.DB) (*atcrypto.PrivateKeyP256, error) { if err := db.PutCryptoKey(database, "oauth_p256", keyBytes); err != nil { return nil, fmt.Errorf("failed to store generated OAuth key in database: %w", err) } - slog.Info("Generated new OAuth P-256 key and stored in database") - return p256Key, nil + // Re-read rather than trusting the key we just generated. If another + // instance won the race to write first, PutCryptoKey kept theirs, and the + // stored key is the one the published JWKS will advertise. Using ours would + // mean signing client assertions with a key nobody can verify. + stored, err := db.GetCryptoKey(database, "oauth_p256") + if err != nil { + return nil, fmt.Errorf("failed to re-read OAuth key after storing: %w", err) + } + if stored == nil { + return nil, fmt.Errorf("OAuth key missing immediately after storing it") + } + key, err := atcrypto.ParsePrivateBytesP256(stored) + if err != nil { + return nil, fmt.Errorf("failed to parse stored OAuth key: %w", err) + } + if bytes.Equal(stored, keyBytes) { + slog.Info("Generated new OAuth P-256 key and stored in database") + } else { + slog.Info("Another instance stored an OAuth P-256 key first, using theirs") + } + + return key, nil } // loadJWTKeyAndCert loads the JWT RSA key from the DB and generates a self-signed @@ -91,9 +112,29 @@ func loadRSAKey(database *sql.DB) (*rsa.PrivateKey, error) { if err := db.PutCryptoKey(database, "jwt_rsa", keyPEM); err != nil { return nil, fmt.Errorf("failed to store generated RSA key in database: %w", err) } - slog.Info("Generated new JWT RSA key and stored in database") - return rsaKey, nil + // Re-read for the same reason as loadOAuthKey: whoever wrote first owns the + // key, and the certificate written to disk is derived from whatever we + // return here. Keeping our own copy after losing the race would mean issuing + // registry JWTs that do not match the advertised certificate. + stored, err := db.GetCryptoKey(database, "jwt_rsa") + if err != nil { + return nil, fmt.Errorf("failed to re-read JWT RSA key after storing: %w", err) + } + if stored == nil { + return nil, fmt.Errorf("JWT RSA key missing immediately after storing it") + } + key, err := parseRSAKeyPEM(stored) + if err != nil { + return nil, fmt.Errorf("failed to parse stored JWT RSA key: %w", err) + } + if bytes.Equal(stored, keyPEM) { + slog.Info("Generated new JWT RSA key and stored in database") + } else { + slog.Info("Another instance stored a JWT RSA key first, using theirs") + } + + return key, nil } func parseRSAKeyPEM(data []byte) (*rsa.PrivateKey, error) { diff --git a/pkg/appview/crypto_keys_test.go b/pkg/appview/crypto_keys_test.go new file mode 100644 index 0000000..600ec98 --- /dev/null +++ b/pkg/appview/crypto_keys_test.go @@ -0,0 +1,156 @@ +package appview + +import ( + "bytes" + "database/sql" + "path/filepath" + "sync" + "testing" + + "atcr.io/pkg/appview/db" +) + +// cryptoTestDB returns a file-backed database. +// +// Not ":memory:" — go-libsql gives each connection to an in-memory DSN its own +// private database, so concurrent loaders would not even see each other's writes +// and the test would pass without testing anything. +func cryptoTestDB(t *testing.T) *sql.DB { + t.Helper() + database, err := db.InitDB(filepath.Join(t.TempDir(), "keys.db"), db.LibsqlConfig{}) + if err != nil { + t.Fatalf("InitDB: %v", err) + } + t.Cleanup(func() { database.Close() }) + return database +} + +// TestLoadOAuthKeyConcurrentBootAgreesOnOneKey covers two instances booting +// against a fresh database at the same time. +// +// Both find no key, both generate one, and both write. The write used to be +// last-writer-wins, so the loser kept its own key in memory while the database +// held the other's: it then signed OAuth client assertions with a key absent +// from the published JWKS, and every one of them failed verification. +func TestLoadOAuthKeyConcurrentBootAgreesOnOneKey(t *testing.T) { + database := cryptoTestDB(t) + + const loaders = 6 + keys := make([][]byte, loaders) + errs := make([]error, loaders) + + var wg sync.WaitGroup + start := make(chan struct{}) + for i := range loaders { + wg.Go(func() { + <-start + key, err := loadOAuthKey(database) + if err != nil { + errs[i] = err + return + } + keys[i] = key.Bytes() + }) + } + close(start) + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Fatalf("loader %d: %v", i, err) + } + } + + stored, err := db.GetCryptoKey(database, "oauth_p256") + if err != nil { + t.Fatalf("GetCryptoKey: %v", err) + } + for i, k := range keys { + if !bytes.Equal(k, stored) { + t.Errorf("loader %d returned a key that is not the one in the database; "+ + "it would sign with a key missing from the published JWKS", i) + } + } +} + +// TestLoadRSAKeyConcurrentBootAgreesOnOneKey is the same race for the JWT +// signing key. Losing it means issuing registry JWTs that do not match the +// advertised certificate. +func TestLoadRSAKeyConcurrentBootAgreesOnOneKey(t *testing.T) { + database := cryptoTestDB(t) + + const loaders = 6 + fingerprints := make([][]byte, loaders) + errs := make([]error, loaders) + + var wg sync.WaitGroup + start := make(chan struct{}) + for i := range loaders { + wg.Go(func() { + <-start + key, err := loadRSAKey(database) + if err != nil { + errs[i] = err + return + } + fingerprints[i] = key.N.Bytes() + }) + } + close(start) + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Fatalf("loader %d: %v", i, err) + } + } + + for i := 1; i < loaders; i++ { + if !bytes.Equal(fingerprints[i], fingerprints[0]) { + t.Fatalf("loaders disagreed on the JWT signing key: loader %d differs from loader 0", i) + } + } +} + +// TestPutCryptoKeyKeepsTheFirstWrite pins the storage semantics the loaders rely +// on. If this ever goes back to last-writer-wins, a second instance booting +// would silently replace the key the first one is already signing with. +func TestPutCryptoKeyKeepsTheFirstWrite(t *testing.T) { + database := cryptoTestDB(t) + + first := []byte("first-key") + second := []byte("second-key") + + if err := db.PutCryptoKey(database, "probe", first); err != nil { + t.Fatalf("first put: %v", err) + } + if err := db.PutCryptoKey(database, "probe", second); err != nil { + t.Fatalf("second put: %v", err) + } + + got, err := db.GetCryptoKey(database, "probe") + if err != nil { + t.Fatalf("GetCryptoKey: %v", err) + } + if !bytes.Equal(got, first) { + t.Errorf("stored key = %q, want the first write %q", got, first) + } +} + +// TestLoadOAuthKeyIsStableAcrossRestarts: a restart must reuse the stored key, +// not generate a new one, or every previously issued token breaks. +func TestLoadOAuthKeyIsStableAcrossRestarts(t *testing.T) { + database := cryptoTestDB(t) + + first, err := loadOAuthKey(database) + if err != nil { + t.Fatalf("first load: %v", err) + } + second, err := loadOAuthKey(database) + if err != nil { + t.Fatalf("second load: %v", err) + } + if !bytes.Equal(first.Bytes(), second.Bytes()) { + t.Error("loadOAuthKey generated a new key instead of reusing the stored one") + } +} diff --git a/pkg/appview/db/crypto_keys.go b/pkg/appview/db/crypto_keys.go index e6338b4..6da5643 100644 --- a/pkg/appview/db/crypto_keys.go +++ b/pkg/appview/db/crypto_keys.go @@ -16,10 +16,22 @@ func GetCryptoKey(db DBTX, name string) ([]byte, error) { return data, nil } -// PutCryptoKey stores a key in the database, replacing any existing key with the same name. +// PutCryptoKey stores a key in the database, keeping any key already stored +// under that name. +// +// First writer wins, deliberately. Two instances booting against a fresh +// database both find no key, both generate one, and both write. With +// last-writer-wins the loser kept its own key in memory while the database held +// the other's, so it signed JWTs with a key absent from the published JWKS and +// produced OAuth client assertions that fail verification. Nothing in the +// codebase rotates a key through this function, so the update arm only ever +// fired on that race. +// +// Callers must re-read after writing and use whatever is stored; see +// loadOAuthKey and loadRSAKey. func PutCryptoKey(db DBTX, name string, data []byte) error { _, err := db.Exec( - "INSERT INTO crypto_keys (name, key_data) VALUES (?, ?) ON CONFLICT(name) DO UPDATE SET key_data = excluded.key_data", + "INSERT INTO crypto_keys (name, key_data) VALUES (?, ?) ON CONFLICT(name) DO NOTHING", name, data, ) return err