From d3a623314ca6d48f6d783a96304dec2e269c397c Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Wed, 19 Feb 2020 11:02:08 +0100 Subject: [PATCH 1/4] Add EmailSANs field Signed-off-by: Maartje Eyskens --- .../cert-manager/crds/certificates.yaml | 12 +++++ deploy/manifests/00-crds.yaml | 12 +++++ .../certmanager/v1alpha2/types_certificate.go | 5 ++ .../v1alpha2/zz_generated.deepcopy.go | 5 ++ .../certmanager/v1alpha3/types_certificate.go | 5 ++ .../v1alpha3/zz_generated.deepcopy.go | 5 ++ pkg/controller/webhookbootstrap/controller.go | 6 +++ .../apis/certmanager/types_certificate.go | 4 ++ .../v1alpha2/zz_generated.conversion.go | 2 + .../v1alpha3/zz_generated.conversion.go | 2 + .../certmanager/validation/certificate.go | 33 +++++++++++-- .../validation/certificate_test.go | 47 ++++++++++++++++++- .../apis/certmanager/zz_generated.deepcopy.go | 5 ++ pkg/util/pki/csr.go | 27 ++++++----- test/e2e/framework/helper/certificates.go | 4 ++ .../conformance/certificates/acme/acme.go | 2 + .../conformance/certificates/featureset.go | 4 ++ .../suite/conformance/certificates/suite.go | 26 ++++++++++ 18 files changed, 190 insertions(+), 16 deletions(-) diff --git a/deploy/charts/cert-manager/crds/certificates.yaml b/deploy/charts/cert-manager/crds/certificates.yaml index a57b3d835..b1a5390bc 100644 --- a/deploy/charts/cert-manager/crds/certificates.yaml +++ b/deploy/charts/cert-manager/crds/certificates.yaml @@ -96,6 +96,12 @@ spec: duration: description: Certificate default Duration type: string + emailSANs: + description: EmailSANs is a list of Email Subject Alternative Names + to be set on this Certificate. + type: array + items: + type: string ipAddresses: description: IPAddresses is a list of IP addresses to be used on the Certificate @@ -337,6 +343,12 @@ spec: duration: description: Certificate default Duration type: string + emailSANs: + description: EmailSANs is a list of Email Subject Alternative Names + to be set on this Certificate. + type: array + items: + type: string ipAddresses: description: IPAddresses is a list of IP addresses to be used on the Certificate diff --git a/deploy/manifests/00-crds.yaml b/deploy/manifests/00-crds.yaml index 5bd14dafe..195ea439d 100644 --- a/deploy/manifests/00-crds.yaml +++ b/deploy/manifests/00-crds.yaml @@ -302,6 +302,12 @@ spec: duration: description: Certificate default Duration type: string + emailSANs: + description: EmailSANs is a list of Email Subject Alternative Names + to be set on this Certificate. + type: array + items: + type: string ipAddresses: description: IPAddresses is a list of IP addresses to be used on the Certificate @@ -543,6 +549,12 @@ spec: duration: description: Certificate default Duration type: string + emailSANs: + description: EmailSANs is a list of Email Subject Alternative Names + to be set on this Certificate. + type: array + items: + type: string ipAddresses: description: IPAddresses is a list of IP addresses 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 ceb9914cf..d66dd3d50 100644 --- a/pkg/apis/certmanager/v1alpha2/types_certificate.go +++ b/pkg/apis/certmanager/v1alpha2/types_certificate.go @@ -109,6 +109,11 @@ type CertificateSpec struct { // +optional URISANs []string `json:"uriSANs,omitempty"` + // EmailSANs is a list of Email Subject Alternative Names to be set on this + // Certificate. + // +optional + EmailSANs []string `json:"emailSANs,omitempty"` + // SecretName is the name of the secret resource to store this secret in SecretName string `json:"secretName"` diff --git a/pkg/apis/certmanager/v1alpha2/zz_generated.deepcopy.go b/pkg/apis/certmanager/v1alpha2/zz_generated.deepcopy.go index 1ab249c59..952bf2034 100644 --- a/pkg/apis/certmanager/v1alpha2/zz_generated.deepcopy.go +++ b/pkg/apis/certmanager/v1alpha2/zz_generated.deepcopy.go @@ -312,6 +312,11 @@ func (in *CertificateSpec) DeepCopyInto(out *CertificateSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.EmailSANs != nil { + in, out := &in.EmailSANs, &out.EmailSANs + *out = make([]string, len(*in)) + copy(*out, *in) + } out.IssuerRef = in.IssuerRef if in.Usages != nil { in, out := &in.Usages, &out.Usages diff --git a/pkg/apis/certmanager/v1alpha3/types_certificate.go b/pkg/apis/certmanager/v1alpha3/types_certificate.go index ed371da1f..c9dcc8064 100644 --- a/pkg/apis/certmanager/v1alpha3/types_certificate.go +++ b/pkg/apis/certmanager/v1alpha3/types_certificate.go @@ -105,6 +105,11 @@ type CertificateSpec struct { // +optional URISANs []string `json:"uriSANs,omitempty"` + // EmailSANs is a list of Email Subject Alternative Names to be set on this + // Certificate. + // +optional + EmailSANs []string `json:"emailSANs,omitempty"` + // SecretName is the name of the secret resource to store this secret in SecretName string `json:"secretName"` diff --git a/pkg/apis/certmanager/v1alpha3/zz_generated.deepcopy.go b/pkg/apis/certmanager/v1alpha3/zz_generated.deepcopy.go index fb5b6ae23..dbabbf9ae 100644 --- a/pkg/apis/certmanager/v1alpha3/zz_generated.deepcopy.go +++ b/pkg/apis/certmanager/v1alpha3/zz_generated.deepcopy.go @@ -307,6 +307,11 @@ func (in *CertificateSpec) DeepCopyInto(out *CertificateSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.EmailSANs != nil { + in, out := &in.EmailSANs, &out.EmailSANs + *out = make([]string, len(*in)) + copy(*out, *in) + } out.IssuerRef = in.IssuerRef if in.Usages != nil { in, out := &in.Usages, &out.Usages diff --git a/pkg/controller/webhookbootstrap/controller.go b/pkg/controller/webhookbootstrap/controller.go index 0be79c82c..0c213a341 100644 --- a/pkg/controller/webhookbootstrap/controller.go +++ b/pkg/controller/webhookbootstrap/controller.go @@ -333,6 +333,12 @@ func (c *controller) certificateRequiresIssuance(ctx context.Context, log logr.L return true } + // validate the email addressed are correct + if !util.EqualUnsorted(cert.EmailAddresses, crt.Spec.EmailSANs) { + log.Info("certificate email addresses are not as expected, re-issuing") + return true + } + if c.certificateNeedsRenew(ctx, cert, crt) { log.Info("certificate requires renewal, re-issuing") return true diff --git a/pkg/internal/apis/certmanager/types_certificate.go b/pkg/internal/apis/certmanager/types_certificate.go index 015347106..bb53684d3 100644 --- a/pkg/internal/apis/certmanager/types_certificate.go +++ b/pkg/internal/apis/certmanager/types_certificate.go @@ -86,6 +86,10 @@ type CertificateSpec struct { // Certificate. URISANs []string + // EmailSANs is a list of Email Subject Alternative Names to be set on this + // Certificate. + EmailSANs []string + // SecretName is the name of the secret resource to store this secret in SecretName string diff --git a/pkg/internal/apis/certmanager/v1alpha2/zz_generated.conversion.go b/pkg/internal/apis/certmanager/v1alpha2/zz_generated.conversion.go index 51e8bdbd8..84c80d897 100644 --- a/pkg/internal/apis/certmanager/v1alpha2/zz_generated.conversion.go +++ b/pkg/internal/apis/certmanager/v1alpha2/zz_generated.conversion.go @@ -605,6 +605,7 @@ func autoConvert_v1alpha2_CertificateSpec_To_certmanager_CertificateSpec(in *v1a out.DNSNames = *(*[]string)(unsafe.Pointer(&in.DNSNames)) out.IPAddresses = *(*[]string)(unsafe.Pointer(&in.IPAddresses)) out.URISANs = *(*[]string)(unsafe.Pointer(&in.URISANs)) + out.EmailSANs = *(*[]string)(unsafe.Pointer(&in.EmailSANs)) out.SecretName = in.SecretName // TODO: Inefficient conversion - can we improve it? if err := s.Convert(&in.IssuerRef, &out.IssuerRef, 0); err != nil { @@ -634,6 +635,7 @@ func autoConvert_certmanager_CertificateSpec_To_v1alpha2_CertificateSpec(in *cer out.DNSNames = *(*[]string)(unsafe.Pointer(&in.DNSNames)) out.IPAddresses = *(*[]string)(unsafe.Pointer(&in.IPAddresses)) out.URISANs = *(*[]string)(unsafe.Pointer(&in.URISANs)) + out.EmailSANs = *(*[]string)(unsafe.Pointer(&in.EmailSANs)) out.SecretName = in.SecretName // TODO: Inefficient conversion - can we improve it? if err := s.Convert(&in.IssuerRef, &out.IssuerRef, 0); err != nil { diff --git a/pkg/internal/apis/certmanager/v1alpha3/zz_generated.conversion.go b/pkg/internal/apis/certmanager/v1alpha3/zz_generated.conversion.go index eb83852ef..c63078cd2 100644 --- a/pkg/internal/apis/certmanager/v1alpha3/zz_generated.conversion.go +++ b/pkg/internal/apis/certmanager/v1alpha3/zz_generated.conversion.go @@ -576,6 +576,7 @@ func autoConvert_v1alpha3_CertificateSpec_To_certmanager_CertificateSpec(in *v1a out.DNSNames = *(*[]string)(unsafe.Pointer(&in.DNSNames)) out.IPAddresses = *(*[]string)(unsafe.Pointer(&in.IPAddresses)) out.URISANs = *(*[]string)(unsafe.Pointer(&in.URISANs)) + out.EmailSANs = *(*[]string)(unsafe.Pointer(&in.EmailSANs)) out.SecretName = in.SecretName // TODO: Inefficient conversion - can we improve it? if err := s.Convert(&in.IssuerRef, &out.IssuerRef, 0); err != nil { @@ -602,6 +603,7 @@ func autoConvert_certmanager_CertificateSpec_To_v1alpha3_CertificateSpec(in *cer out.DNSNames = *(*[]string)(unsafe.Pointer(&in.DNSNames)) out.IPAddresses = *(*[]string)(unsafe.Pointer(&in.IPAddresses)) out.URISANs = *(*[]string)(unsafe.Pointer(&in.URISANs)) + out.EmailSANs = *(*[]string)(unsafe.Pointer(&in.EmailSANs)) out.SecretName = in.SecretName // TODO: Inefficient conversion - can we improve it? if err := s.Convert(&in.IssuerRef, &out.IssuerRef, 0); err != nil { diff --git a/pkg/internal/apis/certmanager/validation/certificate.go b/pkg/internal/apis/certmanager/validation/certificate.go index dbd1c4af6..efa4e29d3 100644 --- a/pkg/internal/apis/certmanager/validation/certificate.go +++ b/pkg/internal/apis/certmanager/validation/certificate.go @@ -19,6 +19,7 @@ package validation import ( "fmt" "net" + "net/mail" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" @@ -39,9 +40,8 @@ func ValidateCertificateSpec(crt *cmapi.CertificateSpec, fldPath *field.Path) fi el = append(el, validateIssuerRef(crt.IssuerRef, fldPath)...) - if len(crt.CommonName) == 0 && len(crt.DNSNames) == 0 && len(crt.URISANs) == 0 { - el = append(el, field.Required(fldPath.Child("commonName", "dnsNames", "uriSANs"), - "at least one of commonName, dnsNames, or uriSANs must be set")) + if len(crt.CommonName) == 0 && len(crt.DNSNames) == 0 && len(crt.URISANs) == 0 && len(crt.EmailSANs) == 0 { + el = append(el, field.Invalid(fldPath, "", "at least one of commonName, dnsNames, uriSANs or emailSANs must be set")) } // if a common name has been specified, ensure it is no longer than 64 chars @@ -52,6 +52,15 @@ func ValidateCertificateSpec(crt *cmapi.CertificateSpec, fldPath *field.Path) fi if len(crt.IPAddresses) > 0 { el = append(el, validateIPAddresses(crt, fldPath)...) } + + if len(crt.EmailSANs) > 0 { + el = append(el, validateEmailAddresses(crt, fldPath)...) + } + + if crt.KeySize < 0 { + el = append(el, field.Invalid(fldPath.Child("keySize"), crt.KeySize, "cannot be less than zero")) + } + switch crt.KeyAlgorithm { case cmapi.KeyAlgorithm(""): case cmapi.RSAKeyAlgorithm: @@ -113,6 +122,24 @@ func validateIPAddresses(a *cmapi.CertificateSpec, fldPath *field.Path) field.Er return el } +func validateEmailAddresses(a *cmapi.CertificateSpec, fldPath *field.Path) field.ErrorList { + if len(a.EmailSANs) <= 0 { + return nil + } + el := field.ErrorList{} + for i, d := range a.EmailSANs { + e, err := mail.ParseAddress(d) + if err != nil { + el = append(el, field.Invalid(fldPath.Child("emailSANs").Index(i), d, "invalid email address")) + } else if e.Address != d { + // Go accepts email names as per RFC 5322 (name ) + // This checks if the supplied value only contains the email address and nothing else + el = append(el, field.Invalid(fldPath.Child("emailSANs").Index(i), d, "invalid email address")) + } + } + return el +} + func validateUsages(a *cmapi.CertificateSpec, fldPath *field.Path) field.ErrorList { el := field.ErrorList{} for i, u := range a.Usages { diff --git a/pkg/internal/apis/certmanager/validation/certificate_test.go b/pkg/internal/apis/certmanager/validation/certificate_test.go index 0beb75cb3..4b47a0852 100644 --- a/pkg/internal/apis/certmanager/validation/certificate_test.go +++ b/pkg/internal/apis/certmanager/validation/certificate_test.go @@ -125,7 +125,7 @@ func TestValidateCertificate(t *testing.T) { }, }, errs: []*field.Error{ - field.Required(fldPath.Child("commonName", "dnsNames", "uriSANs"), "at least one of commonName, dnsNames, or uriSANs must be set"), + field.Invalid(fldPath, "", "at least one of commonName, dnsNames, uriSANs or emailSANs must be set"), }, }, "certificate with no issuerRef": { @@ -413,6 +413,51 @@ func TestValidateCertificate(t *testing.T) { }, }, }, + "valid certificate with only email SAN": { + cfg: &cmapi.Certificate{ + Spec: cmapi.CertificateSpec{ + EmailSANs: []string{"alice@example.com"}, + SecretName: "abc", + IssuerRef: validIssuerRef, + }, + }, + }, + "invalid certificate with incorrect email": { + cfg: &cmapi.Certificate{ + Spec: cmapi.CertificateSpec{ + EmailSANs: []string{"aliceexample.com"}, + SecretName: "abc", + IssuerRef: validIssuerRef, + }, + }, + errs: []*field.Error{ + field.Invalid(fldPath.Child("emailSANs").Index(0), "aliceexample.com", "invalid email address"), + }, + }, + "invalid certificate with email formatted with name": { + cfg: &cmapi.Certificate{ + Spec: cmapi.CertificateSpec{ + EmailSANs: []string{"Alice "}, + SecretName: "abc", + IssuerRef: validIssuerRef, + }, + }, + errs: []*field.Error{ + field.Invalid(fldPath.Child("emailSANs").Index(0), "Alice ", "invalid email address"), + }, + }, + "invalid certificate with email formatted with mailto": { + cfg: &cmapi.Certificate{ + Spec: cmapi.CertificateSpec{ + EmailSANs: []string{"mailto:alice@example.com"}, + SecretName: "abc", + IssuerRef: validIssuerRef, + }, + }, + errs: []*field.Error{ + field.Invalid(fldPath.Child("emailSANs").Index(0), "mailto:alice@example.com", "invalid email address"), + }, + }, } for n, s := range scenarios { t.Run(n, func(t *testing.T) { diff --git a/pkg/internal/apis/certmanager/zz_generated.deepcopy.go b/pkg/internal/apis/certmanager/zz_generated.deepcopy.go index c61b8c754..f96bbcffa 100644 --- a/pkg/internal/apis/certmanager/zz_generated.deepcopy.go +++ b/pkg/internal/apis/certmanager/zz_generated.deepcopy.go @@ -307,6 +307,11 @@ func (in *CertificateSpec) DeepCopyInto(out *CertificateSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.EmailSANs != nil { + in, out := &in.EmailSANs, &out.EmailSANs + *out = make([]string, len(*in)) + copy(*out, *in) + } out.IssuerRef = in.IssuerRef if in.Usages != nil { in, out := &in.Usages, &out.Usages diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index 7767d42fc..ed7883a07 100644 --- a/pkg/util/pki/csr.go +++ b/pkg/util/pki/csr.go @@ -212,9 +212,10 @@ func GenerateCSR(crt *v1alpha2.Certificate) (*x509.CertificateRequest, error) { SerialNumber: subject.SerialNumber, CommonName: commonName, }, - DNSNames: dnsNames, - IPAddresses: iPAddresses, - URIs: uriNames, + DNSNames: dnsNames, + IPAddresses: iPAddresses, + URIs: uriNames, + EmailAddresses: crt.Spec.EmailSANs, // TODO: work out how best to handle extensions/key usages here ExtraExtensions: []pkix.Extension{}, }, nil @@ -271,10 +272,11 @@ func GenerateTemplate(crt *v1alpha2.Certificate) (*x509.Certificate, error) { NotBefore: time.Now(), NotAfter: time.Now().Add(certDuration), // see http://golang.org/pkg/crypto/x509/#KeyUsage - KeyUsage: keyUsages, - ExtKeyUsage: extKeyUsages, - DNSNames: dnsNames, - IPAddresses: ipAddresses, + KeyUsage: keyUsages, + ExtKeyUsage: extKeyUsages, + DNSNames: dnsNames, + IPAddresses: ipAddresses, + EmailAddresses: crt.Spec.EmailSANs, }, nil } @@ -328,11 +330,12 @@ func GenerateTemplateFromCSRPEMWithUsages(csrPEM []byte, duration time.Duration, NotBefore: time.Now(), NotAfter: time.Now().Add(duration), // see http://golang.org/pkg/crypto/x509/#KeyUsage - KeyUsage: keyUsage, - ExtKeyUsage: extKeyUsage, - DNSNames: csr.DNSNames, - IPAddresses: csr.IPAddresses, - URIs: csr.URIs, + KeyUsage: keyUsage, + ExtKeyUsage: extKeyUsage, + DNSNames: csr.DNSNames, + IPAddresses: csr.IPAddresses, + EmailAddresses: csr.EmailAddresses, + URIs: csr.URIs, }, nil } diff --git a/test/e2e/framework/helper/certificates.go b/test/e2e/framework/helper/certificates.go index de4131792..11c43132f 100644 --- a/test/e2e/framework/helper/certificates.go +++ b/test/e2e/framework/helper/certificates.go @@ -213,6 +213,10 @@ func (h *Helper) ValidateIssuedCertificate(certificate *cmapi.Certificate, rootC apiutil.ExtKeyUsageStrings(certificateExtKeyUsages), apiutil.ExtKeyUsageStrings(cert.ExtKeyUsage)) } + if !util.EqualUnsorted(cert.EmailAddresses, certificate.Spec.EmailSANs) { + return nil, fmt.Errorf("certificate doesn't contain Email SANs: exp=%v got=%v", certificate.Spec.EmailSANs, cert.EmailAddresses) + } + var dnsName string if len(expectedDNSNames) > 0 { dnsName = expectedDNSNames[0] diff --git a/test/e2e/suite/conformance/certificates/acme/acme.go b/test/e2e/suite/conformance/certificates/acme/acme.go index f367f0858..49a67647e 100644 --- a/test/e2e/suite/conformance/certificates/acme/acme.go +++ b/test/e2e/suite/conformance/certificates/acme/acme.go @@ -54,6 +54,7 @@ func runACMEIssuerTests(eab *cmacme.ACMEExternalAccountBinding) { certificates.URISANsFeature, certificates.CommonNameFeature, certificates.KeyUsagesFeature, + certificates.EmailSANsFeature, ) // unsupportedDNS01Features is a list of features that are not supported by the ACME @@ -64,6 +65,7 @@ func runACMEIssuerTests(eab *cmacme.ACMEExternalAccountBinding) { certificates.URISANsFeature, certificates.CommonNameFeature, certificates.KeyUsagesFeature, + certificates.EmailSANsFeature, ) provisionerHTTP01 := &acmeIssuerProvisioner{ diff --git a/test/e2e/suite/conformance/certificates/featureset.go b/test/e2e/suite/conformance/certificates/featureset.go index 08b1a83f4..62dfc9825 100644 --- a/test/e2e/suite/conformance/certificates/featureset.go +++ b/test/e2e/suite/conformance/certificates/featureset.go @@ -103,6 +103,10 @@ const ( // that includes a URISANs. ACME providers do not support this. URISANsFeature Feature = "URISANs" + // EmailSANs denotes whether to the target issuer is able to sign a certificate + // that includes a EmailSANs. + EmailSANsFeature Feature = "EmailSANs" + // CommonName denotes whether the target issuer is able to sign certificates // with a distinct CommonName. This is useful for issuers such as ACME // providers that ignore, or otherwise have special requirements for the diff --git a/test/e2e/suite/conformance/certificates/suite.go b/test/e2e/suite/conformance/certificates/suite.go index 258ded81f..a6839c963 100644 --- a/test/e2e/suite/conformance/certificates/suite.go +++ b/test/e2e/suite/conformance/certificates/suite.go @@ -242,6 +242,32 @@ func (s *Suite) Define() { Expect(err).NotTo(HaveOccurred()) }) + It("should issue a certificate that defines a Common Name and Email Address", func() { + s.checkFeatures(CommonNameFeature) + s.checkFeatures(EmailSANsFeature) + + testCertificate := &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testcert", + Namespace: f.Namespace.Name, + }, + Spec: cmapi.CertificateSpec{ + SecretName: "testcert-tls", + CommonName: "test-common-name", + EmailSANs: []string{"alice@example.com"}, + IssuerRef: issuerRef, + }, + } + 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 CommonName and URI SAN", func() { s.checkFeatures(URISANsFeature) s.checkFeatures(CommonNameFeature) From 96b54bfdac75aed3087e330faef0169a2718e954 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Tue, 3 Mar 2020 11:24:11 +0100 Subject: [PATCH 2/4] Implement feedback Signed-off-by: Maartje Eyskens --- pkg/internal/apis/certmanager/validation/certificate.go | 8 ++------ test/e2e/suite/conformance/certificates/suite.go | 4 +--- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/pkg/internal/apis/certmanager/validation/certificate.go b/pkg/internal/apis/certmanager/validation/certificate.go index efa4e29d3..a5705852b 100644 --- a/pkg/internal/apis/certmanager/validation/certificate.go +++ b/pkg/internal/apis/certmanager/validation/certificate.go @@ -57,10 +57,6 @@ func ValidateCertificateSpec(crt *cmapi.CertificateSpec, fldPath *field.Path) fi el = append(el, validateEmailAddresses(crt, fldPath)...) } - if crt.KeySize < 0 { - el = append(el, field.Invalid(fldPath.Child("keySize"), crt.KeySize, "cannot be less than zero")) - } - switch crt.KeyAlgorithm { case cmapi.KeyAlgorithm(""): case cmapi.RSAKeyAlgorithm: @@ -130,11 +126,11 @@ func validateEmailAddresses(a *cmapi.CertificateSpec, fldPath *field.Path) field for i, d := range a.EmailSANs { e, err := mail.ParseAddress(d) if err != nil { - el = append(el, field.Invalid(fldPath.Child("emailSANs").Index(i), d, "invalid email address")) + el = append(el, field.Invalid(fldPath.Child("emailSANs").Index(i), d, fmt.Sprintf("invalid email address: %s", err))) } else if e.Address != d { // Go accepts email names as per RFC 5322 (name ) // This checks if the supplied value only contains the email address and nothing else - el = append(el, field.Invalid(fldPath.Child("emailSANs").Index(i), d, "invalid email address")) + el = append(el, field.Invalid(fldPath.Child("emailSANs").Index(i), d, "invalid email address: make sure the supplied value only contains the email address itself")) } } return el diff --git a/test/e2e/suite/conformance/certificates/suite.go b/test/e2e/suite/conformance/certificates/suite.go index a6839c963..59dbd2d4f 100644 --- a/test/e2e/suite/conformance/certificates/suite.go +++ b/test/e2e/suite/conformance/certificates/suite.go @@ -242,8 +242,7 @@ func (s *Suite) Define() { Expect(err).NotTo(HaveOccurred()) }) - It("should issue a certificate that defines a Common Name and Email Address", func() { - s.checkFeatures(CommonNameFeature) + It("should issue a certificate that defines an Email Address", func() { s.checkFeatures(EmailSANsFeature) testCertificate := &cmapi.Certificate{ @@ -253,7 +252,6 @@ func (s *Suite) Define() { }, Spec: cmapi.CertificateSpec{ SecretName: "testcert-tls", - CommonName: "test-common-name", EmailSANs: []string{"alice@example.com"}, IssuerRef: issuerRef, }, From 267ac50ff04f916687752818baf64a491ecf8099 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Tue, 3 Mar 2020 13:44:22 +0100 Subject: [PATCH 3/4] Update unit tests fir new errors Signed-off-by: Maartje Eyskens --- .../apis/certmanager/validation/certificate_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/internal/apis/certmanager/validation/certificate_test.go b/pkg/internal/apis/certmanager/validation/certificate_test.go index 4b47a0852..6ca6e38c1 100644 --- a/pkg/internal/apis/certmanager/validation/certificate_test.go +++ b/pkg/internal/apis/certmanager/validation/certificate_test.go @@ -431,7 +431,7 @@ func TestValidateCertificate(t *testing.T) { }, }, errs: []*field.Error{ - field.Invalid(fldPath.Child("emailSANs").Index(0), "aliceexample.com", "invalid email address"), + field.Invalid(fldPath.Child("emailSANs").Index(0), "aliceexample.com", "invalid email address: mail: missing '@' or angle-addr"), }, }, "invalid certificate with email formatted with name": { @@ -443,7 +443,7 @@ func TestValidateCertificate(t *testing.T) { }, }, errs: []*field.Error{ - field.Invalid(fldPath.Child("emailSANs").Index(0), "Alice ", "invalid email address"), + field.Invalid(fldPath.Child("emailSANs").Index(0), "Alice ", "invalid email address: make sure the supplied value only contains the email address itself"), }, }, "invalid certificate with email formatted with mailto": { @@ -455,7 +455,7 @@ func TestValidateCertificate(t *testing.T) { }, }, errs: []*field.Error{ - field.Invalid(fldPath.Child("emailSANs").Index(0), "mailto:alice@example.com", "invalid email address"), + field.Invalid(fldPath.Child("emailSANs").Index(0), "mailto:alice@example.com", "invalid email address: mail: expected comma"), }, }, } From 1c27fcb8d94b56e55b949f00575457b1a30c2cf5 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Tue, 3 Mar 2020 15:02:51 +0100 Subject: [PATCH 4/4] Fix CSR validation for Email SANs Signed-off-by: Maartje Eyskens --- pkg/util/pki/csr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index ed7883a07..12c394050 100644 --- a/pkg/util/pki/csr.go +++ b/pkg/util/pki/csr.go @@ -188,8 +188,8 @@ func GenerateCSR(crt *v1alpha2.Certificate) (*x509.CertificateRequest, error) { return nil, err } - if len(commonName) == 0 && len(dnsNames) == 0 && len(uriNames) == 0 { - return nil, fmt.Errorf("no common name, DNS name, or URI SAN specified on certificate") + if len(commonName) == 0 && len(dnsNames) == 0 && len(uriNames) == 0 && len(crt.Spec.EmailSANs) == 0 { + return nil, fmt.Errorf("no common name, DNS name, URI SAN, or Email SAN specified on certificate") } pubKeyAlgo, sigAlgo, err := SignatureAlgorithm(crt)