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

Validate x-forwarded-proto and connection scheme before redirect to https #1844

Merged
merged 1 commit into from
Dec 21, 2017

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Dec 21, 2017

Which issue this PR fixes:

fixes #1841
fixes #1779

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 21, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 21, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 36.764% when pulling 97c40f4 on aledbf:fix-redirect into 18a4e63 on kubernetes:master.

@aledbf aledbf merged commit fead908 into kubernetes:master Dec 21, 2017
@tpolekhin
Copy link

Isn't it simpler to do if ($scheme = http) { return 301 } and stop considering x-forwarded-proto at all?

@tpolekhin
Copy link

BTW, if previous hop was https then you get no redirection if you go in with http. still broken

@aledbf
Copy link
Member Author

aledbf commented Dec 21, 2017

Isn't it simpler to do if ($scheme = http) { return 301 } and stop considering x-forwarded-proto at all?

Several users are sending this header so we cannot remove it.

@aledbf
Copy link
Member Author

aledbf commented Dec 21, 2017

BTW, if previous hop was https then you get no redirection if you go in with http. still broken

That can happen only if the previous hop is a load balancer and switches protocols without handling the x-forwarded-* headers.
This also happens if we use if ($scheme = http) { return 301 }
In this scenario the previous hop (load balancer) is the one in charge of doing the redirects.

@aledbf
Copy link
Member Author

aledbf commented Dec 21, 2017

Just to be clear, if the previous hop is a load balancer (like an ELB):

  • In TCP mode this is not an issue
  • In HTTP mode the load balancer is in charge of the TLS termination and the ingress controller never receives a HTTPS request

@tpolekhin
Copy link

I have a feeling that we're not on the same page here...
Let me explain my situation again and why did i get this issue:

This is a request path: app1 -> proxy -> nginx -> app2
Nginx is configured with SSL endpoint for app2 with redirect, so if i do curl http://app2/ i will get 301. app1 cannot do a request to nginx directly because it is behind a proxy, so app1 sends a request to a proxy over HTTP. Then proxy establishes a HTTPS connection to the nginx and sends a request header x-forwarded-proto:http to inform the nginx that app1 is connected to proxy via HTTP. proxy is connected to nginx via HTTPS still. So i don't see a reason why nginx behavior should be any different based on this header.

If we use if ($scheme = http) { return 301 } then we basically redirect HTTP and accept HTTPS.

Now let's look at current code:
$pass_access_scheme = $http_x_forwarded_proto if the header is present

map "$scheme:$pass_access_scheme" $redirect_to_https {
    default          0;
    "http:http"      1;
}

and how it will behave on different scenarios:

Scheme X-Forwarded-Proto Redirect?
http http yes
http https no
https http no
https https no

You can see that you still able to bypass redirect with HTTP request if you set XF HTTPS header

@aledbf
Copy link
Member Author

aledbf commented Dec 21, 2017

@tpolekhin please send a PR with the change you propose.

@aledbf aledbf deleted the fix-redirect branch December 27, 2017 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x-forwarded-proto unexpected behavior Passing header X-Forwarded-Proto:https from client breaks SSL redirect
5 participants