Merge pull request #2219 from dmcgowan/fix-forwarded-logic

Fix forwarded logic

registry/api/v2/urls.go

@@ -2,10 +2,8 @@ package v2
 
 import (
 	"fmt"
-	"net"
 	"net/http"
 	"net/url"
-	"strconv"
 	"strings"
 
 	"github.com/docker/distribution/reference"
@@ -49,27 +47,34 @@ func NewURLBuilderFromString(root string, relative bool) (*URLBuilder, error) {
 // NewURLBuilderFromRequest uses information from an *http.Request to
 // construct the root url.
 func NewURLBuilderFromRequest(r *http.Request, relative bool) *URLBuilder {
-	var scheme string
+	var (
-
-	forwardedProto := r.Header.Get("X-Forwarded-Proto")
-	// TODO: log the error
-	forwardedHeader, _, _ := parseForwardedHeader(r.Header.Get("Forwarded"))
-
-	switch {
-	case len(forwardedProto) > 0:
-		scheme = forwardedProto
-	case len(forwardedHeader["proto"]) > 0:
-		scheme = forwardedHeader["proto"]
-	case r.TLS != nil:
-		scheme = "https"
-	case len(r.URL.Scheme) > 0:
-		scheme = r.URL.Scheme
-	default:
 		scheme = "http"
+		host   = r.Host
+	)
+
+	if r.TLS != nil {
+		scheme = "https"
+	} else if len(r.URL.Scheme) > 0 {
+		scheme = r.URL.Scheme
 	}
 
-	host := r.Host
+	// Handle fowarded headers
-
+	// Prefer "Forwarded" header as defined by rfc7239 if given
+	// see https://tools.ietf.org/html/rfc7239
+	if forwarded := r.Header.Get("Forwarded"); len(forwarded) > 0 {
+		forwardedHeader, _, err := parseForwardedHeader(forwarded)
+		if err == nil {
+			if fproto := forwardedHeader["proto"]; len(fproto) > 0 {
+				scheme = fproto
+			}
+			if fhost := forwardedHeader["host"]; len(fhost) > 0 {
+				host = fhost
+			}
+		}
+	} else {
+		if forwardedProto := r.Header.Get("X-Forwarded-Proto"); len(forwardedProto) > 0 {
+			scheme = forwardedProto
+		}
 		if forwardedHost := r.Header.Get("X-Forwarded-Host"); len(forwardedHost) > 0 {
 			// According to the Apache mod_proxy docs, X-Forwarded-Host can be a
 			// comma-separated list of hosts, to which each proxy appends the
@@ -77,38 +82,7 @@ func NewURLBuilderFromRequest(r *http.Request, relative bool) *URLBuilder {
 			// list.
 			hosts := strings.SplitN(forwardedHost, ",", 2)
 			host = strings.TrimSpace(hosts[0])
-	} else if addr, exists := forwardedHeader["for"]; exists {
-		host = addr
-	} else if h, exists := forwardedHeader["host"]; exists {
-		host = h
 		}
-
-	portLessHost, port := host, ""
-	if !isIPv6Address(portLessHost) {
-		// with go 1.6, this would treat the last part of IPv6 address as a port
-		portLessHost, port, _ = net.SplitHostPort(host)
-	}
-	if forwardedPort := r.Header.Get("X-Forwarded-Port"); len(port) == 0 && len(forwardedPort) > 0 {
-		ports := strings.SplitN(forwardedPort, ",", 2)
-		forwardedPort = strings.TrimSpace(ports[0])
-		if _, err := strconv.ParseInt(forwardedPort, 10, 32); err == nil {
-			port = forwardedPort
-		}
-	}
-
-	if len(portLessHost) > 0 {
-		host = portLessHost
-	}
-	if len(port) > 0 {
-		// remove enclosing brackets of ipv6 address otherwise they will be duplicated
-		if len(host) > 1 && host[0] == '[' && host[len(host)-1] == ']' {
-			host = host[1 : len(host)-1]
-		}
-		// JoinHostPort properly encloses ipv6 addresses in square brackets
-		host = net.JoinHostPort(host, port)
-	} else if isIPv6Address(host) && host[0] != '[' {
-		// ipv6 needs to be enclosed in square brackets in urls
-		host = "[" + host + "]"
 	}
 
 	basePath := routeDescriptorsMap[RouteNameBase].Path
@@ -290,28 +264,3 @@ func appendValues(u string, values ...url.Values) string {
 
 	return appendValuesURL(up, values...).String()
 }
-
-// isIPv6Address returns true if given string is a valid IPv6 address. No port is allowed. The address may be
-// enclosed in square brackets.
-func isIPv6Address(host string) bool {
-	if len(host) > 1 && host[0] == '[' && host[len(host)-1] == ']' {
-		host = host[1 : len(host)-1]
-	}
-	// The IPv6 scoped addressing zone identifier starts after the last percent sign.
-	if i := strings.LastIndexByte(host, '%'); i > 0 {
-		host = host[:i]
-	}
-	ip := net.ParseIP(host)
-	if ip == nil {
-		return false
-	}
-	if ip.To16() == nil {
-		return false
-	}
-	if ip.To4() == nil {
-		return true
-	}
-	// dot can be present in ipv4-mapped address, it needs to come after a colon though
-	i := strings.IndexAny(host, ":.")
-	return i >= 0 && host[i] == ':'
-}

registry/api/v2/urls_test.go

@@ -207,7 +207,7 @@ func TestBuilderFromRequest(t *testing.T) {
 		{
 			name: "https protocol forwarded with a non-standard header",
 			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
-				"X-Forwarded-Proto": []string{"https"},
+				"X-Custom-Forwarded-Proto": []string{"https"},
 			}},
 			base: "http://example.com",
 		},
@@ -253,6 +253,7 @@ func TestBuilderFromRequest(t *testing.T) {
 		{
 			name: "forwarded port with a non-standard header",
 			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
+				"X-Forwarded-Host": []string{"example.com:5000"},
 				"X-Forwarded-Port": []string{"5000"},
 			}},
 			base: "http://example.com:5000",
@@ -262,16 +263,33 @@ func TestBuilderFromRequest(t *testing.T) {
 			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
 				"X-Forwarded-Port": []string{"443 , 5001"},
 			}},
-			base: "http://example.com:443",
+			base: "http://example.com",
+		},
+		{
+			name: "forwarded standard port with non-standard headers",
+			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
+				"X-Forwarded-Proto": []string{"https"},
+				"X-Forwarded-Host":  []string{"example.com"},
+				"X-Forwarded-Port":  []string{"443"},
+			}},
+			base: "https://example.com",
+		},
+		{
+			name: "forwarded standard port with non-standard headers and explicit port",
+			request: &http.Request{URL: u, Host: u.Host + ":443", Header: http.Header{
+				"X-Forwarded-Proto": []string{"https"},
+				"X-Forwarded-Host":  []string{u.Host + ":443"},
+				"X-Forwarded-Port":  []string{"443"},
+			}},
+			base: "https://example.com:443",
 		},
 		{
 			name: "several non-standard headers",
 			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
 				"X-Forwarded-Proto": []string{"https"},
-				"X-Forwarded-Host":  []string{" first.example.com "},
+				"X-Forwarded-Host":  []string{" first.example.com:12345 "},
-				"X-Forwarded-Port":  []string{" 12345 \t"},
 			}},
