From 172c240d1b10fbead7dbf7acfca350180c26b568 Mon Sep 17 00:00:00 2001 From: Ivan Godwin Date: Fri, 17 Jul 2026 20:27:49 -0700 Subject: [PATCH] fix(auth): repair persistence layer and stop storing plaintext keys - Store SHA-256 digests (key_hash + key_preview) instead of raw keys, in both the in-memory store and Postgres; migrate legacy plaintext rows in place and drop the plaintext column. - Fix TEXT[] scans that failed at runtime (missing pq.Array) in GetKey/ListKeys/LoadAllKeys. - Load persisted keys at startup (InitializeFromDatabase was never called) and fall back to the database on cache miss, so issued keys survive restarts. - Make HybridKeyStore.CreateKey genuinely write-through: cache is only updated after a successful DB write. - Guard nil database backend (auth enabled without DB previously panicked on key creation) and degrade to in-memory operation. - Persist bootstrap admin keys when a database is configured. - Record real audit-log details as JSON and log audit failures instead of silently dropping them; add DB pool limits and ping timeout. - Sentinel errors matched with errors.Is; unit tests for hashing, write-through ordering, DB fallback, and nil-DB operation. Co-Authored-By: Claude Fable 5 --- api/rest/keys.go | 6 +- cmd/server/main.go | 29 ++- internal/auth/auth.go | 154 ++++++++---- internal/auth/bootstrap.go | 28 +-- internal/auth/keystore_db.go | 400 ++++++++++++++++--------------- internal/auth/keystore_hybrid.go | 284 +++++++++++----------- internal/auth/keystore_test.go | 320 +++++++++++++++++++++++++ 7 files changed, 822 insertions(+), 399 deletions(-) create mode 100644 internal/auth/keystore_test.go diff --git a/api/rest/keys.go b/api/rest/keys.go index b19951a..17c94ca 100644 --- a/api/rest/keys.go +++ b/api/rest/keys.go @@ -2,10 +2,10 @@ package rest import ( "encoding/json" + "errors" "fmt" "net/http" "strconv" - "strings" "time" "github.com/gorilla/mux" @@ -173,7 +173,7 @@ func (h *KeyManagementHandler) ListKeys(w http.ResponseWriter, r *http.Request) keyInfos := make([]*KeyInfo, len(keys)) for i, key := range keys { keyInfos[i] = &KeyInfo{ - Key: "nk_" + key.Key[len(key.Key)-4:], // Show only last 4 chars + Key: key.KeyPreview, // non-sensitive preview, e.g. "nk_…ab12" Name: key.Name, ClientID: key.ClientID, Roles: key.Roles, @@ -214,7 +214,7 @@ func (h *KeyManagementHandler) RevokeKey(w http.ResponseWriter, r *http.Request) err := h.keyStore.DeactivateKeyByName(ctx, keyName, authCtx.ClientID) if err != nil { - if strings.Contains(err.Error(), "not found") { + if errors.Is(err, auth.ErrKeyNotFound) { h.respondError(w, http.StatusNotFound, "Key not found", "") } else { h.logger.Errorf("Failed to revoke API key: %v", err) diff --git a/cmd/server/main.go b/cmd/server/main.go index 68847cc..837ce6b 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -99,7 +99,7 @@ func main() { // Create database backend if configured var dbStore *auth.KeyStoreDB if cfg.Auth.Database.URL != "" { - dbStore, err = auth.NewKeyStoreDB(cfg.Auth.Database.URL) + dbStore, err = auth.NewKeyStoreDB(cfg.Auth.Database.URL, logger) if err != nil { logger.Fatalf("Failed to create database key store: %v", err) } @@ -112,6 +112,16 @@ func main() { hybridKeyStore = auth.NewHybridKeyStore(authStore, dbStore) logger.Debugf("Initialized hybrid key store for API key management") + // Load persisted keys into the cache so previously issued keys + // keep authenticating across restarts. + if hybridKeyStore.HasDatabase() { + if loaded, err := hybridKeyStore.InitializeFromDatabase(ctx); err != nil { + logger.Errorf("Failed to load API keys from database: %v", err) + } else { + logger.Infof("Loaded %d API key(s) from database into cache", loaded) + } + } + // Bootstrap admin key if configured if cfg.Auth.Bootstrap.Enabled { bootstrapCfg := &auth.BootstrapConfig{ @@ -133,12 +143,23 @@ func main() { // If we have an existing key, use it if existingKey != "" { - if _, err := auth.RegisterAdminKeyInMemory(authStore, existingKey, logger); err != nil { + apiKey, err := auth.RegisterAdminKeyInMemory(authStore, existingKey, logger) + if err != nil { logger.Warnf("Failed to register existing admin key: %v", err) + } else if err := hybridKeyStore.EnsurePersisted(ctx, apiKey, "bootstrap"); err != nil { + logger.Warnf("Failed to persist existing admin key: %v", err) } } else { - // Generate new key - if apiKey, err := auth.BootstrapAdminKeyInMemory(authStore, bootstrapCfg, logger); err != nil { + // Generate a new key — through the hybrid store when a + // database is configured so the admin key survives restarts. + var apiKey *auth.APIKey + var err error + if hybridKeyStore.HasDatabase() { + apiKey, err = auth.BootstrapAdminKey(ctx, hybridKeyStore, bootstrapCfg, logger) + } else { + apiKey, err = auth.BootstrapAdminKeyInMemory(authStore, bootstrapCfg, logger) + } + if err != nil { logger.Warnf("Bootstrap admin key creation failed: %v", err) } else if apiKey != nil { // Store in Kubernetes secret if configured diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 199c32d..07b2e34 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -3,22 +3,38 @@ package auth import ( "context" "crypto/rand" + "crypto/sha256" "encoding/hex" + "errors" "fmt" "sync" "time" ) -// APIKeyStore manages API keys with rate limiting +// Sentinel errors for key validation and lookup. Match with errors.Is. +var ( + ErrInvalidKey = errors.New("invalid API key") + ErrKeyInactive = errors.New("API key is inactive") + ErrKeyExpired = errors.New("API key has expired") + ErrKeyNotFound = errors.New("API key not found") +) + +// APIKeyStore manages API keys with rate limiting. +// Keys are stored indexed by SHA-256 digest; the raw key is never retained +// after creation. type APIKeyStore struct { mu sync.RWMutex - keys map[string]*APIKey + keys map[string]*APIKey // keyed by KeyHash rateLimits map[string]*RateLimiter } -// APIKey represents an API key with metadata +// APIKey represents an API key with metadata. +// Key holds the raw secret only on the value returned from key creation; +// stored and persisted records carry only KeyHash and KeyPreview. type APIKey struct { - Key string `json:"key"` + Key string `json:"key,omitempty"` + KeyHash string `json:"-"` + KeyPreview string `json:"key_preview,omitempty"` // e.g. "nk_…ab12" Name string `json:"name"` ClientID string `json:"client_id"` Roles []string `json:"roles"` @@ -45,20 +61,26 @@ type AuthContext struct { Roles []string } -// NewAPIKeyStore creates a new API key store -func NewAPIKeyStore() *APIKeyStore { - return &APIKeyStore{ - keys: make(map[string]*APIKey), - rateLimits: make(map[string]*RateLimiter), - } +// HashKey returns the hex-encoded SHA-256 digest of a raw API key. +// All storage and lookups are keyed by this digest so a leaked store or +// database dump does not expose usable credentials. +func HashKey(rawKey string) string { + sum := sha256.Sum256([]byte(rawKey)) + return hex.EncodeToString(sum[:]) } -// CreateKey generates a new API key -func (s *APIKeyStore) CreateKey(clientID string, roles []string, rateLimit int, expiresIn *time.Duration) (*APIKey, error) { - s.mu.Lock() - defer s.mu.Unlock() +// keyPreview returns a non-sensitive display form of a raw key ("nk_…ab12"). +func keyPreview(rawKey string) string { + if len(rawKey) < 4 { + return "" + } + return "nk_…" + rawKey[len(rawKey)-4:] +} - // Generate random key +// generateAPIKey creates a new APIKey with a random secret without touching +// any store. The returned value carries the raw secret in Key; callers decide +// where (and whether) to register it. +func generateAPIKey(clientID string, roles []string, rateLimit int, expiresIn *time.Duration) (*APIKey, error) { keyBytes := make([]byte, 32) if _, err := rand.Read(keyBytes); err != nil { return nil, fmt.Errorf("failed to generate key: %w", err) @@ -67,13 +89,15 @@ func (s *APIKeyStore) CreateKey(clientID string, roles []string, rateLimit int, now := time.Now().UTC() apiKey := &APIKey{ - Key: key, - ClientID: clientID, - Roles: roles, - CreatedAt: now, - IsActive: true, - RateLimit: rateLimit, - Name: fmt.Sprintf("%s-%d", clientID, now.Unix()), + Key: key, + KeyHash: HashKey(key), + KeyPreview: keyPreview(key), + ClientID: clientID, + Roles: roles, + CreatedAt: now, + IsActive: true, + RateLimit: rateLimit, + Name: fmt.Sprintf("%s-%d", clientID, now.Unix()), } if expiresIn != nil { @@ -81,33 +105,75 @@ func (s *APIKeyStore) CreateKey(clientID string, roles []string, rateLimit int, apiKey.ExpiresAt = &expiresAt } - s.keys[key] = apiKey - s.rateLimits[key] = &RateLimiter{ - maxRequests: rateLimit, + return apiKey, nil +} + +// NewAPIKeyStore creates a new API key store +func NewAPIKeyStore() *APIKeyStore { + return &APIKeyStore{ + keys: make(map[string]*APIKey), + rateLimits: make(map[string]*RateLimiter), + } +} + +// CreateKey generates a new API key and registers it in the store. +// The returned APIKey carries the raw secret; the stored copy does not. +func (s *APIKeyStore) CreateKey(clientID string, roles []string, rateLimit int, expiresIn *time.Duration) (*APIKey, error) { + apiKey, err := generateAPIKey(clientID, roles, rateLimit, expiresIn) + if err != nil { + return nil, err + } + s.RegisterKey(apiKey) + return apiKey, nil +} + +// RegisterKey adds a key to the store, indexed by digest. The stored copy has +// the raw secret stripped. If KeyHash is unset it is computed from Key. +func (s *APIKeyStore) RegisterKey(apiKey *APIKey) { + stored := *apiKey + if stored.KeyHash == "" && stored.Key != "" { + stored.KeyHash = HashKey(stored.Key) + } + if stored.KeyPreview == "" && stored.Key != "" { + stored.KeyPreview = keyPreview(stored.Key) + } + stored.Key = "" // never retain the raw secret + + s.mu.Lock() + defer s.mu.Unlock() + s.keys[stored.KeyHash] = &stored + s.rateLimits[stored.KeyHash] = &RateLimiter{ + maxRequests: stored.RateLimit, window: time.Minute, resetTime: time.Now().Add(time.Minute), count: 0, } - - return apiKey, nil } -// ValidateKey checks if an API key is valid and returns the key metadata +// RemoveKeyByHash deletes a key and its rate limiter from the store. +func (s *APIKeyStore) RemoveKeyByHash(keyHash string) { + s.mu.Lock() + defer s.mu.Unlock() + delete(s.keys, keyHash) + delete(s.rateLimits, keyHash) +} + +// ValidateKey checks if a raw API key is valid and returns the key metadata func (s *APIKeyStore) ValidateKey(keyStr string) (*APIKey, error) { s.mu.RLock() defer s.mu.RUnlock() - key, exists := s.keys[keyStr] + key, exists := s.keys[HashKey(keyStr)] if !exists { - return nil, fmt.Errorf("invalid API key") + return nil, ErrInvalidKey } if !key.IsActive { - return nil, fmt.Errorf("API key is inactive") + return nil, ErrKeyInactive } if key.ExpiresAt != nil && time.Now().After(*key.ExpiresAt) { - return nil, fmt.Errorf("API key has expired") + return nil, ErrKeyExpired } return key, nil @@ -115,13 +181,15 @@ func (s *APIKeyStore) ValidateKey(keyStr string) (*APIKey, error) { // CheckRateLimit checks if a key has exceeded its rate limit func (s *APIKeyStore) CheckRateLimit(keyStr string) (bool, error) { + keyHash := HashKey(keyStr) + // Look up key and limiter under the store lock, then release it // before acquiring the per-key limiter lock to avoid nested locking. s.mu.RLock() - key, exists := s.keys[keyStr] + key, exists := s.keys[keyHash] if !exists { s.mu.RUnlock() - return false, fmt.Errorf("invalid API key") + return false, ErrInvalidKey } // Unlimited rate limit @@ -130,7 +198,7 @@ func (s *APIKeyStore) CheckRateLimit(keyStr string) (bool, error) { return true, nil } - limiter, exists := s.rateLimits[keyStr] + limiter, exists := s.rateLimits[keyHash] if !exists { s.mu.RUnlock() return false, fmt.Errorf("rate limiter not found") @@ -160,9 +228,9 @@ func (s *APIKeyStore) UpdateLastUsed(keyStr string) error { s.mu.Lock() defer s.mu.Unlock() - key, exists := s.keys[keyStr] + key, exists := s.keys[HashKey(keyStr)] if !exists { - return fmt.Errorf("invalid API key") + return ErrInvalidKey } now := time.Now().UTC() @@ -170,28 +238,28 @@ func (s *APIKeyStore) UpdateLastUsed(keyStr string) error { return nil } -// DeactivateKey deactivates an API key +// DeactivateKey deactivates an API key by its raw value func (s *APIKeyStore) DeactivateKey(keyStr string) error { s.mu.Lock() defer s.mu.Unlock() - key, exists := s.keys[keyStr] + key, exists := s.keys[HashKey(keyStr)] if !exists { - return fmt.Errorf("invalid API key") + return ErrInvalidKey } key.IsActive = false return nil } -// GetKey retrieves key metadata (for management purposes) +// GetKey retrieves key metadata by raw key (for management purposes) func (s *APIKeyStore) GetKey(keyStr string) (*APIKey, error) { s.mu.RLock() defer s.mu.RUnlock() - key, exists := s.keys[keyStr] + key, exists := s.keys[HashKey(keyStr)] if !exists { - return nil, fmt.Errorf("key not found") + return nil, ErrKeyNotFound } return key, nil diff --git a/internal/auth/bootstrap.go b/internal/auth/bootstrap.go index 89755f9..a0c0d73 100644 --- a/internal/auth/bootstrap.go +++ b/internal/auth/bootstrap.go @@ -101,26 +101,18 @@ func RegisterAdminKeyInMemory(keyStore *APIKeyStore, adminKey string, logger *lo adminRoles := []string{"admin", "notify-email", "notify-slack", "notify-ntfy"} now := time.Now().UTC() apiKey := &APIKey{ - Key: adminKey, - ClientID: "admin-bootstrap", - Roles: adminRoles, - CreatedAt: now, - IsActive: true, - RateLimit: 0, // Unlimited - Name: fmt.Sprintf("admin-bootstrap-%d", now.Unix()), + Key: adminKey, + KeyHash: HashKey(adminKey), + KeyPreview: keyPreview(adminKey), + ClientID: "admin-bootstrap", + Roles: adminRoles, + CreatedAt: now, + IsActive: true, + RateLimit: 0, // Unlimited + Name: fmt.Sprintf("admin-bootstrap-%d", now.Unix()), } - // Add to keystore - keyStore.mu.Lock() - defer keyStore.mu.Unlock() - - keyStore.keys[adminKey] = apiKey - keyStore.rateLimits[adminKey] = &RateLimiter{ - maxRequests: 0, // Unlimited - window: time.Minute, - resetTime: time.Now().Add(time.Minute), - count: 0, - } + keyStore.RegisterKey(apiKey) logger.Infof("Registered existing admin key from Kubernetes secret") return apiKey, nil diff --git a/internal/auth/keystore_db.go b/internal/auth/keystore_db.go index c07f3fd..177adc5 100644 --- a/internal/auth/keystore_db.go +++ b/internal/auth/keystore_db.go @@ -3,34 +3,41 @@ package auth import ( "context" "database/sql" + "encoding/json" "fmt" "time" + "github.com/igodwin/notifier/internal/logging" "github.com/lib/pq" - _ "github.com/lib/pq" // PostgreSQL driver ) -// KeyStoreDB provides persistent storage for API keys using PostgreSQL -// It acts as the backend for the in-memory cache +// KeyStoreDB provides persistent storage for API keys using PostgreSQL. +// It acts as the backend for the in-memory cache. Only SHA-256 digests of +// keys are persisted — the raw secret never reaches the database. type KeyStoreDB struct { - db *sql.DB + db *sql.DB + logger *logging.Logger } // NewKeyStoreDB creates a new database-backed key store -func NewKeyStoreDB(dbURL string) (*KeyStoreDB, error) { +func NewKeyStoreDB(dbURL string, logger *logging.Logger) (*KeyStoreDB, error) { db, err := sql.Open("postgres", dbURL) if err != nil { return nil, fmt.Errorf("failed to connect to database: %w", err) } - // Test connection - if err := db.Ping(); err != nil { + db.SetMaxOpenConns(10) + db.SetMaxIdleConns(5) + db.SetConnMaxLifetime(30 * time.Minute) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if err := db.PingContext(ctx); err != nil { return nil, fmt.Errorf("failed to ping database: %w", err) } - ks := &KeyStoreDB{db: db} + ks := &KeyStoreDB{db: db, logger: logger} - // Initialize schema if err := ks.initializeSchema(); err != nil { return nil, fmt.Errorf("failed to initialize schema: %w", err) } @@ -38,14 +45,15 @@ func NewKeyStoreDB(dbURL string) (*KeyStoreDB, error) { return ks, nil } -// initializeSchema creates the necessary tables and indexes if they don't exist +// initializeSchema creates the necessary tables and indexes if they don't +// exist, and migrates legacy plaintext-key rows to hashed storage. func (ks *KeyStoreDB) initializeSchema() error { - // Create tables tableSchema := ` - -- API Keys table + -- API Keys table (key_hash is the SHA-256 digest of the raw key) CREATE TABLE IF NOT EXISTS api_keys ( id SERIAL PRIMARY KEY, - key VARCHAR(255) UNIQUE NOT NULL, + key_hash VARCHAR(64) UNIQUE NOT NULL, + key_preview VARCHAR(16) NOT NULL DEFAULT '', name VARCHAR(255) NOT NULL, client_id VARCHAR(255) NOT NULL, roles TEXT[] DEFAULT '{}', @@ -73,9 +81,11 @@ func (ks *KeyStoreDB) initializeSchema() error { return fmt.Errorf("failed to create tables: %w", err) } - // Create indexes separately (PostgreSQL syntax) + if err := ks.migrateLegacyPlaintextKeys(); err != nil { + return fmt.Errorf("failed to migrate legacy plaintext keys: %w", err) + } + indexSchema := ` - CREATE INDEX IF NOT EXISTS idx_api_keys_key ON api_keys(key); CREATE INDEX IF NOT EXISTS idx_api_keys_client_id ON api_keys(client_id); CREATE INDEX IF NOT EXISTS idx_api_keys_active ON api_keys(is_active); CREATE INDEX IF NOT EXISTS idx_api_keys_expires ON api_keys(expires_at); @@ -90,12 +100,87 @@ func (ks *KeyStoreDB) initializeSchema() error { return nil } -// SaveKey persists an API key to the database +// migrateLegacyPlaintextKeys upgrades tables created by earlier versions that +// stored the raw key in a "key" column: it adds the hash columns, hashes each +// plaintext key in place, then drops the plaintext column entirely. +func (ks *KeyStoreDB) migrateLegacyPlaintextKeys() error { + var hasLegacyColumn bool + err := ks.db.QueryRow(` + SELECT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_name = 'api_keys' AND column_name = 'key' + )`).Scan(&hasLegacyColumn) + if err != nil { + return fmt.Errorf("failed to inspect schema: %w", err) + } + if !hasLegacyColumn { + return nil + } + + if _, err := ks.db.Exec(` + ALTER TABLE api_keys ADD COLUMN IF NOT EXISTS key_hash VARCHAR(64); + ALTER TABLE api_keys ADD COLUMN IF NOT EXISTS key_preview VARCHAR(16) NOT NULL DEFAULT ''; + `); err != nil { + return fmt.Errorf("failed to add hash columns: %w", err) + } + + rows, err := ks.db.Query(`SELECT id, key FROM api_keys WHERE key_hash IS NULL AND key IS NOT NULL`) + if err != nil { + return fmt.Errorf("failed to read legacy keys: %w", err) + } + defer rows.Close() + + type legacyRow struct { + id int + key string + } + var legacy []legacyRow + for rows.Next() { + var r legacyRow + if err := rows.Scan(&r.id, &r.key); err != nil { + return fmt.Errorf("failed to scan legacy key: %w", err) + } + legacy = append(legacy, r) + } + if err := rows.Err(); err != nil { + return err + } + + for _, r := range legacy { + if _, err := ks.db.Exec( + `UPDATE api_keys SET key_hash = $1, key_preview = $2 WHERE id = $3`, + HashKey(r.key), keyPreview(r.key), r.id, + ); err != nil { + return fmt.Errorf("failed to hash legacy key id=%d: %w", r.id, err) + } + } + + // Drop the plaintext column and enforce uniqueness on the digest. + if _, err := ks.db.Exec(` + ALTER TABLE api_keys DROP COLUMN key; + ALTER TABLE api_keys ALTER COLUMN key_hash SET NOT NULL; + CREATE UNIQUE INDEX IF NOT EXISTS idx_api_keys_key_hash ON api_keys(key_hash); + `); err != nil { + return fmt.Errorf("failed to finalize hash migration: %w", err) + } + + if ks.logger != nil { + ks.logger.Infof("Migrated %d legacy plaintext API key(s) to hashed storage", len(legacy)) + } + return nil +} + +// SaveKey persists an API key to the database. Only the digest and preview +// are stored; apiKey.Key (the raw secret) is never written. func (ks *KeyStoreDB) SaveKey(ctx context.Context, key *APIKey, createdBy string) error { + if key.KeyHash == "" { + return fmt.Errorf("refusing to save key without digest") + } + query := ` - INSERT INTO api_keys (key, name, client_id, roles, created_at, last_used_at, expires_at, is_active, rate_limit, created_by) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) - ON CONFLICT (key) DO UPDATE SET + INSERT INTO api_keys (key_hash, key_preview, name, client_id, roles, created_at, last_used_at, expires_at, is_active, rate_limit, created_by) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) + ON CONFLICT (key_hash) DO UPDATE SET name = EXCLUDED.name, roles = EXCLUDED.roles, is_active = EXCLUDED.is_active, @@ -104,10 +189,11 @@ func (ks *KeyStoreDB) SaveKey(ctx context.Context, key *APIKey, createdBy string ` _, err := ks.db.ExecContext(ctx, query, - key.Key, + key.KeyHash, + key.KeyPreview, key.Name, key.ClientID, - pq.Array(key.Roles), // Convert Go slice to PostgreSQL array + pq.Array(key.Roles), key.CreatedAt, key.LastUsedAt, key.ExpiresAt, @@ -120,8 +206,7 @@ func (ks *KeyStoreDB) SaveKey(ctx context.Context, key *APIKey, createdBy string return fmt.Errorf("failed to save key: %w", err) } - // Log to audit trail - ks.logAudit(ctx, key.Key, "created", createdBy, map[string]interface{}{ + ks.logAudit(ctx, key.KeyHash, "created", createdBy, map[string]interface{}{ "client_id": key.ClientID, "roles": key.Roles, }) @@ -129,50 +214,54 @@ func (ks *KeyStoreDB) SaveKey(ctx context.Context, key *APIKey, createdBy string return nil } -// GetKey retrieves an API key from the database -func (ks *KeyStoreDB) GetKey(ctx context.Context, keyStr string) (*APIKey, error) { - query := ` - SELECT key, name, client_id, roles, created_at, last_used_at, expires_at, is_active, rate_limit - FROM api_keys - WHERE key = $1 - ` - +// scanKey scans a single api_keys row into an APIKey. +func scanKey(scanner interface{ Scan(...interface{}) error }) (*APIKey, error) { var key APIKey var roles []string - err := ks.db.QueryRowContext(ctx, query, keyStr).Scan( - &key.Key, + err := scanner.Scan( + &key.KeyHash, + &key.KeyPreview, &key.Name, &key.ClientID, - &roles, + pq.Array(&roles), &key.CreatedAt, &key.LastUsedAt, &key.ExpiresAt, &key.IsActive, &key.RateLimit, ) - - if err == sql.ErrNoRows { - return nil, ErrKeyNotFound - } if err != nil { - return nil, fmt.Errorf("failed to get key: %w", err) + return nil, err } key.Roles = roles return &key, nil } -// ListKeys retrieves all API keys for a client -func (ks *KeyStoreDB) ListKeys(ctx context.Context, clientID string) ([]*APIKey, error) { - query := ` - SELECT key, name, client_id, roles, created_at, last_used_at, expires_at, is_active, rate_limit - FROM api_keys - WHERE client_id = $1 AND is_active = true - ORDER BY created_at DESC - ` +const keyColumns = `key_hash, key_preview, name, client_id, roles, created_at, last_used_at, expires_at, is_active, rate_limit` - rows, err := ks.db.QueryContext(ctx, query, clientID) +// GetKeyByHash retrieves an API key from the database by its digest +func (ks *KeyStoreDB) GetKeyByHash(ctx context.Context, keyHash string) (*APIKey, error) { + row := ks.db.QueryRowContext(ctx, + `SELECT `+keyColumns+` FROM api_keys WHERE key_hash = $1`, keyHash) + + key, err := scanKey(row) + if err == sql.ErrNoRows { + return nil, ErrKeyNotFound + } + if err != nil { + return nil, fmt.Errorf("failed to get key: %w", err) + } + return key, nil +} + +// ListKeys retrieves all active API keys for a client +func (ks *KeyStoreDB) ListKeys(ctx context.Context, clientID string) ([]*APIKey, error) { + rows, err := ks.db.QueryContext(ctx, + `SELECT `+keyColumns+` FROM api_keys + WHERE client_id = $1 AND is_active = true + ORDER BY created_at DESC`, clientID) if err != nil { return nil, fmt.Errorf("failed to list keys: %w", err) } @@ -180,36 +269,20 @@ func (ks *KeyStoreDB) ListKeys(ctx context.Context, clientID string) ([]*APIKey, var keys []*APIKey for rows.Next() { - var key APIKey - var roles []string - - err := rows.Scan( - &key.Key, - &key.Name, - &key.ClientID, - &roles, - &key.CreatedAt, - &key.LastUsedAt, - &key.ExpiresAt, - &key.IsActive, - &key.RateLimit, - ) + key, err := scanKey(rows) if err != nil { return nil, fmt.Errorf("failed to scan key: %w", err) } - - key.Roles = roles - keys = append(keys, &key) + keys = append(keys, key) } return keys, rows.Err() } -// DeactivateKey disables an API key -func (ks *KeyStoreDB) DeactivateKey(ctx context.Context, keyStr string, deactivatedBy string) error { - query := `UPDATE api_keys SET is_active = false WHERE key = $1` - - result, err := ks.db.ExecContext(ctx, query, keyStr) +// DeactivateKeyByHash disables an API key identified by its digest +func (ks *KeyStoreDB) DeactivateKeyByHash(ctx context.Context, keyHash string, deactivatedBy string) error { + result, err := ks.db.ExecContext(ctx, + `UPDATE api_keys SET is_active = false WHERE key_hash = $1`, keyHash) if err != nil { return fmt.Errorf("failed to deactivate key: %w", err) } @@ -223,31 +296,25 @@ func (ks *KeyStoreDB) DeactivateKey(ctx context.Context, keyStr string, deactiva return ErrKeyNotFound } - ks.logAudit(ctx, keyStr, "deactivated", deactivatedBy, nil) + ks.logAudit(ctx, keyHash, "deactivated", deactivatedBy, nil) return nil } -// UpdateLastUsed updates the last_used_at timestamp -func (ks *KeyStoreDB) UpdateLastUsed(ctx context.Context, keyStr string) error { - query := `UPDATE api_keys SET last_used_at = CURRENT_TIMESTAMP WHERE key = $1` - - _, err := ks.db.ExecContext(ctx, query, keyStr) +// UpdateLastUsed updates the last_used_at timestamp for a key digest +func (ks *KeyStoreDB) UpdateLastUsed(ctx context.Context, keyHash string) error { + _, err := ks.db.ExecContext(ctx, + `UPDATE api_keys SET last_used_at = CURRENT_TIMESTAMP WHERE key_hash = $1`, keyHash) if err != nil { return fmt.Errorf("failed to update last used: %w", err) } - return nil } // LoadAllKeys loads all active keys into memory for caching func (ks *KeyStoreDB) LoadAllKeys(ctx context.Context) ([]*APIKey, error) { - query := ` - SELECT key, name, client_id, roles, created_at, last_used_at, expires_at, is_active, rate_limit - FROM api_keys - WHERE is_active = true AND (expires_at IS NULL OR expires_at > CURRENT_TIMESTAMP) - ` - - rows, err := ks.db.QueryContext(ctx, query) + rows, err := ks.db.QueryContext(ctx, + `SELECT `+keyColumns+` FROM api_keys + WHERE is_active = true AND (expires_at IS NULL OR expires_at > CURRENT_TIMESTAMP)`) if err != nil { return nil, fmt.Errorf("failed to load keys: %w", err) } @@ -255,53 +322,52 @@ func (ks *KeyStoreDB) LoadAllKeys(ctx context.Context) ([]*APIKey, error) { var keys []*APIKey for rows.Next() { - var key APIKey - var roles []string - - err := rows.Scan( - &key.Key, - &key.Name, - &key.ClientID, - &roles, - &key.CreatedAt, - &key.LastUsedAt, - &key.ExpiresAt, - &key.IsActive, - &key.RateLimit, - ) + key, err := scanKey(rows) if err != nil { return nil, fmt.Errorf("failed to scan key: %w", err) } - - key.Roles = roles - keys = append(keys, &key) + keys = append(keys, key) } return keys, rows.Err() } -// logAudit logs a key operation to the audit trail -func (ks *KeyStoreDB) logAudit(ctx context.Context, keyStr string, action string, performedBy string, details map[string]interface{}) { - // Get key ID +// logAudit logs a key operation to the audit trail. Failures don't abort the +// calling operation but are logged rather than silently dropped. +func (ks *KeyStoreDB) logAudit(ctx context.Context, keyHash string, action string, performedBy string, details map[string]interface{}) { var keyID int - err := ks.db.QueryRowContext(ctx, "SELECT id FROM api_keys WHERE key = $1", keyStr).Scan(&keyID) - if err != nil { - return // Silently fail audit logging + if err := ks.db.QueryRowContext(ctx, + "SELECT id FROM api_keys WHERE key_hash = $1", keyHash).Scan(&keyID); err != nil { + ks.warnf("audit log: failed to resolve key for action=%s: %v", action, err) + return } - // Log the action - detailsJSON := "{}" + detailsJSON := []byte("{}") if len(details) > 0 { - // Simple JSON encoding (could use jsonb package for robustness) - detailsJSON = fmt.Sprintf(`{"event": "%s"}`, action) + encoded, err := json.Marshal(details) + if err != nil { + ks.warnf("audit log: failed to encode details for action=%s: %v", action, err) + } else { + detailsJSON = encoded + } } - query := ` - INSERT INTO api_key_audit_log (key_id, action, performed_by, details) - VALUES ($1, $2, $3, $4) - ` + if _, err := ks.db.ExecContext(ctx, + `INSERT INTO api_key_audit_log (key_id, action, performed_by, details) VALUES ($1, $2, $3, $4)`, + keyID, action, performedBy, detailsJSON); err != nil { + ks.warnf("audit log: failed to record action=%s: %v", action, err) + } +} - ks.db.ExecContext(ctx, query, keyID, action, performedBy, detailsJSON) +func (ks *KeyStoreDB) warnf(format string, args ...interface{}) { + if ks.logger != nil { + ks.logger.Warnf(format, args...) + } +} + +// Ping verifies database connectivity (used by readiness checks). +func (ks *KeyStoreDB) Ping(ctx context.Context) error { + return ks.db.PingContext(ctx) } // Close closes the database connection @@ -309,101 +375,55 @@ func (ks *KeyStoreDB) Close() error { return ks.db.Close() } -// GetAuditLog retrieves audit log entries for a key -func (ks *KeyStoreDB) GetAuditLog(ctx context.Context, keyStr string, limit int) ([]map[string]interface{}, error) { - query := ` - SELECT al.action, al.performed_by, al.performed_at, al.details - FROM api_key_audit_log al - JOIN api_keys ak ON al.key_id = ak.id - WHERE ak.key = $1 - ORDER BY al.performed_at DESC - LIMIT $2 - ` - - rows, err := ks.db.QueryContext(ctx, query, keyStr, limit) - if err != nil { - return nil, fmt.Errorf("failed to get audit log: %w", err) - } - defer rows.Close() - - var logs []map[string]interface{} - for rows.Next() { - var action, performedBy, details string - var performedAt time.Time - - err := rows.Scan(&action, &performedBy, &performedAt, &details) - if err != nil { - return nil, err - } - - logs = append(logs, map[string]interface{}{ - "action": action, - "performed_by": performedBy, - "performed_at": performedAt, - "details": details, - }) - } - - return logs, rows.Err() +// GetAuditLog retrieves audit log entries for a key digest +func (ks *KeyStoreDB) GetAuditLog(ctx context.Context, keyHash string, limit int) ([]map[string]interface{}, error) { + return ks.auditLogQuery(ctx, + `SELECT al.action, al.performed_by, al.performed_at, al.details + FROM api_key_audit_log al + JOIN api_keys ak ON al.key_id = ak.id + WHERE ak.key_hash = $1 + ORDER BY al.performed_at DESC + LIMIT $2`, keyHash, limit) } // GetKeyByName retrieves an API key by its name func (ks *KeyStoreDB) GetKeyByName(ctx context.Context, name string) (*APIKey, error) { - query := ` - SELECT key, name, client_id, roles, created_at, last_used_at, expires_at, is_active, rate_limit - FROM api_keys - WHERE name = $1 - ` - - var key APIKey - var roles []string - - err := ks.db.QueryRowContext(ctx, query, name).Scan( - &key.Key, - &key.Name, - &key.ClientID, - pq.Array(&roles), - &key.CreatedAt, - &key.LastUsedAt, - &key.ExpiresAt, - &key.IsActive, - &key.RateLimit, - ) + row := ks.db.QueryRowContext(ctx, + `SELECT `+keyColumns+` FROM api_keys WHERE name = $1`, name) + key, err := scanKey(row) if err == sql.ErrNoRows { return nil, ErrKeyNotFound } if err != nil { return nil, fmt.Errorf("failed to get key by name: %w", err) } - - key.Roles = roles - return &key, nil + return key, nil } // DeactivateKeyByName disables an API key by its name func (ks *KeyStoreDB) DeactivateKeyByName(ctx context.Context, name string, deactivatedBy string) error { - // First get the key to find its raw key for cache invalidation and audit key, err := ks.GetKeyByName(ctx, name) if err != nil { return err } - return ks.DeactivateKey(ctx, key.Key, deactivatedBy) + return ks.DeactivateKeyByHash(ctx, key.KeyHash, deactivatedBy) } // GetAuditLogByName retrieves audit log entries for a key identified by name func (ks *KeyStoreDB) GetAuditLogByName(ctx context.Context, name string, limit int) ([]map[string]interface{}, error) { - query := ` - SELECT al.action, al.performed_by, al.performed_at, al.details - FROM api_key_audit_log al - JOIN api_keys ak ON al.key_id = ak.id - WHERE ak.name = $1 - ORDER BY al.performed_at DESC - LIMIT $2 - ` + return ks.auditLogQuery(ctx, + `SELECT al.action, al.performed_by, al.performed_at, al.details + FROM api_key_audit_log al + JOIN api_keys ak ON al.key_id = ak.id + WHERE ak.name = $1 + ORDER BY al.performed_at DESC + LIMIT $2`, name, limit) +} - rows, err := ks.db.QueryContext(ctx, query, name, limit) +func (ks *KeyStoreDB) auditLogQuery(ctx context.Context, query string, ident string, limit int) ([]map[string]interface{}, error) { + rows, err := ks.db.QueryContext(ctx, query, ident, limit) if err != nil { return nil, fmt.Errorf("failed to get audit log: %w", err) } @@ -414,8 +434,7 @@ func (ks *KeyStoreDB) GetAuditLogByName(ctx context.Context, name string, limit var action, performedBy, details string var performedAt time.Time - err := rows.Scan(&action, &performedBy, &performedAt, &details) - if err != nil { + if err := rows.Scan(&action, &performedBy, &performedAt, &details); err != nil { return nil, err } @@ -429,8 +448,3 @@ func (ks *KeyStoreDB) GetAuditLogByName(ctx context.Context, name string, limit return logs, rows.Err() } - -// Custom errors -var ( - ErrKeyNotFound = fmt.Errorf("API key not found") -) diff --git a/internal/auth/keystore_hybrid.go b/internal/auth/keystore_hybrid.go index 1934546..450fa34 100644 --- a/internal/auth/keystore_hybrid.go +++ b/internal/auth/keystore_hybrid.go @@ -2,128 +2,180 @@ package auth import ( "context" + "errors" "fmt" "sync" "time" ) -// HybridKeyStore combines in-memory cache with persistent database backend -// Write-through strategy: writes go to DB first, then cache is updated -// This ensures consistency: if DB write fails, cache is not updated +// keyDatabase is the persistence surface HybridKeyStore needs. *KeyStoreDB +// implements it; tests substitute a fake. +type keyDatabase interface { + SaveKey(ctx context.Context, key *APIKey, createdBy string) error + GetKeyByHash(ctx context.Context, keyHash string) (*APIKey, error) + GetKeyByName(ctx context.Context, name string) (*APIKey, error) + ListKeys(ctx context.Context, clientID string) ([]*APIKey, error) + DeactivateKeyByHash(ctx context.Context, keyHash string, deactivatedBy string) error + UpdateLastUsed(ctx context.Context, keyHash string) error + LoadAllKeys(ctx context.Context) ([]*APIKey, error) + GetAuditLog(ctx context.Context, keyHash string, limit int) ([]map[string]interface{}, error) + GetAuditLogByName(ctx context.Context, name string, limit int) ([]map[string]interface{}, error) + Close() error +} + +// HybridKeyStore combines an in-memory cache with an optional persistent +// database backend. Write-through strategy: writes go to the DB first and the +// cache is only updated after the DB write succeeds. Without a database the +// store degrades to in-memory-only operation. type HybridKeyStore struct { - cache *APIKeyStore // In-memory cache for fast lookups - db *KeyStoreDB // Database backend for persistence + cache *APIKeyStore + db keyDatabase // nil when no database is configured mu sync.RWMutex } -// NewHybridKeyStore creates a new hybrid key store +// NewHybridKeyStore creates a new hybrid key store. db may be nil, in which +// case all operations are served from the in-memory cache only. func NewHybridKeyStore(cache *APIKeyStore, db *KeyStoreDB) *HybridKeyStore { - return &HybridKeyStore{ - cache: cache, - db: db, + h := &HybridKeyStore{cache: cache} + if db != nil { + h.db = db } + return h } -// InitializeFromDatabase loads all keys from database into cache at startup -func (h *HybridKeyStore) InitializeFromDatabase(ctx context.Context) error { +// HasDatabase reports whether a persistent backend is configured. +func (h *HybridKeyStore) HasDatabase() bool { + return h.db != nil +} + +// InitializeFromDatabase loads all active keys from the database into the +// cache. Call once at startup so previously issued keys survive restarts. +// Returns the number of keys loaded. +func (h *HybridKeyStore) InitializeFromDatabase(ctx context.Context) (int, error) { + if h.db == nil { + return 0, nil + } + keys, err := h.db.LoadAllKeys(ctx) if err != nil { - return fmt.Errorf("failed to load keys from database: %w", err) + return 0, fmt.Errorf("failed to load keys from database: %w", err) } for _, key := range keys { - h.cache.keys[key.Key] = key - rateLimit := key.RateLimit - if rateLimit <= 0 { - rateLimit = 100 // Default rate limit - } - h.cache.rateLimits[key.Key] = &RateLimiter{ - maxRequests: rateLimit, - window: time.Minute, - resetTime: time.Now().Add(time.Minute), - count: 0, - } + h.cache.RegisterKey(key) } - return nil + return len(keys), nil } -// CreateKey generates a new API key and persists it -// Returns error if database write fails +// CreateKey generates a new API key, persists it (when a database is +// configured), and only then registers it in the cache. If the database write +// fails the key is not usable anywhere. func (h *HybridKeyStore) CreateKey(ctx context.Context, clientID string, roles []string, rateLimit int, expiresIn *time.Duration, createdBy string) (*APIKey, error) { h.mu.Lock() defer h.mu.Unlock() - // Generate random key in memory - apiKey, err := h.generateKey(clientID, roles, rateLimit, expiresIn) + apiKey, err := generateAPIKey(clientID, roles, rateLimit, expiresIn) if err != nil { return nil, err } - // Write to database first (consistency) - if err := h.db.SaveKey(ctx, apiKey, createdBy); err != nil { - return nil, err - } - - // Update cache after successful DB write - h.cache.keys[apiKey.Key] = apiKey - h.cache.rateLimits[apiKey.Key] = &RateLimiter{ - maxRequests: rateLimit, - window: time.Minute, - resetTime: time.Now().Add(time.Minute), - count: 0, + if h.db != nil { + if err := h.db.SaveKey(ctx, apiKey, createdBy); err != nil { + return nil, err + } } + h.cache.RegisterKey(apiKey) return apiKey, nil } -// ValidateKey checks if a key is valid -// Checks cache first for performance, falls back to database if cache miss -func (h *HybridKeyStore) ValidateKey(keyStr string) (*APIKey, error) { - // Check cache first (fast path) - h.cache.mu.RLock() - key, exists := h.cache.keys[keyStr] - h.cache.mu.RUnlock() - - if exists { - if h.isKeyValid(key) { - return key, nil - } - return nil, fmt.Errorf("key is inactive or expired") +// EnsurePersisted upserts an externally created key (e.g. a bootstrap admin +// key loaded from a Kubernetes secret) into the database, if one is configured. +func (h *HybridKeyStore) EnsurePersisted(ctx context.Context, apiKey *APIKey, createdBy string) error { + if h.db == nil { + return nil } - - // Cache miss - this is normal in distributed deployments - // Could implement database fallback here if needed: - // key, err := h.db.GetKey(context.Background(), keyStr) - // But for now, rely on cache being populated at startup - - return nil, fmt.Errorf("API key not found") + return h.db.SaveKey(ctx, apiKey, createdBy) } -// ListKeys returns all active keys for a client +// ValidateKey checks if a raw key is valid. The cache is consulted first; on +// a miss the database is checked and the cache repopulated, so keys created +// by another instance (or before a restart) still authenticate. +func (h *HybridKeyStore) ValidateKey(ctx context.Context, keyStr string) (*APIKey, error) { + if key, err := h.cache.ValidateKey(keyStr); err == nil { + return key, nil + } else if !errors.Is(err, ErrInvalidKey) { + // Present in cache but inactive/expired — no point hitting the DB. + return nil, err + } + + if h.db == nil { + return nil, ErrKeyNotFound + } + + key, err := h.db.GetKeyByHash(ctx, HashKey(keyStr)) + if err != nil { + return nil, err + } + + h.cache.RegisterKey(key) + return h.cache.ValidateKey(keyStr) +} + +// ListKeys returns all active keys for a client. func (h *HybridKeyStore) ListKeys(ctx context.Context, clientID string) ([]*APIKey, error) { + if h.db == nil { + return h.cache.ListKeys(clientID), nil + } return h.db.ListKeys(ctx, clientID) } -// DeactivateKey deactivates a key in both cache and database -func (h *HybridKeyStore) DeactivateKey(ctx context.Context, keyStr string, deactivatedBy string) error { +// DeactivateKeyByName deactivates a key identified by name in both the +// database and the cache. +func (h *HybridKeyStore) DeactivateKeyByName(ctx context.Context, name string, deactivatedBy string) error { h.mu.Lock() defer h.mu.Unlock() - // Remove from cache first - h.cache.mu.Lock() - delete(h.cache.keys, keyStr) - delete(h.cache.rateLimits, keyStr) - h.cache.mu.Unlock() + if h.db == nil { + return h.deactivateCachedByName(name) + } - // Update database - return h.db.DeactivateKey(ctx, keyStr, deactivatedBy) + key, err := h.db.GetKeyByName(ctx, name) + if err != nil { + return err + } + + if err := h.db.DeactivateKeyByHash(ctx, key.KeyHash, deactivatedBy); err != nil { + return err + } + + h.cache.RemoveKeyByHash(key.KeyHash) + return nil } -// UpdateLastUsed updates the last used timestamp in database -// Cache is not updated to avoid contention +// deactivateCachedByName handles revocation when running without a database. +func (h *HybridKeyStore) deactivateCachedByName(name string) error { + h.cache.mu.Lock() + defer h.cache.mu.Unlock() + + for hash, key := range h.cache.keys { + if key.Name == name { + delete(h.cache.keys, hash) + delete(h.cache.rateLimits, hash) + return nil + } + } + return ErrKeyNotFound +} + +// UpdateLastUsed updates the last used timestamp in the database. +// Cache is not updated to avoid contention. func (h *HybridKeyStore) UpdateLastUsed(ctx context.Context, keyStr string) error { - return h.db.UpdateLastUsed(ctx, keyStr) + if h.db == nil { + return h.cache.UpdateLastUsed(keyStr) + } + return h.db.UpdateLastUsed(ctx, HashKey(keyStr)) } // CheckRateLimit checks if a key has exceeded its rate limit @@ -131,69 +183,37 @@ func (h *HybridKeyStore) CheckRateLimit(keyStr string) (bool, error) { return h.cache.CheckRateLimit(keyStr) } -// GetAuditLog retrieves audit log for a key +// GetAuditLog retrieves audit log for a raw key func (h *HybridKeyStore) GetAuditLog(ctx context.Context, keyStr string, limit int) ([]map[string]interface{}, error) { - return h.db.GetAuditLog(ctx, keyStr, limit) -} - -// DeactivateKeyByName deactivates a key by its name (avoids exposing raw key in URLs) -func (h *HybridKeyStore) DeactivateKeyByName(ctx context.Context, name string, deactivatedBy string) error { - h.mu.Lock() - defer h.mu.Unlock() - - // Look up the key by name in DB to get the raw key for cache invalidation - key, err := h.db.GetKeyByName(ctx, name) - if err != nil { - return err + if h.db == nil { + return nil, fmt.Errorf("audit log requires a database backend") } - - // Remove from cache - h.cache.mu.Lock() - delete(h.cache.keys, key.Key) - delete(h.cache.rateLimits, key.Key) - h.cache.mu.Unlock() - - // Deactivate in database - return h.db.DeactivateKey(ctx, key.Key, deactivatedBy) + return h.db.GetAuditLog(ctx, HashKey(keyStr), limit) } // GetAuditLogByName retrieves audit log for a key identified by name func (h *HybridKeyStore) GetAuditLogByName(ctx context.Context, name string, limit int) ([]map[string]interface{}, error) { + if h.db == nil { + return nil, fmt.Errorf("audit log requires a database backend") + } return h.db.GetAuditLogByName(ctx, name, limit) } -// Close closes the database connection +// Close closes the database connection, if any. func (h *HybridKeyStore) Close() error { + if h.db == nil { + return nil + } return h.db.Close() } -// Helper functions - -// generateKey creates an APIKey with cryptographic random bytes -func (h *HybridKeyStore) generateKey(clientID string, roles []string, rateLimit int, expiresIn *time.Duration) (*APIKey, error) { - apiKey, err := h.cache.CreateKey(clientID, roles, rateLimit, expiresIn) - if err != nil { - return nil, err - } - return apiKey, nil -} - -// isKeyValid checks if a key is currently valid -func (h *HybridKeyStore) isKeyValid(key *APIKey) bool { - if !key.IsActive { - return false - } - - if key.ExpiresAt != nil && time.Now().After(*key.ExpiresAt) { - return false - } - - return true -} - -// SyncCache performs a full cache refresh from database -// Useful for multi-instance deployments where keys may be created elsewhere +// SyncCache performs a full cache refresh from the database. Useful for +// multi-instance deployments where keys may be created or revoked elsewhere. func (h *HybridKeyStore) SyncCache(ctx context.Context) error { + if h.db == nil { + return nil + } + h.mu.Lock() defer h.mu.Unlock() @@ -202,26 +222,14 @@ func (h *HybridKeyStore) SyncCache(ctx context.Context) error { return fmt.Errorf("failed to sync cache: %w", err) } - // Clear cache h.cache.mu.Lock() h.cache.keys = make(map[string]*APIKey) h.cache.rateLimits = make(map[string]*RateLimiter) - - // Repopulate cache - for _, key := range keys { - h.cache.keys[key.Key] = key - rateLimit := key.RateLimit - if rateLimit <= 0 { - rateLimit = 100 - } - h.cache.rateLimits[key.Key] = &RateLimiter{ - maxRequests: rateLimit, - window: time.Minute, - resetTime: time.Now().Add(time.Minute), - count: 0, - } - } h.cache.mu.Unlock() + for _, key := range keys { + h.cache.RegisterKey(key) + } + return nil } diff --git a/internal/auth/keystore_test.go b/internal/auth/keystore_test.go new file mode 100644 index 0000000..3211f47 --- /dev/null +++ b/internal/auth/keystore_test.go @@ -0,0 +1,320 @@ +package auth + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/igodwin/notifier/internal/logging" +) + +// fakeKeyDB is an in-memory keyDatabase for exercising HybridKeyStore. +type fakeKeyDB struct { + byHash map[string]*APIKey + saveErr error + saveCnt int + closeCnt int +} + +func newFakeKeyDB() *fakeKeyDB { + return &fakeKeyDB{byHash: make(map[string]*APIKey)} +} + +func (f *fakeKeyDB) SaveKey(_ context.Context, key *APIKey, _ string) error { + f.saveCnt++ + if f.saveErr != nil { + return f.saveErr + } + stored := *key + stored.Key = "" + f.byHash[key.KeyHash] = &stored + return nil +} + +func (f *fakeKeyDB) GetKeyByHash(_ context.Context, keyHash string) (*APIKey, error) { + key, ok := f.byHash[keyHash] + if !ok { + return nil, ErrKeyNotFound + } + return key, nil +} + +func (f *fakeKeyDB) GetKeyByName(_ context.Context, name string) (*APIKey, error) { + for _, key := range f.byHash { + if key.Name == name { + return key, nil + } + } + return nil, ErrKeyNotFound +} + +func (f *fakeKeyDB) ListKeys(_ context.Context, clientID string) ([]*APIKey, error) { + var keys []*APIKey + for _, key := range f.byHash { + if key.ClientID == clientID && key.IsActive { + keys = append(keys, key) + } + } + return keys, nil +} + +func (f *fakeKeyDB) DeactivateKeyByHash(_ context.Context, keyHash string, _ string) error { + key, ok := f.byHash[keyHash] + if !ok { + return ErrKeyNotFound + } + key.IsActive = false + return nil +} + +func (f *fakeKeyDB) UpdateLastUsed(_ context.Context, keyHash string) error { return nil } + +func (f *fakeKeyDB) LoadAllKeys(_ context.Context) ([]*APIKey, error) { + var keys []*APIKey + for _, key := range f.byHash { + if key.IsActive { + keys = append(keys, key) + } + } + return keys, nil +} + +func (f *fakeKeyDB) GetAuditLog(_ context.Context, _ string, _ int) ([]map[string]interface{}, error) { + return nil, nil +} + +func (f *fakeKeyDB) GetAuditLogByName(_ context.Context, _ string, _ int) ([]map[string]interface{}, error) { + return nil, nil +} + +func (f *fakeKeyDB) Close() error { + f.closeCnt++ + return nil +} + +func newHybridWithFake(db keyDatabase) (*HybridKeyStore, *APIKeyStore) { + cache := NewAPIKeyStore() + h := &HybridKeyStore{cache: cache, db: db} + return h, cache +} + +func TestAPIKeyStoreDoesNotRetainRawKey(t *testing.T) { + store := NewAPIKeyStore() + apiKey, err := store.CreateKey("client-a", []string{"notify-email"}, 10, nil) + if err != nil { + t.Fatalf("CreateKey: %v", err) + } + if apiKey.Key == "" { + t.Fatal("creation response must include the raw key") + } + + // The store must be indexed by digest, not raw key, and stored copies + // must not carry the raw secret. + store.mu.RLock() + defer store.mu.RUnlock() + if _, ok := store.keys[apiKey.Key]; ok { + t.Error("store is keyed by raw key; expected digest") + } + stored, ok := store.keys[apiKey.KeyHash] + if !ok { + t.Fatal("store missing entry under key digest") + } + if stored.Key != "" { + t.Error("stored record retains raw key") + } + if stored.KeyPreview == "" { + t.Error("stored record missing preview") + } +} + +func TestValidateKeyByRawValue(t *testing.T) { + store := NewAPIKeyStore() + apiKey, err := store.CreateKey("client-a", []string{"admin"}, 0, nil) + if err != nil { + t.Fatalf("CreateKey: %v", err) + } + + got, err := store.ValidateKey(apiKey.Key) + if err != nil { + t.Fatalf("ValidateKey with raw key: %v", err) + } + if got.ClientID != "client-a" { + t.Errorf("ClientID = %q, want client-a", got.ClientID) + } + + if _, err := store.ValidateKey("nk_bogus"); !errors.Is(err, ErrInvalidKey) { + t.Errorf("bogus key error = %v, want ErrInvalidKey", err) + } + + if err := store.DeactivateKey(apiKey.Key); err != nil { + t.Fatalf("DeactivateKey: %v", err) + } + if _, err := store.ValidateKey(apiKey.Key); !errors.Is(err, ErrKeyInactive) { + t.Errorf("inactive key error = %v, want ErrKeyInactive", err) + } +} + +func TestValidateKeyExpiry(t *testing.T) { + store := NewAPIKeyStore() + expires := -time.Minute // already expired + apiKey, err := store.CreateKey("client-a", []string{"admin"}, 0, &expires) + if err != nil { + t.Fatalf("CreateKey: %v", err) + } + if _, err := store.ValidateKey(apiKey.Key); !errors.Is(err, ErrKeyExpired) { + t.Errorf("expired key error = %v, want ErrKeyExpired", err) + } +} + +func TestHybridCreateKeyIsWriteThrough(t *testing.T) { + db := newFakeKeyDB() + db.saveErr = errors.New("db down") + h, cache := newHybridWithFake(db) + + _, err := h.CreateKey(context.Background(), "client-a", []string{"admin"}, 0, nil, "tester") + if err == nil { + t.Fatal("expected error when DB write fails") + } + + // The failed key must not be usable from the cache. + cache.mu.RLock() + n := len(cache.keys) + cache.mu.RUnlock() + if n != 0 { + t.Errorf("cache has %d key(s) after failed DB write, want 0", n) + } +} + +func TestHybridCreateKeySucceedsAndCaches(t *testing.T) { + db := newFakeKeyDB() + h, _ := newHybridWithFake(db) + + apiKey, err := h.CreateKey(context.Background(), "client-a", []string{"admin"}, 0, nil, "tester") + if err != nil { + t.Fatalf("CreateKey: %v", err) + } + if db.saveCnt != 1 { + t.Errorf("saveCnt = %d, want 1", db.saveCnt) + } + if _, err := h.ValidateKey(context.Background(), apiKey.Key); err != nil { + t.Errorf("ValidateKey after create: %v", err) + } +} + +func TestHybridValidateKeyFallsBackToDatabase(t *testing.T) { + db := newFakeKeyDB() + h, cache := newHybridWithFake(db) + + // Simulate a key created before a restart: present in DB, absent in cache. + apiKey, err := generateAPIKey("client-a", []string{"admin"}, 5, nil) + if err != nil { + t.Fatalf("generateAPIKey: %v", err) + } + if err := db.SaveKey(context.Background(), apiKey, "tester"); err != nil { + t.Fatalf("SaveKey: %v", err) + } + + got, err := h.ValidateKey(context.Background(), apiKey.Key) + if err != nil { + t.Fatalf("ValidateKey via DB fallback: %v", err) + } + if got.ClientID != "client-a" { + t.Errorf("ClientID = %q, want client-a", got.ClientID) + } + + // The fallback must repopulate the cache (including a rate limiter). + if _, err := cache.ValidateKey(apiKey.Key); err != nil { + t.Errorf("cache not repopulated after fallback: %v", err) + } + if ok, err := h.CheckRateLimit(apiKey.Key); err != nil || !ok { + t.Errorf("CheckRateLimit after fallback: ok=%v err=%v", ok, err) + } +} + +func TestHybridInitializeFromDatabase(t *testing.T) { + db := newFakeKeyDB() + h, cache := newHybridWithFake(db) + + for i := 0; i < 3; i++ { + apiKey, err := generateAPIKey("client-a", []string{"admin"}, 0, nil) + if err != nil { + t.Fatalf("generateAPIKey: %v", err) + } + if err := db.SaveKey(context.Background(), apiKey, "tester"); err != nil { + t.Fatalf("SaveKey: %v", err) + } + } + + loaded, err := h.InitializeFromDatabase(context.Background()) + if err != nil { + t.Fatalf("InitializeFromDatabase: %v", err) + } + if loaded != 3 { + t.Errorf("loaded = %d, want 3", loaded) + } + cache.mu.RLock() + n := len(cache.keys) + cache.mu.RUnlock() + if n != 3 { + t.Errorf("cache has %d key(s), want 3", n) + } +} + +func TestHybridWithoutDatabase(t *testing.T) { + h := NewHybridKeyStore(NewAPIKeyStore(), nil) + + if h.HasDatabase() { + t.Fatal("HasDatabase() = true with nil db") + } + + // All of these must work (or fail cleanly) without panicking. + apiKey, err := h.CreateKey(context.Background(), "client-a", []string{"admin"}, 0, nil, "tester") + if err != nil { + t.Fatalf("CreateKey without db: %v", err) + } + if _, err := h.ValidateKey(context.Background(), apiKey.Key); err != nil { + t.Errorf("ValidateKey without db: %v", err) + } + if keys, err := h.ListKeys(context.Background(), "client-a"); err != nil || len(keys) != 1 { + t.Errorf("ListKeys without db: keys=%d err=%v", len(keys), err) + } + if err := h.UpdateLastUsed(context.Background(), apiKey.Key); err != nil { + t.Errorf("UpdateLastUsed without db: %v", err) + } + if err := h.DeactivateKeyByName(context.Background(), apiKey.Name, "tester"); err != nil { + t.Errorf("DeactivateKeyByName without db: %v", err) + } + if _, err := h.ValidateKey(context.Background(), apiKey.Key); err == nil { + t.Error("key still valid after revocation") + } + if _, err := h.GetAuditLogByName(context.Background(), apiKey.Name, 10); err == nil { + t.Error("expected audit log error without db") + } + if err := h.Close(); err != nil { + t.Errorf("Close without db: %v", err) + } +} + +func TestRegisterAdminKeyInMemoryHashesKey(t *testing.T) { + store := NewAPIKeyStore() + logger, _ := logging.NewFromConfig("error", "stdout") + + raw := "nk_test_admin_key_value" + apiKey, err := RegisterAdminKeyInMemory(store, raw, logger) + if err != nil { + t.Fatalf("RegisterAdminKeyInMemory: %v", err) + } + if apiKey.KeyHash != HashKey(raw) { + t.Error("registered key missing correct digest") + } + if _, err := store.ValidateKey(raw); err != nil { + t.Errorf("ValidateKey after admin registration: %v", err) + } + + store.mu.RLock() + defer store.mu.RUnlock() + if _, ok := store.keys[raw]; ok { + t.Error("admin key stored under raw value; expected digest") + } +}