From 9ca869c2cfa5d897e22f3477ba4745cf52d4cd2d Mon Sep 17 00:00:00 2001 From: joshvanl Date: Tue, 1 Feb 2022 15:33:47 +0000 Subject: [PATCH] Add tests to secret manager for additional output formats Signed-off-by: joshvanl --- .../certificates/issuing/internal/secret.go | 7 +- .../certificates/issuing/secret_manager.go | 31 ++- .../issuing/secret_manager_test.go | 208 +++++++++++++++++- 3 files changed, 228 insertions(+), 18 deletions(-) diff --git a/pkg/controller/certificates/issuing/internal/secret.go b/pkg/controller/certificates/issuing/internal/secret.go index 2fe783634..c145dcb07 100644 --- a/pkg/controller/certificates/issuing/internal/secret.go +++ b/pkg/controller/certificates/issuing/internal/secret.go @@ -17,10 +17,8 @@ limitations under the License. package internal import ( - "bytes" "context" "crypto/x509" - "encoding/pem" "fmt" corev1 "k8s.io/api/core/v1" @@ -287,11 +285,10 @@ func setAdditionalOutputFormats(crt *cmapi.Certificate, secret *corev1.Secret, d switch format.Type { case cmapi.CertificateOutputFormatDER: // Store binary format of the private key - block, _ := pem.Decode(data.PrivateKey) - secret.Data[cmapi.CertificateOutputFormatDERKey] = block.Bytes + secret.Data[cmapi.CertificateOutputFormatDERKey] = certificates.OutputFormatDER(data.PrivateKey) case cmapi.CertificateOutputFormatCombinedPEM: // Combine tls.key and tls.crt - secret.Data[cmapi.CertificateOutputFormatCombinedPEMKey] = bytes.Join([][]byte{data.PrivateKey, data.Certificate}, []byte("\n")) + secret.Data[cmapi.CertificateOutputFormatCombinedPEMKey] = certificates.OutputFormatCombinedPEM(data.PrivateKey, data.Certificate) default: return fmt.Errorf("unknown additional output format %s", format.Type) } diff --git a/pkg/controller/certificates/issuing/secret_manager.go b/pkg/controller/certificates/issuing/secret_manager.go index 433525b8f..319bac993 100644 --- a/pkg/controller/certificates/issuing/secret_manager.go +++ b/pkg/controller/certificates/issuing/secret_manager.go @@ -32,18 +32,17 @@ import ( ) // ensureSecretData ensures that the Certificate's Secret is up to date with -// non-issuing condition related data. Currently only reconciles on Annotations -// and Labels from the Certificate's SecretTemplate. +// non-issuing condition related data. +// Reconciles over the Certificate's SecretTemplate, and +// AdditionalOutputFormats. func (c *controller) ensureSecretData(ctx context.Context, log logr.Logger, crt *cmapi.Certificate) error { - dbg := log.V(logf.DebugLevel) - // Retrieve the Secret which is associated with this Certificate. secret, err := c.secretLister.Secrets(crt.Namespace).Get(crt.Spec.SecretName) // Secret doesn't exist so we can't do anything. The Certificate will be // marked for a re-issuance and the resulting Secret will be evaluated again. if apierrors.IsNotFound(err) { - dbg.Info("secret not found", "error", err.Error()) + log.V(logf.DebugLevel).Info("secret not found", "error", err.Error()) return nil } @@ -55,13 +54,21 @@ func (c *controller) ensureSecretData(ctx context.Context, log logr.Logger, crt log = log.WithValues("secret", secret.Name) - var data internal.SecretData - if secret.Data != nil { - data = internal.SecretData{ - PrivateKey: secret.Data[corev1.TLSPrivateKeyKey], - Certificate: secret.Data[corev1.TLSCertKey], - CA: secret.Data[cmmeta.TLSCAKey], - } + // If there is no certificate or private key data available at the target + // Secret then exit early. The absense of these keys should cause an issuance + // of the Certificate, so there is no need to run post issuance checks. + if secret.Data == nil || + len(secret.Data[corev1.TLSCertKey]) == 0 || + len(secret.Data[corev1.TLSPrivateKeyKey]) == 0 { + log.V(logf.DebugLevel).Info("secret doesn't contain both certificate and private key data", + "cert_data_len", len(secret.Data[corev1.TLSCertKey]), "key_data_len", len(secret.Data[corev1.TLSPrivateKeyKey])) + return nil + } + + data := internal.SecretData{ + PrivateKey: secret.Data[corev1.TLSPrivateKeyKey], + Certificate: secret.Data[corev1.TLSCertKey], + CA: secret.Data[cmmeta.TLSCAKey], } // Check whether the Certificate's Secret has correct output format and diff --git a/pkg/controller/certificates/issuing/secret_manager_test.go b/pkg/controller/certificates/issuing/secret_manager_test.go index a34197f66..aada14269 100644 --- a/pkg/controller/certificates/issuing/secret_manager_test.go +++ b/pkg/controller/certificates/issuing/secret_manager_test.go @@ -18,6 +18,7 @@ package issuing import ( "context" + "encoding/pem" "testing" "github.com/stretchr/testify/assert" @@ -29,11 +30,18 @@ import ( cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" "github.com/cert-manager/cert-manager/pkg/controller/certificates/issuing/internal" testpkg "github.com/cert-manager/cert-manager/pkg/controller/test" + testcrypto "github.com/cert-manager/cert-manager/test/unit/crypto" ) func Test_ensureSecretData(t *testing.T) { const fieldManager = "cert-manager-unit-tests" + pk := testcrypto.MustCreatePEMPrivateKey(t) + cert := testcrypto.MustCreateCert(t, pk, &cmapi.Certificate{Spec: cmapi.CertificateSpec{CommonName: "test"}}) + block, _ := pem.Decode(pk) + pkDER := block.Bytes + combinedPEM := append(append(pk, '\n'), cert...) + tests := map[string]struct { // key that should be passed to ProcessItem. // if not set, the 'namespace/name' of the 'Certificate' field will be used. @@ -61,7 +69,57 @@ func Test_ensureSecretData(t *testing.T) { key: "random-namespace/random-certificate", expectedAction: false, }, - "if Certificate and Secret exists, but the Certificate has a True Issuing condition, do nothing": {key: "test-namespace/test-name", + "if Certificate and Secret exists, but the Secret contains no certificate or private key data, do nothing": { + key: "test-namespace/test-name", + cert: &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-name"}, + Spec: cmapi.CertificateSpec{ + SecretName: "test-secret", + SecretTemplate: &cmapi.CertificateSecretTemplate{Annotations: map[string]string{"foo": "bar"}, Labels: map[string]string{"abc": "123"}}, + }, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-secret"}, + Data: map[string][]byte{}, + }, + expectedAction: false, + }, + "if Certificate and Secret exists, but the Secret contains no certificate data, do nothing": { + key: "test-namespace/test-name", + cert: &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-name"}, + Spec: cmapi.CertificateSpec{ + SecretName: "test-secret", + SecretTemplate: &cmapi.CertificateSecretTemplate{Annotations: map[string]string{"foo": "bar"}, Labels: map[string]string{"abc": "123"}}, + }, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-secret"}, + Data: map[string][]byte{ + "tls.key": pk, + }, + }, + expectedAction: false, + }, + "if Certificate and Secret exists, but the Secret contains no private key data, do nothing": { + key: "test-namespace/test-name", + cert: &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-name"}, + Spec: cmapi.CertificateSpec{ + SecretName: "test-secret", + SecretTemplate: &cmapi.CertificateSecretTemplate{Annotations: map[string]string{"foo": "bar"}, Labels: map[string]string{"abc": "123"}}, + }, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-secret"}, + Data: map[string][]byte{ + "tls.cert": cert, + }, + }, + expectedAction: false, + }, + "if Certificate and Secret exists, but the Certificate has a True Issuing condition, do nothing": { + key: "test-namespace/test-name", cert: &cmapi.Certificate{ ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-name"}, Spec: cmapi.CertificateSpec{ @@ -74,6 +132,10 @@ func Test_ensureSecretData(t *testing.T) { }, secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-secret"}, + Data: map[string][]byte{ + "tls.crt": cert, + "tls.key": pk, + }, }, expectedAction: false, }, @@ -107,6 +169,10 @@ func Test_ensureSecretData(t *testing.T) { Namespace: "test-namespace", Name: "test-secret", Annotations: map[string]string{"foo": "bar"}, Labels: map[string]string{"abc": "123"}, }, + Data: map[string][]byte{ + "tls.crt": cert, + "tls.key": pk, + }, }, expectedAction: true, }, @@ -142,6 +208,10 @@ func Test_ensureSecretData(t *testing.T) { }, }}, }, + Data: map[string][]byte{ + "tls.crt": cert, + "tls.key": pk, + }, }, expectedAction: true, }, @@ -175,6 +245,10 @@ func Test_ensureSecretData(t *testing.T) { }}, }, }, + Data: map[string][]byte{ + "tls.crt": cert, + "tls.key": pk, + }, }, expectedAction: true, }, @@ -208,6 +282,10 @@ func Test_ensureSecretData(t *testing.T) { }}, }, }, + Data: map[string][]byte{ + "tls.crt": cert, + "tls.key": pk, + }, }, expectedAction: false, }, @@ -228,6 +306,134 @@ func Test_ensureSecretData(t *testing.T) { }, secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-secret"}, + Data: map[string][]byte{ + "tls.crt": cert, + "tls.key": pk, + }, + }, + expectedAction: true, + }, + "if Certificate with combined pem and Secret exists, but the Secret doesn't have combined pem, should apply the combined pem": { + key: "test-namespace/test-name", + cert: &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-name"}, + Spec: cmapi.CertificateSpec{ + SecretName: "test-secret", + AdditionalOutputFormats: []cmapi.CertificateAdditionalOutputFormat{ + {Type: "CombinedPEM"}, + }, + }, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-secret"}, + Data: map[string][]byte{ + "tls.crt": cert, + "tls.key": pk, + }, + }, + expectedAction: true, + }, + "if Certificate with der and Secret exists, but the Secret doesn't have der, should apply the der": { + key: "test-namespace/test-name", + cert: &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-name"}, + Spec: cmapi.CertificateSpec{ + SecretName: "test-secret", + AdditionalOutputFormats: []cmapi.CertificateAdditionalOutputFormat{ + {Type: "DER"}, + }, + }, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-secret"}, + Data: map[string][]byte{ + "tls.crt": cert, + "tls.key": pk, + }, + }, + expectedAction: true, + }, + "if Certificate with combined pem and der, and Secret exists, but the Secret doesn't have combined pem or der, should apply the combined pem and der": { + key: "test-namespace/test-name", + cert: &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-name"}, + Spec: cmapi.CertificateSpec{ + SecretName: "test-secret", + AdditionalOutputFormats: []cmapi.CertificateAdditionalOutputFormat{ + {Type: "CombinedPEM"}, + {Type: "DER"}, + }, + }, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-secret"}, + Data: map[string][]byte{ + "tls.crt": cert, + "tls.key": pk, + }, + }, + expectedAction: true, + }, + "if Certificate with combined pem and der, and Secret exists with combined pem and der with managed fields, should do nothing": { + key: "test-namespace/test-name", + cert: &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-name"}, + Spec: cmapi.CertificateSpec{ + SecretName: "test-secret", + AdditionalOutputFormats: []cmapi.CertificateAdditionalOutputFormat{ + {Type: "CombinedPEM"}, + {Type: "DER"}, + }, + }, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-secret", + ManagedFields: []metav1.ManagedFieldsEntry{{ + Manager: fieldManager, + FieldsV1: &metav1.FieldsV1{ + Raw: []byte(`{"f:data": { + "f:tls-combined.pem": {}, + "f:key.der": {} + }}`), + }, + }}, + }, + Data: map[string][]byte{ + "tls.crt": cert, + "tls.key": pk, + "tls-combined.pem": combinedPEM, + "key.der": pkDER, + }, + }, + expectedAction: false, + }, + "if Certificate with no combined pem or der, and Secret exists with combined pem and der managed by field manager, should apply to remove them": { + key: "test-namespace/test-name", + cert: &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-name"}, + Spec: cmapi.CertificateSpec{ + SecretName: "test-secret", + AdditionalOutputFormats: []cmapi.CertificateAdditionalOutputFormat{}, + }, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "test-namespace", Name: "test-secret", + ManagedFields: []metav1.ManagedFieldsEntry{{ + Manager: fieldManager, + FieldsV1: &metav1.FieldsV1{ + Raw: []byte(`{"f:data": { + "f:tls-combined.pem": {}, + "f:key.der": {} + }}`), + }, + }}, + }, + Data: map[string][]byte{ + "tls.crt": cert, + "tls.key": pk, + "tls-combined.pem": combinedPEM, + "key.der": pkDER, + }, }, expectedAction: true, },