-			base: "http://first.example.com:12345",
+			base: "https://first.example.com:12345",
 		},
 		{
 			name: "forwarded host with port supplied takes priority",
@@ -292,16 +310,16 @@ func TestBuilderFromRequest(t *testing.T) {
 		{
 			name: "forwarded protocol and addr using standard header",
 			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
-				"Forwarded": []string{`proto=https;for="192.168.22.30:80"`},
+				"Forwarded": []string{`proto=https;host="192.168.22.30:80"`},
 			}},
 			base: "https://192.168.22.30:80",
 		},
 		{
-			name: "forwarded addr takes priority over host",
+			name: "forwarded host takes priority over for",
 			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
-				"Forwarded": []string{`host=reg.example.com;for="192.168.22.30:5000"`},
+				"Forwarded": []string{`host="reg.example.com:5000";for="192.168.22.30"`},
 			}},
-			base: "http://192.168.22.30:5000",
+			base: "http://reg.example.com:5000",
 		},
 		{
 			name: "forwarded host and protocol using standard header",
@@ -320,73 +338,26 @@ func TestBuilderFromRequest(t *testing.T) {
 		{
 			name: "process just the first list element of standard header",
 			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
-				"Forwarded": []string{`for="reg.example.com:443";proto=https, for="reg.example.com:80";proto=http`},
+				"Forwarded": []string{`host="reg.example.com:443";proto=https, host="reg.example.com:80";proto=http`},
 			}},
 			base: "https://reg.example.com:443",
 		},
 		{
-			name: "IPv6 address override port",
+			name: "IPv6 address use host",
 			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
-				"Forwarded":        []string{`for="2607:f0d0:1002:51::4"`},
+				"Forwarded":        []string{`for="2607:f0d0:1002:51::4";host="[2607:f0d0:1002:51::4]:5001"`},
-				"X-Forwarded-Port": []string{"5001"},
+				"X-Forwarded-Port": []string{"5002"},
 			}},
 			base: "http://[2607:f0d0:1002:51::4]:5001",
 		},
 		{
 			name: "IPv6 address with port",
 			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
-				"Forwarded":        []string{`for="[2607:f0d0:1002:51::4]:4000"`},
+				"Forwarded":        []string{`host="[2607:f0d0:1002:51::4]:4000"`},
 				"X-Forwarded-Port": []string{"5001"},
 			}},
 			base: "http://[2607:f0d0:1002:51::4]:4000",
 		},
