Overworked header handling.

fabio.properties

@@ -35,7 +35,7 @@
 
 
 # proxy.localip configures the ip address of the proxy which is added
-# to the X-Forwarded-For and Forwarded headers.
+# to the Header configured by header.clientip and to the 'Forwarded: by=' attribute.
 #
 # The local non-loopback address is detected during startup
 # but can be overwritten with this property.
@@ -102,25 +102,6 @@
 # proxy.header.clientip configures the header for the request ip.
 #
 # The remoteIP is taken from http.Request.RemoteAddr.
-# The localIP is either detected or the value of proxy.localip.
-#
-# When set to 'X-Forwarded-For' the proxy will set this header to
-#
-#   <remoteIP>, <localIP>
-#
-# if the header is not set in the incoming request. Otherwise, it
-# will just append
-#
-#   , <localIP>
-#
-# to the existing header value.
-#
-# When set to another non-empty value the proxy will set this header
-# to
-#
-#   <remoteIP>
-#
-# The default is
 #
 # proxy.header.clientip =
 

proxy/proxy_integration_test.go

@@ -0,0 +1,40 @@
+package proxy
+
+import (
+	"net"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"testing"
+
+	"github.com/eBay/fabio/config"
+	"github.com/eBay/fabio/route"
+)
+
+func TestProxyProducesCorrectXffHeader(t *testing.T) {
+	got := "not called"
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		got = r.Header.Get("X-Forwarded-For")
+	}))
+	defer server.Close()
+
+	table := make(route.Table)
+	table.AddRoute("mock", "/", server.URL, 1, nil)
+	route.SetTable(table)
+
+	tr := &http.Transport{Dial: (&net.Dialer{}).Dial}
+	proxy := New(tr, config.Proxy{LocalIP: "1.1.1.1", ClientIPHeader: "X-Forwarded-For"})
+
+	req := &http.Request{
+		RequestURI: "/",
+		Header:     http.Header{"X-Forwarded-For": {"3.3.3.3"}},
+		RemoteAddr: "2.2.2.2:666",
+		URL:        &url.URL{},
+	}
+
+	proxy.ServeHTTP(httptest.NewRecorder(), req)
+
+	if want := "3.3.3.3, 2.2.2.2"; got != want {
+		t.Errorf("got %v, but want %v", got, want)
+	}
+}

proxy/util.go

