diff --git a/challenge/dnspersist01/dns_persist_challenge.go b/challenge/dnspersist01/dns_persist_challenge.go index 71871c39c..0f7f681e8 100644 --- a/challenge/dnspersist01/dns_persist_challenge.go +++ b/challenge/dnspersist01/dns_persist_challenge.go @@ -40,7 +40,7 @@ type ChallengeInfo struct { // `_validation-persist.[domain].`). FQDN string - // Value contains the TXT record issue-value. + // Value contains the TXT record value, an RFC 8659 issue-value. Value string // IssuerDomainName is the normalized issuer-domain-name used in Value. @@ -301,26 +301,22 @@ func GetAuthorizationDomainName(domain string) string { } // GetChallengeInfo returns information used to create a DNS TXT record which -// can fulfill the `dns-persist-01` challenge for a selected issuer. Domain, -// issuerDomainName, and accountURI parameters are required. Wildcard and -// persistUntil parameters are optional. +// can fulfill the `dns-persist-01` challenge. Domain, issuerDomainName, and +// accountURI parameters are required. Wildcard and persistUntil parameters are +// optional. func GetChallengeInfo(domain, issuerDomainName, accountURI string, wildcard bool, persistUntil *time.Time) (ChallengeInfo, error) { if domain == "" { return ChallengeInfo{}, errors.New("dnspersist01: domain cannot be empty") } - if accountURI == "" { - return ChallengeInfo{}, errors.New("dnspersist01: ACME account URI cannot be empty") - } - - err := validateIssuerDomainName(issuerDomainName) + value, err := BuildIssueValue(issuerDomainName, accountURI, wildcard, persistUntil) if err != nil { - return ChallengeInfo{}, fmt.Errorf("dnspersist01: %w", err) + return ChallengeInfo{}, err } return ChallengeInfo{ FQDN: GetAuthorizationDomainName(domain), - Value: BuildIssueValues(issuerDomainName, accountURI, wildcard, persistUntil), + Value: value, IssuerDomainName: issuerDomainName, }, nil } @@ -394,7 +390,7 @@ func (c *Challenge) selectIssuerDomainName(challIssuers []string, records []TXTR func (c *Challenge) hasMatchingRecord(records []TXTRecord, issuerDomainName string, wildcard bool) bool { for _, record := range records { - parsed, err := ParseIssueValues(record.Value) + parsed, err := ParseIssueValue(record.Value) if err != nil { continue } diff --git a/challenge/dnspersist01/dns_persist_challenge_test.go b/challenge/dnspersist01/dns_persist_challenge_test.go index 9e5e5b583..4a31fb436 100644 --- a/challenge/dnspersist01/dns_persist_challenge_test.go +++ b/challenge/dnspersist01/dns_persist_challenge_test.go @@ -264,7 +264,7 @@ func TestChallenge_selectIssuerDomainName(t *testing.T) { "ca.example", "backup.example", }, records: []TXTRecord{ - {Value: BuildIssueValues("ca.example", "https://authority.example/acct/123", false, nil)}, + {Value: mustChallengeValue(t, "ca.example", "https://authority.example/acct/123", false, nil)}, }, expectIssuerDomainName: "ca.example", }, @@ -274,7 +274,7 @@ func TestChallenge_selectIssuerDomainName(t *testing.T) { "ca.example", "backup.example", }, records: []TXTRecord{ - {Value: BuildIssueValues("ca.example", "https://authority.example/acct/123", false, nil)}, + {Value: mustChallengeValue(t, "ca.example", "https://authority.example/acct/123", false, nil)}, }, overrideIssuerDomainName: "backup.example", expectIssuerDomainName: "backup.example", @@ -320,39 +320,39 @@ func TestChallenge_hasMatchingRecord(t *testing.T) { }{ { desc: "match basic", - records: []TXTRecord{{Value: BuildIssueValues("ca.example", "acc", false, nil)}}, + records: []TXTRecord{{Value: mustChallengeValue(t, "ca.example", "acc", false, nil)}}, issuer: "ca.example", expect: true, }, { desc: "issuer mismatch", - records: []TXTRecord{{Value: BuildIssueValues("other.example", "acc", false, nil)}}, + records: []TXTRecord{{Value: mustChallengeValue(t, "other.example", "acc", false, nil)}}, issuer: "ca.example", expect: false, }, { desc: "account mismatch", - records: []TXTRecord{{Value: BuildIssueValues("ca.example", "other", false, nil)}}, + records: []TXTRecord{{Value: mustChallengeValue(t, "ca.example", "other", false, nil)}}, issuer: "ca.example", expect: false, }, { desc: "wildcard requires policy", - records: []TXTRecord{{Value: BuildIssueValues("ca.example", "acc", false, nil)}}, + records: []TXTRecord{{Value: mustChallengeValue(t, "ca.example", "acc", false, nil)}}, issuer: "ca.example", wildcard: true, expect: false, }, { desc: "wildcard match", - records: []TXTRecord{{Value: BuildIssueValues("ca.example", "acc", true, nil)}}, + records: []TXTRecord{{Value: mustChallengeValue(t, "ca.example", "acc", true, nil)}}, issuer: "ca.example", wildcard: true, expect: true, }, { desc: "policy wildcard allowed for non-wildcard", - records: []TXTRecord{{Value: BuildIssueValues("ca.example", "acc", true, nil)}}, + records: []TXTRecord{{Value: mustChallengeValue(t, "ca.example", "acc", true, nil)}}, issuer: "ca.example", wildcard: false, expect: true, @@ -382,13 +382,13 @@ func TestChallenge_hasMatchingRecord(t *testing.T) { }, { desc: "persistUntil present without requirement is not a match", - records: []TXTRecord{{Value: BuildIssueValues("ca.example", "acc", false, &expiredPersistUntil)}}, + records: []TXTRecord{{Value: mustChallengeValue(t, "ca.example", "acc", false, &expiredPersistUntil)}}, issuer: "ca.example", expect: false, }, { desc: "future persistUntil without requirement is not a match", - records: []TXTRecord{{Value: BuildIssueValues("ca.example", "acc", false, &futurePersistUntil)}}, + records: []TXTRecord{{Value: mustChallengeValue(t, "ca.example", "acc", false, &futurePersistUntil)}}, issuer: "ca.example", expect: false, }, @@ -401,7 +401,7 @@ func TestChallenge_hasMatchingRecord(t *testing.T) { }, { desc: "required persistUntil matches even when expired", - records: []TXTRecord{{Value: BuildIssueValues("ca.example", "acc", false, &expiredPersistUntil)}}, + records: []TXTRecord{{Value: mustChallengeValue(t, "ca.example", "acc", false, &expiredPersistUntil)}}, issuer: "ca.example", requiredPersistUTC: Pointer(expiredPersistUntil), expect: true, @@ -436,3 +436,12 @@ func TestChallenge_hasMatchingRecord(t *testing.T) { // TODO(ldez) factorize. func Pointer[T any](v T) *T { return &v } + +func mustChallengeValue(t *testing.T, issuerDomainName, accountURI string, wildcard bool, persistUntil *time.Time) string { + t.Helper() + + info, err := GetChallengeInfo("example.com", issuerDomainName, accountURI, wildcard, persistUntil) + require.NoError(t, err) + + return info.Value +} diff --git a/challenge/dnspersist01/issue_values.go b/challenge/dnspersist01/issue_values.go index dd9ff0173..4106992a6 100644 --- a/challenge/dnspersist01/issue_values.go +++ b/challenge/dnspersist01/issue_values.go @@ -21,40 +21,48 @@ type IssueValue struct { AccountURI string Policy string PersistUntil *time.Time - Params map[string]string } -// BuildIssueValues constructs an issue-value string for dns-persist-01 and -// optionally includes the persistUntil parameter. -func BuildIssueValues(issuerDomainName, accountURI string, wildcard bool, persistUntil *time.Time) string { - parts := []string{issuerDomainName} - - if accountURI != "" { - parts = append(parts, fmt.Sprintf("%s=%s", paramAccountURI, accountURI)) +// BuildIssueValue constructs an RFC 8659 issue-value for a dns-persist-01 TXT +// record. issuerDomainName and accountURI are required. wildcard and +// persistUntil are optional. +func BuildIssueValue(issuerDomainName, accountURI string, wildcard bool, persistUntil *time.Time) (string, error) { + if accountURI == "" { + return "", errors.New("dnspersist01: ACME account URI cannot be empty") } + err := validateIssuerDomainName(issuerDomainName) + if err != nil { + return "", fmt.Errorf("dnspersist01: %w", err) + } + + value := issuerDomainName + "; " + paramAccountURI + "=" + accountURI + if wildcard { - parts = append(parts, fmt.Sprintf("%s=%s", paramPolicy, policyWildcard)) + value += "; " + paramPolicy + "=" + policyWildcard } if persistUntil != nil { - parts = append(parts, fmt.Sprintf("persistUntil=%d", persistUntil.UTC().Unix())) + value += fmt.Sprintf("; persistUntil=%d", persistUntil.UTC().Unix()) } - return strings.Join(parts, "; ") + return value, nil } +// trimWSP trims RFC 5234 WSP (SP / HTAB) characters from both ends of a +// string, as referenced by RFC 8659. func trimWSP(s string) string { return strings.TrimFunc(s, func(r rune) bool { return r == ' ' || r == '\t' }) } -// ParseIssueValues parses an issue-value string. Unknown parameters are -// preserved in Params. +// ParseIssueValue parses the issuer-domain-name and parameters for an RFC +// 8659 issue-value TXT record and returns the extracted fields. It returns +// an error if any portion of the value is malformed. // -//nolint:gocyclo // parsing and validating tagged parameters requires branching per field. -func ParseIssueValues(value string) (IssueValue, error) { +//nolint:gocyclo // parsing and validating tagged parameters requires branching +func ParseIssueValue(value string) (IssueValue, error) { fields := strings.Split(value, ";") issuerDomainName := trimWSP(fields[0]) @@ -64,9 +72,9 @@ func ParseIssueValues(value string) (IssueValue, error) { parsed := IssueValue{ IssuerDomainName: issuerDomainName, - Params: map[string]string{}, } + // Parse parameters (with optional surrounding WSP). seenTags := map[string]bool{} for _, raw := range fields[1:] { @@ -75,6 +83,7 @@ func ParseIssueValues(value string) (IssueValue, error) { return IssueValue{}, errors.New("empty parameter or trailing semicolon provided") } + // Capture each tag=value pair. tagValue := strings.SplitN(part, "=", 2) if len(tagValue) != 2 { return IssueValue{}, fmt.Errorf("malformed parameter %q should be tag=value pair", part) @@ -87,13 +96,13 @@ func ParseIssueValues(value string) (IssueValue, error) { return IssueValue{}, fmt.Errorf("malformed parameter %q, empty tag", part) } - key := strings.ToLower(tag) - if seenTags[key] { + canonicalTag := strings.ToLower(tag) + if seenTags[canonicalTag] { return IssueValue{}, fmt.Errorf("duplicate parameter %q", tag) } - seenTags[key] = true - + seenTags[canonicalTag] = true + // Ensure values contain no whitespace/control/non-ASCII characters. for _, r := range val { if (r >= 0x21 && r <= 0x3A) || (r >= 0x3C && r <= 0x7E) { continue @@ -102,7 +111,10 @@ func ParseIssueValues(value string) (IssueValue, error) { return IssueValue{}, fmt.Errorf("malformed value %q for tag %q", val, tag) } - switch key { + // Finally, capture expected tag values. + // + // Note: according to RFC 8659 matching of tags is case insensitive. + switch canonicalTag { case paramAccountURI: if val == "" { return IssueValue{}, errors.New("empty value provided for mandatory accounturi") @@ -110,7 +122,13 @@ func ParseIssueValues(value string) (IssueValue, error) { parsed.AccountURI = val case paramPolicy: + // Per the dns-persist-01 specification, if the policy tag is + // present parameter's tag and defined values MUST be treated as + // case-insensitive. if val != "" && !strings.EqualFold(val, policyWildcard) { + // If the policy parameter's value is anything other than + // "wildcard", the a CA MUST proceed as if the policy parameter + // were not present. val = "" } @@ -124,7 +142,7 @@ func ParseIssueValues(value string) (IssueValue, error) { persistUntil := time.Unix(ts, 0).UTC() parsed.PersistUntil = &persistUntil default: - parsed.Params[key] = val + // Unknown parameters are permitted but not currently consumed. } } diff --git a/challenge/dnspersist01/issue_values_test.go b/challenge/dnspersist01/issue_values_test.go index 7aadef817..ca55974c1 100644 --- a/challenge/dnspersist01/issue_values_test.go +++ b/challenge/dnspersist01/issue_values_test.go @@ -8,14 +8,15 @@ import ( "github.com/stretchr/testify/require" ) -func TestBuildIssueValues(t *testing.T) { +func TestBuildIssueValue(t *testing.T) { testCases := []struct { - desc string - issuer string - accountURI string - wildcard bool - persistUTC *time.Time - expect string + desc string + issuer string + accountURI string + wildcard bool + persistUTC *time.Time + expect string + expectErrContains string }{ { desc: "basic", @@ -23,12 +24,6 @@ func TestBuildIssueValues(t *testing.T) { accountURI: "https://authority.example/acct/123", expect: "authority.example; accounturi=https://authority.example/acct/123", }, - { - desc: "no account", - issuer: "authority.example", - wildcard: true, - expect: "authority.example; policy=wildcard", - }, { desc: "with persistUntil", issuer: "authority.example", @@ -37,17 +32,36 @@ func TestBuildIssueValues(t *testing.T) { persistUTC: Pointer(time.Unix(4102444800, 0).UTC()), expect: "authority.example; accounturi=https://authority.example/acct/123; policy=wildcard; persistUntil=4102444800", }, + { + desc: "missing account uri", + issuer: "authority.example", + expectErrContains: "ACME account URI cannot be empty", + }, + { + desc: "invalid issuer", + issuer: "Authority.Example.", + accountURI: "https://authority.example/acct/123", + expectErrContains: "issuer-domain-name must be lowercase", + }, } for _, test := range testCases { t.Run(test.desc, func(t *testing.T) { - actual := BuildIssueValues(test.issuer, test.accountURI, test.wildcard, test.persistUTC) + actual, err := BuildIssueValue(test.issuer, test.accountURI, test.wildcard, test.persistUTC) + if test.expectErrContains != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), test.expectErrContains) + + return + } + + require.NoError(t, err) assert.Equal(t, test.expect, actual) }) } } -func TestParseIssueValues(t *testing.T) { +func TestParseIssueValue(t *testing.T) { testCases := []struct { desc string value string @@ -61,7 +75,6 @@ func TestParseIssueValues(t *testing.T) { expected: IssueValue{ IssuerDomainName: "authority.example", AccountURI: "https://authority.example/acct/123", - Params: map[string]string{}, }, }, { @@ -71,7 +84,6 @@ func TestParseIssueValues(t *testing.T) { IssuerDomainName: "authority.example", AccountURI: "https://authority.example/acct/123", Policy: "wIlDcArD", - Params: map[string]string{}, }, }, { @@ -80,7 +92,6 @@ func TestParseIssueValues(t *testing.T) { expected: IssueValue{ IssuerDomainName: "authority.example", AccountURI: "https://authority.example/acct/123", - Params: map[string]string{"extra": "value"}, }, }, { @@ -89,7 +100,6 @@ func TestParseIssueValues(t *testing.T) { expected: IssueValue{ IssuerDomainName: "authority.example", AccountURI: "https://authority.example/acct/123", - Params: map[string]string{"foo": ""}, }, }, { @@ -98,10 +108,6 @@ func TestParseIssueValues(t *testing.T) { expected: IssueValue{ IssuerDomainName: "authority.example", AccountURI: "https://authority.example/acct/123", - Params: map[string]string{ - "bad tag": "value", - "\nweird": "\\x01337", - }, }, }, { @@ -111,7 +117,6 @@ func TestParseIssueValues(t *testing.T) { IssuerDomainName: "authority.example", AccountURI: "https://authority.example/acct/123", Policy: "wildcard", - Params: map[string]string{}, }, expectedPersistUTC: Pointer(time.Unix(4102444800, 0).UTC()), }, @@ -121,7 +126,6 @@ func TestParseIssueValues(t *testing.T) { expected: IssueValue{ IssuerDomainName: "authority.example", AccountURI: "https://authority.example/acct/123", - Params: map[string]string{}, }, }, { @@ -129,7 +133,6 @@ func TestParseIssueValues(t *testing.T) { value: "authority.example", expected: IssueValue{ IssuerDomainName: "authority.example", - Params: map[string]string{}, }, }, { @@ -186,7 +189,7 @@ func TestParseIssueValues(t *testing.T) { for _, test := range testCases { t.Run(test.desc, func(t *testing.T) { - parsed, err := ParseIssueValues(test.value) + parsed, err := ParseIssueValue(test.value) if test.expectErrContains != "" { require.Error(t, err) assert.Contains(t, err.Error(), test.expectErrContains) diff --git a/e2e/dnschallenge/dns_persist_challenges_test.go b/e2e/dnschallenge/dns_persist_challenges_test.go index 65329c865..fbcee1b8f 100644 --- a/e2e/dnschallenge/dns_persist_challenges_test.go +++ b/e2e/dnschallenge/dns_persist_challenges_test.go @@ -83,6 +83,16 @@ func clearTXTRecord(t *testing.T, host string) { require.Equal(t, http.StatusOK, resp.StatusCode) } +//nolint:unparam // kept generic for future e2e tests. +func mustDNSPersistIssueValue(t *testing.T, issuerDomainName, accountURI string, wildcard bool, persistUntil *time.Time) string { + t.Helper() + + value, err := dnspersist01.BuildIssueValue(issuerDomainName, accountURI, wildcard, persistUntil) + require.NoError(t, err) + + return value +} + func createCLIAccountState(t *testing.T, email string) string { t.Helper() @@ -200,7 +210,7 @@ func TestChallengeDNSPersist_Client_Obtain(t *testing.T) { user.registration = reg txtHost := fmt.Sprintf("_validation-persist.%s", testPersistBaseDomain) - txtValue := dnspersist01.BuildIssueValues(testPersistIssuer, reg.URI, true, nil) + txtValue := mustDNSPersistIssueValue(t, testPersistIssuer, reg.URI, true, nil) setTXTRecord(t, txtHost, txtValue) defer clearTXTRecord(t, txtHost) @@ -245,7 +255,7 @@ func TestChallengeDNSPersist_Run(t *testing.T) { require.NotEmpty(t, accountURI) txtHost := fmt.Sprintf("_validation-persist.%s", testPersistCLIDomain) - txtValue := dnspersist01.BuildIssueValues(testPersistIssuer, accountURI, true, nil) + txtValue := mustDNSPersistIssueValue(t, testPersistIssuer, accountURI, true, nil) setTXTRecord(t, txtHost, txtValue) defer clearTXTRecord(t, txtHost) @@ -295,7 +305,7 @@ func TestChallengeDNSPersist_Run_NewAccount(t *testing.T) { return } - txtValue := dnspersist01.BuildIssueValues(testPersistIssuer, accountURI, true, nil) + txtValue := mustDNSPersistIssueValue(t, testPersistIssuer, accountURI, true, nil) err = setTXTRecordRaw(txtHost, txtValue) if err != nil { @@ -342,7 +352,7 @@ func TestChallengeDNSPersist_Renew(t *testing.T) { require.NotEmpty(t, accountURI) txtHost := fmt.Sprintf("_validation-persist.%s", testPersistCLIDomain) - txtValue := dnspersist01.BuildIssueValues(testPersistIssuer, accountURI, true, nil) + txtValue := mustDNSPersistIssueValue(t, testPersistIssuer, accountURI, true, nil) setTXTRecord(t, txtHost, txtValue) defer clearTXTRecord(t, txtHost)