-		{
-			name: "IPv6 long address override port",
-			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
-				"Forwarded":        []string{`for="2607:f0d0:1002:0051:0000:0000:0000:0004"`},
-				"X-Forwarded-Port": []string{"5001"},
-			}},
-			base: "http://[2607:f0d0:1002:0051:0000:0000:0000:0004]:5001",
-		},
-		{
-			name: "IPv6 long address enclosed in brackets - be benevolent",
-			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
-				"Forwarded":        []string{`for="[2607:f0d0:1002:0051:0000:0000:0000:0004]"`},
-				"X-Forwarded-Port": []string{"5001"},
-			}},
-			base: "http://[2607:f0d0:1002:0051:0000:0000:0000:0004]:5001",
-		},
-		{
-			name: "IPv6 long address with port",
-			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
-				"Forwarded":        []string{`for="[2607:f0d0:1002:0051:0000:0000:0000:0004]:4321"`},
-				"X-Forwarded-Port": []string{"5001"},
-			}},
-			base: "http://[2607:f0d0:1002:0051:0000:0000:0000:0004]:4321",
-		},
-		{
-			name: "IPv6 address with zone ID",
-			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
-				"Forwarded":        []string{`for="fe80::bd0f:a8bc:6480:238b%11"`},
-				"X-Forwarded-Port": []string{"5001"},
-			}},
-			base: "http://[fe80::bd0f:a8bc:6480:238b%2511]:5001",
-		},
-		{
-			name: "IPv6 address with zone ID and port",
-			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
-				"Forwarded":        []string{`for="[fe80::bd0f:a8bc:6480:238b%eth0]:12345"`},
-				"X-Forwarded-Port": []string{"5001"},
-			}},
-			base: "http://[fe80::bd0f:a8bc:6480:238b%25eth0]:12345",
-		},
-		{
-			name: "IPv6 address without port",
-			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
-				"Forwarded": []string{`for="::FFFF:129.144.52.38"`},
-			}},
-			base: "http://[::FFFF:129.144.52.38]",
-		},
 		{
 			name: "non-standard and standard forward headers",
 			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
@@ -398,14 +369,34 @@ func TestBuilderFromRequest(t *testing.T) {
 			base: "https://first.example.com",
 		},
 		{
-			name: "non-standard headers take precedence over standard one",
+			name: "standard header takes precedence over non-standard headers",
 			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
 				"X-Forwarded-Proto": []string{`http`},
 				"Forwarded":         []string{`host=second.example.com; proto=https`},
 				"X-Forwarded-Host":  []string{`first.example.com`},
 				"X-Forwarded-Port":  []string{`4000`},
 			}},
-			base: "http://first.example.com:4000",
+			base: "https://second.example.com",
+		},
+		{
+			name: "incomplete standard header uses default",
+			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
+				"X-Forwarded-Proto": []string{`https`},
+				"Forwarded":         []string{`for=127.0.0.1`},
+				"X-Forwarded-Host":  []string{`first.example.com`},
+				"X-Forwarded-Port":  []string{`4000`},
+			}},
+			base: "http://" + u.Host,
+		},
+		{
+			name: "standard with just proto",
+			request: &http.Request{URL: u, Host: u.Host, Header: http.Header{
+				"X-Forwarded-Proto": []string{`https`},
+				"Forwarded":         []string{`proto=https`},
+				"X-Forwarded-Host":  []string{`first.example.com`},
+				"X-Forwarded-Port":  []string{`4000`},
+			}},
+			base: "https://" + u.Host,
 		},
 	}
 
@@ -428,24 +419,10 @@ func TestBuilderFromRequest(t *testing.T) {
 					continue
 				}
 
