Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use forwarded Host header without any changes #14319

Merged
merged 1 commit into from
Sep 2, 2017
Merged

Use forwarded Host header without any changes #14319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 28 additions & 9 deletions pkg/util/httprequest/httprequest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,32 +66,43 @@ func SchemeHost(req *http.Request) (string /*scheme*/, string /*host*/) {
return strings.TrimSpace(value)
}

forwardedProto := forwarded("Proto")
forwardedHost := forwarded("Host")
// If both X-Forwarded-Host and X-Forwarded-Port are sent, use the explicit port info
if forwardedPort := forwarded("Port"); len(forwardedHost) > 0 && len(forwardedPort) > 0 {
if h, _, err := net.SplitHostPort(forwardedHost); err == nil {
forwardedHost = net.JoinHostPort(h, forwardedPort)
} else {
forwardedHost = net.JoinHostPort(forwardedHost, forwardedPort)
}
hasExplicitHost := func(h string) bool {
_, _, err := net.SplitHostPort(h)
return err == nil
}

forwardedHost := forwarded("Host")
host := ""
hostHadExplicitPort := false
switch {
case len(forwardedHost) > 0:
host = forwardedHost
hostHadExplicitPort = hasExplicitHost(host)

// If both X-Forwarded-Host and X-Forwarded-Port are sent, use the explicit port info
if forwardedPort := forwarded("Port"); len(forwardedPort) > 0 {
if h, _, err := net.SplitHostPort(forwardedHost); err == nil {
host = net.JoinHostPort(h, forwardedPort)
} else {
host = net.JoinHostPort(forwardedHost, forwardedPort)
}
}

case len(req.Host) > 0:
host = req.Host
hostHadExplicitPort = hasExplicitHost(host)

case len(req.URL.Host) > 0:
host = req.URL.Host
hostHadExplicitPort = hasExplicitHost(host)
}

port := ""
if _, p, err := net.SplitHostPort(host); err == nil {
port = p
}

forwardedProto := forwarded("Proto")
scheme := ""
switch {
case len(forwardedProto) > 0:
Expand All @@ -106,5 +117,13 @@ func SchemeHost(req *http.Request) (string /*scheme*/, string /*host*/) {
scheme = "http"
}

if !hostHadExplicitPort {
if (scheme == "https" && port == "443") || (scheme == "http" && port == "80") {
if hostWithoutPort, _, err := net.SplitHostPort(host); err == nil {
host = hostWithoutPort
}
}
}

return scheme, host
}
46 changes: 43 additions & 3 deletions pkg/util/httprequest/httprequest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func TestSchemeHost(t *testing.T) {
},
},
expectedScheme: "https",
expectedHost: "example.com:443",
expectedHost: "example.com",
},
"X-Forwarded-Port overwrites X-Forwarded-Host port": {
req: &http.Request{
Expand All @@ -51,7 +51,32 @@ func TestSchemeHost(t *testing.T) {
},
},
expectedScheme: "https",
expectedHost: "example.com:443",
expectedHost: "example.com",
},
"stripped X-Forwarded-Host and X-Forwarded-Port with non-standard port": {
req: &http.Request{
URL: &url.URL{Path: "/"},
Host: "127.0.0.1",
Header: http.Header{
"X-Forwarded-Host": []string{"example.com"},
"X-Forwarded-Port": []string{"80"},
"X-Forwarded-Proto": []string{"https"},
},
},
expectedScheme: "https",
expectedHost: "example.com:80",
},
"detect scheme from X-Forwarded-Port": {
req: &http.Request{
URL: &url.URL{Path: "/"},
Host: "127.0.0.1",
Header: http.Header{
"X-Forwarded-Host": []string{"example.com"},
"X-Forwarded-Port": []string{"443"},
},
},
expectedScheme: "https",
expectedHost: "example.com",
},

"req host": {
Expand Down Expand Up @@ -156,7 +181,7 @@ func TestSchemeHost(t *testing.T) {
},
},
expectedScheme: "http",
expectedHost: "route-namespace.router.default.svc.cluster.local:80",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restore this... this example was captured from live data and expects the explicit port to work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change the input, it still the same as it was captured. Why this example expects the explicit port? It's the reason of the bug with the Docker Server.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on @miminar's explanation, this was likely captured while our router had the port-stripping bug. Can you set up a pod and access it through the current haproxy router and verify what X-Forwarded-Host is set to when accessing http://route, http://route:80, https://route and https://route:443?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

X-Forwarded-Host is set to the hostname without a port, as it written in the test.
If you use docker pull route:80/foo/bar, then it'll be with the port.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make sure we have tests capturing all the headers we see for all four of those cases?

expectedHost: "route-namespace.router.default.svc.cluster.local",
},
"haproxy edge terminated route -> svc -> non-tls pod": {
req: &http.Request{
Expand All @@ -171,6 +196,21 @@ func TestSchemeHost(t *testing.T) {
},
},
expectedScheme: "https",
expectedHost: "route-namespace.router.default.svc.cluster.local",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you capture this via an actual request to haproxy and verify haproxy differentiates between an explicit port and an implicit one in the headers it sets?

},
"haproxy edge terminated route -> svc -> non-tls pod with the explicit port": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question for this testcase... was this verified with actual backend capture through haproxy?

req: &http.Request{
URL: &url.URL{Path: "/"},
Host: "route-namespace.router.default.svc.cluster.local:443",
Header: http.Header{
"X-Forwarded-Host": []string{"route-namespace.router.default.svc.cluster.local:443"},
"X-Forwarded-Port": []string{"443"},
"X-Forwarded-Proto": []string{"https"},
"Forwarded": []string{"for=172.18.2.57;host=route-namespace.router.default.svc.cluster.local:443;proto=https"},
"X-Forwarded-For": []string{"172.18.2.57"},
},
},
expectedScheme: "https",
expectedHost: "route-namespace.router.default.svc.cluster.local:443",
},
"haproxy passthrough route -> svc -> tls pod": {
Expand Down