diff --git a/deploy/manifests/00-crds.yaml b/deploy/manifests/00-crds.yaml index 902d99313..237dd9d5a 100644 --- a/deploy/manifests/00-crds.yaml +++ b/deploy/manifests/00-crds.yaml @@ -1825,21 +1825,18 @@ 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. - If no CommonName is given, then the first entry in DNSNames is used - as the CommonName. The CommonName should have a length of 64 characters - or fewer to avoid generating invalid CSRs; in order to have longer - domain names, set the CommonName (or first DNSNames entry) to have - 64 characters or fewer, and then add the longer domain name to DNSNames. + The CommonName should have a length of 64 characters or fewer to avoid + generating invalid CSRs. type: string dnsNames: description: DNSNames is a list of subject alt names to be used on the - Certificate. If no CommonName is given, then the first entry in DNSNames - is used as the CommonName and must have a length of 64 characters - or fewer. + Certificate. items: type: string type: array @@ -1912,6 +1909,12 @@ spec: description: SecretName is the name of the secret resource to store this secret in type: string + uriSANs: + description: URISANs is a list of URI Subject Alternative Names to be + set on this Certificate. + items: + type: string + type: array usages: description: Usages is the set of x509 actions that are enabled for a given key. Defaults are ('digital signature', 'key encipherment') diff --git a/docs/generated/reference/output/reference/api-docs/index.html b/docs/generated/reference/output/reference/api-docs/index.html index ad170196a..4c97f5e23 100755 --- a/docs/generated/reference/output/reference/api-docs/index.html +++ b/docs/generated/reference/output/reference/api-docs/index.html @@ -81,11 +81,11 @@ Appears In: commonName
string -CommonName is a common name to be used on the Certificate. If no CommonName is given, then the first entry in DNSNames is used as the CommonName. The CommonName should have a length of 64 characters or fewer to avoid generating invalid CSRs; in order to have longer domain names, set the CommonName (or first DNSNames entry) to have 64 characters or fewer, and then add the longer domain name to DNSNames. +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. dnsNames
string array -DNSNames is a list of subject alt names to be used on the Certificate. If no CommonName is given, then the first entry in DNSNames is used as the CommonName and must have a length of 64 characters or fewer. +DNSNames is a list of subject alt names to be used on the Certificate. duration
*Duration* @@ -128,6 +128,10 @@ Appears In: SecretName is the name of the secret resource to store this secret in +uriSANs
string array +URISANs is a list of URI Subject Alternative Names to be set on this Certificate. + + usages
string array Usages is the set of x509 actions that are enabled for a given key. Defaults are ('digital signature', 'key encipherment') if empty diff --git a/docs/tasks/issuing-certificates/index.rst b/docs/tasks/issuing-certificates/index.rst index 2f8b594c2..fd653c535 100644 --- a/docs/tasks/issuing-certificates/index.rst +++ b/docs/tasks/issuing-certificates/index.rst @@ -22,7 +22,8 @@ Certificates specify which issuer they want to obtain the certificate from by specifying the ``certificate.spec.issuerRef`` field. A basic Certificate resource, for the ``example.com`` and ``www.example.com`` -DNS names that is valid for 90d and renews 15d before expiry is below: +DNS names, ``spiffe://cluster.local/ns/sandbox/sa/example`` URI Subject +Alternative Name, that is valid for 90d and renews 15d before expiry is below: .. code-block:: yaml :linenos: @@ -41,6 +42,8 @@ DNS names that is valid for 90d and renews 15d before expiry is below: dnsNames: - example.com - www.example.com + uriSANs: + - spiffe://cluster.local/ns/sandbox/sa/example issuerRef: name: ca-issuer # We can reference ClusterIssuers by changing the kind here. diff --git a/pkg/apis/certmanager/v1alpha2/types.go b/pkg/apis/certmanager/v1alpha2/types.go index f47571481..9d81d9a18 100644 --- a/pkg/apis/certmanager/v1alpha2/types.go +++ b/pkg/apis/certmanager/v1alpha2/types.go @@ -20,6 +20,7 @@ package v1alpha2 const ( AltNamesAnnotationKey = "cert-manager.io/alt-names" IPSANAnnotationKey = "cert-manager.io/ip-sans" + URISANAnnotationKey = "cert-manager.io/uri-sans" CommonNameAnnotationKey = "cert-manager.io/common-name" IssuerNameAnnotationKey = "cert-manager.io/issuer-name" IssuerKindAnnotationKey = "cert-manager.io/issuer-kind" diff --git a/pkg/apis/certmanager/v1alpha2/types_certificate.go b/pkg/apis/certmanager/v1alpha2/types_certificate.go index d2894bd52..b453dd504 100644 --- a/pkg/apis/certmanager/v1alpha2/types_certificate.go +++ b/pkg/apis/certmanager/v1alpha2/types_certificate.go @@ -68,15 +68,13 @@ 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 { // CommonName is a common name to be used on the Certificate. - // If no CommonName is given, then the first entry in DNSNames is used as - // the CommonName. // The CommonName should have a length of 64 characters or fewer to avoid - // generating invalid CSRs; in order to have longer domain names, set the - // CommonName (or first DNSNames entry) to have 64 characters or fewer, - // and then add the longer domain name to DNSNames. + // generating invalid CSRs. // +optional CommonName string `json:"commonName,omitempty"` @@ -93,8 +91,6 @@ type CertificateSpec struct { RenewBefore *metav1.Duration `json:"renewBefore,omitempty"` // DNSNames is a list of subject alt names to be used on the Certificate. - // If no CommonName is given, then the first entry in DNSNames is used as - // the CommonName and must have a length of 64 characters or fewer. // +optional DNSNames []string `json:"dnsNames,omitempty"` @@ -102,6 +98,11 @@ type CertificateSpec struct { // +optional IPAddresses []string `json:"ipAddresses,omitempty"` + // URISANs is a list of URI Subject Alternative Names to be set on this + // Certificate. + // +optional + URISANs []string `json:"uriSANs,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 b6351a0ea..98be2da5f 100644 --- a/pkg/apis/certmanager/v1alpha2/zz_generated.deepcopy.go +++ b/pkg/apis/certmanager/v1alpha2/zz_generated.deepcopy.go @@ -302,6 +302,11 @@ func (in *CertificateSpec) DeepCopyInto(out *CertificateSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.URISANs != nil { + in, out := &in.URISANs, &out.URISANs + *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/certificaterequests/acme/BUILD.bazel b/pkg/controller/certificaterequests/acme/BUILD.bazel index 64d1575cd..74eff8bb9 100644 --- a/pkg/controller/certificaterequests/acme/BUILD.bazel +++ b/pkg/controller/certificaterequests/acme/BUILD.bazel @@ -17,6 +17,7 @@ go_library( "//pkg/controller/certificaterequests/util:go_default_library", "//pkg/issuer:go_default_library", "//pkg/logs:go_default_library", + "//pkg/util:go_default_library", "//pkg/util/pki:go_default_library", "@io_k8s_apimachinery//pkg/api/errors:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", diff --git a/pkg/controller/certificaterequests/acme/acme.go b/pkg/controller/certificaterequests/acme/acme.go index c8279eca8..ef2f23caf 100644 --- a/pkg/controller/certificaterequests/acme/acme.go +++ b/pkg/controller/certificaterequests/acme/acme.go @@ -38,6 +38,7 @@ import ( crutil "github.com/jetstack/cert-manager/pkg/controller/certificaterequests/util" issuerpkg "github.com/jetstack/cert-manager/pkg/issuer" logf "github.com/jetstack/cert-manager/pkg/logs" + "github.com/jetstack/cert-manager/pkg/util" "github.com/jetstack/cert-manager/pkg/util/pki" ) @@ -92,6 +93,18 @@ func (a *ACME) Sign(ctx context.Context, cr *v1alpha2.CertificateRequest, issuer return nil, nil } + // 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 := "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, "InvalidOrder", message) + + log.V(4).Info(fmt.Sprintf("%s: %s", message, err)) + + return nil, nil + } + // If we fail to build the order we have to hard fail. expectedOrder, err := buildOrder(cr, csr) if err != nil { diff --git a/pkg/controller/certificaterequests/acme/acme_test.go b/pkg/controller/certificaterequests/acme/acme_test.go index 1e65259e4..d265394f0 100644 --- a/pkg/controller/certificaterequests/acme/acme_test.go +++ b/pkg/controller/certificaterequests/acme/acme_test.go @@ -22,7 +22,6 @@ import ( "crypto/rand" "crypto/x509" "crypto/x509/pkix" - "encoding/asn1" "encoding/pem" "errors" "testing" @@ -51,13 +50,15 @@ var ( fixedClock = fakeclock.NewFakeClock(fixedClockStart) ) -func generateCSR(t *testing.T, secretKey crypto.Signer) []byte { - asn1Subj, _ := asn1.Marshal(pkix.Name{ - CommonName: "test", - }.ToRDNSequence()) +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{ - RawSubject: asn1Subj, + Subject: pkix.Name{ + CommonName: commonName, + }, SignatureAlgorithm: x509.SHA256WithRSA, + DNSNames: dnsNames, } csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &template, secretKey) @@ -82,8 +83,8 @@ func TestSign(t *testing.T) { t.FailNow() } - //skPEM := pki.EncodePKCS1PrivateKey(sk) - csrPEM := 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), @@ -152,6 +153,36 @@ func TestSign(t *testing.T) { }, }, + "if the common name is not present in the DNS names then should hard fail": { + certificateRequest: gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestCSR(csrPEMExampleNotPresent), + ), + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{baseCR.DeepCopy(), baseIssuer.DeepCopy()}, + ExpectedEvents: []string{ + `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.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificaterequests"), + "status", + gen.DefaultTestNamespace, + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestCSR(csrPEMExampleNotPresent), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + Reason: cmapi.CertificateRequestReasonFailed, + 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), + ), + )), + }, + }, + }, + //TODO: Think of a creative way to get `buildOrder` to fail :thinking_face: "if order doesn't exist then attempt to create one": { @@ -159,7 +190,7 @@ func TestSign(t *testing.T) { builder: &testpkg.Builder{ CertManagerObjects: []runtime.Object{baseCR.DeepCopy(), baseIssuer.DeepCopy()}, ExpectedEvents: []string{ - "Normal OrderCreated Created Order resource default-unit-test-ns/test-cr-1049712215", + "Normal OrderCreated Created Order resource default-unit-test-ns/test-cr-3921610499", }, ExpectedActions: []testpkg.Action{ testpkg.NewAction(coretesting.NewCreateAction( @@ -176,7 +207,7 @@ func TestSign(t *testing.T) { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, - Message: "Created Order resource default-unit-test-ns/test-cr-1049712215", + Message: "Created Order resource default-unit-test-ns/test-cr-3921610499", LastTransitionTime: &metaFixedClockStart, }), ), @@ -189,7 +220,7 @@ func TestSign(t *testing.T) { certificateRequest: baseCR.DeepCopy(), builder: &testpkg.Builder{ ExpectedEvents: []string{ - "Normal OrderGetError Failed to get order resource default-unit-test-ns/test-cr-1049712215: this is a network error", + "Normal OrderGetError Failed to get order resource default-unit-test-ns/test-cr-3921610499: this is a network error", }, CertManagerObjects: []runtime.Object{baseCR.DeepCopy(), baseIssuer.DeepCopy()}, ExpectedActions: []testpkg.Action{ @@ -202,7 +233,7 @@ func TestSign(t *testing.T) { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, - Message: "Failed to get order resource default-unit-test-ns/test-cr-1049712215: this is a network error", + Message: "Failed to get order resource default-unit-test-ns/test-cr-3921610499: this is a network error", LastTransitionTime: &metaFixedClockStart, }), ), @@ -225,7 +256,7 @@ func TestSign(t *testing.T) { certificateRequest: baseCR.DeepCopy(), builder: &testpkg.Builder{ ExpectedEvents: []string{ - `Warning OrderFailed Failed to wait for order resource default-unit-test-ns/test-cr-1049712215 to become ready: order is in "invalid" state`, + `Warning OrderFailed Failed to wait for order resource default-unit-test-ns/test-cr-3921610499 to become ready: order is in "invalid" state`, }, CertManagerObjects: []runtime.Object{baseCR.DeepCopy(), baseIssuer.DeepCopy(), gen.OrderFrom(baseOrder, @@ -242,7 +273,7 @@ func TestSign(t *testing.T) { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonFailed, - Message: `Failed to wait for order resource default-unit-test-ns/test-cr-1049712215 to become ready: order is in "invalid" state`, + Message: `Failed to wait for order resource default-unit-test-ns/test-cr-3921610499 to become ready: order is in "invalid" state`, LastTransitionTime: &metaFixedClockStart, }), gen.SetCertificateRequestFailureTime(metaFixedClockStart), @@ -256,7 +287,7 @@ func TestSign(t *testing.T) { certificateRequest: baseCR.DeepCopy(), builder: &testpkg.Builder{ ExpectedEvents: []string{ - `Normal OrderPending Waiting on certificate issuance from order default-unit-test-ns/test-cr-1049712215: "pending"`, + `Normal OrderPending Waiting on certificate issuance from order default-unit-test-ns/test-cr-3921610499: "pending"`, }, CertManagerObjects: []runtime.Object{baseCR.DeepCopy(), baseIssuer.DeepCopy(), gen.OrderFrom(baseOrder, @@ -273,7 +304,7 @@ func TestSign(t *testing.T) { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, - Message: `Waiting on certificate issuance from order default-unit-test-ns/test-cr-1049712215: "pending"`, + Message: `Waiting on certificate issuance from order default-unit-test-ns/test-cr-3921610499: "pending"`, LastTransitionTime: &metaFixedClockStart, }), ), diff --git a/pkg/controller/certificates/BUILD.bazel b/pkg/controller/certificates/BUILD.bazel index 0e852b215..1b23b9c81 100644 --- a/pkg/controller/certificates/BUILD.bazel +++ b/pkg/controller/certificates/BUILD.bazel @@ -42,13 +42,17 @@ go_library( go_test( name = "go_default_test", - srcs = ["sync_test.go"], + srcs = [ + "sync_test.go", + "util_test.go", + ], embed = [":go_default_library"], deps = [ "//pkg/api/util:go_default_library", "//pkg/apis/certmanager/v1alpha2:go_default_library", "//pkg/apis/meta/v1:go_default_library", "//pkg/controller/test:go_default_library", + "//pkg/util:go_default_library", "//pkg/util/pki:go_default_library", "//test/unit/gen:go_default_library", "@io_k8s_api//core/v1:go_default_library", diff --git a/pkg/controller/certificates/sync.go b/pkg/controller/certificates/sync.go index eaf0e5cc1..eed08beda 100644 --- a/pkg/controller/certificates/sync.go +++ b/pkg/controller/certificates/sync.go @@ -102,7 +102,12 @@ func (c *certificateRequestManager) updateCertificateStatus(ctx context.Context, var matches bool var matchErrs []string if key != nil && cert != nil { - matches, matchErrs = certificateMatchesSpec(crt, key, cert, c.secretLister) + secret, err := c.secretLister.Secrets(crt.Namespace).Get(crt.Spec.SecretName) + if err != nil { + return err + } + + matches, matchErrs = certificateMatchesSpec(crt, key, cert, secret) } isTempCert := isTemporaryCertificate(cert) @@ -255,7 +260,7 @@ func (c *certificateRequestManager) processCertificate(ctx context.Context, crt // a valid partner to the stored certificate. var matchErrs []string dbg.Info("checking if existing certificate stored in Secret resource is not expiring soon and matches certificate spec") - needsIssue, matchErrs, err = c.certificateRequiresIssuance(ctx, crt, existingKey, existingCert) + needsIssue, matchErrs, err = c.certificateRequiresIssuance(ctx, crt, existingKey, existingCert, existingSecret) if err != nil && !errors.IsInvalidData(err) { return err } @@ -330,7 +335,7 @@ func (c *certificateRequestManager) processCertificate(ctx context.Context, crt } // We don't issue a temporary certificate if the existing stored // certificate already 'matches', even if it isn't a temporary certificate. - matches, _ := certificateMatchesSpec(crt, privateKey, existingX509Cert, c.secretLister) + matches, _ := certificateMatchesSpec(crt, privateKey, existingX509Cert, existingSecret) if !matches { log.Info("existing certificate fields do not match certificate spec, issuing temporary certificate") return c.issueTemporaryCertificate(ctx, existingSecret, crt, existingKey) @@ -556,7 +561,7 @@ func (c *certificateRequestManager) issueTemporaryCertificate(ctx context.Contex return nil } -func (c *certificateRequestManager) certificateRequiresIssuance(ctx context.Context, crt *cmapi.Certificate, keyBytes, certBytes []byte) (bool, []string, error) { +func (c *certificateRequestManager) certificateRequiresIssuance(ctx context.Context, crt *cmapi.Certificate, keyBytes, certBytes []byte, secret *corev1.Secret) (bool, []string, error) { key, err := pki.DecodePrivateKeyBytes(keyBytes) if err != nil { return false, nil, err @@ -568,7 +573,7 @@ func (c *certificateRequestManager) certificateRequiresIssuance(ctx context.Cont if isTemporaryCertificate(cert) { return true, nil, nil } - matches, matchErrs := certificateMatchesSpec(crt, key, cert, c.secretLister) + matches, matchErrs := certificateMatchesSpec(crt, key, cert, secret) if !matches { return true, matchErrs, nil } @@ -790,6 +795,7 @@ func setSecretValues(ctx context.Context, crt *cmapi.Certificate, s *corev1.Secr delete(s.Annotations, cmapi.CommonNameAnnotationKey) delete(s.Annotations, cmapi.AltNamesAnnotationKey) delete(s.Annotations, cmapi.IPSANAnnotationKey) + delete(s.Annotations, cmapi.URISANAnnotationKey) } else { x509Cert, err := pki.DecodeX509CertificateBytes(data.cert) // TODO: handle InvalidData here? @@ -800,6 +806,7 @@ func setSecretValues(ctx context.Context, crt *cmapi.Certificate, s *corev1.Secr s.Annotations[cmapi.CommonNameAnnotationKey] = x509Cert.Subject.CommonName s.Annotations[cmapi.AltNamesAnnotationKey] = strings.Join(x509Cert.DNSNames, ",") s.Annotations[cmapi.IPSANAnnotationKey] = strings.Join(pki.IPAddressesToString(x509Cert.IPAddresses), ",") + s.Annotations[cmapi.URISANAnnotationKey] = strings.Join(pki.URLsToString(x509Cert.URIs), ",") } return nil diff --git a/pkg/controller/certificates/sync_test.go b/pkg/controller/certificates/sync_test.go index 2e0e034c3..4ac069cb6 100644 --- a/pkg/controller/certificates/sync_test.go +++ b/pkg/controller/certificates/sync_test.go @@ -692,7 +692,8 @@ func TestProcessCertificate(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -731,7 +732,8 @@ func TestProcessCertificate(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -787,7 +789,8 @@ func TestProcessCertificate(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -844,7 +847,8 @@ func TestProcessCertificate(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -874,7 +878,8 @@ func TestProcessCertificate(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -906,7 +911,8 @@ func TestProcessCertificate(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -936,7 +942,8 @@ func TestProcessCertificate(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -966,7 +973,8 @@ func TestProcessCertificate(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -1009,7 +1017,8 @@ func TestProcessCertificate(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -1204,7 +1213,8 @@ func TestTemporaryCertificateEnabled(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -1261,7 +1271,8 @@ func TestTemporaryCertificateEnabled(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -1318,7 +1329,8 @@ func TestTemporaryCertificateEnabled(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -1348,7 +1360,8 @@ func TestTemporaryCertificateEnabled(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -1378,7 +1391,8 @@ func TestTemporaryCertificateEnabled(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -1408,7 +1422,8 @@ func TestTemporaryCertificateEnabled(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -1438,7 +1453,8 @@ func TestTemporaryCertificateEnabled(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -1468,7 +1484,8 @@ func TestTemporaryCertificateEnabled(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -1501,7 +1518,8 @@ func TestTemporaryCertificateEnabled(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -1556,7 +1574,8 @@ func TestTemporaryCertificateEnabled(t *testing.T) { cmapi.IssuerNameAnnotationKey: exampleBundle1.certificate.Spec.IssuerRef.Name, cmapi.IPSANAnnotationKey: "", cmapi.AltNamesAnnotationKey: "example.com", - cmapi.CommonNameAnnotationKey: "example.com", + cmapi.CommonNameAnnotationKey: "", + cmapi.URISANAnnotationKey: "", }, }, Data: map[string][]byte{ @@ -1968,7 +1987,7 @@ func TestUpdateStatus(t *testing.T) { Type: cmapi.CertificateConditionReady, Status: cmmeta.ConditionFalse, Reason: "DoesNotMatch", - Message: "Common name on TLS certificate not up to date: \"notexample.com\", DNS names on TLS certificate not up to date: [\"notexample.com\"]", + Message: "DNS names on TLS certificate not up to date: [\"notexample.com\"]", LastTransitionTime: &metaFixedClockStart, }), ), diff --git a/pkg/controller/certificates/util.go b/pkg/controller/certificates/util.go index f9c3d62d5..e5ada1489 100644 --- a/pkg/controller/certificates/util.go +++ b/pkg/controller/certificates/util.go @@ -27,6 +27,7 @@ import ( "time" "github.com/kr/pretty" + corev1 "k8s.io/api/core/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" corelisters "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" @@ -75,7 +76,7 @@ func certificateGetter(lister cmlisters.CertificateLister) func(namespace, name var keyFunc = controllerpkg.KeyFunc -func certificateMatchesSpec(crt *v1alpha2.Certificate, key crypto.Signer, cert *x509.Certificate, secretLister corelisters.SecretLister) (bool, []string) { +func certificateMatchesSpec(crt *v1alpha2.Certificate, key crypto.Signer, cert *x509.Certificate, secret *corev1.Secret) (bool, []string) { var errs []string // TODO: add checks for KeySize, KeyAlgorithm fields @@ -83,6 +84,7 @@ func certificateMatchesSpec(crt *v1alpha2.Certificate, key crypto.Signer, cert * // TODO: add checks for IsCA field // check if the private key is the corresponding pair to the certificate + matches, err := pki.PublicKeyMatchesCertificate(key.Public(), cert) if err != nil { errs = append(errs, err.Error()) @@ -90,27 +92,34 @@ func certificateMatchesSpec(crt *v1alpha2.Certificate, key crypto.Signer, cert * errs = append(errs, fmt.Sprintf("Certificate private key does not match certificate")) } - // validate the common name is correct - expectedCN := pki.CommonNameForCertificate(crt) - if expectedCN != cert.Subject.CommonName { - errs = append(errs, fmt.Sprintf("Common name on TLS certificate not up to date: %q", cert.Subject.CommonName)) + // If CN is set on the resource then it should exist on the certificate as + // the Common Name or a DNS Name + expectedCN := crt.Spec.CommonName + gotCN := append(cert.DNSNames, cert.Subject.CommonName) + if len(expectedCN) > 0 && !util.Contains(gotCN, expectedCN) { + errs = append(errs, fmt.Sprintf("Common Name on TLS certificate not up to date (%q): %s", + expectedCN, gotCN)) } // validate the dns names are correct - expectedDNSNames := pki.DNSNamesForCertificate(crt) - if !util.EqualUnsorted(cert.DNSNames, expectedDNSNames) { + expectedDNSNames := crt.Spec.DNSNames + if !util.Subset(cert.DNSNames, expectedDNSNames) { errs = append(errs, fmt.Sprintf("DNS names on TLS certificate not up to date: %q", cert.DNSNames)) } + expectedURIs := crt.Spec.URISANs + if !util.EqualUnsorted(pki.URLsToString(cert.URIs), expectedURIs) { + errs = append(errs, fmt.Sprintf("URI SANs on TLS certificate not up to date: %q", cert.URIs)) + } + // validate the ip addresses are correct if !util.EqualUnsorted(pki.IPAddressesToString(cert.IPAddresses), crt.Spec.IPAddresses) { errs = append(errs, fmt.Sprintf("IP addresses on TLS certificate not up to date: %q", pki.IPAddressesToString(cert.IPAddresses))) } - // get a copy of the current secret resource - // Note that we already know that it exists, no need to check for errors - // TODO: Refactor so that the secret is passed as argument? - secret, err := secretLister.Secrets(crt.Namespace).Get(crt.Spec.SecretName) + if secret.Annotations == nil { + secret.Annotations = make(map[string]string) + } // validate that the issuer is correct if crt.Spec.IssuerRef.Name != secret.Annotations[v1alpha2.IssuerNameAnnotationKey] { diff --git a/pkg/controller/certificates/util_test.go b/pkg/controller/certificates/util_test.go new file mode 100644 index 000000000..ac18e5b95 --- /dev/null +++ b/pkg/controller/certificates/util_test.go @@ -0,0 +1,130 @@ +/* +Copyright 2019 The Jetstack cert-manager contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package certificates + +import ( + "testing" + "time" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" + cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" + "github.com/jetstack/cert-manager/pkg/util" + "github.com/jetstack/cert-manager/test/unit/gen" +) + +func TestCertificateMatchesSpec(t *testing.T) { + baseCert := gen.Certificate("test", + gen.SetCertificateIssuer(cmmeta.ObjectReference{Name: "ca-issuer", Kind: "Issuer", Group: "not-empty"}), + gen.SetCertificateSecretName("output"), + gen.SetCertificateRenewBefore(time.Hour*36), + ) + + exampleBundle := mustCreateCryptoBundle(t, gen.CertificateFrom(baseCert, + gen.SetCertificateDNSNames("a.example.com"), + gen.SetCertificateCommonName("common.name.example.com"), + gen.SetCertificateURIs("spiffe://cluster.local/ns/sandbox/sa/foo"), + )) + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + cmapi.IssuerNameAnnotationKey: "ca-issuer", + cmapi.IssuerKindAnnotationKey: "Issuer", + }, + }, + } + + type testT struct { + cb cryptoBundle + certificate *cmapi.Certificate + secret *corev1.Secret + expMatch bool + expErrors []string + } + + for name, test := range map[string]testT{ + "if all match then return matched": { + cb: exampleBundle, + certificate: exampleBundle.certificate, + expMatch: true, + expErrors: nil, + }, + + "if no common name but DNS and all match then return matched": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate, + gen.SetCertificateCommonName(""), + )), + certificate: gen.CertificateFrom(exampleBundle.certificate, + gen.SetCertificateCommonName(""), + ), + expMatch: true, + expErrors: nil, + }, + + "if common name empty but requested common name in DNS names then match": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate, + gen.SetCertificateDNSNames("a.example.com", "common.name.example.com"), + gen.SetCertificateCommonName(""), + )), + certificate: gen.CertificateFrom(exampleBundle.certificate), + expMatch: true, + expErrors: nil, + }, + + "if common name random string but requested common name in DNS names then match": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate, + gen.SetCertificateDNSNames("a.example.com", "common.name.example.com"), + gen.SetCertificateCommonName("foobar"), + )), + certificate: gen.CertificateFrom(exampleBundle.certificate), + expMatch: true, + expErrors: nil, + }, + + "if common name random string and no request DNS names but request common name then error missing common name": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate, + gen.SetCertificateDNSNames(), + gen.SetCertificateCommonName("foobar"), + )), + certificate: gen.CertificateFrom(exampleBundle.certificate), + expMatch: false, + expErrors: []string{ + `Common Name on TLS certificate not up to date ("common.name.example.com"): [foobar]`, + "DNS names on TLS certificate not up to date: []", + }, + }, + } { + t.Run(name, func(t *testing.T) { + match, errs := certificateMatchesSpec( + test.certificate, test.cb.privateKey, test.cb.cert, secret) + + if match != test.expMatch { + t.Errorf("got unexpected match bool, exp=%t got=%t", + test.expMatch, match) + } + + if !util.EqualSorted(test.expErrors, errs) { + t.Errorf("got unexpected errors, exp=%s got=%s", + test.expErrors, errs) + } + }) + } + +} diff --git a/pkg/controller/webhookbootstrap/controller.go b/pkg/controller/webhookbootstrap/controller.go index 5f4dbba0f..cc924004e 100644 --- a/pkg/controller/webhookbootstrap/controller.go +++ b/pkg/controller/webhookbootstrap/controller.go @@ -315,19 +315,27 @@ func (c *controller) certificateRequiresIssuance(ctx context.Context, log logr.L } // validate the common name is correct - expectedCN := pki.CommonNameForCertificate(crt) - if expectedCN != cert.Subject.CommonName { + expectedCN := crt.Spec.CommonName + gotCN := append(cert.DNSNames, cert.Subject.CommonName) + if !util.Contains(gotCN, expectedCN) { log.Info("certificate common name is not as expected, re-issuing") return true } // validate the dns names are correct - expectedDNSNames := pki.DNSNamesForCertificate(crt) - if !util.EqualUnsorted(cert.DNSNames, expectedDNSNames) { + expectedDNSNames := crt.Spec.DNSNames + if !util.Subset(cert.DNSNames, expectedDNSNames) { log.Info("certificate dns names are not as expected, re-issuing") return true } + // validate the uri sans are correct + expectedURINames := crt.Spec.URISANs + if !util.EqualUnsorted(pki.URLsToString(cert.URIs), expectedURINames) { + log.Info("certificate uri sans are not as expected, re-issuing") + return true + } + // validate the ip addresses are correct if !util.EqualUnsorted(pki.IPAddressesToString(cert.IPAddresses), crt.Spec.IPAddresses) { log.Info("certificate ip addresses are not as expected, re-issuing") diff --git a/pkg/internal/apis/certmanager/types_certificate.go b/pkg/internal/apis/certmanager/types_certificate.go index c748fc1ea..32f010ee9 100644 --- a/pkg/internal/apis/certmanager/types_certificate.go +++ b/pkg/internal/apis/certmanager/types_certificate.go @@ -59,13 +59,12 @@ const ( // CertificateSpec defines the desired state of Certificate 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. - // If no CommonName is given, then the first entry in DNSNames is used as - // the CommonName. // The CommonName should have a length of 64 characters or fewer to avoid - // generating invalid CSRs; in order to have longer domain names, set the - // CommonName (or first DNSNames entry) to have 64 characters or fewer, - // and then add the longer domain name to DNSNames. + // generating invalid CSRs. // +optional CommonName string `json:"commonName,omitempty"` @@ -82,8 +81,6 @@ type CertificateSpec struct { RenewBefore *metav1.Duration `json:"renewBefore,omitempty"` // DNSNames is a list of subject alt names to be used on the Certificate. - // If no CommonName is given, then the first entry in DNSNames is used as - // the CommonName and must have a length of 64 characters or fewer. // +optional DNSNames []string `json:"dnsNames,omitempty"` @@ -91,6 +88,11 @@ type CertificateSpec struct { // +optional IPAddresses []string `json:"ipAddresses,omitempty"` + // URISANs is a list of URI Subject Alternative Names to be set on this + // Certificate. + // +optional + URISANs []string `json:"uriSANs,omitempty"` + // SecretName is the name of the secret resource to store this secret in SecretName string `json:"secretName"` @@ -103,7 +105,7 @@ type CertificateSpec struct { IssuerRef cmmeta.ObjectReference `json:"issuerRef"` // IsCA will mark this Certificate as valid for signing. - // This implies that the 'signing' usage is set + // This implies that the 'cert sign' usage is set // +optional IsCA bool `json:"isCA,omitempty"` diff --git a/pkg/internal/apis/certmanager/v1alpha2/zz_generated.conversion.go b/pkg/internal/apis/certmanager/v1alpha2/zz_generated.conversion.go index 3a460382a..327100a16 100644 --- a/pkg/internal/apis/certmanager/v1alpha2/zz_generated.conversion.go +++ b/pkg/internal/apis/certmanager/v1alpha2/zz_generated.conversion.go @@ -565,6 +565,7 @@ func autoConvert_v1alpha2_CertificateSpec_To_certmanager_CertificateSpec(in *v1a out.RenewBefore = (*v1.Duration)(unsafe.Pointer(in.RenewBefore)) out.DNSNames = *(*[]string)(unsafe.Pointer(&in.DNSNames)) out.IPAddresses = *(*[]string)(unsafe.Pointer(&in.IPAddresses)) + out.URISANs = *(*[]string)(unsafe.Pointer(&in.URISANs)) out.SecretName = in.SecretName // TODO: Inefficient conversion - can we improve it? if err := s.Convert(&in.IssuerRef, &out.IssuerRef, 0); err != nil { @@ -590,6 +591,7 @@ func autoConvert_certmanager_CertificateSpec_To_v1alpha2_CertificateSpec(in *cer out.RenewBefore = (*v1.Duration)(unsafe.Pointer(in.RenewBefore)) out.DNSNames = *(*[]string)(unsafe.Pointer(&in.DNSNames)) out.IPAddresses = *(*[]string)(unsafe.Pointer(&in.IPAddresses)) + out.URISANs = *(*[]string)(unsafe.Pointer(&in.URISANs)) 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 96b1a9299..349c1df33 100644 --- a/pkg/internal/apis/certmanager/validation/certificate.go +++ b/pkg/internal/apis/certmanager/validation/certificate.go @@ -38,18 +38,15 @@ func ValidateCertificateSpec(crt *v1alpha2.CertificateSpec, fldPath *field.Path) el = append(el, validateIssuerRef(crt.IssuerRef, fldPath)...) - if len(crt.CommonName) == 0 && len(crt.DNSNames) == 0 { - el = append(el, field.Required(fldPath.Child("dnsNames"), "at least one dnsName is required if commonName is not set")) + 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 a common name has been specified, ensure it is no longer than 64 chars if len(crt.CommonName) > 64 { el = append(el, field.TooLong(fldPath.Child("commonName"), crt.CommonName, 64)) } - // if the common name has *not* been specified, ensure the first dnsName is no longer than 64 chars - // as it will be used as the commonName - if crt.CommonName == "" && len(crt.DNSNames) > 0 && len(crt.DNSNames[0]) > 64 { - el = append(el, field.TooLong(fldPath.Child("dnsNames").Index(0), crt.DNSNames[0], 64)) - } if len(crt.IPAddresses) > 0 { el = append(el, validateIPAddresses(crt, fldPath)...) diff --git a/pkg/internal/apis/certmanager/validation/certificate_test.go b/pkg/internal/apis/certmanager/validation/certificate_test.go index 0375fccff..4740c8d01 100644 --- a/pkg/internal/apis/certmanager/validation/certificate_test.go +++ b/pkg/internal/apis/certmanager/validation/certificate_test.go @@ -114,7 +114,7 @@ func TestValidateCertificate(t *testing.T) { field.Required(fldPath.Child("secretName"), "must be specified"), }, }, - "certificate with no domains": { + "certificate with no domains, URIs or common name": { cfg: &v1alpha2.Certificate{ Spec: v1alpha2.CertificateSpec{ SecretName: "abc", @@ -122,7 +122,7 @@ func TestValidateCertificate(t *testing.T) { }, }, errs: []*field.Error{ - field.Required(fldPath.Child("dnsNames"), "at least one dnsName is required if commonName is not set"), + field.Required(fldPath.Child("commonName", "dnsNames", "uriSANs"), "at least one of commonName, dnsNames, or uriSANs must be set"), }, }, "certificate with no issuerRef": { @@ -354,21 +354,6 @@ func TestValidateCertificate(t *testing.T) { field.TooLong(fldPath.Child("commonName"), "this-is-a-big-long-string-which-has-exactly-sixty-five-characters", 64), }, }, - "invalid certificate with no commonName and first dnsName longer than 64 bytes": { - cfg: &v1alpha2.Certificate{ - Spec: v1alpha2.CertificateSpec{ - SecretName: "abc", - IssuerRef: validIssuerRef, - DNSNames: []string{ - "this-is-a-big-long-string-which-has-exactly-sixty-five-characters", - "dnsName", - }, - }, - }, - errs: []*field.Error{ - field.TooLong(fldPath.Child("dnsNames").Index(0), "this-is-a-big-long-string-which-has-exactly-sixty-five-characters", 64), - }, - }, "valid certificate with no commonName and second dnsName longer than 64 bytes": { cfg: &v1alpha2.Certificate{ Spec: v1alpha2.CertificateSpec{ @@ -427,6 +412,17 @@ func TestValidateCertificate(t *testing.T) { field.Invalid(fldPath.Child("usages").Index(0), v1alpha2.KeyUsage("nonexistant"), "unknown keyusage"), }, }, + "valid certificate with only URI SAN name": { + cfg: &v1alpha2.Certificate{ + Spec: v1alpha2.CertificateSpec{ + SecretName: "abc", + IssuerRef: validIssuerRef, + URISANs: []string{ + "foo.bar", + }, + }, + }, + }, } 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 75c1b99c9..a18e5f689 100644 --- a/pkg/internal/apis/certmanager/zz_generated.deepcopy.go +++ b/pkg/internal/apis/certmanager/zz_generated.deepcopy.go @@ -302,6 +302,11 @@ func (in *CertificateSpec) DeepCopyInto(out *CertificateSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.URISANs != nil { + in, out := &in.URISANs, &out.URISANs + *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/internal/vault/vault.go b/pkg/internal/vault/vault.go index 66a1e97dc..e522e5796 100644 --- a/pkg/internal/vault/vault.go +++ b/pkg/internal/vault/vault.go @@ -97,6 +97,7 @@ func (v *Vault) Sign(csrPEM []byte, duration time.Duration) (cert []byte, ca []b "common_name": csr.Subject.CommonName, "alt_names": strings.Join(csr.DNSNames, ","), "ip_sans": strings.Join(pki.IPAddressesToString(csr.IPAddresses), ","), + "uri_sans": strings.Join(pki.URLsToString(csr.URIs), ","), "ttl": duration.String(), "csr": string(csrPEM), diff --git a/pkg/internal/venafi/sign.go b/pkg/internal/venafi/sign.go index c0753b58d..e67150bac 100644 --- a/pkg/internal/venafi/sign.go +++ b/pkg/internal/venafi/sign.go @@ -18,6 +18,7 @@ package venafi import ( "crypto/x509" + "errors" "strings" "time" @@ -63,6 +64,23 @@ func (v *Venafi) Sign(csrPEM []byte, duration time.Duration) (cert []byte, err e //// TODO: better set the timeout here. Right now, we'll block for this amount of time. vreq.Timeout = time.Minute * 5 + // Set the 'ObjectName' through the request friendly name. This is set in + // order of precedence CN->DNS->URI. + switch { + case len(tmpl.Subject.CommonName) > 0: + vreq.FriendlyName = tmpl.Subject.CommonName + break + case len(tmpl.DNSNames) > 0: + vreq.FriendlyName = tmpl.DNSNames[0] + break + case len(tmpl.URIs) > 0: + vreq.FriendlyName = tmpl.URIs[0].String() + break + default: + return nil, errors.New( + "certificate request contains no Common Name, DNS Name, nor URI SAN, at least one must be supplied to be used as the Venafi certificate objects name") + } + // Set the request CSR with the passed value if err := vreq.SetCSR(csrPEM); err != nil { return nil, err diff --git a/pkg/internal/venafi/sign_test.go b/pkg/internal/venafi/sign_test.go index 84ab44b63..63f35763d 100644 --- a/pkg/internal/venafi/sign_test.go +++ b/pkg/internal/venafi/sign_test.go @@ -111,6 +111,8 @@ func TestSign(t *testing.T) { csrPEM := generateCSR(t, sk, "common-name", []string{ "foo.example.com", "bar.example.com"}) + csrNonePEM := generateCSR(t, sk, "", []string{}) + tests := map[string]testSignT{ "if reading the zone configuration fails then error": { csrPEM: csrPEM, @@ -161,6 +163,11 @@ func TestSign(t *testing.T) { checkFn: checkNoCetificateIssued, expectedErr: true, }, + "if no Common Name, DNS Name, or URI SANs in CSR then error": { + csrPEM: csrNonePEM, + checkFn: checkNoCetificateIssued, + expectedErr: true, + }, "obtain a certificate with DNS names specified": { csrPEM: csrPEM, checkFn: checkCertificateIssued, diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index fb885587b..4331eb64e 100644 --- a/pkg/util/pki/csr.go +++ b/pkg/util/pki/csr.go @@ -27,39 +27,14 @@ import ( "fmt" "math/big" "net" + "net/url" + "strings" "time" apiutil "github.com/jetstack/cert-manager/pkg/api/util" "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" ) -// CommonNameForCertificate returns the common name that should be used for the -// given Certificate resource, by inspecting the CommonName and DNSNames fields. -func CommonNameForCertificate(crt *v1alpha2.Certificate) string { - if crt.Spec.CommonName != "" { - return crt.Spec.CommonName - } - if len(crt.Spec.DNSNames) == 0 { - return "" - } - return crt.Spec.DNSNames[0] -} - -// DNSNamesForCertificate returns the DNS names that should be used for the -// given Certificate resource, by inspecting the CommonName and DNSNames fields. -func DNSNamesForCertificate(crt *v1alpha2.Certificate) []string { - if len(crt.Spec.DNSNames) == 0 { - if crt.Spec.CommonName == "" { - return []string{} - } - return []string{crt.Spec.CommonName} - } - if crt.Spec.CommonName != "" { - return removeDuplicates(append([]string{crt.Spec.CommonName}, crt.Spec.DNSNames...)) - } - return crt.Spec.DNSNames -} - func IPAddressesForCertificate(crt *v1alpha2.Certificate) []net.IP { var ipAddresses []net.IP var ip net.IP @@ -72,6 +47,45 @@ func IPAddressesForCertificate(crt *v1alpha2.Certificate) []net.IP { return ipAddresses } +func URIsForCertificate(crt *v1alpha2.Certificate) ([]*url.URL, error) { + 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 _, urlStr := range urlStrs { + url, err := url.Parse(urlStr) + if err != nil { + errs = append(errs, err.Error()) + continue + } + + urls = append(urls, url) + } + + if len(errs) > 0 { + return nil, errors.New(strings.Join(errs, ", ")) + } + + return urls, nil +} + func IPAddressesToString(ipAddresses []net.IP) []string { var ipNames []string for _, ip := range ipAddresses { @@ -80,6 +94,19 @@ func IPAddressesToString(ipAddresses []net.IP) []string { return ipNames } +func URLsToString(uris []*url.URL) []string { + var uriStrs []string + for _, uri := range uris { + if uri == nil { + panic("provided uri to string is nil") + } + + uriStrs = append(uriStrs, uri.String()) + } + + return uriStrs +} + func removeDuplicates(in []string) []string { var found []string Outer: @@ -137,13 +164,22 @@ func buildUsages(usages []v1alpha2.KeyUsage, isCA bool) (ku x509.KeyUsage, eku [ // The CSR will not be signed, and should be passed to either EncodeCSR or // to the x509.CreateCertificateRequest function. func GenerateCSR(crt *v1alpha2.Certificate) (*x509.CertificateRequest, error) { - commonName := CommonNameForCertificate(crt) - dnsNames := DNSNamesForCertificate(crt) + commonName := crt.Spec.CommonName iPAddresses := IPAddressesForCertificate(crt) organization := OrganizationForCertificate(crt) - if len(commonName) == 0 && len(dnsNames) == 0 { - return nil, fmt.Errorf("no domains specified on certificate") + dnsNames, err := DNSNamesForCertificate(crt) + if err != nil { + return nil, err + } + + uriNames, err := URIsForCertificate(crt) + if err != nil { + 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") } pubKeyAlgo, sigAlgo, err := SignatureAlgorithm(crt) @@ -161,6 +197,7 @@ func GenerateCSR(crt *v1alpha2.Certificate) (*x509.CertificateRequest, error) { }, DNSNames: dnsNames, IPAddresses: iPAddresses, + URIs: uriNames, // TODO: work out how best to handle extensions/key usages here ExtraExtensions: []pkix.Extension{}, }, nil @@ -171,8 +208,8 @@ func GenerateCSR(crt *v1alpha2.Certificate) (*x509.CertificateRequest, error) { // generated by GenerateCSR. // The PublicKey field must be populated by the caller. func GenerateTemplate(crt *v1alpha2.Certificate) (*x509.Certificate, error) { - commonName := CommonNameForCertificate(crt) - dnsNames := DNSNamesForCertificate(crt) + commonName := crt.Spec.CommonName + dnsNames := crt.Spec.DNSNames ipAddresses := IPAddressesForCertificate(crt) organization := OrganizationForCertificate(crt) keyUsages, extKeyUsages, err := buildUsages(crt.Spec.Usages, crt.Spec.IsCA) diff --git a/pkg/util/pki/csr_test.go b/pkg/util/pki/csr_test.go index d70c7a71d..53d48efd1 100644 --- a/pkg/util/pki/csr_test.go +++ b/pkg/util/pki/csr_test.go @@ -125,7 +125,7 @@ func TestCommonNameForCertificate(t *testing.T) { { name: "certificate with one DNS name set", crtDNSNames: []string{"dnsname"}, - expectedCN: "dnsname", + expectedCN: "", }, { name: "certificate with both common name and dnsName set", @@ -136,12 +136,12 @@ func TestCommonNameForCertificate(t *testing.T) { { name: "certificate with multiple dns names set", crtDNSNames: []string{"dnsname1", "dnsname2"}, - expectedCN: "dnsname1", + expectedCN: "", }, } testFn := func(test testT) func(*testing.T) { return func(t *testing.T) { - actualCN := CommonNameForCertificate(buildCertificate(test.crtCN, test.crtDNSNames...)) + actualCN := buildCertificate(test.crtCN, test.crtDNSNames...).Spec.CommonName if actualCN != test.expectedCN { t.Errorf("expected %q but got %q", test.expectedCN, actualCN) return @@ -164,7 +164,7 @@ func TestDNSNamesForCertificate(t *testing.T) { { name: "certificate with CommonName set", crtCN: "test", - expectDNSNames: []string{"test"}, + expectDNSNames: []string{}, }, { name: "certificate with one DNS name set", @@ -175,7 +175,7 @@ func TestDNSNamesForCertificate(t *testing.T) { name: "certificate with both common name and dnsName set", crtCN: "cn", crtDNSNames: []string{"dnsname"}, - expectDNSNames: []string{"cn", "dnsname"}, + expectDNSNames: []string{"dnsname"}, }, { name: "certificate with multiple dns names set", @@ -192,12 +192,12 @@ func TestDNSNamesForCertificate(t *testing.T) { name: "certificate with a dnsName equal to cn", crtCN: "cn", crtDNSNames: []string{"dnsname", "cn"}, - expectDNSNames: []string{"cn", "dnsname"}, + expectDNSNames: []string{"dnsname", "cn"}, }, } testFn := func(test testT) func(*testing.T) { return func(t *testing.T) { - actualDNSNames := DNSNamesForCertificate(buildCertificate(test.crtCN, test.crtDNSNames...)) + actualDNSNames := buildCertificate(test.crtCN, test.crtDNSNames...).Spec.DNSNames if len(actualDNSNames) != len(test.expectDNSNames) { t.Errorf("expected %q but got %q", test.expectDNSNames, actualDNSNames) return diff --git a/pkg/util/util.go b/pkg/util/util.go index 95b276811..4e5532f24 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -142,3 +142,14 @@ func Contains(ss []string, s string) bool { } return false } + +// Subset returns true if one slice is an unsorted subset of the first. +func Subset(set, subset []string) bool { + for _, s := range subset { + if !Contains(set, s) { + return false + } + } + + return true +} diff --git a/test/e2e/framework/addon/vault/setup.go b/test/e2e/framework/addon/vault/setup.go index a8193b7c3..60a1228d3 100644 --- a/test/e2e/framework/addon/vault/setup.go +++ b/test/e2e/framework/addon/vault/setup.go @@ -404,9 +404,11 @@ func (v *VaultInitializer) setupRole() error { } params := map[string]string{ - "allow_any_name": "true", - "max_ttl": "2160h", - "key_type": "any", + "allow_any_name": "true", + "max_ttl": "2160h", + "key_type": "any", + "require_cn": "false", + "allowed_uri_sans": "spiffe://cluster.local/*", } url := path.Join("/v1", v.IntermediateMount, "roles", v.Role) diff --git a/test/e2e/framework/helper/certificaterequests.go b/test/e2e/framework/helper/certificaterequests.go index 633fa626a..d6e894358 100644 --- a/test/e2e/framework/helper/certificaterequests.go +++ b/test/e2e/framework/helper/certificaterequests.go @@ -96,7 +96,6 @@ func (h *Helper) ValidateIssuedCertificateRequest(cr *v1alpha2.CertificateReques // TODO: validate private key KeySize // check the provided certificate is valid - expectedCN := csr.Subject.CommonName expectedOrganization := csr.Subject.Organization expectedDNSNames := csr.DNSNames expectedIPAddresses := csr.IPAddresses @@ -107,7 +106,17 @@ func (h *Helper) ValidateIssuedCertificateRequest(cr *v1alpha2.CertificateReques return nil, err } - if expectedCN != cert.Subject.CommonName || + commonNameCorrect := true + expectedCN := csr.Subject.CommonName + if len(expectedCN) == 0 && len(cert.Subject.CommonName) > 0 { + if !util.Contains(cert.DNSNames, cert.Subject.CommonName) { + commonNameCorrect = false + } + } else if expectedCN != cert.Subject.CommonName { + commonNameCorrect = false + } + + if !commonNameCorrect || !util.EqualUnsorted(cert.DNSNames, expectedDNSNames) || !util.EqualUnsorted(cert.Subject.Organization, expectedOrganization) || !util.EqualIPsUnsorted(cert.IPAddresses, expectedIPAddresses) || diff --git a/test/e2e/framework/helper/certificates.go b/test/e2e/framework/helper/certificates.go index 2bef35a4f..ad2c7c80b 100644 --- a/test/e2e/framework/helper/certificates.go +++ b/test/e2e/framework/helper/certificates.go @@ -60,11 +60,8 @@ func (h *Helper) WaitForCertificateReady(ns, name string, timeout time.Duration) }, ) - if err != nil { - return nil, err - } - - return certificate, nil + // return certificate even when error to use for debugging + return certificate, err } // WaitForCertificateNotReady waits for the certificate resource to enter a @@ -91,11 +88,8 @@ func (h *Helper) WaitForCertificateNotReady(ns, name string, timeout time.Durati }, ) - if err != nil { - return nil, err - } - - return certificate, nil + // return certificate even when error to use for debugging + return certificate, err } // ValidateIssuedCertificate will ensure that the given Certificate has a @@ -140,9 +134,14 @@ func (h *Helper) ValidateIssuedCertificate(certificate *v1alpha2.Certificate, ro // TODO: validate private key KeySize // check the provided certificate is valid - expectedCN := pki.CommonNameForCertificate(certificate) expectedOrganization := pki.OrganizationForCertificate(certificate) - expectedDNSNames := pki.DNSNamesForCertificate(certificate) + expectedDNSNames := certificate.Spec.DNSNames + uris, err := pki.URIsForCertificate(certificate) + if err != nil { + return nil, fmt.Errorf("failed to parse URIs: %s", err) + } + + expectedURIs := pki.URLsToString(uris) certBytes, ok := secret.Data[corev1.TLSCertKey] if !ok { @@ -153,8 +152,21 @@ func (h *Helper) ValidateIssuedCertificate(certificate *v1alpha2.Certificate, ro if err != nil { return nil, err } - if expectedCN != cert.Subject.CommonName || !util.EqualUnsorted(cert.DNSNames, expectedDNSNames) || !(len(cert.Subject.Organization) == 0 || util.EqualUnsorted(cert.Subject.Organization, expectedOrganization)) { - return nil, fmt.Errorf("Expected certificate valid for CN %q, O %v, dnsNames %v but got a certificate valid for CN %q, O %v, dnsNames %v", expectedCN, expectedOrganization, expectedDNSNames, cert.Subject.CommonName, cert.Subject.Organization, cert.DNSNames) + + commonNameCorrect := true + expectedCN := certificate.Spec.CommonName + if len(expectedCN) == 0 && len(cert.Subject.CommonName) > 0 { + if !util.Contains(cert.DNSNames, cert.Subject.CommonName) { + commonNameCorrect = false + } + } else if expectedCN != cert.Subject.CommonName { + commonNameCorrect = false + } + + if !commonNameCorrect || !util.Subset(cert.DNSNames, expectedDNSNames) || !util.EqualUnsorted(pki.URLsToString(cert.URIs), expectedURIs) || + !(len(cert.Subject.Organization) == 0 || util.EqualUnsorted(cert.Subject.Organization, expectedOrganization)) { + return nil, fmt.Errorf("Expected certificate valid for CN %q, O %v, dnsNames %v, uriSANs %v,but got a certificate valid for CN %q, O %v, dnsNames %v, uriSANs %v", + expectedCN, expectedOrganization, expectedDNSNames, expectedURIs, cert.Subject.CommonName, cert.Subject.Organization, cert.DNSNames, cert.URIs) } if certificate.Status.NotAfter == nil { @@ -194,6 +206,11 @@ func (h *Helper) ValidateIssuedCertificate(certificate *v1alpha2.Certificate, ro } } + var dnsName string + if len(expectedDNSNames) > 0 { + dnsName = expectedDNSNames[0] + } + // TODO: move this verification step out of this function if rootCAPEM != nil { rootCertPool := x509.NewCertPool() @@ -201,7 +218,7 @@ func (h *Helper) ValidateIssuedCertificate(certificate *v1alpha2.Certificate, ro intermediateCertPool := x509.NewCertPool() intermediateCertPool.AppendCertsFromPEM(certBytes) opts := x509.VerifyOptions{ - DNSName: expectedDNSNames[0], + DNSName: dnsName, Intermediates: intermediateCertPool, Roots: rootCertPool, } @@ -224,6 +241,7 @@ func (h *Helper) WaitCertificateIssuedValidTLS(ns, name string, timeout time.Dur log.Logf("Error waiting for Certificate to become Ready: %v", err) h.Kubectl(ns).DescribeResource("certificate", name) h.Kubectl(ns).Describe("order", "challenge") + h.describeCertificateRequestFromCertificate(ns, certificate) return err } @@ -232,8 +250,22 @@ func (h *Helper) WaitCertificateIssuedValidTLS(ns, name string, timeout time.Dur log.Logf("Error validating issued certificate: %v", err) h.Kubectl(ns).DescribeResource("certificate", name) h.Kubectl(ns).Describe("order", "challenge") + h.describeCertificateRequestFromCertificate(ns, certificate) return err } return nil } + +func (h *Helper) describeCertificateRequestFromCertificate(ns string, certificate *v1alpha2.Certificate) { + if certificate == nil { + return + } + + crName, err := apiutil.ComputeCertificateRequestName(certificate) + if err != nil { + log.Logf("Failed to compute CertificateRequest name from certificate: %s", err) + return + } + h.Kubectl(ns).DescribeResource("certificaterequest", crName) +} diff --git a/test/e2e/suite/conformance/certificates/acme/acme.go b/test/e2e/suite/conformance/certificates/acme/acme.go index b4d3f19ca..9a92c3895 100644 --- a/test/e2e/suite/conformance/certificates/acme/acme.go +++ b/test/e2e/suite/conformance/certificates/acme/acme.go @@ -38,6 +38,8 @@ var _ = framework.ConformanceDescribe("Certificates", func() { certificates.IPAddressFeature, certificates.DurationFeature, certificates.WildcardsFeature, + certificates.URISANsFeature, + certificates.CommonNameFeature, ) // unsupportedDNS01Features is a list of features that are not supported by the ACME @@ -45,6 +47,8 @@ var _ = framework.ConformanceDescribe("Certificates", func() { var unsupportedDNS01Features = certificates.NewFeatureSet( certificates.IPAddressFeature, certificates.DurationFeature, + certificates.URISANsFeature, + certificates.CommonNameFeature, ) provisionerHTTP01 := new(acmeIssuerProvisioner) diff --git a/test/e2e/suite/conformance/certificates/featureset.go b/test/e2e/suite/conformance/certificates/featureset.go index 0731f24a3..c752fb534 100644 --- a/test/e2e/suite/conformance/certificates/featureset.go +++ b/test/e2e/suite/conformance/certificates/featureset.go @@ -98,4 +98,14 @@ const ( // certificates for the same private key. This is useful for some issuers // that have trouble being configured to support this feature. ReusePrivateKeyFeature Feature = "ReusePrivateKey" + + // URISANs denotes whether to the target issuer is able to sign a certificate + // that includes a URISANs. ACME providers do not support this. + URISANsFeature Feature = "URISANs" + + // 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 + // CommonName such as needing to be present in the DNS Name list. + CommonNameFeature = "CommonName" ) diff --git a/test/e2e/suite/conformance/certificates/suite.go b/test/e2e/suite/conformance/certificates/suite.go index 25264572e..5d2151655 100644 --- a/test/e2e/suite/conformance/certificates/suite.go +++ b/test/e2e/suite/conformance/certificates/suite.go @@ -124,7 +124,7 @@ func (s *Suite) Define() { s.DeleteIssuerFunc(f, issuerRef) }) - It("should issue a basic, defaulted certificate for a single commonName and distinct dnsName", func() { + It("should issue a basic, defaulted certificate for a single distinct DNS Name", func() { testCertificate := &cmapi.Certificate{ ObjectMeta: metav1.ObjectMeta{ Name: "testcert", @@ -132,9 +132,8 @@ func (s *Suite) Define() { }, Spec: cmapi.CertificateSpec{ SecretName: "testcert-tls", - CommonName: s.newDomain(), - DNSNames: []string{s.newDomain()}, IssuerRef: issuerRef, + DNSNames: []string{s.newDomain()}, }, } By("Creating a Certificate") @@ -146,7 +145,7 @@ func (s *Suite) Define() { Expect(err).NotTo(HaveOccurred()) }) - It("should issue an ECDSA, defaulted certificate for a single commonName and distinct dnsName", func() { + It("should issue an ECDSA, defaulted certificate for a single distinct dnsName", func() { s.checkFeatures(ECDSAFeature) testCertificate := &cmapi.Certificate{ @@ -157,7 +156,6 @@ func (s *Suite) Define() { Spec: cmapi.CertificateSpec{ SecretName: "testcert-tls", KeyAlgorithm: cmapi.ECDSAKeyAlgorithm, - CommonName: s.newDomain(), DNSNames: []string{s.newDomain()}, IssuerRef: issuerRef, }, @@ -171,7 +169,56 @@ func (s *Suite) Define() { Expect(err).NotTo(HaveOccurred()) }) - It("should issue a certificate that defines a commonName and ipAddresses", func() { + It("should issue a basic, defaulted certificate for a single Common Name", func() { + s.checkFeatures(CommonNameFeature) + + testCertificate := &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testcert", + Namespace: f.Namespace.Name, + }, + Spec: cmapi.CertificateSpec{ + SecretName: "testcert-tls", + IssuerRef: issuerRef, + CommonName: "test-common-name", + }, + } + 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 an ECDSA, defaulted certificate for a single Common Name", func() { + s.checkFeatures(ECDSAFeature) + s.checkFeatures(CommonNameFeature) + + testCertificate := &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testcert", + Namespace: f.Namespace.Name, + }, + Spec: cmapi.CertificateSpec{ + SecretName: "testcert-tls", + KeyAlgorithm: cmapi.ECDSAKeyAlgorithm, + CommonName: "test-common-name", + 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 Common Name and IP Address", func() { + s.checkFeatures(CommonNameFeature) s.checkFeatures(IPAddressFeature) testCertificate := &cmapi.Certificate{ @@ -181,7 +228,7 @@ func (s *Suite) Define() { }, Spec: cmapi.CertificateSpec{ SecretName: "testcert-tls", - CommonName: s.newDomain(), + CommonName: "test-common-name", IPAddresses: []string{"127.0.0.1"}, IssuerRef: issuerRef, }, @@ -195,8 +242,33 @@ func (s *Suite) Define() { Expect(err).NotTo(HaveOccurred()) }) - It("should issue a certificate that defines a commonName and sets a duration", func() { - s.checkFeatures(DurationFeature) + It("should issue a certificate that defines a URI Name and URI SAN", func() { + s.checkFeatures(URISANsFeature) + s.checkFeatures(CommonNameFeature) + + testCertificate := &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testcert", + Namespace: f.Namespace.Name, + }, + Spec: cmapi.CertificateSpec{ + SecretName: "testcert-tls", + CommonName: "test-common-name", + URISANs: []string{"spiffe://cluster.local/ns/sandbox/sa/foo"}, + 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 2 distinct DNS Name with one copied to the Common Name", func() { + s.checkFeatures(CommonNameFeature) testCertificate := &cmapi.Certificate{ ObjectMeta: metav1.ObjectMeta{ @@ -207,6 +279,58 @@ func (s *Suite) Define() { SecretName: "testcert-tls", CommonName: s.newDomain(), IssuerRef: issuerRef, + }, + } + testCertificate.Spec.DNSNames = []string{ + testCertificate.Spec.CommonName, 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 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) + + testCertificate := &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testcert", + Namespace: f.Namespace.Name, + }, + Spec: cmapi.CertificateSpec{ + SecretName: "testcert-tls", + IssuerRef: issuerRef, + DNSNames: []string{s.newDomain()}, Duration: &metav1.Duration{ Duration: time.Hour * 896, }, @@ -238,7 +362,6 @@ func (s *Suite) Define() { }, Spec: cmapi.CertificateSpec{ SecretName: "testcert-tls", - CommonName: s.newDomain(), IssuerRef: issuerRef, DNSNames: []string{"foo." + s.newDomain()}, }, @@ -252,6 +375,31 @@ func (s *Suite) Define() { Expect(err).NotTo(HaveOccurred()) }) + It("should issue a certificate that includes only a URISANs name", func() { + s.checkFeatures(URISANsFeature) + + testCertificate := &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testcert", + Namespace: f.Namespace.Name, + }, + Spec: cmapi.CertificateSpec{ + SecretName: "testcert-tls", + URISANs: []string{ + "spiffe://cluster.local/ns/sandbox/sa/foo", + }, + 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 another certificate with the same private key if the existing certificate and CertificateRequest are deleted", func() { s.checkFeatures(ReusePrivateKeyFeature) @@ -262,7 +410,6 @@ func (s *Suite) Define() { }, Spec: cmapi.CertificateSpec{ SecretName: "testcert-tls", - CommonName: s.newDomain(), DNSNames: []string{s.newDomain()}, IssuerRef: issuerRef, }, diff --git a/test/unit/gen/certificate.go b/test/unit/gen/certificate.go index f309feeae..320537761 100644 --- a/test/unit/gen/certificate.go +++ b/test/unit/gen/certificate.go @@ -64,6 +64,18 @@ func SetCertificateCommonName(commonName string) CertificateModifier { } } +func SetCertificateIPs(ips ...string) CertificateModifier { + return func(crt *v1alpha2.Certificate) { + crt.Spec.IPAddresses = ips + } +} + +func SetCertificateURIs(uris ...string) CertificateModifier { + return func(crt *v1alpha2.Certificate) { + crt.Spec.URISANs = uris + } +} + func SetCertificateIsCA(isCA bool) CertificateModifier { return func(crt *v1alpha2.Certificate) { crt.Spec.IsCA = isCA