diff --git a/cmd/ctl/pkg/create/certificatesigningrequest/certificatesigningrequest.go b/cmd/ctl/pkg/create/certificatesigningrequest/certificatesigningrequest.go index 38eb3fc1e..a77de3b76 100644 --- a/cmd/ctl/pkg/create/certificatesigningrequest/certificatesigningrequest.go +++ b/cmd/ctl/pkg/create/certificatesigningrequest/certificatesigningrequest.go @@ -372,7 +372,10 @@ func buildCertificateSigningRequest(crt *cmapi.Certificate, pk []byte, crName, s } csr.Annotations[experimentalapi.CertificateSigningRequestIsCAAnnotationKey] = strconv.FormatBool(crt.Spec.IsCA) if crt.Spec.Duration != nil { - csr.Annotations[experimentalapi.CertificateSigningRequestDurationAnnotationKey] = crt.Spec.Duration.Duration.String() + duration := crt.Spec.Duration.Duration + csr.Annotations[experimentalapi.CertificateSigningRequestDurationAnnotationKey] = duration.String() + seconds := int32(duration.Seconds()) // technically this could overflow but I do not think it matters + csr.Spec.ExpirationSeconds = &seconds // if this is less than 600, the API server will fail the request } return csr, nil diff --git a/pkg/controller/certificatesigningrequests/ca/ca_test.go b/pkg/controller/certificatesigningrequests/ca/ca_test.go index 2c8efd4ce..e72eb98ef 100644 --- a/pkg/controller/certificatesigningrequests/ca/ca_test.go +++ b/pkg/controller/certificatesigningrequests/ca/ca_test.go @@ -649,6 +649,41 @@ func TestCA_Sign(t *testing.T) { assert.LessOrEqualf(t, deltaSec, 2., "expected a time delta lower than 2 second. Time expected='%s', got='%s'", expectNotAfter.String(), got.NotAfter.String()) }, }, + "when the CertificateSigningRequest has the expiration seconds field set, it should appear as notAfter on the signed ca": { + givenCASecret: gen.SecretFrom(gen.Secret("secret-1"), gen.SetSecretNamespace("default"), gen.SetSecretData(secretDataFor(t, rootPK, rootCert))), + givenCAIssuer: gen.Issuer("issuer-1", gen.SetIssuerCA(cmapi.CAIssuer{ + SecretName: "secret-1", + })), + givenCSR: gen.CertificateSigningRequest("csr-1", + gen.SetCertificateSigningRequestRequest(testCSR), + gen.SetCertificateSigningRequestSignerName("issers.cert-manager.io/"+gen.DefaultTestNamespace+".issuer-1"), + gen.SetCertificateSigningRequestExpirationSeconds(654), + ), + assertSignedCert: func(t *testing.T, got *x509.Certificate) { + // Although there is less than 1µs between the time.Now + // call made by the certificate template func (in the "pki" + // package) and the time.Now below, rounding or truncating + // will always end up with a flaky test. This is due to the + // rounding made to the notAfter value when serializing the + // certificate to ASN.1 [1]. + // + // [1]: https://tools.ietf.org/html/rfc5280#section-4.1.2.5.1 + // + // So instead of using a truncation or rounding in order to + // check the time, we use a delta of 2 seconds. One entire + // second is totally overkill since, as detailed above, the + // delay is probably less than a microsecond. But that will + // do for now! + // + // Note that we do have a plan to fix this. We want to be + // injecting a time (instead of time.Now) to the template + // functions. This work is being tracked in this issue: + // https://github.com/cert-manager/cert-manager/issues/3738 + expectNotAfter := time.Now().UTC().Add(654 * time.Second) + deltaSec := math.Abs(expectNotAfter.Sub(got.NotAfter).Seconds()) + assert.LessOrEqualf(t, deltaSec, 2., "expected a time delta lower than 2 second. Time expected='%s', got='%s'", expectNotAfter.String(), got.NotAfter.String()) + }, + }, "when the CertificateSigningRequest has the isCA field set, it should appear on the signed ca": { givenCASecret: gen.SecretFrom(gen.Secret("secret-1"), gen.SetSecretNamespace("default"), gen.SetSecretData(secretDataFor(t, rootPK, rootCert))), givenCAIssuer: gen.Issuer("issuer-1", gen.SetIssuerCA(cmapi.CAIssuer{ diff --git a/pkg/controller/certificatesigningrequests/selfsigned/selfsigned_test.go b/pkg/controller/certificatesigningrequests/selfsigned/selfsigned_test.go index 33300298a..d2a2f3a81 100644 --- a/pkg/controller/certificatesigningrequests/selfsigned/selfsigned_test.go +++ b/pkg/controller/certificatesigningrequests/selfsigned/selfsigned_test.go @@ -732,6 +732,41 @@ func TestSign(t *testing.T) { assert.LessOrEqualf(t, deltaSec, 2., "expected a time delta lower than 2 second. Time expected='%s', got='%s'", expectNotAfter.String(), got.NotAfter.String()) }, }, + "when the CertificateSigningRequest has the expiration seconds field set, it should appear as notAfter on the signed certificate": { + csr: gen.CertificateSigningRequest("csr-1", + gen.AddCertificateSigningRequestAnnotations(map[string]string{ + "experimental.cert-manager.io/private-key-secret-name": "test-secret", + }), + gen.SetCertificateSigningRequestSignerName("issuers.cert-manager.io/default-unit-test-ns.issuer-1"), + gen.SetCertificateSigningRequestExpirationSeconds(444), + gen.SetCertificateSigningRequestRequest(csrBundle.csrPEM), + ), + issuer: baseIssuer, + assertSignedCert: func(t *testing.T, got *x509.Certificate) { + // Although there is less than 1µs between the time.Now + // call made by the certificate template func (in the "pki" + // package) and the time.Now below, rounding or truncating + // will always end up with a flaky test. This is due to the + // rounding made to the notAfter value when serializing the + // certificate to ASN.1 [1]. + // + // [1]: https://tools.ietf.org/html/rfc5280#section-4.1.2.5.1 + // + // So instead of using a truncation or rounding in order to + // check the time, we use a delta of 2 seconds. One entire + // second is totally overkill since, as detailed above, the + // delay is probably less than a microsecond. But that will + // do for now! + // + // Note that we do have a plan to fix this. We want to be + // injecting a time (instead of time.Now) to the template + // functions. This work is being tracked in this issue: + // https://github.com/cert-manager/cert-manager/issues/3738 + expectNotAfter := time.Now().UTC().Add(444 * time.Second) + deltaSec := math.Abs(expectNotAfter.Sub(got.NotAfter).Seconds()) + assert.LessOrEqualf(t, deltaSec, 2., "expected a time delta lower than 2 second. Time expected='%s', got='%s'", expectNotAfter.String(), got.NotAfter.String()) + }, + }, "when the CertificateSigningRequest has the isCA field set, it should appear on the signed certificate": { csr: gen.CertificateSigningRequest("csr-1", gen.AddCertificateSigningRequestAnnotations(map[string]string{ diff --git a/pkg/util/pki/kube.go b/pkg/util/pki/kube.go index d42daefa1..d2bec20ec 100644 --- a/pkg/util/pki/kube.go +++ b/pkg/util/pki/kube.go @@ -48,12 +48,18 @@ func GenerateTemplateFromCertificateSigningRequest(csr *certificatesv1.Certifica // DurationFromCertificateSigningRequest returns the duration that the user may // have requested using the annotation -// "experimental.cert-manager.io/request-duration". +// "experimental.cert-manager.io/request-duration" or via the CSR +// spec.expirationSeconds field (the annotation is preferred since it predates +// the field which is only available in Kubernetes v1.22+). // Returns the cert-manager default certificate duration when the user hasn't -// provided the annotation. +// provided the annotation or spec.expirationSeconds. func DurationFromCertificateSigningRequest(csr *certificatesv1.CertificateSigningRequest) (time.Duration, error) { requestedDuration, ok := csr.Annotations[experimentalapi.CertificateSigningRequestDurationAnnotationKey] if !ok { + if csr.Spec.ExpirationSeconds != nil { + return time.Duration(*csr.Spec.ExpirationSeconds) * time.Second, nil + } + // The user may not have set a duration annotation. Use the default // duration in this case. return cmapi.DefaultCertificateDuration, nil diff --git a/pkg/util/pki/kube_test.go b/pkg/util/pki/kube_test.go index 7fc17d4f6..1c9e873bb 100644 --- a/pkg/util/pki/kube_test.go +++ b/pkg/util/pki/kube_test.go @@ -131,6 +131,73 @@ func TestGenerateTemplateFromCertificateSigningRequest(t *testing.T) { DNSNames: []string{"example.com", "foo.example.com"}, }, }, + "a CSR with expiration seconds that is valid should return a valid *x509.Certificate": { + csr: gen.CertificateSigningRequest("", + gen.SetCertificateSigningRequestExpirationSeconds(999), + gen.SetCertificateSigningRequestUsages([]certificatesv1.KeyUsage{ + certificatesv1.UsageAny, + certificatesv1.UsageDigitalSignature, + certificatesv1.UsageCRLSign, + certificatesv1.UsageCodeSigning, + certificatesv1.UsageContentCommitment, + }), + gen.SetCertificateSigningRequestIsCA(false), + gen.SetCertificateSigningRequestRequest(csr), + ), + expCertificate: &x509.Certificate{ + Version: 2, + BasicConstraintsValid: true, + SerialNumber: nil, + PublicKeyAlgorithm: x509.RSA, + PublicKey: pk.Public(), + IsCA: false, + Subject: pkix.Name{ + CommonName: "example.com", + }, + NotBefore: time.Now(), + NotAfter: time.Now().Add(999 * time.Second), + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCRLSign | x509.KeyUsageContentCommitment, + ExtKeyUsage: []x509.ExtKeyUsage{ + x509.ExtKeyUsageAny, + x509.ExtKeyUsageCodeSigning, + }, + DNSNames: []string{"example.com", "foo.example.com"}, + }, + }, + "a CSR with expiration seconds and duration annotation should prefer the annotation duration": { + csr: gen.CertificateSigningRequest("", + gen.SetCertificateSigningRequestExpirationSeconds(999), + gen.SetCertificateSigningRequestDuration("777s"), + gen.SetCertificateSigningRequestUsages([]certificatesv1.KeyUsage{ + certificatesv1.UsageAny, + certificatesv1.UsageDigitalSignature, + certificatesv1.UsageCRLSign, + certificatesv1.UsageCodeSigning, + certificatesv1.UsageContentCommitment, + }), + gen.SetCertificateSigningRequestIsCA(false), + gen.SetCertificateSigningRequestRequest(csr), + ), + expCertificate: &x509.Certificate{ + Version: 2, + BasicConstraintsValid: true, + SerialNumber: nil, + PublicKeyAlgorithm: x509.RSA, + PublicKey: pk.Public(), + IsCA: false, + Subject: pkix.Name{ + CommonName: "example.com", + }, + NotBefore: time.Now(), + NotAfter: time.Now().Add(777 * time.Second), + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCRLSign | x509.KeyUsageContentCommitment, + ExtKeyUsage: []x509.ExtKeyUsage{ + x509.ExtKeyUsageAny, + x509.ExtKeyUsageCodeSigning, + }, + DNSNames: []string{"example.com", "foo.example.com"}, + }, + }, } for name, test := range tests { diff --git a/test/e2e/framework/helper/validation/certificatesigningrequests/certificatesigningrequests.go b/test/e2e/framework/helper/validation/certificatesigningrequests/certificatesigningrequests.go index f610f1351..84cc524c7 100644 --- a/test/e2e/framework/helper/validation/certificatesigningrequests/certificatesigningrequests.go +++ b/test/e2e/framework/helper/validation/certificatesigningrequests/certificatesigningrequests.go @@ -194,8 +194,12 @@ func ExpectValidDuration(csr *certificatesv1.CertificateSigningRequest, _ crypto var expectedDuration time.Duration durationString, ok := csr.Annotations[experimentalapi.CertificateSigningRequestDurationAnnotationKey] if !ok { - // If duration wasn't requested, then we match against the default. - expectedDuration = cmapi.DefaultCertificateDuration + if csr.Spec.ExpirationSeconds != nil { + expectedDuration = time.Duration(*csr.Spec.ExpirationSeconds) * time.Second + } else { + // If duration wasn't requested, then we match against the default. + expectedDuration = cmapi.DefaultCertificateDuration + } } else { expectedDuration, err = time.ParseDuration(durationString) if err != nil { diff --git a/test/e2e/suite/conformance/certificatesigningrequests/BUILD.bazel b/test/e2e/suite/conformance/certificatesigningrequests/BUILD.bazel index 8378a89f5..db999c751 100644 --- a/test/e2e/suite/conformance/certificatesigningrequests/BUILD.bazel +++ b/test/e2e/suite/conformance/certificatesigningrequests/BUILD.bazel @@ -24,6 +24,7 @@ go_library( "@io_k8s_api//certificates/v1:go_default_library", "@io_k8s_api//core/v1:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", + "@io_k8s_utils//pointer:go_default_library", ], ) diff --git a/test/e2e/suite/conformance/certificatesigningrequests/tests.go b/test/e2e/suite/conformance/certificatesigningrequests/tests.go index 554437497..a5299de2e 100644 --- a/test/e2e/suite/conformance/certificatesigningrequests/tests.go +++ b/test/e2e/suite/conformance/certificatesigningrequests/tests.go @@ -28,6 +28,7 @@ import ( certificatesv1 "k8s.io/api/certificates/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/pointer" experimentalapi "github.com/cert-manager/cert-manager/pkg/apis/experimental/v1alpha1" "github.com/cert-manager/cert-manager/pkg/util" @@ -76,9 +77,10 @@ func (s *Suite) Define() { // csrModifers define the shape of the X.509 CSR which is used in the // test case. We use a function to allow access to variables that are // initialized at test runtime by complete(). - csrModifiers func() []gen.CSRModifier - kubeCSRUsages []certificatesv1.KeyUsage - kubeCSRAnnotations map[string]string + csrModifiers func() []gen.CSRModifier + kubeCSRUsages []certificatesv1.KeyUsage + kubeCSRAnnotations map[string]string + kubeCSRExpirationSeconds *int32 // The list of features that are required by the Issuer for the test to // run. requiredFeatures []featureset.Feature @@ -252,6 +254,22 @@ func (s *Suite) Define() { requiredFeatures: []featureset.Feature{featureset.DurationFeature}, }, + "should issue a certificate that defines a Common Name, DNS Name, and sets a duration via expiration seconds": { + keyAlgo: x509.RSA, + csrModifiers: func() []gen.CSRModifier { + return []gen.CSRModifier{ + gen.SetCSRDNSNames(sharedCommonName), + gen.SetCSRDNSNames(sharedCommonName), + } + }, + kubeCSRUsages: []certificatesv1.KeyUsage{ + certificatesv1.UsageDigitalSignature, + certificatesv1.UsageKeyEncipherment, + }, + kubeCSRExpirationSeconds: pointer.Int32(3333), + requiredFeatures: []featureset.Feature{featureset.DurationFeature}, + }, + "should issue a certificate that defines a DNS Name and sets a duration": { keyAlgo: x509.RSA, csrModifiers: func() []gen.CSRModifier { @@ -353,9 +371,10 @@ func (s *Suite) Define() { Annotations: test.kubeCSRAnnotations, }, Spec: certificatesv1.CertificateSigningRequestSpec{ - Request: csr, - SignerName: signerName, - Usages: test.kubeCSRUsages, + Request: csr, + SignerName: signerName, + Usages: test.kubeCSRUsages, + ExpirationSeconds: test.kubeCSRExpirationSeconds, }, } diff --git a/test/unit/gen/certificatesigningrequest.go b/test/unit/gen/certificatesigningrequest.go index d6e7aa332..50df072a6 100644 --- a/test/unit/gen/certificatesigningrequest.go +++ b/test/unit/gen/certificatesigningrequest.go @@ -95,6 +95,12 @@ func SetCertificateSigningRequestSignerName(signerName string) CertificateSignin } } +func SetCertificateSigningRequestExpirationSeconds(seconds int32) CertificateSigningRequestModifier { + return func(csr *certificatesv1.CertificateSigningRequest) { + csr.Spec.ExpirationSeconds = &seconds + } +} + func SetCertificateSigningRequestDuration(duration string) CertificateSigningRequestModifier { return AddCertificateSigningRequestAnnotations(map[string]string{ experimentalapi.CertificateSigningRequestDurationAnnotationKey: duration,