Merge pull request #4261 from SgtCoDFish/tsuru-ca-chain-without-root

CA chain fix without root

pkg/internal/vault/vault_test.go

@@ -222,6 +222,12 @@ func TestSign(t *testing.T) {
 		t.FailNow()
 	}
 
+	rootBundleData, err := bundlePEM(testIntermediateCa, testRootCa)
+	if err != nil {
+		t.Errorf("failed to encode root bundle for testing: %s", err)
+		t.FailNow()
+	}
+
 	tests := map[string]testSignT{
 		"a garbage csr should return err": {
 			csrPEM:       []byte("a bad csr"),
@@ -241,7 +247,7 @@ func TestSign(t *testing.T) {
 			expectedCA:   "",
 		},
 
-		"a good csr and good response should return a certificate": {
+		"a good csr and good response with no root should return a certificate with the intermediate in the chain and as the CA": {
 			csrPEM: csrPEM,
 			issuer: gen.Issuer("vault-issuer",
 				gen.SetIssuerVault(cmapi.VaultIssuer{}),
@@ -251,10 +257,24 @@ func TestSign(t *testing.T) {
 					Body: ioutil.NopCloser(bytes.NewReader(bundleData))},
 			}, nil),
 			expectedErr:  nil,
-			expectedCert: testLeafCertificate,
+			expectedCert: testLeafCertificate + testIntermediateCa,
 			expectedCA:   testIntermediateCa,
 		},
 
+		"a good csr and good response with a root should return a certificate without the root in the chain but with the root as the CA": {
+			csrPEM: csrPEM,
+			issuer: gen.Issuer("vault-issuer",
+				gen.SetIssuerVault(cmapi.VaultIssuer{}),
+			),
+			fakeClient: vaultfake.NewFakeClient().WithRawRequest(&vault.Response{
+				Response: &http.Response{
+					Body: ioutil.NopCloser(bytes.NewReader(rootBundleData))},
+			}, nil),
+			expectedErr:  nil,
+			expectedCert: testLeafCertificate + testIntermediateCa,
+			expectedCA:   testRootCa,
+		},
+
 		"vault issuer with namespace specified": {
 			csrPEM: csrPEM,
 			issuer: gen.Issuer("vault-issuer",
@@ -265,7 +285,7 @@ func TestSign(t *testing.T) {
 					Body: ioutil.NopCloser(bytes.NewReader(bundleData))},
 			}, nil),
 			expectedErr:  nil,
-			expectedCert: testLeafCertificate,
+			expectedCert: testLeafCertificate + testIntermediateCa,
 			expectedCA:   testIntermediateCa,
 		},
 	}
