diff --git a/internal/api/db_rate_limiter.go b/internal/api/db_rate_limiter.go index d18500656..bc559e888 100644 --- a/internal/api/db_rate_limiter.go +++ b/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() diff --git a/internal/api/db_rate_limiter_test.go b/internal/api/db_rate_limiter_test.go index ab15e52c7..9bb5385f4 100644 --- a/internal/api/db_rate_limiter_test.go +++ b/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) { diff --git a/internal/credentials/cipher.go b/internal/credentials/cipher.go index 9f8063827..868b667e3 100644 --- a/internal/credentials/cipher.go +++ b/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 diff --git a/internal/credentials/cipher_extra_test.go b/internal/credentials/cipher_extra_test.go index 9428cee63..6fbd0cb9d 100644 --- a/internal/credentials/cipher_extra_test.go +++ b/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 diff --git a/internal/credentials/cipher_loadkey_retry_test.go b/internal/credentials/cipher_loadkey_retry_test.go new file mode 100644 index 000000000..082870f5a --- /dev/null +++ b/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") +} diff --git a/internal/server/http.go b/internal/server/http.go index 8bbf2261c..66dbefd98 100644 --- a/internal/server/http.go +++ b/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{ diff --git a/internal/server/source_ip.go b/internal/server/source_ip.go new file mode 100644 index 000000000..17ee6ea9f --- /dev/null +++ b/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 +} diff --git a/internal/server/source_ip_test.go b/internal/server/source_ip_test.go new file mode 100644 index 000000000..e53a80be8 --- /dev/null +++ b/internal/server/source_ip_test.go @@ -0,0 +1,74 @@ +package server + +import ( + "context" + "go/ast" + "go/parser" + "go/token" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The source IP keys the per-IP rate limits on login, setup-admin and +// password reset. A port-bearing key would put every TCP connection in a +// fresh bucket, so those limits would never bind (issue #104). +func TestRequestSourceIP(t *testing.T) { + cases := []struct { + name string + remoteAddr string + xff string + want string + }{ + {name: "no XFF: IPv4 peer port is dropped", remoteAddr: "203.0.113.7:51234", want: "203.0.113.7"}, + {name: "no XFF: same client on another connection shares the key", remoteAddr: "203.0.113.7:40001", want: "203.0.113.7"}, + {name: "no XFF: IPv6 peer port and brackets are dropped", remoteAddr: "[2001:db8::1]:443", want: "2001:db8::1"}, + {name: "no XFF: peer without a port is kept", remoteAddr: "203.0.113.7", want: "203.0.113.7"}, + {name: "XFF: rightmost entry wins", remoteAddr: "10.0.0.5:9000", xff: "198.51.100.1, 203.0.113.9", want: "203.0.113.9"}, + {name: "XFF: rightmost entry with a port is reduced to the address", remoteAddr: "10.0.0.5:9000", xff: "198.51.100.1, 203.0.113.9:6123", want: "203.0.113.9"}, + {name: "XFF: bare IPv6 rightmost entry is kept", remoteAddr: "10.0.0.5:9000", xff: "2001:db8::2", want: "2001:db8::2"}, + {name: "XFF: bracketed IPv6 rightmost entry with a port is reduced", remoteAddr: "10.0.0.5:9000", xff: "[2001:db8::2]:6123", want: "2001:db8::2"}, + {name: "XFF: unparseable rightmost entry falls back to the peer address", remoteAddr: "10.0.0.5:9000", xff: "198.51.100.1, not-an-ip", want: "10.0.0.5"}, + {name: "XFF: empty rightmost entry falls back to the peer address", remoteAddr: "10.0.0.5:9000", xff: "198.51.100.1, ", want: "10.0.0.5"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequestWithContext(context.Background(), "POST", "/api/auth/login", nil) + req.RemoteAddr = tc.remoteAddr + if tc.xff != "" { + req.Header.Set("X-Forwarded-For", tc.xff) + } + assert.Equal(t, tc.want, requestSourceIP(req)) + }) + } +} + +// httpToLambdaRequest must take its SourceIP from requestSourceIP. This reads +// the source rather than calling httpToLambdaRequest so it does not depend on +// that function's signature (other in-flight changes alter it). +func TestHTTPToLambdaRequest_UsesRequestSourceIP(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "http.go", nil, 0) + require.NoError(t, err) + var found, calls bool + ast.Inspect(f, func(n ast.Node) bool { + fn, ok := n.(*ast.FuncDecl) + if !ok || fn.Name.Name != "httpToLambdaRequest" { + return true + } + found = true + ast.Inspect(fn.Body, func(m ast.Node) bool { + if c, ok := m.(*ast.CallExpr); ok { + if id, ok := c.Fun.(*ast.Ident); ok && id.Name == "requestSourceIP" { + calls = true + } + } + return true + }) + return false + }) + require.True(t, found, "httpToLambdaRequest not found in http.go") + assert.True(t, calls, "httpToLambdaRequest must derive SourceIP via requestSourceIP") +}