fix(security): add trusted proxy support and rate limit token endpoint
- commit
- 912a154927eac7842974f205fbe0d77c295620c9
- author
- bnema · · 2026-01-18 01:59 UTC
- parent
- bfdb3cf
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
4 files · +223 -59
M docs/config/reference.md +2 -0
| @@ -34,6 +34,7 @@ enabled = true # Enable rate limiting (default: tr | |||
| 34 | 34 | global_rps = 500 # Max requests/second globally | |
| 35 | 35 | per_ip_rps = 50 # Max requests/second per client IP | |
| 36 | 36 | burst = 100 # Burst size for rate limiters | |
| 37 | + | trusted_proxies = [] # IPs/CIDRs trusted to set X-Forwarded-For | |
| 37 | 38 | ||
| 38 | 39 | # ============================================================================= | |
| 39 | 40 | # LOGGING | |
| @@ -132,6 +133,7 @@ preserve = true # Keep volumes when containers are | |||
| 132 | 133 | | `api.rate_limit.global_rps` | `500` | Global requests/second | | |
| 133 | 134 | | `api.rate_limit.per_ip_rps` | `50` | Per-IP requests/second | | |
| 134 | 135 | | `api.rate_limit.burst` | `100` | Burst size | | |
| 136 | + | | `api.rate_limit.trusted_proxies` | `[]` | IPs/CIDRs trusted for X-Forwarded-For | | |
| 135 | 137 | | `logging.level` | `"info"` | Log level | | |
| 136 | 138 | | `logging.format` | `"console"` | Log format | | |
| 137 | 139 | | `logging.file.enabled` | `false` | File logging disabled | | |
| @@ -34,6 +34,7 @@ enabled = true # Enable rate limiting (default: tr | |||||
| 34 | global_rps = 500 # Max requests/second globally | 34 | global_rps = 500 # Max requests/second globally | ||
| 35 | per_ip_rps = 50 # Max requests/second per client IP | 35 | per_ip_rps = 50 # Max requests/second per client IP | ||
| 36 | burst = 100 # Burst size for rate limiters | 36 | burst = 100 # Burst size for rate limiters | ||
| 37 | + | trusted_proxies = [] # IPs/CIDRs trusted to set X-Forwarded-For | |||
| 37 | 38 | ||||
| 38 | # ============================================================================= | 39 | # ============================================================================= | ||
| 39 | # LOGGING | 40 | # LOGGING | ||
| @@ -132,6 +133,7 @@ preserve = true # Keep volumes when containers are | |||||
| 132 | | `api.rate_limit.global_rps` | `500` | Global requests/second | | 133 | | `api.rate_limit.global_rps` | `500` | Global requests/second | | ||
| 133 | | `api.rate_limit.per_ip_rps` | `50` | Per-IP requests/second | | 134 | | `api.rate_limit.per_ip_rps` | `50` | Per-IP requests/second | | ||
| 134 | | `api.rate_limit.burst` | `100` | Burst size | | 135 | | `api.rate_limit.burst` | `100` | Burst size | | ||
| 136 | + | | `api.rate_limit.trusted_proxies` | `[]` | IPs/CIDRs trusted for X-Forwarded-For | | |||
| 135 | | `logging.level` | `"info"` | Log level | | 137 | | `logging.level` | `"info"` | Log level | | ||
| 136 | | `logging.format` | `"console"` | Log format | | 138 | | `logging.format` | `"console"` | Log format | | ||
| 137 | | `logging.file.enabled` | `false` | File logging disabled | | 139 | | `logging.file.enabled` | `false` | File logging disabled | | ||
M internal/adapters/in/http/registry/ratelimit.go +76 -19
| @@ -31,6 +31,12 @@ type RateLimitConfig struct { | |||
| 31 | 31 | // Burst is the maximum burst size for rate limiters. | |
| 32 | 32 | // Default: 100 | |
| 33 | 33 | Burst int | |
| 34 | + | ||
| 35 | + | // TrustedProxies is a list of IP addresses or CIDR ranges that are trusted | |
| 36 | + | // to set X-Forwarded-For and X-Real-IP headers. If empty, these headers are | |
| 37 | + | // ignored and RemoteAddr is always used (secure default for direct exposure). | |
| 38 | + | // Example: ["127.0.0.1", "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16"] | |
| 39 | + | TrustedProxies []string | |
| 34 | 40 | } | |
| 35 | 41 | ||
| 36 | 42 | // DefaultRateLimitConfig returns sensible default rate limiting configuration. | |
| @@ -93,6 +99,9 @@ func RateLimitMiddleware(config RateLimitConfig) func(http.Handler) http.Handler | |||
| 93 | 99 | globalLimiter := rate.NewLimiter(rate.Limit(config.GlobalRPS), config.Burst) | |
| 94 | 100 | ipLimiter := newIPRateLimiter(config.PerIPRPS, config.Burst) | |
| 95 | 101 | ||
| 102 | + | // Parse trusted proxy CIDRs once at middleware creation | |
| 103 | + | trustedNets := parseTrustedProxies(config.TrustedProxies) | |
| 104 | + | ||
| 96 | 105 | return func(next http.Handler) http.Handler { | |
| 97 | 106 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 98 | 107 | // Check global rate limit | |
| @@ -102,7 +111,7 @@ func RateLimitMiddleware(config RateLimitConfig) func(http.Handler) http.Handler | |||
| 102 | 111 | } | |
| 103 | 112 | ||
| 104 | 113 | // Check per-IP rate limit | |
| 105 | - | ip := getClientIP(r) | |
| 114 | + | ip := getClientIP(r, trustedNets) | |
| 106 | 115 | if !ipLimiter.getLimiter(ip).Allow() { | |
| 107 | 116 | sendRateLimitError(w) | |
| 108 | 117 | return | |
| @@ -113,6 +122,47 @@ func RateLimitMiddleware(config RateLimitConfig) func(http.Handler) http.Handler | |||
| 113 | 122 | } | |
| 114 | 123 | } | |
| 115 | 124 | ||
| 125 | + | // parseTrustedProxies converts a list of IP addresses and CIDR ranges to net.IPNet. | |
| 126 | + | func parseTrustedProxies(proxies []string) []*net.IPNet { | |
| 127 | + | var nets []*net.IPNet | |
| 128 | + | for _, proxy := range proxies { | |
| 129 | + | // Try parsing as CIDR | |
| 130 | + | _, ipNet, err := net.ParseCIDR(proxy) | |
| 131 | + | if err == nil { | |
| 132 | + | nets = append(nets, ipNet) | |
| 133 | + | continue | |
| 134 | + | } | |
| 135 | + | // Try parsing as single IP | |
| 136 | + | ip := net.ParseIP(proxy) | |
| 137 | + | if ip != nil { | |
| 138 | + | // Convert single IP to /32 or /128 CIDR | |
| 139 | + | bits := 32 | |
| 140 | + | if ip.To4() == nil { | |
| 141 | + | bits = 128 | |
| 142 | + | } | |
| 143 | + | nets = append(nets, &net.IPNet{IP: ip, Mask: net.CIDRMask(bits, bits)}) | |
| 144 | + | } | |
| 145 | + | } | |
| 146 | + | return nets | |
| 147 | + | } | |
| 148 | + | ||
| 149 | + | // isTrustedProxy checks if the given IP is from a trusted proxy. | |
| 150 | + | func isTrustedProxy(ip string, trustedNets []*net.IPNet) bool { | |
| 151 | + | if len(trustedNets) == 0 { | |
| 152 | + | return false | |
| 153 | + | } | |
| 154 | + | parsedIP := net.ParseIP(ip) | |
| 155 | + | if parsedIP == nil { | |
| 156 | + | return false | |
| 157 | + | } | |
| 158 | + | for _, ipNet := range trustedNets { | |
| 159 | + | if ipNet.Contains(parsedIP) { | |
| 160 | + | return true | |
| 161 | + | } | |
| 162 | + | } | |
| 163 | + | return false | |
| 164 | + | } | |
| 165 | + | ||
| 116 | 166 | // sendRateLimitError sends an HTTP 429 response in Docker Registry API format. | |
| 117 | 167 | func sendRateLimitError(w http.ResponseWriter) { | |
| 118 | 168 | w.Header().Set("Content-Type", "application/json") | |
| @@ -128,26 +178,33 @@ func sendRateLimitError(w http.ResponseWriter) { | |||
| 128 | 178 | } | |
| 129 | 179 | ||
| 130 | 180 | // getClientIP extracts the client IP address from the request. | |
| 131 | - | // It checks X-Forwarded-For and X-Real-IP headers for proxied requests. | |
| 132 | - | func getClientIP(r *http.Request) string { | |
| 133 | - | // Check X-Forwarded-For for proxied requests | |
| 134 | - | if xff := r.Header.Get("X-Forwarded-For"); xff != "" { | |
| 135 | - | // Take first IP in chain (original client) | |
| 136 | - | if idx := strings.Index(xff, ","); idx != -1 { | |
| 137 | - | return strings.TrimSpace(xff[:idx]) | |
| 138 | - | } | |
| 139 | - | return strings.TrimSpace(xff) | |
| 181 | + | // It only honors X-Forwarded-For and X-Real-IP headers when the request | |
| 182 | + | // originates from a trusted proxy. This prevents attackers from spoofing | |
| 183 | + | // their IP address when Gordon is exposed directly without a reverse proxy. | |
| 184 | + | func getClientIP(r *http.Request, trustedNets []*net.IPNet) string { | |
| 185 | + | // Extract the direct connection IP from RemoteAddr | |
| 186 | + | remoteIP, _, err := net.SplitHostPort(r.RemoteAddr) | |
| 187 | + | if err != nil { | |
| 188 | + | remoteIP = r.RemoteAddr | |
| 140 | 189 | } | |
| 141 | 190 | ||
| 142 | - | // Check X-Real-IP | |
| 143 | - | if xri := r.Header.Get("X-Real-IP"); xri != "" { | |
| 144 | - | return xri | |
| 145 | - | } | |
| 191 | + | // Only honor proxy headers if the request comes from a trusted proxy | |
| 192 | + | if isTrustedProxy(remoteIP, trustedNets) { | |
| 193 | + | // Check X-Forwarded-For for proxied requests | |
| 194 | + | if xff := r.Header.Get("X-Forwarded-For"); xff != "" { | |
| 195 | + | // Take first IP in chain (original client) | |
| 196 | + | if first, _, found := strings.Cut(xff, ","); found { | |
| 197 | + | return strings.TrimSpace(first) | |
| 198 | + | } | |
| 199 | + | return strings.TrimSpace(xff) | |
| 200 | + | } | |
| 146 | 201 | ||
| 147 | - | // Fall back to RemoteAddr | |
| 148 | - | ip, _, err := net.SplitHostPort(r.RemoteAddr) | |
| 149 | - | if err != nil { | |
| 150 | - | return r.RemoteAddr | |
| 202 | + | // Check X-Real-IP | |
| 203 | + | if xri := r.Header.Get("X-Real-IP"); xri != "" { | |
| 204 | + | return xri | |
| 205 | + | } | |
| 151 | 206 | } | |
| 152 | - | return ip | |
| 207 | + | ||
| 208 | + | // Fall back to RemoteAddr (direct connection IP) | |
| 209 | + | return remoteIP | |
| 153 | 210 | } | |
| @@ -31,6 +31,12 @@ type RateLimitConfig struct { | |||||
| 31 | // Burst is the maximum burst size for rate limiters. | 31 | // Burst is the maximum burst size for rate limiters. | ||
| 32 | // Default: 100 | 32 | // Default: 100 | ||
| 33 | Burst int | 33 | Burst int | ||
| 34 | + | ||||
| 35 | + | // TrustedProxies is a list of IP addresses or CIDR ranges that are trusted | |||
| 36 | + | // to set X-Forwarded-For and X-Real-IP headers. If empty, these headers are | |||
| 37 | + | // ignored and RemoteAddr is always used (secure default for direct exposure). | |||
| 38 | + | // Example: ["127.0.0.1", "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16"] | |||
| 39 | + | TrustedProxies []string | |||
| 34 | } | 40 | } | ||
| 35 | 41 | ||||
| 36 | // DefaultRateLimitConfig returns sensible default rate limiting configuration. | 42 | // DefaultRateLimitConfig returns sensible default rate limiting configuration. | ||
| @@ -93,6 +99,9 @@ func RateLimitMiddleware(config RateLimitConfig) func(http.Handler) http.Handler | |||||
| 93 | globalLimiter := rate.NewLimiter(rate.Limit(config.GlobalRPS), config.Burst) | 99 | globalLimiter := rate.NewLimiter(rate.Limit(config.GlobalRPS), config.Burst) | ||
| 94 | ipLimiter := newIPRateLimiter(config.PerIPRPS, config.Burst) | 100 | ipLimiter := newIPRateLimiter(config.PerIPRPS, config.Burst) | ||
| 95 | 101 | ||||
| 102 | + | // Parse trusted proxy CIDRs once at middleware creation | |||
| 103 | + | trustedNets := parseTrustedProxies(config.TrustedProxies) | |||
| 104 | + | ||||
| 96 | return func(next http.Handler) http.Handler { | 105 | return func(next http.Handler) http.Handler { | ||
| 97 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | 106 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 98 | // Check global rate limit | 107 | // Check global rate limit | ||
| @@ -102,7 +111,7 @@ func RateLimitMiddleware(config RateLimitConfig) func(http.Handler) http.Handler | |||||
| 102 | } | 111 | } | ||
| 103 | 112 | ||||
| 104 | // Check per-IP rate limit | 113 | // Check per-IP rate limit | ||
| 105 | - | ip := getClientIP(r) | 114 | + | ip := getClientIP(r, trustedNets) |
| 106 | if !ipLimiter.getLimiter(ip).Allow() { | 115 | if !ipLimiter.getLimiter(ip).Allow() { | ||
| 107 | sendRateLimitError(w) | 116 | sendRateLimitError(w) | ||
| 108 | return | 117 | return | ||
| @@ -113,6 +122,47 @@ func RateLimitMiddleware(config RateLimitConfig) func(http.Handler) http.Handler | |||||
| 113 | } | 122 | } | ||
| 114 | } | 123 | } | ||
| 115 | 124 | ||||
| 125 | + | // parseTrustedProxies converts a list of IP addresses and CIDR ranges to net.IPNet. | |||
| 126 | + | func parseTrustedProxies(proxies []string) []*net.IPNet { | |||
| 127 | + | var nets []*net.IPNet | |||
| 128 | + | for _, proxy := range proxies { | |||
| 129 | + | // Try parsing as CIDR | |||
| 130 | + | _, ipNet, err := net.ParseCIDR(proxy) | |||
| 131 | + | if err == nil { | |||
| 132 | + | nets = append(nets, ipNet) | |||
| 133 | + | continue | |||
| 134 | + | } | |||
| 135 | + | // Try parsing as single IP | |||
| 136 | + | ip := net.ParseIP(proxy) | |||
| 137 | + | if ip != nil { | |||
| 138 | + | // Convert single IP to /32 or /128 CIDR | |||
| 139 | + | bits := 32 | |||
| 140 | + | if ip.To4() == nil { | |||
| 141 | + | bits = 128 | |||
| 142 | + | } | |||
| 143 | + | nets = append(nets, &net.IPNet{IP: ip, Mask: net.CIDRMask(bits, bits)}) | |||
| 144 | + | } | |||
| 145 | + | } | |||
| 146 | + | return nets | |||
| 147 | + | } | |||
| 148 | + | ||||
| 149 | + | // isTrustedProxy checks if the given IP is from a trusted proxy. | |||
| 150 | + | func isTrustedProxy(ip string, trustedNets []*net.IPNet) bool { | |||
| 151 | + | if len(trustedNets) == 0 { | |||
| 152 | + | return false | |||
| 153 | + | } | |||
| 154 | + | parsedIP := net.ParseIP(ip) | |||
| 155 | + | if parsedIP == nil { | |||
| 156 | + | return false | |||
| 157 | + | } | |||
| 158 | + | for _, ipNet := range trustedNets { | |||
| 159 | + | if ipNet.Contains(parsedIP) { | |||
| 160 | + | return true | |||
| 161 | + | } | |||
| 162 | + | } | |||
| 163 | + | return false | |||
| 164 | + | } | |||
| 165 | + | ||||
| 116 | // sendRateLimitError sends an HTTP 429 response in Docker Registry API format. | 166 | // sendRateLimitError sends an HTTP 429 response in Docker Registry API format. | ||
| 117 | func sendRateLimitError(w http.ResponseWriter) { | 167 | func sendRateLimitError(w http.ResponseWriter) { | ||
| 118 | w.Header().Set("Content-Type", "application/json") | 168 | w.Header().Set("Content-Type", "application/json") | ||
| @@ -128,26 +178,33 @@ func sendRateLimitError(w http.ResponseWriter) { | |||||
| 128 | } | 178 | } | ||
| 129 | 179 | ||||
| 130 | // getClientIP extracts the client IP address from the request. | 180 | // getClientIP extracts the client IP address from the request. | ||
| 131 | - | // It checks X-Forwarded-For and X-Real-IP headers for proxied requests. | 181 | + | // It only honors X-Forwarded-For and X-Real-IP headers when the request |
| 132 | - | func getClientIP(r *http.Request) string { | 182 | + | // originates from a trusted proxy. This prevents attackers from spoofing |
| 133 | - | // Check X-Forwarded-For for proxied requests | 183 | + | // their IP address when Gordon is exposed directly without a reverse proxy. |
| 134 | - | if xff := r.Header.Get("X-Forwarded-For"); xff != "" { | 184 | + | func getClientIP(r *http.Request, trustedNets []*net.IPNet) string { |
| 135 | - | // Take first IP in chain (original client) | 185 | + | // Extract the direct connection IP from RemoteAddr |
| 136 | - | if idx := strings.Index(xff, ","); idx != -1 { | 186 | + | remoteIP, _, err := net.SplitHostPort(r.RemoteAddr) |
| 137 | - | return strings.TrimSpace(xff[:idx]) | 187 | + | if err != nil { |
| 138 | - | } | 188 | + | remoteIP = r.RemoteAddr |
| 139 | - | return strings.TrimSpace(xff) | |||
| 140 | } | 189 | } | ||
| 141 | 190 | ||||
| 142 | - | // Check X-Real-IP | 191 | + | // Only honor proxy headers if the request comes from a trusted proxy |
| 143 | - | if xri := r.Header.Get("X-Real-IP"); xri != "" { | 192 | + | if isTrustedProxy(remoteIP, trustedNets) { |
| 144 | - | return xri | 193 | + | // Check X-Forwarded-For for proxied requests |
| 145 | - | } | 194 | + | if xff := r.Header.Get("X-Forwarded-For"); xff != "" { |
| 195 | + | // Take first IP in chain (original client) | |||
| 196 | + | if first, _, found := strings.Cut(xff, ","); found { | |||
| 197 | + | return strings.TrimSpace(first) | |||
| 198 | + | } | |||
| 199 | + | return strings.TrimSpace(xff) | |||
| 200 | + | } | |||
| 146 | 201 | ||||
| 147 | - | // Fall back to RemoteAddr | 202 | + | // Check X-Real-IP |
| 148 | - | ip, _, err := net.SplitHostPort(r.RemoteAddr) | 203 | + | if xri := r.Header.Get("X-Real-IP"); xri != "" { |
| 149 | - | if err != nil { | 204 | + | return xri |
| 150 | - | return r.RemoteAddr | 205 | + | } |
| 151 | } | 206 | } | ||
| 152 | - | return ip | 207 | + | |
| 208 | + | // Fall back to RemoteAddr (direct connection IP) | |||
| 209 | + | return remoteIP | |||
| 153 | } | 210 | } | ||
M internal/adapters/in/http/registry/ratelimit_test.go +129 -29
| @@ -1,6 +1,7 @@ | |||
| 1 | 1 | package registry | |
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | + | "net" | |
| 4 | 5 | "net/http" | |
| 5 | 6 | "net/http/httptest" | |
| 6 | 7 | "testing" | |
| @@ -126,47 +127,86 @@ func TestRateLimitMiddleware_ResponseFormat(t *testing.T) { | |||
| 126 | 127 | } | |
| 127 | 128 | ||
| 128 | 129 | func TestGetClientIP(t *testing.T) { | |
| 130 | + | // Parse trusted proxies for tests | |
| 131 | + | trustedNets := parseTrustedProxies([]string{"127.0.0.1", "10.0.0.0/8"}) | |
| 132 | + | noTrustedNets := []*net.IPNet{} | |
| 133 | + | ||
| 129 | 134 | tests := []struct { | |
| 130 | - | name string | |
| 131 | - | remoteAddr string | |
| 132 | - | xff string | |
| 133 | - | xRealIP string | |
| 134 | - | wantIP string | |
| 135 | + | name string | |
| 136 | + | remoteAddr string | |
| 137 | + | xff string | |
| 138 | + | xRealIP string | |
| 139 | + | trustedNets []*net.IPNet | |
| 140 | + | wantIP string | |
| 135 | 141 | }{ | |
| 136 | 142 | { | |
| 137 | - | name: "from RemoteAddr with port", | |
| 138 | - | remoteAddr: "192.168.1.100:12345", | |
| 139 | - | wantIP: "192.168.1.100", | |
| 143 | + | name: "from RemoteAddr with port", | |
| 144 | + | remoteAddr: "192.168.1.100:12345", | |
| 145 | + | trustedNets: noTrustedNets, | |
| 146 | + | wantIP: "192.168.1.100", | |
| 147 | + | }, | |
| 148 | + | { | |
| 149 | + | name: "from RemoteAddr without port", | |
| 150 | + | remoteAddr: "192.168.1.100", | |
| 151 | + | trustedNets: noTrustedNets, | |
| 152 | + | wantIP: "192.168.1.100", | |
| 153 | + | }, | |
| 154 | + | { | |
| 155 | + | name: "XFF ignored when no trusted proxies", | |
| 156 | + | remoteAddr: "192.168.1.100:12345", | |
| 157 | + | xff: "203.0.113.50", | |
| 158 | + | trustedNets: noTrustedNets, | |
| 159 | + | wantIP: "192.168.1.100", | |
| 160 | + | }, | |
| 161 | + | { | |
| 162 | + | name: "XFF ignored when remote is not trusted", | |
| 163 | + | remoteAddr: "192.168.1.100:12345", | |
| 164 | + | xff: "203.0.113.50", | |
| 165 | + | trustedNets: trustedNets, // trusts 127.0.0.1 and 10.0.0.0/8 | |
| 166 | + | wantIP: "192.168.1.100", | |
| 167 | + | }, | |
| 168 | + | { | |
| 169 | + | name: "XFF honored from trusted proxy (single IP)", | |
| 170 | + | remoteAddr: "127.0.0.1:12345", | |
| 171 | + | xff: "203.0.113.50", | |
| 172 | + | trustedNets: trustedNets, | |
| 173 | + | wantIP: "203.0.113.50", | |
| 140 | 174 | }, | |
| 141 | 175 | { | |
| 142 | - | name: "from RemoteAddr without port", | |
| 143 | - | remoteAddr: "192.168.1.100", | |
| 144 | - | wantIP: "192.168.1.100", | |
| 176 | + | name: "XFF honored from trusted proxy (multiple IPs)", | |
| 177 | + | remoteAddr: "127.0.0.1:12345", | |
| 178 | + | xff: "203.0.113.50, 10.0.0.1, 172.16.0.1", | |
| 179 | + | trustedNets: trustedNets, | |
| 180 | + | wantIP: "203.0.113.50", | |
| 145 | 181 | }, | |
| 146 | 182 | { | |
| 147 | - | name: "from X-Forwarded-For single IP", | |
| 148 | - | remoteAddr: "127.0.0.1:12345", | |
| 149 | - | xff: "203.0.113.50", | |
| 150 | - | wantIP: "203.0.113.50", | |
| 183 | + | name: "XFF honored from trusted CIDR", | |
| 184 | + | remoteAddr: "10.1.2.3:12345", | |
| 185 | + | xff: "203.0.113.50", | |
| 186 | + | trustedNets: trustedNets, | |
| 187 | + | wantIP: "203.0.113.50", | |
| 151 | 188 | }, | |
| 152 | 189 | { | |
| 153 | - | name: "from X-Forwarded-For multiple IPs", | |
| 154 | - | remoteAddr: "127.0.0.1:12345", | |
| 155 | - | xff: "203.0.113.50, 10.0.0.1, 172.16.0.1", | |
| 156 | - | wantIP: "203.0.113.50", | |
| 190 | + | name: "X-Real-IP honored from trusted proxy", | |
| 191 | + | remoteAddr: "127.0.0.1:12345", | |
| 192 | + | xRealIP: "203.0.113.50", | |
| 193 | + | trustedNets: trustedNets, | |
| 194 | + | wantIP: "203.0.113.50", | |
| 157 | 195 | }, | |
| 158 | 196 | { | |
| 159 | - | name: "from X-Real-IP", | |
| 160 | - | remoteAddr: "127.0.0.1:12345", | |
| 161 | - | xRealIP: "203.0.113.50", | |
| 162 | - | wantIP: "203.0.113.50", | |
| 197 | + | name: "X-Real-IP ignored when not trusted", | |
| 198 | + | remoteAddr: "192.168.1.100:12345", | |
| 199 | + | xRealIP: "203.0.113.50", | |
| 200 | + | trustedNets: trustedNets, | |
| 201 | + | wantIP: "192.168.1.100", | |
| 163 | 202 | }, | |
| 164 | 203 | { | |
| 165 | - | name: "X-Forwarded-For takes precedence", | |
| 166 | - | remoteAddr: "127.0.0.1:12345", | |
| 167 | - | xff: "203.0.113.50", | |
| 168 | - | xRealIP: "203.0.113.60", | |
| 169 | - | wantIP: "203.0.113.50", | |
| 204 | + | name: "XFF takes precedence over X-Real-IP", | |
| 205 | + | remoteAddr: "127.0.0.1:12345", | |
| 206 | + | xff: "203.0.113.50", | |
| 207 | + | xRealIP: "203.0.113.60", | |
| 208 | + | trustedNets: trustedNets, | |
| 209 | + | wantIP: "203.0.113.50", | |
| 170 | 210 | }, | |
| 171 | 211 | } | |
| 172 | 212 | ||
| @@ -181,12 +221,72 @@ func TestGetClientIP(t *testing.T) { | |||
| 181 | 221 | req.Header.Set("X-Real-IP", tt.xRealIP) | |
| 182 | 222 | } | |
| 183 | 223 | ||
| 184 | - | ip := getClientIP(req) | |
| 224 | + | ip := getClientIP(req, tt.trustedNets) | |
| 185 | 225 | assert.Equal(t, tt.wantIP, ip) | |
| 186 | 226 | }) | |
| 187 | 227 | } | |
| 188 | 228 | } | |
| 189 | 229 | ||
| 230 | + | func TestParseTrustedProxies(t *testing.T) { | |
| 231 | + | tests := []struct { | |
| 232 | + | name string | |
| 233 | + | proxies []string | |
| 234 | + | testIP string | |
| 235 | + | want bool | |
| 236 | + | }{ | |
| 237 | + | { | |
| 238 | + | name: "empty list", | |
| 239 | + | proxies: []string{}, | |
| 240 | + | testIP: "192.168.1.1", | |
| 241 | + | want: false, | |
| 242 | + | }, | |
| 243 | + | { | |
| 244 | + | name: "single IP match", | |
| 245 | + | proxies: []string{"192.168.1.1"}, | |
| 246 | + | testIP: "192.168.1.1", | |
| 247 | + | want: true, | |
| 248 | + | }, | |
| 249 | + | { | |
| 250 | + | name: "single IP no match", | |
| 251 | + | proxies: []string{"192.168.1.1"}, | |
| 252 | + | testIP: "192.168.1.2", | |
| 253 | + | want: false, | |
| 254 | + | }, | |
| 255 | + | { | |
| 256 | + | name: "CIDR match", | |
| 257 | + | proxies: []string{"10.0.0.0/8"}, | |
| 258 | + | testIP: "10.1.2.3", | |
| 259 | + | want: true, | |
| 260 | + | }, | |
| 261 | + | { | |
| 262 | + | name: "CIDR no match", | |
| 263 | + | proxies: []string{"10.0.0.0/8"}, | |
| 264 | + | testIP: "192.168.1.1", | |
| 265 | + | want: false, | |
| 266 | + | }, | |
| 267 | + | { | |
| 268 | + | name: "mixed IP and CIDR", | |
| 269 | + | proxies: []string{"127.0.0.1", "10.0.0.0/8", "172.16.0.0/12"}, | |
| 270 | + | testIP: "172.20.1.1", | |
| 271 | + | want: true, | |
| 272 | + | }, | |
| 273 | + | { | |
| 274 | + | name: "invalid entries ignored", | |
| 275 | + | proxies: []string{"not-an-ip", "10.0.0.0/8"}, | |
| 276 | + | testIP: "10.1.2.3", | |
| 277 | + | want: true, | |
| 278 | + | }, | |
| 279 | + | } | |
| 280 | + | ||
| 281 | + | for _, tt := range tests { | |
| 282 | + | t.Run(tt.name, func(t *testing.T) { | |
| 283 | + | nets := parseTrustedProxies(tt.proxies) | |
| 284 | + | got := isTrustedProxy(tt.testIP, nets) | |
| 285 | + | assert.Equal(t, tt.want, got) | |
| 286 | + | }) | |
| 287 | + | } | |
| 288 | + | } | |
| 289 | + | ||
| 190 | 290 | func TestDefaultRateLimitConfig(t *testing.T) { | |
| 191 | 291 | config := DefaultRateLimitConfig() | |
| 192 | 292 | ||
| @@ -1,6 +1,7 @@ | |||||
| 1 | package registry | 1 | package registry | ||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | + | "net" | |||
| 4 | "net/http" | 5 | "net/http" | ||
| 5 | "net/http/httptest" | 6 | "net/http/httptest" | ||
| 6 | "testing" | 7 | "testing" | ||
| @@ -126,47 +127,86 @@ func TestRateLimitMiddleware_ResponseFormat(t *testing.T) { | |||||
| 126 | } | 127 | } | ||
| 127 | 128 | ||||
| 128 | func TestGetClientIP(t *testing.T) { | 129 | func TestGetClientIP(t *testing.T) { | ||
| 130 | + | // Parse trusted proxies for tests | |||
| 131 | + | trustedNets := parseTrustedProxies([]string{"127.0.0.1", "10.0.0.0/8"}) | |||
| 132 | + | noTrustedNets := []*net.IPNet{} | |||
| 133 | + | ||||
| 129 | tests := []struct { | 134 | tests := []struct { | ||
| 130 | - | name string | 135 | + | name string |
| 131 | - | remoteAddr string | 136 | + | remoteAddr string |
| 132 | - | xff string | 137 | + | xff string |
| 133 | - | xRealIP string | 138 | + | xRealIP string |
| 134 | - | wantIP string | 139 | + | trustedNets []*net.IPNet |
| 140 | + | wantIP string | |||
| 135 | }{ | 141 | }{ | ||
| 136 | { | 142 | { | ||
| 137 | - | name: "from RemoteAddr with port", | 143 | + | name: "from RemoteAddr with port", |
| 138 | - | remoteAddr: "192.168.1.100:12345", | 144 | + | remoteAddr: "192.168.1.100:12345", |
| 139 | - | wantIP: "192.168.1.100", | 145 | + | trustedNets: noTrustedNets, |
| 146 | + | wantIP: "192.168.1.100", | |||
| 147 | + | }, | |||
| 148 | + | { | |||
| 149 | + | name: "from RemoteAddr without port", | |||
| 150 | + | remoteAddr: "192.168.1.100", | |||
| 151 | + | trustedNets: noTrustedNets, | |||
| 152 | + | wantIP: "192.168.1.100", | |||
| 153 | + | }, | |||
| 154 | + | { | |||
| 155 | + | name: "XFF ignored when no trusted proxies", | |||
| 156 | + | remoteAddr: "192.168.1.100:12345", | |||
| 157 | + | xff: "203.0.113.50", | |||
| 158 | + | trustedNets: noTrustedNets, | |||
| 159 | + | wantIP: "192.168.1.100", | |||
| 160 | + | }, | |||
| 161 | + | { | |||
| 162 | + | name: "XFF ignored when remote is not trusted", | |||
| 163 | + | remoteAddr: "192.168.1.100:12345", | |||
| 164 | + | xff: "203.0.113.50", | |||
| 165 | + | trustedNets: trustedNets, // trusts 127.0.0.1 and 10.0.0.0/8 | |||
| 166 | + | wantIP: "192.168.1.100", | |||
| 167 | + | }, | |||
| 168 | + | { | |||
| 169 | + | name: "XFF honored from trusted proxy (single IP)", | |||
| 170 | + | remoteAddr: "127.0.0.1:12345", | |||
| 171 | + | xff: "203.0.113.50", | |||
| 172 | + | trustedNets: trustedNets, | |||
| 173 | + | wantIP: "203.0.113.50", | |||
| 140 | }, | 174 | }, | ||
| 141 | { | 175 | { | ||
| 142 | - | name: "from RemoteAddr without port", | 176 | + | name: "XFF honored from trusted proxy (multiple IPs)", |
| 143 | - | remoteAddr: "192.168.1.100", | 177 | + | remoteAddr: "127.0.0.1:12345", |
| 144 | - | wantIP: "192.168.1.100", | 178 | + | xff: "203.0.113.50, 10.0.0.1, 172.16.0.1", |
| 179 | + | trustedNets: trustedNets, | |||
| 180 | + | wantIP: "203.0.113.50", | |||
| 145 | }, | 181 | }, | ||
| 146 | { | 182 | { | ||
| 147 | - | name: "from X-Forwarded-For single IP", | 183 | + | name: "XFF honored from trusted CIDR", |
| 148 | - | remoteAddr: "127.0.0.1:12345", | 184 | + | remoteAddr: "10.1.2.3:12345", |
| 149 | - | xff: "203.0.113.50", | 185 | + | xff: "203.0.113.50", |
| 150 | - | wantIP: "203.0.113.50", | 186 | + | trustedNets: trustedNets, |
| 187 | + | wantIP: "203.0.113.50", | |||
| 151 | }, | 188 | }, | ||
| 152 | { | 189 | { | ||
| 153 | - | name: "from X-Forwarded-For multiple IPs", | 190 | + | name: "X-Real-IP honored from trusted proxy", |
| 154 | - | remoteAddr: "127.0.0.1:12345", | 191 | + | remoteAddr: "127.0.0.1:12345", |
| 155 | - | xff: "203.0.113.50, 10.0.0.1, 172.16.0.1", | 192 | + | xRealIP: "203.0.113.50", |
| 156 | - | wantIP: "203.0.113.50", | 193 | + | trustedNets: trustedNets, |
| 194 | + | wantIP: "203.0.113.50", | |||
| 157 | }, | 195 | }, | ||
| 158 | { | 196 | { | ||
| 159 | - | name: "from X-Real-IP", | 197 | + | name: "X-Real-IP ignored when not trusted", |
| 160 | - | remoteAddr: "127.0.0.1:12345", | 198 | + | remoteAddr: "192.168.1.100:12345", |
| 161 | - | xRealIP: "203.0.113.50", | 199 | + | xRealIP: "203.0.113.50", |
| 162 | - | wantIP: "203.0.113.50", | 200 | + | trustedNets: trustedNets, |
| 201 | + | wantIP: "192.168.1.100", | |||
| 163 | }, | 202 | }, | ||
| 164 | { | 203 | { | ||
| 165 | - | name: "X-Forwarded-For takes precedence", | 204 | + | name: "XFF takes precedence over X-Real-IP", |
| 166 | - | remoteAddr: "127.0.0.1:12345", | 205 | + | remoteAddr: "127.0.0.1:12345", |
| 167 | - | xff: "203.0.113.50", | 206 | + | xff: "203.0.113.50", |
| 168 | - | xRealIP: "203.0.113.60", | 207 | + | xRealIP: "203.0.113.60", |
| 169 | - | wantIP: "203.0.113.50", | 208 | + | trustedNets: trustedNets, |
| 209 | + | wantIP: "203.0.113.50", | |||
| 170 | }, | 210 | }, | ||
| 171 | } | 211 | } | ||
| 172 | 212 | ||||
| @@ -181,12 +221,72 @@ func TestGetClientIP(t *testing.T) { | |||||
| 181 | req.Header.Set("X-Real-IP", tt.xRealIP) | 221 | req.Header.Set("X-Real-IP", tt.xRealIP) | ||
| 182 | } | 222 | } | ||
| 183 | 223 | ||||
| 184 | - | ip := getClientIP(req) | 224 | + | ip := getClientIP(req, tt.trustedNets) |
| 185 | assert.Equal(t, tt.wantIP, ip) | 225 | assert.Equal(t, tt.wantIP, ip) | ||
| 186 | }) | 226 | }) | ||
| 187 | } | 227 | } | ||
| 188 | } | 228 | } | ||
| 189 | 229 | ||||
| 230 | + | func TestParseTrustedProxies(t *testing.T) { | |||
| 231 | + | tests := []struct { | |||
| 232 | + | name string | |||
| 233 | + | proxies []string | |||
| 234 | + | testIP string | |||
| 235 | + | want bool | |||
| 236 | + | }{ | |||
| 237 | + | { | |||
| 238 | + | name: "empty list", | |||
| 239 | + | proxies: []string{}, | |||
| 240 | + | testIP: "192.168.1.1", | |||
| 241 | + | want: false, | |||
| 242 | + | }, | |||
| 243 | + | { | |||
| 244 | + | name: "single IP match", | |||
| 245 | + | proxies: []string{"192.168.1.1"}, | |||
| 246 | + | testIP: "192.168.1.1", | |||
| 247 | + | want: true, | |||
| 248 | + | }, | |||
| 249 | + | { | |||
| 250 | + | name: "single IP no match", | |||
| 251 | + | proxies: []string{"192.168.1.1"}, | |||
| 252 | + | testIP: "192.168.1.2", | |||
| 253 | + | want: false, | |||
| 254 | + | }, | |||
| 255 | + | { | |||
| 256 | + | name: "CIDR match", | |||
| 257 | + | proxies: []string{"10.0.0.0/8"}, | |||
| 258 | + | testIP: "10.1.2.3", | |||
| 259 | + | want: true, | |||
| 260 | + | }, | |||
| 261 | + | { | |||
| 262 | + | name: "CIDR no match", | |||
| 263 | + | proxies: []string{"10.0.0.0/8"}, | |||
| 264 | + | testIP: "192.168.1.1", | |||
| 265 | + | want: false, | |||
| 266 | + | }, | |||
| 267 | + | { | |||
| 268 | + | name: "mixed IP and CIDR", | |||
| 269 | + | proxies: []string{"127.0.0.1", "10.0.0.0/8", "172.16.0.0/12"}, | |||
| 270 | + | testIP: "172.20.1.1", | |||
| 271 | + | want: true, | |||
| 272 | + | }, | |||
| 273 | + | { | |||
| 274 | + | name: "invalid entries ignored", | |||
| 275 | + | proxies: []string{"not-an-ip", "10.0.0.0/8"}, | |||
| 276 | + | testIP: "10.1.2.3", | |||
| 277 | + | want: true, | |||
| 278 | + | }, | |||
| 279 | + | } | |||
| 280 | + | ||||
| 281 | + | for _, tt := range tests { | |||
| 282 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 283 | + | nets := parseTrustedProxies(tt.proxies) | |||
| 284 | + | got := isTrustedProxy(tt.testIP, nets) | |||
| 285 | + | assert.Equal(t, tt.want, got) | |||
| 286 | + | }) | |||
| 287 | + | } | |||
| 288 | + | } | |||
| 289 | + | ||||
| 190 | func TestDefaultRateLimitConfig(t *testing.T) { | 290 | func TestDefaultRateLimitConfig(t *testing.T) { | ||
| 191 | config := DefaultRateLimitConfig() | 291 | config := DefaultRateLimitConfig() | ||
| 192 | 292 | ||||
M internal/app/run.go +16 -11
| @@ -103,10 +103,11 @@ type Config struct { | |||
| 103 | 103 | ||
| 104 | 104 | API struct { | |
| 105 | 105 | RateLimit struct { | |
| 106 | - | Enabled bool `mapstructure:"enabled"` | |
| 107 | - | GlobalRPS float64 `mapstructure:"global_rps"` | |
| 108 | - | PerIPRPS float64 `mapstructure:"per_ip_rps"` | |
| 109 | - | Burst int `mapstructure:"burst"` | |
| 106 | + | Enabled bool `mapstructure:"enabled"` | |
| 107 | + | GlobalRPS float64 `mapstructure:"global_rps"` | |
| 108 | + | PerIPRPS float64 `mapstructure:"per_ip_rps"` | |
| 109 | + | Burst int `mapstructure:"burst"` | |
| 110 | + | TrustedProxies []string `mapstructure:"trusted_proxies"` | |
| 110 | 111 | } `mapstructure:"rate_limit"` | |
| 111 | 112 | } `mapstructure:"api"` | |
| 112 | 113 | } | |
| @@ -874,12 +875,14 @@ func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Ha | |||
| 874 | 875 | ||
| 875 | 876 | // Add rate limiting middleware | |
| 876 | 877 | rateLimitConfig := registry.RateLimitConfig{ | |
| 877 | - | Enabled: cfg.API.RateLimit.Enabled, | |
| 878 | - | GlobalRPS: cfg.API.RateLimit.GlobalRPS, | |
| 879 | - | PerIPRPS: cfg.API.RateLimit.PerIPRPS, | |
| 880 | - | Burst: cfg.API.RateLimit.Burst, | |
| 878 | + | Enabled: cfg.API.RateLimit.Enabled, | |
| 879 | + | GlobalRPS: cfg.API.RateLimit.GlobalRPS, | |
| 880 | + | PerIPRPS: cfg.API.RateLimit.PerIPRPS, | |
| 881 | + | Burst: cfg.API.RateLimit.Burst, | |
| 882 | + | TrustedProxies: cfg.API.RateLimit.TrustedProxies, | |
| 881 | 883 | } | |
| 882 | - | registryMiddlewares = append(registryMiddlewares, registry.RateLimitMiddleware(rateLimitConfig)) | |
| 884 | + | rateLimitMiddleware := registry.RateLimitMiddleware(rateLimitConfig) | |
| 885 | + | registryMiddlewares = append(registryMiddlewares, rateLimitMiddleware) | |
| 883 | 886 | ||
| 884 | 887 | if cfg.Auth.Enabled && svc.authSvc != nil { | |
| 885 | 888 | internalAuth := middleware.InternalRegistryAuth{ | |
| @@ -897,11 +900,13 @@ func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Ha | |||
| 897 | 900 | registryMux := http.NewServeMux() | |
| 898 | 901 | if svc.tokenHandler != nil { | |
| 899 | 902 | // Token endpoint is NOT protected by auth - it's where clients get tokens | |
| 900 | - | tokenWithLogging := middleware.Chain( | |
| 903 | + | // but it still needs rate limiting to prevent brute force attacks | |
| 904 | + | tokenWithMiddleware := middleware.Chain( | |
| 901 | 905 | middleware.PanicRecovery(log), | |
| 902 | 906 | middleware.RequestLogger(log), | |
| 907 | + | rateLimitMiddleware, | |
| 903 | 908 | )(svc.tokenHandler) | |
| 904 | - | registryMux.Handle("/v2/token", tokenWithLogging) | |
| 909 | + | registryMux.Handle("/v2/token", tokenWithMiddleware) | |
| 905 | 910 | } | |
| 906 | 911 | registryMux.Handle("/v2/", registryWithMiddleware) | |
| 907 | 912 | ||
| @@ -103,10 +103,11 @@ type Config struct { | |||||
| 103 | 103 | ||||
| 104 | API struct { | 104 | API struct { | ||
| 105 | RateLimit struct { | 105 | RateLimit struct { | ||
| 106 | - | Enabled bool `mapstructure:"enabled"` | 106 | + | Enabled bool `mapstructure:"enabled"` |
| 107 | - | GlobalRPS float64 `mapstructure:"global_rps"` | 107 | + | GlobalRPS float64 `mapstructure:"global_rps"` |
| 108 | - | PerIPRPS float64 `mapstructure:"per_ip_rps"` | 108 | + | PerIPRPS float64 `mapstructure:"per_ip_rps"` |
| 109 | - | Burst int `mapstructure:"burst"` | 109 | + | Burst int `mapstructure:"burst"` |
| 110 | + | TrustedProxies []string `mapstructure:"trusted_proxies"` | |||
| 110 | } `mapstructure:"rate_limit"` | 111 | } `mapstructure:"rate_limit"` | ||
| 111 | } `mapstructure:"api"` | 112 | } `mapstructure:"api"` | ||
| 112 | } | 113 | } | ||
| @@ -874,12 +875,14 @@ func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Ha | |||||
| 874 | 875 | ||||
| 875 | // Add rate limiting middleware | 876 | // Add rate limiting middleware | ||
| 876 | rateLimitConfig := registry.RateLimitConfig{ | 877 | rateLimitConfig := registry.RateLimitConfig{ | ||
| 877 | - | Enabled: cfg.API.RateLimit.Enabled, | 878 | + | Enabled: cfg.API.RateLimit.Enabled, |
| 878 | - | GlobalRPS: cfg.API.RateLimit.GlobalRPS, | 879 | + | GlobalRPS: cfg.API.RateLimit.GlobalRPS, |
| 879 | - | PerIPRPS: cfg.API.RateLimit.PerIPRPS, | 880 | + | PerIPRPS: cfg.API.RateLimit.PerIPRPS, |
| 880 | - | Burst: cfg.API.RateLimit.Burst, | 881 | + | Burst: cfg.API.RateLimit.Burst, |
| 882 | + | TrustedProxies: cfg.API.RateLimit.TrustedProxies, | |||
| 881 | } | 883 | } | ||
| 882 | - | registryMiddlewares = append(registryMiddlewares, registry.RateLimitMiddleware(rateLimitConfig)) | 884 | + | rateLimitMiddleware := registry.RateLimitMiddleware(rateLimitConfig) |
| 885 | + | registryMiddlewares = append(registryMiddlewares, rateLimitMiddleware) | |||
| 883 | 886 | ||||
| 884 | if cfg.Auth.Enabled && svc.authSvc != nil { | 887 | if cfg.Auth.Enabled && svc.authSvc != nil { | ||
| 885 | internalAuth := middleware.InternalRegistryAuth{ | 888 | internalAuth := middleware.InternalRegistryAuth{ | ||
| @@ -897,11 +900,13 @@ func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Ha | |||||
| 897 | registryMux := http.NewServeMux() | 900 | registryMux := http.NewServeMux() | ||
| 898 | if svc.tokenHandler != nil { | 901 | if svc.tokenHandler != nil { | ||
| 899 | // Token endpoint is NOT protected by auth - it's where clients get tokens | 902 | // Token endpoint is NOT protected by auth - it's where clients get tokens | ||
| 900 | - | tokenWithLogging := middleware.Chain( | 903 | + | // but it still needs rate limiting to prevent brute force attacks |
| 904 | + | tokenWithMiddleware := middleware.Chain( | |||
| 901 | middleware.PanicRecovery(log), | 905 | middleware.PanicRecovery(log), | ||
| 902 | middleware.RequestLogger(log), | 906 | middleware.RequestLogger(log), | ||
| 907 | + | rateLimitMiddleware, | |||
| 903 | )(svc.tokenHandler) | 908 | )(svc.tokenHandler) | ||
| 904 | - | registryMux.Handle("/v2/token", tokenWithLogging) | 909 | + | registryMux.Handle("/v2/token", tokenWithMiddleware) |
| 905 | } | 910 | } | ||
| 906 | registryMux.Handle("/v2/", registryWithMiddleware) | 911 | registryMux.Handle("/v2/", registryWithMiddleware) | ||
| 907 | 912 | ||||