@@ -321,7 +341,7 @@ func TestExtractCertificatesFromVaultCertificateSecret(t *testing.T) {
 	tests := map[string]testExtractCertificatesFromVaultCertT{
 		"when a Vault engine is a root CA": {
 			secret:       signedCertificateSecret(testIntermediateCa),
-			expectedCert: testLeafCertificate,
+			expectedCert: testLeafCertificate + testIntermediateCa,
 			expectedCA:   testIntermediateCa,
 		},
 		"when a Vault engine is an intermediate CA, and its parent is a root CA": {

pkg/util/pki/parse.go

@@ -252,8 +252,21 @@ func (c *chainNode) toBundleAndCA() (PEMBundle, error) {
 
 	for {
 		// If the issuer is nil, we have hit the root of the chain. Assign the CA
-		// to this certificate and stop traversing.
+		// to this certificate and stop traversing. If the certificate at the root
+		// of the chain is not self-signed (i.e. is not a root CA), then also append
+		// that certificate to the chain.
+
+		// Root certificates are omitted from the chain as per
+		// https://datatracker.ietf.org/doc/html/rfc5246#section-7.4.2
+		// > [T]he self-signed certificate that specifies the root certificate authority
+		// > MAY be omitted from the chain, under the assumption that the remote end must
+		// > already possess it in order to validate it in any case.
+
 		if c.issuer == nil {
+			if len(certs) > 0 && !isSelfSignedCertificate(c.cert) {
+				certs = append(certs, c.cert)
+			}
+
 			ca = c.cert
 			break
 		}
@@ -270,8 +283,13 @@ func (c *chainNode) toBundleAndCA() (PEMBundle, error) {
 	}
 
 	// If no certificates parsed, then CA is the only certificate and should be
-	// the chain
+	// the chain. If the CA is also self-signed, then by definition it's also the
+	// issuer and so can be placed in CAPEM too.
 	if len(certs) == 0 {
+		if isSelfSignedCertificate(ca) {
+			return PEMBundle{ChainPEM: caPEM, CAPEM: caPEM}, nil
+		}
+
 		return PEMBundle{ChainPEM: caPEM}, nil
 	}
 
@@ -336,3 +354,9 @@ func (c *chainNode) root() *chainNode {
 
 	return c
 }
+
+// isSelfSignedCertificate returns true if the given X.509 certificate has been
+// signed by itself, which would make it a "root" certificate.
+func isSelfSignedCertificate(cert *x509.Certificate) bool {
+	return cert.CheckSignatureFrom(cert) == nil
+}

pkg/util/pki/parse_test.go

@@ -258,16 +258,16 @@ func TestParseSingleCertificateChain(t *testing.T) {
 		expPEMBundle PEMBundle
 		expErr       bool
 	}{
-		"if single certificate passed, return single certificate": {
-			inputBundle:  root.pem,
-			expPEMBundle: PEMBundle{ChainPEM: root.pem},
-			expErr:       false,
-		},
 		"if two certificate chain passed in order, should return single ca and certificate": {
 			inputBundle:  joinPEM(intA1.pem, root.pem),
 			expPEMBundle: PEMBundle{ChainPEM: intA1.pem, CAPEM: root.pem},
 			expErr:       false,
 		},
+		"if two certificate chain passed with leaf and intermediate, should return both certs in chain with intermediate as CA": {
+			inputBundle:  joinPEM(leaf.pem, intA2.pem),
+			expPEMBundle: PEMBundle{ChainPEM: joinPEM(leaf.pem, intA2.pem), CAPEM: intA2.pem},
+			expErr:       false,
+		},
 		"if two certificate chain passed out of order, should return single ca and certificate": {
 			inputBundle:  joinPEM(root.pem, intA1.pem),
 			expPEMBundle: PEMBundle{ChainPEM: intA1.pem, CAPEM: root.pem},
@@ -324,6 +324,26 @@ func TestParseSingleCertificateChain(t *testing.T) {
 			expPEMBundle: PEMBundle{},
 			expErr:       true,
 		},
+		"if certificate chain does not have a root ca, should append all intermediates to ChainPEM and use the root-most cert as CAPEM": {
+			inputBundle:  joinPEM(intA1.pem, intA2.pem, leaf.pem),
+			expPEMBundle: PEMBundle{ChainPEM: joinPEM(leaf.pem, intA2.pem, intA1.pem), CAPEM: intA1.pem},
+			expErr:       false,
+		},
+		"if only a single leaf certificate was parsed, ChainPEM should contain a single leaf certificate and CAPEM should remain empty": {
+			inputBundle:  joinPEM(leaf.pem),
+			expPEMBundle: PEMBundle{ChainPEM: joinPEM(leaf.pem), CAPEM: nil},
+			expErr:       false,
+		},
+		"if only a single intermediate certificate was parsed, ChainPEM should contain a single intermediate certificate and CAPEM should remain empty": {
+			inputBundle:  joinPEM(intA1.pem),
+			expPEMBundle: PEMBundle{ChainPEM: joinPEM(intA1.pem), CAPEM: nil},
+			expErr:       false,
+		},
+		"if only a single root certificate was parsed, ChainPEM should contain a single root certificate and CAPEM should also contain that root": {
+			inputBundle:  joinPEM(root.pem),
+			expPEMBundle: PEMBundle{ChainPEM: joinPEM(root.pem), CAPEM: root.pem},
+			expErr:       false,
+		},
 	}
 
 	for name, test := range tests {