sec(auth): retry failed key loads, fail closed without a limiter pool, key rate limits on the client address

internal/api/db_rate_limiter.go

@@ -3,6 +3,7 @@ package api
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sync"
 	"sync/atomic"
@@ -28,6 +29,12 @@ type DBRateLimiter struct {
 // Verify that DBRateLimiter implements RateLimiterInterface.
 var _ RateLimiterInterface = (*DBRateLimiter)(nil)
 
+// ErrRateLimiterUnavailable is returned by a DBRateLimiter that has no
+// database pool. It cannot count attempts, so it reports an error instead of
+// allowing: checkRateLimitStrict then answers 503 on the credential endpoints
+// rather than silently dropping their brute-force protection (issue #104).
+var ErrRateLimiterUnavailable = errors.New("rate limiter has no database pool")
+
 // dbRateLimiterScheduledCleanupInterval is the period between scheduled
 // cleanup runs started by StartCleanupWorker. Separate from cleanupInterval
 // (the minimum time between opportunistic cleanup runs) so that the scheduled
@@ -105,7 +112,7 @@ func (rl *DBRateLimiter) SetLimit(endpoint string, config RateLimitConfig) {
 // trade for atomicity and is acceptable for rate-limit semantics.
 func (rl *DBRateLimiter) Allow(ctx context.Context, key, endpoint string) (bool, error) {
 	if rl == nil || rl.pool == nil {
-		return true, nil
+		return false, ErrRateLimiterUnavailable
 	}
 
 	rl.limitsMu.RLock()

internal/api/db_rate_limiter_test.go

@@ -39,40 +39,53 @@ func TestDBRateLimiter_SetLimit_NilLimitsMap(t *testing.T) {
 	assert.Equal(t, 5, rl.limits["endpoint"].MaxAttempts)
 }
 
+// A limiter with no pool cannot count attempts, so it must report an error
+// rather than allow (issue #104): an allow would silently disable brute-force
+// protection on the credential endpoints, whose strict check only fails
+// closed on an error.
 func TestDBRateLimiter_Allow_NilLimiter(t *testing.T) {
 	var rl *DBRateLimiter
-	// A nil receiver should be safe (fail-open)
 	allowed, err := rl.Allow(context.Background(), "IP#1.2.3.4", "login")
-	require.NoError(t, err)
-	assert.True(t, allowed)
+	require.ErrorIs(t, err, ErrRateLimiterUnavailable)
+	assert.False(t, allowed)
 }
 
-func TestDBRateLimiter_Allow_NilPool(t *testing.T) {
+func TestDBRateLimiter_NilPool_ReportsUnavailable(t *testing.T) {
+	ctx := context.Background()
 	rl := NewDBRateLimiter(nil)
-	allowed, err := rl.Allow(context.Background(), "IP#1.2.3.4", "login")
-	require.NoError(t, err)
-	assert.True(t, allowed)
-}
-
-func TestDBRateLimiter_AllowWithIP_NilPool(t *testing.T) {
-	rl := NewDBRateLimiter(nil)
-	allowed, err := rl.AllowWithIP(context.Background(), "1.2.3.4", "login")
-	require.NoError(t, err)
-	assert.True(t, allowed)
-}
-
-func TestDBRateLimiter_AllowWithEmail_NilPool(t *testing.T) {
-	rl := NewDBRateLimiter(nil)
-	allowed, err := rl.AllowWithEmail(context.Background(), "user@example.com", "forgot_password")
-	require.NoError(t, err)
-	assert.True(t, allowed)
+	calls := map[string]func() (bool, error){
+		"Allow":          func() (bool, error) { return rl.Allow(ctx, "IP#1.2.3.4", "login") },
+		"AllowWithIP":    func() (bool, error) { return rl.AllowWithIP(ctx, "1.2.3.4", "login") },
+		"AllowWithEmail": func() (bool, error) { return rl.AllowWithEmail(ctx, "user@example.com", "forgot_password") },
+		"AllowWithUser":  func() (bool, error) { return rl.AllowWithUser(ctx, "user-id-123", "admin") },
+	}
+	for name, call := range calls {
+		t.Run(name, func(t *testing.T) {
+			allowed, err := call()
+			require.ErrorIs(t, err, ErrRateLimiterUnavailable)
+			assert.False(t, allowed)
+		})
+	}
 }
 
-func TestDBRateLimiter_AllowWithUser_NilPool(t *testing.T) {
-	rl := NewDBRateLimiter(nil)
-	allowed, err := rl.AllowWithUser(context.Background(), "user-id-123", "admin")
-	require.NoError(t, err)
-	assert.True(t, allowed)
+// End to end through the handler: a pool-less DB limiter makes the strict
+// credential-endpoint check answer 503 instead of letting the request through,
+// while the ordinary check keeps its documented fail-open behavior.
+func TestHandler_checkRateLimitStrict_NilPoolFailsClosed(t *testing.T) {
+	ctx := context.Background()
+	for name, rl := range map[string]*DBRateLimiter{"nil pool": NewDBRateLimiter(nil), "nil limiter": nil} {
+		t.Run(name, func(t *testing.T) {
+			h := &Handler{rateLimiter: rl}
+			req := makeSourceIPReq("198.51.100.4")
+
+			err := h.checkRateLimitStrict(ctx, req, "login")
+			ce, ok := IsClientError(err)
+			require.True(t, ok, "expected a 503 client error, got %v", err)
+			assert.Equal(t, 503, ce.code)
+
+			assert.NoError(t, h.checkRateLimit(ctx, req, "api_general"))
+		})
+	}
 }
 
 func TestDBRateLimiter_cleanup_NilPool(t *testing.T) {

internal/credentials/cipher.go

@@ -44,11 +44,12 @@ const (
 	EnvAllowDev   = "CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY"   // 1 = permit zero-key fallback
 )
 
+// The memoized key. keyMu is held across a load so concurrent callers queue
+// behind one fetch instead of all hitting the secret store at once.
 var (
+	keyMu           sync.Mutex
 	cachedKey       []byte
 	cachedKeySource string
-	cachedKeyOnce   sync.Once
-	cachedKeyErr    error
 )
 
 // LoadKey loads the 32-byte AES-256 key for tenant credential encryption.
@@ -65,13 +66,23 @@ var (
 // The returned source string is the env var name that resolved (for logging).
 // Callers MUST NOT log the key value.
 //
-// Result is memoized via sync.Once for the process lifetime; tests reset the
+// A successful load is memoized for the process lifetime. A failed load is
+// not: one throttled secret fetch, or a first caller whose context was
+// already canceled, would otherwise break credential decryption until the
+// process restarts (issue #104), so the next call retries. Tests reset the
 // cache via resetKeyCacheForTest() in this package's test files.
 func LoadKey(ctx context.Context, resolver secrets.Resolver) (key []byte, source string, err error) {
-	cachedKeyOnce.Do(func() {
-		cachedKey, cachedKeySource, cachedKeyErr = loadKey(ctx, resolver)
-	})
-	return cachedKey, cachedKeySource, cachedKeyErr
+	keyMu.Lock()
+	defer keyMu.Unlock()
+	if cachedKey != nil {
+		return cachedKey, cachedKeySource, nil
+	}
+	key, source, err = loadKey(ctx, resolver)
+	if err != nil {
+		return nil, source, err
+	}
+	cachedKey, cachedKeySource = key, source
+	return key, source, nil
 }
 
 // DevKey returns the all-zero 32-byte dev key. Exposed for the rekey

internal/credentials/cipher_extra_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"strings"
-	"sync"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -14,12 +13,12 @@ import (
 )
 
 // resetKeyCacheForTest clears the LoadKey memoization cache so each test
-// gets a fresh sync.Once. Test-only — never called from production code.
+// starts from an unloaded key. Test-only — never called from production code.
 func resetKeyCacheForTest() {
+	keyMu.Lock()
+	defer keyMu.Unlock()
 	cachedKey = nil
 	cachedKeySource = ""
-	cachedKeyErr = nil
-	cachedKeyOnce = sync.Once{}
 }
 
 // fakeResolver is a test double for secrets.Resolver. It returns the

internal/credentials/cipher_loadkey_retry_test.go

@@ -0,0 +1,100 @@
+package credentials
+
+import (
+	"context"
+	"errors"
+	"sync"
+	"sync/atomic"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// flakyResolver fails its first failFirst GetSecret calls, and any call made
+// with an already-canceled context, then serves value. It is safe for
+// concurrent use.
+type flakyResolver struct {
+	fakeResolver
+	calls     atomic.Int32
+	failFirst int32
+}
+
+var errThrottled = errors.New("ThrottlingException: rate exceeded")
+
+func (f *flakyResolver) GetSecret(ctx context.Context, _ string) (string, error) {
+	n := f.calls.Add(1)
+	if err := ctx.Err(); err != nil {
+		return "", err
+	}
+	if n <= f.failFirst {
+		return "", errThrottled
+	}
+	return f.value, nil
+}
+
+// A failed key fetch must not be memoized: one throttled or canceled fetch at
+// cold start would otherwise break credential decryption for the rest of the
+// process lifetime (issue #104). A successful load is still memoized.
+func TestLoadKey_FailedLoadIsRetried(t *testing.T) {
+	resetKeyCacheForTest()
+	t.Cleanup(resetKeyCacheForTest)
+	clearAllKeyEnvs(t)
+	t.Setenv(EnvSecretARN, "arn:aws:secretsmanager:us-east-1:123:secret:enc-key-AbC")
+	r := &flakyResolver{fakeResolver: fakeResolver{value: validHexKey}}
+
+	canceled, cancel := context.WithCancel(context.Background())
+	cancel()
+	_, _, err := LoadKey(canceled, r)
+	require.ErrorIs(t, err, context.Canceled, "the first caller's own canceled context fails its own load")
+
+	key, source, err := LoadKey(context.Background(), r)
+	require.NoError(t, err, "a later caller must retry instead of getting the cached failure")
+	assert.Len(t, key, 32)
+	assert.Equal(t, EnvSecretARN, source)
+
+	again, _, err := LoadKey(context.Background(), r)
+	require.NoError(t, err)
+	assert.Equal(t, key, again)
+	assert.Equal(t, int32(2), r.calls.Load(), "a successful load is memoized: no third fetch")
+}
+
+// Concurrent callers racing a failing first fetch: loads are serialized, so
+// exactly one caller sees the throttle, exactly one more fetch succeeds, and
+// every other caller is served the memoized key without fetching again.
+func TestLoadKey_ConcurrentCallersAfterTransientFailure(t *testing.T) {
+	resetKeyCacheForTest()
+	t.Cleanup(resetKeyCacheForTest)
+	clearAllKeyEnvs(t)
+	t.Setenv(EnvSecretARN, "arn:aws:secretsmanager:us-east-1:123:secret:enc-key-AbC")
+	r := &flakyResolver{fakeResolver: fakeResolver{value: validHexKey}, failFirst: 1}
+
+	const callers = 32
+	var (
+		wg        sync.WaitGroup
+		start     = make(chan struct{})
+		failures  atomic.Int32
+		successes atomic.Int32
+	)
+	for range callers {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			<-start
+			key, _, err := LoadKey(context.Background(), r)
+			if err != nil {
+				assert.ErrorIs(t, err, errThrottled)
+				failures.Add(1)
+				return
+			}
+			assert.Len(t, key, 32)
+			successes.Add(1)
+		}()
+	}
+	close(start)
+	wg.Wait()
+
+	assert.Equal(t, int32(1), failures.Load(), "only the caller whose fetch was throttled fails")
+	assert.Equal(t, int32(callers-1), successes.Load())
+	assert.Equal(t, int32(2), r.calls.Load(), "one failed fetch, one successful fetch, then the memoized key")
+}

internal/server/http.go

@@ -425,14 +425,7 @@ func httpToLambdaRequest(r *http.Request) *events.LambdaFunctionURLRequest {
 		}
 	}
 
-	// Get client IP from X-Forwarded-For using the rightmost entry, which is
-	// set by the nearest trusted proxy (ALB, Cloud Run ingress, etc.). The
-	// leftmost entry is client-controlled and trivially spoofable.
-	sourceIP := r.RemoteAddr
-	if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
-		parts := strings.Split(xff, ",")
-		sourceIP = strings.TrimSpace(parts[len(parts)-1])
-	}
+	sourceIP := requestSourceIP(r)
 
 	return &events.LambdaFunctionURLRequest{
 		RequestContext: events.LambdaFunctionURLRequestContext{

internal/server/source_ip.go

@@ -0,0 +1,42 @@
+package server
+
+import (
+	"net"
+	"net/http"
+	"net/netip"
+	"strings"
+)
+
+// requestSourceIP returns the client address the API rate limits key on.
+//
+// It takes the rightmost X-Forwarded-For entry, which is set by the nearest
+// trusted proxy (ALB, Cloud Run ingress, etc.); the leftmost entries are
+// client-controlled and trivially spoofable. Without that header it uses the
+// TCP peer. Either way a port is dropped: net/http's RemoteAddr is always
+// "ip:port", and some proxies append "ip:port" to X-Forwarded-For, so keying
+// on the raw value would give every connection its own bucket and the
+// credential-endpoint limits would never bind (issue #104). A rightmost entry
+// that is not an IP address is not trusted as a key; the peer is used instead.
+func requestSourceIP(r *http.Request) string {
+	peer := stripPort(r.RemoteAddr)
+	xff := r.Header.Get("X-Forwarded-For")
+	if xff == "" {
+		return peer
+	}
+	parts := strings.Split(xff, ",")
+	last := stripPort(strings.TrimSpace(parts[len(parts)-1]))
+	if _, err := netip.ParseAddr(last); err != nil {
+		return peer
+	}
+	return last
+}
+
+// stripPort returns addr without a ":port" suffix (and without the brackets
+// around an IPv6 host). An address with no port, including a bare IPv6
+// address, is returned unchanged.
+func stripPort(addr string) string {
+	if host, _, err := net.SplitHostPort(addr); err == nil {
+		return host
+	}
+	return addr
+}