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