backport of commit 3b14cd2061b49c8c698205eef87ceb3d0e69983b (#21181)

Co-authored-by: Steven Clark <steven.clark@hashicorp.com>

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)
+			}
 		}
 	}
 

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,
 ) {

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 {

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
+}