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

Add support for 301 redirect to https #116

Merged
merged 1 commit into from
Feb 17, 2017

Conversation

cwhenderson20
Copy link

Adds the ability to denote via annotation or ConfigMap that all traffic should be over HTTPS

@pleshakov
Copy link
Contributor

@cwhenderson20 Thanks for the PR!

A couple of suggestions:

  • When this option is enabled, I think NGINX should forward the https scheme to the backend in the X-Forwarded-Proto header. Currently it will forward http. What do you think?
  • To be consistent with other boolean options that are disabled by default, it is better not to initialize RedirectToHTTPS with the default false value in the config.go file.
  • Thanks for adding documentation. I think it is better to clarify when this option is useful. I suggest to document this option as Sets the 301 redirect rule based on the value of the http_x_forwarded_proto header on the server block to force incoming traffic to be over HTTPS. Useful when terminating SSL in a load balancer in front of the Ingress controller — see [115](https://github.com/nginxinc/kubernetes-ingress/issues/115)
  • Please squash your commits into one

@cwhenderson20
Copy link
Author

@pleshakov, those are all reasonable changes. I'll take a stab at them and update the PR today

@cwhenderson20
Copy link
Author

cwhenderson20 commented Feb 15, 2017

@pleshakov I pushed the changes. I'm not 100% on issue 2 that you raised. I believe changing $scheme to $http_x_forwarded_proto is what you're looking for, but please let me know if that's not correct.

@pleshakov
Copy link
Contributor

@cwhenderson20
Great!
It's better to have something like
proxy_set_header X-Forwarded-Proto {{ if $server.RedirectToHTTPS}}https{{else}}$scheme{{end}};

NGINX will pass https only when the redirect-to-https is enabled and $scheme when it is not. This will protect against any client who wants to control this header for the default case when the option is disabled.

@cwhenderson20
Copy link
Author

@pleshakov — updated.

Though I'm curious from a technical standpoint...why would $scheme or $http_x_forwarded_proto not accurately represent the real scheme?

I would have guessed that the redirect block would force the browser to re-request the page with the https scheme, thus setting both values to https on the second request. What actually happens?

@pleshakov
Copy link
Contributor

@cwhenderson20

$scheme is determined by NGINX. If a request comes through an SSL connection, the value of $scheme is https. Otherwise it is http.

$http_x_forwarded_proto is determined based on the value of the X-Forwarded-Proto header of a client request.

In case redirect-to-https is enabled, we assume that NGINX is behind another proxy, thus we want to pass to the backend the value of $http_x_forwarded_proto. However, since we do a 301 redirect to https when $http_x_forwarded_proto = http, we will only pass a request to the backend when $http_x_forwarded_proto = https, thus, we can use https in the proxy_set_header directive.

In case redirect-to-https is disabled, we assume that the client connects to NGINX directly, thus we want to pass to the backend the $scheme, ignoring the $http_x_forwarded_proto value. That is why we use $scheme in the proxy_set_header directive.

@cwhenderson20
Copy link
Author

Great, thanks for the explanation! Any more changes needed before merging?

@pleshakov pleshakov merged commit 7d35fed into nginxinc:master Feb 17, 2017
@pleshakov
Copy link
Contributor

@cwhenderson20 Thanks!

@pleshakov pleshakov added this to the v0.8.0 milestone Feb 23, 2017
@pleshakov pleshakov mentioned this pull request Mar 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants