From 44bc7ff4245bed4e6fffe4447bb98506057371f3 Mon Sep 17 00:00:00 2001 From: Brian Hill Date: Mon, 21 Sep 2026 22:00:42 -0400 Subject: [PATCH] Rename PublishCnameDcv step and explain why it's a no-op for non-CNAME methods The DcvAutoPublish step (formerly PublishCnameDcv) ran unconditionally on every New enrollment and, for EMAIL-validated certs, silently no-op'd while still showing a bare [OK] under a CNAME-sounding step name in the flow summary - confusing when read back in Command's UI. Add a FlowLogger.StepAsync overload that uses the action's own return value as the step detail (the existing overload's detail parameter is evaluated before the action runs, so it can't reflect what the action decided), and have TryPublishCnameDcvAsync return why it skipped or how many records it published. --- .../CSCGlobalCAPluginTests.cs | 6 +++- cscglobal-caplugin/CSCGlobalCAPlugin.cs | 18 +++++----- cscglobal-caplugin/FlowLogger.cs | 36 +++++++++++++++++++ 3 files changed, 51 insertions(+), 9 deletions(-) diff --git a/cscglobal-caplugin.Tests/CSCGlobalCAPluginTests.cs b/cscglobal-caplugin.Tests/CSCGlobalCAPluginTests.cs index 6443a8d..a763f2e 100644 --- a/cscglobal-caplugin.Tests/CSCGlobalCAPluginTests.cs +++ b/cscglobal-caplugin.Tests/CSCGlobalCAPluginTests.cs @@ -1048,10 +1048,14 @@ public async Task Enroll_New_WithDnsValidatorFactoryButEmailMethod_DoesNotAttemp [EnrollmentConfigConstants.DomainControlValidationMethod] = "EMAIL" }); - await plugin.Enroll("csr", "CN=test", new Dictionary(), productInfo, + var result = await plugin.Enroll("csr", "CN=test", new Dictionary(), productInfo, RequestFormat.PKCS10, EnrollmentType.New); mockFactory.Verify(f => f.ResolveDomainValidator(It.IsAny(), It.IsAny()), Times.Never); + // The DcvAutoPublish step must explain *why* it was a no-op for a non-CNAME method, + // rather than showing a bare [OK] under a CNAME-sounding step name. + var publishStep = result.EnrollmentContext.Single(e => e.Key.Contains("DcvAutoPublish")); + Assert.Contains("not CNAME", publishStep.Value); } [Fact] diff --git a/cscglobal-caplugin/CSCGlobalCAPlugin.cs b/cscglobal-caplugin/CSCGlobalCAPlugin.cs index 928e8d7..82129a3 100644 --- a/cscglobal-caplugin/CSCGlobalCAPlugin.cs +++ b/cscglobal-caplugin/CSCGlobalCAPlugin.cs @@ -698,10 +698,7 @@ await flow.StepAsync("SubmitRegistrationToCSC", async () => var enrollResult = _requestManager.GetEnrollmentResult(enrollmentResponse); flow.Step("MapResult", $"Status={enrollResult?.Status}, ID={enrollResult?.CARequestID ?? "(null)"}"); - await flow.StepAsync("PublishCnameDcv", async () => - { - await TryPublishCnameDcvAsync(productInfo, enrollResult); - }); + await flow.StepAsync("DcvAutoPublish", () => TryPublishCnameDcvAsync(productInfo, enrollResult)); EnrollmentResult? newPolled = null; await flow.StepAsync("PollForIssuance", async () => @@ -1388,19 +1385,21 @@ record = null; /// resolves for its domain. No-op if the factory wasn't injected, the cert isn't using CNAME /// validation, or the response contains no CNAME details. Failures are logged but never thrown — /// manual publishing remains a fallback so the enrollment result is still returned to Keyfactor. + /// Returns a short description of what happened (published/skipped/why), surfaced as the + /// flow step's detail so a no-op for non-CNAME methods doesn't look unexplained. /// - private async Task TryPublishCnameDcvAsync(EnrollmentProductInfo productInfo, EnrollmentResult? enrollResult) + private async Task TryPublishCnameDcvAsync(EnrollmentProductInfo productInfo, EnrollmentResult? enrollResult) { if (_validatorFactory == null) { Logger.LogTrace("TryPublishCnameDcvAsync: no IDomainValidatorFactory was injected, skipping auto-publish."); - return; + return "skipped - no DNS validator factory injected"; } if (enrollResult?.EnrollmentContext == null || enrollResult.EnrollmentContext.Count == 0) { Logger.LogTrace("TryPublishCnameDcvAsync: no CNAME entries in EnrollmentContext, skipping."); - return; + return "skipped - no DCV entries returned by CSC"; } var dcvMethod = productInfo?.ProductParameters != null @@ -1412,7 +1411,7 @@ private async Task TryPublishCnameDcvAsync(EnrollmentProductInfo productInfo, En !string.Equals(dcvMethod, "CNAME", StringComparison.OrdinalIgnoreCase)) { Logger.LogTrace("TryPublishCnameDcvAsync: DCV method '{Method}' is not CNAME, skipping auto-publish.", dcvMethod ?? "(null)"); - return; + return $"skipped - DCV method is '{dcvMethod ?? "(none)"}', not CNAME"; } Logger.LogInformation( @@ -1498,6 +1497,9 @@ private async Task TryPublishCnameDcvAsync(EnrollmentProductInfo productInfo, En Logger.LogInformation( "TryPublishCnameDcvAsync: complete. Published={Published}, Failed={Failed}, Unresolved={Unresolved}", successCount, failCount, unresolvedCount); + + return $"published {successCount}, failed {failCount}, unresolved {unresolvedCount} " + + $"of {enrollResult.EnrollmentContext.Count} CNAME record(s)"; } //Trying to fix leaf extraction diff --git a/cscglobal-caplugin/FlowLogger.cs b/cscglobal-caplugin/FlowLogger.cs index 4ce4ef4..fb90961 100644 --- a/cscglobal-caplugin/FlowLogger.cs +++ b/cscglobal-caplugin/FlowLogger.cs @@ -124,6 +124,42 @@ public async Task StepAsync(string name, Func action, string d return this; } + /// + /// Record an async step whose own return value becomes the step's detail - unlike the + /// parameter on the other overload (which is evaluated before + /// the action runs and so can't reflect anything the action decided), this reflects what + /// actually happened during execution (e.g. why a conditional step was a no-op). + /// + public async Task StepAsync(string name, Func> action) + { + var sw = Stopwatch.StartNew(); + var step = new FlowStep { Name = name }; + try + { + _logger.LogTrace(" [{FlowName}] {StepName} ...", _flowName, name); + var detail = await action(); + sw.Stop(); + step.Status = FlowStepStatus.Success; + step.ElapsedMs = sw.ElapsedMilliseconds; + step.Detail = detail; + AddStep(step); + _logger.LogTrace(" [{FlowName}] {StepName} ... OK ({Elapsed}ms){Detail}", + _flowName, name, sw.ElapsedMilliseconds, detail != null ? $" {detail}" : ""); + } + catch (Exception ex) + { + sw.Stop(); + step.Status = FlowStepStatus.Failed; + step.ElapsedMs = sw.ElapsedMilliseconds; + step.Detail = ex.Message; + AddStep(step); + _logger.LogTrace(" [{FlowName}] {StepName} ... FAILED ({Elapsed}ms): {Error}", + _flowName, name, sw.ElapsedMilliseconds, ex.Message); + throw; + } + return this; + } + /// Record a failed step without throwing. public FlowLogger Fail(string name, string reason = null) {