diff --git a/deploy/manifests/00-crds.yaml b/deploy/manifests/00-crds.yaml index e2f5b4210..237dd9d5a 100644 --- a/deploy/manifests/00-crds.yaml +++ b/deploy/manifests/00-crds.yaml @@ -1825,7 +1825,9 @@ spec: metadata: type: object spec: - description: CertificateSpec defines the desired state of Certificate + description: CertificateSpec defines the desired state of Certificate. A + valid Certificate requires at least one of a CommonName, DNSName, or URISAN + to be valid. properties: commonName: description: CommonName is a common name to be used on the Certificate. diff --git a/pkg/apis/certmanager/v1alpha2/types_certificate.go b/pkg/apis/certmanager/v1alpha2/types_certificate.go index 058203db9..b453dd504 100644 --- a/pkg/apis/certmanager/v1alpha2/types_certificate.go +++ b/pkg/apis/certmanager/v1alpha2/types_certificate.go @@ -68,11 +68,10 @@ const ( PKCS8 KeyEncoding = "pkcs8" ) -// CertificateSpec defines the desired state of Certificate +// CertificateSpec defines the desired state of Certificate. +// A valid Certificate requires at least one of a CommonName, DNSName, or +// URISAN to be valid. type CertificateSpec struct { - // A valid Certificate requires at least one of a CommonName, DNSName, or - // URISAN to be valid. - // 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. diff --git a/pkg/controller/certificaterequests/acme/acme.go b/pkg/controller/certificaterequests/acme/acme.go index c1b36df61..ef2f23caf 100644 --- a/pkg/controller/certificaterequests/acme/acme.go +++ b/pkg/controller/certificaterequests/acme/acme.go @@ -96,12 +96,11 @@ func (a *ACME) Sign(ctx context.Context, cr *v1alpha2.CertificateRequest, issuer // If the CommonName is also not present in the DNS names of the CSR then hard fail. if len(csr.Subject.CommonName) > 0 && !util.Contains(csr.DNSNames, csr.Subject.CommonName) { err = fmt.Errorf("%q does not exist in %s", csr.Subject.CommonName, csr.DNSNames) - message := fmt.Sprintf("Requested certificate contains CommonName not present in the requested DNS Subject Alternative Names: %s", - err) + 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" - a.reporter.Failed(cr, err, "OrderBuildingError", message) + a.reporter.Failed(cr, err, "InvalidOrder", message) - log.Error(err, message) + log.V(4).Info(fmt.Sprintf("%s: %s", message, err)) return nil, nil } diff --git a/pkg/controller/certificaterequests/acme/acme_test.go b/pkg/controller/certificaterequests/acme/acme_test.go index 6a88855ce..6eb633582 100644 --- a/pkg/controller/certificaterequests/acme/acme_test.go +++ b/pkg/controller/certificaterequests/acme/acme_test.go @@ -50,15 +50,15 @@ var ( fixedClock = fakeclock.NewFakeClock(fixedClockStart) ) -func generateCSR(t *testing.T, secretKey crypto.Signer, extraDNSNames ...string) []byte { +func generateCSR(t *testing.T, secretKey crypto.Signer, commonName string, dnsNames ...string) []byte { // The CommonName of the certificate request must also be present in the DNS // Names. template := x509.CertificateRequest{ Subject: pkix.Name{ - CommonName: "example.com", + CommonName: commonName, }, SignatureAlgorithm: x509.SHA256WithRSA, - DNSNames: append(extraDNSNames, "foo.com"), + DNSNames: dnsNames, } csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &template, secretKey) @@ -83,8 +83,8 @@ func TestSign(t *testing.T) { t.FailNow() } - csrPEM := generateCSR(t, sk, "example.com") - csrPEMExampleNotPresent := generateCSR(t, sk) + csrPEM := generateCSR(t, sk, "example.com", "example.com", "foo.com") + csrPEMExampleNotPresent := generateCSR(t, sk, "example.com", "foo.com") baseCR := gen.CertificateRequest("test-cr", gen.SetCertificateRequestCSR(csrPEM), @@ -160,7 +160,7 @@ func TestSign(t *testing.T) { builder: &testpkg.Builder{ CertManagerObjects: []runtime.Object{baseCR.DeepCopy(), baseIssuer.DeepCopy()}, ExpectedEvents: []string{ - `Warning OrderBuildingError Requested certificate contains CommonName not present in the requested DNS Subject Alternative Names: "example.com" does not exist in [foo.com]: "example.com" does not exist in [foo.com]`, + `Warning InvalidOrder 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: "example.com" does not exist in [foo.com]`, }, ExpectedActions: []testpkg.Action{ testpkg.NewAction(coretesting.NewUpdateAction( @@ -172,7 +172,7 @@ func TestSign(t *testing.T) { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonFailed, - Message: `Requested certificate contains CommonName not present in the requested DNS Subject Alternative Names: "example.com" does not exist in [foo.com]: "example.com" does not exist in [foo.com]`, + 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: "example.com" does not exist in [foo.com]`, LastTransitionTime: &metaFixedClockStart, }), gen.SetCertificateRequestFailureTime(metaFixedClockStart), diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index b8db325f1..4331eb64e 100644 --- a/pkg/util/pki/csr.go +++ b/pkg/util/pki/csr.go @@ -48,25 +48,42 @@ func IPAddressesForCertificate(crt *v1alpha2.Certificate) []net.IP { } func URIsForCertificate(crt *v1alpha2.Certificate) ([]*url.URL, error) { - var uris []*url.URL + uris, err := URLsFromStrings(crt.Spec.URISANs) + if err != nil { + return nil, fmt.Errorf("failed to parse URIs: %s", err) + } + + return uris, nil +} + +func DNSNamesForCertificate(crt *v1alpha2.Certificate) ([]string, error) { + _, err := URLsFromStrings(crt.Spec.DNSNames) + if err != nil { + return nil, fmt.Errorf("failed to parse DNSNames: %s", err) + } + + return crt.Spec.DNSNames, nil +} + +func URLsFromStrings(urlStrs []string) ([]*url.URL, error) { + var urls []*url.URL var errs []string - for _, uriStr := range crt.Spec.URISANs { - uri, err := url.Parse(uriStr) + for _, urlStr := range urlStrs { + url, err := url.Parse(urlStr) if err != nil { errs = append(errs, err.Error()) continue } - uris = append(uris, uri) + urls = append(urls, url) } if len(errs) > 0 { - return nil, fmt.Errorf("failed to parse URIs: %s", - strings.Join(errs, ", ")) + return nil, errors.New(strings.Join(errs, ", ")) } - return uris, nil + return urls, nil } func IPAddressesToString(ipAddresses []net.IP) []string { @@ -148,9 +165,14 @@ func buildUsages(usages []v1alpha2.KeyUsage, isCA bool) (ku x509.KeyUsage, eku [ // to the x509.CreateCertificateRequest function. func GenerateCSR(crt *v1alpha2.Certificate) (*x509.CertificateRequest, error) { commonName := crt.Spec.CommonName - dnsNames := crt.Spec.DNSNames iPAddresses := IPAddressesForCertificate(crt) organization := OrganizationForCertificate(crt) + + dnsNames, err := DNSNamesForCertificate(crt) + if err != nil { + return nil, err + } + uriNames, err := URIsForCertificate(crt) if err != nil { return nil, err diff --git a/test/e2e/suite/conformance/certificates/suite.go b/test/e2e/suite/conformance/certificates/suite.go index 6439eabe0..5d2151655 100644 --- a/test/e2e/suite/conformance/certificates/suite.go +++ b/test/e2e/suite/conformance/certificates/suite.go @@ -294,6 +294,31 @@ func (s *Suite) Define() { Expect(err).NotTo(HaveOccurred()) }) + It("should issue a certificate that defines a distinct DNS Name and another distinct Common Name", func() { + s.checkFeatures(CommonNameFeature) + + testCertificate := &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testcert", + Namespace: f.Namespace.Name, + }, + Spec: cmapi.CertificateSpec{ + SecretName: "testcert-tls", + CommonName: s.newDomain(), + IssuerRef: issuerRef, + DNSNames: []string{s.newDomain()}, + }, + } + + By("Creating a Certificate") + err := f.CRClient.Create(ctx, testCertificate) + Expect(err).NotTo(HaveOccurred()) + + By("Waiting for the Certificate to be issued...") + err = f.Helper().WaitCertificateIssuedValid(f.Namespace.Name, "testcert", time.Minute*5) + Expect(err).NotTo(HaveOccurred()) + }) + It("should issue a certificate that defines a DNS Name and sets a duration", func() { s.checkFeatures(DurationFeature) diff --git a/test/e2e/suite/conformance/certificates/venafi/venafi.go b/test/e2e/suite/conformance/certificates/venafi/venafi.go index 2424e2e85..380b9f340 100644 --- a/test/e2e/suite/conformance/certificates/venafi/venafi.go +++ b/test/e2e/suite/conformance/certificates/venafi/venafi.go @@ -38,10 +38,6 @@ var _ = framework.ConformanceDescribe("Certificates", func() { // key or using the same private key multiple times. certificates.ECDSAFeature, certificates.ReusePrivateKeyFeature, - - // Venafi does not support signing a certificate with only URISANs and an - // empty common name. - certificates.URISANsFeature, ) provisioner := new(venafiProvisioner)