Ingress-nginx: Force SSL with default cert should not require tls section

[kingdonb]

In a cluster with nginx ingress installed, it was possible to fall into this trap:

1. Set a default cert. Ours was a star cert generated through cert-manager.

2. Enable force-ssl mode

3. Ingress routes without a tls: block will fail

It makes sense that the tls: block is required, because the ingress spec probably demands it, and without it nginx likely won't know that it should add a config block to listen on 443.

But it doesn't make sense that you would configure the ingress this way on purpose. Having set a default cert, there's no need to list a secret in the tls: section. The hostname is already described in the hosts block.

The feature request is to add a MutatingAdmission hook. I think it's obvious what is missing from the config, and it should be added automatically. I just learned about admission controllers and I think the MutatingAdmissionWebhook is the tool for the job.

Wdyt?

[kingdonb]

At minimum there should be some documentation to accompany the default cert mode that explains this, I think.

[0verc1ocker]

Yes, so I spent a few hours debugging this yesterday and believe this would be a great feature. Why do I have to define a specific tls: block for an ingress just to make sure that it opens up port 443 on nginx-ingress for that specific ingress object? This is extra-configuration for limited gain.

I had this in my extraArgs section of nginx-ingress helm values:

controller:
  # Necessary to publish LB service IP on ingress objects and acquire static IPs for external-dns to work:
  publishService:
    enabled: true
  # ConfigMap of the nginx-ingress:
  config:
    ssl-redirect: "true"
    # Proxy protocol is not supported by GKE loadbalancers, use externalTrafficPolicy Local instead
    # https://github.com/kubernetes/ingress-nginx/issues/3723
    use-proxy-protocol: "false"
    large-client-header-buffers: "4 32k"
  extraArgs:
    default-ssl-certificate: "kube-system/certificate"

When default-ssl-certificate is set, shouldn't all ingresses expose 443 as we are assuming to be using a cert for the whole cluster. If the domain does not match then we can get SSL errors or SNI errors, but at least every route that matches the default domain wildcard cert will work automatically and I don't have to edit every ingress on every helm chart that I deploy...

[aledbf]

It makes sense that the tls: block is required, because the ingress spec probably demands it, and without it nginx likely won't know that it should add a config block to listen on 443.

@kingdonb sorry but no. The ingress spec doesn't have too many rules but changing the behavior of the tls block is something I am not willing to change.
We can review this if the ingress spec is updated with such modification.

[aledbf]

When default-ssl-certificate is set, shouldn't all ingresses expose 443 as we are assuming to be using a cert for the whole cluster.

@0verc1ocker the objective of the default-ssl-certificate is to allow the use of a custom certificate instead of the fake one generated for the ingress controller, not the use case you describe.

[0verc1ocker]

In nginx, when you define a default server_name block it is used as a catch all for server_names that do not match explicitly. Interpreting "default-ssl-certificate" with the same meaning would be a certificate that will match all protocol 443 configured server_name blocks. Here the --default-ssl-certificate is set for all server_names but the protocol is not supported because of how the ingress spec is defined and did not automatically open 443 in the config of nginx.

Correct me if I am wrong, but "default" in this case is not "default" in the same sense as the "default" of the nginx config spec?

At minimum we should clearly outline this in the documentation? I lost some time just figuring this out because I thought the ingress object would be smart enough to open 443 in the config. Point me to docs and I'll make a PR to add it.

[0verc1ocker]

For example, in the docs for nginx-ingress --default-ssl-certificate section it only says that "For HTTPS, a certificate is naturally required.". This is not totally true because I'm only adding a tls section in my ingress spec and not generating a specific cert for that particular domain...

[kingdonb]

The ingress spec doesn't have too many rules but changing the behavior of the tls block is something I am not willing to change.

That's the beauty of an admission hook. You do not have to change the behavior of the TLS block, @aledbf – you can either validate upon admission, or mutate to correct the TLS block in-place, so that the behavior of your ingress does not have to change, and the configuration maintains consistency with the upstream ingress spec.

