Ingress-nginx: auth-tls-pass-certificate-to-upstream: true shadowed by other ingress resource where it is undefined

[kd7lxl]

NGINX Ingress controller version: 0.25.0

Kubernetes version (use kubectl version): v1.12

What happened: The Ingress annotations included auth-tls-pass-certificate-to-upstream: true, but nginx.conf was rendered without the line proxy_set_header ssl-client-cert $ssl_client_escaped_cert;

What you expected to happen: Consider multiple Ingress resources on a single hostname. With other TLS-related annotations (like auth-tls-verify-client), adding them to one Ingress will cause cause them to be enabled for all Ingresses on that hostname. However, with auth-tls-pass-certificate-to-upstream, the certificate is not passed passed to the upstream unless auth-tls-pass-certificate-to-upstream: true is set on all ingresses for that hostname. Since this annotation simply controls a boolean in the location section of the template, I would expect to be able to enable it on a per-ingress basis.

When reading the template, it is clear what the problem is:
https://github.com/kubernetes/ingress-nginx/blob/b30115aba7e11da3da275910e17448b17f9892aa/rootfs/etc/nginx/template/nginx.tmpl#L932
PassCertToUpstream is scoped under server, not location, so the value may actually be populated by a different ingress resource.

How to reproduce it:

apiVersion: networking.k8s.io/v1beta1
kind: Ingress
metadata:
  name: foo
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/auth-tls-secret: default/ca-cert
    nginx.ingress.kubernetes.io/auth-tls-verify-client: optional
spec:
  rules:
  - host: example.com
    http:
      paths:
      - backend:
          serviceName: http-svc1
          servicePort: 80
        path: /foo
---
apiVersion: networking.k8s.io/v1beta1
kind: Ingress
metadata:
  name: bar
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/auth-tls-pass-certificate-to-upstream: "true"
    nginx.ingress.kubernetes.io/auth-tls-secret: default/ca-cert
    nginx.ingress.kubernetes.io/auth-tls-verify-client: optional
spec:
  rules:
  - host: example.com
    http:
      paths:
      - backend:
          serviceName: http-svc2
          servicePort: 80
        path: /bar

Anything else we need to know:

I suspect this patch to the e2e tests will reproduce the issue, but I don't have a system to run the e2e tests on at the moment to try it:

