From d03dde81301e1c6234114a2e1ffe282a1987851e Mon Sep 17 00:00:00 2001 From: Alex Richman Date: Thu, 19 Sep 2024 20:19:17 +0000 Subject: [PATCH] Asynchronously issue certificates Signed-off-by: Alex Richman --- pkg/aws/pca.go | 32 ++-- pkg/aws/pca_test.go | 60 ++++++- .../certificaterequest_controller.go | 32 +++- .../certificaterequest_controller_test.go | 157 ++++++++++++++++-- 4 files changed, 244 insertions(+), 37 deletions(-) diff --git a/pkg/aws/pca.go b/pkg/aws/pca.go index 0030d7ce..22a77b73 100644 --- a/pkg/aws/pca.go +++ b/pkg/aws/pca.go @@ -33,6 +33,7 @@ import ( injections "github.com/cert-manager/aws-privateca-issuer/pkg/api/injections" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/go-logr/logr" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ) @@ -42,7 +43,8 @@ var collection = new(sync.Map) // GenericProvisioner abstracts over the Provisioner type for mocking purposes type GenericProvisioner interface { - Sign(ctx context.Context, cr *cmapi.CertificateRequest, log logr.Logger) ([]byte, []byte, error) + Get(ctx context.Context, cr *cmapi.CertificateRequest, certArn string, log logr.Logger) ([]byte, []byte, error) + Sign(ctx context.Context, cr *cmapi.CertificateRequest, log logr.Logger) error } // acmPCAClient abstracts over the methods used from acmpca.Client @@ -94,10 +96,10 @@ func idempotencyToken(cr *cmapi.CertificateRequest) string { } // Sign takes a certificate request and signs it using PCA -func (p *PCAProvisioner) Sign(ctx context.Context, cr *cmapi.CertificateRequest, log logr.Logger) ([]byte, []byte, error) { +func (p *PCAProvisioner) Sign(ctx context.Context, cr *cmapi.CertificateRequest, log logr.Logger) error { block, _ := pem.Decode(cr.Spec.Request) if block == nil { - return nil, nil, fmt.Errorf("failed to decode CSR") + return fmt.Errorf("failed to decode CSR") } validityExpiration := int64(p.now().Unix()) + DEFAULT_DURATION @@ -112,7 +114,7 @@ func (p *PCAProvisioner) Sign(ctx context.Context, cr *cmapi.CertificateRequest, err := getSigningAlgorithm(ctx, p) if err != nil { - return nil, nil, err + return err } issueParams := acmpca.IssueCertificateInput{ @@ -130,20 +132,20 @@ func (p *PCAProvisioner) Sign(ctx context.Context, cr *cmapi.CertificateRequest, issueOutput, err := p.pcaClient.IssueCertificate(ctx, &issueParams) if err != nil { - return nil, nil, err + return err } - getParams := acmpca.GetCertificateInput{ - CertificateArn: aws.String(*issueOutput.CertificateArn), - CertificateAuthorityArn: aws.String(p.arn), - } + metav1.SetMetaDataAnnotation(&cr.ObjectMeta, "aws-privateca-issuer/certificate-arn", *issueOutput.CertificateArn) - log.Info("Created certificate with arn: " + *issueOutput.CertificateArn) + log.Info("Issued certificate with arn: " + *issueOutput.CertificateArn) - waiter := acmpca.NewCertificateIssuedWaiter(p.pcaClient) - err = waiter.Wait(ctx, &getParams, 5*time.Minute) - if err != nil { - return nil, nil, err + return nil +} + +func (p *PCAProvisioner) Get(ctx context.Context, cr *cmapi.CertificateRequest, certArn string, log logr.Logger) ([]byte, []byte, error) { + getParams := acmpca.GetCertificateInput{ + CertificateArn: aws.String(certArn), + CertificateAuthorityArn: aws.String(p.arn), } getOutput, err := p.pcaClient.GetCertificate(ctx, &getParams) @@ -159,6 +161,8 @@ func (p *PCAProvisioner) Sign(ctx context.Context, cr *cmapi.CertificateRequest, } certPem = append(certPem, chainIntCAs...) + log.Info("Created certificate with arn: ") + return certPem, rootCA, nil } diff --git a/pkg/aws/pca_test.go b/pkg/aws/pca_test.go index 3ef0f88c..d81e0c79 100644 --- a/pkg/aws/pca_test.go +++ b/pkg/aws/pca_test.go @@ -173,6 +173,10 @@ func (m *errorACMPCAClient) IssueCertificate(_ context.Context, input *acmpca.Is return nil, errors.New("Cannot issue certificate") } +func (m *errorACMPCAClient) GetCertificate(_ context.Context, input *acmpca.GetCertificateInput, _ ...func(*acmpca.Options)) (*acmpca.GetCertificateOutput, error) { + return nil, errors.New("Cannot get certificate") +} + type workingACMPCAClient struct { acmPCAClient issueCertInput *acmpca.IssueCertificateInput @@ -334,7 +338,7 @@ func TestIdempotencyToken(t *testing.T) { } } -func TestPCASign(t *testing.T) { +func TestPCAGet(t *testing.T) { type testCase struct { provisioner PCAProvisioner expectFailure bool @@ -349,7 +353,7 @@ func TestPCASign(t *testing.T) { expectedChain: string([]byte(root + "\n")), expectedCert: string([]byte(cert + "\n" + intermediate + "\n")), }, - "failure-error-issueCertificate": { + "failure-error-getCertificate": { provisioner: PCAProvisioner{arn: arn, pcaClient: &errorACMPCAClient{}}, expectFailure: true, }, @@ -369,7 +373,8 @@ func TestPCASign(t *testing.T) { }, } - leaf, chain, err := tc.provisioner.Sign(context.TODO(), cr, logr.Discard()) + leaf, chain, err := tc.provisioner.Get(context.TODO(), cr, certArn, logr.Discard()) + if tc.expectFailure && err == nil { fmt.Print(err.Error()) assert.Fail(t, "Expected an error but received none") @@ -383,6 +388,53 @@ func TestPCASign(t *testing.T) { } } +func TestPCASign(t *testing.T) { + type testCase struct { + provisioner PCAProvisioner + expectFailure bool + expectedCertArn string + } + + tests := map[string]testCase{ + "success": { + provisioner: PCAProvisioner{arn: arn, pcaClient: &workingACMPCAClient{}}, + expectFailure: false, + expectedCertArn: "arn", + }, + "failure-error-issueCertificate": { + provisioner: PCAProvisioner{arn: arn, pcaClient: &errorACMPCAClient{}}, + expectFailure: true, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + key, _ := rsa.GenerateKey(rand.Reader, 2048) + csrBytes, _ := x509.CreateCertificateRequest(rand.Reader, &template, key) + + cr := &v1.CertificateRequest{ + Spec: v1.CertificateRequestSpec{ + Request: pem.EncodeToMemory(&pem.Block{ + Bytes: csrBytes, + Type: "CERTIFICATE REQUEST", + }), + }, + } + + err := tc.provisioner.Sign(context.TODO(), cr, logr.Discard()) + + if tc.expectFailure && err == nil { + fmt.Print(err.Error()) + assert.Fail(t, "Expected an error but received none") + } + + if tc.expectedCertArn != "" { + assert.Equal(t, cr.ObjectMeta.GetAnnotations()["aws-privateca-issuer/certificate-arn"], tc.expectedCertArn) + } + }) + } +} + func TestPCASignValidity(t *testing.T) { now := time.Now() client := &workingACMPCAClient{} @@ -432,7 +484,7 @@ func TestPCASignValidity(t *testing.T) { }, } - _, _, _ = provisioner.Sign(context.TODO(), cr, logr.Discard()) + _ = provisioner.Sign(context.TODO(), cr, logr.Discard()) got := client.issueCertInput if got == nil { assert.Fail(t, "Expected certificate input, got none") diff --git a/pkg/controllers/certificaterequest_controller.go b/pkg/controllers/certificaterequest_controller.go index e9459e75..4804ddbe 100644 --- a/pkg/controllers/certificaterequest_controller.go +++ b/pkg/controllers/certificaterequest_controller.go @@ -18,12 +18,14 @@ package controllers import ( "context" + "errors" "fmt" + acmpcatypes "github.com/aws/aws-sdk-go-v2/service/acmpca/types" "github.com/cert-manager/aws-privateca-issuer/pkg/aws" "github.com/cert-manager/aws-privateca-issuer/pkg/util" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - "k8s.io/apimachinery/pkg/api/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" @@ -65,7 +67,7 @@ func (r *CertificateRequestReconciler) Reconcile(ctx context.Context, req ctrl.R log := r.Log.WithValues("certificaterequest", req.NamespacedName) cr := new(cmapi.CertificateRequest) if err := r.Client.Get(ctx, req.NamespacedName, cr); err != nil { - if errors.IsNotFound(err) { + if apierrors.IsNotFound(err) { return ctrl.Result{}, client.IgnoreNotFound(err) } @@ -161,14 +163,31 @@ func (r *CertificateRequestReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, err } - pem, ca, err := provisioner.Sign(ctx, cr, log) + certArn, exists := cr.ObjectMeta.GetAnnotations()["aws-privateca-issuer/certificate-arn"] + if !exists { + err := provisioner.Sign(ctx, cr, log) + if err != nil { + log.Error(err, "failed to request certificate from PCA") + return ctrl.Result{}, r.setStatus(ctx, cr, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonFailed, "failed to request certificate from PCA: "+err.Error()) + } + + return ctrl.Result{Requeue: true}, r.Client.Update(ctx, cr) + } + + pem, ca, err := provisioner.Get(ctx, cr, certArn, log) if err != nil { - log.Error(err, "failed to request certificate from PCA") - return ctrl.Result{}, r.setStatus(ctx, cr, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonFailed, "failed to request certificate from PCA: "+err.Error()) + var errorType *acmpcatypes.RequestInProgressException + if errors.As(err, &errorType) { + log.Info("certificate is still issuing") + return ctrl.Result{Requeue: true}, r.setStatus(ctx, cr, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonPending, "waiting for certificate to be issued") + } + + log.Error(err, "failed to issue certificate from PCA") + return ctrl.Result{}, r.setStatus(ctx, cr, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonFailed, "failed to issue certificate from PCA: "+err.Error()) } + cr.Status.Certificate = pem cr.Status.CA = ca - return ctrl.Result{}, r.setStatus(ctx, cr, cmmeta.ConditionTrue, cmapi.CertificateRequestReasonIssued, "certificate issued") } @@ -197,6 +216,5 @@ func (r *CertificateRequestReconciler) setStatus(ctx context.Context, cr *cmapi. eventType = core.EventTypeWarning } r.Recorder.Event(cr, eventType, reason, completeMessage) - return r.Client.Status().Update(ctx, cr) } diff --git a/pkg/controllers/certificaterequest_controller_test.go b/pkg/controllers/certificaterequest_controller_test.go index b1e04fc9..94428e88 100644 --- a/pkg/controllers/certificaterequest_controller_test.go +++ b/pkg/controllers/certificaterequest_controller_test.go @@ -20,6 +20,7 @@ import ( "testing" "github.com/aws/aws-sdk-go-v2/aws" + acmpcatypes "github.com/aws/aws-sdk-go-v2/service/acmpca/types" cmutil "github.com/cert-manager/cert-manager/pkg/api/util" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" @@ -44,13 +45,19 @@ import ( ) type fakeProvisioner struct { - cert []byte - caCert []byte - err error + cert []byte + caCert []byte + getErr error + signErr error } -func (p *fakeProvisioner) Sign(ctx context.Context, cr *cmapi.CertificateRequest, log logr.Logger) ([]byte, []byte, error) { - return p.cert, p.caCert, p.err +func (p *fakeProvisioner) Sign(ctx context.Context, cr *cmapi.CertificateRequest, log logr.Logger) error { + metav1.SetMetaDataAnnotation(&cr.ObjectMeta, "aws-privateca-issuer/certificate-arn", "arn") + return p.signErr +} + +func (p *fakeProvisioner) Get(ctx context.Context, cr *cmapi.CertificateRequest, certArn string, log logr.Logger) ([]byte, []byte, error) { + return p.cert, p.caCert, p.getErr } type createMockProvisioner func() @@ -67,7 +74,8 @@ func TestCertificateRequestReconcile(t *testing.T) { type testCase struct { name types.NamespacedName objects []client.Object - expectedResult ctrl.Result + expectedSignResult ctrl.Result + expectedGetResult ctrl.Result expectedError bool expectedReadyConditionStatus cmmeta.ConditionStatus expectedReadyConditionReason string @@ -127,6 +135,8 @@ func TestCertificateRequestReconcile(t *testing.T) { }, }, }, + expectedSignResult: ctrl.Result{Requeue: true}, + expectedGetResult: ctrl.Result{}, expectedReadyConditionStatus: cmmeta.ConditionTrue, expectedReadyConditionReason: cmapi.CertificateRequestReasonIssued, expectedError: false, @@ -192,6 +202,125 @@ func TestCertificateRequestReconcile(t *testing.T) { awspca.StoreProvisioner(types.NamespacedName{Name: "clusterissuer1"}, &fakeProvisioner{caCert: []byte("cacert"), cert: []byte("cert")}) }, }, + "failure-certificate-not-issued": { + name: types.NamespacedName{Namespace: "ns1", Name: "cr1"}, + objects: []client.Object{ + cmgen.CertificateRequest( + "cr1", + cmgen.SetCertificateRequestNamespace("ns1"), + cmgen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ + Name: "issuer1", + Group: issuerapi.GroupVersion.Group, + Kind: "Issuer", + }), + cmgen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionUnknown, + }), + ), + &issuerapi.AWSPCAIssuer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "issuer1", + Namespace: "ns1", + }, + Spec: issuerapi.AWSPCAIssuerSpec{ + SecretRef: issuerapi.AWSCredentialsSecretReference{ + SecretReference: v1.SecretReference{ + Name: "issuer1-credentials", + Namespace: "ns1", + }, + }, + Region: "us-east-1", + Arn: "arn:aws:acm-pca:us-east-1:account:certificate-authority/12345678-1234-1234-1234-123456789012", + }, + Status: issuerapi.AWSPCAIssuerStatus{ + Conditions: []metav1.Condition{ + { + Type: issuerapi.ConditionTypeReady, + Status: metav1.ConditionTrue, + }, + }, + }, + }, + &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "issuer1-credentials", + Namespace: "ns1", + }, + Data: map[string][]byte{ + "AWS_ACCESS_KEY_ID": []byte("ZXhhbXBsZQ=="), + "AWS_SECRET_ACCESS_KEY": []byte("ZXhhbXBsZQ=="), + }, + }, + }, + expectedSignResult: ctrl.Result{Requeue: true}, + expectedGetResult: ctrl.Result{Requeue: true}, + expectedReadyConditionStatus: cmmeta.ConditionFalse, + expectedReadyConditionReason: cmapi.CertificateRequestReasonPending, + expectedError: false, + mockProvisioner: func() { + awspca.StoreProvisioner(types.NamespacedName{Namespace: "ns1", Name: "issuer1"}, &fakeProvisioner{getErr: &acmpcatypes.RequestInProgressException{}}) + }, + }, + "failure-get-failure": { + name: types.NamespacedName{Namespace: "ns1", Name: "cr1"}, + objects: []client.Object{ + cmgen.CertificateRequest( + "cr1", + cmgen.SetCertificateRequestNamespace("ns1"), + cmgen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ + Name: "issuer1", + Group: issuerapi.GroupVersion.Group, + Kind: "Issuer", + }), + cmgen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionUnknown, + }), + ), + &issuerapi.AWSPCAIssuer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "issuer1", + Namespace: "ns1", + }, + Spec: issuerapi.AWSPCAIssuerSpec{ + SecretRef: issuerapi.AWSCredentialsSecretReference{ + SecretReference: v1.SecretReference{ + Name: "issuer1-credentials", + Namespace: "ns1", + }, + }, + Region: "us-east-1", + Arn: "arn:aws:acm-pca:us-east-1:account:certificate-authority/12345678-1234-1234-1234-123456789012", + }, + Status: issuerapi.AWSPCAIssuerStatus{ + Conditions: []metav1.Condition{ + { + Type: issuerapi.ConditionTypeReady, + Status: metav1.ConditionTrue, + }, + }, + }, + }, + &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "issuer1-credentials", + Namespace: "ns1", + }, + Data: map[string][]byte{ + "AWS_ACCESS_KEY_ID": []byte("ZXhhbXBsZQ=="), + "AWS_SECRET_ACCESS_KEY": []byte("ZXhhbXBsZQ=="), + }, + }, + }, + expectedSignResult: ctrl.Result{Requeue: true}, + expectedReadyConditionStatus: cmmeta.ConditionFalse, + expectedReadyConditionReason: cmapi.CertificateRequestReasonFailed, + expectedError: false, + mockProvisioner: func() { + awspca.StoreProvisioner(types.NamespacedName{Namespace: "ns1", Name: "issuer1"}, &fakeProvisioner{getErr: errors.New("Get Failure")}) + }, + }, "failure-issuer-not-ready": { name: types.NamespacedName{Namespace: "ns1", Name: "cr1"}, objects: []client.Object{ @@ -367,7 +496,7 @@ func TestCertificateRequestReconcile(t *testing.T) { expectedReadyConditionReason: cmapi.CertificateRequestReasonFailed, expectedError: false, mockProvisioner: func() { - awspca.StoreProvisioner(types.NamespacedName{Namespace: "ns1", Name: "issuer1"}, &fakeProvisioner{err: errors.New("Sign Failure")}) + awspca.StoreProvisioner(types.NamespacedName{Namespace: "ns1", Name: "issuer1"}, &fakeProvisioner{signErr: errors.New("Sign Failure")}) }, }, } @@ -397,15 +526,18 @@ func TestCertificateRequestReconcile(t *testing.T) { tc.mockProvisioner() } - result, err := controller.Reconcile(ctx, reconcile.Request{NamespacedName: tc.name}) - if tc.expectedError && err == nil { + result, signErr := controller.Reconcile(ctx, reconcile.Request{NamespacedName: tc.name}) + assert.Equal(t, tc.expectedSignResult, result, "Unexpected sign result") + + result, getErr := controller.Reconcile(ctx, reconcile.Request{NamespacedName: tc.name}) + assert.Equal(t, tc.expectedGetResult, result, "Unexpected get result") + + if tc.expectedError && (signErr == nil && getErr == nil) { assert.Fail(t, "Expected an error but got none") } - assert.Equal(t, tc.expectedResult, result, "Unexpected result") - var cr cmapi.CertificateRequest - err = fakeClient.Get(ctx, tc.name, &cr) + err := fakeClient.Get(ctx, tc.name, &cr) require.NoError(t, client.IgnoreNotFound(err), "unexpected error from fake client") if err == nil { if tc.expectedReadyConditionStatus != "" { @@ -431,6 +563,7 @@ func assertCertificateRequestHasReadyCondition(t *testing.T, status cmmeta.Condi validReasons := sets.NewString( cmapi.CertificateRequestReasonFailed, cmapi.CertificateRequestReasonIssued, + cmapi.CertificateRequestReasonPending, ) assert.Contains(t, validReasons, reason, "unexpected condition reason") assert.Equal(t, reason, condition.Reason, "unexpected condition reason")