Support cert-manager for Generated Certificates

[scottd018]

Is your feature request related to a problem? Please describe.

Not a problem, per se, but hard-coded backend values do not fit my use case. For my use case, which is a Government compliance use case, I may not use self-signed certificates anywhere in my cluster, so I must replace them with trusted certificates.

Describe the solution you'd like

Allow for use of a cert-manager Certificate object to be used in order to generate the following certificates:

• pinniped-concierge-api-tls-serving-certificate
• pinniped-concierge-impersonation-proxy-signer-ca-certificate
• pinniped-concierge-impersonation-proxy-tls-serving-certificate
• pinniped-concierge-impersonation-proxy-ca-certificate

The following are currently hard-coded (there may be others as well) which will not allow us to use cert-manager certificates for our use case in order to use trusted certificates:

https://github.com/vmware-tanzu/pinniped/blob/main/internal/controller/apicerts/certs_manager.go#L23-L26

Configuration can default to the current values if no additional configuration is provided.

Describe alternatives you've considered

I've considered using External Secrets as a translation layer, but this is currently not possible with their implementation. See comment external-secrets/external-secrets#850 (comment).

Are you considering submitting a PR for this feature?

Right now, I will not have time to support a PR for this feature, but I would like to if I can ever catch up.

Additional context

Relates to issue #1237

See conversation on Slack at https://kubernetes.slack.com/archives/C01BW364RJA/p1658264659180309

[joshuatcasey]

See #1547 for the third and fourth bullets (pinniped-concierge-impersonation-proxy-tls-serving-certificate and pinniped-concierge-impersonation-proxy-ca-certificate).

[joshuatcasey]

I took a look at the first two secrets mentioned here:

• pinniped-concierge-api-tls-serving-certificate
• pinniped-concierge-impersonation-proxy-signer-ca-certificate

I wanted to see how they are used, especially which controllers use them, with a view to see what changes we would need to make for these to be externally provided as secrets of type kubernetes.io/tls

pinniped-concierge-api-tls-serving-certificate

This secret is used to serve TLS for all Concierge endpoints. In practice, these endpoints are:

• /healthz
• The backing endpoint for the login APIService
• The backing endpoint for the identity APIService

This secret uses keys caCertificate and caCertificatePrivateKey (to hold a CA and its private key), tlsCertificateChain, and tlsPrivateKey (to hold a TLS serving certificate and its private key).

This secret is given to five controllers:

• CertsManager, responsible for creating the secret (with all keys and values) if it does not exist.
• CertsExpirer, responsible for rotating the value in key tlsCertificateChain in the secret if it exceeds a hard-coded TTL
• APIServiceUpdater, once to update spec.CABundle for the login APIService, and once to updatespec.CABundle for the identity APIService, using the value from key caCertificate
• CertsObserver, responsible for reading into memory the values from keys tlsCertificateChain and tlsPrivateKey so that they can be used to serve TLS

Changes required to support kubernetes.io/tls

cert-manager will give us something like the following Secret.

apiVersion: v1
kind: Secret
type: kubernetes.io/tls
data:
  ca.crt: LS0t... # this is optional
  tls.crt: LS0t...
  tls.key: LS0t...

The APIServiceUpdater controller should look for keys in the following order: ca.crt, tls.crt, and caCertificate.

The CertsObserver controller should look for keys tls.crt and tls.key first, and then fall back to using tlsCertificateChain and tlsPrivateKey.

The CertsManager controller will create this secret if it does not exist. If the secret exists, the CertsManager controller will take no action. (No change needed).

The CertsExpirer controller should continue to look for keys caCertificate and caCertificatePrivateKey and if those do not exist, should continue to provide a log message and requeue itself (by returning an error). We should confirm that this does not "thrash" the controller, but behaviorally should work. (No change needed).

pinniped-concierge-impersonation-proxy-signer-ca-certificate

This secret is used to issue mTLS certificates to clients that have made a TokenCredentialRequest when the impersonation proxy is used. This secret only uses keys caCertificate and caCertificatePrivateKey.

This secret is given to three controllers:

