feat: add ACME "dns-account-01" challenge #7387

sheurich · 2024-03-16T20:09:03Z

Description:

This pull request addresses #7240 by integrating the "dns-account-01" challenge into Boulder. This challenge introduces a novel method for domain control validation within the ACME protocol.

Background:

The "dns-account-01" challenge, in its current Internet Draft form, introduces an additional approach for domain control validation. It uses a DNS resource linked to the ACME Account Resource URL and the authorization scope, offering enhanced flexibility and security in domain validation processes.

Changes:

In va/dns.go, the getDNSAccountChallengeSubdomain function has been introduced to compute the DNS subdomain for DNSAccount01 challenges based on the account's resource URL and scope.
The validateTXT function has been added to query TXT records associated with a challenge subdomain and validate the authorization keys.
The existing validateDNS01 function continues to validate DNS01 challenges but now leverages validateTXT for validation.
The new validateDNSAccount01 function constructs the challenge subdomain using getDNSAccountChallengeSubdomain and validates the authorization keys for DNSAccount01 challenges.
va/va_test.go has been updated to test the validation of malformed challenges with the new scope parameter.
Integration test TestDNSAccountChallenge has been added to validate the end-to-end functionality of DNSAccount01 challenges.

Vendor Dependency Updates:

Updated github.com/eggsampler/acme/v3 to version v3.5.0.

These changes enhance the security and flexibility of Boulder's validation process for DNS-based challenges, particularly with the introduction of support for the DNSAccount01 challenge type, thereby improving the overall robustness of the system.

- add dns-account-01 to ChallType maps - derive accountURI from accountID using va.accountURIPrefixes

- add TestDNSAccountValidationEmpty - use switch in bdns.LookupTXT mock

aaomidi · 2024-03-18T14:25:27Z

va/dns.go

+	// Iterate over the account URIs to find the correct one
+	// for the accountID and track any errors
+	var validationError error
+	for _, accountURIPrefix := range va.accountURIPrefixes {


This differs from the latest draft. The recommendation is to use the KID from the JWS message:

https://aaomidi.github.io/draft-ietf-acme-scoped-dns-challenges/#section-4-8.2

Compute the validation domain name with the KID value in the JWS message

Of course Boulder should be validating that KID value in the JWS message before it's used.

This would remove the need for this for loop too.

Although let's probably focus on getting this one done first? letsencrypt/pebble#435

I believe that's the desire from the Let's Encrypt team too?

The kid from the JWS is parsed in the WFE (as accountURL). The accountID (aka regid) portion of the URL is retrieved from the kid and stored before discarding the remainder of the URL.

accountURL := header.KeyID accountID, prob := wfe.acctIDFromURL(accountURL, request)

The accountID portion of the accountURL is properly authenticated information, since the entire JWS is validated first and accountID is extracted from the protected header.

As the remainder of the kid (the accountURL prefix) is not passed along from the WFE, we need to reconstruct this in the VA. The existing method used for CAA account binding is to specify the accountURL prefix in a configuration file. The loop is used to allow for migrations (e.g. "old" and "new" prefixes).

IF the kid value is required verbatim (versus the reconstituted version currently used), the Boulder changes would be more substantial in order to convey the additional data to the VA.

I see. It's good that the accountID prefix is coming from the JWS.

The current method would technically create unwanted behavior. It would have the server search potentially more than 1 DNS record for the resource, which is an unintended side effect and could potentially be considered non-compliant behavior.

I think this might warrant the extra changes necessary to pass those through here?

The configurable prefixes for accountURLs also provide a degree of administrative control over the allowed host values in the kid.

Another approach would be to attach the AccountKeyID to the Authz when the WFE sends to the RA. This would flow through to the RA->VA as well and be available for validation.

This last approach doesn't match the Challenge object description

// Challenge is an aggregate of all data needed for any challenges. // // Rather than define individual types for different types of // challenge, we just throw all the elements into one bucket, // together with the common metadata elements.

by putting the KID in the Authorization object. However it seems to fit more naturally as being scoped to the entire Authorization.

As there is discussion on IETF ACME working group about how to tell witch parental scope client want to use: like to challagne on example.com for get a cert for foo.example.com, I think we'll need plumbing from WFE to VA anyway, if we make pipe one can add everything needed from it from accounturi client sent to what scope clinet want to use
https://mailarchive.ietf.org/arch/msg/acme/HiD1xNZD7b4kLgHmYAsmVuT0HVA/

Right. I was thinking about moving the Scope attribute from the Challenge to the Authorization as well for the same reason.

In the latest change, the Challenge.AccountURL is populated in the WFE with the JWS kid and passed via the RA to the VA. The Authorization.Scope is currently unused (and removed before sending to clients) but can be used for passing scope to/from client.

The scope decision currently rests with the VA, but this could move to RA and be potentially modified by the client requested scope.

jsha · 2024-03-18T21:19:21Z

policy/pa.go

 		if !pa.ChallengeTypeEnabled(core.ChallengeTypeDNS01) {
-			return nil, fmt.Errorf(
-				"Challenges requested for wildcard identifier but DNS-01 " +
-					"challenge type is not enabled")
+			if !pa.ChallengeTypeEnabled(core.ChallengeTypeDNSAccount01) {


It seems like we can clean up this logic a little by turning it around:

append dns-01 if enabled

append dns-account-01 if enabled

return error if list is empty

jsha · 2024-03-18T21:23:09Z

policy/pa_test.go

+	// Enable DNS-ACCOUNT-01 and HTTP-01. It should not error and
+	// should return only one DNS-ACCOUNT-01 type challenge


Seems like we should also have a test case where both dns-01 and dns-account-01 are enabled?

aarongable · 2024-03-18T23:39:39Z

va/va.go

@@ -455,7 +460,7 @@ func (va *ValidationAuthorityImpl) validate(
 	return validationRecords, nil
 }

-func (va *ValidationAuthorityImpl) validateChallenge(ctx context.Context, identifier identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, error) {
+func (va *ValidationAuthorityImpl) validateChallenge(ctx context.Context, identifier identifier.ACMEIdentifier, challenge core.Challenge, scope core.AuthorizationScope, regid int64) ([]core.ValidationRecord, error) {


Given this comment:

boulder/core/objects.go

Lines 176 to 178 in 5e68cbe

// Rather than define individual types for different types of

// challenge, we just throw all the elements into one bucket,

// together with the common metadata elements.

I think it may make more sense the "scope" to be part of the core.Challenge object, rather than passed into this function separately.

if one is willing to expend and make change to GRPC I think it'd better add regid to core.challenge too: it can hydrated from auth object in sa/model.go

Done (scope).

The regid could get bundled in the Challenge with the current method but this won't be necessary if the kid is passed through from the WFE.

Oh right, regid is still needed. Makes sense to add it as well.

sa/model.go

aarongable · 2024-03-19T00:10:26Z

va/dns.go

+	// Iterate over the account URIs to find the correct one
+	// for the accountID and track any errors
+	var validationError error
+	for _, accountURIPrefix := range va.accountURIPrefixes {


Boulder currently configures two ACME account URI prefixes: the one from when we were still running acme-v01, and the current acme-v02 prefix. There are still very old, very stable ACME clients which present an "https://acme-v01.api.letsencrypt.org" KID in their JWS headers, and removing that option would break those clients seemingly unnecessarily.

That said, I agree that it's suboptimal for the ACME server to make multiple queries. And if the draft continues forward with the current language, it would be non-compliant to do so: it would be possible for a client to use the acme-v01 KID in their request JWS, and then for us to confirm the presence of a DNS record computed from an acme-v02-prefixed KID.

So one path forward that we're considering (and that @jsha and I discussed earlier today) is this: only allow a single accountURIPrefix (which would be the acme-v02 one) to be configured for the purposes of this challenge. When we receive a "please validate this challenge for me" request, confirm that the KID in the request also has that same singular prefix; if not, reject the request.

But teaching the WFE about one blessed account prefix and giving it extra challenge-specific logic doesn't sound that much simpler than simply plumbing the request's KID through into the PerformValidation request. So I'm personally also considering that path forward.

One other path forward would be to update the draft: one possibility would be to update the draft to say that the validation domain should be computed from the value returned by the Location: header in the Server's response to a new-acct onlyReturnExisting request.

- Add AccountURL to core.Challenge and protobufs - Fix test cases and add more

…postChallenge

sheurich · 2024-04-03T01:00:09Z

Hey friends, I think the bulk of the work here is complete for the moment. I am beginning a thru-hike of the Appalachian Trail and will not be available to work on this for the time being. If there are any adjustments needed due to spec changes, bugs or nits, someone else will need to make those changes before merging.
Best, Shiloh

sheurich added 12 commits March 15, 2024 18:04

Add DNS-ACCOUNT-01 support.

d341dc5

- add dns-account-01 to ChallType maps - derive accountURI from accountID using va.accountURIPrefixes

Initial tests for DNS-ACCOUNT-01.

1c0653f

- add TestDNSAccountValidationEmpty - use switch in bdns.LookupTXT mock

Upgrade eggsampler/acme/v3 to v3.5.0 for DNS-ACCOUNT-01 support

72c4b09

test(integration): add TestDNSAccountChallenge

bcec8c4

fix(validateDNSAccount01): reject unsupported scopes

4489f43

test(dns-account-01): additional unit tests

14ea785

test(pa): add dns-account-01 to wildcard challenges

22d2210

test(dns-account-01): add to core and ra unit tests

0718a30

feat(load-generator): add DNS-ACCOUNT-01 challenge strategy

ece427a

feat(ra): Allow DNS-ACCOUNT-01 authorizations to be reused for wildcards

e0fce1b

rename

eb7c094

fix(TestDNSAccountChallenge): only run test in config-next

a948e01

sheurich marked this pull request as ready for review March 16, 2024 20:29

sheurich requested a review from a team as a code owner March 16, 2024 20:29

sheurich requested a review from jsha March 16, 2024 20:29

aaomidi suggested changes Mar 18, 2024

View reviewed changes

aaomidi mentioned this pull request Mar 18, 2024

Add "dns-account-01" support from draft-ietf-acme-scoped-dns-challenges letsencrypt/pebble#435

Merged

Merge branch 'main' into add-dns-account-01

9e5e388

jsha reviewed Mar 18, 2024

View reviewed changes

sheurich added 2 commits March 18, 2024 18:18

clean up challenge policy logic

492ce0f

fix(test): add dns-01+dns-account-01 wildcard policy test

4ea3634

aarongable reviewed Mar 19, 2024

View reviewed changes

sheurich and others added 7 commits March 19, 2024 09:41

doc(model): explain challTypeToUint and uintToChallType

b2f90f7

bundle authz Scope into core.Challenge

ac488a5

Merge branch 'main' into add-dns-account-01

dcc624e

Merge branch 'main' into add-dns-account-01

4f7a8a1

Merge branch 'main' into add-dns-account-01

18d89dc

- Add AccountURL and Scope to core.Authorization and protobufs

a6f87c4

- Add AccountURL to core.Challenge and protobufs - Fix test cases and add more

lint

ab01878

sheurich added 4 commits March 22, 2024 19:30

fix(akamai-purger): deep copy batch of purge entries

528f6de

remove AccountURL from Authorization; pass using Challenge instead

67cfe74

fix: use jws protected header for KeyID

d2fe3e0

fix: use challenge pointer for setting AccountURL in WebFrontEndImpl.…

c941420

…postChallenge

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ACME "dns-account-01" challenge #7387

feat: add ACME "dns-account-01" challenge #7387

sheurich commented Mar 16, 2024

aaomidi Mar 18, 2024

aaomidi Mar 18, 2024

sheurich Mar 18, 2024

aaomidi Mar 18, 2024 •

edited

sheurich Mar 18, 2024

sheurich Mar 21, 2024

sheurich Mar 21, 2024

orangepizza Mar 21, 2024

sheurich Mar 21, 2024

sheurich Mar 22, 2024 •

edited

jsha Mar 18, 2024

sheurich Mar 19, 2024

jsha Mar 18, 2024

sheurich Mar 19, 2024

aarongable Mar 18, 2024

orangepizza Mar 19, 2024

sheurich Mar 19, 2024

sheurich Mar 22, 2024

aarongable Mar 19, 2024

sheurich commented Apr 3, 2024

		// Enable DNS-ACCOUNT-01 and HTTP-01. It should not error and
		// should return only one DNS-ACCOUNT-01 type challenge

	// Rather than define individual types for different types of
	// challenge, we just throw all the elements into one bucket,
	// together with the common metadata elements.

feat: add ACME "dns-account-01" challenge #7387

Are you sure you want to change the base?

feat: add ACME "dns-account-01" challenge #7387

Conversation

sheurich commented Mar 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaomidi Mar 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheurich Mar 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheurich commented Apr 3, 2024

aaomidi Mar 18, 2024 •

edited

sheurich Mar 22, 2024 •

edited