diff --git a/test/e2e/annotations/authtls.go b/test/e2e/annotations/authtls.go
index 350e21a92..abbea4a88 100644
--- a/test/e2e/annotations/authtls.go
+++ b/test/e2e/annotations/authtls.go
@@ -125,11 +125,14 @@ var _ = framework.IngressNginxDescribe("Annotations - AuthTLS", func() {
                Expect(err).ToNot(HaveOccurred())

                annotations := map[string]string{
-                       "nginx.ingress.kubernetes.io/auth-tls-secret":                       nameSpace + "/" + host,
-                       "nginx.ingress.kubernetes.io/auth-tls-error-page":                   f.GetURL(framework.HTTP) + errorPath,
-                       "nginx.ingress.kubernetes.io/auth-tls-pass-certificate-to-upstream": "true",
+                       "nginx.ingress.kubernetes.io/auth-tls-secret":     nameSpace + "/" + host,
+                       "nginx.ingress.kubernetes.io/auth-tls-error-page": f.GetURL(framework.HTTP) + errorPath,
                }

+               f.EnsureIngress(framework.NewSingleIngressWithTLS(host, "/foo", host, []string{host}, nameSpace, framework.EchoService, 80, annotations))
+
+               annotations["nginx.ingress.kubernetes.io/auth-tls-pass-certificate-to-upstream"] = "true"
+
                f.EnsureIngress(framework.NewSingleIngressWithTLS(host, "/", host, []string{host}, nameSpace, framework.EchoService, 80, annotations))

                assertSslClientCertificateConfig(f, host, "on", "1")

/kind bug

[kmindi]

This might have created a lot of effort to create a separate service to listen for client-cert requests and pass it to the main service then, also introducing incompatibilities and errors if the nginx configs are not exactly the same, which is not easy to achieve.

I would very much like to see this fixed, somebody here who might point out which files need to be changed to fix it, so a pull request can be provided?

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[kmindi]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[kmindi]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[kmindi]

/remove-lifecycle stale

[aledbf]

Since this annotation simply controls a boolean in the location section of the template, I would expect to be able to enable it on a per-ingress basis.

The certificate authentication only works at server level. This means is enabled or not. We cannot provide a dynamic behavior per ingress
http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_client_certificate

[kmindi]

@aledbf you are only right on one point I think, which is not the problem here: client-cert must be enabled globally, yes. BUT:

This is about "proxy_set_header " (which is in location context). E.g. When to pass cert info to upstream, not if client cert should be prompted in general ;)

E.g. the mentioned code line: https://github.com/kubernetes/ingress-nginx/blob/master/rootfs/etc/nginx/template/nginx.tmpl#L1024 is within the location context, but refers to a server context annotation?!

That's why it is not working, as the server context annotation will be overriden if it is not set in all ingresses! So if only one ingress is not setting it, then this will not be set in any ingress! Therefore this (and only this) configuration{{ if $server.CertificateAuth.PassCertToUpstream }} should be moved to a location context.

And this does not mean that the client-cert in general should be moved to the location context, which does not work as you mentioned. Yet, the proxy_set_header part should work in location context!

[aledbf]

is within the location context, but refers to a server context annotation?!

That's correct. The source for PassCertToUpstream is here https://github.com/kubernetes/ingress-nginx/blob/a85e53f4cb4e309f75a60b704b93480b48681081/internal/ingress/controller/controller.go#L507-L512
This means the value to use is extracted from the ingress where the certificate authentication is defined.

[kmindi]

Hmm I'm not sure I get it compeletly and if messages in this issue are appropriate for a discussion.

I still think that PassCertToUpstream is something location context specific. But it tries to access server annotations, which can only be there one time. And if they are only there one time, than the last parsed ingress will set that. E.g. if there is more than one ingress and one of them does not define passCertToUpstream, then not one single location will get this. And that is wrong behavior, isn't it?

Each ingress should individually be able to allow passCertToUpstream. That does not work currently.
By making this PassCerttoUpstream annotation a location annotation instead of server, then the ingresses would not overwrite (shadow) each other and the last one wins!

(I think there is a differcen between CertificateAuth on a global level and PassCerttoupstream on each location level)

[aledbf]

Each ingress should individually be able to allow passCertToUpstream.

The annotation auth-tls-pass-certificate-to-upstream is valid only for client-certificate-authentication not a generic variable.

[aledbf]

You can use the configuration-snippet to pass the variable ssl_client_escaped_cert

[kmindi]

Okay, I'll try one more time to explain.

The check if client-cert-authentication is active for the server, that part is correct. So if it is enabled or not does work correctly.
Now in case client-cert-authentication is enabled:

1. the following lines are executed:

# Pass the extracted client certificate to the auth provider
{{ if not (empty $server.CertificateAuth.CAFileName) }}
{{ if $server.CertificateAuth.PassCertToUpstream }}
proxy_set_header ssl-client-cert        $ssl_client_escaped_cert;
{{ end }}

The if check for $server.CertificateAuth.PassCertToUpstream is correct in general, e.g that it checks that annotation. Yet, that annotation should be $location.PassCertToUpstream (or similar) as proxy_set_header ssl-client-cert $ssl_client_escaped_cert; can be set for each location individually.

Right now this annotation $server.CertificateAuth.PassCertToUpstream begins with "server" so it will only be there once for the complete server.
If various ingresses define this annotation or not define it, it does not work, because it can only be there once, right? so either true or false.

But this should be different for each location! as the proxy_set_header ssl-client-cert $ssl_client_escaped_cert; is something that should be set for each location individually not either for all of them or for none of them!

Hmm I just saw your other comment, if that is the intended way to do this that's also fine for me, but still I'm thinking passCertToUpstream should be available per location as annotation otherwise it does not make sense, because it does not work as explained (it gets shadowed by other configurations, e.g. either all ingresses have to define passcertotupstream or it will not work)