From 9622b664bfac104e778e16e6b604ea1d40013725 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 10 May 2021 20:29:13 +0100 Subject: [PATCH 1/4] Adds SecretTLSKeyPairAndCA to parse a certificate chain and CA from a target Secret Signed-off-by: joshvanl --- pkg/util/kube/BUILD.bazel | 1 + pkg/util/kube/pki.go | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/pkg/util/kube/BUILD.bazel b/pkg/util/kube/BUILD.bazel index 82d8aac83..b885eae50 100644 --- a/pkg/util/kube/BUILD.bazel +++ b/pkg/util/kube/BUILD.bazel @@ -6,6 +6,7 @@ go_library( importpath = "github.com/jetstack/cert-manager/pkg/util/kube", visibility = ["//visibility:public"], deps = [ + "//pkg/apis/meta/v1:go_default_library", "//pkg/util/errors:go_default_library", "//pkg/util/pki:go_default_library", "@io_k8s_api//core/v1:go_default_library", diff --git a/pkg/util/kube/pki.go b/pkg/util/kube/pki.go index 89e401a0c..f9ff3fe97 100644 --- a/pkg/util/kube/pki.go +++ b/pkg/util/kube/pki.go @@ -25,6 +25,7 @@ import ( corev1 "k8s.io/api/core/v1" corelisters "k8s.io/client-go/listers/core/v1" + cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" "github.com/jetstack/cert-manager/pkg/util/errors" "github.com/jetstack/cert-manager/pkg/util/pki" ) @@ -88,6 +89,32 @@ func SecretTLSCertChain(ctx context.Context, secretLister corelisters.SecretList return cert, nil } +// SecretTLSKeyPairAndCA returns the X.509 certificate chain and private key of +// the leaf certificate contained in the target Secret. If the ca.crt field exists +// on the Secret, it is parsed and added to the end of the certificate chain. +func SecretTLSKeyPairAndCA(ctx context.Context, secretLister corelisters.SecretLister, namespace, name string) ([]*x509.Certificate, crypto.Signer, error) { + certs, key, err := SecretTLSKeyPair(ctx, secretLister, namespace, name) + if err != nil { + return nil, nil, err + } + + secret, err := secretLister.Secrets(namespace).Get(name) + if err != nil { + return nil, nil, err + } + + caBytes, ok := secret.Data[cmmeta.TLSCAKey] + if !ok || len(caBytes) == 0 { + return certs, key, nil + } + ca, err := pki.DecodeX509CertificateBytes(caBytes) + if err != nil { + return nil, key, errors.NewInvalidData(err.Error()) + } + + return append(certs, ca), key, nil +} + func SecretTLSKeyPair(ctx context.Context, secretLister corelisters.SecretLister, namespace, name string) ([]*x509.Certificate, crypto.Signer, error) { secret, err := secretLister.Secrets(namespace).Get(name) if err != nil { From d327d402972555578bfbc8bb0f06733029e82d7e Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 10 May 2021 20:29:45 +0100 Subject: [PATCH 2/4] Updates SignCSRTemplate to use ParseCertificateChain Signed-off-by: joshvanl --- pkg/util/pki/csr.go | 26 ++---- pkg/util/pki/csr_test.go | 168 +++++++++++++++++---------------------- 2 files changed, 82 insertions(+), 112 deletions(-) diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index 6751c046b..6b1c38110 100644 --- a/pkg/util/pki/csr.go +++ b/pkg/util/pki/csr.go @@ -418,35 +418,25 @@ func SignCertificate(template *x509.Certificate, issuerCert *x509.Certificate, p // SignCSRTemplate signs a certificate template usually based upon a CSR. This // function expects all fields to be present in the certificate template, // including it's public key. -// It returns the certificate data followed by the CA data, encoded in PEM format. -func SignCSRTemplate(caCerts []*x509.Certificate, caKey crypto.Signer, template *x509.Certificate) ([]byte, []byte, error) { +// It returns the PEM bundle containing certificate data and the CA data, encoded in PEM format. +func SignCSRTemplate(caCerts []*x509.Certificate, caKey crypto.Signer, template *x509.Certificate) (PEMBundle, error) { if len(caCerts) == 0 { - return nil, nil, errors.New("no CA certificates given to sign CSR template") + return PEMBundle{}, errors.New("no CA certificates given to sign CSR template") } issuingCACert := caCerts[0] - certPem, _, err := SignCertificate(template, issuingCACert, template.PublicKey, caKey) + _, cert, err := SignCertificate(template, issuingCACert, template.PublicKey, caKey) if err != nil { - return nil, nil, err - + return PEMBundle{}, err } - chainPem, err := EncodeX509Chain(caCerts) + bundle, err := ParseCertificateChain(append(caCerts, cert)) if err != nil { - return nil, nil, err + return PEMBundle{}, err } - certPem = append(certPem, chainPem...) - - // encode the CA certificate to be bundled in the output - caCert := caCerts[len(caCerts)-1] - caPem, err := EncodeX509(caCert) - if err != nil { - return nil, nil, err - } - - return certPem, caPem, nil + return bundle, nil } // EncodeCSR calls x509.CreateCertificateRequest to sign the given CSR template. diff --git a/pkg/util/pki/csr_test.go b/pkg/util/pki/csr_test.go index 619985972..9a541d32c 100644 --- a/pkg/util/pki/csr_test.go +++ b/pkg/util/pki/csr_test.go @@ -25,6 +25,7 @@ import ( "math/big" "reflect" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -564,63 +565,46 @@ func TestSignCSRTemplate(t *testing.T) { // for that, we construct a chain of four certificates: // a root CA, two intermediate CA, and a leaf certificate. - // We start with a root CA: - rootPrivKey, err := GenerateRSAPrivateKey(MinRSAKeySize) - require.NoError(t, err) - rootTemplate := &x509.Certificate{ - SerialNumber: big.NewInt(0), - Subject: pkix.Name{ - CommonName: "testing-root", - }, - PublicKey: rootPrivKey.Public(), - IsCA: true, - } - rootPem, rootCert, err := SignCertificate(rootTemplate, rootTemplate, rootTemplate.PublicKey, rootPrivKey) - require.NoError(t, err) + mustCreatePair := func(issuerCert *x509.Certificate, issuerPK crypto.Signer, name string, isCA bool) ([]byte, *x509.Certificate, *x509.Certificate, crypto.Signer) { + pk, err := GenerateECPrivateKey(256) + require.NoError(t, err) + tmpl := &x509.Certificate{ + Version: 3, + BasicConstraintsValid: true, + SerialNumber: big.NewInt(0), + Subject: pkix.Name{ + CommonName: name, + }, + PublicKeyAlgorithm: x509.ECDSA, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Minute), + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, + PublicKey: pk.Public(), + IsCA: isCA, + } - // Then, we want an intermediate CA: - inter1PrivKey, err := GenerateRSAPrivateKey(MinRSAKeySize) - require.NoError(t, err) - inter1Template := &x509.Certificate{ - SerialNumber: big.NewInt(1), - Subject: pkix.Name{ - CommonName: "testing-intermediate-1", - }, - PublicKey: inter1PrivKey.Public(), - IsCA: true, - } - inter1Pem, inter1Cert, err := SignCertificate(inter1Template, rootCert, inter1Template.PublicKey, rootPrivKey) - require.NoError(t, err) + if isCA { + tmpl.KeyUsage |= x509.KeyUsageCertSign + } - // Then, we want another intermediate CA: - inter2PrivKey, err := GenerateRSAPrivateKey(MinRSAKeySize) - require.NoError(t, err) - inter2Template := &x509.Certificate{ - SerialNumber: big.NewInt(2), - Subject: pkix.Name{ - CommonName: "testing-intermediate-2", - }, - PublicKey: inter2PrivKey.Public(), - IsCA: true, - } - inter2Pem, inter2Cert, err := SignCertificate(inter2Template, inter1Cert, inter2Template.PublicKey, inter1PrivKey) - require.NoError(t, err) + if issuerCert == nil { + issuerCert = tmpl + } + if issuerPK == nil { + issuerPK = pk + } - // And finally, we also want a leaf certificate: - leafPrivKey, err := GenerateRSAPrivateKey(MinRSAKeySize) - require.NoError(t, err) - leafTemplate := &x509.Certificate{ - SerialNumber: big.NewInt(3), - Subject: pkix.Name{ - CommonName: "testing-leaf", - }, - PublicKey: leafPrivKey.Public(), + pem, cert, err := SignCertificate(tmpl, issuerCert, tmpl.PublicKey, issuerPK) + require.NoError(t, err) + return pem, cert, tmpl, pk } - leafPem, _, err := SignCertificate(leafTemplate, inter2Cert, leafTemplate.PublicKey, inter2PrivKey) - require.NoError(t, err) - tests := []struct { - name string + rootPEM, rootCert, rootTmpl, rootPK := mustCreatePair(nil, nil, "root", true) + int1PEM, int1Cert, int1Tmpl, int1PK := mustCreatePair(rootCert, rootPK, "int1", true) + int2PEM, int2Cert, int2Tmpl, int2PK := mustCreatePair(int1Cert, int1PK, "int2", true) + leafPEM, _, leafTmpl, _ := mustCreatePair(int2Cert, int2PK, "leaf", false) + + tests := map[string]struct { caCerts []*x509.Certificate caKey crypto.Signer template *x509.Certificate @@ -628,68 +612,64 @@ func TestSignCSRTemplate(t *testing.T) { expectedCaCertPem []byte wantErr bool }{ - { - name: "Sign intermediate 1 template", + "Sign intermediate 1 template": { caCerts: []*x509.Certificate{rootCert}, - caKey: rootPrivKey, - template: inter1Template, - expectedCertPem: inter1Pem, - expectedCaCertPem: rootPem, + caKey: rootPK, + template: int1Tmpl, + expectedCertPem: int1PEM, + expectedCaCertPem: rootPEM, wantErr: false, }, - { - name: "Sign intermediate 2 template", - caCerts: []*x509.Certificate{inter1Cert, rootCert}, - caKey: inter1PrivKey, - template: inter2Template, - expectedCertPem: append(inter2Pem, inter1Pem...), - expectedCaCertPem: rootPem, + "Sign intermediate 2 template": { + caCerts: []*x509.Certificate{int1Cert, rootCert}, + caKey: int1PK, + template: int2Tmpl, + expectedCertPem: append(int2PEM, int1PEM...), + expectedCaCertPem: rootPEM, wantErr: false, }, - { - name: "Sign leaf template", - caCerts: []*x509.Certificate{inter2Cert, inter1Cert, rootCert}, - caKey: inter2PrivKey, - template: leafTemplate, - expectedCertPem: append(append(leafPem, inter1Pem...), inter2Pem...), - expectedCaCertPem: rootPem, + "Sign leaf template": { + caCerts: []*x509.Certificate{int2Cert, int1Cert, rootCert}, + caKey: int2PK, + template: leafTmpl, + expectedCertPem: append(append(leafPEM, int1PEM...), int2PEM...), + expectedCaCertPem: rootPEM, wantErr: false, }, - { - name: "Sign leaf template no root", - caCerts: []*x509.Certificate{inter2Cert, inter1Cert}, - caKey: inter2PrivKey, - template: leafTemplate, - expectedCertPem: append(leafPem, inter2Pem...), - expectedCaCertPem: inter1Pem, + "Sign leaf template no root": { + caCerts: []*x509.Certificate{int2Cert, int1Cert}, + caKey: int2PK, + template: leafTmpl, + expectedCertPem: append(leafPEM, int2PEM...), + expectedCaCertPem: int1PEM, wantErr: false, }, - { - name: "Error on no CA", - caKey: rootPrivKey, - template: rootTemplate, + "Error on no CA": { + caKey: rootPK, + template: rootTmpl, wantErr: true, }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - actualCertPem, actualCaCertPem, err := SignCSRTemplate(tt.caCerts, tt.caKey, tt.template) - if (err != nil) != tt.wantErr { - t.Errorf("TestSignCSRTemplate() error = %v, wantErr %v", err, tt.wantErr) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + actualBundle, err := SignCSRTemplate(test.caCerts, test.caKey, test.template) + if (err != nil) != test.wantErr { + t.Errorf("TestSignCSRTemplate() error = %v, wantErr %v", err, test.wantErr) return } - if !bytes.Equal(tt.expectedCertPem, actualCertPem) { + + if !bytes.Equal(test.expectedCertPem, actualBundle.ChainPEM) { // To help us identify where the mismatch is, we decode turn the // into strings and do a textual diff. - expected, _ := DecodeX509CertificateBytes(tt.expectedCertPem) - actual, _ := DecodeX509CertificateBytes(actualCertPem) + expected, _ := DecodeX509CertificateBytes(test.expectedCertPem) + actual, _ := DecodeX509CertificateBytes(actualBundle.ChainPEM) assert.Equal(t, expected.Subject.String(), actual.Subject.String()) } - if !bytes.Equal(tt.expectedCaCertPem, actualCaCertPem) { + if !bytes.Equal(test.expectedCaCertPem, actualBundle.CAPEM) { // To help us identify where the mismatch is, we decode turn the // into strings and do a textual diff. - expected, _ := DecodeX509CertificateBytes(tt.expectedCaCertPem) - actual, _ := DecodeX509CertificateBytes(actualCaCertPem) + expected, _ := DecodeX509CertificateBytes(test.expectedCaCertPem) + actual, _ := DecodeX509CertificateBytes(actualBundle.CAPEM) assert.Equal(t, expected.Subject.String(), actual.Subject.String()) } }) From ea2cfdc3c99adccc9a2de7c115b659e3cc991fae Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 10 May 2021 20:30:31 +0100 Subject: [PATCH 3/4] Updates CA issuer to updates SignCSRTemplate and propagate CA certificate down Signed-off-by: joshvanl --- .../certificaterequests/acme/acme_test.go | 36 ++++- pkg/controller/certificaterequests/ca/ca.go | 8 +- .../certificaterequests/ca/ca_test.go | 132 ++++++++---------- 3 files changed, 95 insertions(+), 81 deletions(-) diff --git a/pkg/controller/certificaterequests/acme/acme_test.go b/pkg/controller/certificaterequests/acme/acme_test.go index 9a7249b1e..9727d2172 100644 --- a/pkg/controller/certificaterequests/acme/acme_test.go +++ b/pkg/controller/certificaterequests/acme/acme_test.go @@ -24,6 +24,7 @@ import ( "crypto/x509/pkix" "encoding/pem" "errors" + "math/big" "net" "reflect" "testing" @@ -111,6 +112,31 @@ func TestSign(t *testing.T) { }), ) + rootPK, err := pki.GenerateECPrivateKey(256) + if err != nil { + t.Fatal() + } + + rootTmpl := &x509.Certificate{ + Version: 3, + BasicConstraintsValid: true, + SerialNumber: big.NewInt(0), + Subject: pkix.Name{ + CommonName: "root", + }, + PublicKeyAlgorithm: x509.RSA, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Minute), + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + PublicKey: rootPK.Public(), + IsCA: true, + } + + _, rootCert, err := pki.SignCertificate(rootTmpl, rootTmpl, rootPK.Public(), rootPK) + if err != nil { + t.Fatal(err) + } + sk, err := pki.GenerateRSAPrivateKey(2048) if err != nil { t.Fatal(err) @@ -158,7 +184,7 @@ func TestSign(t *testing.T) { t.Errorf("error generating template: %v", err) } - certPEM, _, err := pki.SignCSRTemplate([]*x509.Certificate{template}, sk, template) + certBundle, err := pki.SignCSRTemplate([]*x509.Certificate{rootCert}, rootPK, template) if err != nil { t.Fatal(err) } @@ -173,7 +199,7 @@ func TestSign(t *testing.T) { if err != nil { t.Fatal(err) } - certPEM2, _, err := pki.SignCSRTemplate([]*x509.Certificate{template}, sk, template2) + cert2Bundle, err := pki.SignCSRTemplate([]*x509.Certificate{rootCert}, rootPK, template2) if err != nil { t.Fatal(err) } @@ -561,7 +587,7 @@ func TestSign(t *testing.T) { builder: &testpkg.Builder{ CertManagerObjects: []runtime.Object{gen.OrderFrom(baseOrder, gen.SetOrderState(cmacme.Valid), - gen.SetOrderCertificate(certPEM2), + gen.SetOrderCertificate(cert2Bundle.ChainPEM), ), baseCR.DeepCopy(), baseIssuer.DeepCopy()}, ExpectedActions: []testpkg.Action{ testpkg.NewAction(coretesting.NewDeleteAction( @@ -581,7 +607,7 @@ func TestSign(t *testing.T) { }, CertManagerObjects: []runtime.Object{gen.OrderFrom(baseOrder, gen.SetOrderState(cmacme.Valid), - gen.SetOrderCertificate(certPEM), + gen.SetOrderCertificate(certBundle.ChainPEM), ), baseCR.DeepCopy(), baseIssuer.DeepCopy()}, ExpectedActions: []testpkg.Action{ testpkg.NewAction(coretesting.NewUpdateSubresourceAction( @@ -596,7 +622,7 @@ func TestSign(t *testing.T) { Message: "Certificate fetched from issuer successfully", LastTransitionTime: &metaFixedClockStart, }), - gen.SetCertificateRequestCertificate(certPEM), + gen.SetCertificateRequestCertificate(certBundle.ChainPEM), ), )), }, diff --git a/pkg/controller/certificaterequests/ca/ca.go b/pkg/controller/certificaterequests/ca/ca.go index 88887878c..79bffb131 100644 --- a/pkg/controller/certificaterequests/ca/ca.go +++ b/pkg/controller/certificaterequests/ca/ca.go @@ -80,7 +80,7 @@ func (c *CA) Sign(ctx context.Context, cr *cmapi.CertificateRequest, issuerObj c resourceNamespace := c.issuerOptions.ResourceNamespace(issuerObj) // get a copy of the CA certificate named on the Issuer - caCerts, caKey, err := kube.SecretTLSKeyPair(ctx, c.secretsLister, resourceNamespace, issuerObj.GetSpec().CA.SecretName) + caCerts, caKey, err := kube.SecretTLSKeyPairAndCA(ctx, c.secretsLister, resourceNamespace, issuerObj.GetSpec().CA.SecretName) if k8sErrors.IsNotFound(err) { message := fmt.Sprintf("Referenced secret %s/%s not found", resourceNamespace, secretName) @@ -117,7 +117,7 @@ func (c *CA) Sign(ctx context.Context, cr *cmapi.CertificateRequest, issuerObj c template.CRLDistributionPoints = issuerObj.GetSpec().CA.CRLDistributionPoints template.OCSPServer = issuerObj.GetSpec().CA.OCSPServers - certPEM, caPEM, err := pki.SignCSRTemplate(caCerts, caKey, template) + bundle, err := pki.SignCSRTemplate(caCerts, caKey, template) if err != nil { message := "Error signing certificate" c.reporter.Failed(cr, err, "SigningError", message) @@ -128,7 +128,7 @@ func (c *CA) Sign(ctx context.Context, cr *cmapi.CertificateRequest, issuerObj c log.V(logf.DebugLevel).Info("certificate issued") return &issuerpkg.IssueResponse{ - Certificate: certPEM, - CA: caPEM, + Certificate: bundle.ChainPEM, + CA: bundle.CAPEM, }, nil } diff --git a/pkg/controller/certificaterequests/ca/ca_test.go b/pkg/controller/certificaterequests/ca/ca_test.go index 263e1034c..c5b0c96ea 100644 --- a/pkg/controller/certificaterequests/ca/ca_test.go +++ b/pkg/controller/certificaterequests/ca/ca_test.go @@ -17,7 +17,6 @@ limitations under the License. package ca import ( - "bytes" "context" "crypto" "crypto/rand" @@ -48,6 +47,7 @@ import ( "github.com/jetstack/cert-manager/pkg/controller" "github.com/jetstack/cert-manager/pkg/controller/certificaterequests" "github.com/jetstack/cert-manager/pkg/controller/certificaterequests/util" + controllertest "github.com/jetstack/cert-manager/pkg/controller/test" testpkg "github.com/jetstack/cert-manager/pkg/controller/test" "github.com/jetstack/cert-manager/pkg/util/pki" "github.com/jetstack/cert-manager/test/unit/gen" @@ -70,8 +70,7 @@ func generateCSR(t *testing.T, secretKey crypto.Signer) []byte { 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}) @@ -79,27 +78,28 @@ func generateCSR(t *testing.T, secretKey crypto.Signer) []byte { return csr } -func generateSelfSignedCertFromCR(t *testing.T, cr *cmapi.CertificateRequest, key crypto.Signer, - duration time.Duration) (*x509.Certificate, []byte) { - template, err := pki.GenerateTemplateFromCertificateRequest(cr) - if err != nil { - t.Errorf("error generating template: %v", err) +func generateSelfSignedCACert(t *testing.T, key crypto.Signer, name string) (*x509.Certificate, []byte) { + tmpl := &x509.Certificate{ + Version: 3, + BasicConstraintsValid: true, + SerialNumber: big.NewInt(0), + Subject: pkix.Name{ + CommonName: name, + }, + PublicKeyAlgorithm: x509.RSA, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Minute), + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + PublicKey: key.Public(), + IsCA: true, } - derBytes, err := x509.CreateCertificate(rand.Reader, template, template, key.Public(), key) + pem, cert, err := pki.SignCertificate(tmpl, tmpl, key.Public(), key) if err != nil { - t.Errorf("error signing cert: %v", err) - t.FailNow() + t.Fatal(err) } - pemByteBuffer := bytes.NewBuffer([]byte{}) - err = pem.Encode(pemByteBuffer, &pem.Block{Type: "CERTIFICATE", Bytes: derBytes}) - if err != nil { - t.Errorf("failed to encode cert: %v", err) - t.FailNow() - } - - return template, pemByteBuffer.Bytes() + return cert, pem } func TestSign(t *testing.T) { @@ -114,18 +114,21 @@ func TestSign(t *testing.T) { ) // Build root RSA CA - skRSA, err := pki.GenerateRSAPrivateKey(2048) + rootPK, err := pki.GenerateRSAPrivateKey(2048) if err != nil { - t.Error(err) - t.FailNow() + t.Fatal() } + rootPKPEM := pki.EncodePKCS1PrivateKey(rootPK) - skRSAPEM := pki.EncodePKCS1PrivateKey(skRSA) - rsaCSR := generateCSR(t, skRSA) + testpk, err := pki.GenerateRSAPrivateKey(2048) + if err != nil { + t.Fatal() + } + testCSR := generateCSR(t, testpk) baseCRNotApproved := gen.CertificateRequest("test-cr", gen.SetCertificateRequestIsCA(true), - gen.SetCertificateRequestCSR(rsaCSR), + gen.SetCertificateRequestCSR(testCSR), gen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ Name: baseIssuer.DeepCopy().Name, Group: certmanager.GroupName, @@ -153,15 +156,15 @@ func TestSign(t *testing.T) { ) // generate a self signed root ca valid for 60d - _, rsaPEMCert := generateSelfSignedCertFromCR(t, baseCR, skRSA, time.Hour*24*60) + rootCert, rootCertPEM := generateSelfSignedCACert(t, rootPK, "root") rsaCASecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "root-ca-secret", Namespace: gen.DefaultTestNamespace, }, Data: map[string][]byte{ - corev1.TLSPrivateKeyKey: skRSAPEM, - corev1.TLSCertKey: rsaPEMCert, + corev1.TLSPrivateKeyKey: rootPKPEM, + corev1.TLSCertKey: rootCertPEM, }, } @@ -170,13 +173,11 @@ func TestSign(t *testing.T) { template, err := pki.GenerateTemplateFromCertificateRequest(baseCR) if err != nil { - t.Error(err) - t.FailNow() + t.Fatal(err) } - certPEM, _, err := pki.SignCSRTemplate([]*x509.Certificate{template}, skRSA, template) + certBundle, err := pki.SignCSRTemplate([]*x509.Certificate{rootCert}, rootPK, template) if err != nil { - t.Error(err) - t.FailNow() + t.Fatal(err) } tests := map[string]testT{ @@ -392,8 +393,8 @@ func TestSign(t *testing.T) { Message: "Certificate fetched from issuer successfully", LastTransitionTime: &metaFixedClockStart, }), - gen.SetCertificateRequestCA(rsaPEMCert), - gen.SetCertificateRequestCertificate(certPEM), + gen.SetCertificateRequestCertificate(certBundle.ChainPEM), + gen.SetCertificateRequestCA(rootCertPEM), ), )), }, @@ -436,13 +437,10 @@ func runTest(t *testing.T, test testT) { } controller := certificaterequests.New(apiutil.IssuerCA, ca) - _, _, err := controller.Register(test.builder.Context) - if err != nil { - t.Errorf("controller.Register failed: %v", err) - } + controller.Register(test.builder.Context) test.builder.Start() - err = controller.Sync(context.Background(), test.certificateRequest) + err := controller.Sync(context.Background(), test.certificateRequest) if err != nil && !test.expectedErr { t.Errorf("expected to not get an error, but got: %v", err) } @@ -454,9 +452,19 @@ func runTest(t *testing.T, test testT) { } func TestCA_Sign(t *testing.T) { - rsaPair, err := pki.GenerateRSAPrivateKey(2048) - require.NoError(t, err) - rsaCSR := generateCSR(t, rsaPair) + // Build root RSA CA + rootPK, err := pki.GenerateRSAPrivateKey(2048) + if err != nil { + t.Fatal() + } + rootCert, _ := generateSelfSignedCACert(t, rootPK, "root") + + // Build test CSR + testpk, err := pki.GenerateRSAPrivateKey(2048) + if err != nil { + t.Fatal() + } + testCSR := generateCSR(t, testpk) tests := map[string]struct { givenCASecret *corev1.Secret @@ -466,11 +474,12 @@ func TestCA_Sign(t *testing.T) { wantErr string }{ "when the CertificateRequest has the duration 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", })), givenCR: gen.CertificateRequest("cr-1", - gen.SetCertificateRequestCSR(rsaCSR), + gen.SetCertificateRequestCSR(testCSR), gen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ Name: "issuer-1", Group: certmanager.GroupName, @@ -480,12 +489,6 @@ func TestCA_Sign(t *testing.T) { Duration: 30 * time.Minute, }), ), - givenCASecret: gen.SecretFrom(gen.Secret("secret-1"), gen.SetSecretNamespace("default"), gen.SetSecretData(secretDataFor(t, rsaPair, - &x509.Certificate{ - SerialNumber: big.NewInt(1234), - IsCA: true, - }, - ))), 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" @@ -512,11 +515,12 @@ func TestCA_Sign(t *testing.T) { }, }, "when the CertificateRequest 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{ SecretName: "secret-1", })), givenCR: gen.CertificateRequest("cr-1", - gen.SetCertificateRequestCSR(rsaCSR), + gen.SetCertificateRequestCSR(testCSR), gen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ Name: "issuer-1", Group: certmanager.GroupName, @@ -524,59 +528,43 @@ func TestCA_Sign(t *testing.T) { }), gen.SetCertificateRequestIsCA(true), ), - givenCASecret: gen.SecretFrom(gen.Secret("secret-1"), gen.SetSecretNamespace("default"), gen.SetSecretData(secretDataFor(t, rsaPair, - &x509.Certificate{ - SerialNumber: big.NewInt(1234), - IsCA: true, - }, - ))), assertSignedCert: func(t *testing.T, got *x509.Certificate) { assert.Equal(t, true, got.IsCA) }, }, "when the Issuer has ocspServers 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{ SecretName: "secret-1", OCSPServers: []string{"http://ocsp-v3.example.org"}, })), givenCR: gen.CertificateRequest("cr-1", - gen.SetCertificateRequestCSR(rsaCSR), + gen.SetCertificateRequestCSR(testCSR), gen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ Name: "issuer-1", Group: certmanager.GroupName, Kind: "Issuer", }), ), - givenCASecret: gen.SecretFrom(gen.Secret("secret-1"), gen.SetSecretNamespace("default"), gen.SetSecretData(secretDataFor(t, rsaPair, - &x509.Certificate{ - SerialNumber: big.NewInt(1234), - IsCA: true, - }, - ))), assertSignedCert: func(t *testing.T, got *x509.Certificate) { assert.Equal(t, []string{"http://ocsp-v3.example.org"}, got.OCSPServer) }, }, "when the Issuer has crlDistributionPoints 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{ SecretName: "secret-1", CRLDistributionPoints: []string{"http://www.example.com/crl/test.crl"}, })), givenCR: gen.CertificateRequest("cr-1", gen.SetCertificateRequestIsCA(true), - gen.SetCertificateRequestCSR(rsaCSR), + gen.SetCertificateRequestCSR(testCSR), gen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ Name: "issuer-1", Group: certmanager.GroupName, Kind: "Issuer", }), ), - givenCASecret: gen.SecretFrom(gen.Secret("secret-1"), gen.SetSecretNamespace("default"), gen.SetSecretData(secretDataFor(t, rsaPair, - &x509.Certificate{ - SerialNumber: big.NewInt(1234), - IsCA: true, - }, - ))), assertSignedCert: func(t *testing.T, gotCA *x509.Certificate) { assert.Equal(t, []string{"http://www.example.com/crl/test.crl"}, gotCA.CRLDistributionPoints) }, @@ -584,7 +572,7 @@ func TestCA_Sign(t *testing.T) { } for name, test := range tests { t.Run(name, func(t *testing.T) { - rec := &testpkg.FakeRecorder{} + rec := &controllertest.FakeRecorder{} c := &CA{ issuerOptions: controller.IssuerOptions{ From 58a25314f799975cfa10d2d9504b45ecbd7928d7 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Wed, 12 May 2021 15:07:25 +0100 Subject: [PATCH 4/4] Changes CR CA controller to use ECDSA keys Signed-off-by: joshvanl --- .../certificaterequests/acme/acme_test.go | 11 ++-- pkg/controller/certificaterequests/ca/ca.go | 6 +- .../certificaterequests/ca/ca_test.go | 59 +++++++++++-------- pkg/util/pki/csr.go | 2 +- 4 files changed, 44 insertions(+), 34 deletions(-) diff --git a/pkg/controller/certificaterequests/acme/acme_test.go b/pkg/controller/certificaterequests/acme/acme_test.go index 9727d2172..ccac912c5 100644 --- a/pkg/controller/certificaterequests/acme/acme_test.go +++ b/pkg/controller/certificaterequests/acme/acme_test.go @@ -124,12 +124,11 @@ func TestSign(t *testing.T) { Subject: pkix.Name{ CommonName: "root", }, - PublicKeyAlgorithm: x509.RSA, - NotBefore: time.Now(), - NotAfter: time.Now().Add(time.Minute), - KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, - PublicKey: rootPK.Public(), - IsCA: true, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Minute), + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + PublicKey: rootPK.Public(), + IsCA: true, } _, rootCert, err := pki.SignCertificate(rootTmpl, rootTmpl, rootPK.Public(), rootPK) diff --git a/pkg/controller/certificaterequests/ca/ca.go b/pkg/controller/certificaterequests/ca/ca.go index 79bffb131..ad39cb676 100644 --- a/pkg/controller/certificaterequests/ca/ca.go +++ b/pkg/controller/certificaterequests/ca/ca.go @@ -18,6 +18,7 @@ package ca import ( "context" + "crypto" "crypto/x509" "fmt" @@ -41,6 +42,7 @@ const ( ) type templateGenerator func(*cmapi.CertificateRequest) (*x509.Certificate, error) +type signingFn func([]*x509.Certificate, crypto.Signer, *x509.Certificate) (pki.PEMBundle, error) type CA struct { issuerOptions controllerpkg.IssuerOptions @@ -50,6 +52,7 @@ type CA struct { // Used for testing to get reproducible resulting certificates templateGenerator templateGenerator + signingFn signingFn } func init() { @@ -67,6 +70,7 @@ func NewCA(ctx *controllerpkg.Context) *CA { secretsLister: ctx.KubeSharedInformerFactory.Core().V1().Secrets().Lister(), reporter: crutil.NewReporter(ctx.Clock, ctx.Recorder), templateGenerator: pki.GenerateTemplateFromCertificateRequest, + signingFn: pki.SignCSRTemplate, } } @@ -117,7 +121,7 @@ func (c *CA) Sign(ctx context.Context, cr *cmapi.CertificateRequest, issuerObj c template.CRLDistributionPoints = issuerObj.GetSpec().CA.CRLDistributionPoints template.OCSPServer = issuerObj.GetSpec().CA.OCSPServers - bundle, err := pki.SignCSRTemplate(caCerts, caKey, template) + bundle, err := c.signingFn(caCerts, caKey, template) if err != nil { message := "Error signing certificate" c.reporter.Failed(cr, err, "SigningError", message) diff --git a/pkg/controller/certificaterequests/ca/ca_test.go b/pkg/controller/certificaterequests/ca/ca_test.go index c5b0c96ea..98f1269d6 100644 --- a/pkg/controller/certificaterequests/ca/ca_test.go +++ b/pkg/controller/certificaterequests/ca/ca_test.go @@ -19,8 +19,8 @@ package ca import ( "context" "crypto" + "crypto/ecdsa" "crypto/rand" - "crypto/rsa" "crypto/x509" "crypto/x509/pkix" "encoding/asn1" @@ -47,7 +47,6 @@ import ( "github.com/jetstack/cert-manager/pkg/controller" "github.com/jetstack/cert-manager/pkg/controller/certificaterequests" "github.com/jetstack/cert-manager/pkg/controller/certificaterequests/util" - controllertest "github.com/jetstack/cert-manager/pkg/controller/test" testpkg "github.com/jetstack/cert-manager/pkg/controller/test" "github.com/jetstack/cert-manager/pkg/util/pki" "github.com/jetstack/cert-manager/test/unit/gen" @@ -59,13 +58,13 @@ var ( fixedClock = fakeclock.NewFakeClock(fixedClockStart) ) -func generateCSR(t *testing.T, secretKey crypto.Signer) []byte { +func generateCSR(t *testing.T, secretKey crypto.Signer, sigAlg x509.SignatureAlgorithm) []byte { asn1Subj, _ := asn1.Marshal(pkix.Name{ CommonName: "test", }.ToRDNSequence()) template := x509.CertificateRequest{ RawSubject: asn1Subj, - SignatureAlgorithm: x509.SHA256WithRSA, + SignatureAlgorithm: sigAlg, } csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &template, secretKey) @@ -86,12 +85,11 @@ func generateSelfSignedCACert(t *testing.T, key crypto.Signer, name string) (*x5 Subject: pkix.Name{ CommonName: name, }, - PublicKeyAlgorithm: x509.RSA, - NotBefore: time.Now(), - NotAfter: time.Now().Add(time.Minute), - KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, - PublicKey: key.Public(), - IsCA: true, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Minute), + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + PublicKey: key.Public(), + IsCA: true, } pem, cert, err := pki.SignCertificate(tmpl, tmpl, key.Public(), key) @@ -113,18 +111,20 @@ func TestSign(t *testing.T) { }), ) - // Build root RSA CA - rootPK, err := pki.GenerateRSAPrivateKey(2048) + rootPK, err := pki.GenerateECPrivateKey(256) if err != nil { - t.Fatal() + t.Fatal(err) + } + rootPKPEM, err := pki.EncodeECPrivateKey(rootPK) + if err != nil { + t.Fatal(err) } - rootPKPEM := pki.EncodePKCS1PrivateKey(rootPK) - testpk, err := pki.GenerateRSAPrivateKey(2048) + testpk, err := pki.GenerateECPrivateKey(256) if err != nil { - t.Fatal() + t.Fatal(err) } - testCSR := generateCSR(t, testpk) + testCSR := generateCSR(t, testpk, x509.ECDSAWithSHA256) baseCRNotApproved := gen.CertificateRequest("test-cr", gen.SetCertificateRequestIsCA(true), @@ -374,6 +374,9 @@ func TestSign(t *testing.T) { return template, nil }, + signingFn: func(_ []*x509.Certificate, _ crypto.Signer, _ *x509.Certificate) (pki.PEMBundle, error) { + return pki.PEMBundle{CAPEM: certBundle.CAPEM, ChainPEM: certBundle.ChainPEM}, nil + }, builder: &testpkg.Builder{ KubeObjects: []runtime.Object{rsaCASecret}, CertManagerObjects: []runtime.Object{baseCR.DeepCopy(), baseIssuer.DeepCopy()}, @@ -415,6 +418,7 @@ type testT struct { builder *testpkg.Builder certificateRequest *cmapi.CertificateRequest templateGenerator templateGenerator + signingFn signingFn expectedErr bool @@ -435,6 +439,9 @@ func runTest(t *testing.T, test testT) { if test.templateGenerator != nil { ca.templateGenerator = test.templateGenerator } + if test.signingFn != nil { + ca.signingFn = test.signingFn + } controller := certificaterequests.New(apiutil.IssuerCA, ca) controller.Register(test.builder.Context) @@ -452,19 +459,18 @@ func runTest(t *testing.T, test testT) { } func TestCA_Sign(t *testing.T) { - // Build root RSA CA - rootPK, err := pki.GenerateRSAPrivateKey(2048) + rootPK, err := pki.GenerateECPrivateKey(256) if err != nil { - t.Fatal() + t.Fatal(err) } rootCert, _ := generateSelfSignedCACert(t, rootPK, "root") // Build test CSR - testpk, err := pki.GenerateRSAPrivateKey(2048) + testpk, err := pki.GenerateECPrivateKey(256) if err != nil { - t.Fatal() + t.Fatal(err) } - testCSR := generateCSR(t, testpk) + testCSR := generateCSR(t, testpk, x509.ECDSAWithSHA256) tests := map[string]struct { givenCASecret *corev1.Secret @@ -572,7 +578,7 @@ func TestCA_Sign(t *testing.T) { } for name, test := range tests { t.Run(name, func(t *testing.T) { - rec := &controllertest.FakeRecorder{} + rec := &testpkg.FakeRecorder{} c := &CA{ issuerOptions: controller.IssuerOptions{ @@ -585,6 +591,7 @@ func TestCA_Sign(t *testing.T) { testlisters.SetFakeSecretNamespaceListerGet(test.givenCASecret, nil), ), templateGenerator: pki.GenerateTemplateFromCertificateRequest, + signingFn: pki.SignCSRTemplate, } gotIssueResp, gotErr := c.Sign(context.Background(), test.givenCR, test.givenCAIssuer) @@ -605,12 +612,12 @@ func TestCA_Sign(t *testing.T) { // Returns a map that is meant to be used for creating a certificate Secret // that contains the fields "tls.crt" and "tls.key". -func secretDataFor(t *testing.T, caKey *rsa.PrivateKey, caCrt *x509.Certificate) (secretData map[string][]byte) { +func secretDataFor(t *testing.T, caKey *ecdsa.PrivateKey, caCrt *x509.Certificate) (secretData map[string][]byte) { rootCADER, err := x509.CreateCertificate(rand.Reader, caCrt, caCrt, caKey.Public(), caKey) require.NoError(t, err) caCrt, err = x509.ParseCertificate(rootCADER) require.NoError(t, err) - caKeyPEM, err := pki.EncodePKCS8PrivateKey(caKey) + caKeyPEM, err := pki.EncodeECPrivateKey(caKey) require.NoError(t, err) caCrtPEM, err := pki.EncodeX509(caCrt) require.NoError(t, err) diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index 6b1c38110..e734d2964 100644 --- a/pkg/util/pki/csr.go +++ b/pkg/util/pki/csr.go @@ -431,7 +431,7 @@ func SignCSRTemplate(caCerts []*x509.Certificate, caKey crypto.Signer, template return PEMBundle{}, err } - bundle, err := ParseCertificateChain(append(caCerts, cert)) + bundle, err := ParseSingleCertificateChain(append(caCerts, cert)) if err != nil { return PEMBundle{}, err }