From f6910bbb2e947b354905836f33912fe3194dbcc5 Mon Sep 17 00:00:00 2001 From: Victor Rodriguez Date: Mon, 23 Dec 2024 20:56:41 +0100 Subject: [PATCH] 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 | 113 +++++++++++++++++++++ builtin/logical/pki/issuing/cert_verify.go | 24 +++-- changelog/29255.txt | 3 + 3 files changed, 133 insertions(+), 7 deletions(-) create mode 100644 changelog/29255.txt diff --git a/builtin/logical/pki/cert_util_test.go b/builtin/logical/pki/cert_util_test.go index 30f0f71c7715..90b8a12f438f 100644 --- a/builtin/logical/pki/cert_util_test.go +++ b/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()) + } + }) + } +} diff --git a/builtin/logical/pki/issuing/cert_verify.go b/builtin/logical/pki/issuing/cert_verify.go index f17cd7de087d..a97f67bc5d43 100644 --- a/builtin/logical/pki/issuing/cert_verify.go +++ b/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, diff --git a/changelog/29255.txt b/changelog/29255.txt new file mode 100644 index 000000000000..2d9eda7b28af --- /dev/null +++ b/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. +```