feat: add a trusted-proxy boundary for the client source address

The address MinIO attributes a request to is read from X-Forwarded-For, X-Real-IP or RFC 7239 Forwarded, and never from the connection unless all three are absent. It becomes aws:SourceIp and the audit remotehost field, so any client that can reach the API port chooses the value an IpAddress condition is evaluated against and the address every logged action is attributed to. MINIO_API_TRUSTED_PROXIES now selects who may make that claim: unset the historical behaviour, unchanged none no forwarded header is believed; the TCP peer wins <CIDRs> believed only from listed peers, chains read right-to-left Reading right-to-left is what makes an appending proxy safe: each hop appends the peer it actually saw, so an entry a client injected can only sit to the left of one a proxy wrote. The stock nginx recipe $proxy_add_x_forwarded_for appends, which leaves the client's entry left-most - exactly where the untrusted path reads - so a deployment with no direct route to the API port was forgeable too. _MINIO_API_XFF_HEADER is deliberately untouched, in semantics and in read timing. Widening it to mean "trust nothing" was implemented and reverted: it is the only part of this change that could alter a deployed configuration, and the new variable expresses the same guarantee at no compatibility cost. Upstream's TestXFFDisabled is retained verbatim. Notes on the allow-list mode, all covered by tests: - it must name proxies, not the subnet they sit in; listed entries are skipped while walking, so a range covering clients lets them forge - a cluster must list its own nodes, because MinIO forwards between them and a client can force a hop via the ListObjectsV2 token - loopback is trusted as a peer, not as a chain entry, so FTP and SFTP keep attributing their sessions - the node-to-node forwarder drops X-Real-IP and Forwarded from a peer not entitled to have set them - the walk scans the header in place and stops after 100 hops, so a long chain costs neither allocation nor unbounded work No behaviour change for any deployment that does not set the new variable: the untrusted path is a verbatim copy of the previous function body, differentially verified against it over ~5.1M header combinations. The LDAP STS allow-list now shares the list parser as pure code motion, verified identical across every combination of 37 allow-list values and 21 peer addresses. Co-authored-by: ChatGPT <noreply@openai.com> Co-authored-by: Claude <noreply@anthropic.com>
2026-08-09 07:43:29 +03:00 · 2026-08-04 13:09:06 +08:00
parent 744a9dcd71
commit fe6dc47804
10 changed files with 1369 additions and 74 deletions
@@ -22,9 +22,7 @@ import (
 	"crypto/x509"
 	"errors"
 	"net"
-	"net/netip"
 	"sort"
-	"strings"
 	"time"

 	"github.com/minio/madmin-go/v3"
@@ -45,7 +43,7 @@ type Config struct {
 	LDAP ldap.Config

 	stsExpiryDuration time.Duration // contains converted value
-	stsTrustedProxies []netip.Prefix
+	stsTrustedProxies config.TrustedProxies
 }

 // Enabled returns if LDAP is enabled.
@@ -61,7 +59,7 @@ func (l *Config) Clone() Config {
 	cfg := Config{
 		LDAP:              l.LDAP.Clone(),
 		stsExpiryDuration: l.stsExpiryDuration,
-		stsTrustedProxies: append([]netip.Prefix(nil), l.stsTrustedProxies...),
+		stsTrustedProxies: append(config.TrustedProxies(nil), l.stsTrustedProxies...),
 	}
 	return cfg
 }
@@ -168,49 +166,9 @@ var (
 	}
 )

