Merge pull request #4238 from jetstack-bot/cherry-pick-4237-to-release-1.4

[release-1.4] Fix check for self-signed certs in EncodeX509Chain
2026-08-09 05:06:38 +10:00 · 2021-07-21 18:35:20 +01:00
parent 655e63122c c686a2f69a
commit f970eca762
3 changed files with 106 additions and 11 deletions
@@ -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
@@ -650,6 +650,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)
@@ -663,15 +664,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)
+			}
+		})
+	}
+}
@@ -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},