Merge to main#22
Open
indrora wants to merge 49 commits into
Open
Conversation
Brings in upstream fixes from main while preserving the dnsplugins branch architecture (inline DNS providers removed in favor of external plugins resolved via IDomainValidatorFactory). Resolutions: - Kept deletion of inline Cloudflare/Google/Factory providers (moved to plugin projects on this branch). - Dropped provider-specific config fields/annotations re-added by main; those configs now belong to each DNS plugin. - Added main's new Enabled disable-switch: cached AcmeClientConfig in Initialize, early-return in Initialize, and FAILED enrollment when the connector is disabled. - Added main's DnsVerificationServer config (with annotation) for private DNS zones, and wired it into the DnsVerificationHelper constructor call in Enroll. - Kept dnsplugins' AcmeCaPlugin.csproj state (newer IAnyCAPlugin prerelease, Google package removed, other provider packages pending cleanup). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ports the FlowLogger pattern from Keyfactor/barracuda-wafasaas-orchestrator and adapts it for an IAnyCAPlugin. The accumulated step breadcrumb is appended to EnrollmentResult.StatusMessage on both success and failure, so operators see a scannable per-step summary in the Command UI instead of just a single exception message. Changes: - FlowLogger.cs: ported verbatim from barracuda with namespace changed to Keyfactor.Extensions.CAPlugin.Acme. Added StepAsync<T> overload for async methods that return a value. - Enroll: wraps each stage (ValidateInput, FormatCsr, LoadConfig, CreateHttpClient, InitAcmeAccount, CreateAcmeClient, DecodeCsr, ExtractDomainsFromCsr, CreateOrder, ExtractOrderIdentifier, FinalizeOrder, DownloadCertificate, EncodeCertificateToPem) as a timed flow.Step. Success returns include flow.GetSummary(); failure paths include DescribeException(ex) + flow.GetSummary(). - ProcessAuthorizations: takes the flow and records per-domain work in three branches (StageDnsRecords / VerifyAndSubmit / CleanupDnsRecords), so the breadcrumb shows which specific domain failed when a challenge breaks. - DescribeException helper: unwraps AggregateException/TargetInvocation wrappers, surfaces HttpRequestException context, and truncates overlong messages so the summary stays readable. - Initialize: added ValidateConfigForEnrollment — fails fast (at save time, not first enroll) on missing DirectoryUrl/Email, non-absolute or non-http(s) DirectoryUrl, mismatched EAB key pair, or negative DnsPropagationDelaySeconds. Build: net6.0 / net8.0 / net10.0 — 0 errors, pre-existing warnings only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removed specific root/intermediate names (ISRG Root X1, R3) that go stale when Let's Encrypt rotates their chain. Users are now directed to the official Let's Encrypt certificates page to identify the currently active root and intermediate certificates. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolve the ACME challenge record name through any chain of CNAME delegations to its terminal target before staging validation. The resolved name now drives DNS provider plugin selection, record creation, propagation verification, and cleanup, so challenges delegated into a zone on a different provider are routed to the plugin that owns that zone. Non-delegated domains are unaffected.
There was a problem hiding this comment.
Pull request overview
This PR advances the ACME CA Gateway plugin toward a plugin-based DNS provider architecture by removing embedded DNS provider implementations, introducing CNAME-delegation support for DNS-01 challenges, and improving enrollment traceability/logging. It also updates target frameworks/dependencies and adds documentation around DNS provider plugin migration.
Changes:
- Externalizes DNS providers (removes embedded provider classes/factory + config fields) and introduces
IDomainValidatorFactory-based resolution during enrollment. - Adds CNAME delegation resolution for
_acme-challengeTXT placement and step-oriented flow logging for enrollment troubleshooting. - Updates TFMs/dependencies (drops
net6.0, bumps Keyfactor/AWS/Azure packages) and refreshes docs/changelog/workflow.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| TestProgram/TestProgram.csproj | Drops net6.0 target; bumps Keyfactor/AWS deps; adds Azure.Identity. |
| TestProgram/Program.cs | Updates test harness to construct AcmeCaPlugin with a validator factory; adds initialization error handling; adds mock factory. |
| README.md | Documentation edits and formatting; updates extension folder examples; edits configuration text. |
| docsource/configuration.md | Updates Let’s Encrypt chain guidance in configuration docs. |
| DNS-PLUGINS-COMPLETE.md | Adds DNS plugin migration “complete” documentation and deployment/build notes. |
| dns-plugin-developer-guide.html | Adds an HTML developer guide describing DNS provider plugin architecture and manifests. |
| CHANGELOG.md | Adds v2.0.0 entry for DNS plugin + CNAME proxy support. |
| AcmeCaPlugin/FlowLogger.cs | Adds a step/branch-based trace logger that emits a summarized flow. |
| AcmeCaPlugin/Clients/DNS/Rfc2136DnsProvider.cs | Removes embedded RFC2136 DNS provider implementation. |
| AcmeCaPlugin/Clients/DNS/Ns1DnsProvider.cs | Removes embedded NS1 DNS provider implementation. |
| AcmeCaPlugin/Clients/DNS/InfobloxDnsProvider.cs | Removes embedded Infoblox DNS provider implementation. |
| AcmeCaPlugin/Clients/DNS/IDnsProvider.cs | Removes embedded IDnsProvider abstraction. |
| AcmeCaPlugin/Clients/DNS/GoogleDnsProvider.cs | Removes embedded Google Cloud DNS provider implementation. |
| AcmeCaPlugin/Clients/DNS/DnsProviderFactory.cs | Removes embedded DNS provider factory selection logic. |
| AcmeCaPlugin/Clients/DNS/CloudflareDnsProvider.cs | Removes embedded Cloudflare DNS provider implementation. |
| AcmeCaPlugin/Clients/DNS/AzureDnsProvider.cs | Removes embedded Azure DNS provider implementation. |
| AcmeCaPlugin/Clients/DNS/AwsRoute53DnsProvider.cs | Removes embedded AWS Route53 DNS provider implementation. |
| AcmeCaPlugin/Clients/DNS/CnameResolver.cs | Adds CNAME-chain resolver to support delegated DNS-01 challenge records. |
| AcmeCaPlugin/Clients/Acme/AcmeClient.cs | Improves ACME challenge failure logging (logs challenge.Error details). |
| AcmeCaPlugin/AcmeClientConfig.cs | Removes per-provider DNS config fields; adds DNS propagation delay setting. |
| AcmeCaPlugin/AcmeCaPluginConfig.cs | Removes per-provider configuration annotations; adds propagation delay annotation. |
| AcmeCaPlugin/AcmeCaPlugin.csproj | Drops net6.0; updates Keyfactor package versions; reorganizes dependency comments. |
| AcmeCaPlugin/AcmeCaPlugin.cs | Requires IDomainValidatorFactory, adds config validation, flow logging, CNAME-aware DNS record targeting, plugin-based DNS staging/cleanup. |
| AcmeCaPlugin.sln | Adds x86/x64 solution configurations. |
| .github/workflows/keyfactor-bootstrap-workflow.yml | Updates bootstrap workflow reference to v5 and adds new inputs/secrets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+705
to
+709
| var domainValidator = flow.Step($"ResolveValidator:{domain}", | ||
| () => | ||
| { | ||
| var v = _validatorFactory.ResolveDomainValidator(validatorLookupName, DNS_CHALLENGE_TYPE); | ||
| if (v == null) |
Comment on lines
68
to
72
| @@ -71,13 +71,24 @@ public class AcmeCaPlugin : IAnyCAPlugin | |||
| private const int DNS_PROPAGATION_DELAY_SECONDS = 30; | |||
| private const string USER_AGENT = "KeyfactorAcmePlugin/1.0"; | |||
Comment on lines
+26
to
+50
| <!-- DNS Provider dependencies - REMOVE THESE after migrating all providers to plugins --> | ||
| <!-- TODO: These should be removed once all DNS providers are moved to separate plugin projects --> | ||
| <!-- Google DNS - MIGRATED to Keyfactor.DnsProvider.Google plugin --> | ||
| <!-- <PackageReference Include="Google.Apis.Dns.v1" Version="1.69.0.3753" /> --> | ||
|
|
||
| <!-- AWS Route53 - TODO: Migrate to plugin --> | ||
| <PackageReference Include="AWSSDK.Core" Version="4.0.3.11" /> | ||
| <PackageReference Include="AWSSDK.Route53" Version="4.0.8.8" /> | ||
|
|
||
| <!-- Azure DNS - TODO: Migrate to plugin --> | ||
| <PackageReference Include="Azure.Identity" Version="1.14.0" /> | ||
| <PackageReference Include="Azure.ResourceManager.Cdn" Version="1.4.0" /> | ||
| <PackageReference Include="Azure.ResourceManager.Dns" Version="1.1.1" /> | ||
|
|
||
| <!-- RFC2136 - TODO: Migrate to plugin --> | ||
| <PackageReference Include="ARSoft.Tools.Net" Version="3.6.0" /> | ||
|
|
||
| <!-- Cloudflare - TODO: Migrate to plugin (uses standard HTTP client) --> | ||
|
|
||
| <!-- NS1 - TODO: Migrate to plugin (uses standard HTTP client) --> | ||
|
|
||
| <!-- Infoblox - TODO: Migrate to plugin (uses standard HTTP client) --> | ||
|
|
||
| <!-- Public Suffix - TODO: Evaluate if needed in core or per-provider --> | ||
| <PackageReference Include="Nager.PublicSuffix" Version="3.5.0" /> |
Comment on lines
+74
to
+80
| catch (Exception ex) | ||
| { | ||
| logger.LogError($"❌ Failed to initialize plugin: {ex.Message}"); | ||
| logger.LogInformation("📌 Note: The plugin now requires DNS provider plugins to be deployed separately."); | ||
| logger.LogInformation("📌 See DNS-PLUGINS-COMPLETE.md for deployment instructions."); | ||
| return; | ||
| } |
Comment on lines
+424
to
+425
| // For now, this returns null which will cause the plugin to fail initialization | ||
| // TODO: Load actual DNS provider plugin assemblies for testing |
| <h3>csproj wiring so the manifest ships with the DLL</h3> | ||
|
|
||
| <pre><code><<span class="type">ItemGroup</span>> | ||
| <<span class="type">PackageReference</span> Include=<span class="str">"Keyfactor.AnyGateway.IAnyCAPlugin"</span> Version=<span class="str">"3.3.0-PRERELEASE-..."</span> /> |
Comment on lines
+676
to
+686
| * **Enabled** - Enable or disable this CA connector. When disabled, all operations (ping, enroll, sync) are skipped. | ||
| * **DirectoryUrl** - ACME directory URL (e.g. Let's Encrypt, ZeroSSL, etc.) | ||
| * **Email** - Email for ACME account registration. | ||
| * **EabKid** - External Account Binding Key ID (optional) | ||
| * **EabHmacKey** - External Account Binding HMAC key (optional) | ||
| * **SignerEncryptionPhrase** - Used to encrypt singer information when account is saved to disk (optional) | ||
| * **DnsProvider** - DNS Provider to use for ACME DNS-01 challenges (options: Google, Cloudflare, AwsRoute53, Azure, Ns1, Rfc2136, Infoblox) | ||
| * **Google_ServiceAccountKeyPath** - Google Cloud DNS: Path to service account JSON key file only if using Google DNS (Optional) | ||
| * **Google_ServiceAccountKeyJson** - Google Cloud DNS: Service account JSON key content (alternative to file path for containerized deployments) | ||
| * **Google_ProjectId** - Google Cloud DNS: Project ID only if using Google DNS (Optional) | ||
| * **AccountStoragePath** - Path for ACME account storage. Defaults to %APPDATA%\AcmeAccounts on Windows or ./AcmeAccounts in containers. |
| * **Email** - Email for ACME account registration. | ||
| * **EabKid** - External Account Binding Key ID (optional) | ||
| * **EabHmacKey** - External Account Binding HMAC key (optional) | ||
| * **SignerEncryptionPhrase** - Used to encrypt singer information when account is saved to disk (optional) |
Comment on lines
+1
to
+4
| # v2.0.0 | ||
| * Dns Plugin Support | ||
| * Cname Proxy Support | ||
|
|
Comment on lines
+112
to
+115
| ```bash | ||
| cd "c:\Users\bhill\source\repos\acme-provider-caplugin" | ||
| dotnet build --configuration Release | ||
| ``` |
Add a CNAME Delegation section to docsource/configuration.md describing why challenge names are delegated to an isolated validation zone, how the CnameResolver follows a multi-level CNAME chain to its terminus, how the DNS provider plugin is selected against the resolved target (enabling cross-provider delegation), the loop/depth safety guards, and private-zone resolution via DnsVerificationServer. Update the enrollment flow summary to include the CNAME resolution step.
Cname delegation dnsplugins
DNS providers are now standalone, pluggable plugins deployed alongside the AnyCA Gateway rather than built into this ACME plugin. Remove the hardcoded "supported DNS providers" lists, per-provider credential/config tables, RFC 2136 setup, and the obsolete IDnsProvider/DnsProviderFactory "adding new providers" guidance. Point instead to the Keyfactor -dnsplugin repositories query as the authoritative source, and document that providers are configured via the Gateway's Domain Validation config and resolved per domain (including CNAME-delegated targets).
Cname delegation dnsplugins
DNS providers are now separate plugins, so the ACME plugin no longer needs their SDKs or per-provider config fields. Drop the AWS, Azure, ARSoft (RFC2136), and Nager.PublicSuffix package references (all unused in code) and remove the DnsProvider selector plus every per-provider config entry from the integration manifest. Keep the ACME-level fields, AccountStoragePath (still used for account storage), and DnsVerificationServer (used for propagation checks and CNAME resolution). DnsClient is retained for CNAME delegation and TXT propagation.
Cname delegation dnsplugins
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (4)
AcmeCaPlugin/AcmeCaPlugin.cs:708
IDomainValidatorFactory.ResolveDomainValidatoris being called withDNS_CHALLENGE_TYPE("dns-01"). The domain validator contract and manifests use a validation type of "DNS", so passing the ACME challenge type here is likely to prevent the factory from resolving any validator (always returning null).
var v = _validatorFactory.ResolveDomainValidator(validatorLookupName, DNS_CHALLENGE_TYPE);
AcmeCaPlugin/AcmeCaPlugin.cs:72
DNS_PROPAGATION_DELAY_SECONDSis declared but not used anywhere in this file (propagation is now driven byDnsPropagationDelaySecondsin config). Removing the unused constant avoids confusion about which delay is in effect.
private const string DEFAULT_PRODUCT_ID = "default";
private const string DNS_CHALLENGE_TYPE = "dns-01";
private const int DNS_PROPAGATION_DELAY_SECONDS = 30;
private const string USER_AGENT = "KeyfactorAcmePlugin/1.0";
TestProgram/Program.cs:76
LogErroris currently called with an interpolated string, which drops the exception details/stack trace from structured logs. Pass the exception as the first argument and use structured parameters so failures are diagnosable.
logger.LogError($"❌ Failed to initialize plugin: {ex.Message}");
TestProgram/Program.cs:425
- This mock factory returns null, which will cause enrollment/challenge processing to fail when the plugin attempts to resolve a validator (not during
Initialize). The comment is currently inaccurate and can mislead debugging.
// In a real test scenario, you would load the actual plugin assemblies here
// For now, this returns null which will cause the plugin to fail initialization
// TODO: Load actual DNS provider plugin assemblies for testing
Comment on lines
+755
to
+759
| var validatorTypeName = validator.GetType().Name.ToLowerInvariant(); | ||
| bool isPrivateDnsProvider = validatorTypeName.Contains("rfc2136") || validatorTypeName.Contains("infoblox"); | ||
|
|
||
| if (!propagated) | ||
| if (isPrivateDnsProvider) | ||
| { |
Comment on lines
44
to
47
| { | ||
| "name": "DnsVerificationServer", | ||
| "description": "DNS server to use for verifying TXT record propagation. For private/local DNS zones, set this to your authoritative DNS server IP (e.g., 10.3.10.37). Leave empty to use public DNS servers (Google, Cloudflare, etc.)." | ||
| }, | ||
| { | ||
| "name": "Infoblox_Host", | ||
| "description": "Infoblox DNS: API URL (e.g., https://infoblox.example.com/wapi/v2.12) only if using Infoblox DNS (Optional)" | ||
| }, | ||
| { | ||
| "name": "Infoblox_Username", | ||
| "description": "Infoblox DNS: Username for authentication only if using Infoblox DNS (Optional)" | ||
| }, | ||
| { | ||
| "name": "Infoblox_Password", | ||
| "description": "Infoblox DNS: Password for authentication only if using Infoblox DNS (Optional)" | ||
| "description": "DNS server used to verify TXT record propagation and to resolve CNAME delegation chains. For private/local DNS zones, set this to your authoritative DNS server IP (e.g., 10.3.10.37). Leave empty to use public DNS servers (Google, Cloudflare, etc.)." | ||
| } |
Comment on lines
+185
to
+186
| "AwsRoute53_AccessKey": "AKIAIOSFODNN7EXAMPLE", | ||
| "AwsRoute53_SecretKey": "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" |
Comment on lines
+158
to
+166
| ## Configuration Examples | ||
|
|
||
| ### Using Cloudflare | ||
| ```json | ||
| { | ||
| "DirectoryUrl": "https://acme-v02.api.letsencrypt.org/directory", | ||
| "Email": "admin@example.com", | ||
| "DnsProvider": "cloudflare", | ||
| "Cloudflare_ApiToken": "your-api-token-here" |
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.
Merge dnsplugins to main - Automated PR