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.

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

Location header https rewrite is too broad #5807

Closed
keesverruijt opened this issue Nov 11, 2019 · 10 comments
Closed

Location header https rewrite is too broad #5807

keesverruijt opened this issue Nov 11, 2019 · 10 comments
Labels
area/middleware kind/bug/confirmed a confirmed bug (reproducible). priority/P2 need to be fixed in the future status/5-frozen-due-to-age
Milestone

Comments

@keesverruijt
Copy link

keesverruijt commented Nov 11, 2019

Bug

Traefik change #5574 rewrites HTTP to HTTPS whenever it finds an HTTP scheme and the Traefik is running in SSL mode. This is too broad, it should only apply this when the URL is under the control of the Traefik server and contains no port number.

What did you do?

Backend returned an HTTP 302 redirect to Traefik with a Location header that refers to an external website that is available only on HTTP, not HTTPS, and Traefik is running in SSL mode.

What did you expect to see?

Unchanged Location header, as the URL is outside the span of control of my Traefik server.

What did you see instead?

Traefik change #5574 makes it rewrite the Location header if Traefik is running in SSL mode and the URL in the header is not HTTPS.

What this was supposed to fix

The change was meant to reduce the number of redirects so that the HTTP client immediately receives an HTTPS location.

For example, if the Location header contains

Location: http://traefik-server/path

it correctly rewrites this to

Location: https://traefik-server/path

What the change broke as collateral damage

If the Location header contains a port, it should not do the rewrite (or know which port to substitute.) For example, the following should not be rewritten:

Location: http://traefik-server:8080/path

It should also not rewrite the header if the URL refers to a different server altogether:

Location: http://other.company.site/path

as there may not be an https server at other.company.site.

Output of traefik version: (What version of Traefik are you using?)

Version:      2.0.4
Codename:     montdor
Go version:   go1.13.3
Built:        2019-10-28T20:23:57Z
OS/Arch:      linux/amd64
@keesverruijt
Copy link
Author

Maybe the simplest solution is to revert #5574 and accept any extra http->https redirects.

@traefiker
Copy link
Contributor

Hi! I'm Træfiker 🤖 the bot in charge of communication regulation.

Thanks for your interest in Traefik!

Issue templates help us help you by providing all necessary information.

Please edit your issue and use the available templates:

And remember: each time someone ignores the template, a cute little bunny dies.

#SaveTheCuteBunny ❤️ 🐰 ❤️

southpark - estrella kill rabbit

southpark - estrella kill rabbit2

@Ullaakut
Copy link
Contributor

Hi @keesverruijt !

We're not sure what you mean 🤔 Do you mean that when a port is specified, the HTTP->HTTPS rewrite should NOT be done? Or that it should and it isn't?

Thanks in advance :)

@ldez ldez added kind/bug/possible a possible bug that needs analysis before it is confirmed or fixed. and removed status/0-needs-triage labels Nov 12, 2019
@dtomcej
Copy link
Contributor

dtomcej commented Nov 12, 2019

@keesverruijt I have opened issue unrolled/secure#61 for this issue, and will submit a PR shortly.

Once merged, we will update the dependency here, and will link back to this issue.

@keesverruijt
Copy link
Author

@Ullaakut yes, when a port is specified the rewrite should not be done. @dtomcej has understood my points correctly in his issue unrolled/secure#61.

@dtomcej dtomcej added area/middleware kind/bug/confirmed a confirmed bug (reproducible). priority/P2 need to be fixed in the future and removed contributor/need-more-information kind/bug/possible a possible bug that needs analysis before it is confirmed or fixed. labels Nov 13, 2019
@dduportal dduportal added this to issues in v2 via automation Nov 13, 2019
@dduportal dduportal removed this from issues in v2 Nov 13, 2019
@traefiker traefiker added this to the 2.0 milestone Nov 15, 2019
@traefiker
Copy link
Contributor

Closed by #5835.

@traefiker traefiker removed this from the 2.0 milestone Nov 18, 2019
@traefiker
Copy link
Contributor

Closed by #5857.

@traefiker traefiker added this to the 1.7 milestone Nov 18, 2019
@mvrhov
Copy link

mvrhov commented Dec 10, 2019

Can we get a new release of 1.7.. I've wasted almost 2 days because of this bug.

@dduportal
Copy link
Contributor

Hi @mvrhov , you might want to look at #5989

@mvrhov
Copy link

mvrhov commented Dec 11, 2019

Thanks.

@traefik traefik locked and limited conversation to collaborators Jan 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/middleware kind/bug/confirmed a confirmed bug (reproducible). priority/P2 need to be fixed in the future status/5-frozen-due-to-age
Projects
None yet
Development

No branches or pull requests

7 participants