Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deployment fails for TLS certificates via cert-manager and ACME #134

Open
miclip opened this issue Jul 15, 2022 · 5 comments
Open

Deployment fails for TLS certificates via cert-manager and ACME #134

miclip opened this issue Jul 15, 2022 · 5 comments

Comments

@miclip
Copy link

miclip commented Jul 15, 2022

Using a cluster issue for Let's Encrypt over ACME fails. Let's encrypt requires that if the common name is set, the same value must also be listed as a SAN. The yugabyte helm chart is hard-coding the common name. In addition, the TLS secret that is created does not contain the CA, the statefulset is expecting to mount the CA from the TLS secret as a volume.

I believe these are not unique to let's encrypt and corporate PKI solutions will have the same CSR requirements, and not return the CA within the TLS secret cert-manager generates.

@iSignal
Copy link
Contributor

iSignal commented Jul 15, 2022

@miclip : Thanks for filing this, this is useful feedback.

  1. What do you suggest having in the CN instead? Given that the certificate is common to all pods, I guess we could use the DNS for yb-tserver-0/yb-master-0, which is also one of the SAN entries. Any other suggestions?

  2. I do see such documentation on cert-manager docs confirming this.

Additionally, if the Certificate Authority is known, the corresponding CA certificate will be stored in the secret with key ca.crt. For example, with the ACME issuer, the CA is not known and ca.crt will not exist in acme-crt-secret.

But this is confusing. How are we supposed to discover the CA cert in such cases? Is this supposed to a mechanism of distribution outside of k8s and the customer manually specifies the ca.crt in such cases?

@subramanian-neelakantan @bhavin192 @baba230896

@miclip
Copy link
Author

miclip commented Jul 15, 2022

  1. I need to confirm but I think the CN is no longer required and the SANs (DNS Names) are all you need?
  2. They're assuming the CA is public or already trusted. Perhaps supporting an 'additionalTrustedCerts' field in the values file so any custom certs can be trusted within the deployment.

@miclip
Copy link
Author

miclip commented Jul 15, 2022

k explain certificate.spec.commonName


KIND:     Certificate
VERSION:  cert-manager.io/v1

FIELD:    commonName <string>

DESCRIPTION:
     CommonName is a common name to be used on the Certificate. The CommonName
     should have a length of 64 characters or fewer to avoid generating invalid
     CSRs. This value is ignored by TLS clients when any subject alt name is
     set. This is x509 behaviour:
     https://tools.ietf.org/html/rfc6125#section-6.4.4

So you should be able to just not set the CN provided you always set the SANs, which should work for self signed and ACME etc. I see in certs that are requested as a result of Ingress annotations, cert-manager never sets the CN.

@iSignal
Copy link
Contributor

iSignal commented Jul 25, 2022

@miclip : ok, got it. We are planning to iterate on the cert-manager functionality soon and will take this feedback into account.

@rastaman
Copy link

rastaman commented Jul 22, 2024

Edit: Oups i forgot to read the full issue before, below patch helps but not for the CA.

I have the same issue, so i used a kustomization to patch the helm chart:

patches:
- patch: |-
    apiVersion: cert-manager.io/v1
    kind: Certificate
    metadata:
      name: yugabyte-tls-client-cert
      namespace: yugabyte
    spec:
      commonName: yugabyte-tls-client-cert.homelab.expansible.io
      dnsNames:
      - yugabyte-tls-client-cert.homelab.expansible.io

(where homelab.expansible.io is my own domain)

However after the patch owrks, it still fails because there is no ca.crt in the certificate provided by Let's Encrypt. I guess i'll open an issue for this too, there is a few lines of helm chart/shell script to rework. Perhaps do you know if it Is already seen in the iteration of the cert-manager functionnality or should i open an issue @iSignal ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants