From 1030bbadb5bb790b8d7bbe5fde4351899431966e Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 10 May 2021 19:32:22 +0100 Subject: [PATCH] Change Venafi Signer to use ParseCertificateChain to populate Status.CA correctly Signed-off-by: joshvanl --- .../certificaterequests/venafi/BUILD.bazel | 1 + .../certificaterequests/venafi/venafi.go | 24 ++++---- .../certificaterequests/venafi/venafi_test.go | 59 ++++++++++++++----- 3 files changed, 54 insertions(+), 30 deletions(-) diff --git a/pkg/controller/certificaterequests/venafi/BUILD.bazel b/pkg/controller/certificaterequests/venafi/BUILD.bazel index db126a952..bfeaebe5f 100644 --- a/pkg/controller/certificaterequests/venafi/BUILD.bazel +++ b/pkg/controller/certificaterequests/venafi/BUILD.bazel @@ -16,6 +16,7 @@ go_library( "//pkg/issuer/venafi/client:go_default_library", "//pkg/issuer/venafi/client/api:go_default_library", "//pkg/logs:go_default_library", + "//pkg/util/pki:go_default_library", "@com_github_venafi_vcert_v4//pkg/endpoint: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/venafi/venafi.go b/pkg/controller/certificaterequests/venafi/venafi.go index f6b964bc5..f5c83b71c 100644 --- a/pkg/controller/certificaterequests/venafi/venafi.go +++ b/pkg/controller/certificaterequests/venafi/venafi.go @@ -19,7 +19,6 @@ package venafi import ( "context" "encoding/json" - "encoding/pem" "fmt" k8sErrors "k8s.io/apimachinery/pkg/api/errors" @@ -38,6 +37,7 @@ import ( venaficlient "github.com/jetstack/cert-manager/pkg/issuer/venafi/client" "github.com/jetstack/cert-manager/pkg/issuer/venafi/client/api" logf "github.com/jetstack/cert-manager/pkg/logs" + utilpki "github.com/jetstack/cert-manager/pkg/util/pki" ) const ( @@ -163,20 +163,16 @@ func (v *Venafi) Sign(ctx context.Context, cr *cmapi.CertificateRequest, issuerO log.V(logf.DebugLevel).Info("certificate issued") - // Assume the last certificate is the root CA - var ( - block, lastBlock *pem.Block - remainingBytes = certPem - ) - for { - block, remainingBytes = pem.Decode(remainingBytes) - if block == nil { - break - } - lastBlock = block + bundle, err := utilpki.ParseCertificateChainPEM(certPem) + if err != nil { + message := "Failed to parse returned certificate bundle" + v.reporter.Failed(cr, err, "ParseError", message) + log.Error(err, message) + return nil, err } + return &issuerpkg.IssueResponse{ - Certificate: certPem, - CA: pem.EncodeToMemory(lastBlock), + Certificate: bundle.ChainPEM, + CA: bundle.CAPEM, }, nil } diff --git a/pkg/controller/certificaterequests/venafi/venafi_test.go b/pkg/controller/certificaterequests/venafi/venafi_test.go index 019e285c1..6b00a21ad 100644 --- a/pkg/controller/certificaterequests/venafi/venafi_test.go +++ b/pkg/controller/certificaterequests/venafi/venafi_test.go @@ -24,6 +24,7 @@ import ( "crypto/x509/pkix" "encoding/pem" "errors" + "math/big" "testing" "time" @@ -63,12 +64,12 @@ func generateCSR(t *testing.T, secretKey crypto.Signer, alg x509.SignatureAlgori "foo.example.com", "bar.example.com", }, SignatureAlgorithm: alg, + PublicKey: secretKey.Public(), } csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &template, secretKey) if err != nil { - t.Error(err) - t.FailNow() + t.Fatal(err) } csr := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrBytes}) @@ -78,13 +79,41 @@ func generateCSR(t *testing.T, secretKey crypto.Signer, alg x509.SignatureAlgori func TestSign(t *testing.T) { metaFixedClockStart := metav1.NewTime(fixedClockStart) - rsaSK, err := pki.GenerateRSAPrivateKey(2048) + rootPK, err := pki.GenerateECPrivateKey(256) if err != nil { - t.Error(err) - t.FailNow() + t.Fatal(err) } - csrPEM := generateCSR(t, rsaSK, x509.SHA1WithRSA) + serialNumber, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) + if err != nil { + t.Fatal(err) + } + + rootTmpl := &x509.Certificate{ + Version: 3, + BasicConstraintsValid: true, + SerialNumber: serialNumber, + PublicKeyAlgorithm: x509.ECDSA, + PublicKey: rootPK.Public(), + IsCA: true, + Subject: pkix.Name{ + CommonName: "root-ca", + }, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Minute), + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + } + rootPEM, rootCert, err := pki.SignCertificate(rootTmpl, rootTmpl, rootPK.Public(), rootPK) + if err != nil { + t.Fatal(err) + } + + testPK, err := pki.GenerateECPrivateKey(256) + if err != nil { + t.Fatal(err) + } + + csrPEM := generateCSR(t, testPK, x509.ECDSAWithSHA256) tppSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -191,14 +220,12 @@ func TestSign(t *testing.T) { template, err := pki.GenerateTemplateFromCertificateRequest(baseCR) if err != nil { - t.Error(err) - t.FailNow() + t.Fatal(err) } - certPEM, _, err := pki.SignCertificate(template, template, rsaSK.Public(), rsaSK) + certPEM, _, err := pki.SignCertificate(template, rootCert, testPK.Public(), rootPK) if err != nil { - t.Error(err) - t.FailNow() + t.Fatal(err) } clientReturnsPending := &internalvenafifake.Venafi{ @@ -222,7 +249,7 @@ func TestSign(t *testing.T) { return "test", nil }, RetrieveCertificateFn: func(string, []byte, time.Duration, []api.CustomField) ([]byte, error) { - return certPEM, nil + return append(certPEM, rootPEM...), nil }, } @@ -234,7 +261,7 @@ func TestSign(t *testing.T) { return "", errors.New("Custom field not set") }, RetrieveCertificateFn: func(string, []byte, time.Duration, []api.CustomField) ([]byte, error) { - return certPEM, nil + return append(certPEM, rootPEM...), nil }, } @@ -602,7 +629,7 @@ func TestSign(t *testing.T) { LastTransitionTime: &metaFixedClockStart, }), gen.SetCertificateRequestCertificate(certPEM), - gen.SetCertificateRequestCA(certPEM), + gen.SetCertificateRequestCA(rootPEM), gen.AddCertificateRequestAnnotations(map[string]string{cmapi.VenafiPickupIDAnnotationKey: "test"}), ), )), @@ -649,7 +676,7 @@ func TestSign(t *testing.T) { LastTransitionTime: &metaFixedClockStart, }), gen.SetCertificateRequestCertificate(certPEM), - gen.SetCertificateRequestCA(certPEM), + gen.SetCertificateRequestCA(rootPEM), gen.AddCertificateRequestAnnotations(map[string]string{cmapi.VenafiPickupIDAnnotationKey: "test"}), ), )), @@ -695,7 +722,7 @@ func TestSign(t *testing.T) { LastTransitionTime: &metaFixedClockStart, }), gen.SetCertificateRequestCertificate(certPEM), - gen.SetCertificateRequestCA(certPEM), + gen.SetCertificateRequestCA(rootPEM), gen.AddCertificateRequestAnnotations(map[string]string{cmapi.VenafiPickupIDAnnotationKey: "test"}), ), )),