-func parseSTSTrustedProxies(value string) ([]netip.Prefix, error) {
-	fields := strings.FieldsFunc(value, func(r rune) bool {
-		switch r {
-		case ',', ';', ' ', '\n', '\r', '\t':
-			return true
-		default:
-			return false
-		}
-	})
-	if len(fields) == 0 {
-		return nil, nil
-	}
-
-	prefixes := make([]netip.Prefix, 0, len(fields))
-	for _, field := range fields {
-		if prefix, err := netip.ParsePrefix(field); err == nil {
-			masked := prefix.Masked()
-			// Reject catch-all ranges (0.0.0.0/0, ::/0): they would trust
-			// forwarded headers from every peer and defeat the allowlist.
-			if masked.Bits() == 0 {
-				return nil, config.Errorf("LDAP STS trusted proxy %q is too broad", field)
-			}
-			prefixes = append(prefixes, masked)
-			continue
-		}
-
-		addr, err := netip.ParseAddr(field)
-		if err != nil {
-			return nil, config.Errorf("invalid LDAP STS trusted proxy %q", field)
-		}
-		bits := 32
-		if addr.Is6() {
-			bits = 128
-		}
-		prefixes = append(prefixes, netip.PrefixFrom(addr, bits))
-	}
-
-	return prefixes, nil
-}
-
 // SetSTSTrustedProxies parses and stores the LDAP STS trusted proxy allowlist.
 func (l *Config) SetSTSTrustedProxies(value string) error {
-	prefixes, err := parseSTSTrustedProxies(value)
+	prefixes, err := config.ParseTrustedProxies(value, "LDAP STS trusted proxy")
 	if err != nil {
 		return err
 	}
@@ -219,23 +177,14 @@ func (l *Config) SetSTSTrustedProxies(value string) error {
 }

 // IsSTSTrustedProxy reports whether the peer IP is allowed to supply forwarded headers
-// for LDAP STS source bucketing.
+// for LDAP STS source bucketing. This allowlist covers STS login rate-limit
+// bucketing only; the allowlist governing aws:SourceIp and the audit client
+// address lives in internal/handlers.
 func (l *Config) IsSTSTrustedProxy(peerIP string) bool {
-	if l == nil || len(l.stsTrustedProxies) == 0 {
+	if l == nil {
 		return false
 	}
-
-	addr, err := netip.ParseAddr(peerIP)
-	if err != nil {
-		return false
-	}
-
-	for _, prefix := range l.stsTrustedProxies {
-		if prefix.Contains(addr) {
-			return true
-		}
-	}
-	return false
+	return l.stsTrustedProxies.Contains(peerIP)
 }

 // Enabled returns if LDAP config is enabled.
@@ -0,0 +1,104 @@
+// Copyright (c) 2015-2025 MinIO, Inc.
+//
+// This file is part of MinIO Object Storage stack
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package config
+
+import (
+	"net/netip"
+	"strings"
+)
+
+// TrustedProxies is an allow-list of peer addresses whose forwarded headers may
+// be believed. The zero value trusts nobody, which is the safe default: a
+// request whose peer is not on the list is attributed to the peer itself rather
+// than to whatever address the request claims.
+type TrustedProxies []netip.Prefix
+
+// ParseTrustedProxies parses a list of IP addresses and CIDR blocks separated by
+// commas, semicolons or whitespace. A bare address is widened to a single-host
+// prefix. what names the setting being parsed and is used in error messages.
+//
+// Catch-all prefixes are rejected rather than accepted, because a list
+// containing 0.0.0.0/0 or ::/0 trusts every peer and so silently reinstates the
+// behavior the allow-list exists to prevent.
+func ParseTrustedProxies(value, what string) (TrustedProxies, error) {
+	fields := strings.FieldsFunc(value, func(r rune) bool {
+		switch r {
+		case ',', ';', ' ', '\n', '\r', '\t':
+			return true
+		default:
+			return false
+		}
+	})
+	if len(fields) == 0 {
+		return nil, nil
+	}
+
+	prefixes := make(TrustedProxies, 0, len(fields))
+	for _, field := range fields {
+		if prefix, err := netip.ParsePrefix(field); err == nil {
+			masked := prefix.Masked()
+			if masked.Bits() == 0 {
+				return nil, Errorf("%s %q is too broad", what, field)
+			}
+			prefixes = append(prefixes, masked)
+			continue
+		}
+
+		addr, err := netip.ParseAddr(field)
+		if err != nil {
+			return nil, Errorf("invalid %s %q", what, field)
+		}
+		bits := 32
+		if addr.Is6() {
+			bits = 128
+		}
+		prefixes = append(prefixes, netip.PrefixFrom(addr, bits))
+	}
+
+	return prefixes, nil
+}
+
+// Contains reports whether ip is covered by the allow-list. ip must already be a
+// bare address; anything that does not parse is not trusted.
+//
+// Matching is deliberately literal. An entry written in IPv4-mapped form
+// (::ffff:192.168.1.10) is a 128-bit prefix and will not match a peer presenting
+// as 192.168.1.10, because netip.Prefix.Contains is false across differing
+// widths - so such an entry is accepted and then matches nothing. Rewriting
+// those entries to the IPv4 prefix they denote was tried and reverted: callers
+// already reduce the address through net.ParseIP(...).String() before arriving
+// here, which collapses the mapped form anyway, so the rewrite reached no real
+// request and served only to change what this shared function means for the
+// LDAP STS allow-list that was using it first. Write entries in plain form.
+func (t TrustedProxies) Contains(ip string) bool {
+	if len(t) == 0 {
+		return false
+	}
+
+	addr, err := netip.ParseAddr(ip)
+	if err != nil {
+		return false
+	}
+
+	for _, prefix := range t {
+		if prefix.Contains(addr) {
+			return true
+		}
+	}
+	return false
+}