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

Fix HSTS header over non tls ingresses #481

Merged
merged 6 commits into from
Jan 28, 2019

Conversation

Dean-Coakley
Copy link
Contributor

Fixes: #404

Proposed changes

  • HSTS headers are no longer added to responses over HTTP
  • Added HSTSBehindProxy annotation and configmap to allow HSTS headers to be sent over HTTP. This is useful for the use case of TLS termination being done in a proxy deployed in front of the ingress controller

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

* Add HSTSBehindProxy annotation
* Add HSTSBehindProxy configmap entry
* Update ConfigMap/Annotations docs
* Update OSS/Plus ingress templates
* HSTS headers now added on redirects over https
@Dean-Coakley Dean-Coakley added the bug An issue reporting a potential bug label Jan 23, 2019
@Dean-Coakley Dean-Coakley self-assigned this Jan 23, 2019
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Found a bug. Otherwise looks good!

{{- if $server.RedirectToHTTPS}}
if ($http_x_forwarded_proto = 'http') {
return 301 https://$host$request_uri;
}
{{- end}}

{{- if $server.SSLRedirect}}
Copy link
Contributor

Choose a reason for hiding this comment

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

the SSL redirect part here must go into the following block

	{{if $server.SSL}}
	{{if not $server.GRPCOnly}}
          <here, after hsts stuff>
        {{end}}
        {{end}}

Otherwise it is a BUG, because the redirect will be enabled for Ingress resources without any TLS termination.

Please double check that for an Ingress resource without TLS termination, no SSL redirect is generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

same for nginx oss template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 nice catch

@Dean-Coakley Dean-Coakley merged commit 07fdfe3 into master Jan 28, 2019
@Dean-Coakley Dean-Coakley deleted the fix/hsts-non-tls-ingresses branch January 28, 2019 10:05
Dean-Coakley added a commit that referenced this pull request Jan 31, 2019
nginx-controller binary was mistakenly added in #481
This PR removes the binary again
Dean-Coakley added a commit that referenced this pull request Jan 31, 2019
nginx-controller binary was mistakenly added in #481
This PR removes the binary again
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a potential bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants