feat: Support for dns-account-01 Challenge Type#8
Closed
sheurich wants to merge 10 commits intofeat-dns-account-01-corefrom
Closed
feat: Support for dns-account-01 Challenge Type#8sheurich wants to merge 10 commits intofeat-dns-account-01-corefrom
sheurich wants to merge 10 commits intofeat-dns-account-01-corefrom
Conversation
- 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
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.
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.
…se config-driven approach - Remove AccountURI from gRPC PerformValidationRequest to avoid protocol changes - Pass DNSAccountChallengeURIPrefix via config to VA instead of per-request - Update NewValidationAuthorityImpl constructor to accept DNSAccountChallengeURIPrefix - Refactor all tests to use new constructor signature - Update va/va.go to construct accountURI from config prefix + accountID - Ensure all config files have DNSAccountChallengeURIPrefix set correctly
|
@sheurich, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values. |
|
@sheurich, this PR adds one or more new feature flags: DNSAccount01Enabled. As such, this PR must be accompanied by a review of the Let's Encrypt CP/CPS to ensure that our behavior both before and after this flag is flipped is compliant with that document. Please conduct such a review, then add your findings to the PR description in a paragraph beginning with "CPS Compliance Review:". |
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces support for the new ACME challenge type dns-account-01. The changes span core validation logic, configuration updates, client helper methods, and additional testing to ensure end‐to‐end functionality for this challenge type.
- Added support for dns-account-01 in the Validation Authority and auxiliary modules.
- Updated configuration files, mocks, and tests to include the new challenge type.
- Refactored challenge selection logic to optionally include dns-account-01 based on the feature flag.
Reviewed Changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| va/va_test.go | Updated test calls to include the new accountURI parameter |
| va/va.go | Modified validateChallenge and DoDCV to handle dns-account-01 challenges |
| va/dns_test.go | Renamed DNS test functions to reflect dns-01 naming conventions |
| va/dns_account_test.go | New tests covering dns-account-01 challenge validations |
| va/dns.go | Added validateDNSAccount01 with account-specific DNS label construction |
| va/config/config.go | Added new DNSAccountChallengeURIPrefix configuration field |
| test/v2_integration.py | Updated challenge selection helper in integration tests |
| test/chisel2.py | Excluded dns-account-01 from supported challenges in Python ACME library |
| test/chall-test-srv-client/client.go | Added helpers for adding/removing dns-account-01 responses |
| policy/pa_test.go | Extended challenge types tests to include the new dns-account-01 flag |
| policy/pa.go | Updated ChallengeTypesFor to optionally include dns-account-01 challenges |
| features/features.go | Added DNSAccount01Enabled flag |
| cmd/* files | Updated various command-line configuration files to support the new flag |
| bdns/mocks.go | Extended mock DNS lookup to simulate account-specific validation records |
Comments suppressed due to low confidence (1)
va/dns_account_test.go:66
- The variable 'ctx' is used without being defined in this test. Consider using 'context.Background()' explicitly to ensure the context is properly initialized.
_, err := va.validateDNSAccount01(ctx, identifier.NewDNS("localhost"), expectedKeyAuthorization, testAccountURI)
| } | ||
|
|
||
| // Calculate the DNS prefix label based on the account URI | ||
| sha256sum := sha256.Sum256([]byte(accountURI)) |
There was a problem hiding this comment.
[nitpick] The account label calculation logic appears in both the DNS validation function and the client helper. Consider extracting this logic into a shared utility function to reduce duplication.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements the server-side validation logic and necessary protocol/model updates for the dns-account-01 challenge type (draft-ietf-acme-dns-account-label-01), building upon a previous PR (letsencrypt#8140) which introduced the core type definitions.
Support for
dns-account-01Challenge Type:Core Challenge Logic:
dns-account-01challenge type in thePAConfigstruct and theChallengeTypesFormethod inpolicy/pa.go. The feature is gated by theDNSAccount01Enabledflag. [1] [2]MockClientinbdns/mocks.goto simulate various DNS scenarios for thedns-account-01challenge. [1] [2]Configuration and Feature Toggles:
DNSAccount01Enabledflag in thefeatures.Configstruct to enable or disable thedns-account-01challenge.dnsAccountChallengeURIPrefixto the VA and RVA configurations for handling account-specific DNS challenges. [1] [2] [3]Testing Enhancements:
policy/pa_test.goto include test cases fordns-account-01with the feature both enabled and disabled. [1] [2] [3] [4] [5]test/v2_integration.pyto use a helper method for selecting supported challenges, ensuring compatibility withdns-account-01. [1] [2]Client and Utility Updates:
dns-account-01challenge responses intest/chall-test-srv-client/client.go. These include adding and removing DNS records for the challenge.test/chisel2.pyto fetch supported challenges, excludingdns-account-01for now due to library limitations.Configuration Changes for Testing:
dns-account-01in test configurations for RA, VA, and RVA services to validate end-to-end functionality. [1] [2] [3] [4] [5] [6] [7]