From 22440e8710e2500770b98eb7fd3588ad579fd956 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Tue, 20 Jun 2023 16:30:25 +0200 Subject: [PATCH 1/5] add SecretPublicKeysDiffersFromCurrentCertificateRequest check Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- .../certificates/policies/checks.go | 25 +++++++++++++++++++ .../certificates/policies/checks_test.go | 25 +++++++++++++++++++ .../certificates/policies/constants.go | 4 +++ .../certificates/policies/policies.go | 2 ++ 4 files changed, 56 insertions(+) diff --git a/internal/controller/certificates/policies/checks.go b/internal/controller/certificates/policies/checks.go index 9e6f1224a97..10a260fa940 100644 --- a/internal/controller/certificates/policies/checks.go +++ b/internal/controller/certificates/policies/checks.go @@ -166,6 +166,31 @@ func SecretIssuerAnnotationsNotUpToDate(input Input) (string, string, bool) { return "", "", false } +func SecretPublicKeysDiffersFromCurrentCertificateRequest(input Input) (string, string, bool) { + if input.CurrentRevisionRequest == nil { + return "", "", false + } + pk, err := pki.DecodePrivateKeyBytes(input.Secret.Data[corev1.TLSPrivateKeyKey]) + if err != nil { + return InvalidKeyPair, fmt.Sprintf("Issuing certificate as Secret contains invalid private key data: %v", err), true + } + + csr, err := pki.DecodeX509CertificateRequestBytes(input.CurrentRevisionRequest.Spec.Request) + if err != nil { + return InvalidCertificateRequest, fmt.Sprintf("Failed to decode current CertificateRequest: %v", err), true + } + + equal, err := pki.PublicKeysEqual(csr.PublicKey, pk.Public()) + if err != nil { + return InvalidCertificateRequest, fmt.Sprintf("CertificateRequest's public key is invalid: %v", err), true + } + if !equal { + return InvalidCertificateRequest, "Secret contains a private key that does not match the current CertificateRequest", true + } + + return "", "", false +} + func CurrentCertificateRequestNotValidForSpec(input Input) (string, string, bool) { if input.CurrentRevisionRequest == nil { // Fallback to comparing the Certificate spec with the issued certificate. diff --git a/internal/controller/certificates/policies/checks_test.go b/internal/controller/certificates/policies/checks_test.go index 381952cb21c..26482dcc4cb 100644 --- a/internal/controller/certificates/policies/checks_test.go +++ b/internal/controller/certificates/policies/checks_test.go @@ -213,6 +213,31 @@ func Test_NewTriggerPolicyChain(t *testing.T) { message: "Issuing certificate as Secret was previously issued by IssuerKind.new.example.com/testissuer", reissue: true, }, + "trigger issuance as current CertificateRequest is not signed with private key": { + certificate: &cmapi.Certificate{Spec: cmapi.CertificateSpec{SecretName: "something"}}, + secret: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "something"}, + Data: map[string][]byte{ + corev1.TLSPrivateKeyKey: staticFixedPrivateKey, + corev1.TLSCertKey: testcrypto.MustCreateCert( + t, staticFixedPrivateKey, + &cmapi.Certificate{Spec: cmapi.CertificateSpec{CommonName: "example.com"}}, + ), + }, + }, + request: &cmapi.CertificateRequest{Spec: cmapi.CertificateRequestSpec{ + IssuerRef: cmmeta.ObjectReference{ + Name: "testissuer", + Kind: "IssuerKind", + Group: "group.example.com", + }, + Request: testcrypto.MustGenerateCSRImpl(t, testcrypto.MustCreatePEMPrivateKey(t), &cmapi.Certificate{Spec: cmapi.CertificateSpec{ + CommonName: "example.com", + }}), + }}, + reason: InvalidCertificateRequest, + message: "Secret contains a private key that does not match the current CertificateRequest", + reissue: true, + }, // we only have a basic test here for this as unit tests for the // `pki.RequestMatchesSpec` function cover all other cases. "trigger issuance when CertificateRequest does not match certificate spec": { diff --git a/internal/controller/certificates/policies/constants.go b/internal/controller/certificates/policies/constants.go index 6afde6e0e74..32d556fdd58 100644 --- a/internal/controller/certificates/policies/constants.go +++ b/internal/controller/certificates/policies/constants.go @@ -29,6 +29,10 @@ const ( // InvalidCertificate is a policy violation whereby the signed certificate in // the Input Secret could not be parsed or decoded. InvalidCertificate string = "InvalidCertificate" + // InvalidCertificateRequest is a policy violation whereby the CSR in + // the Input CertificateRequest could not be parsed or decoded. + InvalidCertificateRequest string = "InvalidCertificateRequest" + // SecretMismatch is a policy violation reason for a scenario where Secret's // private key does not match spec. SecretMismatch string = "SecretMismatch" diff --git a/internal/controller/certificates/policies/policies.go b/internal/controller/certificates/policies/policies.go index ff8f27cc56b..3087e821234 100644 --- a/internal/controller/certificates/policies/policies.go +++ b/internal/controller/certificates/policies/policies.go @@ -72,6 +72,7 @@ func NewTriggerPolicyChain(c clock.Clock) Chain { SecretPublicKeysDiffer, SecretPrivateKeyMatchesSpec, SecretIssuerAnnotationsNotUpToDate, + SecretPublicKeysDiffersFromCurrentCertificateRequest, CurrentCertificateRequestNotValidForSpec, CurrentCertificateNearingExpiry(c), } @@ -84,6 +85,7 @@ func NewReadinessPolicyChain(c clock.Clock) Chain { SecretDoesNotExist, SecretIsMissingData, SecretPublicKeysDiffer, + SecretPublicKeysDiffersFromCurrentCertificateRequest, CurrentCertificateRequestNotValidForSpec, CurrentCertificateHasExpired(c), } From d310d8597c06cb81e39dca90c215ef0061dacf73 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Tue, 20 Jun 2023 16:36:46 +0200 Subject: [PATCH 2/5] improve comments Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- internal/controller/certificates/policies/checks.go | 4 ++++ internal/controller/certificates/policies/constants.go | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/controller/certificates/policies/checks.go b/internal/controller/certificates/policies/checks.go index 10a260fa940..8f1b2245453 100644 --- a/internal/controller/certificates/policies/checks.go +++ b/internal/controller/certificates/policies/checks.go @@ -166,6 +166,10 @@ func SecretIssuerAnnotationsNotUpToDate(input Input) (string, string, bool) { return "", "", false } +// SecretCertificateMatchesSpec checks that the current CertificateRequest contains a CSR that is +// signed by the key stored in the Secret. A failure is often caused by the Secret being changed +// outside of the control of cert-manager, causing the current CertificateRequest to no longer +// match what is stored in the Secret. func SecretPublicKeysDiffersFromCurrentCertificateRequest(input Input) (string, string, bool) { if input.CurrentRevisionRequest == nil { return "", "", false diff --git a/internal/controller/certificates/policies/constants.go b/internal/controller/certificates/policies/constants.go index 32d556fdd58..011d7bfce28 100644 --- a/internal/controller/certificates/policies/constants.go +++ b/internal/controller/certificates/policies/constants.go @@ -30,7 +30,8 @@ const ( // the Input Secret could not be parsed or decoded. InvalidCertificate string = "InvalidCertificate" // InvalidCertificateRequest is a policy violation whereby the CSR in - // the Input CertificateRequest could not be parsed or decoded. + // the Input CertificateRequest could not be parsed or decoded or is + // eg. signed using an unknown key. InvalidCertificateRequest string = "InvalidCertificateRequest" // SecretMismatch is a policy violation reason for a scenario where Secret's From 82499eb75bd1a307ff3c07e8a9f5c92d86ec3232 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Tue, 20 Jun 2023 19:06:02 +0200 Subject: [PATCH 3/5] fix failing TestNewReadinessPolicyChain test Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- .../readiness/readiness_controller_test.go | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/pkg/controller/certificates/readiness/readiness_controller_test.go b/pkg/controller/certificates/readiness/readiness_controller_test.go index b4da05de563..1e0a6c11611 100644 --- a/pkg/controller/certificates/readiness/readiness_controller_test.go +++ b/pkg/controller/certificates/readiness/readiness_controller_test.go @@ -467,7 +467,38 @@ func TestNewReadinessPolicyChain(t *testing.T) { Name: "testissuer", Kind: "IssuerKind", Group: "group.example.com", + }), + gen.SetCertificateRequestCSR(testcrypto.MustGenerateCSRImpl(t, privKey, + gen.Certificate("something", + gen.SetCertificateCommonName("new.example.com")))), + ), + reason: policies.Expired, + message: "Certificate expired on Sun, 31 Dec 0000 23:00:00 UTC", + violationFound: true, + }, + "Certificate is not Ready when it has expired (no cr)": { + cert: gen.Certificate("something", + gen.SetCertificateCommonName("new.example.com"), + gen.SetCertificateIssuer(cmmeta.ObjectReference{ + Name: "testissuer", + Kind: "IssuerKind", + Group: "group.example.com", })), + secret: gen.Secret("something", + gen.SetSecretAnnotations(map[string]string{ + cmapi.IssuerNameAnnotationKey: "testissuer", + cmapi.IssuerKindAnnotationKey: "IssuerKind", + cmapi.IssuerGroupAnnotationKey: "group.example.com", + }), + gen.SetSecretData( + map[string][]byte{ + corev1.TLSPrivateKeyKey: privKey, + corev1.TLSCertKey: testcrypto.MustCreateCertWithNotBeforeAfter(t, privKey, + gen.Certificate("something", gen.SetCertificateCommonName("new.example.com")), + clock.Now().Add(-3*time.Hour), clock.Now().Add(-1*time.Hour), + ), + }, + )), reason: policies.Expired, message: "Certificate expired on Sun, 31 Dec 0000 23:00:00 UTC", violationFound: true, From 19377b43b1ac523e9dcf3213e736eda510a1f70a Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Wed, 21 Jun 2023 15:31:20 +0200 Subject: [PATCH 4/5] fix feedback from @wallrj Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- .../certificates/policies/checks.go | 12 ++++----- .../certificates/policies/constants.go | 3 +-- .../certificates/policies/policies.go | 4 +-- .../readiness/readiness_controller_test.go | 27 ------------------- 4 files changed, 9 insertions(+), 37 deletions(-) diff --git a/internal/controller/certificates/policies/checks.go b/internal/controller/certificates/policies/checks.go index 8f1b2245453..4341c773c54 100644 --- a/internal/controller/certificates/policies/checks.go +++ b/internal/controller/certificates/policies/checks.go @@ -166,11 +166,11 @@ func SecretIssuerAnnotationsNotUpToDate(input Input) (string, string, bool) { return "", "", false } -// SecretCertificateMatchesSpec checks that the current CertificateRequest contains a CSR that is -// signed by the key stored in the Secret. A failure is often caused by the Secret being changed -// outside of the control of cert-manager, causing the current CertificateRequest to no longer -// match what is stored in the Secret. -func SecretPublicKeysDiffersFromCurrentCertificateRequest(input Input) (string, string, bool) { +// SecretPublicKeyDiffersFromCurrentCertificateRequest checks that the current CertificateRequest +// contains a CSR that is signed by the key stored in the Secret. A failure is often caused by the +// Secret being changed outside of the control of cert-manager, causing the current CertificateRequest +// to no longer match what is stored in the Secret. +func SecretPublicKeyDiffersFromCurrentCertificateRequest(input Input) (string, string, bool) { if input.CurrentRevisionRequest == nil { return "", "", false } @@ -189,7 +189,7 @@ func SecretPublicKeysDiffersFromCurrentCertificateRequest(input Input) (string, return InvalidCertificateRequest, fmt.Sprintf("CertificateRequest's public key is invalid: %v", err), true } if !equal { - return InvalidCertificateRequest, "Secret contains a private key that does not match the current CertificateRequest", true + return SecretMismatch, "Secret contains a private key that does not match the current CertificateRequest", true } return "", "", false diff --git a/internal/controller/certificates/policies/constants.go b/internal/controller/certificates/policies/constants.go index 011d7bfce28..32d556fdd58 100644 --- a/internal/controller/certificates/policies/constants.go +++ b/internal/controller/certificates/policies/constants.go @@ -30,8 +30,7 @@ const ( // the Input Secret could not be parsed or decoded. InvalidCertificate string = "InvalidCertificate" // InvalidCertificateRequest is a policy violation whereby the CSR in - // the Input CertificateRequest could not be parsed or decoded or is - // eg. signed using an unknown key. + // the Input CertificateRequest could not be parsed or decoded. InvalidCertificateRequest string = "InvalidCertificateRequest" // SecretMismatch is a policy violation reason for a scenario where Secret's diff --git a/internal/controller/certificates/policies/policies.go b/internal/controller/certificates/policies/policies.go index 3087e821234..37dd60caa41 100644 --- a/internal/controller/certificates/policies/policies.go +++ b/internal/controller/certificates/policies/policies.go @@ -72,7 +72,7 @@ func NewTriggerPolicyChain(c clock.Clock) Chain { SecretPublicKeysDiffer, SecretPrivateKeyMatchesSpec, SecretIssuerAnnotationsNotUpToDate, - SecretPublicKeysDiffersFromCurrentCertificateRequest, + SecretPublicKeyDiffersFromCurrentCertificateRequest, CurrentCertificateRequestNotValidForSpec, CurrentCertificateNearingExpiry(c), } @@ -85,7 +85,7 @@ func NewReadinessPolicyChain(c clock.Clock) Chain { SecretDoesNotExist, SecretIsMissingData, SecretPublicKeysDiffer, - SecretPublicKeysDiffersFromCurrentCertificateRequest, + SecretPublicKeyDiffersFromCurrentCertificateRequest, CurrentCertificateRequestNotValidForSpec, CurrentCertificateHasExpired(c), } diff --git a/pkg/controller/certificates/readiness/readiness_controller_test.go b/pkg/controller/certificates/readiness/readiness_controller_test.go index 1e0a6c11611..1b44908ca0f 100644 --- a/pkg/controller/certificates/readiness/readiness_controller_test.go +++ b/pkg/controller/certificates/readiness/readiness_controller_test.go @@ -476,33 +476,6 @@ func TestNewReadinessPolicyChain(t *testing.T) { message: "Certificate expired on Sun, 31 Dec 0000 23:00:00 UTC", violationFound: true, }, - "Certificate is not Ready when it has expired (no cr)": { - cert: gen.Certificate("something", - gen.SetCertificateCommonName("new.example.com"), - gen.SetCertificateIssuer(cmmeta.ObjectReference{ - Name: "testissuer", - Kind: "IssuerKind", - Group: "group.example.com", - })), - secret: gen.Secret("something", - gen.SetSecretAnnotations(map[string]string{ - cmapi.IssuerNameAnnotationKey: "testissuer", - cmapi.IssuerKindAnnotationKey: "IssuerKind", - cmapi.IssuerGroupAnnotationKey: "group.example.com", - }), - gen.SetSecretData( - map[string][]byte{ - corev1.TLSPrivateKeyKey: privKey, - corev1.TLSCertKey: testcrypto.MustCreateCertWithNotBeforeAfter(t, privKey, - gen.Certificate("something", gen.SetCertificateCommonName("new.example.com")), - clock.Now().Add(-3*time.Hour), clock.Now().Add(-1*time.Hour), - ), - }, - )), - reason: policies.Expired, - message: "Certificate expired on Sun, 31 Dec 0000 23:00:00 UTC", - violationFound: true, - }, "Certificate is Ready, no policy violations found": { cert: gen.Certificate("something", gen.SetCertificateCommonName("new.example.com"), From 229f99c197b352c8274a1fe1e515b386df5e6a0a Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Fri, 23 Jun 2023 09:14:38 +0200 Subject: [PATCH 5/5] update testcase based on feedback Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- internal/controller/certificates/policies/checks_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controller/certificates/policies/checks_test.go b/internal/controller/certificates/policies/checks_test.go index 26482dcc4cb..09aa477e95b 100644 --- a/internal/controller/certificates/policies/checks_test.go +++ b/internal/controller/certificates/policies/checks_test.go @@ -213,7 +213,7 @@ func Test_NewTriggerPolicyChain(t *testing.T) { message: "Issuing certificate as Secret was previously issued by IssuerKind.new.example.com/testissuer", reissue: true, }, - "trigger issuance as current CertificateRequest is not signed with private key": { + "trigger if the Secret contains a different private key than was used to sign the CSR": { certificate: &cmapi.Certificate{Spec: cmapi.CertificateSpec{SecretName: "something"}}, secret: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "something"}, Data: map[string][]byte{ @@ -234,7 +234,7 @@ func Test_NewTriggerPolicyChain(t *testing.T) { CommonName: "example.com", }}), }}, - reason: InvalidCertificateRequest, + reason: SecretMismatch, message: "Secret contains a private key that does not match the current CertificateRequest", reissue: true, },