Sort CA chain into root and intermediates on VerifyCertificate. (#29255)

Sort CA chain into root and intermediates on VerifyCertificate.

In order for the Certificate.Verify method to work correctly, the certificates
in the CA chain need to be sorted into separate root and intermediate
certificate pools.

Add unit tests to verify that name constraints in both the root and intermediate
certificates are checked.

builtin/logical/pki/cert_util_test.go

@@ -8,6 +8,7 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"fmt"
+	"github.com/hashicorp/vault/sdk/helper/testhelpers/schema"
 	"net"
 	"net/url"
 	"os"
@@ -1165,3 +1166,115 @@ func testParseCsrToFields(t *testing.T, issueTime time.Time, tt *parseCertificat
 		t.Errorf("testParseCertificateToFields() diff: %s", strings.ReplaceAll(strings.Join(diff, "\n"), "map", "\nmap"))
 	}
 }
+
+// TestVerify_chained_name_constraints verifies that we perform name constraints certificate validation using the
+// entire CA chain.
+//
+// This test constructs a root CA that
+// - allows: .example.com
+// - excludes: bad.example.com
+//
+// and an intermediate that
+// - forbids alsobad.example.com
+//
+// It verifies that the intermediate
+// - can issue certs like good.example.com
+// - rejects names like notanexample.com since they are not in the namespace of names permitted by the root CA
+// - rejects bad.example.com, since the root CA excludes it
+// - rejects alsobad.example.com, since the intermediate CA excludes it.
+func TestVerify_chained_name_constraints(t *testing.T) {
+	t.Parallel()
+	bRoot, sRoot := CreateBackendWithStorage(t)
+
+	////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+	// Setup
+
+	var bInt *backend
+	var sInt logical.Storage
+	{
+		resp, err := CBWrite(bRoot, sRoot, "root/generate/internal", map[string]interface{}{
+			"ttl":                   "40h",
+			"common_name":           "myvault.com",
+			"permitted_dns_domains": ".example.com",
+			"excluded_dns_domains":  "bad.example.com",
+		})
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+
+		// Create the CSR
+		bInt, sInt = CreateBackendWithStorage(t)
+		resp, err = CBWrite(bInt, sInt, "intermediate/generate/internal", map[string]interface{}{
+			"common_name": "myint.com",
+		})
+		require.NoError(t, err)
+		schema.ValidateResponse(t, schema.GetResponseSchema(t, bRoot.Route("intermediate/generate/internal"), logical.UpdateOperation), resp, true)
+		csr := resp.Data["csr"]
+
+		// Sign the CSR
+		resp, err = CBWrite(bRoot, sRoot, "root/sign-intermediate", map[string]interface{}{
+			"common_name":          "myint.com",
+			"csr":                  csr,
+			"ttl":                  "60h",
+			"excluded_dns_domains": "alsobad.example.com",
+		})
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+
+		// Import the New Signed Certificate into the Intermediate Mount.
+		// Note that we append the root CA certificate to the signed intermediate, so that
+		// the entire chain is stored by set-signed.
+		resp, err = CBWrite(bInt, sInt, "intermediate/set-signed", map[string]interface{}{
+			"certificate": strings.Join(resp.Data["ca_chain"].([]string), "\n"),
+		})
+		require.NoError(t, err)
+
+		// Create a Role in the Intermediate Mount
+		resp, err = CBWrite(bInt, sInt, "roles/test", map[string]interface{}{
+			"allow_bare_domains": true,
+			"allow_subdomains":   true,
+			"allow_any_name":     true,
+		})
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+	}
+
+	////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+	// Tests
+
+	testCases := []struct {
+		commonName string
+		wantError  string
+	}{
+		{
+			commonName: "good.example.com",
+		},
+		{
+			commonName: "notanexample.com",
+			wantError:  "should not be permitted by root CA",
+		},
+		{
+			commonName: "bad.example.com",
+			wantError:  "should be rejected by the root CA",
+		},
+		{
+			commonName: "alsobad.example.com",
+			wantError:  "should be rejected by the intermediate CA",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.commonName, func(t *testing.T) {
+			resp, err := CBWrite(bInt, sInt, "issue/test", map[string]any{
+				"common_name": tc.commonName,
+			})
+			if tc.wantError != "" {
+				require.Error(t, err, tc.wantError)
+				require.ErrorContains(t, err, "certificate is not authorized to sign for this name")
+				require.Nil(t, resp)
+			} else {
+				require.NoError(t, err)
+				require.NoError(t, resp.Error())
+			}
+		})
+	}
+}

builtin/logical/pki/issuing/cert_verify.go

@@ -4,6 +4,7 @@
 package issuing
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"os"
@@ -42,26 +43,35 @@ func VerifyCertificate(ctx context.Context, storage logical.Storage, issuerId Is
 		return nil
 	}
 
-	certChainPool := ctx509.NewCertPool()
+	rootCertPool := ctx509.NewCertPool()
+	intermediateCertPool := ctx509.NewCertPool()
+
 	for _, certificate := range parsedBundle.CAChain {
 		cert, err := convertCertificate(certificate.Bytes)
 		if err != nil {
 			return err
 		}
-		certChainPool.AddCert(cert)
+		if bytes.Equal(cert.RawIssuer, cert.RawSubject) {
+			rootCertPool.AddCert(cert)
+		} else {
+			intermediateCertPool.AddCert(cert)
+		}
+	}
+	if len(rootCertPool.Subjects()) < 1 {
+		// Alright, this is weird, since we don't have the root CA, we'll treat the intermediate as
+		// the root, otherwise we'll get a "x509: certificate signed by unknown authority" error.
+		rootCertPool, intermediateCertPool = intermediateCertPool, rootCertPool
 	}
-
-	// Validation Code, assuming we need to validate the entire chain of constraints
 
 	// Note that we use github.com/google/certificate-transparency-go/x509 to perform certificate verification,
 	// since that library provides options to disable checks that the standard library does not.
 
 	options := ctx509.VerifyOptions{
-		Intermediates:                  nil, // We aren't verifying the chain here, this would do more work
-		Roots:                          certChainPool,
+		Roots:                          rootCertPool,
+		Intermediates:                  intermediateCertPool,
 		CurrentTime:                    time.Time{},
 		KeyUsages:                      nil,
-		MaxConstraintComparisions:      0, // This means infinite
+		MaxConstraintComparisions:      0, // Use the library's 'sensible default'
 		DisableTimeChecks:              true,
 		DisableEKUChecks:               true,
 		DisableCriticalExtensionChecks: false,

changelog/29255.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/pki: Fix a bug that prevented the full CA chain to be used when enforcing name constraints.
+```