• ImpersonatorConfig, responsible for reading the values from keys caCertificate and caCertificatePrivateKey into memory so that they can be used to issue client certificates for mTLS.
• CertsManager, responsible for creating the secret (with all keys and values) if it does not exist.
• CertsExpirer, responsible for rotating the CA in the secret if it exceeds a hard-coded TTL

Changes required to support kubernetes.io/tls

cert-manager will give us something like the following Secret. Note that Pinniped does appear to verify whether tls.crt has isCA: true set, so be sure that is configured!

apiVersion: v1
kind: Secret
type: kubernetes.io/tls
data:
  ca.crt: LS0t... # this is optional
  tls.crt: LS0t...
  tls.key: LS0t...

I think the only change here is that the ImpersonatorConfig controller should first check for existence of keys tls.crt and tls.key (instead of caCertificate and caCertificatePrivateKey) and then fall back to using caCertificate and caCertificatePrivateKey.

The CertsManager controller will create this secret if it does not exist. If the secret exists, the CertsManager controller will take no action. (No change needed).

The CertsExpirer controller should continue to look for keys caCertificate and caCertificatePrivateKey and if those do not exist, should continue to provide a log message and requeue itself (by returning an error). We should confirm that this does not "thrash" the controller, but behaviorally should work. (No change needed).

[joshuatcasey]

I wonder if we could take it a step further and have the CertsManager controller generate secrets with type kubernetes.io/tls. Such secrets are meant to have only a single certificate in them, and secret pinniped-concierge-api-tls-serving-certificate has two certificates - one is a CA and the other is a TLS serving certificate. We'd have to split pinniped-concierge-api-tls-serving-certificate into two secrets, both of which would be created by the CertsManager controller. Only the serving cert would be rotated by the CertsExpirer controller. There would need to be some additional coordination (either new or existing controller) to read the CA certificate secret into memory so that it could be used to generate the TLS serving certificate.

There would need to be some additional thought given to backwards compatibility: the "reading" controllers such as CertsObserver, APIServiceUpdater, and ImpersonatorConfig should continue to fall back to reading "legacy" keys such as caCertificate. The only way around this is to introduce some sort of "migrating" controller that finds secrets of the legacy format and rewrites them to secrets of type kubernetes.io/tls.

One additional consideration if Pinniped writes secrets of type kubernetes.io/tls is that Pinniped would need to be aware of which secrets it manages itself and which secrets are provided externally. In particular should CertsExpirer remove secrets close to expiration, or assume that externally-provided secrets will be managed appropriately? And how would the CertsManager controller know when to not create a secret? Perhaps we could introduce a configuration option to tell Concierge that these secrets will be externally provided. Perhaps we could use ownerReferences for Pinniped to know which secrets it made itself.

[cfryanr]

It would be good to make a comprehensive list of certs that should be considered, including in the Pinniped Supervisor.

[cfryanr]

Would it be possible to make only very minor changes to the controllers, and then simply document how to pre-write and/or overwrite the secrets that the controllers will read, and how to effectively disable the CertsExpirer controller? Maybe that's what you are already thinking, but I'm interested in explicitly considering how could we design this feature to make it as small as possible. For example, can we design it so there are no new user-facing configuration options? Would it be possible to implement this with no code changes at all, only docs? Maybe not, but would be worth considering.

[cfryanr]

Should we make assumptions about key names inside Secrets based on how cert-manager works, or should we make the user configure them? Same question for Secret types.

[joshuatcasey]

Should we make assumptions about key names inside Secrets based on how cert-manager works, or should we make the user configure them? Same question for Secret types.

I think generally controllers should detect the secret type, and assume the magic key names tls.crt and tls.key when the secret type is kubernetes.io/tls. If a CA bundle is needed, check for an optional field ca.crt, otherwise have some sort of fallback behavior (to tls.crt or something else).

If the secret type is other than kubernetes.io/tls that depends on how far we take this implementation. If Pinniped continues to generate secrets in its on bespoke Opaque format, then Pinniped should assume secrets have that format. I'm not aware of any other style or format in common use.