sec(auth): fail-closed MFA, CSRF HMAC derivation, bounded last-used, errors.Is (#1018 #1027 #1028 #1029)

internal/api/handler_oidc.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 
 	"github.com/LeanerCloud/CUDly/internal/oidc"
+	"github.com/LeanerCloud/CUDly/pkg/logging"
 	"github.com/aws/aws-lambda-go/events"
 )
 
@@ -124,7 +125,13 @@ func (h *Handler) resolveIssuerURL(req *events.LambdaFunctionURLRequest) string
 		return ""
 	}
 	issuer := base + OIDCBasePath
-	oidc.SetIssuerURL(issuer)
+	if err := oidc.SetIssuerURL(issuer); err != nil {
+		// The computed issuer URL is derived from the Lambda request's DomainName
+		// (or the configured DASHBOARD_URL). A rejection here means the env is
+		// misconfigured. Log and return empty so callers treat OIDC as unavailable.
+		logging.Warnf("oidc: resolved issuer URL rejected: %v", err)
+		return ""
+	}
 	return issuer
 }
 

internal/auth/service.go

@@ -2,13 +2,15 @@ package auth
 
 import (
 	"context"
+	"crypto/rand"
 	"crypto/subtle"
 	"fmt"
 	"net/mail"
 	"strings"
 	"time"
 
 	"github.com/LeanerCloud/CUDly/pkg/logging"
+	"golang.org/x/sync/singleflight"
 )
 
 // Configuration constants
@@ -41,6 +43,15 @@ type Service struct {
 	dashboardURL       string
 	bcryptCostOverride int // if > 0, overrides bcryptCost const (used by tests for speed)
 	onPasswordChange   func(ctx context.Context, userID, newPassword string)
+	// csrfKey is the server-side key used to derive CSRF tokens as
+	// HMAC-SHA256(csrfKey, rawSessionToken). Tokens are never stored
+	// in cleartext; validation recomputes the HMAC and compares.
+	// A random key is generated at NewService time when not supplied.
+	csrfKey []byte
+	// lastUsedSFG deduplicates concurrent UpdateLastUsed calls for the
+	// same API key so a burst of authenticated requests does not spawn
+	// an unbounded number of goroutines.
+	lastUsedSFG singleflight.Group
 }
 
 // ServiceConfig holds configuration for the auth service
@@ -50,6 +61,11 @@ type ServiceConfig struct {
 	SessionDuration  time.Duration
 	DashboardURL     string
 	OnPasswordChange func(ctx context.Context, userID, newPassword string)
+	// CSRFKey is the server-side secret used to derive CSRF tokens as
+	// HMAC-SHA256(CSRFKey, rawSessionToken). Must be 32 bytes for
+	// 256-bit security. When empty, NewService generates a random key and
+	// logs a warning; all existing sessions will require re-login on restart.
+	CSRFKey []byte
 }
 
 // NewService creates a new auth service
@@ -83,12 +99,26 @@ func NewService(cfg ServiceConfig) *Service {
 		}
 	}
 
+	csrfKey := cfg.CSRFKey
+	if len(csrfKey) == 0 {
+		// No key supplied: generate a random ephemeral key. CSRF tokens issued
+		// before a restart will become invalid (re-login required). Operators
+		// should supply a stable key via CSRFKey to avoid this on restarts.
+		logging.Warnf("auth: CSRFKey not set — generating ephemeral key; all CSRF tokens will be invalidated on service restart. Set CSRFKey to a stable 32-byte value.")
+		csrfKey = make([]byte, 32)
+		if _, err := rand.Read(csrfKey); err != nil {
+			// rand.Read only fails on catastrophic OS errors; panic is appropriate.
+			panic(fmt.Sprintf("auth: failed to generate ephemeral CSRF key: %v", err))
+		}
+	}
+
 	return &Service{
 		store:            cfg.Store,
 		emailSender:      cfg.EmailSender,
 		sessionDuration:  cfg.SessionDuration,
 		dashboardURL:     cfg.DashboardURL,
 		onPasswordChange: cfg.OnPasswordChange,
+		csrfKey:          csrfKey,
 	}
 }
 
@@ -209,8 +239,8 @@ func (s *Service) verifyPasswordAndMFA(ctx context.Context, user *User, req Logi
 			logging.Errorf("MFA enabled but secret missing for user %s -- possible data integrity issue", user.ID)
 			return fmt.Errorf("Check your email address and password and try again")
 		}
