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},