@@ -13,7 +13,8 @@ import (
 // addHeaders adds/updates headers in request
 //
 // * add/update `Forwarded` header
-// * ClientIPHeader == "X-Forwarded-For": add/update `X-Forwarded-For` header
+// * add X-Forwarded-Proto header, if not present
+// * add X-Real-Ip, if not present
 // * ClientIPHeader != "": Set header with that name to <remote ip>
 // * TLS connection: Set header with name from `cfg.TLSHeader` to `cfg.TLSHeaderValue`
 //
@@ -23,13 +24,22 @@ func addHeaders(r *http.Request, cfg config.Proxy) error {
 		return errors.New("cannot parse " + r.RemoteAddr)
 	}
 
-	if cfg.ClientIPHeader != "" {
+	// Set configurable ClientIPHeader here, but no if it is X-Forwarded-For here,
+	// because X-Forwarded-For will be set by the Golang reverse proxy handler, later.
+	if cfg.ClientIPHeader != "" && cfg.ClientIPHeader != "X-Forwarded-For" && cfg.ClientIPHeader != "X-Real-Ip" {
 		r.Header.Set(cfg.ClientIPHeader, remoteIP)
 	}
 
-	xff := r.Header.Get("X-Forwarded-For")
-	if xff != "" && cfg.LocalIP != "" {
-		r.Header.Set("X-Forwarded-For", xff+", "+cfg.LocalIP)
+	if r.Header.Get("X-Real-Ip") == "" {
+		r.Header.Set("X-Real-Ip", remoteIP)
+	}
+
+	if r.Header.Get("X-Forwarded-Proto") == "" {
+		if r.TLS != nil {
+			r.Header.Set("X-Forwarded-Proto", "https")
+		} else {
+			r.Header.Set("X-Forwarded-Proto", "http")
+		}
 	}
 
 	fwd := r.Header.Get("Forwarded")

proxy/util_test.go

@@ -3,100 +3,114 @@ package proxy
 import (
 	"crypto/tls"
 	"net/http"
-	"reflect"
 	"testing"
 
 	"github.com/eBay/fabio/config"
 )
 
 func TestAddHeaders(t *testing.T) {
 	tests := []struct {
+		desc string
 		r    *http.Request
 		cfg  config.Proxy
 		hdrs http.Header
 		err  string
 	}{
-		{ // error
+		{"error",
 			&http.Request{RemoteAddr: "1.2.3.4"},
 			config.Proxy{},
 			http.Header{},
 			"cannot parse 1.2.3.4",
 		},
 
-		{ // set remote ip header
+		{"set remote ip header",
 			&http.Request{RemoteAddr: "1.2.3.4:5555"},
 			config.Proxy{ClientIPHeader: "Client-IP"},
-			http.Header{"Forwarded": {"for=1.2.3.4; proto=http"}, "Client-Ip": []string{"1.2.3.4"}},
+			http.Header{"Client-Ip": []string{"1.2.3.4"}},
 			"",
 		},
 
-		{ // set remote ip header with local ip (no change expected)
+		{"set remote ip header with local ip (no change expected)",
 			&http.Request{RemoteAddr: "1.2.3.4:5555"},
 			config.Proxy{LocalIP: "5.6.7.8", ClientIPHeader: "Client-IP"},
-			http.Header{"Forwarded": {"for=1.2.3.4; proto=http; by=5.6.7.8"}, "Client-Ip": []string{"1.2.3.4"}},
+			http.Header{"Client-Ip": []string{"1.2.3.4"}},
 			"",
 		},
 
-		{ // set X-Forwarded-For
-			&http.Request{RemoteAddr: "1.2.3.4:5555"},
-			config.Proxy{ClientIPHeader: "X-Forwarded-For"},
-			http.Header{"Forwarded": {"for=1.2.3.4; proto=http"}, "X-Forwarded-For": []string{"1.2.3.4"}},
-			"",
-		},
-
-		{ // set X-Forwarded-For with local ip
-			&http.Request{RemoteAddr: "1.2.3.4:5555"},
-			config.Proxy{LocalIP: "5.6.7.8", ClientIPHeader: "X-Forwarded-For"},
-			http.Header{"Forwarded": {"for=1.2.3.4; proto=http; by=5.6.7.8"}, "X-Forwarded-For": []string{"1.2.3.4, 5.6.7.8"}},
-			"",
-		},
-
-		{ // extend X-Forwarded-For with local ip
-			&http.Request{RemoteAddr: "1.2.3.4:5555", Header: http.Header{"X-Forwarded-For": []string{"9.9.9.9"}}},
-			config.Proxy{LocalIP: "5.6.7.8"},
-			http.Header{"Forwarded": {"for=1.2.3.4; proto=http; by=5.6.7.8"}, "X-Forwarded-For": []string{"9.9.9.9, 5.6.7.8"}},
-			"",
-		},
-
-		{ // set Forwarded
+		{"set Forwarded",
 			&http.Request{RemoteAddr: "1.2.3.4:5555"},
 			config.Proxy{},
 			http.Header{"Forwarded": {"for=1.2.3.4; proto=http"}},
 			"",
 		},
 
-		{ // set Forwarded with localIP
+		{"set Forwarded with localIP",
 			&http.Request{RemoteAddr: "1.2.3.4:5555"},
 			config.Proxy{LocalIP: "5.6.7.8"},
 			http.Header{"Forwarded": {"for=1.2.3.4; proto=http; by=5.6.7.8"}},
 			"",
 		},
 
-		{ // set Forwarded with localIP and HTTPS
+		{"set Forwarded with localIP and HTTPS",
 			&http.Request{RemoteAddr: "1.2.3.4:5555", TLS: &tls.ConnectionState{}},
 			config.Proxy{LocalIP: "5.6.7.8"},
 			http.Header{"Forwarded": {"for=1.2.3.4; proto=https; by=5.6.7.8"}},
 			"",
 		},
 
-		{ // extend Forwarded with localIP
+		{"extend Forwarded with localIP",
 			&http.Request{RemoteAddr: "1.2.3.4:5555", Header: http.Header{"Forwarded": {"for=9.9.9.9; proto=http; by=8.8.8.8"}}},
 			config.Proxy{LocalIP: "5.6.7.8"},
 			http.Header{"Forwarded": {"for=9.9.9.9; proto=http; by=8.8.8.8; by=5.6.7.8"}},
 			"",
 		},
 
-		{ // set tls header
+		{"set tls header",
 			&http.Request{RemoteAddr: "1.2.3.4:5555", TLS: &tls.ConnectionState{}},
-			config.Proxy{LocalIP: "5.6.7.8", TLSHeader: "Secure"},
-			http.Header{"Forwarded": {"for=1.2.3.4; proto=https; by=5.6.7.8"}, "Secure": {""}},
+			config.Proxy{TLSHeader: "Secure"},
+			http.Header{"Secure": {""}},
+			"",
+		},
+
+		{"set tls header with value",
+			&http.Request{RemoteAddr: "1.2.3.4:5555", TLS: &tls.ConnectionState{}},
+			config.Proxy{TLSHeader: "Secure", TLSHeaderValue: "true"},
+			http.Header{"Secure": {"true"}},
+			"",
+		},
+
+		{"set Proto",
+			&http.Request{RemoteAddr: "1.2.3.4:5555"},
+			config.Proxy{},
+			http.Header{"X-Forwarded-Proto": {"http"}},
 			"",
 		},
 
-		{ // set tls header with value
+		{"set Proto with TLS",
 			&http.Request{RemoteAddr: "1.2.3.4:5555", TLS: &tls.ConnectionState{}},
-			config.Proxy{LocalIP: "5.6.7.8", TLSHeader: "Secure", TLSHeaderValue: "true"},
-			http.Header{"Forwarded": {"for=1.2.3.4; proto=https; by=5.6.7.8"}, "Secure": {"true"}},
+			config.Proxy{},
+			http.Header{"X-Forwarded-Proto": {"https"}},
+			"",
+		},
+
+		{"don't overwrite X-Forwarded-Proto header, if present",
+			&http.Request{RemoteAddr: "1.2.3.4:5555", Header: http.Header{"X-Forwarded-Proto": {"https"}}},
+			config.Proxy{},
+			http.Header{"X-Forwarded-Proto": {"https"}},
+			"",
+		},
+
+		{"set X-Real-Ip header, if not present",
+			&http.Request{RemoteAddr: "1.2.3.4:5555"},
+			config.Proxy{},
+			http.Header{"X-Real-Ip": {"1.2.3.4"}},
+			"",
+		},
+
+		{"don't overwrite X-Real-Ip header, if present",
+			&http.Request{RemoteAddr: "1.2.3.4:5555", Header: http.Header{"X-Real-Ip": {"6.6.6.6"}}},
+			config.Proxy{},
+			http.Header{"X-Real-Ip": {"6.6.6.6"}},
 			"",
 		},
 	}
@@ -109,16 +123,20 @@ func TestAddHeaders(t *testing.T) {
 		err := addHeaders(tt.r, tt.cfg)
 		if err != nil {
 			if got, want := err.Error(), tt.err; got != want {
-				t.Errorf("%d: got %q want %q", i, got, want)
+				t.Errorf("%d: %s\ngot  %q\nwant %q", i, tt.desc, got, want)
 			}
 			continue
 		}
 		if tt.err != "" {
 			t.Errorf("%d: got nil want %q", i, tt.err)
 			continue
 		}
-		if got, want := tt.r.Header, tt.hdrs; !reflect.DeepEqual(got, want) {
-			t.Errorf("%d: got %v want %v", i, got, want)
+		for headerName, _ := range tt.hdrs {
+			got := tt.r.Header.Get(headerName)
+			want := tt.hdrs.Get(headerName)
+			if got != want {
+				t.Errorf("%d: %s \nWrong value for Header: %s \ngot  %q \nwant %q", i, tt.desc, headerName, got, want)
+			}
 		}
 	}
 }