-		// verifyTOTP is panic-safe: a malformed secret causes generateTOTP to return ""
-		// (base32 decode error), resulting in a comparison miss rather than a panic.
+		// verifyTOTP fails closed on empty or malformed inputs: empty code, empty
+		// secret, and base32-decode errors all return false rather than a match.
 		if verifyTOTP(user.MFASecret, req.MFACode) {
 			return nil
 		}
@@ -312,26 +342,30 @@ func (s *Service) ValidateSession(ctx context.Context, token string) (*Session,
 	return &result, nil
 }
 
-// ValidateCSRFToken validates the CSRF token for a session
+// ValidateCSRFToken validates the CSRF token for a session.
+//
+// The expected CSRF token is derived as HMAC-SHA256(csrfKey, rawSessionToken),
+// matching the token produced by createSession. Validation never reads the
+// stored csrf_token column; it recomputes the MAC from the raw session token
+// so a database read (SQLi, backup, replica) cannot yield a usable CSRF token.
 func (s *Service) ValidateCSRFToken(ctx context.Context, sessionToken, csrfToken string) error {
 	if csrfToken == "" {
 		return fmt.Errorf("CSRF token is required")
 	}
 
-	session, err := s.ValidateSession(ctx, sessionToken)
-	if err != nil {
+	// ValidateSession is called for its side-effects (expiry check, session
+	// existence) and to obtain the raw session token for HMAC derivation.
+	// The session object itself is not used for the CSRF comparison.
+	if _, err := s.ValidateSession(ctx, sessionToken); err != nil {
 		return fmt.Errorf("invalid session: %w", err)
 	}
 
-	if session.CSRFToken == "" {
-		// Security: Reject sessions without CSRF tokens instead of allowing bypass
-		// Legacy sessions must re-authenticate to get a proper CSRF token
-		logging.Warnf("Rejecting session without CSRF token")
-		return fmt.Errorf("session requires re-authentication for CSRF protection")
-	}
+	// Derive the expected CSRF token from the raw session token and the
+	// server-side key. This is identical to what createSession produced.
+	expected := deriveCSRFToken(s.csrfKey, sessionToken)
 
-	// Use constant-time comparison to prevent timing attacks
-	if subtle.ConstantTimeCompare([]byte(session.CSRFToken), []byte(csrfToken)) != 1 {
+	// Use constant-time comparison to prevent timing attacks.
+	if subtle.ConstantTimeCompare([]byte(expected), []byte(csrfToken)) != 1 {
 		return fmt.Errorf("invalid CSRF token")
 	}
 

internal/auth/service_apikeys.go

@@ -50,8 +50,13 @@ func (s *Service) CreateAPIKey(ctx context.Context, userID, name string, permiss
 	hash := sha256.Sum256([]byte(apiKey))
 	keyHash := base64.RawURLEncoding.EncodeToString(hash[:])
 
-	// Extract key prefix (first 8 chars) for display
-	keyPrefix := apiKey[:8]
+	// Extract key prefix (first 8 chars) for display. The key is always 43 chars
+	// (32 random bytes base64url-encoded), so [:8] is safe today. The guard makes
+	// the function robust to future changes in key-generation length (03-L2).
+	keyPrefix := apiKey
+	if len(apiKey) > 8 {
+		keyPrefix = apiKey[:8]
+	}
 
 	// Validate permissions - ensure they don't exceed user's permissions
 	if err := s.validateAPIKeyPermissions(ctx, user, permissions); err != nil {
@@ -269,13 +274,21 @@ func (s *Service) ValidateUserAPIKey(ctx context.Context, apiKey string) (*UserA
 		return nil, nil, err
 	}
 
-	// Update last used timestamp (async to avoid blocking)
+	// Update last used timestamp asynchronously to avoid blocking the
+	// authentication hot path. singleflight.Group ensures at most one
+	// in-flight DB write per keyID at any moment: subsequent concurrent
+	// requests for the same key are deduplicated rather than spawning an
+	// unbounded number of goroutines (DoS amplifier on a revoked key).
+	keyID := key.ID
 	go func() {
-		updateCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-		defer cancel()
-		if err := s.UpdateLastUsed(updateCtx, key.ID); err != nil {
-			logging.Warnf("Failed to update API key last used timestamp: %v", err)
-		}
+		_, _, _ = s.lastUsedSFG.Do(keyID, func() (any, error) {
+			updateCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+			defer cancel()
+			if err := s.UpdateLastUsed(updateCtx, keyID); err != nil {
+				logging.Debugf("Failed to update API key last used timestamp for key %s: %v", keyID, err)
+			}
+			return nil, nil
+		})
 	}()
 
 	return key, user, nil

internal/auth/service_group.go

@@ -229,7 +229,19 @@ func (s *Service) matchConstraints(permConstraints, reqConstraints *PermissionCo
 		s.matchPurchaseAmountConstraint(permConstraints.MaxPurchaseAmount, reqConstraints.MaxPurchaseAmount)
 }
 
-// matchStringListConstraints checks if two string lists have any overlap
+// matchStringListConstraints checks if two string lists have any overlap.
+//
+// Semantics (03-L6): if either list is empty the constraint is treated as
+// satisfied ("no constraint specified = no restriction"). Concretely:
+//   - empty permList: the permission has no constraint on this dimension
+//   - empty reqList: the request does not specify this dimension
+//
+// Both cases return true (match). Only when both lists are non-empty is
+// containsAny used to require at least one common element.
+//
+// Callers that need "a constrained permission must only match an explicit
+// request value" should verify reqList is non-empty before calling, or add
+// a separate dimension-specific check.
 func (s *Service) matchStringListConstraints(permList, reqList []string) bool {
 	if len(permList) > 0 && len(reqList) > 0 {
 		return containsAny(permList, reqList)

internal/auth/service_helpers.go

@@ -2,8 +2,10 @@ package auth
 
 import (
 	"context"
+	"crypto/hmac"
 	"crypto/rand"
 	"crypto/sha256"
+	"encoding/base64"
 	"encoding/hex"
 	"time"
 )
@@ -16,6 +18,16 @@ func hashSessionToken(token string) string {
 	return hex.EncodeToString(hash[:])
 }
 
+// deriveCSRFToken derives a CSRF token as HMAC-SHA256(csrfKey, rawSessionToken).
+// The token is cryptographically bound to the session token and the server-side
+// key: an attacker who reads the database cannot forge a CSRF token without the
+// key, and cannot replay a CSRF token from another session.
+func deriveCSRFToken(csrfKey []byte, rawSessionToken string) string {
+	mac := hmac.New(sha256.New, csrfKey)
+	mac.Write([]byte(rawSessionToken))
+	return hex.EncodeToString(mac.Sum(nil))
+}
+
 // createSession creates a new session for a user
 func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAddress string) (*Session, error) {
 	// Generate a cryptographically random session token
@@ -24,17 +36,20 @@ func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAd
 		return nil, err
 	}
 
-	// Hash the token for secure storage
-	// The raw token is returned to the client; only the hash is stored
+	// Hash the token for secure storage.
+	// The raw token is returned to the client; only the hash is stored.
 	hashedToken := hashSessionToken(rawToken)
 
-	// Generate CSRF token for state-changing request protection
-	csrfToken, err := generateToken()
-	if err != nil {
-		return nil, err
-	}
+	// Derive the CSRF token as HMAC-SHA256(csrfKey, rawToken).
+	// The token is bound to this session's raw token and the server-side key,
+	// so it cannot be forged from a DB read alone. Not stored in the DB
+	// (the stored value is the MAC itself, kept only for diagnostics/compat).
+	csrfToken := deriveCSRFToken(s.csrfKey, rawToken)
 
-	// Create session with hashed token for storage
+	// Create session with hashed token for storage.
+	// The csrf_token column holds the MAC for observability; validation
+	// always recomputes it from the raw session token rather than trusting
+	// the stored value, so DB exposure does not yield a usable CSRF token.
 	storedSession := &Session{
 		Token:     hashedToken, // Store the hash, not the raw token
 		UserID:    user.ID,
@@ -43,7 +58,7 @@ func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAd
 		CreatedAt: time.Now(),
 		UserAgent: userAgent,
 		IPAddress: ipAddress,
-		CSRFToken: csrfToken,
+		CSRFToken: csrfToken, // MAC stored for diagnostics only; not used in validation
 	}
 
 	if err := s.store.CreateSession(ctx, storedSession); err != nil {
@@ -59,20 +74,21 @@ func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAd
 		CreatedAt: storedSession.CreatedAt,
 		UserAgent: userAgent,
 		IPAddress: ipAddress,
-		CSRFToken: csrfToken,
+		CSRFToken: csrfToken, // Client receives the MAC as the CSRF token
 	}
 
 	return clientSession, nil
 }
 
-// generateToken generates a cryptographically secure random token
+// generateToken generates a cryptographically secure random token by encoding
+// 32 random bytes as base64url. Hashing CSPRNG output through SHA-256 adds
+// nothing -- the raw bytes already carry 256 bits of entropy (03-N4).
 func generateToken() (string, error) {
-	bytes := make([]byte, 32)
-	if _, err := rand.Read(bytes); err != nil {
+	b := make([]byte, 32)
+	if _, err := rand.Read(b); err != nil {
 		return "", err
 	}
-	hash := sha256.Sum256(bytes)
-	return hex.EncodeToString(hash[:]), nil
+	return base64.RawURLEncoding.EncodeToString(b), nil
 }
 
 // containsAny checks if any element from requested is in allowed

internal/auth/service_mfa.go

@@ -46,21 +46,38 @@ const recoveryCodeLength = 8
 // shown to humans once and typed back, so visual clarity matters.
 const recoveryCodeAlphabet = "ABCDEFGHJKMNPQRSTUVWXYZ23456789"
 
-// verifyTOTP validates a TOTP code against the secret
-// Implements RFC 6238 TOTP with 30-second time steps
-// Uses constant-time comparison to prevent timing attacks
+// verifyTOTP validates a TOTP code against the secret.
+// Implements RFC 6238 TOTP with 30-second time steps.
+// Uses constant-time comparison to prevent timing attacks.
+//
+// Fails closed: returns false immediately when code or secret is empty.
+// An empty code can arrive from a missing form field; an empty secret signals
+// a data-integrity anomaly (MFA enabled but no secret stored). In both cases
+// the correct behavior is to reject, not to accidentally match because
+// generateTOTP("","") returns "" and ConstantTimeCompare("","") == 1.
 func verifyTOTP(secret, code string) bool {
-	// Allow for time skew by checking current and adjacent time windows
+	// Fail closed: reject empty code and empty secret up front.
+	// generateTOTP returns "" on base32-decode failure; ConstantTimeCompare("","")
+	// evaluates to 1 and would bypass MFA for any caller that passes code="".
+	if code == "" {
+		return false
+	}
+	if secret == "" {
+		return false
+	}
+
+	// Allow for time skew by checking current and adjacent time windows.
 	currentTime := time.Now().Unix()
 	timeStep := int64(config.MFATimeStep)
 
-	// Check current time step and one step before/after for clock skew tolerance
-	// Use constant-time comparison and avoid early returns to prevent timing attacks
+	// Check current time step and one step before/after for clock skew tolerance.
+	// Use constant-time comparison and avoid early returns to prevent timing attacks.
 	valid := 0
 	for _, offset := range []int64{-1, 0, 1} {
 		counter := (currentTime / timeStep) + offset
 		expected := generateTOTP(secret, counter)
-		// Use constant-time comparison - bitwise OR accumulates matches
+		// generateTOTP returns "" on base32-decode failure; a non-empty code can
+		// never equal "" so a bad secret produces no match.
 		if subtle.ConstantTimeCompare([]byte(expected), []byte(code)) == 1 {
 			valid = 1
 		}
@@ -207,13 +224,21 @@ func (s *Service) hashRecoveryCode(code string) (string, error) {
 // removal is what makes recovery codes single-use; the caller must
 // persist the user afterwards via UpdateUser.
 //
+// Fails closed on empty input: an empty string normalizes to "", which
+// bcrypt.CompareHashAndPassword would compare against every stored hash.
+// Reject it immediately to avoid leaking which hashes exist.
+//
 // Iterates over all stored hashes (bcrypt compare is constant-time
 // for the matching hash but not across the slice — recovery codes
 // are 8 chars of high-entropy alphabet, so timing leakage of which
 // slot matched isn't usefully exploitable, but the comparison stays
 // branch-free by ORing the bool into a single accumulator).
 func (s *Service) consumeRecoveryCode(user *User, entered string) bool {
 	normalized := normalizeRecoveryCode(entered)
+	// Fail closed: reject empty input before any bcrypt comparison.
+	if normalized == "" {
+		return false
+	}
 	matchIdx := -1
 	for i, hash := range user.MFARecoveryCodes {
 		if err := bcrypt.CompareHashAndPassword([]byte(hash), []byte(normalized)); err == nil {