From 8b5d31e5ebfe84808722411db957973ff60e7ec5 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 11 Feb 2026 12:47:03 -0800 Subject: [PATCH] s3api/policy_engine: use forwarded client IP for aws:SourceIp (#8304) * s3api: honor forwarded source IP for policy conditions Prefer X-Forwarded-For/X-Real-Ip before RemoteAddr when populating aws:SourceIp in policy condition evaluation. Also avoid noisy parsing behavior for unix socket markers and add coverage for precedence/fallback paths.\n\nFixes #8301. * s3api: simplify remote addr parsing * s3api: guard aws:SourceIp against DNS hosts * s3api: simplify remote addr fallback * s3api: simplify remote addr parsing * Update weed/s3api/policy_engine/engine.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fix TestExtractConditionValuesFromRequestSourceIPPrecedence using trusted private IP * Refactor extractSourceIP to use R-to-L XFF parsing and net.IP.IsPrivate --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- weed/s3api/policy_engine/engine.go | 105 ++++++++++++++++++++++-- weed/s3api/policy_engine/engine_test.go | 83 +++++++++++++++++++ 2 files changed, 179 insertions(+), 9 deletions(-) diff --git a/weed/s3api/policy_engine/engine.go b/weed/s3api/policy_engine/engine.go index ef795e253..d7b4d4758 100644 --- a/weed/s3api/policy_engine/engine.go +++ b/weed/s3api/policy_engine/engine.go @@ -381,15 +381,7 @@ func ExtractConditionValuesFromRequest(r *http.Request) map[string][]string { values := make(map[string][]string) // AWS condition keys - // Extract IP address without port for proper IP matching - host, _, err := net.SplitHostPort(r.RemoteAddr) - if err != nil { - // Log a warning if splitting fails - glog.Warningf("Failed to parse IP address from RemoteAddr %q: %v", r.RemoteAddr, err) - // If splitting fails, use the original RemoteAddr (might be just IP without port) - host = r.RemoteAddr - } - values["aws:SourceIp"] = []string{host} + values["aws:SourceIp"] = []string{extractSourceIP(r)} values["aws:SecureTransport"] = []string{fmt.Sprintf("%t", r.TLS != nil)} // Use AWS standard condition key for current time values["aws:CurrentTime"] = []string{time.Now().Format(time.RFC3339)} @@ -445,6 +437,101 @@ func ExtractConditionValuesFromRequest(r *http.Request) map[string][]string { return values } +// 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() +} + +func extractSourceIP(r *http.Request) string { + if r == nil { + return "" + } + + remoteAddr := strings.TrimSpace(r.RemoteAddr) + if remoteAddr == "" { + return "" + } + + // Fall back to unix socket markers or other non-IP placeholders. + if remoteAddr == "@" { + return remoteAddr + } + + host := remoteAddr + if h, _, err := net.SplitHostPort(remoteAddr); err == nil { + host = h + } + + 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() +} + // BuildResourceArn builds an ARN for the given bucket and object func BuildResourceArn(bucketName, objectName string) string { if objectName == "" { diff --git a/weed/s3api/policy_engine/engine_test.go b/weed/s3api/policy_engine/engine_test.go index 77e2b5395..6b0180be5 100644 --- a/weed/s3api/policy_engine/engine_test.go +++ b/weed/s3api/policy_engine/engine_test.go @@ -458,6 +458,89 @@ func TestExtractConditionValuesFromRequest(t *testing.T) { } } +func TestExtractConditionValuesFromRequestSourceIPPrecedence(t *testing.T) { + tests := []struct { + name string + header map[string][]string + remoteAddr string + expectedIP string + }{ + { + name: "uses right-most public X-Forwarded-For entry", + 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", + }, + { + name: "falls back to X-Real-Ip when X-Forwarded-For has no valid ip", + 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", + }, + { + name: "uses RemoteAddr ip when no forwarding headers", + header: map[string][]string{}, + remoteAddr: "192.168.1.100:12345", + expectedIP: "192.168.1.100", + }, + { + name: "keeps unix socket marker when RemoteAddr is not an ip", + header: map[string][]string{}, + remoteAddr: "@", + expectedIP: "@", + }, + { + name: "uses IPv6 X-Forwarded-For entry", + 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", + }, + { + name: "ignores spoofed IP when real client is public", + 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", + }, + { + name: "handles bracketed IPv6 remote address", + header: map[string][]string{}, + remoteAddr: "[2001:db8::1]:12345", + expectedIP: "2001:db8::1", + }, + { + name: "avoids returning DNS host names", + header: map[string][]string{}, + remoteAddr: "example.com:9000", + expectedIP: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := &http.Request{ + Method: "GET", + URL: &url.URL{Path: "/"}, + Header: tt.header, + RemoteAddr: tt.remoteAddr, + } + + values := ExtractConditionValuesFromRequest(req) + if len(values["aws:SourceIp"]) != 1 || values["aws:SourceIp"][0] != tt.expectedIP { + t.Errorf("Expected SourceIp %q, got %v", tt.expectedIP, values["aws:SourceIp"]) + } + }) + } +} + func TestPolicyEvaluationWithConditions(t *testing.T) { engine := NewPolicyEngine()