-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Use forwarded Host header without any changes #14319
Conversation
[test] |
Evaluated for origin test up to f3340fc |
pkg/util/httprequest/httprequest.go
Outdated
@@ -100,8 +85,6 @@ func SchemeHost(req *http.Request) (string /*scheme*/, string /*host*/) { | |||
scheme = "https" | |||
case len(req.URL.Scheme) > 0: | |||
scheme = req.URL.Scheme | |||
case port == "443": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing this is wrong... it means we can redirect to http when the request is expecting https
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which reverse proxy sends X-Forwarded-Port without X-Forwarded-Proto? Guessing the protocol by the port is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're guessing it either way at this point. in the absence of proto info, using https for port 443 is more sane
pkg/util/httprequest/httprequest.go
Outdated
@@ -68,14 +66,6 @@ func SchemeHost(req *http.Request) (string /*scheme*/, string /*host*/) { | |||
|
|||
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing this is just as likely to cause problems as it is to fix them. we should only omit the port when it is the default port for the chosen scheme (80 for http, 443 for https), and even then, we don't know whether the original request explicitly set the port or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As X-Forwarded-Host has no standard at all I use RFC 7239. It says that Forwarded host should have the original value of the "Host" header field
. So if we have this header, then we know. Do you know any real reverse proxy that requires this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is amended. Now X-Forwarded-Port
has priority over the port in the Host header. But if there is no port in the Host header and X-Forwarded-Port is a default port, then the port is not added to the Host header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if there is no port in the Host header and X-Forwarded-Port is a default port, then the port is not added to the Host header.
It should only be excluded if it is a default port that matches the scheme (80 && http, 443 && https)
@@ -25,20 +25,7 @@ func TestSchemeHost(t *testing.T) { | |||
}, | |||
}, | |||
expectedScheme: "https", | |||
expectedHost: "example.com:443", | |||
}, | |||
"X-Forwarded-Port overwrites X-Forwarded-Host port": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't remove this test case... we should not ignore an explicitly propagated port
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know which proxy can behave like this? For me it sounds like a terribly broken configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can change the title to "broken configuration"
and change the expected values instead of deleting the test.
@@ -70,14 +57,6 @@ func TestSchemeHost(t *testing.T) { | |||
expectedScheme: "http", | |||
expectedHost: "example.com:80", | |||
}, | |||
"req host with tls port": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
restore this test
@@ -156,7 +135,7 @@ func TestSchemeHost(t *testing.T) { | |||
}, | |||
}, | |||
expectedScheme: "http", | |||
expectedHost: "route-namespace.router.default.svc.cluster.local:80", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
@@ -171,6 +150,21 @@ func TestSchemeHost(t *testing.T) { | |||
}, | |||
}, | |||
expectedScheme: "https", | |||
expectedHost: "route-namespace.router.default.svc.cluster.local", |
There was a problem hiding this comment.
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?
@@ -171,6 +150,21 @@ func TestSchemeHost(t *testing.T) { | |||
}, | |||
}, | |||
expectedScheme: "https", | |||
expectedHost: "route-namespace.router.default.svc.cluster.local", | |||
}, | |||
"haproxy edge terminated route -> svc -> non-tls pod with the explicit port": { |
There was a problem hiding this comment.
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?
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1712/) (Base Commit: 16e8fe4) |
@miminar FYI can you have a look? |
The changes seem reasonable to me. The upstream reverted support for As far as I know, we had a problem with our router that used to strip custom port from the I'd like to spin up a fork_ami and get this tested behind both ELB, haproxy and f5 routers/balancers. |
pkg/util/httprequest/httprequest.go
Outdated
@@ -87,11 +77,6 @@ func SchemeHost(req *http.Request) (string /*scheme*/, string /*host*/) { | |||
host = req.URL.Host | |||
} | |||
|
|||
port := "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
restore this
dropping support for X-Forwarded-Port is ok with me if we can prove we preserve the user's request when accessing via the haproxy router, for both http and https protocols, with both implicit and explicit ports. |
#14319 (comment) stands...
|
pkg/util/httprequest/httprequest.go
Outdated
default: | ||
scheme = "http" | ||
// If X-Forwarded-Port is sent, use the explicit port info | ||
if len(forwardedPort) > 0 && (len(port) != 0 || len(scheme) == 0 && forwardedPort != "80" && forwardedPort != "443") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the scheme used to decide whether to use the port info? this is a confusing combination of conditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To support this weird configuration:
X-Forwarded-Host: example.com
X-Forwarded-Port: 80
X-Forwarded-Proto: https
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with that configuration, this logic leaves the port as "", which would redirect to https://example.com
, which implies port 443, which is wrong... we should send https://example.com:80
in that case.
stepping back, can we just remember whether the original host included an explicit port, and if it did not, insert a final step that strips default ports that match the scheme? I think all the existing logic can stay as-is in that case.
host := ""
hostHadExplicitPort := false
switch {
case len(forwardedHost) > 0:
host = forwardedHost
// remember whether the X-Forwarded-Host header had an explicit port
hostHadExplicitPort = hasExplicitPort(forwarded("Host"))
case len(req.Host) > 0:
host = req.Host
hostHadExplicitPort = hasExplicitPort(host)
case len(req.URL.Host) > 0:
host = req.URL.Host
hostHadExplicitPort = hasExplicitPort(host)
}
...
// Do not include a port if the original host did not explicitly set it,
// and the port is a default port that matches the scheme
if !hostHadExplicitPort {
if (scheme == "https" && port == "443") || ("scheme" == "http" && port == "80") {
if hostWithoutPort, _, err := net.SplitHostPort(host); err == nil {
host = hostWithoutPort
}
}
}
return scheme, host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, amended this way.
pkg/util/httprequest/httprequest.go
Outdated
} | ||
forwardedPort := forwarded("Port") | ||
|
||
scheme := "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to set the scheme in a single block, rather than setting it here, then using the scheme to determine the port, then using the port to determine the scheme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me too, but you want determine the scheme by port, and I want to not add the port if it's a default port for the scheme.
pkg/util/httprequest/httprequest.go
Outdated
@@ -106,5 +112,13 @@ func SchemeHost(req *http.Request) (string /*scheme*/, string /*host*/) { | |||
scheme = "http" | |||
} | |||
|
|||
if !hostHadExplicitPort { | |||
if scheme == "https" && port == "443" || scheme == "http" && port == "80" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add parens for clarity
pkg/util/httprequest/httprequest.go
Outdated
// 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 { | ||
host = net.JoinHostPort(h, forwardedPort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not great to reset host
from forwardedHost and forwardedPort after the switch block where we set host
from the different sources. would prefer all the logic be contained in the different cases of that block.
hasExplicitHost := func(h string) bool {
_, _, err := net.SplitHostPort(h)
return err == nil
}
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)
}
/test extended_image_registry |
Extended registry tests seem to be broken now because of #15763 |
/test extended_templates |
would like to see this merged... there are two outstanding comments (https://github.com/openshift/origin/pull/14319/files#r130969215 and https://github.com/openshift/origin/pull/14319/files#r130962641) and then it LGTM |
Fixes bug 1439614
@liggitt PTAL, I've addressed these comments. |
@dmage: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/test cmd |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dmage, liggitt The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue (batch tested with PRs 16052, 16105, 14319, 16106, 16104) |
I am still experiencing this issue. Openshift is appending a port number to my route when I click on it to open it in the browser. Manually removing the port solves the problem. Followed the trail of issues regarding the port stripping or leaving it alone and this issue seems to be the latest one. Version: |
The Docker Server is sensitive to the presence of the port in a URL. If Docker Server is authorised to https://example.com/ and it is redirected to https://example.com:443, it might loose the Authorization header. So we should not remove nor add the port to the Host header.
Fixes bug 1439614