Merge pull request #453 from myENA/fix/aws-xff

Handle proxy chains of any length

Fixes #449

route/access_rules.go

@@ -36,14 +36,22 @@ func (t *Target) AccessDeniedHTTP(r *http.Request) bool {
 
 	// check xff source if present
 	if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
-		// only use left-most element (client)
-		xff = strings.TrimSpace(strings.SplitN(xff, ",", 2)[0])
-		// only continue if xff differs from host
-		if xff != host {
-			if ip = net.ParseIP(xff); ip == nil {
-				log.Printf("[WARN] failed to parse xff address %s", xff)
+		// Trusting XFF headers sent from clients is dangerous and generally
+		// bad practice.  Therefore, we cannot assume which if any of the elements
+		// is the actual client address.  To try and avoid the chance of spoofed
+		// headers and/or loose upstream proxies we validate all elements in the header.
+		// Specifically AWS does not strip XFF from anonymous internet sources:
+		// https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/x-forwarded-headers.html#x-forwarded-for
+		// See lengthy github discussion for more background: https://github.com/fabiolb/fabio/pull/449
+		for _, xip := range strings.Split(xff, ",") {
+			xip = strings.TrimSpace(xip)
+			if xip == host {
+				continue
+			}
+			if ip = net.ParseIP(xip); ip == nil {
+				log.Printf("[WARN] failed to parse xff address %s", xip)
+				continue
 			}
-			// check xff source and return if denied
 			if t.denyByIP(ip) {
 				return true
 			}

route/access_rules_test.go

@@ -165,11 +165,11 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) {
 		denied bool
 	}{
 		{
-			desc: "denied xff and allowed remote addr",
+			desc: "single denied xff and allowed remote addr",
 			target: &Target{
 				Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"},
 			},
-			xff:    "1.1.1.2, 10.11.12.13, 10.11.12.14",
+			xff:    "10.11.12.13, 1.1.1.2, 10.11.12.14",
 			remote: "10.11.12.1:65500",
 			denied: true,
 		},
@@ -183,13 +183,13 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) {
 			denied: true,
 		},
 		{
-			desc: "allowed xff and allowed remote addr",
+			desc: "single allowed xff and allowed remote addr",
 			target: &Target{
 				Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"},
 			},
 			xff:    "10.11.12.13, 1.2.3.4",
 			remote: "192.168.0.12:65500",
-			denied: false,
+			denied: true,
 		},
 		{
 			desc: "denied xff and denied remote addr",
@@ -200,6 +200,15 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) {
 			remote: "200.17.18.20:65500",
 			denied: true,
 		},
+		{
+			desc: "all allowed xff and allowed remote addr",
+			target: &Target{
+				Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"},
+			},
+			xff:    "10.11.12.13, 10.110.120.130",
+			remote: "192.168.0.12:65500",
+			denied: false,
+		},
 	}
 
 	for i, tt := range tests {