From b88156fe6b3f4b99c6d83535684d13dbe2eb28f9 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 8 Sep 2026 15:09:31 -0700 Subject: [PATCH] fix(s3api): evaluate aws:SourceIp from the direct TCP peer, not forwarded headers (#11231) * fix(s3api): use direct peer IP for aws:SourceIp in bucket policy engine extractSourceIP in the bucket-policy engine trusted X-Forwarded-For and X-Real-Ip whenever the TCP peer looked private (loopback/RFC1918/link-local), with no configurable trusted-proxy allowlist. In containerized deployments the gateway peer is almost always private, so any caller reaching it directly or from a co-located workload could spoof aws:SourceIp and bypass IpAddress/NotIpAddress bucket-policy restrictions. Always return the direct peer address (r.RemoteAddr), matching AWS S3 semantics. Remove the now-unused isPrivateIP helper and header-trust branch. Update TestExtractConditionValuesFromRequestSourceIPPrecedence to assert the peer IP is used regardless of forwarding headers, and add regression tests TestExtractSourceIP_IgnoresForwardedHeaders and TestExtractSourceIP_EnforcesIPRestrictionPolicy. * fix(s3api): use direct peer IP for aws:SourceIp in IAM role/session policies The IAM middleware's extractSourceIP trusted X-Forwarded-For and X-Real-IP whenever the TCP peer looked private (loopback/RFC1918/link-local), with no configurable trusted-proxy allowlist. In containerized deployments the gateway peer is almost always private, so any caller reaching it directly or from a co-located workload could spoof aws:SourceIp and bypass IpAddress/NotIpAddress conditions on role and session policies (IsPrincipalActionExplicitlyDenied). Always return the direct peer address (r.RemoteAddr), matching AWS S3 semantics. Remove the now-unused isPrivateIP helper, privateNetworks table, and its init(). Update TestRequestContextExtraction and TestIPBasedPolicyEnforcement to assert the peer IP is enforced regardless of forwarding headers, and add regression test TestUserInlinePolicySourceIpCondition_IgnoresForwardedHeaders. --- weed/s3api/policy_engine/engine.go | 73 ++---------------- weed/s3api/policy_engine/engine_test.go | 65 ++++++++++++++-- weed/s3api/s3_iam_middleware.go | 75 ++----------------- weed/s3api/s3_jwt_auth_test.go | 10 +-- .../s3api_inline_policy_condition_test.go | 29 +++++++ 5 files changed, 99 insertions(+), 153 deletions(-) diff --git a/weed/s3api/policy_engine/engine.go b/weed/s3api/policy_engine/engine.go index 6e54fdf91..5b950d237 100644 --- a/weed/s3api/policy_engine/engine.go +++ b/weed/s3api/policy_engine/engine.go @@ -519,23 +519,11 @@ func injectSSEForMultipart(conditions map[string][]string, inheritedSSE string) return modified } -// extractSourceIP returns the best-effort client IP address for condition evaluation. -// Preference order: X-Forwarded-For (first valid IP), X-Real-Ip, then RemoteAddr. -// IMPORTANT: X-Forwarded-For and X-Real-Ip are trusted without validation. -// When the service is exposed directly, clients can spoof aws:SourceIp unless a -// reverse proxy overwrites these headers. - -// isPrivateIP returns true if the given IP is considered a "trusted proxy" -// address, such as loopback, link-local, or RFC1918 private ranges. -// isPrivateIP returns true if the given IP is considered a "trusted proxy" -// address, such as loopback, link-local, or private ranges. -func isPrivateIP(ip net.IP) bool { - if ip == nil { - return false - } - return ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsPrivate() -} - +// extractSourceIP returns the direct TCP peer address for aws:SourceIp +// condition evaluation. Forwarding headers (X-Forwarded-For, X-Real-Ip) are +// intentionally ignored: without a configurable trusted-proxy allowlist they +// are client-controlled and spoofable, which would let a caller behind a +// private-looking peer bypass any aws:SourceIp restriction. func extractSourceIP(r *http.Request) string { if r == nil { return "" @@ -546,7 +534,6 @@ func extractSourceIP(r *http.Request) string { return "" } - // Fall back to unix socket markers or other non-IP placeholders. if remoteAddr == "@" { return remoteAddr } @@ -558,59 +545,9 @@ func extractSourceIP(r *http.Request) string { remoteIP := net.ParseIP(host) if remoteIP == nil { - // Do not return DNS names or unparseable values. return "" } - // Only trust forwarding headers when the connection appears to come from - // a trusted proxy (e.g., private/loopback address). - if isPrivateIP(remoteIP) { - if xff := r.Header.Get("X-Forwarded-For"); xff != "" { - // Iterate right-to-left to find the first non-trusted (public) IP - entries := strings.Split(xff, ",") - for i := len(entries) - 1; i >= 0; i-- { - candidate := strings.TrimSpace(entries[i]) - if candidate == "" { - continue - } - - ip := net.ParseIP(candidate) - if ip == nil { - continue - } - - // If the IP is trusted/private, we treat it as another proxy in the chain and continue - if isPrivateIP(ip) { - continue - } - - // Found a public/non-trusted IP, return it as the client IP - return ip.String() - } - - // If we exhausted the list (all were private/trusted) or found no valid IPs, - // fallback related logic could go here. - // For now, if all are private, we continue to check X-Real-Ip or return RemoteIP? - // The prompt implies we should prefer the extracted IP. - // If all in XFF are private, likely the original client IS private (internal network). - // The best guess for "original client" in a fully trusted chain is the left-most valid IP. - for _, candidate := range entries { - candidate = strings.TrimSpace(candidate) - if ip := net.ParseIP(candidate); ip != nil { - return ip.String() - } - } - } - - if xRealIP := strings.TrimSpace(r.Header.Get("X-Real-Ip")); xRealIP != "" { - if ip := net.ParseIP(xRealIP); ip != nil { - return ip.String() - } - } - } - - // Default to the actual peer IP when no trusted proxy is detected or the - // forwarding headers are absent/invalid. return remoteIP.String() } diff --git a/weed/s3api/policy_engine/engine_test.go b/weed/s3api/policy_engine/engine_test.go index 0e4098544..0187a8a61 100644 --- a/weed/s3api/policy_engine/engine_test.go +++ b/weed/s3api/policy_engine/engine_test.go @@ -2,6 +2,7 @@ package policy_engine import ( "net/http" + "net/http/httptest" "net/url" "testing" @@ -425,21 +426,21 @@ func TestExtractConditionValuesFromRequestSourceIPPrecedence(t *testing.T) { expectedIP string }{ { - name: "uses right-most public X-Forwarded-For entry", + name: "ignores X-Forwarded-For and uses RemoteAddr", header: map[string][]string{ "X-Forwarded-For": {"bad-ip, 203.0.113.10, 198.51.100.5"}, }, remoteAddr: "192.168.1.100:12345", - expectedIP: "198.51.100.5", + expectedIP: "192.168.1.100", }, { - name: "falls back to X-Real-Ip when X-Forwarded-For has no valid ip", + name: "ignores X-Real-Ip and uses RemoteAddr", header: map[string][]string{ "X-Forwarded-For": {"bad-ip"}, "X-Real-Ip": {"198.51.100.7"}, }, remoteAddr: "192.168.1.100:12345", - expectedIP: "198.51.100.7", + expectedIP: "192.168.1.100", }, { name: "uses RemoteAddr ip when no forwarding headers", @@ -454,20 +455,20 @@ func TestExtractConditionValuesFromRequestSourceIPPrecedence(t *testing.T) { expectedIP: "@", }, { - name: "uses IPv6 X-Forwarded-For entry", + name: "ignores IPv6 X-Forwarded-For entry and uses RemoteAddr", header: map[string][]string{ "X-Forwarded-For": {"2001:db8::8, 198.51.100.7"}, }, remoteAddr: "192.168.1.100:12345", - expectedIP: "198.51.100.7", + expectedIP: "192.168.1.100", }, { - name: "ignores spoofed IP when real client is public", + name: "ignores spoofed X-Forwarded-For behind private peer", header: map[string][]string{ "X-Forwarded-For": {"8.8.8.8, 203.0.113.10, 10.0.0.1"}, }, remoteAddr: "192.168.1.100:12345", - expectedIP: "203.0.113.10", + expectedIP: "192.168.1.100", }, { name: "handles bracketed IPv6 remote address", @@ -500,6 +501,54 @@ func TestExtractConditionValuesFromRequestSourceIPPrecedence(t *testing.T) { } } +func TestExtractSourceIP_IgnoresForwardedHeaders(t *testing.T) { + req := &http.Request{ + Method: "GET", + URL: &url.URL{Path: "/"}, + Header: map[string][]string{ + "X-Forwarded-For": {"203.0.113.99"}, + "X-Real-Ip": {"198.51.100.1"}, + }, + RemoteAddr: "10.0.0.5:54321", + } + + values := ExtractConditionValuesFromRequest(req) + if got := values["aws:SourceIp"]; len(got) != 1 || got[0] != "10.0.0.5" { + t.Errorf("Expected SourceIp to be the direct peer 10.0.0.5, got %v", got) + } +} + +func TestExtractSourceIP_EnforcesIPRestrictionPolicy(t *testing.T) { + engine := NewPolicyEngine() + policyJSON := `{ + "Version": "2012-10-17", + "Statement": [ + {"Effect": "Allow", "Principal": "*", "Action": ["s3:GetObject"], + "Resource": "arn:aws:s3:::secret-bucket/*"}, + {"Effect": "Deny", "Principal": "*", "Action": ["s3:GetObject"], + "Resource": "arn:aws:s3:::secret-bucket/*", + "Condition": {"NotIpAddress": {"aws:SourceIp": ["10.0.0.0/24"]}}} + ] + }` + if err := engine.SetBucketPolicy("secret-bucket", policyJSON); err != nil { + t.Fatalf("Failed to set bucket policy: %v", err) + } + + r := httptest.NewRequest(http.MethodGet, "/secret-bucket/secret-key", nil) + r.RemoteAddr = "10.0.50.5:54321" + r.Header.Set("X-Forwarded-For", "10.0.0.5") + + result := engine.EvaluatePolicy("secret-bucket", &PolicyEvaluationArgs{ + Action: "s3:GetObject", + Resource: "arn:aws:s3:::secret-bucket/secret-key", + Principal: "*", + Conditions: ExtractConditionValuesFromRequest(r), + }) + if result != PolicyResultDeny { + t.Errorf("Expected Deny for peer outside 10.0.0.0/24 despite spoofed X-Forwarded-For, got %v", result) + } +} + func TestPolicyEvaluationWithConditions(t *testing.T) { engine := NewPolicyEngine() diff --git a/weed/s3api/s3_iam_middleware.go b/weed/s3api/s3_iam_middleware.go index 42dd27d31..179874323 100644 --- a/weed/s3api/s3_iam_middleware.go +++ b/weed/s3api/s3_iam_middleware.go @@ -17,26 +17,6 @@ import ( "github.com/seaweedfs/seaweedfs/weed/security" ) -// privateNetworks contains pre-parsed private IP ranges for efficient lookups -var privateNetworks []*net.IPNet - -func init() { - // Private IPv4 ranges (RFC1918) and IPv6 Unique Local Addresses (ULA) - privateRanges := []string{ - "10.0.0.0/8", // IPv4 private - "172.16.0.0/12", // IPv4 private - "192.168.0.0/16", // IPv4 private - "fc00::/7", // IPv6 Unique Local Addresses (ULA) - } - - for _, cidr := range privateRanges { - _, network, err := net.ParseCIDR(cidr) - if err == nil { - privateNetworks = append(privateNetworks, network) - } - } -} - // IAMIntegration defines the interface for IAM integration type IAMIntegration interface { AuthenticateJWT(ctx context.Context, r *http.Request) (*IAMIdentity, s3err.ErrorCode) @@ -423,64 +403,19 @@ func extractRequestContext(r *http.Request) map[string]interface{} { return context } -// extractSourceIP extracts the real source IP from the request -// SECURITY: Prioritizes RemoteAddr over client-controlled headers to prevent spoofing -// Only trusts X-Forwarded-For/X-Real-IP if RemoteAddr appears to be from a trusted proxy +// extractSourceIP returns the direct TCP peer address for aws:SourceIp +// condition evaluation. Forwarding headers (X-Forwarded-For, X-Real-IP) are +// intentionally ignored: without a configurable trusted-proxy allowlist they +// are client-controlled and spoofable, which would let a caller behind a +// private-looking peer bypass any aws:SourceIp restriction. func extractSourceIP(r *http.Request) string { - // Always start with RemoteAddr as the most trustworthy source remoteIP := r.RemoteAddr if ip, _, err := net.SplitHostPort(remoteIP); err == nil { remoteIP = ip } - - // NOTE: The current heuristic of using isPrivateIP assumes reverse proxies are on a - // private/local network. This may be insufficient for some cloud, CDN, or multi-tier - // proxy deployments where proxies terminate connections from public IPs. In such - // environments, deployment-specific controls (e.g., network ACLs or proxy configs) - // should be used to ensure only trusted components can set forwarding headers. - // Future enhancements may introduce an explicit, configurable trusted proxy CIDR list. - isTrustedProxy := isPrivateIP(remoteIP) - - if isTrustedProxy { - // Check X-Real-IP header first (single IP, more reliable than X-Forwarded-For) - if realIP := r.Header.Get("X-Real-IP"); realIP != "" { - return strings.TrimSpace(realIP) - } - - // Check X-Forwarded-For header (can contain multiple IPs, take the first one) - if forwardedFor := r.Header.Get("X-Forwarded-For"); forwardedFor != "" { - if ips := strings.Split(forwardedFor, ","); len(ips) > 0 { - return strings.TrimSpace(ips[0]) - } - } - } - - // Fall back to RemoteAddr (most secure) return remoteIP } -// isPrivateIP checks if an IP is in a private range (localhost or RFC1918) -func isPrivateIP(ipStr string) bool { - ip := net.ParseIP(ipStr) - if ip == nil { - return false - } - - // Check for localhost and link-local addresses (IPv4/IPv6) - if ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { - return true - } - - // Check against pre-parsed private CIDR ranges - for _, network := range privateNetworks { - if network.Contains(ip) { - return true - } - } - - return false -} - // ParseUnverifiedJWTToken parses a JWT token and returns its claims WITHOUT cryptographic verification // // SECURITY WARNING: This function does NOT validate the token signature! diff --git a/weed/s3api/s3_jwt_auth_test.go b/weed/s3api/s3_jwt_auth_test.go index 8a71e0903..2997f3e81 100644 --- a/weed/s3api/s3_jwt_auth_test.go +++ b/weed/s3api/s3_jwt_auth_test.go @@ -171,11 +171,10 @@ func TestRequestContextExtraction(t *testing.T) { req := httptest.NewRequest("GET", "/test-bucket/test-file.txt", http.NoBody) req.Header.Set("X-Forwarded-For", "192.168.1.100") req.Header.Set("User-Agent", "aws-sdk-go/1.0") - // Set RemoteAddr to private IP to simulate trusted proxy req.RemoteAddr = "127.0.0.1:12345" return req }, - expectedIP: "192.168.1.100", + expectedIP: "127.0.0.1", expectedUA: "aws-sdk-go/1.0", }, { @@ -184,11 +183,10 @@ func TestRequestContextExtraction(t *testing.T) { req := httptest.NewRequest("GET", "/test-bucket/test-file.txt", http.NoBody) req.Header.Set("X-Real-IP", "10.0.0.1") req.Header.Set("User-Agent", "boto3/1.0") - // Set RemoteAddr to private IP to simulate trusted proxy req.RemoteAddr = "127.0.0.1:12345" return req }, - expectedIP: "10.0.0.1", + expectedIP: "127.0.0.1", expectedUA: "boto3/1.0", }, } @@ -258,9 +256,7 @@ func TestIPBasedPolicyEnforcement(t *testing.T) { // Create request with specific IP req := httptest.NewRequest("GET", "/restricted-bucket/file.txt", http.NoBody) req.Header.Set("Authorization", "Bearer "+response.Credentials.SessionToken) - req.Header.Set("X-Forwarded-For", tt.sourceIP) - // Set RemoteAddr to private IP to simulate trusted proxy - req.RemoteAddr = "127.0.0.1:12345" + req.RemoteAddr = tt.sourceIP + ":12345" // Create IAM identity for testing identity := &IAMIdentity{ diff --git a/weed/s3api/s3api_inline_policy_condition_test.go b/weed/s3api/s3api_inline_policy_condition_test.go index 9cd378285..16b688b60 100644 --- a/weed/s3api/s3api_inline_policy_condition_test.go +++ b/weed/s3api/s3api_inline_policy_condition_test.go @@ -123,6 +123,35 @@ func TestUserInlinePolicySourceIpCondition_Allows(t *testing.T) { "PutObject from 127.0.0.1 must be allowed: the user inline policy's aws:SourceIp condition (127.0.0.0/8) matches") } +// TestUserInlinePolicySourceIpCondition_IgnoresForwardedHeaders proves that a +// spoofed X-Forwarded-For cannot satisfy an aws:SourceIp condition: the policy +// requires 198.51.100.0/24, the peer is 127.0.0.1, and the header claims +// 198.51.100.5. The condition must be evaluated against the peer, not the +// header, so the request is denied. +func TestUserInlinePolicySourceIpCondition_IgnoresForwardedHeaders(t *testing.T) { + api := NewEmbeddedIamApiForTest() + api.mockConfig = &iam_pb.S3ApiConfiguration{} + seedInlineCondUser(t, api) + + _, iamErr := api.PutUserPolicy(api.mockConfig, url.Values{ + "UserName": {"alice"}, + "PolicyName": {"OnlyFromTestNet"}, + "PolicyDocument": {inlineCondPolicyDoc("198.51.100.0/24")}, + }) + require.Nil(t, iamErr, "PutUserPolicy must succeed") + require.NoError(t, api.PutS3ApiConfiguration(api.mockConfig)) + require.NoError(t, api.iam.LoadS3ApiConfigurationFromCredentialManager()) + + ident := api.iam.lookupByIdentityName("alice") + require.NotNil(t, ident) + + req := inlineCondRequest(t, http.MethodPut) + req.Header.Set("X-Forwarded-For", "198.51.100.5") + got := api.iam.VerifyActionPermission(req, ident, s3_constants.ACTION_WRITE, inlineCondTestBucket, "obj") + assert.Equal(t, s3err.ErrAccessDenied, got, + "PutObject from 127.0.0.1 must be denied despite X-Forwarded-For claiming 198.51.100.5: aws:SourceIp is the peer, not the header") +} + // TestGroupInlinePolicy_PutAndEnforce verifies that PutGroupPolicy is supported // (no longer returns NotImplemented) and that an aws:SourceIp condition on the // resulting inline policy is honored for group members.