Allow certificates to contain subjects with allowed external names

[NickLarsenNZ]

Overview

Currently, secret-operator can only sign/request certificates with IP/DNS names based on these scopes:

• listener-volume (The external-name that is reported back by the Load Balancer controller¹).
• node (eg: 10.0.0.1 or node-1.local.domain. If I understand correctly, this becomes obsolete since listener-volume scope)
• pod (STS) (eg: pod-0.app-service.app-namespace.svc.cluster.local)
• service (FQDN of the named service, eg: app-service.app-namespace.svc.cluster.local

Customers often want to use an external² DNS name, and in many cases use external-dns to automatically create DNS records for Services/Ingress/Gateway resources.

Common environment setups

In many environments (both Cloud and On-Prem):

• The kubelet clusterDomain is not usable outside of the cluster (especially
  when it is the default of cluster.local), and
• Customers often use names under a suffix separate from the kubelet
  clusterDomain. Often this is a shorter name (eg: app.team.example.com
  instead of app.app-service.app-namespace.cluster.domain).
• DNS is often auto-configured using External DNS, which is configured in these
  ways:
  • For Services, an annotation is place to map the DNS name to the Service
    (NodePort, or LoadBalancer) IP.
  • For Ingress/Gateway, there are dedicated fields for specifying the DNS name.
• Load balancers do TLS termination
  • In the case of internet-facing load balancers, this can be used to hide the
    subject names on the internal certificate which leak information such as the
    Kubernetes namespace and clusterDomain.

In some cases, they can work around this by using a Load Balancer that does L5 (TLS termination) or L7 (HTTP proxying), however with SNI checks becoming more prevalent these methods are becoming less effective when the external name used is not in the certificate used by the application.

This issue is about allowing external names to be added to certificates and a suggested approach follows...

Suggested approach

1. Extend the SecretClass to allow a list of suffixes (and minDepth/maxDepth number of dots) for additional names that can be added to the certificate.
2. Extend Listener with an externalName (or externalNames?) field.
   • If the listener-volume scope is used, and the suffix is permitted (within the minDepth/maxDepth bounds), then secret-operator will add that as a subject to the certificate request.
3. Extend Listener with a serviceAnnotations field.
   • We could use this opportunity to consistently name the fields in Listener and ListenerClass (either all fields to be passed to Service are prefixed with service, or we put everything under ServiceOverrides.

Important

Question: If the externalName set on the listener is not permitted in the certificate (or minDepth/maxDepth are out of bounds), where should an error event go? It could go on the Listener, but the listener doesn't concern itself too much with TLS details.

Example config:

Have a SecretClass which allows signing names under certain additional suffixes.

apiVersion: secrets.stackable.tech/v1alpha1
kind: SecretClass
metadata:
  name: org-pki
spec:
  backend:
    # Using autoTls as an example, but same applies for any supported backend
    autoTls:
      ca:
        secret:
          name: secret-provisioner-tls-ca
          namespace: default
        autoGenerate: true
      maxCertificateLifetime: 15d
  # 👇 New
  allowedSuffixes: # maybe should be additionalSuffixes, since it is on top of existing names
    - suffix: internal.example.com
      maxDepth: 0 # allow anything under internal.example.com, eg: a.b.c.d.internal.example.com

    # or
    - suffix: internal.example.com
      maxDepth: 1 # default, only allow one dot between the name and the suffix, eg: a.internal.example.com

    #  or
    - suffix: internal.example.com
      # Example: the organisation allows signing certs for $app.$team.internal.example.com
      # by requiring two dots between the name and the suffix
      # allow a.b.internal.example.com
      # disallow a.internal.example.com
      # disallow a.b.c.internal.example.com
      minDepth: 2
      maxDepth: 2
      # alternatively we could define depth and make it mutually exclusive to minDepth/maxDepth

    - suffix: internal.example.net

Have a ListenerClass ready for making public facing AWS NLBs with TLS termination

apiVersion: listeners.stackable.tech/v1alpha1
kind: ListenerClass
metadata:
  name: aws-ec2-nlb-public
spec:
  # I think it would make more sense to make a serviceOverrides key instead of prefixing some with "service" and missing it in others (eg: loadBalancerClass)
  serviceType: LoadBalancer
  # https://docs.aws.amazon.com/eks/latest/userguide/auto-configure-nlb.html#_sample_service
  loadBalancerClass: eks.amazonaws.com/nlb
  loadBalancerAllocateNodePorts: false
  preferredAddressType: HostnameConservative
  serviceExternalTrafficPolicy: Local
  serviceAnotations:
    service.beta.kubernetes.io/aws-load-balancer-scheme: internet-facing
    service.beta.kubernetes.io/aws-load-balancer-nlb-target-type: ip # Forward directly through node to Pod IP instead of an L3 hop/NAT through the node.
    service.beta.kubernetes.io/aws-load-balancer-attributes: >-
      proxy_protocol_v2.enabled=true

Have a NifiCluster, specifying an externalName (to be used in the resultant Listener) and service annotations to enable TLS temination on the Load Balancer, and a hostname to be configured by External DNS.

apiVersion: nifi.stackable.tech/v1alpha1
kind: NifiCluster
metadata:
  name: simple-nifi
spec:
  clusterConfig:
    tls:
      serverSecretClass: org-pki
  nodes:
    roleConfig:
      # I think this should be moved under listenerOverrides as className...
      listenerClass: aws-nlb-tls
      # 👇 New
      listenerOverrides:
        className: aws-nlb-tls # moved from roleConfig.listenerClass
        externalName: app.internal.example.com
        serviceAnnotations:
          external-dns.alpha.kubernetes.io/hostname: app.internal.example.com
          service.beta.kubernetes.io/aws-load-balancer-ssl-cert: arn:aws:acm:eu-central-1:123456789012:certificate/4e12c4fe-eed9-48db-98d8-820b6b50ace4
          service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "8443"

Based on the configurations above, the objects below should be created by the NiFi Operator:

# This is the Listener produced by NifiCluster
kind: Listener
metadata:
  name: the-nifi-listener
spec:
  className: aws-nlb-tls-public
  # 👇 New
  externalName: app.internal.example.com # this came from the NifiCluster overrides
  # 👇 Not yet available, see (see: https://github.com/stackabletech/listener-operator/issues/331)
  serviceAnnotations:
    external-dns.alpha.kubernetes.io/hostname: app.internal.example.com
    service.beta.kubernetes.io/aws-load-balancer-ssl-cert: arn:aws:acm:eu-central-1:123456789012:certificate/4e12c4fe-eed9-48db-98d8-820b6b50ace4
    service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "8443"

# This is the Pod produced by the Deployment produced by the NifiCluster
apiVersion: v1
kind: Pod
metadata:
  name: nifi-0
spec:
  volumes:
    - name: tls
      ephemeral:
        volumeClaimTemplate:
          metadata:
            annotations:
              secrets.stackable.tech/class: org-pki
              secrets.stackable.tech/scope: pod,service=nifi,listener-volume=the-nifi-listener

Based on the configurations above, the object below should be created by the Listener Operator:

apiVersion: v1
kind: Service
metadata:
  name: nifi-listener
  annotations:
    external-dns.alpha.kubernetes.io/hostname: app.internal.example.com
    service.beta.kubernetes.io/aws-load-balancer-attributes: proxy_protocol_v2.enabled=true
    service.beta.kubernetes.io/aws-load-balancer-nlb-target-type: ip
    service.beta.kubernetes.io/aws-load-balancer-scheme: internet-facing
    service.beta.kubernetes.io/aws-load-balancer-ssl-cert: arn:aws:acm:eu-central-1:123456789012:certificate/4e12c4fe-eed9-48db-98d8-820b6b50ace4
    service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "8443"
spec:
  type: LoadBalancer
  loadBalancerClass: eks.amazonaws.com/nlb
  loadBalancerAllocateNodePorts: false
  externalTrafficPolicy: Local
  ports: ...

Footnotes

1. Often, this is a dynamic (and not user-friendly) name and is not intended to be used directly, but rather be the value of the DNS CNAME record of the intended external name. ↩

2. Whether that is on the internet, or just within their organization - it is external to the Kubernetes cluster. ↩

[soenkeliebau]

We decided in the planning to hold this in RfC for a while to allow people more time to form thougths on this, as it is a fairly fundamental change.
We'll try to carve out time for people to look at this in the upcoming weeks, to get concerns / ideas / thoughts written down and allow for discussion to happen.

[sbernauer]

I would also like to think about this, but this takes a bit of time to consider all alternatives and impacts, which I'm currently struggling to get.

[siegfriedweber]

A certificate only has value if you can trust it.

If I trust the secret-operator and that operator signs verified information (such as the qualified pod name) then I trust the certificate. However, external names seem not to be verifiable by the operator. This raises the question: who or what else can I trust apart from the operator? For instance, if I also trust the persons who are able to create or modify SecretClasses and the SecretClasses do not take parameters from outside, then I can just reference the SecretClass in my cluster specification and everything is fine. But if the (full) external name is defined in an annotation of some cluster specification then I would have to trust everybody who is allowed to deploy such a specification.

The mentioned approach with suffixes may be okay, but only if the prefix is verified and not just given.

[NickLarsenNZ]

The mentioned approach with suffixes may be okay, but only if the prefix is verified and not just given.

The idea would be that the "admin" (those who can manage the cluster scoped SecretClass) would define what is a trusted suffix (or what are trusted suffixes). We can trust that the people with admin privileges will put trustworthy information in the field.

If there is a concern that anyone can make a SecretClass and put whatever in, then that's more of a problem of cluster/authorization management.

I can just reference the SecretClass in my cluster specification and everything is fine

That is true, but I think that is generally also a cluster management problem. There is tooling out there which can validate what goes into the manifests (eg: Kyverno, OPA).

[NickLarsenNZ]

@sbernauer, @siegfriedweber, and I discussed this yesterday.

We came to the conclusion that this particular decision can be punted on for now and that we would be happy with the pod IP being added to autoTls issued certificates (#620). It appears to be possible (cert-manager/csi-lib#20).

Custom names could be achieved through an alternative issuer (eg: via Certmanager and the configured Issuer).

---

For posterity...

We each had differing concerns and different understandings of trust.

For example, I treat a custom name as being allowed, though would prefer an allowed suffix list to be specified for SANs. Though I neglected to consider Client Authentication certificate SANs.

Others would want some kind of assurance that the additional name(s) added to the certificate are allowed (my opinion is that this is then a manifest validation problem which could be solved by Kyverno or OPA GateKeeper).

For now it is easier not to make policy decisions and instead leave it to tools that deal with it (eg: CertManager with the Venafi Issuer where policies are configured in Venafi). In addition, the immediate problem of Load Balancer certificate trust would be solved by having the pod and node IPs in the certificate.

Hopefully I have correctly, albeit briefly, described the opinions from yesterdays call.

[NickLarsenNZ]

I will leave this in RfC for now until we decide whether we will plan to work on the pod IP in the certificate, or if we will wait until it comes up again by itself.

[lfrancke]

We've decided to implement #620 and revisit this afterwards if needed.