Cert-manager: Can not create a Certificate when the dnsNames field is omitted

[masa213f]

Describe the bug:

Creating a Certificate is failed with cert-manager v0.11.0.

The manifest only contains commonName fields as bellow:
It was successful with cert-manager v0.10.1.

apiVersion: cert-manager.io/v1alpha2
kind: Certificate
metadata:
  name: test-certificate
  namespace: cert-manager
spec:
  secretName: example-com-tls
  issuerRef:
    kind: ClusterIssuer
    name: <issuer name>
  commonName: hoge.example.com

Expected behaviour:

Creating the Certificate is successed.

Anything else we need to know?:

The validation of the "Certificate" became strict by the following PR/commit.

https://github.com/jetstack/cert-manager/pull/2085
https://github.com/jetstack/cert-manager/pull/2085/commits/b6bce10b2fff1d8993f6888aa33a3b82b3a05540

However, the description says as follows:

https://github.com/jetstack/cert-manager/blob/v0.11.0/deploy/manifests/00-crds.yaml#L1839-L1841

A valid Certificate requires at least one of a CommonName, DNSName, or URISAN to be valid.

I think the omission of the dnsNames field should be allowed according to this sentence :)

Environment details::

• Kubernetes version (e.g. v1.10.2): 1.15.4
• Cloud-provider/provisioner (e.g. GKE, kops AWS, etc): On-premises
• cert-manager version (e.g. v0.4.0): v0.11.0
• Install method (e.g. helm or static manifests): Argo CD + Kustomize

/kind bug

[munnerz]

Can you share the error message you are seeing? I cannot see where we have marked the dnsNames field as required, and the referenced commit modified our e2e suite but doesn’t seem to make the field actually required..?

[munnerz]

Ah - https://github.com/jetstack/cert-manager/pull/2085/files#diff-b5082e2ea140a11ff02a0f075decec76R96

So if you set CommonName, it must be also listed as a dnsName. I think we can use defaulting here to handle this better for you:

• iff the spec.dnsNames field is not set at all (ie is nil), and commonName is provided, we should manually copy across commonName to be the only element in dnsNames.

We’ll need to think carefully about how that default works if a user then goes and changes the ‘commonName’ field in their local manifests however - as the defaulting would not be able to copy across the new commonName value upon a call to Update 😬

cc @joshvanl

[masa213f]

@munnerz
Thank you for your reply.

Yes. I just faced the following message.

apiVersion: v1
items:
- apiVersion: cert-manager.io/v1alpha2
  kind: CertificateRequest
...
  status:
    conditions:
    - lastTransitionTime: "2019-11-15T03:31:52Z"
      message: 'The CSR PEM requests a commonName that is not present in the list
        of dnsNames. If a commonName is set, ACME requires that the value is also
        present in the list of dnsNames: "<FQDN>" does
        not exist in []'
      reason: Failed
      status: "False"
      type: Ready
    failureTime: "2019-11-15T03:31:52Z"
...

I fixed our Certificate definitions, so we are okay.

However, I think, it is better to keep the backward compatibility OR to document this behavior. :smile:

[munnerz]

Yes, we should definitely document this behaviour/restriction!

Maintaining backwards compatibility is harder than it sounds, because the Certificate controller is responsible for generate the actual CSR.

The generated CSR directly matches the information specified on the Certificate resource (i.e. uses the specified commonName/dnsNames).

Let's Encrypt does not utilise the commonName field, and if it is present on the CSR, it requires that the domain is listed in spec.dnsNames.

Prior to 0.11, the ACME implementation itself was responsible for generating its own CSRs - because of this change, we now also enforce this restriction in our own API (and thus, across all Issuer types).

We could consider removing this restriction, and instead modifying the ACME certificaterequest controller to regenerate the CSR if this case occurs and iff the cert-manager.io/private-key-secret-name annotation is set (https://github.com/jetstack/cert-manager/blob/master/design/20190708.certificate-request-crd.md#certificaterequest-annotations).

This would mean we can relax the restriction and provide a seamless experience for users in these cases.

cc @JoshVanL what do you think of this idea? We'd then want to consider https://github.com/jetstack/cert-manager/issues/2289 too, in order to allow the ACME implementation to hard fail in these cases when the private-key-secret-name annotation is not set.

[michaelbannister]

So if you set CommonName, it must be also listed as a dnsName

Just to note: seems like this is a change in behaviour since this comment https://github.com/jetstack/cert-manager/issues/267#issuecomment-358745891

Not sure if we missed this in some upgrade notes? We have certs that used to be fine which – when the secret gets renewed – are no longer considered valid by the api server:

spec:
  commonName: microservice-manager.platform.svc
  dnsNames:
  - microservice-manager.platform.svc.cluster.local
  issuerRef:
    kind: Issuer
    name: microservice-manager-selfsigned-issuer
  secretName: microservice-manager-certs

Error: certificate is valid for microservice-manager.platform.svc.cluster.local, not microservice-manager.platform.svc

[mnkg561]

I also faced the same issue. My commonName is no longer included in dnsNames. Is that intentional.? I'm using latest cert-manager 1.12

certificate is valid for iam-manager-webhook-service.iam-manager-system.svc.cluster.local, not iam-manager-webhook-service.iam-manager-system.svc

[munnerz]

Just to note: seems like this is a change in behaviour since this comment #267 (comment)

Correct, we previously did auto-promote this field but we don't any longer (since about 0.11).

After some more thought on this, I don't think defaulting the dnsNames field based on the contents of commonName as part of our API defaulting is a good idea.

Regarding regenerating CSRs specifically in the ACME CertificateRequest controller if the commonName is not a dnsName - this feels like a bit of a hack really as it breaks the contract of "you requested with this CSR and you got this Certificate".

The commonName field can be omitted altogether, and has been deprecated for >10y. I would prefer we encourage users to omit commonName rather than implement bespoke one-off logic to handle this stuff.

[munnerz]

Going to close this as working as intended 😄 I don't think we want to change this behaviour. Instead, things like ingress-shim make it easy to secure your Ingress resources and automatically configure Certificate resources correctly to account for this (i.e. it doesn't set commonName).