Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

ingress: adds configurable SSL redirect nginx controller #850

Merged

Conversation

simonswine
Copy link
Contributor

Hi everyone,

I want my services behind the ingress to decide if HTTPS is strictly required or not. So I added the feature to disable the 301 redirect to the https:/ URL.

To make this actually work #849 needs to be merged before. (Otherwise you cannot override and default value true with a false)

Cheers,

Christian

@aledbf
Copy link
Contributor

aledbf commented Apr 26, 2016

@simonswine I fixed that issue in #766 (not merged yet) using use-hts=false in the configmap

@simonswine
Copy link
Contributor Author

Hi @aledbf, I missed your work on that. I am not too sure why it's not merged. I guess your PR is rather large and doing a lot of different things.

How and where exactly did you solve the default value overriding problem with bool (default=true) and you want to have it set false via ConfigMap. See fix + tests in #849

@@ -256,6 +259,7 @@ func newDefaultNginxCfg() nginxConfiguration {
SSLBufferSize: sslBufferSize,
SSLCiphers: sslCiphers,
SSLProtocols: sslProtocols,
SSLRedirect: true,

Choose a reason for hiding this comment

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

please update the docs on how we flip this

@bprashanth
Copy link

I'm fine with a smaller change, doens't matter either way how it gets in i guess, as long as we alll agree on how it's expressed. You and @aledbf can choose which one of the 2 prs goes in :)

@aledbf
Copy link
Contributor

aledbf commented May 4, 2016

@simonswine please check the latest version gcr.io/google_containers/nginx-ingress-controller:0.61. Contains #766 and #849. Use hts=false in the nginx configmap.

@simonswine
Copy link
Contributor Author

@aledbf thanks for that update. Thinking a bit about it, I came to the conclusion that we probably want to be able to configure HSTS, and server-side SSL redirect enforcement separately.

For my particular use case I want to be always able to access something via HTTP (if the clients wants to do so). It's needed for doing the http-01 challenge for Let's Encrypt. At the same time I want to be able to enforce HTTPS via HSTS for client that support that. Ultimately I want to be able to do that on location level (see #904).

@aledbf
Copy link
Contributor

aledbf commented May 8, 2016

I came to the conclusion that we probably want to be able to configure HSTS, and server-side SSL redirect enforcement separately.

Yes, you are right about this. How about defining the redirect and hsts in the configuration to define a default value and adding an annotation in the services? This way is possible to just enable things like http-01 challenge and force the behavior with tls rules with existing certificates

@rh-kube-bot
Copy link

Can one of the admins verify this patch?

1 similar comment
@rh-kube-bot
Copy link

Can one of the admins verify this patch?

@simonswine simonswine changed the title Adds configurable SSL redirect to Nginx Ingress Controller ingress: adds configurable SSL redirect nginx controller May 25, 2016
@simonswine
Copy link
Contributor Author

simonswine commented May 25, 2016

  • add global value to config map
  • add per service value as annotation to service object (ingress-nginx.kubernetes.io/ssl-redirect: "false")

@aledbf could please take a look at my changes. They are still a WiP. It felt like it is a bit messy implemented.

Definitely missing are docs

@simonswine simonswine changed the title ingress: adds configurable SSL redirect nginx controller [WiP] ingress: adds configurable SSL redirect nginx controller May 25, 2016
@aledbf
Copy link
Contributor

aledbf commented May 25, 2016

@simonswine looks good 👍
I like the getServiceAnnotations addition. Can you make this public? (some of the pending PR like the auth are located in a different package)

Just the server.SetCfg(cfgCamelCase) before the template is what I don't "like". If the user specifies --v=3 it will log the content of the map before the rendering of the template with the cfg repeated N times (one per server)

@aledbf
Copy link
Contributor

aledbf commented May 25, 2016

@simonswine why not the annotation in the ingress instead of the service?

@simonswine
Copy link
Contributor Author

simonswine commented May 26, 2016

@aledbf

why not the annotation in the ingress instead of the service?

As I want it to be able to config stuff on Location-level. If this has to be done with ingress controller annotations, their key has to contain the path as well. This felt a bit ugly.

SetCfg/cfg in Server struct

It's modified now:

  • the struct field is public
  • we use a pointer instead of a map

GetServiceAnnotations

Is now public

Docs

added

@simonswine simonswine changed the title [WiP] ingress: adds configurable SSL redirect nginx controller ingress: adds configurable SSL redirect nginx controller May 27, 2016
@simonswine
Copy link
Contributor Author

@eparis IMHO now ready to merge

@aledbf
Copy link
Contributor

aledbf commented May 27, 2016

their key has to contain the path as well....

Why, just create a restriction: using this annotation in an ingress rules applies to all the paths
(if I want multiple paths with the same host I just need to create multiple rules with 1 path)

As I want it to be able to config stuff on Location-level....

I know, but if the annotation is in the service and not the ingress rule then:

  • There's two places to indicate a "routing" behavior, ingress rule and service.
  • If the annotation is in the service I cannot create two rules with different behavior using the same service (I need to duplicate the service)

@aledbf
Copy link
Contributor

aledbf commented May 31, 2016

@simonswine can you rebase?
I'm working on #1104 and I would really love to get this merged

@simonswine
Copy link
Contributor Author

@aledbf sorry I haven't managed to get this done before 0.7.

I can see your point in the ingress vs. service resource discussion. It feels cleaner to me now to do it in the ingress. I will modify this accordingly..

* add global value to config map
* add per ingress value as annotation to ingress resources
@simonswine
Copy link
Contributor Author

@aledbf @bprashanth

Refactored the code to use ingress resource annotations. It is now part of rewrite package.

Moved config into own package to prevent dependency cycles in the imports


// SSL enabled protocols to use
// http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_protocols
sslProtocols = "TLSv1 TLSv1.1 TLSv1.2"

Choose a reason for hiding this comment

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

I know this is just cut/paste, but why include tls1 as a default?

Copy link
Contributor

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

I was suggesting an opt in, not default. I'm ok if we really need it, but tls 1 is compromised and people can just force a downgrade and BEAST. Please add a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bprashanth just in case the defaults in sslCiphers and sslProtocols mitigate BEAST

captura de pantalla 2016-06-06 a las 9 20 28 p m

(screenshot from ssllabs.com/ssltest/analyze.html of a site with defaults)

Choose a reason for hiding this comment

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

alright, SG, lets add it to the readme somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bprashanth
Copy link

LGTM but for the nit, thanks for the package cleanup also

@aledbf
Copy link
Contributor

aledbf commented Jun 7, 2016

LGTM

@bprashanth
Copy link

Spinoff pr for docs sounds good, merging

@bprashanth bprashanth merged commit 1ec21d8 into kubernetes-retired:master Jun 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants