Add "dns-account-01" support from draft-ietf-acme-scoped-dns-challenges#435
Add "dns-account-01" support from draft-ietf-acme-scoped-dns-challenges#435jsha merged 3 commits intoletsencrypt:mainfrom
Conversation
|
@aarongable @jsha this change is ready to begin a review. As some of the fixes are unrelated to the direct problem at hand, and possibly controversial, feel free to excise anything that doesn't fit. Also note that the method for passing the additional scoping attributes from WFE to VA seems suboptimal and there may be a better fit for how the Pebble code aligns with Boulder. Any feedback in this area would be greatly appreciated. |
|
Awesome, I'm exciting to do a full review of this! I'm on vacation this week, but will be able to do a full review on Monday (and other folks should be able to review before then). In the mean time, a bunch of CI improvement and fixes have been landed, so let's get this rebased on top of those / get those merged into this branch so it's just the code changes without accompanying infrastructure changes. |
sheurich
force-pushed
the
add-dns-account-01
branch
from
2923c70 to
ffadbd9
Compare
|
@aarongable it's only the relevant code changes here. The integration tests have been moved to #444. |
sheurich
force-pushed
the
add-dns-account-01
branch
from
ffadbd9 to
97ea757
Compare
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
Just a few small nits remaining at the point!
| } | ||
|
|
||
| func (va VAImpl) ValidateChallenge(ident acme.Identifier, chal *core.Challenge, acct *core.Account) { | ||
| func (va VAImpl) ValidateChallenge(ident acme.Identifier, chal *core.Challenge, acct *core.Account, acctURL string, wildcard bool) { |
There was a problem hiding this comment.
It feels slightly awkward to pass both a *core.Account and an acctURL, given that one can be constructed from the other, but also given that the construction method is acctURL := wfe.relativeEndpoint(request, fmt.Sprintf("%s%s", acctPath, newAcct.ID)) and half of those values are inaccessible here in the VA, I think this is fine.
aarongable
requested review from
a team and
beautifulentropy
and removed request for
a team
Co-authored-by: Aaron Gable <aaron@aarongable.com>
sheurich
force-pushed
the
add-dns-account-01
branch
from
b6c0da6 to
25c0824
Compare
|
|
||
| // Submit a validation job to the VA, this will be processed asynchronously | ||
| wfe.va.ValidateChallenge(ident, existingChal, existingAcct) | ||
| wfe.va.ValidateChallenge(ident, existingChal, existingAcct, acctURL, wildcard) |
There was a problem hiding this comment.
Same feedback here: letsencrypt/boulder#7387 (comment)
aaomidi
left a comment
aaomidi
left a comment
There was a problem hiding this comment.
I think using the KID from the JWT is probably the way to go here.
|
Good points @aaomidi. Other than that the code looks good to me, and we have an earlier approval from @aarongable. I'm going to land this, and we can explore plumbing the KID from the JWT as a separate PR. |
When testing my Stalwart deployment with Pebble[1], I got the following
ACME error:
ACME error (acme.error) {
reason = "unknown variant `dns-account-01`, expected one of `http-01`, `dns-01`, `tls-alpn-01` at line 15 column 33",
details = JSON deserialization failed
}
In RFC 8555 section 8[2], the validation challenges are meant to be
extensible:
> The identifier validation challenges described in this section all
> relate to validation of domain names. If ACME is extended in the
> future to support other types of identifiers, there will need to be
> new challenge types, and they will need to specify which types of
> identifier they apply to.
The mentioned error refers to the following draft:
https://datatracker.ietf.org/doc/html/draft-ietf-acme-dns-account-label-01
Pebble already implemented[3] this and while it's IMHO too early to
already support this in Stalwart, we should at least make sure that we
don't break existing deployments in case ACME CAs one day add more
challange types like the above.
[1]: https://github.com/letsencrypt/pebble
[2]: https://datatracker.ietf.org/doc/html/rfc8555#section-8
[3]: letsencrypt/pebble#435
Signed-off-by: aszlig <aszlig@nix.build>
When testing my Stalwart deployment with Pebble[1], I got the following
ACME error:
ACME error (acme.error) {
reason = "unknown variant `dns-account-01`, expected one of `http-01`, `dns-01`, `tls-alpn-01` at line 15 column 33",
details = JSON deserialization failed
}
In RFC 8555 section 8[2], the validation challenges are meant to be
extensible:
> The identifier validation challenges described in this section all
> relate to validation of domain names. If ACME is extended in the
> future to support other types of identifiers, there will need to be
> new challenge types, and they will need to specify which types of
> identifier they apply to.
The mentioned error refers to the following draft:
https://datatracker.ietf.org/doc/html/draft-ietf-acme-dns-account-label-01
Pebble already implemented[3] this and while it's IMHO too early to
already support this in Stalwart, we should at least make sure that we
don't break existing deployments in case ACME CAs one day add more
challange types like the above.
[1]: https://github.com/letsencrypt/pebble
[2]: https://datatracker.ietf.org/doc/html/rfc8555#section-8
[3]: letsencrypt/pebble#435
Signed-off-by: aszlig <aszlig@nix.build>
4 participants
This change implements the
dns-account-01ACME challenge as specified in draft-ietf-acme-scoped-dns-challenges.The relevant validation label computation is:
where SCOPE is one of {
host,wildcard}. A SCOPE of {domain} is unimplemented.This implementation is interoperable with the https://github.com/eggsampler/acme changes in eggsampler/acme#21 and passes the
TestWildcardDNSAccounttest.Solves #425.