From d4c3e8842602e0192657fcf05f1804fbfc7f54fd Mon Sep 17 00:00:00 2001 From: Troels Thomsen Date: Mon, 20 Mar 2017 16:10:36 -0700 Subject: [PATCH 1/2] Add test for precendence with standard port Signed-off-by: Troels Thomsen Signed-off-by: Derek McGowan (github: dmcgowan) --- registry/api/v2/urls_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/registry/api/v2/urls_test.go b/registry/api/v2/urls_test.go index 6449c6e4..dae8354b 100644 --- a/registry/api/v2/urls_test.go +++ b/registry/api/v2/urls_test.go @@ -264,6 +264,22 @@ func TestBuilderFromRequest(t *testing.T) { }}, base: "http://example.com:443", }, + { + 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-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-Port": []string{"443"}, + }}, + base: "https://example.com:443", + }, { name: "several non-standard headers", request: &http.Request{URL: u, Host: u.Host, Header: http.Header{ From 81a47d976695238dc4b281e201e81fae3785861a Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Mon, 20 Mar 2017 16:13:33 -0700 Subject: [PATCH 2/2] Remove support for X-Forwarded-Port Partially reverts change adding support for X-Forwarded-Port. Changes the logic to prefer the standard Forwarded header over X-Forwarded headers. Prefer forwarded "host" over "for" since "for" represents the client and not the client's request. Signed-off-by: Derek McGowan (github: dmcgowan) --- registry/api/v2/urls.go | 113 +++++------------ registry/api/v2/urls_test.go | 237 ++++++----------------------------- 2 files changed, 72 insertions(+), 278 deletions(-) diff --git a/registry/api/v2/urls.go b/registry/api/v2/urls.go index 3eb3e09d..1337bdb1 100644 --- a/registry/api/v2/urls.go +++ b/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,66 +47,42 @@ 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 - - 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: + var ( 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 - - 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 - // requested host. We want to grab the first from this comma-separated - // 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 + // 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 + } } - } - - 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] + } 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 + // requested host. We want to grab the first from this comma-separated + // list. + hosts := strings.SplitN(forwardedHost, ",", 2) + host = strings.TrimSpace(hosts[0]) } - // 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] == ':' -} diff --git a/registry/api/v2/urls_test.go b/registry/api/v2/urls_test.go index dae8354b..4f854b23 100644 --- a/registry/api/v2/urls_test.go +++ b/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,12 +263,13 @@ 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", @@ -276,6 +278,7 @@ func TestBuilderFromRequest(t *testing.T) { 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", @@ -284,10 +287,9 @@ func TestBuilderFromRequest(t *testing.T) { 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-Port": []string{" 12345 \t"}, + "X-Forwarded-Host": []string{" first.example.com:12345 "}, }}, - base: "http://first.example.com:12345", + base: "https://first.example.com:12345", }, { name: "forwarded host with port supplied takes priority", @@ -308,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", @@ -336,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"`}, - "X-Forwarded-Port": []string{"5001"}, + "Forwarded": []string{`for="2607:f0d0:1002:51::4";host="[2607:f0d0:1002:51::4]: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{ @@ -414,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, }, } @@ -444,23 +419,9 @@ func TestBuilderFromRequest(t *testing.T) { continue } - var expectedURL string - 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 - } + expectedURL := testCase.expectedPath + if !relative { + expectedURL = tr.base + expectedURL } if buildURL != expectedURL { @@ -557,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) - } - } -}