-
-
Notifications
You must be signed in to change notification settings - Fork 609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add ACME "dns-account-01" challenge #7387
Conversation
- add dns-account-01 to ChallType maps - derive accountURI from accountID using va.accountURIPrefixes
- add TestDNSAccountValidationEmpty - use switch in bdns.LookupTXT mock
va/dns.go
Outdated
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configurable prefixes for accountURLs also provide a degree of administrative control over the allowed host values in the kid
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I was thinking about moving the Scope
attribute from the Challenge to the Authorization as well for the same reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// Enable DNS-ACCOUNT-01 and HTTP-01. It should not error and | ||
// should return only one DNS-ACCOUNT-01 type challenge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we should also have a test case where both dns-01 and dns-account-01 are enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
va/va.go
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this comment:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, regid
is still needed. Makes sense to add it as well.
va/dns.go
Outdated
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
As the acme-scoped-dns-challenges doc has presented no updates at recent IETF meetings, I'm going to close this. It can always be reopened or used as the starting point of a future PR if that work begins moving forward again. |
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:
va/dns.go
, thegetDNSAccountChallengeSubdomain
function has been introduced to compute the DNS subdomain forDNSAccount01
challenges based on the account's resource URL and scope.validateTXT
function has been added to query TXT records associated with a challenge subdomain and validate the authorization keys.validateDNS01
function continues to validateDNS01
challenges but now leveragesvalidateTXT
for validation.validateDNSAccount01
function constructs the challenge subdomain usinggetDNSAccountChallengeSubdomain
and validates the authorization keys forDNSAccount01
challenges.va/va_test.go
has been updated to test the validation of malformed challenges with the new scope parameter.TestDNSAccountChallenge
has been added to validate the end-to-end functionality ofDNSAccount01
challenges.Vendor Dependency Updates:
github.com/eggsampler/acme/v3
to versionv3.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.