generic controller shouldn't require snakeoil certs

[bprashanth]

Currently I can't build a simple controller on busybox because we hit this code path: https://github.com/kubernetes/ingress/blob/master/core/pkg/ingress/controller/controller.go#L837 which goes looking in /etc/ssl for default certs. If we don't find certs we can simply generate them on the fly.

@aledbf fyi

[rikatz]

@bprashanth this is my fault :)

The thing is that, while trying to solve the problem of Default SSL Certificates, I've hit this part of code that you need some kind of certificate.

This was the simplier way to do this, generating those Fake Certificates.

I don't know if maybe a hardcoded certificate would be better in this case, so I'm gonna wait for @aledbf to answer.

[rikatz]

What about https://golang.org/src/crypto/tls/generate_cert.go ?

[bprashanth]

Yeah we can just generate a self signed cert the same way as generate_cert.go.
Can you clarify what exactly this cert is used for? Based on that we might need to allow further parametrization, but for now I think we can just fill in all the defaults

[einarf]

Might be a bit off topic, but it would also be a huge plus if the code checked that the ssl directly exists and can be written to instead of crashing 😃

[rikatz]

@bprashanth I use this in case you have a TLS definition (or a default certificate, as example) and the certificate is invalid or inexistence.

Instead of generating a lot of error messages, we do this to create a fake SSL Certificate.

[rikatz]

@bprashanth I've a WIP here to make this work. Will commit into my repo and work at home.

You can take a look here, I'm going to improve also the controller reloading handling, and make a new PR to solve this problem.

Here is the repo: https://github.com/rikatz/ingress/blob/nginx-ssl-problem/core/pkg/net/ssl/ssl.go#L170

[rikatz]

@bprashanth Opened the PR #194, take a look and report if this solves our problem :)

[bprashanth]

Thanks! Will take a look today/tomorrow

[bprashanth]

I use this in case you have a TLS definition (or a default certificate, as example) and the certificate is invalid or inexistence.

Isn't it better to throw events and fail fast, if the cert is malformed or non-existent? If you use a random fake cert users are probably going to be broken because the cert is signed by an unknown CA, but the admin of the cluster will continue to think everything is fine, right?

[rikatz]

@bprashanth I think this is a point of view.

Generating self signed certificates (as it was being made with the snake-oil-certificate) could make user and admin life easier.

Maybe we should only warn (or make a flag in the program) to generate or not this Self Signed Certificate.

But as the following situation:

• You set Default Backend.
• Default Backend MUST have an SSL Vhost (I think this should be mandatory, anyway, but my opinion)
• You don't set a default tls secret
• Your ingress controller crashes.

I don't think this is a desirable situation. Generating self signed certificates, in my opinion, is a much more elegant solution that giving user an error :)

But we can still change / revert this.

@aledbf your opinion in this is important!

[aledbf]

@rikatz @bprashanth we need (at least in nginx) a certificate as fallback to be used in the default server to listen in port 443. Without this the first ingress configured with a tls section will be used as default server for https (this is the reason of the flag --default-ssl-certificate)

[aledbf]

@bprashanth @rikatz please check kubernetes-retired/contrib#1393 for context

[bprashanth]

Maybe I misunderstood, I think using a default cert for just the default backend is good. I think using a default cert instead for a specific hostname is going to surprise users, i.e:

tls: 
- hosts: 
  - google.com
  secretName: wwww
rules: 
- host: google.com
  paths
    - path: /
      backend: 
        serviceName: frontend 
        servicePort: 80
status:
  ingressIP: 1.2.3.4

If www doesn't exist, I want google.com to be unavailable, not encrypt using a self signed cert.
OTOH, a request sent to 1.2.3.4 with Host: facebook.com should hit the default backend and get the snakeoil cert.

[rikatz]

@bprashanth Whatyou reported in your last comment is dealed by this piece of code:

https://github.com/rikatz/ingress/blob/5fc45ea1f5ee8c6a88a80b023da506148cf72489/core/pkg/ingress/controller/controller.go#L935

This if we could substitute by a glog.Warning message. I've changed this before (to use the default certificate) as this could be easier for users to enable the HTTPs vhost using the generic certificate.

[bprashanth]

How about including the always including the default certificate in the ingress.Configuration and letting the backend decide if it should use when there's no available certificate? Then if you don't find the secret you leave the field empty and throw an event.

