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