Better ACME wildcard validation (#20289)

* Refactor wildcard validation checks Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Add helper to determine if identifier is wildcard Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Better validate wildcard acceptance This correctly validates wildcards to contain only a leading prefix (*.) and must include another label, also ensuring that the remaining domain components pass non-IP and IDNA validation, and removing them from display on the authorization. Finally, we restrict the challenge types available for various combinations of IP, DNS, and wildcard addresses. This then updates the tests to validate this as well. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> --------- Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
2025-11-02 03:27:54 +00:00 · 2023-04-21 12:54:19 -04:00
parent 166f270613
commit 5e700e3347
5 changed files with 224 additions and 92 deletions
--- a/builtin/logical/pki/acme_authorizations.go
+++ b/builtin/logical/pki/acme_authorizations.go
@@ -4,6 +4,7 @@
 package pki

 import (
+	"fmt"
 	"time"
 )

@@ -15,8 +16,44 @@ const (
 )

 type ACMEIdentifier struct {
-	Type  ACMEIdentifierType `json:"type"`
-	Value string             `json:"value"`
+	Type          ACMEIdentifierType `json:"type"`
+	Value         string             `json:"value"`
+	OriginalValue string             `json:"original_value"`
+	IsWildcard    bool               `json:"is_wildcard"`
+}
+
+func (ai *ACMEIdentifier) MaybeParseWildcard() (bool, string, error) {
+	if ai.Type != ACMEDNSIdentifier || !isWildcardDomain(ai.Value) {
+		return false, ai.Value, nil
+	}
+
+	// Here on out, technically it is a wildcard.
+	ai.IsWildcard = true
+
+	wildcardLabel, reducedName, err := validateWildcardDomain(ai.Value)
+	if err != nil {
+		return true, "", err
+	}
+
+	if wildcardLabel != "*" {
+		// Per RFC 8555 Section. 7.1.3. Order Objects:
+		//
+		// > Any identifier of type "dns" in a newOrder request MAY have a
+		// > wildcard domain name as its value.  A wildcard domain name consists
+		// > of a single asterisk character followed by a single full stop
+		// > character ("*.") followed by a domain name as defined for use in the
+		// > Subject Alternate Name Extension by [RFC5280].
+		return true, "", fmt.Errorf("wildcard must be entire left-most label")
+	}
+
+	if reducedName == "" {
+		return true, "", fmt.Errorf("wildcard must not be entire domain name; need at least two domain labels")
+	}
+
+	// Parsing was indeed successful, so update our reduced name.
+	ai.Value = reducedName
+
+	return true, reducedName, nil
 }

 type ACMEAuthorizationStatusType string
--- a/builtin/logical/pki/acme_state.go
+++ b/builtin/logical/pki/acme_state.go
@@ -178,7 +178,10 @@ func (o acmeOrder) getIdentifierDNSValues() []string {
 	var identifiers []string
 	for _, value := range o.Identifiers {
 		if value.Type == ACMEDNSIdentifier {
-			identifiers = append(identifiers, value.Value)
+			// Here, because of wildcard processing, we need to use the
+			// original value provided by the caller rather than the
+			// post-modification (trimmed '*.' prefix) value.
+			identifiers = append(identifiers, value.OriginalValue)
 		}
 	}
 	return identifiers
--- a/builtin/logical/pki/cert_util.go
+++ b/builtin/logical/pki/cert_util.go
@@ -336,6 +336,71 @@ func validateCommonName(b *backend, data *inputBundle, name string) string {
 	return ""
 }

+func isWildcardDomain(name string) bool {
+	// Per RFC 6125 Section 6.4.3, and explicitly contradicting the earlier
+	// RFC 2818 which no modern client will validate against, there are two
+	// main types of wildcards, each with a single wildcard specifier (`*`,
+	// functionally different from the `*` used as a glob from the
+	// AllowGlobDomains parsing path) in the left-most label:
+	//
+	//  1. Entire label is a single wildcard character (most common and
+	//     well-supported),
+	//  2. Part of the label contains a single wildcard character (e.g. per
+	//     RFC 6125: baz*.example.net, *baz.example.net, or b*z.example.net).
+	//
+	// We permit issuance of both but not the older RFC 2818 style under
+	// the new AllowWildcardCertificates option. However, anything with a
+	// glob character is technically a wildcard, though not a valid one.
+
+	return strings.Contains(name, "*")
+}
+
+func validateWildcardDomain(name string) (string, string, error) {
+	// See note in isWildcardDomain(...) about the definition of a wildcard
+	// domain.
+	var wildcardLabel string
+	var reducedName string
+
+	if strings.Count(name, "*") > 1 {
+		// As mentioned above, only one wildcard character is permitted
+		// under RFC 6125 semantics.
+		return wildcardLabel, reducedName, fmt.Errorf("expected only one wildcard identifier in the given domain name")
+	}
+
+	// Split the Common Name into two parts: a left-most label and the
+	// remaining segments (if present).
+	splitLabels := strings.SplitN(name, ".", 2)
+	if len(splitLabels) != 2 {
+		// We've been given a single-part domain name that consists
+		// entirely of a wildcard. This is a little tricky to handle,
+		// but EnforceHostnames validates both the wildcard-containing
+		// label and the reduced name, but _only_ the latter if it is
+		// non-empty. This allows us to still validate the only label
+		// component matches hostname expectations still.
+		wildcardLabel = splitLabels[0]
+		reducedName = ""
+	} else {
+		// We have a (at least) two label domain name. But before we can
+		// update our names, we need to validate the wildcard ended up
+		// in the segment we expected it to. While this is (kinda)
+		// validated under EnforceHostnames's leftWildLabelRegex, we
+		// still need to validate it in the non-enforced mode.
+		//
+		// By validated assumption above, we know there's strictly one
+		// wildcard in this domain so we only need to check the wildcard
+		// label or the reduced name (as one is equivalent to the other).
+		// Because we later assume reducedName _lacks_ wildcard segments,
+		// we validate that.
+		wildcardLabel = splitLabels[0]
+		reducedName = splitLabels[1]
+		if strings.Contains(reducedName, "*") {
+			return wildcardLabel, reducedName, fmt.Errorf("expected wildcard to only be present in left-most domain label")
+		}
+	}
+
+	return wildcardLabel, reducedName, nil
+}
+
 // Given a set of requested names for a certificate, verifies that all of them
 // match the various toggles set in the role for controlling issuance.
 // If one does not pass, it is returned in the string argument.
@@ -370,21 +435,7 @@ func validateNames(b *backend, data *inputBundle, names []string) string {
 			isEmail = true
 		}

-		// Per RFC 6125 Section 6.4.3, and explicitly contradicting the earlier
-		// RFC 2818 which no modern client will validate against, there are two
-		// main types of wildcards, each with a single wildcard specifier (`*`,
-		// functionally different from the `*` used as a glob from the
-		// AllowGlobDomains parsing path) in the left-most label:
-		//
-		//  1. Entire label is a single wildcard character (most common and
-		//     well-supported),
-		//  2. Part of the label contains a single wildcard character (e.g. per
-		///    RFC 6125: baz*.example.net, *baz.example.net, or b*z.example.net).
-		//
-		// We permit issuance of both but not the older RFC 2818 style under
-		// the new AllowWildcardCertificates option. However, anything with a
-		// glob character is technically a wildcard.
-		if strings.Contains(reducedName, "*") {
+		if isWildcardDomain(reducedName) {
 			// Regardless of later rejections below, this common name contains
 			// a wildcard character and is thus technically a wildcard name.
 			isWildcard = true
@@ -399,42 +450,12 @@ func validateNames(b *backend, data *inputBundle, names []string) string {
 				return name
 			}

-			if strings.Count(reducedName, "*") > 1 {
-				// As mentioned above, only one wildcard character is permitted
-				// under RFC 6125 semantics.
+			// Check that this domain is well-formatted per RFC 6125.
+			var err error
+			wildcardLabel, reducedName, err = validateWildcardDomain(reducedName)
+			if err != nil {
 				return name
 			}
-
-			// Split the Common Name into two parts: a left-most label and the
-			// remaining segments (if present).
-			splitLabels := strings.SplitN(reducedName, ".", 2)
-			if len(splitLabels) != 2 {
-				// We've been given a single-part domain name that consists
-				// entirely of a wildcard. This is a little tricky to handle,
-				// but EnforceHostnames validates both the wildcard-containing
-				// label and the reduced name, but _only_ the latter if it is
-				// non-empty. This allows us to still validate the only label
-				// component matches hostname expectations still.
-				wildcardLabel = splitLabels[0]
-				reducedName = ""
-			} else {
-				// We have a (at least) two label domain name. But before we can
-				// update our names, we need to validate the wildcard ended up
-				// in the segment we expected it to. While this is (kinda)
-				// validated under EnforceHostnames's leftWildLabelRegex, we
-				// still need to validate it in the non-enforced mode.
-				//
-				// By validated assumption above, we know there's strictly one
-				// wildcard in this domain so we only need to check the wildcard
-				// label or the reduced name (as one is equivalent to the other).
-				// Because we later assume reducedName _lacks_ wildcard segments,
-				// we validate that.
-				wildcardLabel = splitLabels[0]
-				reducedName = splitLabels[1]
-				if strings.Contains(reducedName, "*") {
-					return name
-				}
-			}
 		}

 		// Email addresses using wildcard domain names do not make sense
--- a/builtin/logical/pki/path_acme_order.go
+++ b/builtin/logical/pki/path_acme_order.go
@@ -11,7 +11,6 @@ import (
 	"net"
 	"net/http"
 	"sort"
-	"strings"
 	"time"

 	"github.com/hashicorp/vault/sdk/helper/strutil"
@@ -328,11 +327,11 @@ func validateCsrMatchesOrder(csr *x509.CertificateRequest, order *acmeOrder) err
 	}

 	if len(orderDNSIdentifiers) != len(csrDNSIdentifiers) {
-		return fmt.Errorf("%w: Order and CSR mismatch on number of DNS identifiers", ErrBadCSR)
+		return fmt.Errorf("%w: Order (%v) and CSR (%v) mismatch on number of DNS identifiers", ErrBadCSR, len(orderDNSIdentifiers), len(csrDNSIdentifiers))
 	}

 	if len(orderIPIdentifiers) != len(csrIPIdentifiers) {
-		return fmt.Errorf("%w: Order and CSR mismatch on number of IP identifiers", ErrBadCSR)
+		return fmt.Errorf("%w: Order (%v) and CSR (%v) mismatch on number of IP identifiers", ErrBadCSR, len(orderIPIdentifiers), len(csrIPIdentifiers))
 	}

 	for i, identifier := range orderDNSIdentifiers {
@@ -705,8 +704,19 @@ func buildOrderUrl(acmeCtx *acmeContext, orderId string) string {

 func generateAuthorization(acct *acmeAccount, identifier *ACMEIdentifier) (*ACMEAuthorization, error) {
 	authId := genUuid()
+
+	// Certain challenges have certain restrictions: DNS challenges cannot
+	// be used to validate IP addresses, and only DNS challenges can be used
+	// to validate wildcards.
+	allowedChallenges := []ACMEChallengeType{ACMEHTTPChallenge, ACMEDNSChallenge}
+	if identifier.Type == ACMEIPIdentifier {
+		allowedChallenges = []ACMEChallengeType{ACMEHTTPChallenge}
+	} else if identifier.IsWildcard {
+		allowedChallenges = []ACMEChallengeType{ACMEDNSChallenge}
+	}
+
 	var challenges []*ACMEChallenge
-	for _, challengeType := range []ACMEChallengeType{ACMEHTTPChallenge} {
+	for _, challengeType := range allowedChallenges {
 		token, err := getACMEToken()
 		if err != nil {
 			return nil, err
@@ -730,7 +740,7 @@ func generateAuthorization(acct *acmeAccount, identifier *ACMEIdentifier) (*ACME
 		Status:     ACMEAuthorizationPending,
 		Expires:    "", // only populated when it switches to valid.
 		Challenges: challenges,
-		Wildcard:   strings.HasPrefix(identifier.Value, "*."),
+		Wildcard:   identifier.IsWildcard,
 	}, nil
 }

@@ -797,31 +807,62 @@ func parseOrderIdentifiers(data map[string]interface{}) ([]*ACMEIdentifier, erro
 			return nil, fmt.Errorf("value argument for value in 'identifiers' can not be blank: %w", ErrMalformed)
 		}

+		identifier := &ACMEIdentifier{
+			Value:         valueStr,
+			OriginalValue: valueStr,
+		}
+
 		switch typeStr {
 		case string(ACMEIPIdentifier):
+			identifier.Type = ACMEIPIdentifier
 			ip := net.ParseIP(valueStr)
 			if ip == nil {
 				return nil, fmt.Errorf("value argument (%s) failed validation: failed parsing as IP: %w", valueStr, ErrMalformed)
 			}
-			identifiers = append(identifiers, &ACMEIdentifier{
-				Type:  ACMEIPIdentifier,
-				Value: valueStr,
-			})
-
 		case string(ACMEDNSIdentifier):
+			identifier.Type = ACMEDNSIdentifier
+
+			// This check modifies the identifier if it is a wildcard,
+			// removing the non-wildcard portion. We do this before the
+			// IP address checks, in case of an attempt to bypass the IP/DNS
+			// check via including a leading wildcard (e.g., *.127.0.0.1).
+			//
+			// Per RFC 8555 Section 7.1.4. Authorization Objects:
+			//
+			// > Wildcard domain names (with "*" as the first label) MUST NOT
+			// > be included in authorization objects.
+			if _, _, err := identifier.MaybeParseWildcard(); err != nil {
+				return nil, fmt.Errorf("value argument (%s) failed validation: invalid wildcard: %v: %w", valueStr, err, ErrMalformed)
+			}
+
+			if isIP := net.ParseIP(identifier.Value); isIP != nil {
+				return nil, fmt.Errorf("refusing to accept argument (%s) as DNS type identifier: parsed OK as IP address: %w", valueStr, ErrMalformed)
+			}
+
+			// Use the reduced (identifier.Value) in case this was a wildcard
+			// domain.
 			p := idna.New(idna.ValidateForRegistration())
-			converted, err := p.ToASCII(valueStr)
+			converted, err := p.ToASCII(identifier.Value)
 			if err != nil {
 				return nil, fmt.Errorf("value argument (%s) failed validation: %s: %w", valueStr, err.Error(), ErrMalformed)
 			}

-			identifiers = append(identifiers, &ACMEIdentifier{
-				Type:  ACMEDNSIdentifier,
-				Value: converted,
-			})
+			// Per RFC 8555 Section 7.1.4. Authorization Objects:
+			//
+			// > The domain name MUST be encoded in the form in which it
+			// > would appear in a certificate.  That is, it MUST be encoded
+			// > according to the rules in Section 7 of [RFC5280]. Servers
+			// > MUST verify any identifier values that begin with the
+			// > ASCII-Compatible Encoding prefix "xn--" as defined in
+			// > [RFC5890] are properly encoded.
+			if identifier.Value != converted {
+				return nil, fmt.Errorf("value argument (%s) failed IDNA round-tripping to ASCII: %w", valueStr, ErrMalformed)
+			}
 		default:
 			return nil, fmt.Errorf("unsupported identifier type %s: %w", typeStr, ErrUnsupportedIdentifier)
 		}
+
+		identifiers = append(identifiers, identifier)
 	}

 	return identifiers, nil
--- a/builtin/logical/pki/path_acme_test.go
+++ b/builtin/logical/pki/path_acme_test.go
@@ -117,14 +117,26 @@ func TestAcmeBasicWorkflow(t *testing.T) {
 				acme.WithOrderNotAfter(time.Now().Add(10*time.Minute)))
 			require.Error(t, err, "should have rejected a new order with NotAfter set")

+			// Make sure DNS identifiers cannot include IP addresses
+			_, err = acmeClient.AuthorizeOrder(testCtx, []acme.AuthzID{{Type: "dns", Value: "127.0.0.1"}},
+				acme.WithOrderNotAfter(time.Now().Add(10*time.Minute)))
+			require.Error(t, err, "should have rejected a new order with IP-like DNS-type identifier")
+			_, err = acmeClient.AuthorizeOrder(testCtx, []acme.AuthzID{{Type: "dns", Value: "*.127.0.0.1"}},
+				acme.WithOrderNotAfter(time.Now().Add(10*time.Minute)))
+			require.Error(t, err, "should have rejected a new order with IP-like DNS-type identifier")
+
 			// Create an order
 			t.Logf("Testing Authorize Order on %s", baseAcmeURL)
-			createOrder, err := acmeClient.AuthorizeOrder(testCtx, []acme.AuthzID{{Type: "dns", Value: "localhost"}})
+			identifiers := []string{"localhost", "*.localhost"}
+			createOrder, err := acmeClient.AuthorizeOrder(testCtx, []acme.AuthzID{
+				{Type: "dns", Value: identifiers[0]},
+				{Type: "dns", Value: identifiers[1]},
+			})
 			require.NoError(t, err, "failed creating order")
 			require.Equal(t, acme.StatusPending, createOrder.Status)
 			require.Empty(t, createOrder.CertURL)
 			require.Equal(t, createOrder.URI+"/finalize", createOrder.FinalizeURL)
-			require.Len(t, createOrder.AuthzURLs, 1, "expected one authzurls")
+			require.Len(t, createOrder.AuthzURLs, 2, "expected two authzurls")

 			// Get order
 			t.Logf("Testing GetOrder on %s", baseAcmeURL)
@@ -144,12 +156,15 @@ func TestAcmeBasicWorkflow(t *testing.T) {
 			require.False(t, auth.Wildcard, "should not be a wildcard")
 			require.True(t, auth.Expires.IsZero(), "authorization should only have expiry set on valid status")

-			require.Len(t, auth.Challenges, 1, "expected one challenge")
+			require.Len(t, auth.Challenges, 2, "expected two challenges")
 			require.Equal(t, acme.StatusPending, auth.Challenges[0].Status)
 			require.True(t, auth.Challenges[0].Validated.IsZero(), "validated time should be 0 on challenge")
 			require.Equal(t, "http-01", auth.Challenges[0].Type)
-
 			require.NotEmpty(t, auth.Challenges[0].Token, "missing challenge token")
+			require.Equal(t, acme.StatusPending, auth.Challenges[1].Status)
+			require.True(t, auth.Challenges[1].Validated.IsZero(), "validated time should be 0 on challenge")
+			require.Equal(t, "dns-01", auth.Challenges[1].Type)
+			require.NotEmpty(t, auth.Challenges[1].Token, "missing challenge token")

 			// Load a challenge directly; this triggers validation to start.
 			challenge, err := acmeClient.GetChallenge(testCtx, auth.Challenges[0].URI)
@@ -164,34 +179,38 @@ func TestAcmeBasicWorkflow(t *testing.T) {
 			//       test.
 			pkiMount := findStorageMountUuid(t, client, "pki")
 			accountId := acct.URI[strings.LastIndex(acct.URI, "/"):]
-			authId := auth.URI[strings.LastIndex(auth.URI, "/"):]
+			for _, authURI := range getOrder.AuthzURLs {
+				authId := authURI[strings.LastIndex(authURI, "/"):]

-			rawPath := path.Join("/sys/raw/logical/", pkiMount, getAuthorizationPath(accountId, authId))
-			resp, err := client.Logical().ReadWithContext(testCtx, rawPath)
-			require.NoError(t, err, "failed looking up authorization storage")
-			require.NotNil(t, resp, "sys raw response was nil")
-			require.NotEmpty(t, resp.Data["value"], "no value field in sys raw response")
+				rawPath := path.Join("/sys/raw/logical/", pkiMount, getAuthorizationPath(accountId, authId))
+				resp, err := client.Logical().ReadWithContext(testCtx, rawPath)
+				require.NoError(t, err, "failed looking up authorization storage")
+				require.NotNil(t, resp, "sys raw response was nil")
+				require.NotEmpty(t, resp.Data["value"], "no value field in sys raw response")

-			var authz ACMEAuthorization
-			err = jsonutil.DecodeJSON([]byte(resp.Data["value"].(string)), &authz)
-			require.NoError(t, err, "error decoding authorization: %w", err)
-			authz.Status = ACMEAuthorizationValid
-			for _, challenge := range authz.Challenges {
-				challenge.Status = ACMEChallengeValid
+				var authz ACMEAuthorization
+				err = jsonutil.DecodeJSON([]byte(resp.Data["value"].(string)), &authz)
+				require.NoError(t, err, "error decoding authorization: %w", err)
+				authz.Status = ACMEAuthorizationValid
+				for _, challenge := range authz.Challenges {
+					challenge.Status = ACMEChallengeValid
+				}
+
+				encodeJSON, err := jsonutil.EncodeJSON(authz)
+				require.NoError(t, err, "failed encoding authz json")
+				_, err = client.Logical().WriteWithContext(testCtx, rawPath, map[string]interface{}{
+					"value":    base64.StdEncoding.EncodeToString(encodeJSON),
+					"encoding": "base64",
+				})
+				require.NoError(t, err, "failed writing authorization storage")
 			}

-			encodeJSON, err := jsonutil.EncodeJSON(authz)
-			require.NoError(t, err, "failed encoding authz json")
-			_, err = client.Logical().WriteWithContext(testCtx, rawPath, map[string]interface{}{
-				"value":    base64.StdEncoding.EncodeToString(encodeJSON),
-				"encoding": "base64",
-			})
-			require.NoError(t, err, "failed writing authorization storage")
-
 			// Make sure sending a CSR with the account key gets rejected.
 			goodCr := &x509.CertificateRequest{
-				Subject: pkix.Name{CommonName: createOrder.Identifiers[0].Value},
+				Subject:  pkix.Name{CommonName: identifiers[1]},
+				DNSNames: []string{identifiers[0], identifiers[1]},
 			}
+			t.Logf("csr: %v", goodCr)

 			// We want to make sure people are not using the same keys for CSR/Certs and their ACME account.
 			csrSignedWithAccountKey, err := x509.CreateCertificateRequest(rand.Reader, goodCr, accountKey)
@@ -214,6 +233,17 @@ func TestAcmeBasicWorkflow(t *testing.T) {
 			_, _, err = acmeClient.CreateOrderCert(testCtx, createOrder.FinalizeURL, csrWithBadName, true)
 			require.Error(t, err, "should not be allowed to csr with different names than order")

+			// Validate we reject CSRs that contains fewer names than in the original order.
+			badCr = &x509.CertificateRequest{
+				Subject: pkix.Name{CommonName: identifiers[0]},
+			}
+
+			csrWithBadName, err = x509.CreateCertificateRequest(rand.Reader, badCr, csrKey)
+			require.NoError(t, err, "failed generating csr with bad name")
+
+			_, _, err = acmeClient.CreateOrderCert(testCtx, createOrder.FinalizeURL, csrWithBadName, true)
+			require.Error(t, err, "should not be allowed to csr with different names than order")
+
 			// Finally test a proper CSR, with the correct name and signed with a different key works.
 			csr, err := x509.CreateCertificateRequest(rand.Reader, goodCr, csrKey)
 			require.NoError(t, err, "failed generating csr")