feat(va, ra, sa): dns-account-01 validation logic and protocol updates #8149
Conversation
sheurich
force-pushed
the
feat-dns-account-01-mod-val
branch
2 times, most recently
from
1b542cc to
749bbf9
Compare
|
|
||
| // Assert the specific error type | ||
| prob := detailedError(err) | ||
| test.AssertEquals(t, prob.Type, probs.ConnectionProblem) |
There was a problem hiding this comment.
When accountURI is missing in validateDNSAccount01, it triggers an InternalServerError. The test TestDNSAccount01ValidationEmptyAccountURI verifies that the resulting client-facing error is mapped to ConnectionProblem (via the WFE mapping). Is ConnectionProblem the most suitable error type for the client, or should we consider a more specific internal error, such as MissingParameter, which could map to a different client-facing error (e.g., Malformed) if needed in the future? However, since the accountURI originates from the JWS, this error scenario should theoretically never occur.
There was a problem hiding this comment.
Note to fellow reviewers: when reviewing, ignore the first two commits in this PR. They are copied from #8140.
Similar to that other PR, I mostly have small editorial suggestions, and one big overarching comment. This time I've left that comment on va.proto; take a look.
va/dns.go
Outdated
| // Compute the digest of the key authorization file | ||
| h := sha256.New() | ||
| h.Write([]byte(keyAuthorization)) | ||
| authorizedKeysDigest := base64.RawURLEncoding.EncodeToString(h.Sum(nil)) |
There was a problem hiding this comment.
This code can also be shared inside the new validateDNS helper method.
| // TODO(#8023): dnsNames are being deprecated in favour of identifiers. | ||
| string dnsName = 1; | ||
| core.Identifier identifier = 5; | ||
| core.Challenge challenge = 2; | ||
| AuthzMeta authz = 3; | ||
| string expectedKeyAuthorization = 4; | ||
| string accountURI = 6; |
There was a problem hiding this comment.
This new parameter should not be necessary. Take a look at how we check for account URIs when doing CAA checking:
- the
IsCAAValidRequestcontains anaccountURIID, which is just an integer (this is the same value as is already held inPerformValidationRequest.AuthzMeta.regID) - the VA is configured with a set of acceptable
accountURIPrefixes(because we've had several account URI schemes over the years) - the CAA code then combines the regID with all the acceptable prefixes to see if the value contained in the CAA record matches
Obviously that's not quite identical the situation here -- now we need the full URL so that we can compute a hash from it. But I think with some simple modifications it could be made to work.
Beyond all that, it's not even clear to me where this URI would come from. You've already realized that the RA can't compute it from first principles, which is why you've added it to the RA's PerformValidationRequest too. But even the WFE doesn't have easy access to this URL: the currAcct object returned by validPOSTForAccount only contains its regID, not its full URL. So the WFE would either need to do some abstraction violation to extract the kid field from the JWS protected header, or the WFE would need to back-construct the URL from the regID, just like the VA can do.
So I'm currently of the opinion that it's better for the VA to construct this URL from information it already has -- the regID and the accountURIPrefixes -- than it is to pass this value all the way down from the WFE (which itself might compute a different value than the client expects anyway, if the client created its account long enough ago!). But I'm definitely open to being persuaded on this one.
There was a problem hiding this comment.
@aarongable Thanks for the review and the detailed comment!
You're right to draw the parallel with CAA checking and question the need for passing the full accountURI. For CAA, the VA only needs to verify if an externally provided URI (from the DNS record) matches any potential URI constructible from the regID and the configured accountURIPrefixes. It doesn't need to know the specific URI the client uses.
However, dns-account-01 has a different requirement, as you noted. The VA must construct the specific DNS label (_<hash>._acme-challenge...) that the client is expected to provision. This construction relies on hashing the exact account URI the client used to authenticate the request – which is the kid value from the JWS Protected Header.
The VA cannot reliably reconstruct this exact kid URI just from the regID and the list of possible accountURIPrefixes. If there are multiple valid prefixes (e.g., due to API versioning or historical reasons), the VA wouldn't know which one the client used for this request, leading to an incorrect hash and label. You correctly pointed out that if the VA tried multiple prefixes, it would lead to incorrect and unnecessary DNS queries.
Regarding the abstraction concern: While ideally the WFE might only work with the resolved currAcct object post-authentication, the dns-account-01 challenge specifically ties its mechanism to the exact kid string used in the JWS authentication. The WFE is the layer responsible for validating this JWS, including the kid. Accessing the verified kid from the protected header after successful validation is utilizing information that is inherently part of the WFE's responsibility domain (the validated authentication context for the request). It seems necessary here to fulfill the specific requirements of the challenge type, rather than being a true abstraction violation.
Therefore, passing the specific kid value down the chain seems necessary. The WFE is the component that processes the JWS and has direct access to the kid. My plan is for the WFE to extract the kid from the verified JWS Protected Header and pass it through the call chain: WFE -> RA -> VA.
My initial thought for the WFE was to reconstruct the URL, but as we've discussed, that's incorrect. The correct approach is to extract the kid directly from the validated JWS header in the WebFrontEndImpl.postChallenge, like this (conceptual):
body, outerJWS, currAcct, err := wfe.validPOSTForAccount(request, ctx, logEvent)
…
if features.Get().DNSAccount01Enabled {
challengeType := authz.Challenges[challengeIndex].Type
if challengeType == core.ChallengeTypeDNSAccount01 {
performValidationReq.AccountURI := outerJWS.Signatures[0].Header.KeyID
}
}Relatedly, I am wondering if we should use the accountURIPrefixes (perhaps passed down from WFE/RA or configured directly in VA) as an allow-list filter in the VA or RA? We could check if the kid extracted by the WFE starts with one of the expected prefixes before using it for the hash calculation. This seems like a potentially useful sanity/security check against unexpected kid values. What do you think?
There was a problem hiding this comment.
Even if the VA wasn't computing this URL and was instead receiving it from the RA/WFE, it shouldn't be extracted from the KID. The current Internet Draft says (emphasis added):
ACCOUNT_URL is defined in [RFC8555], Section 7.3 as the value in the Location header field
So the account URL being hashed is not supposed to be the client's idea of the account URL, it's supposed to be the server's idea of the account URL: the one that the server returns in the Location header. If the WFE were to be passing a URL to the RA and VA, it should be computing that URL itself using web.RelativeEndpoint.
But that also means that it's totally fine for the VA to directly compute the URL, as long as it computes the same URL that the WFE does. And that's what the existing AccountURIPrefixes config item allows the VA to do. We can just declare that the zeroth entry of that list has special meaning (or if you really want to be explicit, split the list into a singular CanonicalAccountURIPrefix and list of LegacyAccountURIPrefixes) and use that one for dns-account-01.
As a further mitigation, we can make sure that the error message in case of an NXDOMAIN failure during dns-account-01 contains the pre-hash account URL we were expecting to find, to help a client that is using the wrong account URL debug.
There was a problem hiding this comment.
RFC 8555 says the JWS kid is the value in the Location.
§7.2
The server returns this account object in a 201 (Created) response, with the account URL in a Location header field. The account URL is used as the "kid" value in the JWS authenticating subsequent requests by this account (see Section 6.2).
§6.2
For all other requests, the request is signed using an existing account, and there MUST be a "kid" field. This field MUST contain the account URL received by POSTing to the newAccount resource.
It seems like both approaches are viable; using the kid is more flexible e.g. handling changes of the account endpoint dynamically, while hardcoding the accountURIPrefix is simpler and less traffic over gRPC.
There was a problem hiding this comment.
The issue is that RFC 8555 doesn't say when the client should have retrieved the Location header for use in the kid -- some clients retrieve it once upon account creation, persist it on disk as the unique identifier for that account key, and then continue to use that URL until they're uninstalled. So since we can't trust the kid to be up-to-date, we need to use the server's idea of what the account URL should be.
It's true that this means there's a very small chance that some clients won't be able to complete dns-account-01. But I expect that population to be so small as to be nonexistent: it's the intersection of a) clients that persist the account URL upon new account creation and never update it; b) accounts that were created prior to 2018 when old-style account URLs were in use; and c) clients that have been updated to support dns-account-01 and reconfigured by their operators to actually use it. I am not concerned about this hyper-niche population. This is why we can include the correct account URL in the error message, to help this group debug, if this group even actually exists.
va/dns.go
Outdated
| // the common DNS validation logic. | ||
| func (va *ValidationAuthorityImpl) validateDNSAccount01(ctx context.Context, ident identifier.ACMEIdentifier, keyAuthorization string, accountURI string) ([]core.ValidationRecord, error) { | ||
| if ident.Type != identifier.TypeDNS { | ||
| va.log.Infof("Identifier type for DNS-ACCOUNT-01 challenge was not DNS: %s", ident) |
There was a problem hiding this comment.
I don't know why we info log this in the existing validateDNS01, but I don't think it's worth copying here.
sheurich
force-pushed
the
feat-dns-account-01-mod-val
branch
13 times, most recently
from
3b5bb07 to
2c19389
Compare
sheurich
force-pushed
the
feat-dns-account-01-mod-val
branch
from
2c19389 to
3f7cef6
Compare
- Add `ChallengeTypeDNSAccount01` constant, `IsValid` update, and `RecordsSane` logic in `core/objects.go` - Add `DNSAccountChallenge01` function and handling in `core/challenges.go` - Add tests for the new challenge type in `core/core_test.go` and `core/objects_test.go` Implements core components for draft-ietf-acme-dns-account-label-00
- Update SA model to store dns-account-01 challenge type
- Modify PA to offer dns-account-01 when feature flag is enabled - Add tests for dns-account-01 challenge type in PA - Update challenge type config handling Enables dns-account-01 to be offered as a valid challenge type
Extracts the common DNS TXT record lookup, comparison, and result/error handling logic from `validateDNS01` into a new unexported helper function `validateDNS`. This change aims to reduce code duplication in preparation for simplifying the upcoming `validateDNSAccount01` function, which shares most of this core validation flow. `validateDNS01` is updated to calculate its specific inputs (digest, query domain) and then call the new helper function. There are no intended functional changes to `validateDNS01` itself.
Adds validation logic for the ACME `dns-account-01` challenge type. Introduces the `validateDNSAccount01` function in `va/dns.go` which implements the account-specific DNS label construction (based on the Account URI) and validation flow, utilizing the shared `validateDNS` function. Unauthorized errors returned during validation are enriched with the Account URI for context. Adds a new test file `va/dns_account_test.go` with unit tests for the `validateDNSAccount01` function, mirroring existing dns-01 test scenarios and checking specific dns-account-01 behavior. Updates `bdns/mocks.go` with necessary entries to support the new dns-account-01 test cases. Ref: draft-ietf-acme-dns-account-label-00
Passes the account URI to the VA's validateChallenge function and adds a case to route dns-account-01 challenges (when enabled) to a new validateDNSAccount01 function. Updates the caller in DoDCV and adjusts tests.
Adds a helper in chisel2.py to select the first supported challenge (currently excluding dns-account-01) and updates v2_integration.py to use it. This prevents test failures until the underlying Python ACME library includes support for the new challenge type.
Update PerformValidation to accept AccountURI from the WFE and pass it to the VA client during validation requests. This is necessary for the VA to calculate the correct DNS label for dns-account-01 challenges. Update unit tests to reflect the change.
- Add accountURI field to PerformValidationRequest in RA and VA protocol definitions Required for passing Account URI from WFE to VA for validation
sheurich
force-pushed
the
feat-dns-account-01-mod-val
branch
from
3f7cef6 to
2e16e53
Compare
Description:
This PR implements the server-side validation logic and necessary protocol/model updates for the
dns-account-01challenge type (draft-ietf-acme-dns-account-label-00), building upon a previous PR (#8140) which introduced the core type definitions.Key Changes:
Protocol, Feature Flag, and Configuration Updates:
ra/proto/ra.proto,va/proto/va.proto) to include theaccountURIin validation requests. This URI is essential for the VA to calculate the account-specific DNS label fordns-account-01.DNSAccount01Enabledfeature flag infeatures/features.goto gate the new challenge type's functionality.policy/pa.goto offer thedns-account-01challenge type when theDNSAccount01Enabledfeature flag is active.policy/pa_test.goto test this conditional offering.ra/ra.goso thatPerformValidationpasses theaccountURI(received from WFE2) to the VA.NewOrderlogic inra/ra.goto considerdns-account-01as a valid challenge for wildcard domains ifDNSAccount01Enabledis true.wfe2/wfe.goso thatpostChallengeextracts theaccountURIfrom the JWSkidheader and includes it in thePerformValidationRequestto the RA when handling adns-account-01challenge and the feature flag is enabled.cmd/config.goto includedns-account-01in the list of valid challenge types forPAConfig.Challengesvalidation.VA Validation Implementation (
va/):va/dns.gointo a sharedvalidateDNShelper function.dns-account-01Logic: Implemented thevalidateDNSAccount01function inva/dns.go. This function:accountURI(SHA-256 hash of the URI -> first 10 bytes -> Base32 encoding), as specified in the draft._<label>._acme-challenge.<domain>).validateDNShelper to perform the DNS TXT record lookup and comparison.Unauthorizederrors with theaccountURIfor better diagnostics.va/va.goto:dns-account-01validation requests to the newvalidateDNSAccount01function if theDNSAccount01Enabledfeature flag is active.accountURItovalidateDNSAccount01.validateChallengeinternal function signature was updated to accept theaccountURI.Testing:
validateDNSAccount01logic inva/dns_account_test.go, covering success cases, various failure modes (e.g., wrong record, SERVFAIL, missingaccountURI), and edge cases, mirroring existingdns-01tests.bdns/mocks.goto support the new account-specific query domain format used indns-account-01tests.va/va_test.goto accommodate the newaccountURIparameter in thevalidateChallengefunction signature.test/config-next/for RA, VA, and RemoteVAs to enable theDNSAccount01Enabledfeature flag and for the PA to offer thedns-account-01challenge.Notes:
dns-account-01.dns-account-01challenges is gated by theDNSAccount01Enabledfeature flag.