From 1be0ebae8a457fe097b6740e459caa448897bc30 Mon Sep 17 00:00:00 2001 From: hc-github-team-secure-vault-core <82990506+hc-github-team-secure-vault-core@users.noreply.github.com> Date: Tue, 13 Jun 2023 16:13:41 -0400 Subject: [PATCH] backport of commit 3b14cd2061b49c8c698205eef87ceb3d0e69983b (#21181) Co-authored-by: Steven Clark --- builtin/logical/pki/path_acme_order.go | 9 ++- builtin/logical/pki/path_acme_test.go | 77 ++++++++++++++++++++++++++ sdk/helper/certutil/certutil_test.go | 61 ++++++++++++++++++++ sdk/helper/certutil/helpers.go | 65 ++++++++++++++++++++++ 4 files changed, 211 insertions(+), 1 deletion(-) diff --git a/builtin/logical/pki/path_acme_order.go b/builtin/logical/pki/path_acme_order.go index 6c1918eec..eb2ffaf9e 100644 --- a/builtin/logical/pki/path_acme_order.go +++ b/builtin/logical/pki/path_acme_order.go @@ -619,7 +619,14 @@ func parseCsrFromFinalize(data map[string]interface{}) (*x509.CertificateRequest for _, ext := range csr.Extensions { if ext.Id.Equal(certutil.ExtensionBasicConstraintsOID) { - return nil, fmt.Errorf("%w: refusing to accept CSR with Basic Constraints extension", ErrBadCSR) + isCa, _, err := certutil.ParseBasicConstraintExtension(ext) + if err != nil { + return nil, fmt.Errorf("%w: refusing to accept CSR with Basic Constraints extension: %v", ErrBadCSR, err.Error()) + } + + if isCa { + return nil, fmt.Errorf("%w: refusing to accept CSR with Basic Constraints extension with CA set to true", ErrBadCSR) + } } } diff --git a/builtin/logical/pki/path_acme_test.go b/builtin/logical/pki/path_acme_test.go index 0ec822b21..aed906863 100644 --- a/builtin/logical/pki/path_acme_test.go +++ b/builtin/logical/pki/path_acme_test.go @@ -24,6 +24,8 @@ import ( "testing" "time" + "github.com/hashicorp/vault/sdk/helper/certutil" + "github.com/go-test/deep" "github.com/stretchr/testify/require" "golang.org/x/crypto/acme" @@ -974,6 +976,81 @@ func TestIssuerRoleDirectoryAssociations(t *testing.T) { // 5. } +// TestAcmeWithCsrIncludingBasicConstraintExtension verify that we error out for a CSR that is requesting a +// certificate with the IsCA set to true, false is okay, within the basic constraints extension and that no matter what +// the extension is not present on the returned certificate. +func TestAcmeWithCsrIncludingBasicConstraintExtension(t *testing.T) { + t.Parallel() + + cluster, client, _ := setupAcmeBackend(t) + defer cluster.Cleanup() + + testCtx := context.Background() + + baseAcmeURL := "/v1/pki/acme/" + accountKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err, "failed creating rsa key") + + acmeClient := getAcmeClientForCluster(t, cluster, baseAcmeURL, accountKey) + + // Create new account + t.Logf("Testing register on %s", baseAcmeURL) + acct, err := acmeClient.Register(testCtx, &acme.Account{}, func(tosURL string) bool { return true }) + require.NoError(t, err, "failed registering account") + + // Create an order + t.Logf("Testing Authorize Order on %s", baseAcmeURL) + identifiers := []string{"*.localdomain"} + order, err := acmeClient.AuthorizeOrder(testCtx, []acme.AuthzID{ + {Type: "dns", Value: identifiers[0]}, + }) + require.NoError(t, err, "failed creating order") + + // HACK: Update authorization/challenge to completed as we can't really do it properly in this workflow test. + markAuthorizationSuccess(t, client, acmeClient, acct, order) + + // Build a CSR with IsCA set to true, making sure we reject it + extension, err := certutil.CreateBasicConstraintExtension(true, -1) + require.NoError(t, err, "failed generating basic constraint extension") + + isCATrueCSR := &x509.CertificateRequest{ + DNSNames: []string{identifiers[0]}, + ExtraExtensions: []pkix.Extension{extension}, + } + csrKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err, "failed generated key for CSR") + csr, err := x509.CreateCertificateRequest(rand.Reader, isCATrueCSR, csrKey) + require.NoError(t, err, "failed generating csr") + + _, _, err = acmeClient.CreateOrderCert(testCtx, order.FinalizeURL, csr, true) + require.Error(t, err, "order finalization should have failed with IsCA set to true") + + extension, err = certutil.CreateBasicConstraintExtension(false, -1) + require.NoError(t, err, "failed generating basic constraint extension") + isCAFalseCSR := &x509.CertificateRequest{ + DNSNames: []string{identifiers[0]}, + Extensions: []pkix.Extension{extension}, + } + + csr, err = x509.CreateCertificateRequest(rand.Reader, isCAFalseCSR, csrKey) + require.NoError(t, err, "failed generating csr") + + certs, _, err := acmeClient.CreateOrderCert(testCtx, order.FinalizeURL, csr, true) + require.NoError(t, err, "order finalization should have failed with IsCA set to false") + + require.GreaterOrEqual(t, len(certs), 1, "expected at least one cert in bundle") + acmeCert, err := x509.ParseCertificate(certs[0]) + require.NoError(t, err, "failed parsing acme cert") + + // Make sure we don't have any basic constraint extension within the returned cert + for _, ext := range acmeCert.Extensions { + if ext.Id.Equal(certutil.ExtensionBasicConstraintsOID) { + // We shouldn't have this extension in our cert + t.Fatalf("acme csr contained a basic constraints extension") + } + } +} + func markAuthorizationSuccess(t *testing.T, client *api.Client, acmeClient *acme.Client, acct *acme.Account, order *acme.Order, ) { diff --git a/sdk/helper/certutil/certutil_test.go b/sdk/helper/certutil/certutil_test.go index 4ee3d1c4e..8b5509461 100644 --- a/sdk/helper/certutil/certutil_test.go +++ b/sdk/helper/certutil/certutil_test.go @@ -13,6 +13,7 @@ import ( "crypto/rsa" "crypto/x509" "crypto/x509/pkix" + "encoding/asn1" "encoding/json" "encoding/pem" "fmt" @@ -945,6 +946,66 @@ func TestSignatureAlgorithmRoundTripping(t *testing.T) { } } +// TestParseBasicConstraintExtension Verify extension generation and parsing of x509 basic constraint extensions +// works as expected. +func TestBasicConstraintExtension(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + isCA bool + maxPathLen int + }{ + {"empty-seq", false, -1}, + {"just-ca-true", true, -1}, + {"just-ca-with-maxpathlen", true, 2}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ext, err := CreateBasicConstraintExtension(tt.isCA, tt.maxPathLen) + if err != nil { + t.Fatalf("failed generating basic extension: %v", err) + } + + gotIsCa, gotMaxPathLen, err := ParseBasicConstraintExtension(ext) + if err != nil { + t.Fatalf("failed parsing basic extension: %v", err) + } + + if tt.isCA != gotIsCa { + t.Fatalf("expected isCa (%v) got isCa (%v)", tt.isCA, gotIsCa) + } + + if tt.maxPathLen != gotMaxPathLen { + t.Fatalf("expected maxPathLen (%v) got maxPathLen (%v)", tt.maxPathLen, gotMaxPathLen) + } + }) + } + + t.Run("bad-extension-oid", func(t *testing.T) { + // Test invalid type errors out + _, _, err := ParseBasicConstraintExtension(pkix.Extension{}) + if err == nil { + t.Fatalf("should have failed parsing non-basic constraint extension") + } + }) + + t.Run("garbage-value", func(t *testing.T) { + extraBytes, err := asn1.Marshal("a string") + if err != nil { + t.Fatalf("failed encoding the struct: %v", err) + } + ext := pkix.Extension{ + Id: ExtensionBasicConstraintsOID, + Value: extraBytes, + } + _, _, err = ParseBasicConstraintExtension(ext) + if err == nil { + t.Fatalf("should have failed parsing basic constraint with extra information") + } + }) +} + func genRsaKey(t *testing.T) *rsa.PrivateKey { key, err := rsa.GenerateKey(rand.Reader, 2048) if err != nil { diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go index b609215b2..28472027f 100644 --- a/sdk/helper/certutil/helpers.go +++ b/sdk/helper/certutil/helpers.go @@ -1392,3 +1392,68 @@ func CreateDeltaCRLIndicatorExt(completeCRLNumber int64) (pkix.Extension, error) Value: bigNumValue, }, nil } + +// ParseBasicConstraintExtension parses a basic constraint pkix.Extension, useful if attempting to validate +// CSRs are requesting CA privileges as Go does not expose its implementation. Values returned are +// IsCA, MaxPathLen or error. If MaxPathLen was not set, a value of -1 will be returned. +func ParseBasicConstraintExtension(ext pkix.Extension) (bool, int, error) { + if !ext.Id.Equal(ExtensionBasicConstraintsOID) { + return false, -1, fmt.Errorf("passed in extension was not a basic constraint extension") + } + + // All elements are set to optional here, as it is possible that we receive a CSR with the extension + // containing an empty sequence by spec. + type basicConstraints struct { + IsCA bool `asn1:"optional"` + MaxPathLen int `asn1:"optional,default:-1"` + } + bc := &basicConstraints{} + leftOver, err := asn1.Unmarshal(ext.Value, bc) + if err != nil { + return false, -1, fmt.Errorf("failed unmarshalling extension value: %w", err) + } + + numLeftOver := len(bytes.TrimSpace(leftOver)) + if numLeftOver > 0 { + return false, -1, fmt.Errorf("%d extra bytes within basic constraints value extension", numLeftOver) + } + + return bc.IsCA, bc.MaxPathLen, nil +} + +// CreateBasicConstraintExtension create a basic constraint extension based on inputs, +// if isCa is false, an empty value sequence will be returned with maxPath being +// ignored. If isCa is true maxPath can be set to -1 to not set a maxPath value. +func CreateBasicConstraintExtension(isCa bool, maxPath int) (pkix.Extension, error) { + var asn1Bytes []byte + var err error + + switch { + case isCa && maxPath >= 0: + CaAndMaxPathLen := struct { + IsCa bool `asn1:""` + MaxPathLen int `asn1:""` + }{ + IsCa: isCa, + MaxPathLen: maxPath, + } + asn1Bytes, err = asn1.Marshal(CaAndMaxPathLen) + case isCa && maxPath < 0: + justCa := struct { + IsCa bool `asn1:""` + }{IsCa: isCa} + asn1Bytes, err = asn1.Marshal(justCa) + default: + asn1Bytes, err = asn1.Marshal(struct{}{}) + } + + if err != nil { + return pkix.Extension{}, err + } + + return pkix.Extension{ + Id: ExtensionBasicConstraintsOID, + Critical: true, + Value: asn1Bytes, + }, nil +}