From 08121f3cd056c58b82f97e3603c9a15af39fd646 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Sat, 8 Aug 2026 23:01:07 -0500 Subject: [PATCH] appview: fix O(n) bcrypt scan making /auth/token take 15s+ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ValidateDeviceSecret ran bcrypt.CompareHashAndPassword against every row in the devices table until one matched — no WHERE clause. At bcrypt cost 10 (~65ms on the single-core production host) and 244 registered devices, a device near the end of the scan cost ~15.8s of pure CPU per /auth/token, which is past Docker's client deadline. Measured on production: 15.7-16.0s steady state with the appview pinned at 100% CPU for the duration, while anonymous requests on the same box served in 20ms. The cost grew linearly with every device registered, and the scan ran in rowid order, so the newest devices — the ones most likely to be in active use — paid the most. This is the timeout users were reporting. Devices now carry secret_lookup = hex(sha256(secret)), indexed, and authentication fetches the single matching row. SHA-256 is the verifier here, not merely an index. Device secrets are 32 bytes from crypto/rand, so presenting a value that hashes to a stored digest requires a preimage or a 2^256 search. bcrypt's work factor only helps when the input space is small enough to enumerate, which does not apply to a random 256-bit token, and a database leak exposes no more than before. The plaintext is not recoverable from a bcrypt hash, so existing rows cannot be backfilled directly. They are migrated lazily on their next successful authentication, which any push, pull or login triggers, and the legacy scan is filtered to un-migrated rows so its cost decays as devices migrate. The backfill runs after the cursor is closed: issuing it inside the rows loop deadlocks, because the open cursor holds the connection the write needs. bcrypt now exists solely to carry legacy rows across and can be deleted once the table is fully migrated. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/appview/db/device_store.go | 133 ++++++++++++++++-- pkg/appview/db/device_store_test.go | 125 ++++++++++++++++ .../0028_add_device_secret_lookup.yaml | 26 ++++ pkg/appview/db/schema.sql | 6 + 4 files changed, 280 insertions(+), 10 deletions(-) create mode 100644 pkg/appview/db/migrations/0028_add_device_secret_lookup.yaml diff --git a/pkg/appview/db/device_store.go b/pkg/appview/db/device_store.go index c06671a..0d1933f 100644 --- a/pkg/appview/db/device_store.go +++ b/pkg/appview/db/device_store.go @@ -3,8 +3,11 @@ package db import ( "context" "crypto/rand" + "crypto/sha256" "database/sql" "encoding/base64" + "encoding/hex" + "errors" "fmt" "log/slog" "time" @@ -218,9 +221,10 @@ func (s *DeviceStore) ApprovePending(userCode, did, handle string) (deviceSecret now := time.Now() _, err = tx.Exec(` - INSERT INTO devices (id, did, handle, name, secret_hash, ip_address, user_agent, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?) - `, deviceID, did, handle, pending.DeviceName, secretHash, pending.IPAddress, pending.UserAgent, now) + INSERT INTO devices (id, did, handle, name, secret_hash, secret_lookup, ip_address, user_agent, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) + `, deviceID, did, handle, pending.DeviceName, secretHash, deviceSecretLookup(deviceSecret), + pending.IPAddress, pending.UserAgent, now) if err != nil { return "", fmt.Errorf("failed to create device: %w", err) @@ -245,18 +249,63 @@ func (s *DeviceStore) ApprovePending(userCode, did, handle string) (deviceSecret return deviceSecret, nil } -// ValidateDeviceSecret validates a device secret and returns the device +// deviceSecretLookup derives the stored verifier for a device secret. +// +// Plain SHA-256 is deliberate, and it is the verifier rather than merely an +// index. Device secrets are 32 bytes from crypto/rand (see the secret +// construction in ApprovePending), so recovering one from its digest means a +// SHA-256 preimage or a 2^256 search. bcrypt's work factor exists to make +// guessing expensive when the input space is small enough to enumerate, which +// a 256-bit random token is not — it buys nothing here and costs ~65ms per +// comparison. This is the same pattern used for API tokens and session +// cookies generally. +// +// The entropy in the generated secret is therefore load-bearing for the whole +// auth model. If that generation is ever weakened, this must change with it. +func deviceSecretLookup(secret string) string { + sum := sha256.Sum256([]byte(secret)) + return hex.EncodeToString(sum[:]) +} + +// ValidateDeviceSecret validates a device secret and returns the device. +// +// Fast path: fetch the single row whose secret_lookup matches. That match is +// the authentication — see deviceSecretLookup. +// +// Legacy path: rows created before migration 0028 have no lookup value, and +// their plaintext is not recoverable from the bcrypt hash, so they are scanned +// and bcrypt-verified once, then backfilled. bcrypt exists here solely to carry +// those rows across; once every device has authenticated once, this branch and +// the bcrypt dependency can be deleted. +// +// The scan used to be unconditional, which made every authentication O(number +// of devices) in bcrypt comparisons. At cost 10 and 244 devices that was ~15.8s +// of CPU per /auth/token, past Docker's client deadline, and it grew with every +// device registered. func (s *DeviceStore) ValidateDeviceSecret(secret string) (*Device, error) { - // Query all devices and check bcrypt hash + if device, err := s.validateBySecretLookup(secret); err != nil { + return nil, err + } else if device != nil { + return device, nil + } + + // Only rows that have not been backfilled still need scanning. rows, err := s.db.Query(` SELECT id, did, handle, name, secret_hash, ip_address, location, user_agent, created_at, last_used FROM devices + WHERE secret_lookup IS NULL OR secret_lookup = '' `) if err != nil { return nil, fmt.Errorf("failed to query devices: %w", err) } defer rows.Close() + // Record the match and finish iterating before writing. Issuing the + // backfill UPDATE inside this loop deadlocks: the open cursor holds a + // connection, and on a single-connection pool the write waits on a + // connection that only the cursor can release. + var matched *Device + for rows.Next() { var device Device var lastUsed sql.NullTime @@ -287,14 +336,78 @@ func (s *DeviceStore) ValidateDeviceSecret(secret string) (*Device, error) { // Check if this device's hash matches the secret if err := bcrypt.CompareHashAndPassword([]byte(device.SecretHash), []byte(secret)); err == nil { - // Update last used asynchronously - go s.UpdateLastUsed(device.SecretHash) - - return &device, nil + matched = &device + break } } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("failed to scan devices: %w", err) + } + // Release the connection before writing. + rows.Close() - return nil, fmt.Errorf("invalid device secret") + if matched == nil { + return nil, fmt.Errorf("invalid device secret") + } + + // Backfill so this device never takes the scan path again. + if _, err := s.db.Exec( + `UPDATE devices SET secret_lookup = ? WHERE id = ?`, + deviceSecretLookup(secret), matched.ID, + ); err != nil { + slog.Warn("Failed to backfill device secret_lookup", + "component", "db/devices", "deviceID", matched.ID, "error", err) + } + + // Update last used asynchronously + go s.UpdateLastUsed(matched.SecretHash) + + return matched, nil +} + +// validateBySecretLookup resolves a secret via the indexed lookup column. +// Returns (nil, nil) when there is no indexed match, so the caller can fall +// back to scanning rows that predate migration 0028. +// +// The SHA-256 match IS the authentication. Presenting a value that hashes to a +// stored digest requires either a SHA-256 preimage or guessing the 256 bits of +// crypto/rand entropy in the secret, so no second factor is needed here; bcrypt +// is retained only on the legacy path below, where it is the sole verifier +// available for rows that have no lookup value yet. +// +// The comparison happens in SQL and so is not constant-time, which is +// immaterial: exploiting the timing would require iterating candidate secrets, +// and every candidate is a 256-bit guess. +func (s *DeviceStore) validateBySecretLookup(secret string) (*Device, error) { + var device Device + var lastUsed sql.NullTime + var location sql.NullString + + err := s.db.QueryRow(` + SELECT id, did, handle, name, secret_hash, ip_address, location, user_agent, created_at, last_used + FROM devices + WHERE secret_lookup = ? + `, deviceSecretLookup(secret)).Scan( + &device.ID, &device.DID, &device.Handle, &device.Name, &device.SecretHash, + &device.IPAddress, &location, &device.UserAgent, &device.CreatedAt, &lastUsed, + ) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("failed to look up device by secret: %w", err) + } + + if lastUsed.Valid { + device.LastUsed = lastUsed.Time + } + if location.Valid { + device.Location = location.String + } + + go s.UpdateLastUsed(device.SecretHash) + + return &device, nil } // ListDevices returns all devices for a DID diff --git a/pkg/appview/db/device_store_test.go b/pkg/appview/db/device_store_test.go index f0b5695..899e6a9 100644 --- a/pkg/appview/db/device_store_test.go +++ b/pkg/appview/db/device_store_test.go @@ -2,6 +2,8 @@ package db import ( "context" + "database/sql" + "errors" "fmt" "strings" "testing" @@ -628,3 +630,126 @@ func TestDeviceStore_SecretHashing(t *testing.T) { t.Error("Wrong secret should not match hash") } } + +// newDeviceForTest approves a pending auth and returns the device secret. +func newDeviceForTest(t *testing.T, store *DeviceStore, did, handle, name string) string { + t.Helper() + pending, err := store.CreatePendingAuth(name, "192.168.1.1", "Test Agent") + if err != nil { + t.Fatalf("CreatePendingAuth() error = %v", err) + } + secret, err := store.ApprovePending(pending.UserCode, did, handle) + if err != nil { + t.Fatalf("ApprovePending() error = %v", err) + } + return secret +} + +func lookupValueFor(t *testing.T, store *DeviceStore, secret string) string { + t.Helper() + var lookup sql.NullString + err := store.db.QueryRow( + `SELECT secret_lookup FROM devices WHERE secret_lookup = ?`, + deviceSecretLookup(secret), + ).Scan(&lookup) + if errors.Is(err, sql.ErrNoRows) { + return "" + } + if err != nil { + t.Fatalf("query secret_lookup: %v", err) + } + return lookup.String +} + +// TestDeviceStore_NewDeviceGetsSecretLookup verifies newly created devices are +// indexed at creation, so they never take the scan path. +func TestDeviceStore_NewDeviceGetsSecretLookup(t *testing.T) { + store := setupTestDB(t) + createTestUser(t, store, "did:plc:alice123", "alice.bsky.social") + secret := newDeviceForTest(t, store, "did:plc:alice123", "alice.bsky.social", "My Device") + + if got := lookupValueFor(t, store, secret); got != deviceSecretLookup(secret) { + t.Fatalf("secret_lookup not populated at creation, got %q", got) + } + + device, err := store.ValidateDeviceSecret(secret) + if err != nil { + t.Fatalf("ValidateDeviceSecret() error = %v", err) + } + if device.DID != "did:plc:alice123" { + t.Errorf("DID = %v, want did:plc:alice123", device.DID) + } +} + +// TestDeviceStore_LegacyDeviceBackfills covers rows created before migration +// 0028. They have a NULL secret_lookup, so the first authentication falls back +// to the scan and must backfill, and subsequent ones resolve via the index. +func TestDeviceStore_LegacyDeviceBackfills(t *testing.T) { + store := setupTestDB(t) + createTestUser(t, store, "did:plc:alice123", "alice.bsky.social") + secret := newDeviceForTest(t, store, "did:plc:alice123", "alice.bsky.social", "Legacy Device") + + // Simulate a pre-migration row. + if _, err := store.db.Exec(`UPDATE devices SET secret_lookup = NULL`); err != nil { + t.Fatalf("clear secret_lookup: %v", err) + } + if got := lookupValueFor(t, store, secret); got != "" { + t.Fatalf("expected no indexed row before backfill, got %q", got) + } + + device, err := store.ValidateDeviceSecret(secret) + if err != nil { + t.Fatalf("ValidateDeviceSecret() on legacy row error = %v", err) + } + if device.Name != "Legacy Device" { + t.Errorf("Name = %v, want Legacy Device", device.Name) + } + + if got := lookupValueFor(t, store, secret); got != deviceSecretLookup(secret) { + t.Fatalf("secret_lookup was not backfilled, got %q", got) + } + + // Second call must still succeed, now via the indexed path. + if _, err := store.ValidateDeviceSecret(secret); err != nil { + t.Fatalf("ValidateDeviceSecret() after backfill error = %v", err) + } +} + +// TestDeviceStore_ValidateDoesNotScanIndexedRows is the regression guard for the +// O(n) bcrypt scan. Every device is indexed, so a wrong secret must not compare +// against any of them — it should miss the index and find nothing left to scan. +func TestDeviceStore_ValidateDoesNotScanIndexedRows(t *testing.T) { + store := setupTestDB(t) + createTestUser(t, store, "did:plc:alice123", "alice.bsky.social") + for i := 0; i < 5; i++ { + newDeviceForTest(t, store, "did:plc:alice123", "alice.bsky.social", fmt.Sprintf("Device %d", i)) + } + + var unindexed int + if err := store.db.QueryRow( + `SELECT COUNT(*) FROM devices WHERE secret_lookup IS NULL OR secret_lookup = ''`, + ).Scan(&unindexed); err != nil { + t.Fatalf("count unindexed: %v", err) + } + if unindexed != 0 { + t.Fatalf("expected all devices indexed, %d still unindexed", unindexed) + } + + if _, err := store.ValidateDeviceSecret("atcr_device_wrong"); err == nil { + t.Error("expected error for an unknown secret") + } +} + +// TestDeviceSecretLookup_StableAndDistinct guards the lookup derivation. +func TestDeviceSecretLookup_StableAndDistinct(t *testing.T) { + a := deviceSecretLookup("atcr_device_aaa") + if a != deviceSecretLookup("atcr_device_aaa") { + t.Error("deviceSecretLookup is not deterministic") + } + if a == deviceSecretLookup("atcr_device_bbb") { + t.Error("distinct secrets produced the same lookup") + } + if len(a) != 64 { + t.Errorf("expected 64 hex chars for sha256, got %d", len(a)) + } +} diff --git a/pkg/appview/db/migrations/0028_add_device_secret_lookup.yaml b/pkg/appview/db/migrations/0028_add_device_secret_lookup.yaml new file mode 100644 index 0000000..5c2cd5c --- /dev/null +++ b/pkg/appview/db/migrations/0028_add_device_secret_lookup.yaml @@ -0,0 +1,26 @@ +description: | + Add an indexed secret_lookup column to devices so authentication can fetch a + single candidate row instead of bcrypt-scanning the whole table. + + ValidateDeviceSecret previously ran bcrypt.CompareHashAndPassword against + every device until one matched. At bcrypt cost 10 (~65ms on a single-core + host) and 244 registered devices that is ~15.8s of pure CPU per /auth/token + request for a device near the end of the scan, which pushes Docker past its + client deadline. The cost grows linearly with every device registered. + + secret_lookup holds hex(sha256(secret)) and is the verifier, not just an + index. Device secrets are 32 bytes from crypto/rand ("atcr_device_" + + base64url), so presenting a value that hashes to a stored digest requires a + SHA-256 preimage or a 2^256 search. bcrypt's work factor only buys protection + where the input space is small enough to enumerate, which does not apply here, + and a database leak exposes no more than it did before. + + Nullable with no backfill: the plaintext is not recoverable from the bcrypt + hash, so existing rows are populated lazily on their next successful + authentication, which any push, pull or login triggers. The legacy scan is + filtered to un-migrated rows, so its cost decays as devices migrate. bcrypt + remains only to carry those rows across and can be removed once the table is + fully migrated. +query: | + ALTER TABLE devices ADD COLUMN secret_lookup TEXT; + CREATE INDEX IF NOT EXISTS idx_devices_secret_lookup ON devices(secret_lookup); diff --git a/pkg/appview/db/schema.sql b/pkg/appview/db/schema.sql index 1563318..481734e 100644 --- a/pkg/appview/db/schema.sql +++ b/pkg/appview/db/schema.sql @@ -132,6 +132,11 @@ CREATE TABLE IF NOT EXISTS devices ( handle TEXT NOT NULL, name TEXT NOT NULL, secret_hash TEXT NOT NULL UNIQUE, + -- hex(sha256(secret)), for O(1) lookup during authentication. Nullable + -- because rows predating migration 0028 are backfilled lazily on their + -- next successful auth (the plaintext is not recoverable from the bcrypt + -- hash). See ValidateDeviceSecret. + secret_lookup TEXT, ip_address TEXT, location TEXT, user_agent TEXT, @@ -139,6 +144,7 @@ CREATE TABLE IF NOT EXISTS devices ( last_used TIMESTAMP, FOREIGN KEY(did) REFERENCES users(did) ON DELETE CASCADE ); +CREATE INDEX IF NOT EXISTS idx_devices_secret_lookup ON devices(secret_lookup); CREATE INDEX IF NOT EXISTS idx_devices_did ON devices(did); CREATE INDEX IF NOT EXISTS idx_devices_hash ON devices(secret_hash);