Addressing comments

Author: beautifulentropy

cmd/boulder-ra/main.go

@@ -172,11 +172,11 @@ func main() {
 	pa, err := policy.New(c.PA.Identifiers, c.PA.Challenges, logger)
 	cmd.FailOnError(err, "Couldn't create PA")
 
-	if features.Get().DNSPersist01Enabled && !pa.ChallengeTypeEnabled(core.ChallengeTypeDNSPersist01) {
-		cmd.Fail("Feature flag DNSPersist01Enabled requires dns-persist-01 to be enabled in the PA")
+	if features.Get().DNSAccount01Enabled != pa.ChallengeTypeEnabled(core.ChallengeTypeDNSAccount01) {
+		cmd.Fail("Feature flag DNSAccount01Enabled and PA dns-account-01 challenge must both be enabled or disabled")
 	}
-	if pa.ChallengeTypeEnabled(core.ChallengeTypeDNSPersist01) && !features.Get().DNSAccount01Enabled {
-		cmd.Fail("Feature flag DNSAccount01Enabled must be enabled to use dns-persist-01 challenge type")
+	if features.Get().DNSPersist01Enabled != pa.ChallengeTypeEnabled(core.ChallengeTypeDNSPersist01) {
+		cmd.Fail("Feature flag DNSPersist01Enabled and PA dns-persist-01 challenge must both be enabled or disabled")
 	}
 
 	if c.RA.HostnamePolicyFile == "" {

core/util.go

@@ -27,6 +27,8 @@ import (
 	"unicode"
 
 	"github.com/go-jose/go-jose/v4"
+	"golang.org/x/net/idna"
+	"golang.org/x/text/unicode/norm"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/durationpb"
@@ -398,3 +400,22 @@ func IsCanceled(err error) bool {
 func Command() string {
 	return path.Base(os.Args[0])
 }
+
+// NormalizeIssuerDomainName normalizes an RFC 8659 issuer-domain-name per the
+// recommended algorithm in draft-ietf-acme-dns-persist-00, Section 9.1.1:
+// case-fold to lowercase, apply Unicode NFC normalization, convert to A-label
+// (Punycode), remove any trailing dot, and ensure the result is no more than
+// 253 octets in length. If normalization fails, an error is returned.
+func NormalizeIssuerDomainName(name string) (string, error) {
+	name = strings.ToLower(name)
+	name = norm.NFC.String(name)
+	name, err := idna.Lookup.ToASCII(name)
+	if err != nil {
+		return "", fmt.Errorf("converting issuer domain name %q to ASCII: %w", name, err)
+	}
+	name = strings.TrimSuffix(name, ".")
+	if len(name) > 253 {
+		return "", fmt.Errorf("issuer domain name %q exceeds 253 octets (%d)", name, len(name))
+	}
+	return name, nil
+}

core/util_test.go

@@ -426,3 +426,54 @@ func TestIsCanceled(t *testing.T) {
 		t.Errorf("Expected random error to not be canceled, but was.")
 	}
 }
+
+func TestNormalizeIssuerDomainName(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name      string
+		input     string
+		want      string
+		wantError bool
+	}{
+		{
+			name:  "Already normalized",
+			input: "ca.example",
+			want:  "ca.example",
+		},
+		{
+			name:  "Normalizes uppercase and trailing dot",
+			input: "CA.EXAMPLE.",
+			want:  "ca.example",
+		},
+		{
+			name:  "Normalizes IDNA issuer",
+			input: "BÜCHER.example",
+			want:  "xn--bcher-kva.example",
+		},
+		{
+			name:      "Rejects invalid issuer with underscore",
+			input:     "ca_.example",
+			wantError: true,
+		},
+		{
+			name:      "Rejects too-long issuer",
+			input:     strings.Repeat("a", 63) + "." + strings.Repeat("b", 63) + "." + strings.Repeat("c", 63) + "." + strings.Repeat("d", 63),
+			wantError: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			got, err := NormalizeIssuerDomainName(tc.input)
+			if tc.wantError {
+				test.AssertError(t, err, "expected normalization error")
+				return
+			}
+			test.AssertNotError(t, err, "unexpected normalization error")
+			test.AssertEquals(t, got, tc.want)
+		})
+	}
+}

ra/ra.go

@@ -2264,8 +2264,8 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New
 		}
 
 		// If the identifier is a wildcard DNS name, all challenges must be
-		// DNS-01 or DNS-Account-01. The PA guarantees this at order creation
-		// time, but we verify again to be safe.
+		// DNS-based. The PA guarantees this at order creation time, but we
+		// verify again to be safe.
 		if ident.Type == identifier.TypeDNS && strings.HasPrefix(ident.Value, "*.") {
 			for _, chall := range authz.Challenges {
 				if chall.Type != core.ChallengeTypeDNS01 &&
@@ -2284,9 +2284,15 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New
 		// the TXT record's TTL and BRs section 3.2.2.4.22 caps it at 10 days.
 		// Since TTLs are typically seconds to minutes, the TTL cap is likely to
 		// be the binding constraint; re-validating every order is simpler.
-		if slices.ContainsFunc(authz.Challenges, func(c core.Challenge) bool {
-			return c.Type == core.ChallengeTypeDNSPersist01 && c.Status == core.StatusValid
-		}) {
+		solvedBy, err := authz.SolvedBy()
+		if err != nil {
+			// This should never happen.
+			return nil, berrors.InternalServerError(
+				"SA.GetAuthorizations returned a DNS wildcard authz (%s) with invalid challenge(s)",
+				authz.ID,
+			)
+		}
+		if solvedBy == core.ChallengeTypeDNSPersist01 {
 			missingAuthzIdents = append(missingAuthzIdents, ident)
 			// Delete the authz from the identToExistingAuthz map since we are not reusing it.
 			delete(identToExistingAuthz, ident)

va/dns_persist.go

@@ -8,9 +8,6 @@ import (
 	"strings"
 	"time"
 
-	"golang.org/x/net/idna"
-	"golang.org/x/text/unicode/norm"
-
 	"github.com/letsencrypt/boulder/core"
 	berrors "github.com/letsencrypt/boulder/errors"
 	"github.com/letsencrypt/boulder/identifier"
@@ -20,51 +17,13 @@ type dnsPersistIssueValue struct {
 	issuerDomain string
 	accountURI   string
 	policy       string
-	persistUntil *time.Time
-}
-
-// NormalizeIssuerDomainName normalizes an RFC 8659 issuer-domain-name per
-// draft-ietf-acme-dns-persist-00, Section 9.1.1: case-fold to lowercase, apply
-// Unicode NFC normalization, convert to A-label (Punycode), remove any trailing
-// dot, and ensure the result is no more than 253 octets in length. If
-// normalization fails, an error is returned.
-func NormalizeIssuerDomainName(name string) (string, error) {
-	name = strings.ToLower(name)
-	name = norm.NFC.String(name)
-	name, err := idna.Lookup.ToASCII(name)
-	if err != nil {
-		return "", fmt.Errorf("converting issuer domain name %q to ASCII: %w", name, err)
-	}
-	name = strings.TrimSuffix(name, ".")
-	if len(name) > 253 {
-		return "", fmt.Errorf("issuer domain name %q exceeds 253 octets (%d)", name, len(name))
-	}
-	return name, nil
+	persistUntil time.Time
 }
 
-// trimWSP trims RFC 5234 WSP (SP / HTAB) characters, as referenced by RFC 8659,
-// from both ends of the input string.
-func trimWSP(input string) string {
-	return strings.TrimFunc(input, func(r rune) bool {
-		return r == ' ' || r == '\t'
-	})
-}
-
-// splitIssueValue splits and returns an RFC 8659 issue-value into
-// issuer-domain-name and raw parameter segments. If parsing fails, zero values
-// are returned.
-func splitIssueValue(raw string) (string, []string) {
-	// Split into issuer-domain-name and parameters.
-	parts := strings.Split(raw, ";")
-	if len(parts) == 0 {
-		return "", nil
-	}
-	// Parse issuer-domain-name.
-	issuerDomainName := trimWSP(parts[0])
-	if issuerDomainName == "" {
-		return "", nil
-	}
-	return issuerDomainName, parts[1:]
+// isWSP checks if a rune is an RFC 5234 WSP (SP / HTAB) character, as
+// referenced by RFC 8659.
+func isWSP(r rune) bool {
+	return r == '\t' || r == ' '
 }
 
 // parseDNSPersistIssueValue parses the raw parameter segments of an RFC 8659
@@ -77,7 +36,7 @@ func parseDNSPersistIssueValue(issuerDomainName string, paramsRaw []string) (*dn
 
 	for _, param := range paramsRaw {
 		// Clean optional WSP from the parameter.
-		param = trimWSP(param)
+		param = strings.TrimFunc(param, isWSP)
 		if param == "" {
 			return nil, errors.New("empty parameter or trailing semicolon provided")
 		}
@@ -87,8 +46,8 @@ func parseDNSPersistIssueValue(issuerDomainName string, paramsRaw []string) (*dn
 		if len(tagValue) != 2 {
 			return nil, fmt.Errorf("malformed parameter %q should be tag=value pair", param)
 		}
-		tag := trimWSP(tagValue[0])
-		value := trimWSP(tagValue[1])
+		tag := strings.TrimFunc(tagValue[0], isWSP)
+		value := strings.TrimFunc(tagValue[1], isWSP)
 		if tag == "" {
 			return nil, fmt.Errorf("malformed parameter %q, empty tag", param)
 		}
@@ -134,18 +93,73 @@ func parseDNSPersistIssueValue(issuerDomainName string, paramsRaw []string) (*dn
 			if err != nil {
 				return nil, fmt.Errorf("malformed persistUntil timestamp %q", value)
 			}
-			persistUntil := time.Unix(persistUntilVal, 0).UTC()
-			result.persistUntil = &persistUntil
+			result.persistUntil = time.Unix(persistUntilVal, 0).UTC()
 		}
 	}
 	return result, nil
 }
 
+// parseDNSPersistRecord parses a raw TXT record string into a
+// dnsPersistIssueValue. It returns ("", nil, nil) if the record's
+// issuer-domain-name is empty or does not match allowedIssuer.
+func parseDNSPersistRecord(record string, allowedIssuer string) (string, *dnsPersistIssueValue, error) {
+	// Split into issuer-domain-name and parameters per RFC 8659.
+	parts := strings.Split(record, ";")
+	receivedIssuer := strings.TrimFunc(parts[0], isWSP)
+	if receivedIssuer == "" {
+		return "", nil, nil
+	}
+
+	normalizedIssuer, err := core.NormalizeIssuerDomainName(receivedIssuer)
+	if err != nil || normalizedIssuer != allowedIssuer {
+		return "", nil, nil
+	}
+
+	params, err := parseDNSPersistIssueValue(receivedIssuer, parts[1:])
+	if err != nil {
+		return receivedIssuer, nil, err
+	}
+	return receivedIssuer, params, nil
+}
+
+// checkDNSPersistRecord checks whether a parsed dns-persist-01 record
+// authorizes issuance for the given account URI and wildcard status at the
+// given time. It returns nil if the record authorizes issuance, a
+// berrors.Malformed error for syntax problems, or a berrors.Unauthorized error
+// for authorization failures.
+func checkDNSPersistRecord(params *dnsPersistIssueValue, validAccountURI string, wildcardName bool, validatedAt time.Time) error {
+	if params.accountURI == "" {
+		return berrors.MalformedError("missing mandatory accountURI parameter")
+	}
+	if params.accountURI != validAccountURI {
+		return berrors.UnauthorizedError("accounturi mismatch: expected %q, got %q", validAccountURI, params.accountURI)
+	}
+	// Per draft-ietf-acme-dns-persist-00, the policy parameter's tag and
+	// defined values MUST be treated as case-insensitive. If the policy
+	// parameter's value is anything other than "wildcard", the CA MUST proceed
+	// as if the policy parameter were not present.
+	if wildcardName && strings.ToLower(params.policy) != "wildcard" {
+		return berrors.UnauthorizedError("policy mismatch: expected \"wildcard\", got %q", params.policy)
+	}
+	if !params.persistUntil.IsZero() && validatedAt.After(params.persistUntil) {
+		return berrors.UnauthorizedError("validation time %s is after persistUntil %s",
+			validatedAt.Format(time.RFC3339), params.persistUntil.Format(time.RFC3339))
+	}
+	return nil
+}
+
 func (va *ValidationAuthorityImpl) validateDNSPersist01(ctx context.Context, ident identifier.ACMEIdentifier, validAccountURI string, wildcardName bool) ([]core.ValidationRecord, error) {
 	if ident.Type != identifier.TypeDNS {
 		return nil, berrors.MalformedError("Identifier type for DNS-PERSIST-01 challenge was not DNS")
 	}
 
+	if va.issuerDomain == "" {
+		// Belt and suspenders check: the VA should not have been configured to
+		// perform DNS-PERSIST-01 validation if it does not have an issuer
+		// domain name configured for comparison.
+		return nil, berrors.InternalServerError("no issuer domain name configured for DNS-PERSIST-01 challenge validation")
+	}
+
 	challengeSubdomain := fmt.Sprintf("%s.%s", core.DNSPersistPrefix, ident.Value)
 	txts, resolver, err := va.dnsClient.LookupTXT(ctx, challengeSubdomain)
 	if err != nil {
@@ -156,59 +170,31 @@ func (va *ValidationAuthorityImpl) validateDNSPersist01(ctx context.Context, ide
 	}
 	validatedAt := va.clk.Now().UTC()
 
-	allowedIssuer := va.issuerDomain
-	if allowedIssuer == "" {
-		// Belt and suspenders check: the VA should not have been configured to
-		// perform DNS-PERSIST-01 validation if it does not have an issuer
-		// domain name configured for comparison.
-		return nil, berrors.InternalServerError("no issuer domain name configured for DNS-PERSIST-01 challenge validation")
-	}
-
 	var syntaxErrs []string
 	var authorizationErrs []string
 	for _, rr := range txts.Final {
 		record := strings.Join(rr.Txt, "")
-		receivedIssuer, paramsRaw := splitIssueValue(record)
-		normalizedIssuer, err := NormalizeIssuerDomainName(receivedIssuer)
-		if err != nil || normalizedIssuer != allowedIssuer {
-			continue
-		}
 
-		params, err := parseDNSPersistIssueValue(receivedIssuer, paramsRaw)
+		receivedIssuer, params, err := parseDNSPersistRecord(record, va.issuerDomain)
 		if err != nil {
-			// We know if this record was intended for us but it is malformed,
-			// we can continue checking other records but we should report the
-			// syntax error if no other record authorizes the challenge.
 			syntaxErrs = append(syntaxErrs, fmt.Sprintf(
 				"Parsing DNS-PERSIST-01 challenge TXT record with issuer-domain-name %q: %s", receivedIssuer, err))
 			continue
 		}
-		if params.accountURI == "" {
-			syntaxErrs = append(syntaxErrs, fmt.Sprintf(
-				"Parsing DNS-PERSIST-01 challenge TXT record with issuer-domain-name %q: missing mandatory accountURI parameter", receivedIssuer))
-			continue
-		}
-		if params.accountURI != validAccountURI {
-			authorizationErrs = append(authorizationErrs, fmt.Sprintf(
-				"Parsing DNS-PERSIST-01 challenge TXT record with issuer-domain-name %q: accounturi mismatch: expected %q, got %q",
-				receivedIssuer, validAccountURI, params.accountURI))
+		if params == nil {
+			// Record didn't match our issuer domain, skip.
 			continue
 		}
-		// Per draft-ietf-acme-dns-persist-00, the policy parameter's tag
-		// and defined values MUST be treated as case-insensitive. If the
-		// policy parameter's value is anything other than "wildcard", the
-		// CA MUST proceed as if the policy parameter were not present.
-		policyLower := strings.ToLower(params.policy)
-		if wildcardName && policyLower != "wildcard" {
-			authorizationErrs = append(authorizationErrs, fmt.Sprintf(
-				"Parsing DNS-PERSIST-01 challenge TXT record with issuer-domain-name %q: policy mismatch: expected \"wildcard\", got %q",
-				receivedIssuer, params.policy))
-			continue
-		}
-		if params.persistUntil != nil && validatedAt.After(*params.persistUntil) {
-			authorizationErrs = append(authorizationErrs, fmt.Sprintf(
-				"Parsing DNS-PERSIST-01 challenge TXT record with issuer-domain-name %q, validation time %s is after persistUntil %s",
-				receivedIssuer, validatedAt.Format(time.RFC3339), params.persistUntil.Format(time.RFC3339)))
+
+		err = checkDNSPersistRecord(params, validAccountURI, wildcardName, validatedAt)
+		if err != nil {
+			msg := fmt.Sprintf(
+				"Parsing DNS-PERSIST-01 challenge TXT record with issuer-domain-name %q: %s", receivedIssuer, err)
+			if errors.Is(err, berrors.Malformed) {
+				syntaxErrs = append(syntaxErrs, msg)
+			} else {
+				authorizationErrs = append(authorizationErrs, msg)
+			}
 			continue
 		}
 
@@ -224,6 +210,5 @@ func (va *ValidationAuthorityImpl) validateDNSPersist01(ctx context.Context, ide
 	if len(authorizationErrs) > 0 {
 		return nil, berrors.UnauthorizedError("%s", strings.Join(authorizationErrs, "; "))
 	}
-
 	return nil, berrors.UnauthorizedError("No valid TXT record found for DNS-PERSIST-01 challenge")
 }