There are two kinds of admission hooks that are common and I am aware of, a "Mutating" and a "Validating" admission webhook. They are exclusive as I understand it, one webhook can either be Mutating or Validating but not both, and I think that either could solve this.

First, the Validating one is the webhook that does not change anything about the content of the manifest, except that it will exclude configurations which can be shown to be terminally invalid:

A validating webhook would recognize that you have configured the TLS block in a completely invalid way, and immediately reject the configuration upon submission, preventing you from modifying your manifests in a way that is verifiably guaranteed to be wrong.

Presumably it would be possible to report with the validation failure, a clear and obvious message which explains that "your selected nginx-ingress is configured with force-ssl, but your ingress did not define a TLS rule. This will result in a bad redirect and you would probably scratch your head for a while trying to figure this out, so we've prevented it."

If there is a reason that you might configure your ingress rule this way intentionally, I can't think of it. That seems like a great candidate for validating webhook. For the user, the obvious answer here upon recognizing the problem is to define a TLS rule on their ingress manifest, with a host matching the particular ingress "host" setting, so that the redirect to SSL can succeed.

On the other hand, that configuration is redundant, and since the correct course of action is obvious, this whole correction could be imputed by a simple mutating webhook, which – having recognized that the user has made an invalid configuration – would simply make the obvious required change, proactively.

I could understand if you don't want to make a mutating webhook, because the behavior could be surprising (ingresses that you write will be modified in place before they are admitted to the cluster, the user will not be provided with feedback about why this happened, it could create unnecessary inquiries and cause users to scratch their heads.)

But admission controllers of these types are not new, having been Beta since 1.9 and recommended to be enabled for a while apparently, even now enabled by default in Kubernetes 1.13, so to me, it looks like these tools are a standard part of the Kube API, even if they are still in beta.

Would you consider reviewing a patch if we were able to write one?

[aledbf]

You do not have to change the behavior of the TLS block, @aledbf – you can either validate upon admission, or mutate to correct the TLS block in-place, so that the behavior of your ingress does not have to change, and the configuration maintains consistency with the upstream ingress spec.

That's exactly what I think we should not do

Would you consider reviewing a patch if we were able to write one?

Sorry but not for this feature. There is already a PR to add a webhook to the ingress controller. Please check https://github.com/kubernetes/ingress-nginx/pull/3802 to avoid invalid ingress definitions.

[aledbf]

Correct me if I am wrong, but "default" in this case is not "default" in the same sense as the "default" of the nginx config spec?

You are right, the word default is not correct if we see this from the ingress rule definition. In that case we should say fallback SSL certificate. That said, the flag name is right because the SSL certificate is used in the default NGINX server definition for port 443.

[aledbf]

This is not totally true because I'm only adding a tls section in my ingress spec and not generating a specific cert for that particular domain...

If you don't have any TLS section in any ingress, then NGINX will not listen in port 443. The default certificate is used in the catch all server definition (check the _ server block) avoid this issue. Also, keep in mind NGINX uses SNI for TLS connections.

[kingdonb]

That's exactly what I think we should not do

Why not at least validate "tls" sections exist, when "force-ssl" is enabled?

This is something that is not ever going to be solved upstream, the annotation:

nginx.ingress.kubernetes.io/force-ssl-redirect: "true"

belongs to nginx-ingress, and isn't a part of the upstream spec. This is the only place it can be solved, unless I start bolting things onto nginx-ingress.

[kingdonb]

You are right, the word default is not correct if we see this from the ingress rule definition. In that case we should say fallback SSL certificate. That said, the flag name is right because the SSL certificate is used in the default NGINX server definition for port 443.

That's understandable, this seems like a non-standard configuration, and highly specific. I think it's going to be a rare case that users have a default SSL certificate, and they want it to be used for every ingress, especially given how you've explained it.

But ignoring default SSL certificates for a minute, isn't it clear that an ingress rule missing a TLS section is invalid, if force-ssl-redirect is enabled? Or is there some mashup configuration I'm not considering, where some other ingress picks up SSL in the case that nginx is not the one listening on 443.