[rikatz]

@bprashanth I Didn't get the idea. What happens here is that if the backend doesn't find a valid certificate, it uses the default one (the configured in --default-ssl-certificate, or otherwise if none configured the fake one).

The idea is just throw a Warning when using the default certificate?

[bprashanth]

Yeah I want that to be the decision of the backend, not the decision of the generic controller.
Meaning here: https://github.com/rikatz/ingress/blob/5fc45ea1f5ee8c6a88a80b023da506148cf72489/core/pkg/ingress/controller/controller.go#L935, don't set it to the fake one, instead make it so that here: https://github.com/kubernetes/ingress/blob/master/examples/custom-controller/server.go#L50 the ingress.Configuration finds a server without a cert, and a fake cert in the ingress.Configuration struct, and it can choose to insert the default into the server or not.

I haven't looked at the implementation yet, or how we'd pipe the default cert into the ingress.Configuration, but does that make sense?

[rikatz]

Hum, ok.

So let's 'draw' the flow:

• If it's a fake certificate (generated because of the need of a fake certificate as the first HTTPs vhost of nginx) and the server definition doesn't contain a secret (or contains an invalid one), then throw a Warning about that and ignore the existence of the fake certificate

• If it's not a default fake certificate (like a valid default), then use it in the server/vhost if that's invalid.

The thing I think is, even if we do this approach, when a vhost doesn't contain a valid certificate but NGINX implements at least one ssl listener, the first one will be used again (the default_server).

Your approach makes sense so we can still reduce the file size of nginx.conf, but the user will get the error anyway, as there is at least one ssl listener, and the system will not be able to find the right certificate for users vhost.

Am I right?

[bprashanth]

I'm thinking about delegation to the specific backend, which may or may not be nginx. The decision to use or ignore the fake cert should be made somewhere here: https://github.com/kubernetes/ingress/blob/master/controllers/nginx/pkg/cmd/controller/nginx.go#L17, not in the core.

After doing that, I'm fine plugging the default where there's no valid cert because any user that doesn't want this behavior will just not specify the --default-ssl-certificate param (and nginx will crashloop if the secret doesn't exist)

[rikatz]

Guys, I'm working in the Authentication TLS here, and will return this issue ASAP.

[rikatz]

@bprashanth so, what's the right workflow?

1. User specified a --default-ssl-certificate that doesn't exist: Crashloop
2. User specified an existent --default-ssl-certificate: The certificate is used in the default vhost and also in vhosts that specify a 'tls' directive, but an invalid secret
3. User does not specify any --default-ssl-certificate: A self signed is generated for the default server, but this cert is only used in the default server, not in remaining misconfigured servers.

Is this right? I couldn't figure out how to accomplish this in the nginx controller code, but only in core.

Sorry about bothering you with this, but I really can't figure out what need to be done

[bprashanth]

I'm fine with not crashlooping.

I'm suggesting that you add a field called protocol to ingress.Server here: https://github.com/kubernetes/ingress/blob/master/core/pkg/ingress/types.go#L156

Set it here, if you find a ing.spec.tls: https://github.com/kubernetes/ingress/blob/master/core/pkg/ingress/controller/controller.go#L880

in cases 1 and 2 the generic controller leaves the cert field empty here:
https://github.com/kubernetes/ingress/blob/master/core/pkg/ingress/controller/controller.go#L812

That will get passed to OnUpdate: https://github.com/kubernetes/ingress/blob/master/core/pkg/ingress/controller/controller.go#L390

And then to the nginx controller backend:
https://github.com/kubernetes/ingress/blob/master/controllers/nginx/pkg/cmd/controller/nginx.go#L292

At the last step the nginx controller backend says:

• hey I found a server with protocol==HTTPS, but no cert field, I'm going to first look at my --default-cert, if that's empty, I'm going to try generating some snakeoil certs and plug them in there

and the GCE controller can say

• hey I found a server with protocol==HTTPS, but no cert field, I'm going to first look at my --default-cert, if that's empty, throw a warning and ignore the ingress

Something like that, make sense? We might also have to pipe the --default-cert arg to the backend for that to work.

[bprashanth]

I'm also not sure if we want to call it protocol, or just TLS=true/false, since protocol=TCP might also want TLS and we won't have a way to express that. Maybe we don't even mention tls and call it TransportSecurity?

[aledbf]

Closing. Fixed in #382