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 <noreply@anthropic.com>
This commit is contained in:
+146
-138
@@ -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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user