-				var expectedURL string
+				expectedURL := testCase.expectedPath
-				proto, ok := tr.request.Header["X-Forwarded-Proto"]
-				if !ok {
-					expectedURL = testCase.expectedPath
 				if !relative {
 					expectedURL = tr.base + expectedURL
 				}
-				} else {
-					urlBase, err := url.Parse(tr.base)
-					if err != nil {
-						t.Fatal(err)
-					}
-					urlBase.Scheme = proto[0]
-					expectedURL = testCase.expectedPath
-					if !relative {
-						expectedURL = urlBase.String() + expectedURL
-					}
-				}
 
 				if buildURL != expectedURL {
 					t.Errorf("[relative=%t, request=%q, case=%q]: %q != %q", relative, tr.name, testCase.description, buildURL, expectedURL)
@@ -541,119 +518,3 @@ func TestBuilderFromRequestWithPrefix(t *testing.T) {
 		}
 	}
 }
-
-func TestIsIPv6Address(t *testing.T) {
-	for _, tc := range []struct {
-		name    string
-		address string
-		isIPv6  bool
-	}{
-		{
-			name:    "IPv6 short address",
-			address: `2607:f0d0:1002:51::4`,
-			isIPv6:  true,
-		},
-		{
-			name:    "IPv6 short address enclosed in brackets",
-			address: "[2607:f0d0:1002:51::4]",
-			isIPv6:  true,
-		},
-		{
-			name:    "IPv6 address",
-			address: `2607:f0d0:1002:0051:0000:0000:0000:0004`,
-			isIPv6:  true,
-		},
-		{
-			name:    "IPv6 address with numeric zone ID",
-			address: `fe80::bd0f:a8bc:6480:238b%11`,
-			isIPv6:  true,
-		},
-		{
-			name:    "IPv6 address with device name as zone ID",
-			address: `fe80::bd0f:a8bc:6480:238b%eth0`,
-			isIPv6:  true,
-		},
-		{
-			name:    "IPv6 address with device name as zone ID enclosed in brackets",
-			address: `[fe80::bd0f:a8bc:6480:238b%eth0]`,
-			isIPv6:  true,
-		},
-		{
-			name:    "IPv4-mapped address",
-			address: "::FFFF:129.144.52.38",
-			isIPv6:  true,
-		},
-		{
-			name:    "localhost",
-			address: "::1",
-			isIPv6:  true,
-		},
-		{
-			name:    "localhost",
-			address: "::1",
-			isIPv6:  true,
-		},
-		{
-			name:    "long localhost address",
-			address: "0:0:0:0:0:0:0:1",
-			isIPv6:  true,
-		},
-		{
-			name:    "IPv6 long address with port",
-			address: "[2607:f0d0:1002:0051:0000:0000:0000:0004]:4321",
-			isIPv6:  false,
-		},
-		{
-			name:    "too many groups",
-			address: "2607:f0d0:1002:0051:0000:0000:0000:0004:4321",
-			isIPv6:  false,
-		},
-		{
-			name:    "square brackets don't make an IPv6 address",
-			address: "[2607:f0d0]",
-			isIPv6:  false,
-		},
-		{
-			name:    "require two consecutive colons in localhost",
-			address: ":1",
-			isIPv6:  false,
-		},
-		{
-			name:    "more then 4 hexadecimal digits",
-			address: "2607:f0d0b:1002:0051:0000:0000:0000:0004",
-			isIPv6:  false,
-		},
-		{
-			name:    "too short address",
-			address: `2607:f0d0:1002:0000:0000:0000:0004`,
-			isIPv6:  false,
-		},
-		{
-			name:    "IPv4 address",
-			address: `192.168.100.1`,
-			isIPv6:  false,
-		},
-		{
-			name:    "unclosed bracket",
-			address: `[2607:f0d0:1002:0051:0000:0000:0000:0004`,
-			isIPv6:  false,
-		},
-		{
-			name:    "trailing bracket",
-			address: `2607:f0d0:1002:0051:0000:0000:0000:0004]`,
-			isIPv6:  false,
-		},
-		{
-			name:    "domain name",
-			address: `localhost`,
-			isIPv6:  false,
-		},
-	} {
-		isIPv6 := isIPv6Address(tc.address)
-		if isIPv6 && !tc.isIPv6 {
-			t.Errorf("[%s] address %q falsely detected as IPv6 address", tc.name, tc.address)
-		} else if !isIPv6 && tc.isIPv6 {
-			t.Errorf("[%s] address %q not recognized as IPv6", tc.name, tc.address)
-		}
-	}
-}