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) <noreply@anthropic.com>

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) {

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")
+	}
+}

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