From 17ec9ea8e7ef831b4d84d8417e8a0460a5916325 Mon Sep 17 00:00:00 2001 From: Ashley Davis Date: Wed, 21 Jul 2021 16:33:07 +0100 Subject: [PATCH] fix check for self-signed certs in EncodeX509Chain see also https://github.com/jetstack/cert-manager/issues/4142 EncodeX509Chain checked for self-signed certs by comparing the subject and issuer of the cert in question, which is invalid since it's perfectly fine for those to match. the correct behavior is to use cert.CheckSignatureFrom(cert). this bug was exposed in 1.4 when ParseSingleCertificateChain started using EncodeX509Chain in the critical path of several issuers; when end-users had leaf certificates with subjects matching their issuer's subject, the bug was triggered. includes newly written tests for EncodeX509Chain and a test for ParseSingleCertificateChain Signed-off-by: Ashley Davis --- pkg/util/pki/csr.go | 16 ++++++-- pkg/util/pki/csr_test.go | 77 ++++++++++++++++++++++++++++++++++++++ pkg/util/pki/parse_test.go | 24 ++++++++---- 3 files changed, 106 insertions(+), 11 deletions(-) diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index 97b7d745b..6650c7d9c 100644 --- a/pkg/util/pki/csr.go +++ b/pkg/util/pki/csr.go @@ -450,7 +450,7 @@ func EncodeCSR(template *x509.CertificateRequest, key crypto.Signer) ([]byte, er return derBytes, nil } -// EncodeX509 will encode a *x509.Certificate into PEM format. +// EncodeX509 will encode a single *x509.Certificate into PEM format. func EncodeX509(cert *x509.Certificate) ([]byte, error) { caPem := bytes.NewBuffer([]byte{}) err := pem.Encode(caPem, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw}) @@ -461,14 +461,24 @@ func EncodeX509(cert *x509.Certificate) ([]byte, error) { return caPem.Bytes(), nil } -// EncodeX509Chain will encode an *x509.Certificate chain into PEM format. +// EncodeX509Chain will encode a list of *x509.Certificates into a PEM format chain. +// Self-signed certificates are not included as per +// https://datatracker.ietf.org/doc/html/rfc5246#section-7.4.2 +// Certificates are output in the order they're given; if the input is not ordered +// as specified in RFC5246 section 7.4.2, the resulting chain might not be valid +// for use in TLS. func EncodeX509Chain(certs []*x509.Certificate) ([]byte, error) { caPem := bytes.NewBuffer([]byte{}) for _, cert := range certs { - if bytes.Equal(cert.RawIssuer, cert.RawSubject) { + if cert == nil { + continue + } + + if cert.CheckSignatureFrom(cert) == nil { // Don't include self-signed certificate continue } + err := pem.Encode(caPem, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw}) if err != nil { return nil, err diff --git a/pkg/util/pki/csr_test.go b/pkg/util/pki/csr_test.go index 45a50371c..de4d465a9 100644 --- a/pkg/util/pki/csr_test.go +++ b/pkg/util/pki/csr_test.go @@ -656,6 +656,7 @@ func TestSignCSRTemplate(t *testing.T) { wantErr: true, }, } + for name, test := range tests { t.Run(name, func(t *testing.T) { actualBundle, err := SignCSRTemplate(test.caCerts, test.caKey, test.template) @@ -669,15 +670,91 @@ func TestSignCSRTemplate(t *testing.T) { // into strings and do a textual diff. expected, _ := DecodeX509CertificateBytes(test.expectedCertPem) actual, _ := DecodeX509CertificateBytes(actualBundle.ChainPEM) + assert.Equal(t, expected.Subject.String(), actual.Subject.String()) } + 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(test.expectedCaCertPem) actual, _ := DecodeX509CertificateBytes(actualBundle.CAPEM) + assert.Equal(t, expected.Subject.String(), actual.Subject.String()) } }) } } + +func TestEncodeX509Chain(t *testing.T) { + root := mustCreateBundle(t, nil, "root") + intA1 := mustCreateBundle(t, root, "intA-1") + intA2 := mustCreateBundle(t, intA1, "intA-2") + leafA1 := mustCreateBundle(t, intA1, "leaf-a1") + leafA2 := mustCreateBundle(t, intA2, "leaf-a2") + leafInterCN := mustCreateBundle(t, intA1, intA1.cert.Subject.CommonName) + + tests := map[string]struct { + inputCerts []*x509.Certificate + expChain []byte + expErr bool + }{ + "simple 3 cert chain should be encoded in the same order as passed, with no root": { + inputCerts: []*x509.Certificate{root.cert, intA1.cert, leafA1.cert}, + expChain: joinPEM(intA1.pem, leafA1.pem), + expErr: false, + }, + "simple 4 cert chain should be encoded in the same order as passed, with no root": { + inputCerts: []*x509.Certificate{root.cert, intA1.cert, intA2.cert, leafA2.cert}, + expChain: joinPEM(intA1.pem, intA2.pem, leafA2.pem), + expErr: false, + }, + "3 cert chain with no leaf be encoded in the same order as passed, with no root": { + inputCerts: []*x509.Certificate{root.cert, intA1.cert, intA2.cert}, + expChain: joinPEM(intA1.pem, intA2.pem), + expErr: false, + }, + "chain with a non-root cert where issuer matches subject should include that cert but not root": { + // see https://github.com/jetstack/cert-manager/issues/4142#issuecomment-884248923 + inputCerts: []*x509.Certificate{root.cert, intA1.cert, leafInterCN.cert}, + expChain: joinPEM(intA1.pem, leafInterCN.pem), + expErr: false, + }, + "empty input chain should result in no output and no error": { + inputCerts: []*x509.Certificate{}, + expChain: []byte(""), + expErr: false, + }, + "chain with just a root should result in no output and no error": { + inputCerts: []*x509.Certificate{root.cert}, + expChain: []byte(""), + expErr: false, + }, + "chain with just a leaf should result in just the leaf": { + inputCerts: []*x509.Certificate{leafA1.cert}, + expChain: leafA1.pem, + expErr: false, + }, + "nil certs are ignored": { + inputCerts: []*x509.Certificate{leafA1.cert, nil}, + expChain: leafA1.pem, + expErr: false, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + chainOut, err := EncodeX509Chain(test.inputCerts) + + if (err != nil) != test.expErr { + t.Errorf("unexpected error, exp=%t got=%v", + test.expErr, err) + } + + if !reflect.DeepEqual(chainOut, test.expChain) { + t.Errorf("unexpected output from EncodeX509Chain, exp=%+s got=%+s", + test.expChain, chainOut) + } + }) + } +} diff --git a/pkg/util/pki/parse_test.go b/pkg/util/pki/parse_test.go index 1b49b2081..4f01f48a2 100644 --- a/pkg/util/pki/parse_test.go +++ b/pkg/util/pki/parse_test.go @@ -219,7 +219,7 @@ func mustCreateBundle(t *testing.T, issuer *testBundle, name string) *testBundle ) if issuer == nil { - // Selfsigned (no issuer) + // No issuer implies the cert should be self signed issuerKey = pk issuerCert = template } else { @@ -235,6 +235,14 @@ func mustCreateBundle(t *testing.T, issuer *testBundle, name string) *testBundle return &testBundle{pem: certPEM, cert: cert, pk: pk} } +func joinPEM(first []byte, rest ...[]byte) []byte { + for _, b := range rest { + first = append(first, b...) + } + + return first +} + func TestParseSingleCertificateChain(t *testing.T) { root := mustCreateBundle(t, nil, "root") intA1 := mustCreateBundle(t, root, "intA-1") @@ -242,15 +250,9 @@ func TestParseSingleCertificateChain(t *testing.T) { intB1 := mustCreateBundle(t, root, "intB-1") intB2 := mustCreateBundle(t, intB1, "intB-2") leaf := mustCreateBundle(t, intA2, "leaf") + leafInterCN := mustCreateBundle(t, intA2, intA2.cert.Subject.CommonName) random := mustCreateBundle(t, nil, "random") - joinPEM := func(first []byte, rest ...[]byte) []byte { - for _, b := range rest { - first = append(first, b...) - } - return first - } - tests := map[string]struct { inputBundle []byte expPEMBundle PEMBundle @@ -286,6 +288,12 @@ func TestParseSingleCertificateChain(t *testing.T) { expPEMBundle: PEMBundle{ChainPEM: joinPEM(leaf.pem, intA2.pem, intA1.pem), CAPEM: root.pem}, expErr: false, }, + "if certificate chain has two certs with the same CN, shouldn't affect output": { + // see https://github.com/jetstack/cert-manager/issues/4142 + inputBundle: joinPEM(leafInterCN.pem, intA1.pem, intA2.pem, root.pem), + expPEMBundle: PEMBundle{ChainPEM: joinPEM(leafInterCN.pem, intA2.pem, intA1.pem), CAPEM: root.pem}, + expErr: false, + }, "if 4 certificate chain passed out of order, should return single ca and chain in order": { inputBundle: joinPEM(root.pem, intA1.pem, leaf.pem, intA2.pem), expPEMBundle: PEMBundle{ChainPEM: joinPEM(leaf.pem, intA2.pem, intA1.pem), CAPEM: root.pem},