From 3e3d5815b96bc9a34faea51440beac5167079923 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 31 Jul 2026 10:11:23 -0700 Subject: [PATCH 01/10] =?UTF-8?q?feat(enroll):=20synchronous=20certificate?= =?UTF-8?q?=20pickup=20(Sectigo=20parity)=20=E2=80=94=20v1.0.1?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After submitting an order, Enroll() polls GetCertificate up to PickupRetries times (default 5), PickupDelay seconds apart (default 10), after a 5s initial delay, so a fast-issuing order returns the certificate in the same enrollment call instead of waiting for the next sync. Mirrors the legacy Sectigo connector's PickUpEnrolledCertificate (~55s max worker-thread occupancy by default). Applied to the new, reissue, and renew paths; both build flavors. PickupRetries=0 disables. Orders not issued within the window are returned pending and imported by a later sync (unchanged). OV/EV are issued asynchronously by the CA and typically exhaust the window; DV / already-approved orders return in-call. --- CERTInext/CERTInextCAPlugin.cs | 136 ++++++++++++++++++++++++++- CERTInext/CERTInextCAPluginConfig.cs | 57 +++++++++++ CERTInext/Constants.cs | 30 ++++++ CHANGELOG.md | 5 + 4 files changed, 227 insertions(+), 1 deletion(-) diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index 231f611..df51796 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -1170,8 +1170,15 @@ private async Task EnrollNewAsync( } #endif + // Synchronous certificate pickup (Sectigo-parity): poll for the issued certificate so + // a fast-issuing order returns GENERATED + PEM in this same call. No-op for the + // already-issued/failed case and for OV/EV orders that CERTInext issues asynchronously + // — those fall back to the pending result and are imported by the next sync. + var newResult = BuildEnrollmentResult(enrollResp, ep.AutoApprove); + newResult = await PickUpEnrolledCertificateAsync(newResult, enrollResp.Id); + _logger.MethodExit(LogLevel.Debug); - return BuildEnrollmentResult(enrollResp, ep.AutoApprove); + return newResult; } /// @@ -1297,6 +1304,9 @@ private async Task RenewOrReissueAsync( "PriorCARequestID={PriorId}, NewCARequestID={NewId}, Status={Status}", priorCaRequestId, renewResult.CARequestID, renewResult.Status); + // Synchronous certificate pickup (Sectigo-parity), same as the new-enrollment path. + renewResult = await PickUpEnrolledCertificateAsync(renewResult, renewResp.Id); + return renewResult; } else @@ -1868,6 +1878,130 @@ private async Task WaitForDcvVerificationAsync(string orderNumber, IReadOnlyList } } + /// + /// Synchronous certificate pickup — parity with the legacy Sectigo connector's + /// PickUpEnrolledCertificate. After an order is submitted, polls + /// GetCertificate up to PickupRetries times, PickupDelay seconds + /// apart (after a fixed initial delay), so an order that issues quickly is returned + /// GENERATED + PEM in the same enrollment call instead of waiting for the next + /// synchronization. If the certificate has not issued within the budget, the original + /// pending result is returned unchanged and the order is imported by a later sync — + /// behaviour identical to before this feature. + /// + /// Applies to ALL products. CERTInext issues OV/EV asynchronously (organization + /// verification, minutes to hours; confirmed by CERTInext support ticket #162763), so + /// those typically exhaust the budget and fall back to pending; only DV / already-approved + /// orders return in-call. Never throws — any polling error degrades to the pending result. + /// + private async Task PickUpEnrolledCertificateAsync( + EnrollmentResult pendingResult, string orderNumber) + { + // Only a still-pending (external-validation) result can benefit from a pickup poll. + // An already issued/failed/revoked result, or a missing order number, is returned as-is. + if (pendingResult == null + || pendingResult.Status != (int)EndEntityStatus.EXTERNALVALIDATION + || string.IsNullOrWhiteSpace(orderNumber)) + return pendingResult; + + int retries = _config.GetEffectivePickupRetries(); + if (retries <= 0) + { + _logger.LogInformation( + "Synchronous certificate pickup disabled (PickupRetries<=0). Order {OrderNumber} " + + "will be picked up on the next synchronization.", orderNumber); + return pendingResult; + } + + int delaySeconds = _config.GetEffectivePickupDelaySeconds(); + _logger.LogInformation( + "Starting synchronous certificate pickup. OrderNumber={OrderNumber}, PickupRetries={Retries}, " + + "PickupDelaySeconds={Delay} (max ~{Max}s including a {Initial}s initial delay).", + orderNumber, retries, delaySeconds, + Constants.Pickup.InitialDelaySeconds + retries * delaySeconds, Constants.Pickup.InitialDelaySeconds); + + try + { + // Small static delay before the first poll — mirrors the Sectigo connector's + // attempt to let a fast order finish issuing before we start polling at all. + await Task.Delay(TimeSpan.FromSeconds(Constants.Pickup.InitialDelaySeconds)); + + for (int attempt = 1; attempt <= retries; attempt++) + { + try + { + var cert = await _client.GetCertificateAsync(orderNumber); + int disposition = StatusMapper.ToRequestDisposition(cert.Status); + + // Issued: only surface GENERATED when the PEM is actually present — never + // hand Command a body-less "issued" record. A body-less issued state keeps + // polling until the body appears or the budget runs out. + if (disposition == (int)EndEntityStatus.GENERATED + && !string.IsNullOrWhiteSpace(cert.Certificate)) + { + _logger.LogInformation( + "Synchronous pickup complete. OrderNumber={OrderNumber}, SerialNumber={Serial}, " + + "Attempt={Attempt}/{Retries}.", + orderNumber, + string.IsNullOrWhiteSpace(cert.SerialNumber) ? "(none)" : cert.SerialNumber, + attempt, retries); + return new EnrollmentResult + { + CARequestID = string.IsNullOrWhiteSpace(cert.Id) ? orderNumber : cert.Id, + Certificate = cert.Certificate, + Status = (int)EndEntityStatus.GENERATED, + StatusMessage = $"Certificate issued successfully. CERTInext ID: {orderNumber}." + }; + } + + // Terminal non-issued outcomes carry no body and are surfaced immediately. + if (disposition == (int)EndEntityStatus.REVOKED + || disposition == (int)EndEntityStatus.FAILED) + { + _logger.LogInformation( + "Order {OrderNumber} reached terminal status '{Status}' during synchronous pickup.", + orderNumber, cert.Status); + return new EnrollmentResult + { + CARequestID = string.IsNullOrWhiteSpace(cert.Id) ? orderNumber : cert.Id, + Certificate = cert.Certificate, + Status = disposition, + StatusMessage = $"Order {orderNumber} reached status '{cert.Status}' during enrollment pickup." + }; + } + } + catch (Exception ex) + { + // A transient fetch failure consumes an attempt rather than aborting the + // wait; if it never recovers the pending result is returned below. + _logger.LogWarning(ex, + "Pickup GetCertificate failed for order {OrderNumber} (attempt {Attempt}/{Retries}).", + orderNumber, attempt, retries); + } + + // Delay after every attempt (including the last), matching the Sectigo + // connector's pickup cadence so the max-occupancy ceiling is identical. + await Task.Delay(TimeSpan.FromSeconds(delaySeconds)); + } + + _logger.LogInformation( + "Synchronous pickup did not complete within {Retries} attempts for order {OrderNumber}. " + + "Returning pending result; the certificate will be imported by the next synchronization. " + + "CERTInext issues OV/EV asynchronously by design (support ticket #162763).", + retries, orderNumber); + pendingResult.StatusMessage = + $"{pendingResult.StatusMessage} The certificate was not issued within the enrollment-pickup " + + "window; it will be imported by a later synchronization."; + } + catch (Exception ex) + { + _logger.LogWarning(ex, + "Synchronous pickup failed for order {OrderNumber}. Returning pending result; " + + "sync will pick up the certificate later.", orderNumber); + } + + return pendingResult; + } + /// /// Converts a CERTInext API enrollment/renewal response into the /// expected by the AnyCA gateway. diff --git a/CERTInext/CERTInextCAPluginConfig.cs b/CERTInext/CERTInextCAPluginConfig.cs index 43d0537..baf86c9 100644 --- a/CERTInext/CERTInextCAPluginConfig.cs +++ b/CERTInext/CERTInextCAPluginConfig.cs @@ -272,6 +272,29 @@ public static Dictionary GetCAConnectorAnnotations() DefaultValue = true, Type = "Boolean" }, + [Constants.Config.PickupRetries] = new PropertyConfigInfo + { + Comments = "OPTIONAL: Number of times Enroll() will poll CERTInext to download the certificate after a " + + "successful order submission. If the certificate has not issued within this window it is " + + "picked up during the next synchronization instead. Set to 0 to disable the wait. " + + $"Default: {Constants.Pickup.DefaultRetries}. NOTE: CERTInext issues OV/EV certificates " + + "asynchronously (organization verification, minutes to hours), so those typically exhaust " + + "the wait and are returned pending regardless of this value.", + Hidden = false, + DefaultValue = Constants.Pickup.DefaultRetries, + Type = "Number" + }, + [Constants.Config.PickupDelay] = new PropertyConfigInfo + { + Comments = "OPTIONAL: Number of seconds between certificate-pickup retries. The total number of retries " + + "times this delay (plus a short initial delay) is the maximum time an enrollment call " + + "occupies a Command worker thread. If the duration is too long the request may time out, so " + + $"keep the total well under ~90s. Default: {Constants.Pickup.DefaultDelaySeconds} " + + $"(with default retries this yields a ~{Constants.Pickup.InitialDelaySeconds + Constants.Pickup.DefaultRetries * Constants.Pickup.DefaultDelaySeconds}s ceiling).", + Hidden = false, + DefaultValue = Constants.Pickup.DefaultDelaySeconds, + Type = "Number" + }, [Constants.Config.DcvEnabled] = new PropertyConfigInfo { Comments = "OPTIONAL: When true, the gateway will perform DNS-based Domain Control Validation (DCV) " + @@ -695,6 +718,23 @@ public class CERTInextConfig /// Seconds to wait after publishing the DNS TXT record before calling VerifyDcv. /// Default: 30. /// + /// + /// Number of GetCertificate poll attempts inside Enroll() after an order is + /// submitted, before falling back to a pending result (picked up by the next sync). + /// Mirrors the legacy Sectigo connector's PickupRetries. Set to 0 to disable. + /// Default: 5. + /// + [JsonPropertyName("PickupRetries")] + public int PickupRetries { get; set; } = Constants.Pickup.DefaultRetries; + + /// + /// Seconds between certificate-pickup retries. PickupRetries * PickupDelay (plus a + /// short initial delay) bounds the time an enrollment call occupies a Command worker + /// thread. Mirrors the legacy Sectigo connector's PickupDelay. Default: 10. + /// + [JsonPropertyName("PickupDelay")] + public int PickupDelayInSeconds { get; set; } = Constants.Pickup.DefaultDelaySeconds; + [JsonPropertyName("DcvPropagationDelaySeconds")] public int DcvPropagationDelaySeconds { get; set; } = 30; @@ -782,5 +822,22 @@ public int GetEffectiveDcvWaitForIssuanceSeconds() return envVal; return DcvWaitForIssuanceSeconds >= 0 ? DcvWaitForIssuanceSeconds : 60; } + + /// + /// Effective number of certificate-pickup retries, clamped to + /// [0, ]. 0 disables the synchronous pickup. + /// + public int GetEffectivePickupRetries() + => System.Math.Max(0, System.Math.Min(PickupRetries, Constants.Pickup.MaxRetries)); + + /// + /// Effective seconds between pickup retries, clamped to + /// [1, ]. A non-positive configured value + /// falls back to the default rather than producing a tight busy-loop. + /// + public int GetEffectivePickupDelaySeconds() + => System.Math.Max(1, System.Math.Min( + PickupDelayInSeconds > 0 ? PickupDelayInSeconds : Constants.Pickup.DefaultDelaySeconds, + Constants.Pickup.MaxDelaySeconds)); } } diff --git a/CERTInext/Constants.cs b/CERTInext/Constants.cs index 83e6929..abf5188 100644 --- a/CERTInext/Constants.cs +++ b/CERTInext/Constants.cs @@ -21,6 +21,16 @@ public static class Config public const string Enabled = "Enabled"; public const string IgnoreExpired = "IgnoreExpired"; public const string PageSize = "PageSize"; + + // Synchronous certificate pickup (parity with the legacy Sectigo connector). + // After submitting an order, Enroll() polls GetCertificate up to PickupRetries + // times, PickupDelay seconds apart (after a fixed initial delay), so a fast-issuing + // order returns the issued certificate in the same enrollment call instead of + // waiting for the next synchronization. On timeout the order is returned pending and + // imported by a later sync — behaviour identical to before this feature. + public const string PickupRetries = "PickupRetries"; + public const string PickupDelay = "PickupDelay"; + public const string RequestorName = "RequestorName"; public const string RequestorEmail = "RequestorEmail"; public const string RequestorIsdCode = "RequestorIsdCode"; @@ -268,6 +278,26 @@ public static class RevocationReasonId public const int Default = KeyCompromise; } + public static class Pickup + { + // Defaults mirror the legacy Sectigo connector's PickUpEnrolledCertificate: + // a 5-second initial delay, then up to 5 poll attempts 10 seconds apart, so the + // maximum time an enrollment call occupies a Command worker thread is + // InitialDelaySeconds + DefaultRetries * DefaultDelaySeconds = 5 + 5*10 = 55 seconds. + // Set PickupRetries to 0 to disable the wait entirely (immediate pending return). + public const int DefaultRetries = 5; + public const int DefaultDelaySeconds = 10; + + // Small static delay before the first poll — gives a fast order a chance to finish + // issuing before we poll at all, avoiding a guaranteed-miss first attempt. + public const int InitialDelaySeconds = 5; + + // Safety clamps so a mis-configured connector cannot orphan a worker thread. Command + // abandons enrollment calls well before these bounds; they only backstop absurd input. + public const int MaxRetries = 30; + public const int MaxDelaySeconds = 60; + } + public static class Dcv { // CERTInext dcvMethod values (dcvDetails.dcvMethod in GetDcv / VerifyDcv) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6065cb2..44022a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +# 1.0.1 + +## Features +- feat(enroll): `Enroll()` now polls for the issued certificate after submitting an order, so fast-issuing (DV / already-approved) orders return in the same call instead of waiting for the next sync. Tunable via `PickupRetries` (default 5, `0` disables) and `PickupDelay` (default 10s); ~55s default ceiling. Orders not issued within the window are returned pending and imported by a later sync, as before. + # 1.0.0 Initial release of the CERTInext (emSign Hub) AnyCA REST Gateway plugin. From 77fe123b6ffc445efa4227294b052978cdf91ffc Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 31 Jul 2026 10:27:27 -0700 Subject: [PATCH 02/10] chore(enroll): log RequestFormat on the enrollment-start line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RequestFormat is received by Enroll() but was never logged, so logs could not show what Command passes for CSR vs PFX enrollments. Add it to the enrollment-start Information line for diagnostics. Behavior unchanged — the value is still not used for any decision (the gateway treats every enrollment as a CSR-based request). --- CERTInext/CERTInextCAPlugin.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index df51796..52fc364 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -573,10 +573,10 @@ public async Task Enroll( _logger.LogInformation( "Enrollment attempt started. " + - "EnrollmentType={EnrollmentType}, Subject={Subject}, " + + "EnrollmentType={EnrollmentType}, RequestFormat={RequestFormat}, Subject={Subject}, " + "ProfileId={ProfileId}, SANs={SANs}, " + "RequesterName={RequesterName}, RequesterEmail={RequesterEmail}", - enrollmentType, subject, + enrollmentType, requestFormat, subject, ep.ProfileId, sanSummary, ep.RequesterName, ep.RequesterEmail); From 49616cc83e640385c9731ff3da8e5ca4f269fc6b Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 31 Jul 2026 13:14:46 -0700 Subject: [PATCH 03/10] fix(client): don't retry non-idempotent order/CSR submits on a network timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A network-level timeout on GenerateOrderSSL / SubmitCSR can occur after CERTInext has already received and created the order. The inner HTTP retry re-sent the same request body (same requestTxn), which CERTInext rejected as EMS-947 "Duplicate requestTxn" — failing the enrollment while orphaning the created order. ExecuteWithRetryAsync gains an `idempotent` flag; PlaceOrderAsync and SubmitCsrAsync now submit once (idempotent:false). A transient submit failure and an EMS-947 duplicate are each logged as an explicit no-retry decision and surfaced with a clear, conditional message (if an order was created it is imported by the next sync). Idempotent read calls are unchanged and still retry. --- CERTInext/Client/CERTInextClient.cs | 80 ++++++++++++++++++++++++++--- CHANGELOG.md | 4 ++ 2 files changed, 78 insertions(+), 6 deletions(-) diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index 255b65a..edc9970 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -216,7 +216,11 @@ public async Task PlaceOrderAsync( req.AddJsonBody(JsonSerializer.Serialize(request, GetJsonOptions())); var sw = System.Diagnostics.Stopwatch.StartNew(); - resp = await ExecuteWithRetryAsync(req, ct); + // idempotent:false — order submission is non-idempotent. A network-level + // timeout may occur after CERTInext already created the order, so re-sending the + // same requestTxn would be rejected as EMS-947 and orphan the created order + // Rate-limit retries are still handled below (with a fresh txn). + resp = await ExecuteWithRetryAsync(req, ct, idempotent: false); sw.Stop(); Logger.LogInformation( @@ -232,6 +236,25 @@ public async Task PlaceOrderAsync( $"Authentication failure during certificate order. HTTP {(int)resp.StatusCode}. See gateway logs for details."); } + // Transient/network failure (5xx or no HTTP status) on a non-idempotent submit: + // CERTInext may have already created the order (the response just didn't reach us). + // We deliberately did not retry (see idempotent:false above). Fail clearly instead + // of deserializing an empty body; if the order was created, the next sync imports it. + bool transientFailure = !resp.IsSuccessful + && !((int)resp.StatusCode >= 400 && (int)resp.StatusCode < 500); + if (transientFailure) + { + Logger.LogWarning( + "PlaceOrder received no usable response (HttpStatus={Status}, LatencyMs={Latency}). " + + "Not retrying to avoid a duplicate order (EMS-947). If CERTInext created the order it " + + "will be imported by the next synchronization.", + (int)resp.StatusCode, sw.ElapsedMilliseconds); + throw new Exception( + "CERTInext did not return a usable response to the order submission. If the order was " + + "created it will be imported by the next synchronization — do not resubmit immediately. " + + "See gateway logs for details."); + } + result = DeserializeOrThrow(resp, "place order"); if (result.Meta != null && !result.Meta.IsSuccess) @@ -259,6 +282,29 @@ public async Task PlaceOrderAsync( continue; // retry } + // EMS-947 "Duplicate requestTxn": CERTInext already received an order for this + // transaction. With the non-idempotent-retry fix above this should no longer be + // caused by our own retry, but if it still surfaces the order exists on the CA + // side and will be imported by the next sync — say so, not a generic failure. + bool isDuplicateTxn = + string.Equals(result.Meta.ErrorCode, "EMS-947", StringComparison.OrdinalIgnoreCase) + || (result.Meta.ErrorMessage?.IndexOf("Duplicate requestTxn", StringComparison.OrdinalIgnoreCase) >= 0); + if (isDuplicateTxn) + { + // Log the classification decision itself (parity with the transient-failure + // branch above) so an auditor sees the plugin deliberately treated this as a + // benign duplicate rather than a hard failure. + Logger.LogWarning( + "PlaceOrder classified {ErrorCode} as a duplicate transaction (not a hard failure). " + + "Path={Path}, HttpStatus={Status}, LatencyMs={Latency}. If an order exists for this " + + "transaction it will be imported by the next synchronization.", + result.Meta.ErrorCode, Constants.Api.GenerateOrderSslPath, (int)resp.StatusCode, sw.ElapsedMilliseconds); + throw new Exception( + "CERTInext reported a duplicate order transaction (EMS-947). If an order was created " + + "for this transaction it will be imported by the next synchronization — do not resubmit " + + "immediately. See gateway logs for details."); + } + throw new Exception( $"CERTInext order failed: {result.Meta.ErrorMessage ?? result.Meta.ErrorCode}. " + "See gateway logs for details."); @@ -300,7 +346,9 @@ public async Task SubmitCsrAsync(SubmitCsrRequest request, CancellationToken ct req.AddJsonBody(JsonSerializer.Serialize(request, GetJsonOptions())); var sw = System.Diagnostics.Stopwatch.StartNew(); - var resp = await ExecuteWithRetryAsync(req, ct); + // idempotent:false — submitting a CSR is non-idempotent; do not resend on a network + // timeout (the first attempt may have been received). See PlaceOrderAsync. + var resp = await ExecuteWithRetryAsync(req, ct, idempotent: false); sw.Stop(); Logger.LogInformation( @@ -310,6 +358,17 @@ public async Task SubmitCsrAsync(SubmitCsrRequest request, CancellationToken ct if (!resp.IsSuccessful) { LogApiFailure(Constants.Api.SubmitCsrPath, resp); + // Parity with PlaceOrderAsync: a transient/network failure on this non-idempotent + // submit was NOT retried, so record that decision (the CSR may already have been + // received). 4xx client errors fall through to the generic failure below. + bool transientFailure = !((int)resp.StatusCode >= 400 && (int)resp.StatusCode < 500); + if (transientFailure) + { + Logger.LogWarning( + "SubmitCSR received no usable response (HttpStatus={Status}, LatencyMs={Latency}); not retrying " + + "(non-idempotent). If CERTInext already received the CSR, do not resubmit immediately.", + (int)resp.StatusCode, sw.ElapsedMilliseconds); + } throw new Exception($"CERTInext SubmitCSR failed. HTTP {(int)resp.StatusCode}. See gateway logs for details."); } @@ -1213,14 +1272,23 @@ private async Task GetOrRefreshTokenAsync(CancellationToken ct) /// attempts, retrying on HTTP 5xx and network-level failures (no status code). /// 4xx responses are returned immediately — client errors will not be resolved /// by retrying. + /// + /// When is false the request is sent exactly + /// once and transient failures are NOT retried. This is required for non-idempotent + /// order-submission calls: a network-level timeout can occur *after* CERTInext has + /// already received and created the order, so re-sending the same body (same + /// requestTxn) is rejected as "Duplicate requestTxn" (EMS-947) and orphans the + /// order the first attempt actually created. /// private async Task ExecuteWithRetryAsync( RestRequest req, CancellationToken ct, - int maxAttempts = 3) + int maxAttempts = 3, + bool idempotent = true) { + int attempts = idempotent ? maxAttempts : 1; RestResponse resp = null; - for (int attempt = 1; attempt <= maxAttempts; attempt++) + for (int attempt = 1; attempt <= attempts; attempt++) { resp = await _http.ExecuteAsync(req, ct); @@ -1229,11 +1297,11 @@ private async Task ExecuteWithRetryAsync( if (resp.IsSuccessful || isClientError) return resp; - if (attempt < maxAttempts) + if (attempt < attempts) { Logger.LogWarning( "CERTInext API returned {Status} on attempt {Attempt}/{Max} — retrying...", - (int)resp.StatusCode, attempt, maxAttempts); + (int)resp.StatusCode, attempt, attempts); } } diff --git a/CHANGELOG.md b/CHANGELOG.md index 44022a4..e53dcd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Features - feat(enroll): `Enroll()` now polls for the issued certificate after submitting an order, so fast-issuing (DV / already-approved) orders return in the same call instead of waiting for the next sync. Tunable via `PickupRetries` (default 5, `0` disables) and `PickupDelay` (default 10s); ~55s default ceiling. Orders not issued within the window are returned pending and imported by a later sync, as before. +- chore(enroll): The enrollment-start log line now includes `RequestFormat` for diagnostics. + +## Bug Fixes +- fix(client): Order submission (`GenerateOrderSSL`) and CSR submission are no longer auto-retried on a network-level timeout. Because a timeout can occur after the CA has already created the order, re-sending the same transaction was being rejected as a duplicate (`EMS-947 "Duplicate requestTxn"`), failing the enrollment while orphaning the created order. Non-idempotent submissions now run once; if the CA created the order it is imported by the next synchronization. A duplicate-transaction response is also now reported with a clear, actionable message. (Idempotent read calls are unaffected and still retry.) # 1.0.0 From e8e47391ec26262f6fb8c0d03c12e1356a32f474 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 31 Jul 2026 13:23:46 -0700 Subject: [PATCH 04/10] fix(client): enrich orphaned-order warnings + SubmitCSR transient guidance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Compliance follow-ups (both Low): - Add DomainName as a non-sensitive correlation key to the PlaceOrder transient and EMS-947 warnings so an orphaned order can be tied to its enrollment under concurrency (requestTxn is deliberately NOT logged — it is part of the authKey preimage). - SubmitCSR now carries the "may already have been received; do not resubmit" guidance in the thrown exception on a transient failure, for parity with PlaceOrderAsync (previously only in the log line). --- CERTInext/Client/CERTInextClient.cs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index edc9970..668c4a3 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -245,10 +245,10 @@ public async Task PlaceOrderAsync( if (transientFailure) { Logger.LogWarning( - "PlaceOrder received no usable response (HttpStatus={Status}, LatencyMs={Latency}). " + + "PlaceOrder received no usable response (DomainName={Domain}, HttpStatus={Status}, LatencyMs={Latency}). " + "Not retrying to avoid a duplicate order (EMS-947). If CERTInext created the order it " + "will be imported by the next synchronization.", - (int)resp.StatusCode, sw.ElapsedMilliseconds); + request.OrderDetails?.CertificateInformation?.DomainName, (int)resp.StatusCode, sw.ElapsedMilliseconds); throw new Exception( "CERTInext did not return a usable response to the order submission. If the order was " + "created it will be imported by the next synchronization — do not resubmit immediately. " + @@ -296,9 +296,9 @@ public async Task PlaceOrderAsync( // benign duplicate rather than a hard failure. Logger.LogWarning( "PlaceOrder classified {ErrorCode} as a duplicate transaction (not a hard failure). " + - "Path={Path}, HttpStatus={Status}, LatencyMs={Latency}. If an order exists for this " + - "transaction it will be imported by the next synchronization.", - result.Meta.ErrorCode, Constants.Api.GenerateOrderSslPath, (int)resp.StatusCode, sw.ElapsedMilliseconds); + "DomainName={Domain}, Path={Path}, HttpStatus={Status}, LatencyMs={Latency}. If an order exists " + + "for this transaction it will be imported by the next synchronization.", + result.Meta.ErrorCode, request.OrderDetails?.CertificateInformation?.DomainName, Constants.Api.GenerateOrderSslPath, (int)resp.StatusCode, sw.ElapsedMilliseconds); throw new Exception( "CERTInext reported a duplicate order transaction (EMS-947). If an order was created " + "for this transaction it will be imported by the next synchronization — do not resubmit " + @@ -368,6 +368,11 @@ public async Task SubmitCsrAsync(SubmitCsrRequest request, CancellationToken ct "SubmitCSR received no usable response (HttpStatus={Status}, LatencyMs={Latency}); not retrying " + "(non-idempotent). If CERTInext already received the CSR, do not resubmit immediately.", (int)resp.StatusCode, sw.ElapsedMilliseconds); + // Parity with PlaceOrderAsync: carry the actionable guidance into the surfaced + // exception, not only the log line. + throw new Exception( + "CERTInext did not return a usable response to the CSR submission. If the CSR was received " + + "it will take effect — do not resubmit immediately. See gateway logs for details."); } throw new Exception($"CERTInext SubmitCSR failed. HTTP {(int)resp.StatusCode}. See gateway logs for details."); } From 6ae12b762884f17483f09a11e21c721433103ba5 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 31 Jul 2026 14:28:07 -0700 Subject: [PATCH 05/10] =?UTF-8?q?fix(enroll):=20harden=20synchronous=20pic?= =?UTF-8?q?kup=20=E2=80=94=20DCV=20gating,=20wait=20ceiling,=20audit=20log?= =?UTF-8?q?ging?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-driven refinements to the v1.0.1 synchronous-pickup feature: - Skip the pickup poll when the DCV path already owns the in-call issuance wait, so the two never stack and a cancelled/rejected order is not re-polled for the full window (fixes a regression that broke the terminal-order guard). - Cap total in-call pickup wait at 180s regardless of how PickupRetries and PickupDelay are configured, so an aggressive combination can't exceed Command's enrollment timeout. - Log a terminal FAILED at Error and REVOKED at Warning; trace each poll at Debug; distinguish "all polls errored" from "still pending" in the timeout summary; include OrderNumber in the CSR transient-failure warning; surface a pending result that has no order number to poll instead of skipping silently. - Default pickup off in the unit-test fixtures and add targeted pickup tests (disabled / issued / terminal / budget-exhausted). Both flavors build clean (0 warnings); DCV 199/199, no-DCV 176/176. --- CERTInext.Tests/CERTInextCAPluginDcvTests.cs | 22 ++-- CERTInext.Tests/CERTInextCAPluginTests.cs | 107 ++++++++++++++++- CERTInext/CERTInextCAPlugin.cs | 119 ++++++++++++++++--- CERTInext/CERTInextCAPluginConfig.cs | 10 +- CERTInext/Client/CERTInextClient.cs | 7 +- CERTInext/Constants.cs | 12 +- CHANGELOG.md | 5 +- 7 files changed, 248 insertions(+), 34 deletions(-) diff --git a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs index 837ae8d..d812074 100644 --- a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs @@ -35,7 +35,8 @@ private static CERTInextConfig DcvConfig( int propagationDelaySeconds = 1, int timeoutMinutes = 1, int dcvWaitForChallengeSeconds = 0, - int dcvWaitForIssuanceSeconds = 0) => + int dcvWaitForIssuanceSeconds = 0, + int pickupRetries = 0) => new CERTInextConfig { DcvEnabled = enabled, @@ -45,7 +46,12 @@ private static CERTInextConfig DcvConfig( // behaviour and run fast. Tests that exercise the new wait paths can opt // in with a positive value (see WaitsForChallenge_ToAppear / WaitsForIssuance). DcvWaitForChallengeSeconds = dcvWaitForChallengeSeconds, - DcvWaitForIssuanceSeconds = dcvWaitForIssuanceSeconds + DcvWaitForIssuanceSeconds = dcvWaitForIssuanceSeconds, + // Disable the synchronous pickup poll by default (same reasoning as the wait + // budgets above): the DCV path owns issuance for these tests, and a DCV-disabled + // or no-factory case that ends on a pending result must not pay the real pickup + // Task.Delay loop. The dedicated pickup tests live in CERTInextCAPluginTests. + PickupRetries = pickupRetries }; private static Mock NewMock() => @@ -437,17 +443,19 @@ public async Task Dcv_Skipped_WhenOrderStatusIdIsTerminal_EvenIfDcvValidated(str }); var validator = new FakeDomainValidator(); - // Issuance-wait budget > 0 so a wrong-path entry would manifest as a - // GetCertificate call we DON'T expect. + // Issuance-wait budget > 0 AND pickup ENABLED (pickupRetries > 0) so a wrong-path + // entry would manifest as a GetCertificate call we DON'T expect — this test must + // fail if either the DCV issuance-wait guard OR the synchronous-pickup gate + // (dcvIssuanceWaitRan) regresses and starts polling a cancelled/rejected order. var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator), - DcvConfig(dcvWaitForIssuanceSeconds: 10)); + DcvConfig(dcvWaitForIssuanceSeconds: 10, pickupRetries: 5)); await Enroll(plugin); mock.Verify(c => c.GetCertificateAsync(It.IsAny(), It.IsAny()), Times.Never, - "Enroll must not enter WaitForIssuanceAfterDcvAsync when the order is " + - "cancelled/rejected, even if DCV happens to be in a 'validated' state"); + "Enroll must not enter WaitForIssuanceAfterDcvAsync OR the synchronous pickup poll " + + "when the order is cancelled/rejected, even if DCV happens to be in a 'validated' state"); validator.StagedRecords.Should().BeEmpty( "DCV staging must not run for a cancelled/rejected order"); } diff --git a/CERTInext.Tests/CERTInextCAPluginTests.cs b/CERTInext.Tests/CERTInextCAPluginTests.cs index 3ec5df1..7064b44 100644 --- a/CERTInext.Tests/CERTInextCAPluginTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginTests.cs @@ -31,8 +31,20 @@ public class CERTInextCAPluginTests // Helpers // --------------------------------------------------------------------------- + // Pickup is disabled by default in the broad fixture (PickupRetries=0) — mirroring how + // DcvConfig defaults its wait budgets to 0 — so tests that don't care about the + // synchronous pickup don't pay its real Task.Delay-based poll. Tests that DO exercise + // pickup opt in via BuildPluginWithPickup. private static CERTInextCAPlugin BuildPlugin(ICERTInextClient client) => - new CERTInextCAPlugin(client); + new CERTInextCAPlugin(client, new CERTInextConfig { PickupRetries = 0 }); + + // Pickup-enabled fixture for the synchronous-pickup tests. PickupDelay is clamped to a + // 1s floor and the loop adds a fixed 5s initial delay, so these tests are intentionally + // a few seconds each. + private static CERTInextCAPlugin BuildPluginWithPickup( + ICERTInextClient client, int retries, int delaySeconds = 1) => + new CERTInextCAPlugin(client, + new CERTInextConfig { PickupRetries = retries, PickupDelayInSeconds = delaySeconds }); private static Mock NewMock() => new Mock(MockBehavior.Strict); @@ -345,6 +357,99 @@ public async Task Enroll_New_ReturnsPendingStatus_WhenCaReturnsPendingApproval() result.Status.Should().Be((int)EndEntityStatus.EXTERNALVALIDATION); } + // --------------------------------------------------------------------------- + // Synchronous certificate pickup (Sectigo parity) + // --------------------------------------------------------------------------- + + [Fact] + public async Task Pickup_Disabled_WhenPickupRetriesZero_ReturnsPendingWithoutPolling() + { + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(MockCertificateData.PendingEnrollResponse()); + + var plugin = BuildPluginWithPickup(mock.Object, retries: 0); + + var result = await plugin.Enroll( + csr: MockCertificateData.FakeCsrPem, subject: "CN=test.example.com", san: null, + productInfo: MakeProductInfo(), requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + result.Status.Should().Be((int)EndEntityStatus.EXTERNALVALIDATION); + mock.Verify(c => c.GetCertificateAsync(It.IsAny(), It.IsAny()), + Times.Never, "PickupRetries=0 must disable the synchronous pickup poll"); + } + + [Fact] + public async Task Pickup_ReturnsIssuedCert_WhenOrderIssuesDuringPoll() + { + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(MockCertificateData.PendingEnrollResponse()); + // The order finishes issuing by the time we poll: GetCertificate reports issued + PEM. + mock.Setup(c => c.GetCertificateAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(MockCertificateData.IssuedCertRecord()); + + var plugin = BuildPluginWithPickup(mock.Object, retries: 2); + + var result = await plugin.Enroll( + csr: MockCertificateData.FakeCsrPem, subject: "CN=test.example.com", san: null, + productInfo: MakeProductInfo(), requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + result.Status.Should().Be((int)EndEntityStatus.GENERATED); + result.Certificate.Should().NotBeNullOrEmpty("a synchronously-picked-up cert must carry its PEM"); + mock.Verify(c => c.GetCertificateAsync(It.IsAny(), It.IsAny()), + Times.AtLeastOnce); + } + + [Fact] + public async Task Pickup_SurfacesTerminalStatus_WhenOrderRevokedDuringPoll() + { + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(MockCertificateData.PendingEnrollResponse()); + mock.Setup(c => c.GetCertificateAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(MockCertificateData.RevokedCertRecord()); + + var plugin = BuildPluginWithPickup(mock.Object, retries: 3); + + var result = await plugin.Enroll( + csr: MockCertificateData.FakeCsrPem, subject: "CN=test.example.com", san: null, + productInfo: MakeProductInfo(), requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + result.Status.Should().Be((int)EndEntityStatus.REVOKED, + "a terminal status observed during pickup is surfaced immediately, not polled to exhaustion"); + } + + [Fact] + public async Task Pickup_ReturnsPending_WhenOrderNeverIssuesWithinBudget() + { + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(MockCertificateData.PendingEnrollResponse()); + // Every poll still reports pending — the budget is exhausted and Enroll returns the + // pending result for a later sync to complete. + mock.Setup(c => c.GetCertificateAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(MockCertificateData.PendingCertRecord()); + + var plugin = BuildPluginWithPickup(mock.Object, retries: 1); + + var result = await plugin.Enroll( + csr: MockCertificateData.FakeCsrPem, subject: "CN=test.example.com", san: null, + productInfo: MakeProductInfo(), requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + result.Status.Should().Be((int)EndEntityStatus.EXTERNALVALIDATION); + mock.Verify(c => c.GetCertificateAsync(It.IsAny(), It.IsAny()), + Times.AtLeastOnce, "an enabled pickup must actually poll before giving up"); + } + [Fact] public async Task Enroll_New_Throws_WhenProfileIdNotSet() { diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index 52fc364..b04c051 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -1105,12 +1105,38 @@ private async Task EnrollNewAsync( var enrollResp = await _client.EnrollCertificateAsync(enrollReq); + // Whether the DCV block below took ownership of the in-call issuance wait for this + // order. Declared outside the #if so both build flavors compile the pickup gate the + // same way (it simply stays false on the no-DCV build). When true, the synchronous + // pickup poll is skipped: on the DCV build the DCV path already owns the issuance + // decision — it either ran WaitForIssuanceAfterDcvAsync itself, deferred to another + // in-flight caller, or determined the order is terminal / not yet validated — so a + // second stacked poll would either double the wait or burn the budget polling an + // order that can never issue in-call (regression guard: a cancelled/rejected order + // must not be re-polled here after DCV already short-circuited it). + bool dcvIssuanceWaitRan = false; + #if SUPPORTS_DCV // DCV: run domain validation if enabled, the factory was injected, and the // order was accepted (not immediately failed). string orderNumber = enrollResp.Id; if (_domainValidatorFactory != null && _config.DcvEnabled && !string.IsNullOrEmpty(orderNumber)) { + // DCV owns the in-call issuance wait for this order from here on: every exit from + // this block (duplicate in-flight, DCV-validated + issuance poll, terminal order, + // or challenge-not-yet-exposed) is a decision the pickup poll must not second-guess. + // Set before any await so it holds on every path out of the block. + // + // This is intentionally coarse — keyed on "the DCV subsystem engaged for this order", + // not on "a DCV wait is actively running". The one case it over-defers is an order + // whose pending domains are all assigned to a non-DNS-01 method (HTTP/email): DCV does + // no work, yet pickup is skipped. That is an accepted trade: this plugin only drives + // DNS-01, so such orders depend on out-of-band validation and would not issue within + // the ~55s pickup window anyway — the next sync completes them. Distinguishing that + // sub-case from the terminal/cancelled case (which MUST skip pickup) would require a + // richer PerformDcvIfNeededAsync result and risk re-opening the terminal-order regression. + dcvIssuanceWaitRan = true; + // SOX CC7.3: bound the entire DCV flow with a hard timeout so a stuck // DNS provider or extreme propagation delay cannot hold a gateway worker // thread indefinitely. Configurable via DcvTimeoutMinutes (config or @@ -1175,7 +1201,7 @@ private async Task EnrollNewAsync( // already-issued/failed case and for OV/EV orders that CERTInext issues asynchronously // — those fall back to the pending result and are imported by the next sync. var newResult = BuildEnrollmentResult(enrollResp, ep.AutoApprove); - newResult = await PickUpEnrolledCertificateAsync(newResult, enrollResp.Id); + newResult = await PickUpEnrolledCertificateAsync(newResult, enrollResp.Id, dcvIssuanceWaitRan); _logger.MethodExit(LogLevel.Debug); return newResult; @@ -1305,7 +1331,8 @@ private async Task RenewOrReissueAsync( priorCaRequestId, renewResult.CARequestID, renewResult.Status); // Synchronous certificate pickup (Sectigo-parity), same as the new-enrollment path. - renewResult = await PickUpEnrolledCertificateAsync(renewResult, renewResp.Id); + // The renew path never runs an in-call DCV issuance wait, so pickup always applies. + renewResult = await PickUpEnrolledCertificateAsync(renewResult, renewResp.Id, dcvIssuanceWaitRan: false); return renewResult; } @@ -1894,15 +1921,31 @@ private async Task WaitForDcvVerificationAsync(string orderNumber, IReadOnlyList /// orders return in-call. Never throws — any polling error degrades to the pending result. /// private async Task PickUpEnrolledCertificateAsync( - EnrollmentResult pendingResult, string orderNumber) + EnrollmentResult pendingResult, string orderNumber, bool dcvIssuanceWaitRan) { + // The DCV path already owns the in-call issuance wait for this order — running a second + // stacked poll here would double the wait budget (when DCV ran WaitForIssuanceAfterDcvAsync) + // or waste it polling an order DCV already found terminal / not-yet-validated. Defer to + // the pending result; a later sync completes it. + if (dcvIssuanceWaitRan) + return pendingResult; + // Only a still-pending (external-validation) result can benefit from a pickup poll. - // An already issued/failed/revoked result, or a missing order number, is returned as-is. + // An already issued/failed/revoked result is returned as-is. if (pendingResult == null - || pendingResult.Status != (int)EndEntityStatus.EXTERNALVALIDATION - || string.IsNullOrWhiteSpace(orderNumber)) + || pendingResult.Status != (int)EndEntityStatus.EXTERNALVALIDATION) return pendingResult; + // A pending result with no order number cannot be polled — surface the anomaly rather + // than silently returning, so an un-pollable pending state leaves an audit trace. + if (string.IsNullOrWhiteSpace(orderNumber)) + { + _logger.LogWarning( + "Synchronous pickup skipped: a pending enrollment was returned with no order " + + "number to poll. The certificate can only be reconciled by a later synchronization."); + return pendingResult; + } + int retries = _config.GetEffectivePickupRetries(); if (retries <= 0) { @@ -1913,12 +1956,30 @@ private async Task PickUpEnrolledCertificateAsync( } int delaySeconds = _config.GetEffectivePickupDelaySeconds(); + + // Hard ceiling on total in-call occupancy. PickupRetries and PickupDelay are each clamped + // independently, but their product can still reach ~30 min at the extremes — enough to push + // Enroll() past Command's own enrollment timeout. If the configured budget would exceed the + // ceiling, cap the retry count to fit; the remainder is imported by the next synchronization. + int maxPollRetries = Math.Max(1, + (Constants.Pickup.MaxTotalWaitSeconds - Constants.Pickup.InitialDelaySeconds) / delaySeconds); + if (retries > maxPollRetries) + { + _logger.LogInformation( + "Configured pickup budget (PickupRetries={Configured}, PickupDelaySeconds={Delay}) exceeds the " + + "{MaxTotal}s in-call ceiling; capping to {Capped} attempts. The certificate will be imported by " + + "the next synchronization if it has not issued by then.", + retries, delaySeconds, Constants.Pickup.MaxTotalWaitSeconds, maxPollRetries); + retries = maxPollRetries; + } + _logger.LogInformation( "Starting synchronous certificate pickup. OrderNumber={OrderNumber}, PickupRetries={Retries}, " + "PickupDelaySeconds={Delay} (max ~{Max}s including a {Initial}s initial delay).", orderNumber, retries, delaySeconds, Constants.Pickup.InitialDelaySeconds + retries * delaySeconds, Constants.Pickup.InitialDelaySeconds); + int pollErrors = 0; try { // Small static delay before the first poll — mirrors the Sectigo connector's @@ -1932,6 +1993,14 @@ private async Task PickUpEnrolledCertificateAsync( var cert = await _client.GetCertificateAsync(orderNumber); int disposition = StatusMapper.ToRequestDisposition(cert.Status); + // SOC2 CC7.3: record each poll's observed disposition so the issuance + // timeline is reconstructable (how many polls ran, what each returned). + _logger.LogDebug( + "Pickup poll observed status. OrderNumber={OrderNumber}, Attempt={Attempt}/{Retries}, " + + "MappedDisposition={Disposition}, Status='{Status}', BodyPresent={HasBody}.", + orderNumber, attempt, retries, disposition, cert.Status, + !string.IsNullOrWhiteSpace(cert.Certificate)); + // Issued: only surface GENERATED when the PEM is actually present — never // hand Command a body-less "issued" record. A body-less issued state keeps // polling until the body appears or the budget runs out. @@ -1957,9 +2026,19 @@ private async Task PickUpEnrolledCertificateAsync( if (disposition == (int)EndEntityStatus.REVOKED || disposition == (int)EndEntityStatus.FAILED) { - _logger.LogInformation( - "Order {OrderNumber} reached terminal status '{Status}' during synchronous pickup.", - orderNumber, cert.Status); + // SOX/SOC2 CC7.2: an issuance FAILURE must cross the error threshold that + // SIEM issuance-failure rules key on (parity with BuildEnrollmentResult's + // enroll-time FAILED handling); a REVOKED terminal state is a warning. + if (disposition == (int)EndEntityStatus.FAILED) + _logger.LogError( + "Order {OrderNumber} reached terminal FAILED status '{Status}' during " + + "synchronous pickup (attempt {Attempt}/{Retries}).", + orderNumber, cert.Status, attempt, retries); + else + _logger.LogWarning( + "Order {OrderNumber} was REVOKED ('{Status}') during synchronous pickup " + + "(attempt {Attempt}/{Retries}).", + orderNumber, cert.Status, attempt, retries); return new EnrollmentResult { CARequestID = string.IsNullOrWhiteSpace(cert.Id) ? orderNumber : cert.Id, @@ -1973,6 +2052,7 @@ private async Task PickUpEnrolledCertificateAsync( { // A transient fetch failure consumes an attempt rather than aborting the // wait; if it never recovers the pending result is returned below. + pollErrors++; _logger.LogWarning(ex, "Pickup GetCertificate failed for order {OrderNumber} (attempt {Attempt}/{Retries}).", orderNumber, attempt, retries); @@ -1983,11 +2063,22 @@ private async Task PickUpEnrolledCertificateAsync( await Task.Delay(TimeSpan.FromSeconds(delaySeconds)); } - _logger.LogInformation( - "Synchronous pickup did not complete within {Retries} attempts for order {OrderNumber}. " + - "Returning pending result; the certificate will be imported by the next synchronization. " + - "CERTInext issues OV/EV asynchronously by design (support ticket #162763).", - retries, orderNumber); + // SOC1 accuracy: don't attribute non-completion to "OV/EV async by design" when the + // real cause was every poll erroring (e.g. a CA-side TrackOrder outage). Distinguish + // the two so the log reflects what actually happened. + if (pollErrors == retries) + _logger.LogWarning( + "Synchronous pickup exhausted {Retries} attempts for order {OrderNumber} — ALL polls " + + "errored (see preceding warnings). Returning pending result; the next synchronization " + + "will re-attempt retrieval.", + retries, orderNumber); + else + _logger.LogInformation( + "Synchronous pickup did not complete within {Retries} attempts for order {OrderNumber} " + + "({Errors} poll error(s); remainder still pending). Returning pending result; the " + + "certificate will be imported by the next synchronization. CERTInext issues OV/EV " + + "asynchronously by design (support ticket #162763).", + retries, orderNumber, pollErrors); pendingResult.StatusMessage = $"{pendingResult.StatusMessage} The certificate was not issued within the enrollment-pickup " + "window; it will be imported by a later synchronization."; diff --git a/CERTInext/CERTInextCAPluginConfig.cs b/CERTInext/CERTInextCAPluginConfig.cs index baf86c9..e77ac68 100644 --- a/CERTInext/CERTInextCAPluginConfig.cs +++ b/CERTInext/CERTInextCAPluginConfig.cs @@ -286,10 +286,12 @@ public static Dictionary GetCAConnectorAnnotations() }, [Constants.Config.PickupDelay] = new PropertyConfigInfo { - Comments = "OPTIONAL: Number of seconds between certificate-pickup retries. The total number of retries " + - "times this delay (plus a short initial delay) is the maximum time an enrollment call " + - "occupies a Command worker thread. If the duration is too long the request may time out, so " + - $"keep the total well under ~90s. Default: {Constants.Pickup.DefaultDelaySeconds} " + + Comments = "OPTIONAL: Number of seconds between certificate-pickup retries. PickupRetries times this " + + "delay (plus a short initial delay) is the maximum time an enrollment call occupies a Command " + + "worker thread. If the duration is too long the request may time out, so target a total well " + + $"under ~90s. As a safety backstop the plugin additionally caps the effective total at " + + $"{Constants.Pickup.MaxTotalWaitSeconds}s regardless of how PickupRetries/PickupDelay are set, " + + $"reducing the retry count to fit. Default: {Constants.Pickup.DefaultDelaySeconds} " + $"(with default retries this yields a ~{Constants.Pickup.InitialDelaySeconds + Constants.Pickup.DefaultRetries * Constants.Pickup.DefaultDelaySeconds}s ceiling).", Hidden = false, DefaultValue = Constants.Pickup.DefaultDelaySeconds, diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index 668c4a3..c6ad56b 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -365,9 +365,10 @@ public async Task SubmitCsrAsync(SubmitCsrRequest request, CancellationToken ct if (transientFailure) { Logger.LogWarning( - "SubmitCSR received no usable response (HttpStatus={Status}, LatencyMs={Latency}); not retrying " + - "(non-idempotent). If CERTInext already received the CSR, do not resubmit immediately.", - (int)resp.StatusCode, sw.ElapsedMilliseconds); + "SubmitCSR received no usable response (OrderNumber={OrderNumber}, HttpStatus={Status}, " + + "LatencyMs={Latency}); not retrying (non-idempotent). If CERTInext already received the CSR, " + + "do not resubmit immediately.", + request.OrderDetails?.OrderNumber, (int)resp.StatusCode, sw.ElapsedMilliseconds); // Parity with PlaceOrderAsync: carry the actionable guidance into the surfaced // exception, not only the log line. throw new Exception( diff --git a/CERTInext/Constants.cs b/CERTInext/Constants.cs index abf5188..4510286 100644 --- a/CERTInext/Constants.cs +++ b/CERTInext/Constants.cs @@ -292,10 +292,18 @@ public static class Pickup // issuing before we poll at all, avoiding a guaranteed-miss first attempt. public const int InitialDelaySeconds = 5; - // Safety clamps so a mis-configured connector cannot orphan a worker thread. Command - // abandons enrollment calls well before these bounds; they only backstop absurd input. + // Per-factor safety clamps so a single mis-typed value cannot produce a tight busy-loop + // or an absurd per-attempt delay. These bound each knob independently; the *product* + // (retries * delay) is bounded separately by MaxTotalWaitSeconds below. public const int MaxRetries = 30; public const int MaxDelaySeconds = 60; + + // Hard ceiling on total in-call pickup occupancy (initial delay + retries * delay). + // The per-factor clamps above still permit a ~1805s product at the extremes, which could + // push Enroll() past Command's enrollment timeout; PickUpEnrolledCertificateAsync caps the + // effective retry count so the total never exceeds this. Kept comfortably under a typical + // enrollment timeout while leaving room for the documented ~90s default guidance. + public const int MaxTotalWaitSeconds = 180; } public static class Dcv diff --git a/CHANGELOG.md b/CHANGELOG.md index e53dcd8..d971478 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,10 @@ # 1.0.1 ## Features -- feat(enroll): `Enroll()` now polls for the issued certificate after submitting an order, so fast-issuing (DV / already-approved) orders return in the same call instead of waiting for the next sync. Tunable via `PickupRetries` (default 5, `0` disables) and `PickupDelay` (default 10s); ~55s default ceiling. Orders not issued within the window are returned pending and imported by a later sync, as before. -- chore(enroll): The enrollment-start log line now includes `RequestFormat` for diagnostics. +- **Faster enrollment for quickly-issued certificates.** Enrollment now waits briefly for the certificate and returns it in the same request when it issues fast (DV and already-approved orders), instead of always waiting for the next synchronization. Two new optional settings control the wait: `PickupRetries` (default 5; set to `0` to disable) and `PickupDelay` (default 10 seconds) — about a 55-second wait by default, with a built-in ceiling so it can't run long enough to time out the enrollment. Orders that don't issue in that window — including OV/EV, which CERTInext validates asynchronously over minutes to hours — return pending and are imported by a later sync, exactly as before. Works with or without DNS-based DCV. ## Bug Fixes -- fix(client): Order submission (`GenerateOrderSSL`) and CSR submission are no longer auto-retried on a network-level timeout. Because a timeout can occur after the CA has already created the order, re-sending the same transaction was being rejected as a duplicate (`EMS-947 "Duplicate requestTxn"`), failing the enrollment while orphaning the created order. Non-idempotent submissions now run once; if the CA created the order it is imported by the next synchronization. A duplicate-transaction response is also now reported with a clear, actionable message. (Idempotent read calls are unaffected and still retry.) +- **No more duplicate or orphaned orders after a network timeout.** Order and CSR submissions are no longer retried after a network timeout. A timeout can happen *after* the CA has already accepted the request, so the automatic retry was being rejected as a duplicate — failing the enrollment and leaving an orphaned order behind. These requests now run once; if the order was created it is imported by the next synchronization, and duplicate responses are reported with clear, actionable guidance. (Read-only calls are unaffected and still retry.) # 1.0.0 From f499f65993b20f79b27a039c2d8b8e726507c2ec Mon Sep 17 00:00:00 2001 From: spb <1661003+spbsoluble@users.noreply.github.com> Date: Thu, 13 Aug 2026 09:15:33 -0700 Subject: [PATCH 06/10] =?UTF-8?q?fix(enroll):=20UCC=20SANs=20never=20reach?= =?UTF-8?q?ed=20CERTInext=20=E2=80=94=20additionalDomains=20sent=20empty?= =?UTF-8?q?=20(#21)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(enroll): UCC SANs never reached CERTInext — additionalDomains sent empty Certificates enrolled through a UCC product came back holding only the CN, even though the requested SANs were present on the CSR and in the SAN data Command supplied. The names were being dropped inside the plugin, not by the CA. Root cause: the AnyCA REST Gateway keys its SAN dictionary "dnsname", but MapSanType only recognized "dns". Every DNS SAN was therefore typed "dnsname", which failed the DNS-only test in BuildAdditionalDomains, so certificateInformation.additionalDomains was null and JsonIgnore-WhenWritingNull removed the field from the order body entirely. Confirmed against a customer gateway log: Enrollment attempt started. ... SANs=dnsname:CLAUDIOTEST20.ucsd.edu; dnsname:CLAUDIOTEST20.ad.ucsd.edu The CSR did not compensate, because CERTInext ignores the CSR's subjectAltName extension outright — measured, see below. Changes: * MapSanType now recognizes the spellings the gateway actually sends: dnsname, rfc822name, ipaddress, uniformresourceidentifier (the short forms still work). * BuildSanList unions the gateway-supplied SANs with the SANs parsed out of the CSR (BouncyCastle, per the project crypto policy), de-duplicating on type+value case-insensitively. Parsing the CSR is not redundant with sending it: CERTInext will not read those names itself, so re-submitting them through additionalDomains is the only way a CSR-only SAN reaches the certificate. CSR parsing is non-throwing — an unparseable CSR falls back to the gateway set. * BuildAdditionalDomains no longer filters to DNS-only. Every requested SAN is submitted; discarding the non-DNS ones issued certificates quietly missing names the subscriber asked for, which is the worse failure. It also excludes the value already going out as domainName so the CN is not submitted twice. * Renewals carried no SANs at all and took their primary domain from the prior order's requestorName. RenewCertificateRequest now carries Subject + Sans, and the renewal order derives domainName from the subject CN with the old value as a logged fallback. * PlaceOrderAsync now logs domainName and additionalDomains. The absence of any outbound domain logging is what made this look like CA-side stripping: the gateway log recorded the SANs Command supplied and nothing about what was put on the wire. Measured CERTInext behaviour (SanSubmissionProbeTests, sandbox-us, product 844 OV SSL UCC) — these replace assumptions the old code encoded but never tested: * additionalDomains is what puts extra names on the order (CN + extra1 → both registered). * CERTInext IGNORES CSR SANs. A CSR carrying two DNS names with additionalDomains omitted produced an order with only the CN registered. This is the customer-facing root cause. * Non-DNS values are NOT rejected, contrary to what the DNS-only filter assumed. An email address, an IPv4 literal and an https URI were each accepted and registered verbatim as order domains, so such an order is created and then cannot pass validation rather than failing up front. The plugin warns accordingly. * Repeating the CN inside additionalDomains is accepted and collapsed by the CA, so our de-duplication is defence in depth rather than a requirement. Tests: 9 new unit tests drive plugin.Enroll through a real client against WireMock and assert on the JSON actually posted — a test of the mapping function alone would not have caught this, since the mapping "worked" and the loss happened in its interaction with the downstream filter. The live probe is opt-in behind CERTINEXT_SAN_PROBE=1. * docs(enroll): scope the CERTInext SAN measurements to the sandbox The probe ran against sandbox-us, but the comments and the non-DNS warning read as though the behaviour were established generally. The customer this fix is for is on production, so the distinction matters. * The non-DNS finding (CERTInext accepts an email/IP/URI verbatim as an order domain rather than rejecting it) is explicitly sandbox-only and flagged unverified on production. The operator-facing warning no longer promises a parked order — it names the offending SANs and says the order will either be rejected or fail validation, noting what the sandbox did. * The CN-collapse finding is likewise marked sandbox-only, which strengthens rather than weakens the case for de-duplicating on our side: we should not depend on undocumented CA behaviour we have not seen in production. * The CSR-SAN finding — CERTInext ignores the CSR's subjectAltName entirely — is noted as corroborated by production independently of the probe: the report that prompted this work was a production UCC order whose CSR carried the SANs and whose certificate came back holding only the CN. * Probe header now says how to re-run against production, and warns that product numbering is per-account (Constants.Products holds defaults, not guarantees). No functional change. * fix(enroll): address full-review round 1 — DCV strand, ASN.1 debris, log injection Six confirmed findings from the five gating lenses, collapsing into three defects plus an upgrade-safety gap. 1. Undrainable pending domains stranded the valid ones (correctness, medium). Submitting non-DNS SANs means CERTInext registers them verbatim as order domains, so an email/URI SAN turns up as a domainVerification key that fails PerformDcvIfNeededAsync's FQDN check. That check threw for the whole order, before staging anything — and the exception escapes Enroll, which has no catch, after the order was already placed. Result: failed enrollment, orphaned order at the CA, no TXT record staged for the valid domains beside it, and every later Synchronize/GetSingleRecord retry re-threw into TryRunDcvDuringSyncAsync's catch-and-return-false, so the order could never progress. Invalid domains are now excluded (still LogError, so the audit trail keeps the signal) and the rest of the order proceeds. Same treatment where no DNS provider resolves for a domain, which is where an IP-literal SAN dead-ends: it clears the FQDN regex but no zone can match it. The genuine "no DNS provider deployed" misconfiguration still throws — distinguished by nothing on the order resolving at all — so Dcv_Throws_WhenNoProviderForDomain keeps its meaning. This is the file's own stated principle, already written at the EMS-956 branch: do not throw out of DCV for a condition that leaves the order legitimately pending. 2. GeneralNameToValue emitted ASN.1 debris (correctness + security + api-compat). Its default branch returned BouncyCastle's stringification, so a UPN otherName from a Windows-generated CSR was submitted as "[1.3.6.1.4.1.311.20.2.3, [CONTEXT 0]svc@corp.example.com]" and a directoryName as "CN=host.example.com,O=Acme" — in a domain-name field, contradicting the method's own doc comment and breaking orders that previously succeeded. These now return null. They are genuinely unrepresentable as a domain, unlike a well-formed IP/email/URI SAN, which we still submit on purpose. Skipping is not silent: ExtractSanEntriesFromCsr reports the skipped GeneralName tags and BuildSanList warns with them. 3. Log injection in the new audit sinks (security, low, CWE-117). SAN values come from the CSR and Command's dictionary — i.e. the requester — and were interpolated into three new log lines unescaped. Structured templates stop format-string abuse but not embedded CRLF, and NLog's text layout does not escape it, so a requester could forge audit records in the very lines added to make the submitted SAN set auditable. Added SanitizeForLog and applied it at all sinks. Deliberately a logging-only scrub: the value submitted to the CA is unchanged. 4. Upgrade safety (api-compat, medium). Submitting non-DNS SANs flips affected enrollments from "issues, silently incomplete" to "parks pending", with no way back short of downgrading the plugin. Added the SubmitNonDnsSans connector setting (default true — current behaviour) to restore DNS-only submission, and a CHANGELOG upgrade note, since the review's residual concern was process rather than logic. Also applied the endorsed advisory: RenewCertificateAsync parsed the subject twice; hoisted to one call, matching what the sibling method already does. Tests: 12 added — two DCV tests proving a non-FQDN domain and an unresolvable domain each leave their co-tenant staged and issuing, an otherName/directoryName test asserting no ASN.1 debris reaches the posted JSON, a SanitizeForLog theory, a CRLF-does-not-break-enrollment test, and SubmitNonDnsSans on/default coverage. Release, no-DCV: 195/195. Release, DCV: 220/220. Zero code warnings in both. * fix(enroll): address full-review round 2 — false pending-domain invariant, TXT leak Four confirmed findings. 1+2. The round-1 misconfiguration throw assumed "the CN is always a pending domain too" — false whenever CERTInext has cached a prior DCV validation for the CN/parent domain (a case this same method already special-cases earlier, at the aggregate/per-domain "already validated" check). When that happens the CN drops out of pendingDomains, and an order carrying only a non-DNS SAN alongside it hit the "nothing on the order resolves a provider" branch and threw — reopening the exact orphaned/stranded-order failure round 1 fixed, just narrowed to this input shape. The check now asks the right question: does ANY domain on the order — pending or already validated — resolve a DNS provider? If yes, a provider is clearly deployed and working, so this is the bad-SAN case (defer, don't throw). Only if nothing on the whole order resolves is it the genuine "no provider deployed" misconfiguration. 4. The TXT-staging loop's throw/defer sites (GetDcv failure, empty token, stage failure, EMS-956 not-ready) were all outside the try/finally that owns cleanup. Round 1 made multi-domain staging the normal case by finally submitting every SAN — before, a UCC order's SANs never reached CERTInext at all, so an order rarely had more than one pending domain. A later domain's failure now orphans every TXT record already published for the earlier domains in the same order, permanently — nothing else in the codebase ever calls CleanupValidation for them. The staging loop is now wrapped so any exit — exception or the not-yet-ready deferral — cleans up whatever was already staged first. 3. BuildAdditionalDomains' new duplicate-collapse debug log was the one sink in the diff that skipped the round-1 SanitizeForLog scrub. Applied. Also applied all 4 endorsed advisory simplifications: collapsed SanTypeFromGeneralNameTag + GeneralNameToValue into one GeneralNameToSanEntry (the split had four dead branches — a type mapping for tags whose value always came back null); moved the twice-duplicated SanitizeForLog into a shared internal LogSanitizer.Strip (Models/LogSanitizer.cs); nested BuildSanList's two non-DNS branches under one `nonDns.Count > 0` test instead of two; hoisted the repeated SAN-list log rendering into a local. Tests: 2 added (cached-CN-plus-unresolvable-SAN must defer without throwing; a second domain's stage failure must clean up the first domain's TXT record). SanitizeForLog's reflection-based test now calls LogSanitizer.Strip directly (it's internal, not private, and InternalsVisibleTo already covers the test project). Release, no-DCV: 195/195. Release, DCV: 222/222. Zero code warnings in both. * fix(enroll): address full-review round 3 — never throw out of DCV staging, CSR fallback not union All three dispositions carried forward from round 2 were broken by this round's adjudicator (real, not accepted), plus 8 more findings collapsing into the same three root causes. 1. PerformDcvIfNeededAsync's per-domain isolation (rounds 1-2) covered only the validator-resolution-null case. A GetDcv failure, an empty DCV token, and a StageValidation failure all still threw and aborted the WHOLE order — exactly the orphaned/stranded-order failure the isolation exists to prevent, just narrower. Confirmed reachable via the non-DNS SANs this PR submits by design (an IP-literal SAN clears the FQDN filter and reaches GetDcv; its live behavior there is unmeasured — my round-2 "measured" claim was based on a Moq stub asserting my own assumption, not the live API). Separately, the post-loop misconfiguration throw ("no DNS provider configured") could fire on an ordinary non-DNS Subject CN with no config escape hatch: SubmitNonDnsSans only filters the returned SAN list, never `subject`/`domainName`, so an IP-format CN reaches this path unfiltered. Fix: every per-domain failure in the staging loop (GetDcv error, empty token, no resolvable validator, StageValidation throwing or returning failure) is now LogError + skip-this-domain-and-continue. Nothing in the loop throws for an input- or API-driven reason any more. The only remaining "abort the whole pass" case is EMS-956 (DCV not yet exposed at the CA) — an order-readiness condition, not a per-domain one, so it still defers immediately rather than isolating per domain. The post-loop misconfiguration throw is gone; "nothing could be staged" now always defers to the next sync cycle with a LogError naming every skipped domain and why, rather than sometimes throwing depending on which domain failed or what else was on the order. 2. BuildSanList's CSR union (rounds 1-2) let a signed CSR's own SAN extension reintroduce names regardless of what Command's SAN dictionary supplied. External research against Keyfactor Command's documented enrollment-pattern behavior found no evidence Command enforces SAN policy by narrowing a signed CSR's embedded SANs before calling Enroll — reconciliation between an externally-generated CSR and Command's SAN data is documented as plugin/CA-configuration-dependent, not Command-enforced. A subscriber's own CSR routinely carries more names than an enrollment pattern computed, and the union let all of them through. Fix: the CSR is now a fallback, consulted only when Command supplies no SAN data at all (the case the original UCC-SAN-drop customer defect actually needed). When Command supplies any SAN entries, the CSR's own SAN extension is ignored entirely — the gateway dictionary is authoritative, not merely first. 3. Three findings on BuildSanList's logging: (a) the "N SAN(s) ... have been added to the order" line fired for CSR-fallback entries before the SubmitNonDnsSans=false filter removed exactly those entries two lines later — a false claim in the same call; (b) the "Resolved N SAN(s)" provenance line had the same before/after-filter mismatch; (c) two throw sites (empty token, stage failure) had no preceding structured log before the bare cleanup-and-rethrow wrapper caught them — moot now that neither throws, since both are LogError'd before being skipped. Fixed by reordering: apply the SubmitNonDnsSans filter first, log the resolved set and CSR-fallback provenance from the final, already-filtered result. Also fixed on the same pass: RenewCertificateAsync's "no usable CN" warning logged the prior order's RequestorName fallback unsanitized — the one sink in this diff that had skipped LogSanitizer.Strip. Tests: rewrote 6 existing DCV tests whose names and assertions pinned the old throw behavior (Dcv_Throws_* → Dcv_SkipsAndDefers_*, including reversing Dcv_Rethrows_When_GetDcv_FailsWithUnrelatedError's stated intent, and rewriting the round-2 TXT-leak test since a skipped domain no longer needs mid-call cleanup — the good domain now just completes its normal lifecycle). Rewrote the CSR-union test into CsrOnlySans_AreIgnored_WhenGatewaySuppliesAnyEntries. Added one test for the log-ordering fix's underlying data flow (CSR-fallback non-DNS SAN genuinely absent from the wire when SubmitNonDnsSans=false, not just mis-described in the log). Release, no-DCV: 196/196. Release, DCV: 223/223. Zero code warnings in both. * fix(enroll): address full-review round 4 — regex $ quirk, cancellation, CSR-fallback edge case Six confirmed findings, a clean round: 0 dismissed, 0 inconclusive. 1. The FQDN validation regex used ^...$, and in .NET's default (non-Multiline) mode $ matches immediately before a single trailing '\n', not only at the true end of the string — so "evil.com\n" passed as "valid" and reached several unsanitized-relative-to-siblings log sinks further down the same method (Staging/Triggering/cleanup/verified/rejected lines). Round 1 fixed log injection at other sinks in this file, but this one slipped through because the domain LOOKED validated. Fixed at the source: the regex now anchors with \A/\z (absolute string bounds regardless of trailing newlines), so a value with any trailing control character is rejected by the FQDN gate itself. Also applied LogSanitizer.Strip at every remaining domain/hostname sink in PerformDcvIfNeededAsync and WaitForDcvVerificationAsync for defense in depth and consistency with the sibling error-path logs that already had it. Worth noting for the record: this path is reachable for ANY order the account has at EXTERNALVALIDATION via Synchronize/GetSingleRecord, not only ones this plugin's own Enroll call placed — TrackOrder's domainVerification keys for an externally-created order are never passed through this plugin's own outbound Trim() calls, so those calls (correct for the outbound path) do not protect this inbound one. 2. SubmitNonDnsSans — the toggle that decides whether a certificate can issue silently missing requested SAN names — was never included in the Initialize startup config-dump log, unlike every sibling setting (DcvEnabled, DcvTxtRecordTemplate, IgnoreExpired, PageSize) that log line exists to make auditable. Added. 3+4+5. The generic per-domain `catch (Exception ex)` blocks around GetDcvAsync and StageValidation also caught OperationCanceledException/ TaskCanceledException raised by the shared DcvTimeoutMinutes-bound cancellation token, mislabeling a genuine timeout as an ordinary per-domain CA/DNS-provider failure in the skippedDomains audit summary — directly contradicting the outer catch's own comment, which claimed to be the handler for exactly this case but could never actually see it, since the inner catches intercepted it first. Both per-domain catch sites now re-throw OperationCanceledException explicitly before their generic Exception clause, so cancellation reaches the outer catch. That outer catch previously logged nothing before cleaning up and rethrowing — with neither EnrollNewAsync nor Enroll adding a catch of their own, an unanticipated failure during the synchronous Enroll-time DCV path left no plugin-emitted audit record at all. Added a LogError there. 6. BuildSanList's CSR-fallback trigger was "the gateway SAN dictionary computed to zero added entries" (fromGateway == 0), which cannot distinguish a null/absent dictionary from a non-null dictionary whose only keys map to empty arrays. An enrollment pattern that runs and deliberately computes zero SANs for a request is a policy decision this plugin must respect — round 3's fallback-over-union redesign existed specifically to stop CSR names overriding Command's SAN policy, and this edge case reopened exactly that. The trigger now checks `san == null` directly: only the literal absence of a dictionary engages the CSR fallback. Tests: 4 added — a trailing-newline domain routed to invalidDomains rather than reaching GetDcv; a cancellation during GetDcv propagating rather than being reported as a skipped-domain failure; and a non-null, all-empty-array gateway SAN dictionary correctly suppressing the CSR fallback (CN-only result, not backfilled from the CSR). Release, no-DCV: 197/197. Release, DCV: 226/226. Zero code warnings in both. * fix(enroll): address full-review round 5 — cancellation swallowed by RestSharp, unsanitized DomainName log Two confirmed findings; the first invalidated round 4's own cancellation fix in a way only a real-HTTP-level test could catch. 1. Round 4 added `catch (OperationCanceledException) { throw; }` guards ahead of the generic per-domain catches in PerformDcvIfNeededAsync, to stop a DCV timeout from being mislabeled as an ordinary per-domain failure. That guard is correct but dead for the real trigger: CERTInextClient is built with ThrowOnAnyError=false, so when the shared cancellation token fires mid-call, RestSharp catches HttpClient.SendAsync's cancellation internally and returns a non-throwing, unsuccessful RestResponse instead of propagating OperationCanceledException. DeserializeOrThrow then wraps that into a plain Exception — which lands in the generic catch, not the new guard, and gets logged and reported as "GetDcv failed" for whatever domain happened to be in flight. Round 4's regression test only proved the plugin-side logic works when a Moq mock is told to throw OperationCanceledException directly — which the real client never does, so it gave false confidence. Fixed at the actual source: ExecuteWithRetryAsync (the one place in the client that holds `ct`) now calls ct.ThrowIfCancellationRequested() immediately after the HTTP call, before any retry or error-wrapping logic sees the response. This fixes every caller of ExecuteWithRetryAsync, not just GetDcvAsync — the same swallow-and-wrap otherwise applies to VerifyDcvAsync, TrackOrderAsync, and everything else that goes through it. 2. PlaceOrderAsync's transient-failure and duplicate-transaction warning logs interpolated the requester-derived DomainName without LogSanitizer.Strip, inconsistent with a sibling log statement three lines above in the same method that already sanitizes the identical field. Fixed both. Also applied the one endorsed advisory: CleanupPartialStagingAsync (added in round 2 for early-exit paths) duplicated the pre-existing try/finally cleanup loop almost line-for-line; both now share CleanupOneStagedValidationAsync, parameterized by a log-context string for the one place their wording differs. Left as-is: a safely-dismissed finding about Enroll()'s original "Enrollment attempt started" log (pre-existing, outside this diff) not being sanitized — the adjudicator tried to break that as out-of-scope and could not. Tests: added GetDcvAsync_ThrowsOperationCanceled_WhenCancellationTokenIsCancelled in CERTInextClientTests.cs — against the REAL client and a real (local) WireMock HTTP call with a pre-cancelled token, not a mock told to throw whatever type is asked for. This is the test shape round 4 was missing. Release, no-DCV: 198/198. Release, DCV: 227/227. Zero code warnings in both. * fix(enroll): address full-review round 6 — cleanup reuses cancelled token, no correlation ID Two confirmed findings, both in code from earlier rounds. 1. The DCV-timeout cleanup path (added round 2, hardened round 3) calls CleanupValidation with the same `ct` the operation was just cancelled by. Any IDomainValidator that forwards its token into its own HTTP calls — the reference CloudflareDomainValidator in this repo does exactly that — throws immediately on an already-cancelled token and never attempts the delete. So the one cleanup path specifically built to handle a DCV timeout is the one most likely to silently no-op in exactly that scenario, leaving a published TXT record behind with only a Warning logged ("may require manual removal"). CleanupOneStagedValidationAsync (deduplicated in round 5) now calls CleanupValidation with CancellationToken.None. This is a best-effort compensating action — removing a record we already published — and it must run regardless of why we're cleaning up, including when `ct` itself is the reason. 2. BuildSanList's provenance/resolution log lines (the exact logging this diff added specifically to close "the blind spot that hid the original defect") carried no Subject, unlike nearly every other enrollment-path log line in this file, which repeats Subject={Subject} per line rather than relying on any log-scope mechanism (there is none in this codebase). Under concurrent enrollments, an auditor could not attribute either line back to a specific request. Threaded `subject` through BuildSanList's signature and both call sites, added to all five of its log statements. Advisory noted, not acted on: the file's original "Enrollment attempt started" log doesn't sanitize Subject/SANs either — round 5's adjudicator already tried to break accepting that as pre-existing/out-of-scope and could not, so it stands. Tests: added Dcv_CleanupAfterCancellation_UsesCancellationTokenNone_NotTheAmbientToken, which asserts CleanupValidation's token argument is exactly CancellationToken.None (not merely "not visibly cancelled during a fast test run", which the real DcvTimeoutMinutes-bound token can't be driven to within a unit test) — proving the code passes the literal value regardless of the ambient token's state. Release, no-DCV: 198/198. Release, DCV: 228/228. Zero code warnings in both. * fix(enroll): address full-review round 7 — cleanup call unbounded, subject unsanitized in BuildSanList Three confirmed findings; two are the same root cause (severity high + medium, same location) and the third is a direct consequence of round 6's own fix. 1+3. Round 6's CancellationToken.None fix for the cleanup call over-corrected: it stopped the compensating CleanupValidation call from reusing an already-cancelled token, but in doing so removed its timeout bound entirely — at every one of its three call sites, including the routine, always-runs finally-block cleanup on the ordinary successful-DCV path, which was never cancellation-related to begin with. This directly contradicts the method's own documented SOX CC7.3 guarantee that the DCV flow is hard-timeout-bounded so a stuck DNS provider cannot hold a gateway worker request open indefinitely. A hanging network call inside any third-party IDomainValidator's CleanupValidation would now block forever. Fixed with a fresh, independently-bounded token instead of either extreme: CleanupOneStagedValidationAsync now creates its own CancellationTokenSource with a new Constants.Dcv.CleanupValidationTimeoutSeconds (60s) ceiling for each cleanup call. Not cancelled going in (so a cooperative validator still gets a real chance to run, closing round 6's original gap), but still bounded (closing this round's regression on top of it). 2. BuildSanList's six Subject={Subject} log lines (added last round for audit-trail correlation) logged the requester-controlled Subject DN raw, while every OTHER requester-controlled value in the same function (domain, SAN value, hostname) already goes through LogSanitizer.Strip — an inconsistency introduced within this diff's own new code, unlike the file's pre-existing "Enrollment attempt started" log (which round 5's adjudicator confirmed is legitimately out of scope, being unrelated pre-existing code). Wrapped all six. Also applied the one endorsed advisory: two new DCV test helpers built three byte-for-byte-identical DomainVerificationDetail JSON blocks; extracted a one-line DcvDetail(dcvStatus) helper. Updated the round-6 regression test to match: it asserted the cleanup token equals CancellationToken.None exactly, which is no longer true. Now asserts the two properties that actually matter — IsCancellationRequested is false (not reusing the cancelled ambient token) and CanBeCanceled is true (still bounded, not CancellationToken.None). Release, no-DCV: 198/198. Release, DCV: 228/228. Zero code warnings in both. * fix(enroll): address full-review round 8 — stale gateway count in log, cancellation swallows audit line Two confirmed findings, low/medium severity — cosmetic/observability rather than functional. 1. BuildSanList's own "Resolved N SAN(s)" log line reported a stale, pre-filter FromGatewayRequest count alongside the post-filter Total — reproducing the exact self-contradicting-audit-trail defect class round 3 already fixed once, but only for the CSR-fallback count (fromCsrKept), not the gateway count. With SubmitNonDnsSans=false and a gateway SAN dictionary mixing DNS and non-DNS entries, the line could read "Resolved 1 SAN(s) ... FromGatewayRequest=3" — 3 does not reconcile to 1. Fixed by computing the gateway count post-filter too: gateway- and CSR-sourced entries are mutually exclusive by construction (the CSR fallback only ever runs when the gateway supplied nothing at all), so `result.Count - fromCsrKept` is exactly the right post-filter gateway count. Removed the now-fully-superseded pre-filter `fromGateway` variable. 2. ExecuteWithRetryAsync's cancellation check (added round 5) throws before any caller reaches its own per-call audit line (Method/Path/HttpStatus/ LatencyMs). A DCV-timeout cancellation landing mid-flight on a CERTInext call therefore left no per-call record anywhere — only a coarser, order-level "unexpected failure" log with no domain/endpoint/status/ latency, since the per-domain cancellation catches in PerformDcvIfNeededAsync deliberately re-throw without logging (to avoid mislabeling a timeout as a per-domain failure). Fixed by logging Method/Path/HttpStatus/ResponseStatus/LatencyMs right at the cancellation-detection point inside ExecuteWithRetryAsync itself — the one place that reliably sees every cancellation regardless of which of its ~10 callers is in flight — before throwing. Also applied the one endorsed advisory: SanSubmissionTests.cs's two CSR builders (GenerateCsrPem, GenerateCsrPemWithGeneralNames) duplicated ~20 lines of BouncyCastle CSR-construction boilerplate; GenerateCsrPem is now a one-line delegator to GenerateCsrPemWithGeneralNames. Tests: added one regression test for the stale-count fix, pinning the payload-level data the log line is computed from (only the DNS entry survives SubmitNonDnsSans=false filtering out of a 3-entry mixed gateway dict) — there is no log-capture seam in this codebase (ILogger comes from a fixed LogHandler.GetClassLogger() field, not an injectable dependency), so the log line's exact text cannot be asserted directly, and the cancellation- logging fix has no independently observable test surface for the same reason (round 5's existing cancellation test already covers the only externally-visible behavior — the exception type — unchanged by this fix). Release, no-DCV: 199/199. Release, DCV: 229/229. Zero code warnings in both. * fix(enroll): address full-review round 9 — TXT cleanup unbounded in aggregate across domains One confirmed root cause (found independently by two lenses), continuing the round 6→7 pattern: round 7 fixed each cleanup call's own timeout bound, but running those calls one after another meant the bound was per-call, not in aggregate. Both the early-exit cleanup (CleanupPartialStagingAsync) and the routine, always-runs finally-block cleanup iterated staged domains sequentially. A UCC/multi-SAN order (the exact feature this diff exists to support) with N staged domains could hold the calling request open for up to N x CleanupValidationTimeoutSeconds if the DNS provider was merely slow — not even hung — on every delete: a realistic degraded-provider condition, not a contrived one. For a large SAN count this can exceed DcvTimeoutMinutes itself, contradicting the method's own SOX CC7.3 "the entire DCV flow is hard-timeout-bounded" comment for exactly the multi-domain case this whole fix chain has been hardening. Fixed by running the per-domain cleanup calls concurrently (Task.WhenAll) instead of sequentially, at both call sites. Each call keeps its own independent 60s bound from round 7; running them concurrently means the wall-clock time for the whole batch is bounded by the slowest single call, not the sum — these are independent per-domain operations on different hostnames/records with no shared mutable state, so there is nothing for concurrent execution to race on. Also applied both endorsed advisories: three new test-only IDomainValidator/ IDomainValidatorFactory implementations (PartiallyFailingDomainValidator, TokenCapturingDomainValidator, SelectiveDomainValidatorFactory) each re-implemented boilerplate the pre-existing FakeDomainValidator/ FakeDomainValidatorFactory already provided. Extended those two shared fakes instead (ShouldFail predicate + CleanupTokens capture on the validator; an optional resolvableDomain filter on the factory) and deleted the three duplicates, updating call sites. Tests: added a timing-based regression test proving cleanup for 3 domains completes in close to one cleanup delay's worth of wall time, not three — verified it actually catches the regression by temporarily reverting the fix locally (confirmed FAIL at ~4.5s) before restoring it (confirmed PASS at ~3s), so the threshold is proven discriminating, not just a number that happens to pass. Extended FakeDomainValidator with a configurable CleanupDelay to make this possible; its two List fields needed a lock now that cleanup calls can genuinely run concurrently (StagedRecords did not, since nothing awaits with a real yield point before writing to it). Release, no-DCV: 199/199. Release, DCV: 230/230. Zero code warnings in both. * fix(enroll): address full-review round 11 — check-after-await cancellation race, Subject sanitization (issue 0008) Round 11 confirmed both round-10 out-of-scope dispositions for real (both landed in safelyDismissed — the Sync N+1 pattern and the broader Subject-unsanitized claim are genuinely pre-existing, untouched by this diff) and surfaced one new, real defect plus one endorsed simplification. 1. ExecuteWithRetryAsync (round 5) checked ct.IsCancellationRequested / called ct.ThrowIfCancellationRequested() BEFORE checking whether the just-completed HTTP call actually succeeded. A CancellationTokenSource's timer callback and the awaited HTTP task's completion are not mutually synchronized, so it's possible for the call to genuinely succeed (the response already fully arrived) while `ct` independently flips to cancelled in the same instant — a real check-after-await race, not a fabricated one. Hitting it discarded a genuine CERTInext success and reported OperationCanceledException instead: for VerifyDcv specifically, that would abort PerformDcvIfNeededAsync's loop before WaitForDcvVerificationAsync ever ran, and its finally block would delete the just-staged TXT record even though CERTInext had genuinely received the verify trigger — a self-inflicted DCV failure out of an actual success. Fixed by checking resp.IsSuccessful (or the 4xx client-error case) BEFORE the cancellation check, so a call that completed successfully is returned regardless of the token's state at that instant. Only a call that did NOT succeed goes on to ask "was that because of cancellation?" No dedicated regression test: reproducing this exact race deterministically requires the HTTP response to fully complete before the cancellation timer fires by mere ticks — round 5's own test already shows a pre-cancelled token makes RestSharp report the call as Aborted, not Successful, so a straightforward pre-cancel test cannot exercise this specific ordering. A test that could would need either a flaky real-timing race or refactoring the HTTP-call/cancellation-check split into an independently testable unit, which is more machinery than this ordering fix warrants. 2. Issue 0008 (filed after round 10, per user request): Subject={Subject} was logged raw at ~13 sites across Enroll's own audit log, Revoke, Synchronize, and RenewOrReissueAsync — all pre-existing, confirmed untouched by this diff, and confirmed pre-existing again by round 11's adjudicator — but folded into this PR anyway per explicit instruction rather than left for a separate PR. Wrapped every site in LogSanitizer.Strip, plus the SANs={SANs} (sanSummary) argument on the "Enrollment attempt started" line, which had the identical unsanitized- raw-dictionary gap for the same reason. Also applied the one endorsed advisory: BuildSanList repeated the exact `LogSanitizer.Strip(string.Join("; ", X.Select(...)))` SAN-formatting expression three times; extracted a local FormatSans(...) helper alongside the method's existing Add(...) local-function pattern. Release, no-DCV: 199/199. Release, DCV: 230/230. Zero code warnings in both. * fix(enroll): don't report GENERATED with no certificate body CERTInext can mark an order auto-approved (certificateStatusId 15) before the certificate bytes are actually generated. The immediate GetCertificate after order placement then fails, but the legacy client still reported Status=issued with Certificate=null, and BuildEnrollmentResult trusted that status over the missing body — handing the gateway framework a GENERATED result with no PEM, which crashes CertificateConverterFactory.FromPEM downstream (confirmed against a live support escalation, UCSD order 5435716354). Demote GENERATED to EXTERNALVALIDATION whenever the certificate body is missing, matching the invariant PickUpEnrolledCertificateAsync already enforces on its own GENERATED branch. * docs(changelog): trim 1.0.1 entries to plain, concise bullets The 1.0.1 section had ballooned into multi-sentence paragraphs per bullet. Cut each down to the essential fact for a customer skimming release notes; no information dropped, just the padding. --- .../SanSubmissionProbeTests.cs | 391 +++++++++ CERTInext.Tests/CERTInextCAPluginDcvTests.cs | 541 +++++++++++- CERTInext.Tests/CERTInextCAPluginTests.cs | 28 + CERTInext.Tests/CERTInextClientTests.cs | 36 + CERTInext.Tests/FakeDomainValidator.cs | 61 +- CERTInext.Tests/MockCertificateData.cs | 14 + CERTInext.Tests/SanSubmissionTests.cs | 707 ++++++++++++++++ CERTInext/API/CertificateRequest.cs | 17 + CERTInext/CERTInextCAPlugin.cs | 780 +++++++++++++++--- CERTInext/CERTInextCAPluginConfig.cs | 30 + CERTInext/Client/CERTInextClient.cs | 147 +++- CERTInext/Constants.cs | 12 + CERTInext/Models/LogSanitizer.cs | 33 + CHANGELOG.md | 10 +- 14 files changed, 2652 insertions(+), 155 deletions(-) create mode 100644 CERTInext.IntegrationTests/SanSubmissionProbeTests.cs create mode 100644 CERTInext.Tests/SanSubmissionTests.cs create mode 100644 CERTInext/Models/LogSanitizer.cs diff --git a/CERTInext.IntegrationTests/SanSubmissionProbeTests.cs b/CERTInext.IntegrationTests/SanSubmissionProbeTests.cs new file mode 100644 index 0000000..23681d1 --- /dev/null +++ b/CERTInext.IntegrationTests/SanSubmissionProbeTests.cs @@ -0,0 +1,391 @@ +// Copyright 2026 Keyfactor +// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. +// At http://www.apache.org/licenses/LICENSE-2.0 +// +// Probe: establish empirically how CERTInext treats the SAN/domain fields on +// GenerateOrderSSL. Written because the plugin's original behaviour encoded three +// assumptions that were never measured: +// +// A. certificateInformation.additionalDomains is the field that puts extra names on +// the certificate (so a UCC order that omits it yields a CN-only certificate). +// B. additionalDomains accepts DNS names only, so a non-DNS SAN is rejected by the CA. +// C. Repeating the primary domainName inside additionalDomains is harmful (duplicate +// domain / consumes the UCC allowance), so it should be de-duplicated. +// +// None of these had a test. This probe answers them against the live API by placing one +// order per variant and reading back the domain set CERTInext actually registered, via +// TrackOrder's domainVerification block (keys are the domains on the order). That is +// ground truth for "which names did the CA put on this order" without waiting for DCV +// and issuance to complete. +// +// --------------------------------------------------------------------------------------- +// MEASURED RESULTS — SANDBOX ONLY: sandbox-us, account 4951571271, product 844 (OV SSL UCC), +// 2026-08-12. (Product 840 / DV UCC is not enabled on that account: "Invalid Product Code".) +// +// These are sandbox observations. Re-run against production before treating B or C as +// settled there — point ~/.env_certinext at the production account and set +// CERTINEXT_SAN_PROBE_PRODUCTS to a UCC code that account can actually order (product +// numbering is per-account; the codes in Constants.Products are defaults, not guarantees). +// Finding A and the CSR-SAN result below are separately corroborated by production: the +// customer report that prompted this work was a production UCC order whose CSR carried the +// SANs and whose issued certificate held only the CN. +// +// A. CONFIRMED. additionalDomains is what puts extra names on the order. Submitting +// CN + extra1. registered BOTH domains. +// +// B. DISPROVEN. Non-DNS values are NOT rejected. An email address, an IPv4 literal and +// an https:// URI were each accepted at placement AND registered as order domains +// ("san-probe@example.com", "192.0.2.10", "https://san-probe.example.com/x" all came +// back as domainVerification keys). So the CA does not validate the field's contents +// at order time; such an order is created and then cannot pass DCV, rather than +// failing cleanly up front. +// +// C. PARTLY DISPROVEN. Repeating the primary domainName inside additionalDomains is +// accepted and CERTInext collapses it itself — the order came back with the CN +// registered once. De-duplicating on our side is therefore belt-and-braces, not a +// correctness requirement. +// +// Root cause of the customer-reported "UCC SANs not populating": CERTInext IGNORES the +// subjectAltName extension in the CSR. A CSR carrying CN + extra2., submitted with +// additionalDomains omitted, registered ONLY the CN. SANs must be sent in +// additionalDomains or they do not reach the certificate, no matter what the CSR says. +// --------------------------------------------------------------------------------------- +// +// Opt-in: this places real orders against whatever account ~/.env_certinext points at. +// +// set -a; . ~/.env_certinext; set +a +// export CERTINEXT_SAN_PROBE=1 +// dotnet test CERTInext.IntegrationTests/CERTInext.IntegrationTests.csproj -c Release \ +// --filter "FullyQualifiedName~SanSubmissionProbeTests" \ +// --logger "console;verbosity=detailed" > /tmp/sanprobe.log 2>&1 +// +// (xUnit buffers ITestOutputHelper output until the test ends — read the report at the tail.) + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using Keyfactor.Extensions.CAPlugin.CERTInext.API; +using Keyfactor.Extensions.CAPlugin.CERTInext.Client; +using Org.BouncyCastle.Asn1; +using Org.BouncyCastle.Asn1.Pkcs; +using Org.BouncyCastle.Asn1.X509; +using Org.BouncyCastle.Crypto; +using Org.BouncyCastle.Crypto.Generators; +using Org.BouncyCastle.Pkcs; +using Org.BouncyCastle.Security; +using Xunit; +using Xunit.Abstractions; + +namespace Keyfactor.Extensions.CAPlugin.CERTInext.IntegrationTests +{ + public class SanSubmissionProbeTests : IClassFixture + { + private const string OptInFlag = "CERTINEXT_SAN_PROBE"; + + /// + /// Comma-separated product codes to probe. Defaults to the Multi-Domain (UCC) codes, + /// because additional domains are only meaningful on a UCC product — a single-domain + /// product (e.g. 842 = OV SSL) registers the CN and nothing else no matter what + /// additionalDomains contains, which makes it useless as a probe target. + /// + private const string ProductCodesFlag = "CERTINEXT_SAN_PROBE_PRODUCTS"; + private const string DefaultProductCodes = "840,844"; + + private readonly IntegrationTestFixture _fixture; + private readonly ITestOutputHelper _out; + + public SanSubmissionProbeTests(IntegrationTestFixture fixture, ITestOutputHelper output) + { + _fixture = fixture; + _out = output; + } + + // ------------------------------------------------------------------------- + // CSR generation (BouncyCastle — project crypto policy) + // ------------------------------------------------------------------------- + + /// + /// Generates a PKCS#10 CSR for , optionally carrying a + /// subjectAltName extension (via the PKCS#9 extensionRequest attribute) holding + /// . The SAN-bearing form is what lets this probe ask + /// whether CERTInext reads SANs out of the CSR at all. + /// + private static string GenerateCsrPem(string cn, params string[] dnsSans) + { + var keyGen = new RsaKeyPairGenerator(); + keyGen.Init(new KeyGenerationParameters(new SecureRandom(), 2048)); + AsymmetricCipherKeyPair kp = keyGen.GenerateKeyPair(); + + Asn1Set attributes = null; + if (dnsSans != null && dnsSans.Length > 0) + { + var names = new GeneralNames( + dnsSans.Select(d => new GeneralName(GeneralName.DnsName, d)).ToArray()); + + var extGen = new X509ExtensionsGenerator(); + extGen.AddExtension(X509Extensions.SubjectAlternativeName, critical: false, extValue: names); + + attributes = new DerSet(new AttributePkcs( + PkcsObjectIdentifiers.Pkcs9AtExtensionRequest, + new DerSet(extGen.Generate()))); + } + + var csr = new Pkcs10CertificationRequest( + "SHA256withRSA", new X509Name($"CN={cn}"), kp.Public, attributes, kp.Private); + + return "-----BEGIN CERTIFICATE REQUEST-----\n" + + Convert.ToBase64String(csr.GetEncoded(), Base64FormattingOptions.InsertLineBreaks) + + "\n-----END CERTIFICATE REQUEST-----"; + } + + // ------------------------------------------------------------------------- + // One probe variant + // ------------------------------------------------------------------------- + + private sealed class ProbeOutcome + { + public string ProductCode; + public string Label; + public bool Accepted; + public string OrderNumber; + public string Detail; + /// Domains CERTInext registered on the order, per TrackOrder. + public List RegisteredDomains = new List(); + /// Names we asked CERTInext to put on the order, for comparison. + public List RequestedDomains = new List(); + + /// + /// True when the rejection was "Invalid Product Code" — the product simply is not + /// enabled on this account, which is not a data point about SAN handling. + /// + public bool ProductUnavailable; + } + + /// + /// Places one order and reads back the domain set CERTInext registered for it. + /// drives certificateInformation.additionalDomains; + /// drives the SAN extension inside the CSR. They are + /// varied independently on purpose — that separation is the whole point of the probe. + /// + private async Task ProbeAsync( + string productCode, + string label, + Func> sansFactory, + string[] csrSans) + { + var outcome = new ProbeOutcome { ProductCode = productCode, Label = label }; + + var client = new CERTInextClient(_fixture.Config); + string cn = $"sanprobe-{DateTime.UtcNow:yyyyMMddHHmmssfff}.{SafeLabel(label)}.example.com"; + + var sans = sansFactory?.Invoke(cn); + outcome.RequestedDomains = sans == null + ? new List() + : sans.Select(s => $"{s.Type}:{s.Value}").ToList(); + + var req = new EnrollCertificateRequest + { + Csr = GenerateCsrPem(cn, csrSans == null ? null : csrSans.Select(s => Format(s, cn)).ToArray()), + Subject = $"CN={cn}", + Sans = sans, + ProfileId = productCode, + RequesterName = _fixture.RequestorName, + RequesterEmail = _fixture.RequestorEmail + }; + + try + { + var resp = await client.EnrollCertificateAsync(req); + outcome.Accepted = true; + outcome.OrderNumber = resp?.Id; + outcome.Detail = $"OrderNumber={resp?.Id} Status={resp?.Status}"; + } + catch (Exception ex) + { + outcome.Accepted = false; + outcome.Detail = ex.Message; + outcome.ProductUnavailable = + ex.Message.IndexOf("Invalid Product Code", StringComparison.OrdinalIgnoreCase) >= 0; + return outcome; + } + + // Read back which domains the CA actually put on the order. + try + { + var track = await client.TrackOrderAsync(outcome.OrderNumber); + var entries = track.OrderDetails?.DomainVerification?.GetDomainEntries(); + if (entries != null) + outcome.RegisteredDomains = entries.Keys.OrderBy(k => k, StringComparer.OrdinalIgnoreCase).ToList(); + } + catch (Exception ex) + { + outcome.Detail += $" | TrackOrder failed: {ex.Message}"; + } + + return outcome; + } + + /// Substitutes the generated CN into a variant's placeholder template. + private static string Format(string template, string cn) => template.Replace("{cn}", cn); + + private static string SafeLabel(string label) => + new string(label.ToLowerInvariant().Select(c => char.IsLetterOrDigit(c) ? c : '-').ToArray()) + .Trim('-'); + + // ------------------------------------------------------------------------- + // The probe + // ------------------------------------------------------------------------- + + [SkippableFact] + public async Task Probe_SanSubmissionBehaviour() + { + IntegrationSkip.IfNotConfigured(_fixture); + Skip.IfNot( + Environment.GetEnvironmentVariable(OptInFlag) == "1", + $"Set {OptInFlag}=1 to run this probe — it places real orders on the configured account."); + + var variants = new List<(string Label, Func> Sans, string[] CsrSans)> + { + // 1. Assumption A, positive control: additionalDomains carries an extra DNS + // name. If the extra name comes back registered, additionalDomains works. + ("dns-extra-via-additionalDomains", + cn => new List + { + new SanEntry { Type = "dns", Value = cn }, + new SanEntry { Type = "dns", Value = $"extra1.{cn}" } + }, + new[] { "{cn}", "extra1.{cn}" }), + + // 2. Assumption A, the actual bug: CSR carries both names, additionalDomains + // is omitted entirely. This is what v1.0.1 sent for every UCC enrollment. + // If only the CN comes back registered, the CA does NOT read CSR SANs and + // the diagnosis is confirmed. + ("csr-sans-only-no-additionalDomains", + _ => null, + new[] { "{cn}", "extra2.{cn}" }), + + // 3. Assumption C: primary domainName repeated inside additionalDomains. + // Does the CA reject it, or silently collapse it? + ("cn-duplicated-in-additionalDomains", + cn => new List + { + new SanEntry { Type = "dns", Value = cn }, + new SanEntry { Type = "dns", Value = cn } + }, + new[] { "{cn}" }), + + // 4-6. Assumption B: non-DNS values in additionalDomains. Rejected, ignored, + // or accepted? Each is submitted alongside a valid DNS name so a rejection + // is attributable to the non-DNS value rather than an empty domain set. + ("nondns-email-in-additionalDomains", + cn => new List + { + new SanEntry { Type = "dns", Value = cn }, + new SanEntry { Type = "email", Value = "san-probe@example.com" } + }, + new[] { "{cn}" }), + + ("nondns-ip-in-additionalDomains", + cn => new List + { + new SanEntry { Type = "dns", Value = cn }, + new SanEntry { Type = "ip", Value = "192.0.2.10" } + }, + new[] { "{cn}" }), + + ("nondns-uri-in-additionalDomains", + cn => new List + { + new SanEntry { Type = "dns", Value = cn }, + new SanEntry { Type = "uri", Value = "https://san-probe.example.com/x" } + }, + new[] { "{cn}" }), + }; + + string[] productCodes = + (Environment.GetEnvironmentVariable(ProductCodesFlag) ?? DefaultProductCodes) + .Split(',', StringSplitOptions.RemoveEmptyEntries) + .Select(p => p.Trim()) + .Where(p => p.Length > 0) + .ToArray(); + + var results = new List(); + foreach (string productCode in productCodes) + { + bool unavailable = false; + foreach (var (label, sans, csrSans) in variants) + { + var outcome = await ProbeAsync(productCode, label, sans, csrSans); + results.Add(outcome); + + // Don't burn five more orders proving the same product code is not + // enabled on this account. + if (outcome.ProductUnavailable) + { + unavailable = true; + break; + } + + // Throttle: the sandbox rate-limits order bursts (~16 orders / 10 s). + await Task.Delay(1500); + } + + if (unavailable) + _out.WriteLine($"(product {productCode} is not enabled on this account — skipped)"); + } + + _out.WriteLine("=== CERTInext SAN submission probe ==="); + _out.WriteLine($"ProductCodes probed : {string.Join(", ", productCodes)}"); + _out.WriteLine($"(fixture default : {_fixture.ProductCode})"); + _out.WriteLine(""); + + foreach (var group in results.GroupBy(r => r.ProductCode)) + { + _out.WriteLine($"--- ProductCode {group.Key} ---"); + foreach (var r in group) + { + _out.WriteLine($"[{(r.Accepted ? "ACCEPTED" : "REJECTED")}] {r.Label}"); + _out.WriteLine($" requested (additionalDomains): {(r.RequestedDomains.Count > 0 ? string.Join(", ", r.RequestedDomains) : "(field omitted)")}"); + _out.WriteLine($" detail : {r.Detail}"); + _out.WriteLine($" registeredDomains (TrackOrder): {(r.RegisteredDomains.Count > 0 ? string.Join(", ", r.RegisteredDomains) : "(none reported)")}"); + _out.WriteLine(""); + } + } + + _out.WriteLine("=== How to read this ==="); + _out.WriteLine("registeredDomains is TrackOrder's domainVerification key set — the domains"); + _out.WriteLine("CERTInext put on the order. Compare it against 'requested':"); + _out.WriteLine("(1) vs (2): if (1) registers the extra name and (2) does not, then"); + _out.WriteLine(" additionalDomains is required and CSR SANs alone are ignored."); + _out.WriteLine("(3) : whether repeating the CN is rejected or collapsed."); + _out.WriteLine("(4)-(6) : whether non-DNS values are rejected, ignored, or accepted"); + _out.WriteLine(" AT PLACEMENT TIME. An order accepted here can still be"); + _out.WriteLine(" rejected later during validation/approval."); + + // The probe reports; it does not assert a specific CA behaviour, because its purpose + // is to discover what that behaviour is. What must hold is that at least one UCC + // product was actually exercised — otherwise the run proved nothing and should not + // read as a pass. + var usable = results + .Where(r => !r.ProductUnavailable) + .GroupBy(r => r.ProductCode) + .ToList(); + + Skip.If( + usable.Count == 0, + "None of the probed product codes are enabled on this account " + + $"({string.Join(", ", productCodes)}). Set {ProductCodesFlag} to a Multi-Domain (UCC) " + + "code this account can order."); + + foreach (var group in usable) + { + var control = group.First(r => r.Label == "dns-extra-via-additionalDomains"); + Assert.True( + control.Accepted, + $"Positive control failed on product {group.Key} — could not place even a " + + $"plain DNS UCC order: {control.Detail}"); + } + } + } +} diff --git a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs index d812074..b45f0ac 100644 --- a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs @@ -527,7 +527,7 @@ public async Task SyncDcvRetry_DoesSingleShotTrackOrder_WhenChallengeNotReady() // --------------------------------------------------------------------------- [Fact] - public async Task Dcv_Throws_WhenNoProviderForDomain() + public async Task Dcv_SkipsAndDefers_WhenNoProviderForDomain() { var mock = NewMock(); mock.Setup(c => c.EnrollCertificateAsync(It.IsAny(), It.IsAny())) @@ -539,17 +539,19 @@ public async Task Dcv_Throws_WhenNoProviderForDomain() mock.Setup(c => c.GetDcvAsync(MockCertificateData.DcvOrderId, MockCertificateData.DcvDomain, Constants.Dcv.MethodDnsTxt, It.IsAny())) .ReturnsAsync(MockCertificateData.DcvTokenResponse()); - // Factory returns null → no DNS provider configured + // Factory returns null → no DNS provider configured. Regression: this used to throw and + // fail the whole order — including when the "unresolvable" domain was actually just a + // non-DNS Subject CN with no config-level way to prevent the throw (SubmitNonDnsSans only + // filters the SAN list, not the subject). Now it is logged loudly and deferred instead. var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator: null)); Func act = () => Enroll(plugin); - await act.Should().ThrowAsync() - .WithMessage("*No DNS provider plugin is configured*"); + await act.Should().NotThrowAsync(); } [Fact] - public async Task Dcv_Throws_WhenStageValidationFails() + public async Task Dcv_SkipsAndDefers_WhenStageValidationFails() { var mock = NewMock(); mock.Setup(c => c.EnrollCertificateAsync(It.IsAny(), It.IsAny())) @@ -566,10 +568,12 @@ public async Task Dcv_Throws_WhenStageValidationFails() Func act = () => Enroll(plugin); - await act.Should().ThrowAsync() - .WithMessage("*Failed to stage DNS validation*DNS zone not writable*"); + // Regression: a StageValidation failure used to throw and fail the whole order. Now it + // is logged loudly and the domain is skipped/deferred — this is the only pending domain, + // so nothing gets staged and the order defers to the next sync cycle. + await act.Should().NotThrowAsync(); - // No VerifyDcv call — failed before reaching that step + // No VerifyDcv call — nothing was staged to verify mock.Verify(c => c.VerifyDcvAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); } @@ -602,7 +606,7 @@ public async Task Dcv_CleanupAlwaysCalled_EvenWhenVerifyDcvThrows() } [Fact] - public async Task Dcv_Throws_WhenGetDcvReturnsNoToken() + public async Task Dcv_SkipsAndDefers_WhenGetDcvReturnsNoToken() { var mock = NewMock(); mock.Setup(c => c.EnrollCertificateAsync(It.IsAny(), It.IsAny())) @@ -619,8 +623,11 @@ public async Task Dcv_Throws_WhenGetDcvReturnsNoToken() Func act = () => Enroll(plugin); - await act.Should().ThrowAsync() - .WithMessage("*GetDcv returned no token*"); + // Regression: an empty token used to throw and fail the whole order. It is now logged + // loudly (LogError) and the domain is skipped — the order defers to the next sync cycle + // rather than failing Enroll with an order already placed at the CA. + await act.Should().NotThrowAsync(); + validator.StagedRecords.Should().BeEmpty("the only pending domain returned no token, so nothing should have been staged"); } // --------------------------------------------------------------------------- @@ -689,11 +696,16 @@ public async Task Dcv_Defers_When_GetDcv_ReturnsInvalidRequestMessage_WithoutEms } [Fact] - public async Task Dcv_Rethrows_When_GetDcv_FailsWithUnrelatedError() + public async Task Dcv_SkipsAndDefers_WhenGetDcvFailsWithUnrelatedError() { - // Tolerance is narrow: a genuine server error (5xx, transport, auth) must still - // bubble up so the gateway treats the enrollment as failed and the operator can - // diagnose. This guards against accidentally swallowing every GetDcv exception. + // Regression: this test used to assert the opposite — that a genuine server error (5xx, + // transport, auth) must bubble up and fail the whole enrollment. That is exactly the + // orphaned-order failure mode: GetDcv's live behavior for a non-DNS order-domain is + // unmeasured (see BuildSanList's sandbox-only caveat), so treating any unrecognized + // GetDcv error as fatal risks failing perfectly good co-tenant DNS domains on the same + // order over one domain's transient or CA-side issue, with the enrollment already + // placed at CERTInext and no catch anywhere above this call. The failure is still loud + // (LogError, with the underlying exception) — it just no longer fails the call. var mock = NewMock(); mock.Setup(c => c.EnrollCertificateAsync(It.IsAny(), It.IsAny())) .ReturnsAsync(new EnrollCertificateResponse { Id = MockCertificateData.DcvOrderId, Status = "pending_dcv" }); @@ -708,8 +720,8 @@ public async Task Dcv_Rethrows_When_GetDcv_FailsWithUnrelatedError() var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator)); Func act = () => Enroll(plugin); - await act.Should().ThrowAsync() - .WithMessage("*HTTP 500*"); + await act.Should().NotThrowAsync(); + validator.StagedRecords.Should().BeEmpty("the only pending domain's GetDcv call failed, so nothing should have been staged"); } // --------------------------------------------------------------------------- @@ -824,5 +836,500 @@ public async Task Dcv_WaitsForIssuance_AfterDcvVerifies() mock.Verify(c => c.GetCertificateAsync(MockCertificateData.DcvOrderId, It.IsAny()), Times.AtLeast(2), "plugin should have polled at least twice for issuance"); } + + // --------------------------------------------------------------------------- + // Undrainable pending domains must not strand the valid ones on the same order + // --------------------------------------------------------------------------- + + /// Builds a DomainVerificationDetail JsonElement for the given dcvStatus. + private static System.Text.Json.JsonElement DcvDetail(string dcvStatus) => + System.Text.Json.JsonSerializer.SerializeToElement(new DomainVerificationDetail + { + DcvMethod = Constants.Dcv.MethodDnsTxt, + DcvStatus = dcvStatus, + Status = "1" + }); + + /// + /// Builds a TrackOrder response whose domainVerification block lists several pending + /// domains, so tests can mix validatable and unvalidatable keys on one order. + /// + private static TrackOrderResponse DcvPendingTrackResponseMultiDomain( + string orderNumber, params string[] domains) + { + var detail = DcvDetail(Constants.Dcv.StatusPending); + var raw = new Dictionary(); + foreach (string d in domains) + raw[d] = detail; + + return new TrackOrderResponse + { + OrderDetails = new TrackOrderResponseDetails + { + OrderStatusId = "1", + CertificateStatusId = "1", + DomainVerification = new TrackOrderDomainVerification + { + Status = Constants.Dcv.StatusPending, + RawDomainEntries = raw + } + } + }; + } + + /// + /// Builds a TrackOrder response with one already-validated domain (dcvStatus=1) and one + /// still-pending, unresolvable domain (dcvStatus=0) — the shape CERTInext produces when it + /// has cached a prior DCV validation for the CN while a non-DNS SAN on the same order is + /// still outstanding. + /// + private static TrackOrderResponse DcvMixedStatusTrackResponse( + string validatedDomain, string pendingDomain) + { + var validated = DcvDetail(Constants.Dcv.StatusValidated); + var pending = DcvDetail(Constants.Dcv.StatusPending); + + return new TrackOrderResponse + { + OrderDetails = new TrackOrderResponseDetails + { + OrderStatusId = "1", + CertificateStatusId = "1", + DomainVerification = new TrackOrderDomainVerification + { + // Aggregate stays pending because one domain still is — this must not take + // the early "already validated" return at the top of the method. + Status = Constants.Dcv.StatusPending, + RawDomainEntries = new Dictionary + { + [validatedDomain] = validated, + [pendingDomain] = pending + } + } + } + }; + } + + /// + /// Regression for the false invariant behind the round-1 fix's own misconfiguration check: + /// "the CN is always a pending domain too" is untrue whenever CERTInext has cached a prior + /// DCV validation for it (a case this same file's cached-validation branch documents), so a + /// non-DNS SAN sharing the order with an already-validated CN must not throw — it must defer + /// to the next sync cycle exactly like the single-domain case does. + /// + [Fact] + public async Task Dcv_CachedCnPlusUnresolvableSan_DefersWithoutThrowing() + { + const string order = MockCertificateData.DcvOrderId; + const string cn = MockCertificateData.DcvDomain; + const string ip = "192.0.2.10"; + + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending" }); + + mock.Setup(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvMixedStatusTrackResponse(validatedDomain: cn, pendingDomain: ip)); + + // The IP clears the FQDN regex and reaches GetDcv, per the sandbox-measured shape. + mock.Setup(c => c.GetDcvAsync(order, ip, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse(MockCertificateData.DcvToken)); + + var validator = new FakeDomainValidator(); + // Resolves for the CN (a real, working DNS provider) but not for the IP literal — the + // scenario that must prove "a provider IS deployed" rather than "nothing is deployed". + var plugin = BuildPlugin( + mock.Object, + new FakeDomainValidatorFactory(validator, resolvableDomain: cn), + DcvConfig()); + + Func act = () => Enroll(plugin); + + await act.Should().NotThrowAsync( + "an unresolvable non-DNS SAN must defer the order to the next sync cycle, not fail " + + "the enrollment — the CN having cached DCV proves a provider is deployed and working, " + + "so this is not the 'nothing is deployed' misconfiguration case"); + + validator.StagedRecords.Should().BeEmpty( + "the only pending domain is unresolvable, so nothing should have been staged"); + } + + /// + /// A non-FQDN pending domain must be skipped, not thrown on. + /// + /// Regression: non-DNS SANs are now submitted to CERTInext, which registers them verbatim + /// as order domains, so an email/URI SAN turns up as a domainVerification key that fails the + /// FQDN check. That check used to throw for the whole order — escaping Enroll (which has no + /// catch) after the order was already placed, so the enrollment failed with an orphaned + /// order and no TXT record was staged for the *valid* domains beside it. Every sync retry + /// re-threw and TryRunDcvDuringSyncAsync swallowed it, so the order could never progress. + /// + [Fact] + public async Task Dcv_NonFqdnPendingDomain_IsSkipped_AndValidDomainStillStaged() + { + const string order = MockCertificateData.DcvOrderId; + const string good = MockCertificateData.DcvDomain; + const string bad = "admin@example.com"; // what an rfc822 SAN comes back as + + var mock = NewMock(); + + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); + + mock.SetupSequence(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, good, bad)) + .ReturnsAsync(MockCertificateData.DcvVerifiedTrackResponse(order, good)); + + // Only the valid domain should ever reach GetDcv/VerifyDcv. MockBehavior.Strict means + // an unexpected call for `bad` fails the test on its own. + mock.Setup(c => c.GetDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse(MockCertificateData.DcvToken)); + mock.Setup(c => c.VerifyDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .Returns(Task.CompletedTask); + mock.Setup(c => c.GetCertificateAsync(order, It.IsAny())) + .ReturnsAsync(MockCertificateData.IssuedCertRecord(order)); + + var validator = new FakeDomainValidator(); + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator), + DcvConfig(dcvWaitForIssuanceSeconds: 10)); + + // Must not throw — that is the regression. + var result = await Enroll(plugin); + + string expectedHostname = string.Format(Constants.Dcv.DefaultTxtRecordTemplate, good); + validator.StagedRecords.Should().ContainSingle( + "the valid DNS domain must still be staged even though a co-tenant domain is unusable") + .Which.Should().Be((expectedHostname, MockCertificateData.DcvToken)); + + mock.Verify(c => c.GetDcvAsync(order, bad, It.IsAny(), It.IsAny()), + Times.Never, "a non-FQDN domain must never be sent to GetDcv"); + result.Status.Should().Be((int)EndEntityStatus.GENERATED); + } + + /// + /// Regression: the FQDN validation regex used ^...$ , and in .NET's default (non-Multiline) + /// mode $ matches immediately before a single trailing '\n', not only at the true end of the + /// string. A domain value ending in '\n' therefore passed as "valid" and reached several log + /// sinks unsanitized further down this same method — a CWE-117 log-injection route into the + /// DCV audit trail, reachable via any order visible through Synchronize/GetSingleRecord (not + /// just ones this plugin's own Enroll call placed, since TrackOrder's domainVerification keys + /// for an externally-created order are never trimmed by this plugin). The regex now anchors + /// with \A/\z, which are absolute string-start/end regardless of trailing newlines. + /// + [Fact] + public async Task Dcv_DomainWithTrailingNewline_IsRejectedAsInvalid_AndValidDomainStillStaged() + { + const string order = MockCertificateData.DcvOrderId; + const string good = MockCertificateData.DcvDomain; + const string bad = "evil.example.com\n"; + + var mock = NewMock(); + + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); + + mock.SetupSequence(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, good, bad)) + .ReturnsAsync(MockCertificateData.DcvVerifiedTrackResponse(order, good)); + + // MockBehavior.Strict: an unexpected GetDcv call for `bad` fails the test on its own — + // if the regex fix regressed, this domain would reach GetDcv instead of being rejected + // by the FQDN check before the staging loop even starts. + mock.Setup(c => c.GetDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse(MockCertificateData.DcvToken)); + mock.Setup(c => c.VerifyDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .Returns(Task.CompletedTask); + mock.Setup(c => c.GetCertificateAsync(order, It.IsAny())) + .ReturnsAsync(MockCertificateData.IssuedCertRecord(order)); + + var validator = new FakeDomainValidator(); + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator), + DcvConfig(dcvWaitForIssuanceSeconds: 10)); + + var result = await Enroll(plugin); + + string expectedHostname = string.Format(Constants.Dcv.DefaultTxtRecordTemplate, good); + validator.StagedRecords.Should().ContainSingle( + "the valid domain must still be staged even though a co-tenant domain carries a " + + "trailing newline") + .Which.Should().Be((expectedHostname, MockCertificateData.DcvToken)); + + mock.Verify(c => c.GetDcvAsync(order, bad, It.IsAny(), It.IsAny()), + Times.Never, "a domain with a trailing newline must never be sent to GetDcv"); + result.Status.Should().Be((int)EndEntityStatus.GENERATED); + } + + /// + /// Regression: the generic per-domain catch blocks around GetDcvAsync and StageValidation + /// used to catch OperationCanceledException along with genuine GetDcv/DNS-provider failures, + /// logging and skipping the domain as an ordinary per-domain failure. A cancellation (the + /// shared DcvTimeoutMinutes-bound token expiring mid-loop) is not that — it must propagate to + /// the outer catch instead, which is the only place that logs it correctly and is the + /// intended timeout-handling path documented at the top of this method's DCV timeout setup. + /// + [Fact] + public async Task Dcv_CancellationDuringGetDcv_PropagatesRatherThanBeingSkippedAsPerDomainFailure() + { + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = MockCertificateData.DcvOrderId, Status = "pending_dcv" }); + + mock.Setup(c => c.TrackOrderAsync(MockCertificateData.DcvOrderId, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvPendingTrackResponse()); + + mock.Setup(c => c.GetDcvAsync(MockCertificateData.DcvOrderId, MockCertificateData.DcvDomain, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ThrowsAsync(new OperationCanceledException("DCV timeout budget exceeded")); + + var validator = new FakeDomainValidator(); + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator)); + + Func act = () => Enroll(plugin); + + // Must propagate as a cancellation, not be swallowed and reported as "GetDcv failed" in + // the skipped-domains summary while Enroll completes normally. + await act.Should().ThrowAsync(); + } + + /// + /// A pending domain that resolves no DNS provider (an IP-literal SAN passes the FQDN regex + /// but no zone can match it) must likewise be skipped rather than failing the whole order. + /// + [Fact] + public async Task Dcv_DomainWithNoResolvableValidator_IsSkipped_AndValidDomainStillStaged() + { + const string order = MockCertificateData.DcvOrderId; + const string good = MockCertificateData.DcvDomain; + const string ip = "192.0.2.10"; // what an iPAddress SAN comes back as + + var mock = NewMock(); + + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); + + mock.SetupSequence(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, good, ip)) + .ReturnsAsync(MockCertificateData.DcvVerifiedTrackResponse(order, good)); + + // The IP literal clears the FQDN filter, so GetDcv IS called for it; the dead end is + // that no validator resolves. Stub it so reaching that point is legitimate. + mock.Setup(c => c.GetDcvAsync(order, It.IsAny(), Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse(MockCertificateData.DcvToken)); + mock.Setup(c => c.VerifyDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .Returns(Task.CompletedTask); + mock.Setup(c => c.GetCertificateAsync(order, It.IsAny())) + .ReturnsAsync(MockCertificateData.IssuedCertRecord(order)); + + var validator = new FakeDomainValidator(); + var plugin = BuildPlugin( + mock.Object, + new FakeDomainValidatorFactory(validator, resolvableDomain: good), + DcvConfig(dcvWaitForIssuanceSeconds: 10)); + + var result = await Enroll(plugin); + + string expectedHostname = string.Format(Constants.Dcv.DefaultTxtRecordTemplate, good); + validator.StagedRecords.Should().ContainSingle( + "only the domain with a resolvable provider should be staged, and it must still be staged") + .Which.Should().Be((expectedHostname, MockCertificateData.DcvToken)); + result.Status.Should().Be((int)EndEntityStatus.GENERATED); + } + + /// + /// Regression: the compensating cleanup call after an early exit from staging (chiefly the + /// shared DcvTimeoutMinutes-bound token firing mid-loop, which is what this scenario + /// simulates via a domain whose GetDcv call raises OperationCanceledException) must not reuse + /// the same token the operation was cancelled by. A cooperative IDomainValidator that forwards + /// its token into its own HTTP calls (the reference CloudflareDomainValidator in this repo + /// does exactly that) would otherwise throw immediately on an already-cancelled token and + /// never even attempt the delete, silently leaving the TXT record published. + /// + /// CancellationToken.None would fix that but removes the cleanup call's timeout bound + /// entirely — a second, adversarially-found regression on top of the first — so the correct + /// fix is a fresh token with its OWN short timeout: not cancelled going in, but still bounded. + /// + [Fact] + public async Task Dcv_CleanupAfterCancellation_UsesAFreshBoundedToken_NotTheAmbientToken() + { + const string order = MockCertificateData.DcvOrderId; + const string good = "a.example.com"; + const string bad = "b.example.com"; + + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); + + mock.Setup(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, good, bad)); + + mock.Setup(c => c.GetDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse("token-a")); + // Domain 'good' is processed first (Dictionary enumeration order matches insertion order + // in practice for the small dictionaries this test builds); 'bad' then throws, driving the + // outer catch's cleanup of the already-staged 'good' entry. + mock.Setup(c => c.GetDcvAsync(order, bad, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ThrowsAsync(new OperationCanceledException("DCV timeout budget exceeded")); + + var validator = new FakeDomainValidator(); + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator)); + + Func act = () => Enroll(plugin); + await act.Should().ThrowAsync(); + + validator.StagedRecords.Should().ContainSingle( + "'good' must have staged before 'bad' threw, for this test to exercise cleanup at all"); + var cleanupToken = validator.CleanupTokens.Should().ContainSingle( + "the staged entry must go through the cancellation cleanup path exactly once").Subject; + + cleanupToken.IsCancellationRequested.Should().BeFalse( + "cleanup is a best-effort compensating action and must run with its own token, " + + "not the already-cancelled ambient one"); + cleanupToken.CanBeCanceled.Should().BeTrue( + "the cleanup call must still be bounded by its own timeout, not unbounded " + + "(CancellationToken.None) — a hanging DNS-provider call must not block forever"); + } + + /// + /// Regression: the routine, always-runs finally-block cleanup used to iterate staged domains + /// sequentially. Each cleanup call already has its own independent + /// CleanupValidationTimeoutSeconds bound, but running them one after another meant that + /// bound was per-call, not in aggregate — a UCC order with N staged domains could hold the + /// calling request open for up to N x the per-call ceiling if the DNS provider was merely + /// slow (not even hung) on every delete, which can exceed DcvTimeoutMinutes itself for a + /// realistic multi-SAN count. Proven here by timing: three domains each with an artificial + /// cleanup delay must complete in close to ONE delay's worth of wall time, not three. + /// + [Fact] + public async Task Dcv_CleanupOfMultipleDomains_RunsConcurrently_NotSequentially() + { + const string order = MockCertificateData.DcvOrderId; + string[] domains = { "a.example.com", "b.example.com", "c.example.com" }; + var cleanupDelay = TimeSpan.FromMilliseconds(800); + + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); + + var verifiedDetail = DcvDetail(Constants.Dcv.StatusValidated); + var verifiedRaw = new Dictionary(); + foreach (string d in domains) verifiedRaw[d] = verifiedDetail; + + mock.SetupSequence(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, domains)) + .ReturnsAsync(new TrackOrderResponse + { + OrderDetails = new TrackOrderResponseDetails + { + OrderStatusId = "1", + CertificateStatusId = "1", + DomainVerification = new TrackOrderDomainVerification + { + Status = Constants.Dcv.StatusValidated, + RawDomainEntries = verifiedRaw + } + } + }); + + foreach (string d in domains) + { + mock.Setup(c => c.GetDcvAsync(order, d, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse($"token-{d}")); + mock.Setup(c => c.VerifyDcvAsync(order, d, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .Returns(Task.CompletedTask); + } + mock.Setup(c => c.GetCertificateAsync(order, It.IsAny())) + .ReturnsAsync(MockCertificateData.IssuedCertRecord(order)); + + var validator = new FakeDomainValidator { CleanupDelay = cleanupDelay }; + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator), + DcvConfig(dcvWaitForIssuanceSeconds: 10)); + + var sw = System.Diagnostics.Stopwatch.StartNew(); + await Enroll(plugin); + sw.Stop(); + + validator.CleanedUpKeys.Should().HaveCount(3, "all three staged domains must be cleaned up"); + + // This flow carries ~2s of fixed overhead unrelated to cleanup (DcvPropagationDelaySeconds + // and WaitForDcvVerificationAsync's poll interval both floor at 1s each — DcvConfig's + // propagationDelaySeconds default is deliberately 1, since 0 falls back to a 30s default + // in PerformDcvIfNeededAsync, not "no delay"). An 800ms-per-domain cleanup delay makes the + // concurrent-vs-sequential gap (≈800ms vs ≈2400ms of cleanup time) large relative to that + // fixed cost and to CI jitter. 4000ms sits well above "fixed overhead + one 800ms delay" + // and well below "fixed overhead + three 800ms delays run one after another". + sw.ElapsedMilliseconds.Should().BeLessThan(4000, + "cleanup for independent domains must run concurrently, not sequentially — " + + "3 domains x 800ms sequential would add roughly 3x this call's actual cleanup time"); + } + + /// + /// Regression: a StageValidation failure on one domain of a multi-domain order must not + /// leave the TXT records already published for the earlier domains orphaned. Before the + /// fix, the staging loop's throw sites were outside the try/finally that owns cleanup, so + /// this was reachable only by accident (pre-fix, a UCC order's SANs never reached CERTInext + /// at all, so an order rarely had more than one pending domain to stage). Submitting every + /// requested SAN makes multi-domain staging the normal case, so this must hold now. + /// + [Fact] + public async Task Dcv_StageFailureOnSecondDomain_DoesNotAbortTheGoodDomain() + { + const string order = MockCertificateData.DcvOrderId; + const string good = "a.example.com"; + const string bad = "b.example.com"; + + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); + + // First TrackOrder call (inside PerformDcvIfNeededAsync) sees both domains pending; + // the second (WaitForDcvVerificationAsync's poll after staging/VerifyDcv) sees the one + // domain that actually got staged — 'good' — as verified. + mock.SetupSequence(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, good, bad)) + .ReturnsAsync(MockCertificateData.DcvVerifiedTrackResponse(order, good)); + + mock.Setup(c => c.GetDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse("token-a")); + mock.Setup(c => c.GetDcvAsync(order, bad, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse("token-b")); + + mock.Setup(c => c.VerifyDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .Returns(Task.CompletedTask); + mock.Setup(c => c.GetCertificateAsync(order, It.IsAny())) + .ReturnsAsync(MockCertificateData.IssuedCertRecord(order)); + + var validator = new FakeDomainValidator + { + ShouldFail = key => key.Contains(bad, StringComparison.OrdinalIgnoreCase) + }; + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator), + DcvConfig(dcvWaitForIssuanceSeconds: 10)); + + Func act = () => Enroll(plugin); + + // Regression: a StageValidation failure on one domain of a multi-domain order must not + // abort the whole order any more — it did before this fix, which both failed the + // enrollment with an orphaned CERTInext order AND (before an earlier round's fix) + // orphaned the 'good' domain's already-published TXT record. Now the bad domain is + // skipped (logged loudly) and the good domain proceeds through the normal DCV lifecycle. + await act.Should().NotThrowAsync(); + + string goodHostname = string.Format(Constants.Dcv.DefaultTxtRecordTemplate, good); + string badHostname = string.Format(Constants.Dcv.DefaultTxtRecordTemplate, bad); + + validator.StagedRecords.Should().ContainSingle( + "only the domain that did not fail to stage should ever have been staged") + .Which.key.Should().Be(goodHostname); + validator.CleanedUpKeys.Should().Contain(goodHostname, + "the good domain completes its normal verify-then-cleanup lifecycle"); + validator.CleanedUpKeys.Should().NotContain(badHostname, + "the bad domain was never staged, so there is nothing to clean up for it"); + } } } diff --git a/CERTInext.Tests/CERTInextCAPluginTests.cs b/CERTInext.Tests/CERTInextCAPluginTests.cs index 7064b44..5154146 100644 --- a/CERTInext.Tests/CERTInextCAPluginTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginTests.cs @@ -357,6 +357,34 @@ public async Task Enroll_New_ReturnsPendingStatus_WhenCaReturnsPendingApproval() result.Status.Should().Be((int)EndEntityStatus.EXTERNALVALIDATION); } + [Fact] + public async Task Enroll_New_ReturnsPendingStatus_WhenCaReportsIssuedButBodyMissing() + { + // CERTInext can report an "issued"/auto-approved certificateStatusId before the + // certificate bytes actually exist — the immediate GetCertificate download fails + // and the legacy client returns Status="issued" with Certificate=null. Reporting + // GENERATED with no PEM crashes the gateway framework's PEM parser downstream, so + // the plugin must demote this to pending rather than trust the raw status string. + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), + It.IsAny())) + .ReturnsAsync(MockCertificateData.AutoApprovedNoBodyEnrollResponse()); + + var plugin = BuildPluginWithPickup(mock.Object, retries: 0); + + var result = await plugin.Enroll( + csr: MockCertificateData.FakeCsrPem, + subject: "CN=test.example.com", + san: null, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + result.Status.Should().Be((int)EndEntityStatus.EXTERNALVALIDATION); + result.Certificate.Should().BeNullOrEmpty(); + } + // --------------------------------------------------------------------------- // Synchronous certificate pickup (Sectigo parity) // --------------------------------------------------------------------------- diff --git a/CERTInext.Tests/CERTInextClientTests.cs b/CERTInext.Tests/CERTInextClientTests.cs index e473e89..a0ade72 100644 --- a/CERTInext.Tests/CERTInextClientTests.cs +++ b/CERTInext.Tests/CERTInextClientTests.cs @@ -790,6 +790,42 @@ await act.Should().ThrowAsync() .WithMessage("*GetDcv failed*"); } + /// + /// Regression: this client is built with ThrowOnAnyError=false, so RestSharp catches a + /// cancelled HttpClient.SendAsync internally and returns a non-throwing, unsuccessful + /// RestResponse instead of propagating OperationCanceledException. Before this fix, + /// ExecuteWithRetryAsync passed that response straight to DeserializeOrThrow, which wrapped + /// it in a plain Exception — indistinguishable from a genuine API failure. A caller such as + /// PerformDcvIfNeededAsync's per-domain "catch (OperationCanceledException) { throw; }" guard + /// (added specifically to stop a DCV timeout from being mislabeled as an ordinary per-domain + /// failure) could never actually see the real cancellation, because it never arrived as + /// OperationCanceledException in the first place — a gap a Moq-level test of the plugin alone + /// cannot expose, since a mock can be told to throw whatever type is asked for. This test + /// exercises the real client against a real (if local) HTTP call, which is the only way to + /// pin the actual failure mode. + /// + [Fact] + public async Task GetDcvAsync_ThrowsOperationCanceled_WhenCancellationTokenIsCancelled() + { + _server + .Given(Request.Create().WithPath("/GetDcv").UsingPost()) + .RespondWith(Response.Create() + .WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody(MockCertificateData.GetDcvSuccessJson())); + + var client = BuildClient(); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + Func act = () => client.GetDcvAsync( + MockCertificateData.OrderNumber1, "example.com", Constants.Dcv.MethodDnsTxt, cts.Token); + + await act.Should().ThrowAsync( + "a cancelled token must surface as a genuine cancellation, not get wrapped into a " + + "plain Exception that a caller's cancellation-specific catch clause cannot recognize"); + } + [Fact] public async Task GetDcvAsync_Throws_WhenServerReturns401() { diff --git a/CERTInext.Tests/FakeDomainValidator.cs b/CERTInext.Tests/FakeDomainValidator.cs index 6b42475..d3917ec 100644 --- a/CERTInext.Tests/FakeDomainValidator.cs +++ b/CERTInext.Tests/FakeDomainValidator.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. // At http://www.apache.org/licenses/LICENSE-2.0 +using System; using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; @@ -21,10 +22,20 @@ internal sealed class FakeDomainValidator : IDomainValidator /// All keys passed to . public List CleanedUpKeys { get; } = new(); + /// All CancellationTokens passed to . + public List CleanupTokens { get; } = new(); + /// When false, returns a failure result. public bool StageSucceeds { get; init; } = true; - /// Error message returned when is false. + /// + /// When set, overrides on a per-key basis — e.g. + /// key => key.Contains("bad", StringComparison.OrdinalIgnoreCase) to fail only a + /// specific hostname in a multi-domain test while the others still stage successfully. + /// + public Func ShouldFail { get; init; } + + /// Error message returned when a StageValidation call fails. public string StageError { get; init; } = "Stage failed (test stub)"; public void Initialize(IDomainValidatorConfigProvider configProvider) { } @@ -32,18 +43,39 @@ public void Initialize(IDomainValidatorConfigProvider configProvider) { } public Task StageValidation(string key, string value, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); - StagedRecords.Add((key, value)); + bool fail = ShouldFail?.Invoke(key) ?? !StageSucceeds; + if (!fail) + StagedRecords.Add((key, value)); + return Task.FromResult(new DomainValidationResult { - Success = StageSucceeds, - ErrorMessage = StageSucceeds ? null : StageError + Success = !fail, + ErrorMessage = fail ? StageError : null }); } - public Task CleanupValidation(string key, CancellationToken cancellationToken) + /// + /// Artificial delay applied inside before completing — lets + /// tests distinguish "cleanup calls run concurrently" (wall time ~= one delay) from + /// "cleanup calls run sequentially" (wall time ~= N x delay). + /// + public TimeSpan CleanupDelay { get; init; } = TimeSpan.Zero; + + // Cleanup calls can genuinely run concurrently (that's what CleanupDelay exists to prove), + // so the two List fields below need a lock — unlike StagedRecords above, which only ever + // sees synchronously-completing calls in practice. + private readonly object _cleanupLock = new(); + + public async Task CleanupValidation(string key, CancellationToken cancellationToken) { - CleanedUpKeys.Add(key); - return Task.FromResult(new DomainValidationResult { Success = true }); + if (CleanupDelay > TimeSpan.Zero) + await Task.Delay(CleanupDelay, cancellationToken); + lock (_cleanupLock) + { + CleanedUpKeys.Add(key); + CleanupTokens.Add(cancellationToken); + } + return new DomainValidationResult { Success = true }; } public Task ValidateConfiguration(Dictionary configuration) => Task.CompletedTask; @@ -53,15 +85,24 @@ public Task CleanupValidation(string key, CancellationTo /// /// Factory that returns a single pre-configured for every - /// domain. Pass null as the validator to simulate "no DNS provider configured". + /// domain, or only for if set. Pass null as the + /// validator to simulate "no DNS provider configured". /// internal sealed class FakeDomainValidatorFactory : IDomainValidatorFactory { private readonly IDomainValidator _validator; + private readonly string _resolvableDomain; - public FakeDomainValidatorFactory(IDomainValidator validator = null) => _validator = validator; + public FakeDomainValidatorFactory(IDomainValidator validator = null, string resolvableDomain = null) + { + _validator = validator; + _resolvableDomain = resolvableDomain; + } - public IDomainValidator ResolveDomainValidator(string domain, string validationType) => _validator; + public IDomainValidator ResolveDomainValidator(string domain, string validationType) => + (_resolvableDomain == null || string.Equals(domain, _resolvableDomain, StringComparison.OrdinalIgnoreCase)) + ? _validator + : null; /// The validator this factory returns; exposed for assertions in tests. public IDomainValidator PrimaryValidator => _validator; diff --git a/CERTInext.Tests/MockCertificateData.cs b/CERTInext.Tests/MockCertificateData.cs index ee6644b..7714152 100644 --- a/CERTInext.Tests/MockCertificateData.cs +++ b/CERTInext.Tests/MockCertificateData.cs @@ -294,6 +294,20 @@ public static EnrollCertificateResponse PendingEnrollResponse(string id = null) Message = "Awaiting approval." }; + // Reproduces the CERTInext "auto-approved" race: TrackOrder reports a + // certificateStatusId the client legacy-maps to "issued", but the immediate + // GetCertificate download failed (cert bytes not generated yet), so no PEM + // ever arrived. See issue 0009. + public static EnrollCertificateResponse AutoApprovedNoBodyEnrollResponse(string id = null) => + new EnrollCertificateResponse + { + Id = id ?? CertId1, + Status = "issued", + Certificate = null, + ProfileId = ProfileIdTls, + Message = "Order auto-approved." + }; + // ----------------------------------------------------------------------- // GetCertificate response (object helpers — used by Moq-based plugin tests) // These use the legacy inferred type (LegacyGetCertificateResponse). diff --git a/CERTInext.Tests/SanSubmissionTests.cs b/CERTInext.Tests/SanSubmissionTests.cs new file mode 100644 index 0000000..c305665 --- /dev/null +++ b/CERTInext.Tests/SanSubmissionTests.cs @@ -0,0 +1,707 @@ +// Copyright 2026 Keyfactor +// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. +// You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 +// Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions +// and limitations under the License. + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using FluentAssertions; +using Keyfactor.AnyGateway.Extensions; +using Keyfactor.Extensions.CAPlugin.CERTInext.API; +using Keyfactor.Extensions.CAPlugin.CERTInext.Client; +using Moq; +using Org.BouncyCastle.Asn1; +using Org.BouncyCastle.Asn1.Pkcs; +using Org.BouncyCastle.Asn1.X509; +using Org.BouncyCastle.Crypto; +using Org.BouncyCastle.Crypto.Generators; +using Org.BouncyCastle.Pkcs; +using Org.BouncyCastle.Security; +using WireMock.RequestBuilders; +using WireMock.ResponseBuilders; +using WireMock.Server; +using Xunit; + +namespace Keyfactor.Extensions.CAPlugin.CERTInext.Tests +{ + /// + /// Regression tests for UCC SAN submission. + /// + /// The defect these pin down: the AnyCA REST Gateway keys its SAN dictionary + /// dnsname, but MapSanType only recognized dns. Every DNS SAN was + /// therefore typed "dnsname", filtered out by a DNS-only test when building + /// certificateInformation.additionalDomains, and the order reached CERTInext with + /// no additional domains at all — yielding a certificate holding only the CN. Because + /// CERTInext ignores the CSR's subjectAltName extension entirely (measured; see + /// SanSubmissionProbeTests), SANs present on the CSR did not compensate. + /// + /// The end-to-end tests below drive a real against WireMock + /// so they assert on the JSON actually put on the wire, not on an intermediate object. + /// A test that only checked the mapping function would not have caught this bug, since + /// the mapping "worked" — it was the interaction with the downstream filter that lost + /// the names. + /// + public class SanSubmissionTests : IDisposable + { + private readonly WireMockServer _server; + + public SanSubmissionTests() + { + _server = WireMockServer.Start(); + StubHappyEnroll(); + } + + public void Dispose() => _server.Stop(); + + // ----------------------------------------------------------------------- + // Harness + // ----------------------------------------------------------------------- + + private void StubHappyEnroll() + { + _server.Given(Request.Create().WithPath("/GenerateOrderSSL").UsingPost()) + .RespondWith(Response.Create().WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody(MockCertificateData.GenerateOrderSuccessJson(MockCertificateData.OrderNumber1))); + + _server.Given(Request.Create().WithPath("/TrackOrder").UsingPost()) + .RespondWith(Response.Create().WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody(MockCertificateData.TrackOrderIssuedJson(MockCertificateData.OrderNumber1))); + + _server.Given(Request.Create().WithPath("/GetCertificate").UsingPost()) + .RespondWith(Response.Create().WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody(MockCertificateData.GetCertificateSuccessJson())); + } + + private CERTInextClient BuildRealClient() => new CERTInextClient(new CERTInextConfig + { + ApiUrl = _server.Urls[0], + AuthMode = "AccessKey", + ApiKey = "test-key", + AccountNumber = "12345", + RequestorName = "Default Requestor", + RequestorEmail = "default@example.com", + RequestorIsdCode = "1", + RequestorMobileNumber = "5550000000", + SignerPlace = "Austin", + SignerIp = "203.0.113.10", + PageSize = 100 + }); + + /// + /// Plugin wired to a real client pointed at WireMock. PickupRetries = 0 is set on + /// the plugin's own config (not the client's) — that is where the synchronous-pickup + /// budget is read, and leaving it at the default would make every test here sit in a + /// polling loop. + /// + private CERTInextCAPlugin BuildPlugin() => + new CERTInextCAPlugin(BuildRealClient(), new CERTInextConfig { PickupRetries = 0 }); + + private static EnrollmentProductInfo MakeProductInfo(string profileId = "842") => + new EnrollmentProductInfo + { + ProductID = profileId, + ProductParameters = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["ProfileId"] = profileId + } + }; + + /// The orderDetails.certificateInformation block actually POSTed. + private JsonElement CapturedCertificateInformation() + { + var posts = _server.LogEntries + .Where(e => e.RequestMessage.Path == "/GenerateOrderSSL") + .ToList(); + posts.Should().HaveCount(1, "exactly one GenerateOrderSSL POST should have been emitted"); + + string body = posts[0].RequestMessage.Body; + body.Should().NotBeNullOrEmpty(); + + return JsonDocument.Parse(body!).RootElement + .GetProperty("orderDetails") + .GetProperty("certificateInformation"); + } + + private static List AdditionalDomains(JsonElement certificateInformation) => + certificateInformation.TryGetProperty("additionalDomains", out var el) + ? el.EnumerateArray().Select(x => x.GetString()).ToList() + : null; + + // ----------------------------------------------------------------------- + // CSR generation (BouncyCastle — project crypto policy) + // ----------------------------------------------------------------------- + + /// + /// Builds a real PKCS#10 CSR for carrying arbitrary + /// in its subjectAltName extension — used to exercise + /// GeneralName types that have no domain-name rendering. + /// + private static string GenerateCsrPemWithGeneralNames(string cn, params GeneralName[] names) + { + var keyGen = new RsaKeyPairGenerator(); + keyGen.Init(new KeyGenerationParameters(new SecureRandom(), 2048)); + AsymmetricCipherKeyPair kp = keyGen.GenerateKeyPair(); + + Asn1Set attributes = null; + if (names != null && names.Length > 0) + { + var extGen = new X509ExtensionsGenerator(); + extGen.AddExtension(X509Extensions.SubjectAlternativeName, critical: false, + extValue: new GeneralNames(names)); + + attributes = new DerSet(new AttributePkcs( + PkcsObjectIdentifiers.Pkcs9AtExtensionRequest, + new DerSet(extGen.Generate()))); + } + + var csr = new Pkcs10CertificationRequest( + "SHA256withRSA", new X509Name($"CN={cn}"), kp.Public, attributes, kp.Private); + + return "-----BEGIN CERTIFICATE REQUEST-----\n" + + Convert.ToBase64String(csr.GetEncoded(), Base64FormattingOptions.InsertLineBreaks) + + "\n-----END CERTIFICATE REQUEST-----"; + } + + /// + /// Builds a real PKCS#10 CSR for , optionally carrying a + /// subjectAltName extension holding . + /// + private static string GenerateCsrPem(string cn, params string[] dnsSans) => + GenerateCsrPemWithGeneralNames( + cn, (dnsSans ?? Array.Empty()).Select(d => new GeneralName(GeneralName.DnsName, d)).ToArray()); + + // ======================================================================= + // End-to-end: Command's SAN dictionary → the JSON on the wire + // ======================================================================= + + /// + /// THE regression test. "dnsname" is the key the real gateway sends — verified against + /// a customer gateway log: + /// SANs=dnsname:CLAUDIOTEST20.ucsd.edu; dnsname:CLAUDIOTEST20.ad.ucsd.edu + /// Before the fix, additionalDomains was absent from the body entirely. + /// + [Fact] + public async Task GatewayDnsNameKey_ReachesAdditionalDomains() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "host.example.com", "alt.example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + var certInfo = CapturedCertificateInformation(); + certInfo.GetProperty("domainName").GetString().Should().Be("host.example.com"); + + AdditionalDomains(certInfo).Should().BeEquivalentTo(new[] { "alt.example.com" }, + "the extra SAN must reach additionalDomains, and the CN must not be repeated there"); + } + + /// + /// The short "dns" spelling must keep working — some callers and older hosts use it. + /// + [Fact] + public async Task ShortDnsKey_StillReachesAdditionalDomains() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dns"] = new[] { "alt.example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "alt.example.com" }); + } + + /// + /// SANs present only on the CSR must still reach additionalDomains. CERTInext does not + /// read the CSR's SAN extension, so if we don't forward these the names never appear + /// on the certificate. + /// + [Fact] + public async Task CsrSans_ReachAdditionalDomains_WhenGatewaySuppliesNone() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com", "host.example.com", "fromcsr.example.com"), + subject: "CN=host.example.com", + san: null, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "fromcsr.example.com" }); + } + + /// + /// Union, not either/or: names unique to each source survive and the overlap collapses. + /// + [Fact] + public async Task CsrOnlySans_AreIgnored_WhenGatewaySuppliesAnyEntries() + { + // Regression: this test used to assert the CSR was unioned in on top of whatever the + // gateway supplied. Full-review's security lens found that risky: Command's SAN + // dictionary is how an enrollment pattern's SAN policy is expressed, and a signed CSR — + // usually generated by the subscriber's own tooling, not by Command — can legitimately + // carry more names than that policy allows. Unioning them in would re-introduce a name + // the policy excluded. The CSR is now consulted only as a fallback when the gateway + // supplies nothing at all (see CsrSans_ReachAdditionalDomains_WhenGatewaySuppliesNone). + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com", "host.example.com", "csronly.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "gatewayonly.example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + var domains = AdditionalDomains(CapturedCertificateInformation()); + + domains.Should().BeEquivalentTo(new[] { "gatewayonly.example.com" }, + "the gateway supplied a (non-empty) SAN set, so the CSR's own SAN extension must be " + + "ignored entirely, not merged in on top of it"); + } + + /// + /// Regression: the CSR-fallback trigger used to be "the gateway dictionary computed to zero + /// added entries", which cannot distinguish "Command never populated SAN data" (the case the + /// fallback exists for) from "Command's enrollment pattern ran and deliberately computed + /// zero SANs for this request" (an explicit policy decision this plugin must respect). A + /// non-null dictionary whose only key maps to an empty array is the latter — the fallback + /// must not engage, even though it computes to the same "0 SANs added" outcome as a null + /// dictionary would. + /// + [Fact] + public async Task CsrSans_AreIgnored_WhenGatewaySuppliesNonNullDictWithOnlyEmptyValues() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com", "host.example.com", "csronly.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + // Non-null dictionary, but the key maps to no values — computes to zero added + // SANs, same as san == null would, but it must NOT be treated the same way. + ["dnsname"] = Array.Empty() + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()).Should().BeNull( + "a non-null gateway SAN dictionary that computes to zero entries must be respected " + + "as Command's own decision, not treated as 'Command supplied nothing' and " + + "backfilled from the CSR"); + } + + /// + /// The CN is already submitted as domainName; repeating it in additionalDomains is + /// suppressed. CERTInext collapses it anyway (measured), so this keeps the body matching + /// what we log rather than relying on undocumented CA-side behaviour. + /// + [Fact] + public async Task Cn_IsNotRepeatedInAdditionalDomains() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com", "host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "host.example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + var certInfo = CapturedCertificateInformation(); + certInfo.GetProperty("domainName").GetString().Should().Be("host.example.com"); + AdditionalDomains(certInfo).Should().BeNull( + "with the CN as the only SAN there is nothing left to send, so the field is omitted"); + } + + /// + /// Non-DNS SANs are submitted rather than silently discarded. CERTInext accepts them + /// verbatim (measured) and the resulting order cannot pass validation — a visible + /// failure, deliberately preferred over issuing a certificate that quietly lacks names + /// the subscriber requested. + /// + [Fact] + public async Task NonDnsSans_AreSubmitted_NotDropped() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "alt.example.com" }, + ["ipaddress"] = new[] { "192.0.2.10" }, + ["rfc822name"] = new[] { "admin@example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "alt.example.com", "192.0.2.10", "admin@example.com" }); + } + + /// + /// Regression for a stale-count bug in BuildSanList's own audit log: with a mixed + /// DNS/non-DNS gateway SAN dictionary and SubmitNonDnsSans=false, the "Resolved N SAN(s)" + /// log line reported the pre-filter gateway count (3) alongside the post-filter total (1) — + /// an arithmetic impossibility ("Resolved 1 ... FromGatewayRequest=3"). There is no log- + /// capture seam in this codebase (ILogger comes from a fixed LogHandler.GetClassLogger() + /// field, not an injectable dependency), so this pins the payload-level data the log line is + /// computed from instead: with the non-DNS entries filtered out, exactly the one DNS name + /// must reach additionalDomains — proving the surviving gateway-sourced count is 1, not the + /// pre-filter 3 the stale log line used to claim. + /// + [Fact] + public async Task MixedGatewaySans_SubmitNonDnsSansFalse_OnlyDnsNameSurvivesFiltering() + { + var plugin = new CERTInextCAPlugin( + BuildRealClient(), + new CERTInextConfig { PickupRetries = 0, SubmitNonDnsSans = false }); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "alt.example.com" }, + ["ipaddress"] = new[] { "192.0.2.10" }, + ["rfc822name"] = new[] { "admin@example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "alt.example.com" }, + "only the DNS entry should survive the SubmitNonDnsSans=false filter, out of " + + "3 the gateway supplied"); + } + + /// + /// A CSR we cannot parse must not break enrollment — the gateway-supplied SANs still go. + /// FakeCsrPem is deliberately truncated, so this also guards the many existing + /// tests that pass it. + /// + [Fact] + public async Task UnparseableCsr_DoesNotBlockGatewaySuppliedSans() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: MockCertificateData.FakeCsrPem, + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "alt.example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "alt.example.com" }); + } + + /// + /// No SANs from either source → the field is omitted rather than emitted as null/empty. + /// + [Fact] + public async Task NoSansAnywhere_OmitsAdditionalDomains() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: null, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()).Should().BeNull(); + } + + // ======================================================================= + // GeneralName types with no domain-name rendering + // ======================================================================= + + /// + /// A UPN otherName and a directoryName must NOT be submitted. + /// + /// Regression: GeneralNameToValue's default branch returned BouncyCastle's ASN.1 + /// stringification, so a Windows-generated CSR carrying a UPN otherName put + /// "[1.3.6.1.4.1.311.20.2.3, [CONTEXT 0]svc@corp.example.com]" into additionalDomains as if + /// it were a domain name — breaking orders that previously succeeded, and contradicting the + /// method's own doc comment. These types cannot become a certificate SAN via a domain-name + /// field at all, which is why they are skipped (with a Warning) rather than submitted the way + /// well-formed IP/email/URI SANs are. + /// + [Fact] + public async Task CsrOtherNameAndDirectoryName_AreNotSubmittedAsDomains() + { + // UPN otherName, as emitted by Windows/AD certificate tooling. + var upn = new GeneralName(GeneralName.OtherName, new DerSequence( + new DerObjectIdentifier("1.3.6.1.4.1.311.20.2.3"), + new DerTaggedObject(true, 0, new DerUtf8String("svc@corp.example.com")))); + + var directoryName = new GeneralName( + GeneralName.DirectoryName, new X509Name("CN=host.example.com,O=Acme")); + + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPemWithGeneralNames( + "host.example.com", + new GeneralName(GeneralName.DnsName, "alt.example.com"), + upn, + directoryName), + subject: "CN=host.example.com", + san: null, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + var domains = AdditionalDomains(CapturedCertificateInformation()); + + domains.Should().BeEquivalentTo(new[] { "alt.example.com" }, + "only the renderable DNS name may be submitted"); + domains.Should().NotContain(d => d.Contains("1.3.6.1.4.1.311.20.2.3"), + "an otherName must never be submitted as an ASN.1 dump"); + domains.Should().NotContain(d => d.Contains("CONTEXT"), + "BouncyCastle ASN.1 debris must never reach the wire"); + domains.Should().NotContain(d => d.StartsWith("CN=", StringComparison.OrdinalIgnoreCase), + "a directoryName must never be submitted as a domain"); + } + + // ======================================================================= + // Log-injection hardening (CWE-117) + // ======================================================================= + + /// + /// SAN values reach the log from the CSR and from Command's SAN dictionary — i.e. from the + /// requester. Structured message templates stop format-string abuse but not embedded + /// newlines, so a value carrying CRLF could forge audit records in the very log lines added + /// to make the submitted SAN set auditable. LogSanitizer is internal (not private) and + /// shared between the plugin and the client, so this is a direct call, not reflection. + /// + [Theory] + [InlineData("evil.example.com\r\nINFO forged record", "evil.example.com\\r\\nINFO forged record")] + [InlineData("a\nb", "a\\nb")] + [InlineData("a\tb", "a\\tb")] + [InlineData("plain.example.com", "plain.example.com")] + [InlineData("", "")] + [InlineData(null, null)] + public void SanitizeForLog_NeutralizesControlCharacters(string input, string expected) + { + var actual = Keyfactor.Extensions.CAPlugin.CERTInext.Models.LogSanitizer.Strip(input); + + actual.Should().Be(expected); + } + + /// + /// A CRLF-bearing SAN must not break enrollment, and the value is still submitted verbatim — + /// the scrub is a logging concern and deliberately does not mutate the payload sent to the CA. + /// + [Fact] + public async Task SanValueWithCrLf_DoesNotBreakEnrollment() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "alt.example.com\r\nforged log line" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().ContainSingle().Which.Should().Contain("alt.example.com"); + } + + // ======================================================================= + // SubmitNonDnsSans escape hatch + // ======================================================================= + + /// + /// Submitting non-DNS SANs flips affected enrollments from "issues, silently missing the + /// name" to "parks pending". SubmitNonDnsSans=false restores the pre-1.0.1 behaviour so an + /// upgraded host has a way back that isn't a plugin downgrade. + /// + [Fact] + public async Task SubmitNonDnsSansFalse_SubmitsDnsNamesOnly() + { + var plugin = new CERTInextCAPlugin( + BuildRealClient(), + new CERTInextConfig { PickupRetries = 0, SubmitNonDnsSans = false }); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "alt.example.com" }, + ["ipaddress"] = new[] { "192.0.2.10" }, + ["rfc822name"] = new[] { "admin@example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "alt.example.com" }, + "with the switch off, only DNS names are submitted"); + } + + /// + /// The switch defaults to true, so the documented default behaviour is pinned independently + /// of any test that sets it explicitly. + /// + [Fact] + public void SubmitNonDnsSans_DefaultsToTrue() + { + new CERTInextConfig().SubmitNonDnsSans.Should().BeTrue(); + } + + /// + /// Regression for a self-contradicting audit record: BuildSanList used to log "N SAN(s) + /// ... have been added to the order" for CSR-fallback entries, then filter exactly those + /// entries back out two lines later when SubmitNonDnsSans is false — a false claim in the + /// same call. The fix reordered the method to filter first and log the final result, which + /// this test exercises functionally: with the gateway supplying nothing (so the CSR fallback + /// engages) and a non-DNS CSR SAN present, SubmitNonDnsSans=false must still result in that + /// name being genuinely absent from the wire, not merely mis-described in the log. + /// + [Fact] + public async Task CsrFallbackNonDnsSan_IsExcluded_WhenSubmitNonDnsSansFalse() + { + var plugin = new CERTInextCAPlugin( + BuildRealClient(), + new CERTInextConfig { PickupRetries = 0, SubmitNonDnsSans = false }); + + await plugin.Enroll( + csr: GenerateCsrPemWithGeneralNames( + "host.example.com", + new GeneralName(GeneralName.DnsName, "host.example.com"), + new GeneralName(GeneralName.DnsName, "alt.example.com"), + new GeneralName(GeneralName.Rfc822Name, "admin@example.com")), + subject: "CN=host.example.com", + san: null, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "alt.example.com" }, + "the CSR-fallback email SAN must be genuinely absent from the order, not just " + + "misreported as present"); + } + + // ======================================================================= + // Renew path — previously submitted no SANs at all + // ======================================================================= + + /// + /// A renewal that goes through the CERTInext renew API must carry the same domain set + /// as a new enrollment, and must take its primary domain from the subject's CN rather + /// than from the prior order's requestor name. + /// + [Fact] + public async Task RenewalRequest_CarriesSubjectAndSans() + { + var clientMock = new Mock(MockBehavior.Loose); + RenewCertificateRequest captured = null; + + clientMock + .Setup(c => c.RenewCertificateAsync( + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Callback((_, req, __) => captured = req) + .ReturnsAsync(MockCertificateData.IssuedEnrollResponse()); + + var readerMock = new Mock(MockBehavior.Loose); + readerMock + .Setup(r => r.GetRequestIDBySerialNumber(It.IsAny())) + .ReturnsAsync("PRIOR-ORDER-1"); + + // The renewal-window decision reads expiry from the data reader, not from the CA. + // Put the prior cert 10 days out so it lands inside the 30-day window below and the + // renew API path is actually taken. + readerMock + .Setup(r => r.GetExpirationDateByRequestId(It.IsAny())) + .Returns(DateTime.UtcNow.AddDays(10)); + + var plugin = new CERTInextCAPlugin(clientMock.Object, readerMock.Object); + + var productInfo = MakeProductInfo(); + productInfo.ProductParameters["PriorCertSN"] = "AABBCCDDEEFF"; + productInfo.ProductParameters["RenewalWindowDays"] = "30"; + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com", "host.example.com", "alt.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "host.example.com", "alt.example.com" } + }, + productInfo: productInfo, + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.Renew); + + captured.Should().NotBeNull("the renew API path should have been taken"); + + // Bind to a local so the compiler's null-flow analysis is satisfied — a + // FluentAssertions NotBeNull() does not narrow the nullable reference. + RenewCertificateRequest renewReq = captured!; + + renewReq.Subject.Should().Be("CN=host.example.com", + "without the subject the renewal order has no usable primary domain"); + renewReq.Sans.Should().NotBeNull("renewals previously dropped every SAN"); + renewReq.Sans.Select(s => s.Value) + .Should().BeEquivalentTo(new[] { "host.example.com", "alt.example.com" }); + } + } +} diff --git a/CERTInext/API/CertificateRequest.cs b/CERTInext/API/CertificateRequest.cs index 7f02df0..043b4b5 100644 --- a/CERTInext/API/CertificateRequest.cs +++ b/CERTInext/API/CertificateRequest.cs @@ -622,6 +622,23 @@ public class RenewCertificateRequest [JsonPropertyName("csr")] public string Csr { get; set; } + /// + /// Distinguished name of the certificate being renewed. Supplies the renewal order's + /// primary domain via its CN — without it the renewal falls back to the prior order's + /// requestor name, which is not a domain at all. + /// + [JsonPropertyName("subject")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public string Subject { get; set; } + + /// + /// SANs to carry onto the renewal order. Renewals previously submitted none, so a + /// renewed UCC certificate came back holding only its primary domain. + /// + [JsonPropertyName("sans")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public System.Collections.Generic.List Sans { get; set; } + [JsonPropertyName("validityDays")] [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] public int? ValidityDays { get; set; } diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index b04c051..a631606 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -247,14 +247,14 @@ public void Initialize(IAnyCAPluginConfigProvider configProvider, ICertificateDa "ApiKeyPresent={ApiKeyPresent}, UsernamePresent={UsernamePresent}, " + "PasswordPresent={PasswordPresent}, OAuth2ClientIdPresent={OAuth2ClientIdPresent}, " + "OAuth2ClientSecretPresent={OAuth2ClientSecretPresent}, OAuth2TokenUrlPresent={OAuth2TokenUrlPresent}, " + - "PageSize={PageSize}, IgnoreExpired={IgnoreExpired}, " + + "PageSize={PageSize}, IgnoreExpired={IgnoreExpired}, SubmitNonDnsSans={SubmitNonDnsSans}, " + "DcvEnabled={DcvEnabled}, DcvTxtRecordTemplate={DcvTxtRecordTemplate}, " + "DomainValidatorFactoryInjected={FactoryInjected}", _config.ApiUrl, _config.AuthMode, _config.Enabled, hasApiKey, hasUsername, hasPassword, hasClientId, hasClientSecret, hasTokenUrl, - _config.PageSize, _config.IgnoreExpired, + _config.PageSize, _config.IgnoreExpired, _config.SubmitNonDnsSans, _config.DcvEnabled, _config.DcvTxtRecordTemplate, _domainValidatorFactory != null); @@ -576,15 +576,15 @@ public async Task Enroll( "EnrollmentType={EnrollmentType}, RequestFormat={RequestFormat}, Subject={Subject}, " + "ProfileId={ProfileId}, SANs={SANs}, " + "RequesterName={RequesterName}, RequesterEmail={RequesterEmail}", - enrollmentType, requestFormat, subject, - ep.ProfileId, sanSummary, + enrollmentType, requestFormat, LogSanitizer.Strip(subject), + ep.ProfileId, LogSanitizer.Strip(sanSummary), ep.RequesterName, ep.RequesterEmail); if (string.IsNullOrWhiteSpace(ep.ProfileId)) { _logger.LogError( "Enrollment rejected — ProfileId parameter is missing. Subject={Subject}, EnrollmentType={EnrollmentType}", - subject, enrollmentType); + LogSanitizer.Strip(subject), enrollmentType); throw new Exception($"Template parameter '{Constants.EnrollmentParam.ProfileId}' is required."); } @@ -605,7 +605,7 @@ public async Task Enroll( default: _logger.LogError( "Enrollment rejected — unsupported enrollment type. EnrollmentType={EnrollmentType}, Subject={Subject}", - enrollmentType, subject); + enrollmentType, LogSanitizer.Strip(subject)); throw new NotSupportedException($"Enrollment type '{enrollmentType}' is not supported."); } @@ -617,7 +617,7 @@ public async Task Enroll( "SerialNumber={SerialNumber}, Subject={Subject}, ProfileId={ProfileId}", enrollmentType, result.CARequestID, result.Status, result.Certificate != null ? ExtractSerialFromPem(result.Certificate) : "(pending)", - subject, ep.ProfileId); + LogSanitizer.Strip(subject), ep.ProfileId); _logger.MethodExit(LogLevel.Debug); return result; } @@ -727,7 +727,7 @@ public async Task Revoke(string caRequestID, string hexSerialNumber, uint r _logger.LogWarning( "Revocation skipped — certificate is already revoked. " + "CARequestID={Id}, HexSerialNumber={Serial}, Subject={Subject}", - caRequestID, hexSerialNumber, current.Subject); + caRequestID, hexSerialNumber, LogSanitizer.Strip(current.Subject)); return (int)EndEntityStatus.REVOKED; } @@ -756,7 +756,7 @@ public async Task Revoke(string caRequestID, string hexSerialNumber, uint r "Revocation complete. " + "CARequestID={Id}, HexSerialNumber={Serial}, Subject={Subject}, " + "ReasonCode={ReasonCode}, ReasonString={ReasonString}", - caRequestID, hexSerialNumber, current.Subject, + caRequestID, hexSerialNumber, LogSanitizer.Strip(current.Subject), revocationReason, reasonString); _logger.MethodExit(LogLevel.Debug); return (int)EndEntityStatus.REVOKED; @@ -937,7 +937,8 @@ public async Task Synchronize( status = StatusMapper.ToRequestDisposition(current.Status); _logger.LogDebug( "Sync: refetched order Id={Id} — status={Status}, certBytes={Bytes}, subject={Subject}.", - current.Id, status, current.Certificate?.Length ?? 0, current.Subject); + current.Id, status, current.Certificate?.Length ?? 0, + LogSanitizer.Strip(current.Subject)); } catch (Exception fetchEx) { @@ -970,7 +971,8 @@ public async Task Synchronize( } _logger.LogDebug( "Sync emit: CARequestID={Id}, Status={Status}, CertBytes={CertBytes}, Subject={Subject}", - record.CARequestID, record.Status, record.Certificate?.Length ?? 0, current.Subject); + record.CARequestID, record.Status, record.Certificate?.Length ?? 0, + LogSanitizer.Strip(current.Subject)); blockingBuffer.Add(record, cancelToken); synced++; @@ -1096,7 +1098,7 @@ private async Task EnrollNewAsync( Csr = csr, ValidityDays = ep.ValidityDays > 0 ? ep.ValidityDays : (int?)null, Subject = subject, - Sans = BuildSanList(san), + Sans = BuildSanList(san, csr, subject), RequesterName = string.IsNullOrWhiteSpace(ep.RequesterName) ? null : ep.RequesterName, RequesterEmail = string.IsNullOrWhiteSpace(ep.RequesterEmail) ? null : ep.RequesterEmail, KeyType = string.IsNullOrWhiteSpace(ep.KeyType) ? null : ep.KeyType, @@ -1229,7 +1231,7 @@ private async Task RenewOrReissueAsync( _logger.LogInformation( "Renewal/reissue probe — read PriorCertSN from EnrollmentProductInfo. " + "Subject={Subject}, PriorCertSN={PriorCertSN}, RenewalWindowDays={WindowDays}", - subject, string.IsNullOrWhiteSpace(priorCertSn) ? "(none)" : priorCertSn, + LogSanitizer.Strip(subject), string.IsNullOrWhiteSpace(priorCertSn) ? "(none)" : priorCertSn, ep.RenewalWindowDays); if (string.IsNullOrWhiteSpace(priorCertSn)) @@ -1238,7 +1240,7 @@ private async Task RenewOrReissueAsync( // production log filters and are available for anomaly detection. _logger.LogInformation( "Renewal/reissue has no PriorCertSN — treating as new enrollment. Subject={Subject}", - subject); + LogSanitizer.Strip(subject)); return await EnrollNewAsync(csr, subject, san, ep); } @@ -1259,7 +1261,7 @@ private async Task RenewOrReissueAsync( { _logger.LogInformation( "CARequestID for serial '{SN}' is empty — falling back to new enrollment. Subject={Subject}", - priorCertSn, subject); + priorCertSn, LogSanitizer.Strip(subject)); return await EnrollNewAsync(csr, subject, san, ep); } @@ -1308,11 +1310,16 @@ private async Task RenewOrReissueAsync( _logger.LogInformation( "Renewal via CERTInext renew API started. " + "PriorCARequestID={PriorId}, Subject={Subject}, ProfileId={ProfileId}", - priorCaRequestId, subject, ep.ProfileId); + priorCaRequestId, LogSanitizer.Strip(subject), ep.ProfileId); var renewReq = new RenewCertificateRequest { Csr = csr, + // Renewals go out as a fresh CERTInext order, so they need the same domain + // set as a new enrollment — otherwise a renewed UCC certificate comes back + // holding only its primary domain. + Subject = subject, + Sans = BuildSanList(san, csr, subject), ValidityDays = ep.ValidityDays > 0 ? ep.ValidityDays : (int?)null, RequesterName = string.IsNullOrWhiteSpace(ep.RequesterName) ? null : ep.RequesterName, RequesterEmail = string.IsNullOrWhiteSpace(ep.RequesterEmail) ? null : ep.RequesterEmail, @@ -1340,7 +1347,7 @@ private async Task RenewOrReissueAsync( { _logger.LogInformation( "Certificate '{Id}' is outside the renewal window ({Window} days) — issuing new certificate. Subject={Subject}", - priorCaRequestId, ep.RenewalWindowDays, subject); + priorCaRequestId, ep.RenewalWindowDays, LogSanitizer.Strip(subject)); return await EnrollNewAsync(csr, subject, san, ep); } } @@ -1602,27 +1609,61 @@ private async Task PerformDcvIfNeededAsync( // SOX CC6.1: validate domain names before passing them to the DNS provider plugin // or the CERTInext API. A malformed domain (empty, whitespace, or containing // characters outside the FQDN alphabet) could cause log injection or unexpected - // DNS plugin behaviour. Invalid entries are rejected loudly rather than silently - // skipped so the condition is visible in the audit trail. - foreach (var (domain, _) in pendingDomains) + // DNS plugin behaviour. Invalid entries are rejected loudly — LogError, so the + // condition is visible in the audit trail — but they are EXCLUDED rather than + // thrown on. + // + // Throwing here would fail the whole order: the exception escapes Enroll (which has + // no catch) after the order was already placed at the CA, so the enrollment reports + // failure with an orphaned order, and no TXT record is staged for the *valid* domains + // on the same order. Worse, it is unrecoverable — every later Synchronize / + // GetSingleRecord retry re-enters here, hits the same undrainable domain, and + // TryRunDcvDuringSyncAsync swallows the exception and returns false, so the order sits + // at EXTERNALVALIDATION forever. + // + // This is reachable in normal operation now that non-DNS SANs are submitted to + // CERTInext (see BuildSanList): the CA registers an email/URI SAN verbatim as an order + // domain, and that key is not an FQDN. One such SAN must not strand the DNS names + // alongside it. Same principle the EMS-956 branch below states explicitly: do not throw + // out of DCV for a condition that leaves the order legitimately pending. + var invalidDomains = new List(); + var validPendingDomains = new List>(); + + foreach (var entry in pendingDomains) { - if (string.IsNullOrWhiteSpace(domain)) - throw new InvalidOperationException( - $"TrackOrder returned a blank domain key in domainVerification for order '{orderNumber}'. " + - "Cannot proceed with DCV."); + string domain = entry.Key; - // Allow standard FQDN characters plus wildcard prefix (*.example.com) - if (!System.Text.RegularExpressions.Regex.IsMatch(domain, @"^(\*\.)?[a-zA-Z0-9]([a-zA-Z0-9\-\.]*[a-zA-Z0-9])?$")) - { - _logger.LogError( - "DCV domain name failed validation and will not be processed. OrderNumber={OrderNumber}, Domain={Domain}", - orderNumber, domain); - throw new InvalidOperationException( - $"TrackOrder returned an invalid domain name '{domain}' in domainVerification for order '{orderNumber}'. " + - "Domain names must conform to FQDN syntax."); - } + // Allow standard FQDN characters plus wildcard prefix (*.example.com). + // + // \A/\z, not ^/$: in .NET's default (non-Multiline) mode, $ matches immediately + // before a single trailing '\n', not only at the true end of the string — so + // "evil.com\n" passes a ^...$ version of this regex. \A and \z are absolute + // start/end-of-string anchors regardless of RegexOptions, so a value with any + // trailing control character is correctly rejected here rather than reaching the + // unsanitized-looking-safe domain this validation exists to guarantee. + bool valid = !string.IsNullOrWhiteSpace(domain) + && System.Text.RegularExpressions.Regex.IsMatch( + domain, @"\A(\*\.)?[a-zA-Z0-9]([a-zA-Z0-9\-\.]*[a-zA-Z0-9])?\z"); + + if (valid) + validPendingDomains.Add(entry); + else + invalidDomains.Add(string.IsNullOrWhiteSpace(domain) ? "(blank)" : domain); } + if (invalidDomains.Count > 0) + { + _logger.LogError( + "{Count} domain(s) on order {OrderNumber} are not valid FQDNs and cannot be DNS-01 validated: " + + "[{Domains}]. They are skipped so the remaining {ValidCount} domain(s) can still be validated. " + + "This order cannot be issued by CERTInext until these are removed — they usually come from a " + + "non-DNS SAN (IP address, email, URI) that was requested on the enrollment.", + invalidDomains.Count, orderNumber, LogSanitizer.Strip(string.Join(", ", invalidDomains)), + validPendingDomains.Count); + } + + pendingDomains = validPendingDomains; + if (pendingDomains.Count == 0) return false; @@ -1632,62 +1673,256 @@ private async Task PerformDcvIfNeededAsync( var stagedValidations = new List<(string domain, string hostname, Keyfactor.AnyGateway.Extensions.IDomainValidator validator)>(); - // Stage DNS TXT records for all pending domains - foreach (var (domain, _) in pendingDomains) + // Domains this pass could not stage, with why — purely for the summary LogError after + // the loop. Every failure mode below is loud (its own LogError, sanitized) before being + // skipped, so nothing here is silent; this list just avoids repeating that detail twice. + var skippedDomains = new List<(string domain, string reason)>(); + + // Set instead of an immediate `return false` inside the loop below, so a not-yet-ready + // deferral goes through the same cleanup as every other exit path — see the try/catch + // around the loop. + bool deferToNextSyncCycle = false; + + // Removes whatever TXT records were already published before an early exit from the + // staging loop. Nothing else in this method cleans up mid-loop: the try/finally further + // down only runs once every pending domain has been staged, so without this, an early + // exit orphans every TXT record already published for the earlier domains in the *same* + // order — permanently, since nothing else in the codebase calls CleanupValidation for + // them. Kept even though every per-domain failure below is now skip-and-continue rather + // than throw: it is the safety net for a genuinely unexpected exception (cancellation, a + // bug, a validator implementation that throws instead of returning a failure result). + // + // Shares its per-entry cleanup logic with the try/finally's own cleanup loop further + // down via CleanupOneStagedValidation — the two call sites differ only in when they run + // (an early exit here vs. always-run-at-the-end there), not in what "clean up one TXT + // record" means. + async Task CleanupPartialStagingAsync() + { + // Concurrent, not sequential: each cleanup call already has its own independent + // CleanupValidationTimeoutSeconds bound (see CleanupOneStagedValidationAsync), but + // running them one after another meant that bound was per-call, not in aggregate — a + // UCC order with N staged domains could hold the calling request open for up to + // N × CleanupValidationTimeoutSeconds if the DNS provider was merely slow (not even + // hung) on every delete, which can exceed DcvTimeoutMinutes itself and defeats the + // "entire DCV flow is hard-timeout-bounded" guarantee for exactly the multi-SAN case + // this diff exists to support. Running them concurrently bounds the wall-clock time + // for the whole batch to the slowest single call, regardless of domain count — these + // are independent per-domain operations (different hostnames/records) with no shared + // mutable state, so there is nothing for concurrent execution to race on. + await Task.WhenAll(stagedValidations.Select(entry => + CleanupOneStagedValidationAsync(entry, " after an early exit from DCV staging"))); + } + + // Shared by CleanupPartialStagingAsync above and the try/finally's own cleanup loop + // below — both mean "remove one already-published TXT record", just at different times + // (an early exit vs. always-run-at-the-end). `context` distinguishes the two in the log + // text without duplicating the try/catch/log structure itself. + async Task CleanupOneStagedValidationAsync( + (string domain, string hostname, Keyfactor.AnyGateway.Extensions.IDomainValidator validator) entry, + string context) { - GetDcvResponse dcvResp; + var (domain, hostname, validator) = entry; try { - dcvResp = await _client.GetDcvAsync(orderNumber, domain, Constants.Dcv.MethodDnsTxt, ct); - } - catch (Exception ex) when (IsDcvNotYetReady(ex)) - { - // CERTInext occasionally exposes the DCV slot in TrackOrder (so - // domainVerification is populated and dcvStatus="0") before the GetDcv - // endpoint will accept calls for that order — observed as EMS-956 - // "Invalid Request for this API" for several hours after enrollment. - // Treat this as "DCV not ready yet": skip the DCV ceremony for now and - // let the sync-driven retry pick it up on a later cycle. We must NOT - // throw, because that would fail the entire Enroll call and prevent the - // gateway from recording the pending order at all. + // A fresh, independently-bounded token — deliberately neither `ct` nor + // CancellationToken.None. + // + // Not `ct`: this is a best-effort compensating action — removing a TXT record we + // already published — and it must run regardless of WHY we are cleaning up, + // including the case where `ct` itself is the reason (the dominant real trigger + // for the early-exit call site is the shared DcvTimeoutMinutes-bound token firing + // mid-loop, which means `ct` is guaranteed already cancelled there). A + // cooperative IDomainValidator that forwards its token into its own HTTP calls — + // the reference CloudflareDomainValidator in this repo does exactly that — would + // throw immediately on an already-cancelled token and never even attempt the + // delete, silently leaving the record published with only a Warning logged. + // + // Not CancellationToken.None either: this method's own SOX CC7.3 guarantee is + // that the whole DCV flow is hard-timeout-bounded so a stuck DNS provider cannot + // hold a gateway worker thread indefinitely. That bound has to come from + // somewhere for THIS call too — including the routine, always-runs finally-block + // cleanup on the ordinary successful-DCV path, which was never cancellation- + // related to begin with and would otherwise hang forever on a DNS provider + // plugin whose underlying network call stalls. + using var cleanupCts = new CancellationTokenSource( + TimeSpan.FromSeconds(Constants.Dcv.CleanupValidationTimeoutSeconds)); + await validator.CleanupValidation(hostname, cleanupCts.Token); _logger.LogInformation( - "GetDcv not yet accepting calls for order {OrderNumber} domain {Domain} ({Error}). " + - "Deferring DCV to the next sync cycle.", - orderNumber, domain, ex.Message); - return false; + "DNS TXT record cleaned up{Context}. Domain={Domain}, Hostname={Hostname}", + context, LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); } catch (Exception ex) { - _logger.LogError(ex, "GetDcv failed for order {OrderNumber} domain {Domain}", orderNumber, domain); - throw; + _logger.LogWarning(ex, + "Failed to clean up DNS TXT record{Context}. Domain={Domain}, Hostname={Hostname}. " + + "May require manual removal.", + context, LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); } + } - string token = dcvResp.DcvDetails?.Token; - if (string.IsNullOrWhiteSpace(token)) - throw new InvalidOperationException( - $"GetDcv returned no token for order '{orderNumber}' domain '{domain}'."); + try + { + // Stage DNS TXT records for all pending domains. Every failure below is scoped to + // the one domain that hit it — logged loudly (LogError, so the audit trail carries + // the reason before the domain is dropped) and skipped, never thrown. A throw here + // would abort the WHOLE order after Enroll already placed it at the CA — Enroll has + // no catch around this call, so the exception would escape as a failed enrollment + // with an orphaned CERTInext order, and TryRunDcvDuringSyncAsync would swallow the + // same exception on every later sync retry, leaving the order stuck at + // EXTERNALVALIDATION forever. That is worse than parking the order pending with a + // clear log entry, for EVERY failure shape here — not just the ones distinguishable + // as "bad input" — because nothing downstream ever gets to see or act on the + // exception anyway. This directly caused three real regressions across the first two + // rounds of fixing this file: a GetDcv error or an empty token for a non-DNS SAN + // (submitted on purpose — see BuildSanList) aborted co-tenant DNS domains on the same + // order; a StageValidation failure on domain N+1 orphaned domain N's TXT record; and + // a misconfiguration-detection throw fired on an ordinary non-DNS Subject CN, which + // no setting could prevent since SubmitNonDnsSans only filters the SAN list, not the + // subject. There is no longer a "this must still throw" case in this loop at all. + foreach (var (domain, _) in pendingDomains) + { + GetDcvResponse dcvResp; + try + { + dcvResp = await _client.GetDcvAsync(orderNumber, domain, Constants.Dcv.MethodDnsTxt, ct); + } + catch (Exception ex) when (IsDcvNotYetReady(ex)) + { + // CERTInext occasionally exposes the DCV slot in TrackOrder (so + // domainVerification is populated and dcvStatus="0") before the GetDcv + // endpoint will accept calls for that order — observed as EMS-956 + // "Invalid Request for this API" for several hours after enrollment. This is + // an order-readiness condition, not a per-domain one, so unlike every other + // case in this loop it defers the whole pass rather than skipping one domain. + _logger.LogInformation( + "GetDcv not yet accepting calls for order {OrderNumber} domain {Domain} ({Error}). " + + "Deferring DCV to the next sync cycle.", + orderNumber, LogSanitizer.Strip(domain), ex.Message); + deferToNextSyncCycle = true; + break; + } + catch (OperationCanceledException) + { + // The shared, DcvTimeoutMinutes-bound cancellation firing mid-loop. This is + // NOT a per-domain CA/DNS-provider failure — it must not be caught by the + // generic clause below, which would mislabel it as "GetDcv failed" for + // whichever domain happened to be in flight and send an operator chasing the + // wrong cause. Propagate to the outer catch, which logs and cleans up. + throw; + } + catch (Exception ex) + { + // Any other GetDcv failure — genuinely unmeasured against the live API for a + // non-DNS order-domain, which is exactly why this must not be allowed to fail + // the whole order on a guess. Skip just this domain. + _logger.LogError(ex, + "GetDcv failed for order {OrderNumber} domain {Domain}; skipping this domain so the " + + "rest of the order can still be validated.", orderNumber, LogSanitizer.Strip(domain)); + skippedDomains.Add((domain, "GetDcv failed")); + continue; + } - string template = string.IsNullOrWhiteSpace(_config.DcvTxtRecordTemplate) - ? Constants.Dcv.DefaultTxtRecordTemplate - : _config.DcvTxtRecordTemplate; - string hostname = string.Format(template, domain); + string token = dcvResp.DcvDetails?.Token; + if (string.IsNullOrWhiteSpace(token)) + { + _logger.LogError( + "GetDcv returned no token for order {OrderNumber} domain {Domain}; skipping this " + + "domain so the rest of the order can still be validated.", + orderNumber, LogSanitizer.Strip(domain)); + skippedDomains.Add((domain, "no DCV token returned")); + continue; + } - var validator = DomainValidatorFactory.ResolveDomainValidator(domain, "dns-01"); - if (validator == null) - throw new InvalidOperationException( - $"No DNS provider plugin is configured for domain '{domain}'. " + - "Ensure the appropriate DNS provider plugin is deployed and configured on the gateway."); + string template = string.IsNullOrWhiteSpace(_config.DcvTxtRecordTemplate) + ? Constants.Dcv.DefaultTxtRecordTemplate + : _config.DcvTxtRecordTemplate; + string hostname = string.Format(template, domain); - _logger.LogInformation( - "Staging DNS TXT record for DCV. OrderNumber={OrderNumber}, Domain={Domain}, Hostname={Hostname}", - orderNumber, domain, hostname); + var validator = DomainValidatorFactory.ResolveDomainValidator(domain, "dns-01"); + if (validator == null) + { + // The canonical case: an IP-literal SAN (or a non-DNS Subject CN) satisfies + // the FQDN regex above but no DNS zone can ever match it. + _logger.LogError( + "No DNS provider plugin resolved for domain '{Domain}' on order {OrderNumber}; " + + "skipping this domain so the rest of the order can still be validated. If this is " + + "a real domain, ensure the appropriate DNS provider plugin is deployed and " + + "configured on the gateway; if it came from a non-DNS SAN (e.g. an IP address) or a " + + "non-DNS Subject CN, remove it from the request.", + LogSanitizer.Strip(domain), orderNumber); + skippedDomains.Add((domain, "no DNS provider resolved")); + continue; + } - var stageResult = await validator.StageValidation(hostname, token, ct); - if (!stageResult.Success) - throw new InvalidOperationException( - $"Failed to stage DNS validation for '{domain}': {stageResult.ErrorMessage}"); + _logger.LogInformation( + "Staging DNS TXT record for DCV. OrderNumber={OrderNumber}, Domain={Domain}, Hostname={Hostname}", + orderNumber, LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); + + DomainValidationResult stageResult; + try + { + stageResult = await validator.StageValidation(hostname, token, ct); + } + catch (OperationCanceledException) + { + // Same reasoning as the GetDcv cancellation catch above: not a per-domain + // failure, must reach the outer catch rather than the generic clause below. + throw; + } + catch (Exception ex) + { + _logger.LogError(ex, + "DNS provider plugin threw while staging '{Domain}' for order {OrderNumber}; " + + "skipping this domain so the rest of the order can still be validated.", + LogSanitizer.Strip(domain), orderNumber); + skippedDomains.Add((domain, "DNS provider plugin threw")); + continue; + } + + if (!stageResult.Success) + { + _logger.LogError( + "Failed to stage DNS validation for '{Domain}' on order {OrderNumber}: {Error}. " + + "Skipping this domain so the rest of the order can still be validated.", + LogSanitizer.Strip(domain), orderNumber, LogSanitizer.Strip(stageResult.ErrorMessage)); + skippedDomains.Add((domain, $"stage failed: {stageResult.ErrorMessage}")); + continue; + } + + stagedValidations.Add((domain, hostname, validator)); + } + } + catch (Exception ex) + { + // Nothing in the loop above throws for a per-domain reason any more — this is the + // safety net for a genuinely unexpected failure: cancellation (the shared + // DcvTimeoutMinutes-bound token expiring mid-loop — explicitly re-thrown past the + // per-domain catches above rather than mislabeled as a per-domain failure) or a bug. + // Log before rethrowing: neither caller (EnrollNewAsync's try/finally, or Enroll + // itself) adds a catch, so without a log line here an unanticipated failure on the + // synchronous Enroll-time DCV path would leave no plugin-emitted record at all + // identifying the order or cause — only whatever the gateway host's own unhandled- + // exception logging happens to capture. + _logger.LogError(ex, + "Unexpected failure during DCV staging for order {OrderNumber}; cleaning up any " + + "already-staged TXT records before this propagates.", orderNumber); + await CleanupPartialStagingAsync(); + throw; + } - stagedValidations.Add((domain, hostname, validator)); + if (deferToNextSyncCycle) + { + await CleanupPartialStagingAsync(); + return false; + } + + if (skippedDomains.Count > 0) + { + _logger.LogError( + "{Count} domain(s) on order {OrderNumber} could not be staged for DCV and were skipped: " + + "[{Domains}]. This order cannot be issued by CERTInext until they are resolved.", + skippedDomains.Count, orderNumber, + LogSanitizer.Strip(string.Join(", ", skippedDomains.Select(d => $"{d.domain} ({d.reason})")))); } if (stagedValidations.Count == 0) @@ -1708,7 +1943,8 @@ private async Task PerformDcvIfNeededAsync( foreach (var (domain, hostname, _) in stagedValidations) { _logger.LogInformation( - "Triggering CERTInext DCV verification. OrderNumber={OrderNumber}, Domain={Domain}", orderNumber, domain); + "Triggering CERTInext DCV verification. OrderNumber={OrderNumber}, Domain={Domain}", + orderNumber, LogSanitizer.Strip(domain)); await _client.VerifyDcvAsync(orderNumber, domain, Constants.Dcv.MethodDnsTxt, ct); } @@ -1719,21 +1955,12 @@ private async Task PerformDcvIfNeededAsync( } finally { - // Always clean up staged DNS records — even on failure - foreach (var (domain, hostname, validator) in stagedValidations) - { - try - { - await validator.CleanupValidation(hostname, ct); - _logger.LogInformation( - "DNS TXT record cleaned up. Domain={Domain}, Hostname={Hostname}", domain, hostname); - } - catch (Exception ex) - { - _logger.LogWarning(ex, - "Failed to clean up DNS TXT record. Domain={Domain}, Hostname={Hostname}", domain, hostname); - } - } + // Always clean up staged DNS records — even on failure. Concurrent, not sequential + // — see CleanupPartialStagingAsync's comment above for why: sequential cleanup made + // the aggregate wall-clock time for this block scale with the number of staged SAN + // domains, unbounded relative to DcvTimeoutMinutes, on this ordinary success path too. + await Task.WhenAll(stagedValidations.Select(entry => + CleanupOneStagedValidationAsync(entry, ""))); } return true; @@ -1860,7 +2087,8 @@ private async Task WaitForDcvVerificationAsync(string orderNumber, IReadOnlyList "DCV verification poll exceeded its internal deadline ({Minutes}min). " + "OrderNumber={OrderNumber}, StillPendingDomains=[{Pending}]. " + "Exiting and leaving TXT records for the caller's finally block to clean up.", - _config.GetEffectiveDcvTimeoutMinutes(), orderNumber, string.Join(",", pending)); + _config.GetEffectiveDcvTimeoutMinutes(), orderNumber, + LogSanitizer.Strip(string.Join(",", pending))); return; } @@ -1893,12 +2121,14 @@ private async Task WaitForDcvVerificationAsync(string orderNumber, IReadOnlyList if (string.Equals(detail.DcvStatus, Constants.Dcv.StatusValidated, StringComparison.Ordinal)) { - _logger.LogInformation("DCV verified by CERTInext. OrderNumber={OrderNumber}, Domain={Domain}", orderNumber, domain); + _logger.LogInformation("DCV verified by CERTInext. OrderNumber={OrderNumber}, Domain={Domain}", + orderNumber, LogSanitizer.Strip(domain)); pending.Remove(domain); } else if (string.Equals(detail.DcvStatus, Constants.Dcv.StatusRejected, StringComparison.Ordinal)) { - _logger.LogWarning("DCV rejected by CERTInext. OrderNumber={OrderNumber}, Domain={Domain}", orderNumber, domain); + _logger.LogWarning("DCV rejected by CERTInext. OrderNumber={OrderNumber}, Domain={Domain}", + orderNumber, LogSanitizer.Strip(domain)); pending.Remove(domain); } } @@ -2103,6 +2333,16 @@ private EnrollmentResult BuildEnrollmentResult(EnrollCertificateResponse resp, b throw new Exception("CERTInext returned a null enrollment response."); int status = StatusMapper.ToRequestDisposition(resp.Status); + + // CertiNext's "auto-approved"/"downloadable" statuses can arrive before the + // certificate bytes are actually generated — GetCertificate right after order + // placement then fails, leaving resp.Certificate null while resp.Status still + // says issued. Never hand Command a GENERATED result with no PEM (it crashes + // CertificateConverterFactory.FromPEM downstream); demote to pending instead, + // matching the same invariant PickUpEnrolledCertificateAsync already enforces. + if (status == (int)EndEntityStatus.GENERATED && string.IsNullOrWhiteSpace(resp.Certificate)) + status = (int)EndEntityStatus.EXTERNALVALIDATION; + string message; switch (status) @@ -2183,46 +2423,358 @@ private static int MapRevocationReasonStringToCode(string reason) } /// - /// Converts the multi-valued SAN dictionary from the AnyCA gateway into the - /// list expected by the CERTInext API. + /// Builds the list submitted to CERTInext: the multi-valued SAN + /// dictionary the AnyCA gateway hands us, falling back to the subjectAltName extension + /// carried inside the CSR itself only when the gateway supplies nothing at all. + /// + /// Fallback, not union, deliberately: Command's SAN dictionary is the channel through + /// which an enrollment pattern's SAN policy is expressed for this request, and a signed + /// CSR — typically generated by the subscriber's own tooling, not by Command — can + /// legitimately carry more names than that policy allows. Unioning them in would + /// re-introduce a name the policy excluded. The CSR is only consulted when the dictionary + /// argument is null — not merely empty or all-empty-arrays. A non-null dictionary, + /// even one that computes to zero names, means Command's enrollment pattern ran and + /// deliberately produced no SANs for this request; only its literal absence means no + /// policy-derived set exists to defer to. + /// + /// The CSR still matters even though CERTInext ignores its subjectAltName extension + /// outright — measured on the US sandbox in SanSubmissionProbeTests: a CSR + /// carrying two DNS names, submitted with additionalDomains omitted, produced an + /// order with only the CN registered. Production behaves the same way: the customer + /// report that prompted this fix was a production UCC order whose CSR carried the SANs + /// and whose issued certificate held only the CN. So on whichever path populates the + /// gateway dictionary — or, in the fallback case, the CSR — this method is the only way + /// those names reach additionalDomains and therefore the certificate. + /// + /// History (UCC SANs silently dropped): the gateway keys this dictionary + /// dnsname, not dns. did not recognize + /// dnsname, so every DNS SAN was typed "dnsname", filtered out by the + /// DNS-only test in BuildAdditionalDomains, and the order went to CERTInext + /// with no additionalDomains at all. The certificate came back holding only + /// the CN, which reads as the CA stripping SANs supplied on the CSR. /// - private static List BuildSanList(Dictionary san) + private List BuildSanList(Dictionary san, string csr, string subject) { - if (san == null || san.Count == 0) - return null; - var result = new List(); + // Type+value identity, so the same name requested as two different SAN types is + // preserved while an exact repeat across the two sources collapses. + var seen = new HashSet(StringComparer.OrdinalIgnoreCase); + // "type|value" keys of entries that came from the CSR fallback, not the gateway + // dictionary — used only to word the provenance log accurately once the final, + // possibly-filtered result is known (see below). + var fromCsrKeys = new HashSet(StringComparer.OrdinalIgnoreCase); + + void Add(string type, string value, bool fromCsr = false) + { + if (string.IsNullOrWhiteSpace(value)) return; + string trimmed = value.Trim(); + string key = $"{type}|{trimmed}"; + if (!seen.Add(key)) return; + result.Add(new SanEntry { Type = type, Value = trimmed }); + if (fromCsr) fromCsrKeys.Add(key); + } - // AnyCA passes SANs keyed by type name (e.g. "Dns", "Ip", "Email", "Uri") - foreach (var kvp in san) - { - string sanType = MapSanType(kvp.Key); - if (kvp.Value == null) continue; + string FormatSans(IEnumerable sans) => + LogSanitizer.Strip(string.Join("; ", sans.Select(s => $"{s.Type}:{s.Value}"))); - foreach (string value in kvp.Value) + // AnyCA passes SANs keyed by type name — the real gateway uses "dnsname", + // "rfc822name", "ipaddress"; MapSanType normalizes the spelling variants. + if (san != null) + { + foreach (var kvp in san) { - if (!string.IsNullOrWhiteSpace(value)) - result.Add(new SanEntry { Type = sanType, Value = value.Trim() }); + string sanType = MapSanType(kvp.Key); + if (kvp.Value == null) continue; + + foreach (string value in kvp.Value) + Add(sanType, value); } } - return result.Count > 0 ? result : null; + // CSR fallback — only when the gateway dictionary is itself absent (san == null), NOT + // merely "computed to zero SAN entries" (i.e. result.Count == 0 at this point). Those + // are different things: + // a non-null dictionary — even an empty one, or one whose keys all map to empty arrays + // — means Command's enrollment pattern ran and deliberately produced no SANs for this + // request, which the CSR fallback must respect rather than override. san == null means + // Command never populated SAN data for this enrollment path at all, which is the one + // case this fallback exists for. Checking "computed to zero" instead of "san is null" + // would let an enrollment pattern that explicitly computes zero SANs still have + // CSR-derived names spliced back in — reopening the policy-reintroduction risk the + // fallback-over-union redesign exists to close. + var skippedCsrTags = new List(); + if (san == null) + { + var csrSans = ExtractSanEntriesFromCsr(csr, out skippedCsrTags); + foreach (var csrSan in csrSans) + Add(csrSan.Type, csrSan.Value, fromCsr: true); + } + + if (skippedCsrTags.Count > 0) + { + // GeneralName types with no domain-name rendering (otherName — e.g. a UPN from a + // Windows-generated CSR — directoryName, x400Address, ediPartyName, registeredID). + // They cannot be expressed in additionalDomains, so they are not forwarded. Warn + // rather than drop silently: the operator needs to know the CSR asked for something + // the certificate will not carry. + _logger.LogWarning( + "{Count} SAN(s) in the CSR use a type that cannot be represented as a domain name " + + "and were not submitted (ASN.1 GeneralName tag(s): {Tags}). CERTInext's " + + "additionalDomains field carries domain names only, so these cannot appear on the " + + "issued certificate. Remove them from the CSR if they are required. Subject={Subject}", + skippedCsrTags.Count, string.Join(", ", skippedCsrTags), LogSanitizer.Strip(subject)); + } + + if (result.Count == 0) + { + _logger.LogDebug( + "No SANs supplied by the gateway and none found in the CSR — submitting the order " + + "with domainName only. Subject={Subject}", LogSanitizer.Strip(subject)); + return null; + } + + // CERTInext's certificateInformation.additionalDomains is a domain-name field, and + // non-DNS SANs are submitted into it deliberately rather than discarded: dropping + // them would issue a certificate silently missing names the subscriber asked for, + // which is the worse failure. + // + // Measured on the US SANDBOX only (SanSubmissionProbeTests, product 844, + // 2026-08-12): CERTInext did NOT reject these at order placement. It accepted the + // order and registered the value verbatim as an order domain — an email address, an + // IP literal and a URI all came back as domainVerification keys. The order then + // cannot pass domain validation, so it parks pending instead of failing fast. + // + // Production is UNVERIFIED for this case and may reject the order outright instead. + // The warning below therefore describes the sandbox outcome as the expected one + // without promising it: either way the operator is told which SANs are the problem, + // which is the part that matters for diagnosis. + // + // This filtering runs BEFORE any of the logging below, and all of that logging is + // computed from `result` as it stands afterward — not from the pre-filter set. A + // prior version of this method logged "resolved" and "added to the order" against the + // pre-filter set and only THEN applied this filter, so with SubmitNonDnsSans=false the + // audit trail could claim a SAN was added when it had in fact just been dropped two + // lines later — a self-contradicting record for the same enrollment. The fix is + // ordering, not new logic: decide what is actually being submitted first, describe + // that. + var nonDns = result.Where(s => !string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)).ToList(); + + if (nonDns.Count > 0 && !_config.SubmitNonDnsSans) + { + _logger.LogWarning( + "{Count} requested SAN(s) are not DNS names and are being DROPPED because " + + "SubmitNonDnsSans is false: {Sans}. The order will issue, but the certificate will " + + "NOT contain these names. Set SubmitNonDnsSans back to true to submit them and have " + + "CERTInext surface the problem instead. Subject={Subject}", + nonDns.Count, FormatSans(nonDns), LogSanitizer.Strip(subject)); + + result = result + .Where(s => string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)) + .ToList(); + nonDns = new List(); + + if (result.Count == 0) + return null; + } + + // The blind spot that hid the original defect was that nothing logged what we + // resolved. Log the final, post-filter resolved set and its provenance at Information. + int fromCsrKept = result.Count(s => fromCsrKeys.Contains($"{s.Type}|{s.Value}")); + // Post-filter, not the pre-filter `fromGateway` snapshot: gateway- and CSR-sourced + // entries are mutually exclusive by construction (the CSR fallback only ever runs when + // the gateway supplied nothing at all), so whatever's left in `result` and isn't + // fromCsrKept must be gateway-sourced. Using the pre-filter count here reproduced the + // exact self-contradicting-audit-trail bug this method was already restructured once to + // fix — with SubmitNonDnsSans=false this line could read e.g. "Resolved 1 SAN(s) ... + // FromGatewayRequest=3", an arithmetic impossibility for anyone reconciling counts. + int fromGatewayKept = result.Count - fromCsrKept; + _logger.LogInformation( + "Resolved {Total} SAN(s) for submission. FromGatewayRequest={FromGateway}, " + + "AddedFromCsrFallback={FromCsr}, Sans={Sans}, Subject={Subject}", + result.Count, fromGatewayKept, fromCsrKept, FormatSans(result), + LogSanitizer.Strip(subject)); + + if (fromCsrKept > 0) + { + // Worth a Warning, not Debug: it means Command handed us no SAN data at all for + // this enrollment, which is a gateway/template wiring smell even though the CSR + // fallback recovers it here. + _logger.LogWarning( + "Command supplied no SAN data for this enrollment; {Count} SAN(s) present in the CSR " + + "have been added to the order instead. Review the enrollment pattern / template SAN " + + "configuration. Subject={Subject}", + fromCsrKept, LogSanitizer.Strip(subject)); + } + + if (nonDns.Count > 0) + { + // Reaching this line means SubmitNonDnsSans is true (the false case already + // returned above), so these are being submitted, not dropped. + _logger.LogWarning( + "{Count} requested SAN(s) are not DNS names: {Sans}. CERTInext's additionalDomains " + + "field takes domain names, so this order will either be rejected outright or be " + + "created and then fail domain validation and sit pending — on the US sandbox it was " + + "accepted verbatim and parked pending. They are submitted rather than dropped on " + + "purpose: a visible failure is preferable to a certificate issued without names the " + + "subscriber requested. Remove them from the CSR or the enrollment pattern if the " + + "order should proceed. Subject={Subject}", + nonDns.Count, FormatSans(nonDns), LogSanitizer.Strip(subject)); + } + + return result; } private static string MapSanType(string anyCAType) { switch (anyCAType?.ToLowerInvariant()) { - case "dns": return "dns"; + // "dnsname" is what the AnyCA REST Gateway actually sends; "dns"/"dnsnames" + // are kept for callers and older hosts that use the shorter spelling. + case "dns": + case "dnsname": + case "dnsnames": return "dns"; case "ip": - case "ipaddress": return "ip"; + case "ipaddress": + case "ipaddresses": return "ip"; case "email": - case "rfc822": return "email"; - case "uri": return "uri"; + case "rfc822": + case "rfc822name": return "email"; + case "uri": + case "uniformresourceidentifier": return "uri"; default: return anyCAType?.ToLowerInvariant() ?? "dns"; } } + /// + /// Extracts the subjectAltName entries from a PEM-encoded PKCS#10 CSR. + /// + /// Implemented with BouncyCastle (per the project's crypto policy: all certificate + /// and key handling goes through BouncyCastle, never BCL System.Security.Cryptography). + /// Never throws — an absent, truncated, or otherwise unparseable CSR returns an empty + /// list so enrollment continues on the gateway-supplied SAN data alone. + /// + /// PEM-encoded PKCS#10 request, or null/garbage. + /// + /// ASN.1 GeneralName tag numbers present in the CSR that have no domain-name rendering and + /// were therefore not returned (otherName, directoryName, x400Address, ediPartyName, + /// registeredID, and any malformed IPAddress). Reported so the caller can warn instead of + /// dropping them silently. + /// + private static List ExtractSanEntriesFromCsr(string csrPem, out List skippedTagNumbers) + { + var result = new List(); + skippedTagNumbers = new List(); + if (string.IsNullOrWhiteSpace(csrPem)) + return result; + + try + { + string b64 = csrPem + .Replace("-----BEGIN CERTIFICATE REQUEST-----", string.Empty) + .Replace("-----END CERTIFICATE REQUEST-----", string.Empty) + .Replace("-----BEGIN NEW CERTIFICATE REQUEST-----", string.Empty) + .Replace("-----END NEW CERTIFICATE REQUEST-----", string.Empty) + .Replace("\r", string.Empty) + .Replace("\n", string.Empty) + .Trim(); + + if (string.IsNullOrWhiteSpace(b64)) + return result; + + var csr = new Org.BouncyCastle.Pkcs.Pkcs10CertificationRequest(Convert.FromBase64String(b64)); + + // SANs live in the PKCS#9 extensionRequest attribute, not the CSR body. + var extensions = csr.GetRequestedExtensions(); + var sanExtension = extensions?.GetExtension( + Org.BouncyCastle.Asn1.X509.X509Extensions.SubjectAlternativeName); + if (sanExtension == null) + return result; + + var names = Org.BouncyCastle.Asn1.X509.GeneralNames.GetInstance(sanExtension.GetParsedValue()); + foreach (var generalName in names.GetNames()) + { + var entry = GeneralNameToSanEntry(generalName); + if (entry != null) + result.Add(entry); + else + skippedTagNumbers.Add(generalName.TagNo); + } + } + catch (Exception ex) + { + // Enrollment must not fail because we could not read the CSR's SANs — the + // gateway-supplied set still applies, and CERTInext validates the CSR itself. + // Debug so an operator diagnosing a missing SAN can see the parse was skipped. + LogHandler.GetClassLogger(typeof(CERTInextCAPlugin)) + .LogDebug(ex, "ExtractSanEntriesFromCsr suppressed CSR parse failure"); + } + + return result; + } + + /// + /// Maps a GeneralName to the this plugin would submit for it, or null + /// for a name whose value cannot be rendered meaningfully — skipped rather than submitted as + /// ASN.1 debris. One switch, not two: a separate tag→type mapping alongside this one used to + /// assign a type string ("directoryname", "registeredid", ...) to tags that always return a + /// null value here anyway, so those branches were dead — the type never reached a caller + /// with no value to pair it with. + /// + private static SanEntry GeneralNameToSanEntry(Org.BouncyCastle.Asn1.X509.GeneralName generalName) + { + string type; + string value; + + switch (generalName.TagNo) + { + case Org.BouncyCastle.Asn1.X509.GeneralName.DnsName: + type = "dns"; + value = Org.BouncyCastle.Asn1.DerIA5String.GetInstance(generalName.Name).GetString(); + break; + + case Org.BouncyCastle.Asn1.X509.GeneralName.Rfc822Name: + type = "email"; + value = Org.BouncyCastle.Asn1.DerIA5String.GetInstance(generalName.Name).GetString(); + break; + + case Org.BouncyCastle.Asn1.X509.GeneralName.UniformResourceIdentifier: + type = "uri"; + value = Org.BouncyCastle.Asn1.DerIA5String.GetInstance(generalName.Name).GetString(); + break; + + case Org.BouncyCastle.Asn1.X509.GeneralName.IPAddress: + type = "ip"; + // Octet string → dotted-quad / RFC 5952 text, so what we submit and log is + // the address the subscriber asked for rather than its hex encoding. + byte[] octets = Org.BouncyCastle.Asn1.Asn1OctetString.GetInstance(generalName.Name).GetOctets(); + value = octets.Length == 4 || octets.Length == 16 + ? new System.Net.IPAddress(octets).ToString() + : null; + break; + + default: + // otherName, directoryName, x400Address, ediPartyName, registeredID. + // + // Deliberately null, not Name.ToString(). BouncyCastle renders these as an + // ASN.1 dump — a UPN otherName from a Windows-generated CSR stringifies to + // "[1.3.6.1.4.1.311.20.2.3, [CONTEXT 0]svc@corp.example.com]" and a + // directoryName to "CN=host.example.com,O=Acme". Submitting that as an entry in + // additionalDomains is not "forwarding the name the subscriber asked for" — it + // is putting ASN.1 debris in a domain-name field, which cannot become a + // certificate SAN under any circumstances and only breaks the order. That is + // different from a well-formed non-DNS SAN (IP/email/URI), which we do submit + // on purpose so nothing the subscriber requested is dropped silently. + // + // Skipped is not silent: BuildSanList warns with the tag numbers so the + // operator can see a SAN was present and not forwarded. + type = null; + value = null; + break; + } + + return string.IsNullOrWhiteSpace(value) ? null : new SanEntry { Type = type, Value = value }; + } + private static string GetStringValue( Dictionary dict, string key, string defaultValue = "") { diff --git a/CERTInext/CERTInextCAPluginConfig.cs b/CERTInext/CERTInextCAPluginConfig.cs index e77ac68..980a26a 100644 --- a/CERTInext/CERTInextCAPluginConfig.cs +++ b/CERTInext/CERTInextCAPluginConfig.cs @@ -256,6 +256,19 @@ public static Dictionary GetCAConnectorAnnotations() DefaultValue = false, Type = "Boolean" }, + [Constants.Config.SubmitNonDnsSans] = new PropertyConfigInfo + { + Comments = "If true (default), SANs that are not DNS names (IP address, email, URI) are " + + "submitted to CERTInext in additionalDomains along with the DNS names. CERTInext " + + "registers them verbatim as order domains and they cannot pass domain validation, " + + "so such an order will not issue until they are removed — but nothing the " + + "subscriber requested is dropped silently. Set to false to submit DNS names only, " + + "which restores the pre-1.0.1 behaviour: the order issues, but the certificate " + + "will not contain the non-DNS names. Default: true.", + Hidden = false, + DefaultValue = true, + Type = "Boolean" + }, [Constants.Config.PageSize] = new PropertyConfigInfo { Comments = "Number of orders to fetch per page during synchronization. " + @@ -691,6 +704,23 @@ public class CERTInextConfig [JsonPropertyName("IgnoreExpired")] public bool IgnoreExpired { get; set; } = false; + /// + /// Whether non-DNS SANs (IP address, email, URI) are submitted to CERTInext. + /// + /// Defaults to true: nothing the subscriber requested is dropped silently. CERTInext + /// registers such values verbatim as order domains, and they cannot pass domain validation, + /// so the order will not issue until they are removed — a visible failure, deliberately + /// preferred over a certificate quietly missing requested names. + /// + /// Set to false to submit DNS names only, restoring the pre-1.0.1 behaviour where the + /// order issues but the non-DNS names are absent from the certificate. This exists as an + /// upgrade escape hatch: on a host that was issuing certificates for requests carrying an IP + /// or email SAN, the default flips those enrollments from "issues (incomplete)" to "parks + /// pending", and an operator needs a way back that does not involve downgrading the plugin. + /// + [JsonPropertyName("SubmitNonDnsSans")] + public bool SubmitNonDnsSans { get; set; } = true; + [JsonPropertyName("PageSize")] public int PageSize { get; set; } = Constants.Api.DefaultPageSize; diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index c6ad56b..9ecde2b 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -186,9 +186,20 @@ public async Task PlaceOrderAsync( if (request.Meta == null) request.Meta = await BuildMetaAsync(ct); + // The domain set is logged here, at the wire, not just where Command hands it to us. + // A UCC order that silently lost its SANs upstream of this point is otherwise + // indistinguishable in the gateway log from one the CA stripped — reconciling the + // enrollment-start "SANs=" line against this one localizes the loss immediately. + var certInfo = request.OrderDetails?.CertificateInformation; Logger.LogInformation( - "Submitting order to CERTInext. ProductCode={ProductCode}", - request.OrderDetails?.ProductCode); + "Submitting order to CERTInext. ProductCode={ProductCode}, DomainName={DomainName}, " + + "AdditionalDomainCount={AdditionalDomainCount}, AdditionalDomains={AdditionalDomains}", + request.OrderDetails?.ProductCode, + LogSanitizer.Strip(certInfo?.DomainName), + certInfo?.AdditionalDomains?.Count ?? 0, + certInfo?.AdditionalDomains != null && certInfo.AdditionalDomains.Count > 0 + ? LogSanitizer.Strip(string.Join("; ", certInfo.AdditionalDomains)) + : "(none)"); GenerateOrderResponse result = null; RestResponse resp = null; @@ -248,7 +259,8 @@ public async Task PlaceOrderAsync( "PlaceOrder received no usable response (DomainName={Domain}, HttpStatus={Status}, LatencyMs={Latency}). " + "Not retrying to avoid a duplicate order (EMS-947). If CERTInext created the order it " + "will be imported by the next synchronization.", - request.OrderDetails?.CertificateInformation?.DomainName, (int)resp.StatusCode, sw.ElapsedMilliseconds); + LogSanitizer.Strip(request.OrderDetails?.CertificateInformation?.DomainName), + (int)resp.StatusCode, sw.ElapsedMilliseconds); throw new Exception( "CERTInext did not return a usable response to the order submission. If the order was " + "created it will be imported by the next synchronization — do not resubmit immediately. " + @@ -298,7 +310,9 @@ public async Task PlaceOrderAsync( "PlaceOrder classified {ErrorCode} as a duplicate transaction (not a hard failure). " + "DomainName={Domain}, Path={Path}, HttpStatus={Status}, LatencyMs={Latency}. If an order exists " + "for this transaction it will be imported by the next synchronization.", - result.Meta.ErrorCode, request.OrderDetails?.CertificateInformation?.DomainName, Constants.Api.GenerateOrderSslPath, (int)resp.StatusCode, sw.ElapsedMilliseconds); + result.Meta.ErrorCode, + LogSanitizer.Strip(request.OrderDetails?.CertificateInformation?.DomainName), + Constants.Api.GenerateOrderSslPath, (int)resp.StatusCode, sw.ElapsedMilliseconds); throw new Exception( "CERTInext reported a duplicate order transaction (EMS-947). If an order was created " + "for this transaction it will be imported by the next synchronization — do not resubmit " + @@ -775,6 +789,27 @@ public async Task RenewCertificateAsync( throw new KeyNotFoundException($"Cannot renew: prior order '{certificateId}' was not found in CERTInext."); } + // Primary domain for the renewal order. Prefer the CN of the subject Command gave + // us; the prior order's requestorName is only a last resort and is not a domain — + // it is retained solely so an old caller that sets no Subject behaves as before. + // Hoisted: the same parse drives both the domain and the "did we get a CN?" warning, + // mirroring BuildOrderRequestFromLegacyEnrollRequest. + string subjectCn = ExtractCnFromSubject(request.Subject); + + string renewalDomainName = + subjectCn + ?? priorTrack.OrderDetails?.RequestorInformation?.RequestorName + ?? "unknown"; + + if (subjectCn == null) + { + Logger.LogWarning( + "Renewal of order {PriorId} has no usable CN in its subject; falling back to " + + "DomainName='{DomainName}' from the prior order. Verify the renewed certificate's " + + "primary domain.", + certificateId, LogSanitizer.Strip(renewalDomainName)); + } + // We don't have the product code from TrackOrder — build an order using // the config defaults and the CSR from the renewal request. var orderReq = new GenerateOrderSslRequest @@ -794,7 +829,8 @@ public async Task RenewCertificateAsync( SubscriptionDetails = new SubscriptionDetails { Validity = "1" }, CertificateInformation = new CertificateInformation { - DomainName = priorTrack.OrderDetails?.RequestorInformation?.RequestorName ?? "unknown" + DomainName = renewalDomainName, + AdditionalDomains = BuildAdditionalDomains(request.Sans, renewalDomainName) }, Csr = request.Csr, AgreementDetails = BuildDefaultAgreementDetails() @@ -1294,15 +1330,57 @@ private async Task ExecuteWithRetryAsync( { int attempts = idempotent ? maxAttempts : 1; RestResponse resp = null; + var sw = System.Diagnostics.Stopwatch.StartNew(); for (int attempt = 1; attempt <= attempts; attempt++) { resp = await _http.ExecuteAsync(req, ct); - // Success or 4xx client error — return immediately + // Success or 4xx client error — return immediately, checked BEFORE the + // cancellation check below. `_http.ExecuteAsync` already ran to completion by the + // time control reaches this line; whether `ct` has *since* flipped to cancelled is + // a separate, unsynchronized fact (a check-after-await race, not a fabricated one — + // a CancellationTokenSource(TimeSpan) callback and this awaited Task's completion + // are not mutually exclusive events). A deadline (the shared DcvTimeoutMinutes + // budget) firing at essentially the same instant a call genuinely succeeded must not + // discard that success: for VerifyDcv specifically, discarding it here would abort + // PerformDcvIfNeededAsync's loop before WaitForDcvVerificationAsync ever ran, and + // its finally block would delete the just-staged TXT record even though CERTInext + // had genuinely received the verify trigger — turning a real CA-side success into a + // self-inflicted DCV failure. bool isClientError = (int)resp.StatusCode >= 400 && (int)resp.StatusCode < 500; if (resp.IsSuccessful || isClientError) return resp; + // Only for a call that did NOT succeed: this client is built with + // ThrowOnAnyError=false (see the constructor), so a cancelled ct does not surface as + // OperationCanceledException from ExecuteAsync — RestSharp catches + // HttpClient.SendAsync's cancellation internally and returns a non-throwing, + // unsuccessful RestResponse instead. Left unchecked, that response reaches + // DeserializeOrThrow and becomes a plain Exception indistinguishable from a genuine + // API failure — which is exactly how a caller such as PerformDcvIfNeededAsync's + // shared DCV-timeout cancellation was still landing in a generic "GetDcv failed" + // per-domain catch instead of the cancellation-specific one, even after that method + // was hardened to re-throw a real OperationCanceledException past its per-domain + // catches. Surface the true cancellation here, at the one place in the client that + // actually holds `ct`, before any retry or error-wrapping logic sees the response. + // + // Throwing here means every caller's own per-call audit line (Method/Path/HttpStatus/ + // LatencyMs, logged after ExecuteWithRetryAsync returns) never executes for the + // cancelled call — that specific attempt would otherwise vanish from the audit trail + // entirely, leaving only a coarser, order-level "unexpected failure" log with no + // domain/endpoint/status/latency. Log that record here instead, at the one place that + // reliably sees every cancellation regardless of which of ExecuteWithRetryAsync's ~10 + // callers is in flight. + if (ct.IsCancellationRequested) + { + Logger.LogWarning( + "CERTInext API call cancelled: Method={Method}, Path={Path}, HttpStatus={Status}, " + + "ResponseStatus={ResponseStatus}, LatencyMs={Latency}, Attempt={Attempt}/{Max}.", + req.Method, req.Resource, (int)resp.StatusCode, resp.ResponseStatus, + sw.ElapsedMilliseconds, attempt, attempts); + } + ct.ThrowIfCancellationRequested(); + if (attempt < attempts) { Logger.LogWarning( @@ -1398,6 +1476,10 @@ private GenerateOrderSslRequest BuildOrderRequestFromLegacyEnrollRequest(EnrollC string requestorIsd = string.IsNullOrWhiteSpace(_config.RequestorIsdCode) ? "1" : _config.RequestorIsdCode; string requestorMobile = _config.RequestorMobileNumber ?? string.Empty; + // Hoisted: additionalDomains is de-duplicated against the primary domain, so both + // fields have to be built from the same value. + string domainName = ExtractCnFromSubject(request.Subject) ?? "unknown"; + return new GenerateOrderSslRequest { // Meta will be set by PlaceOrderAsync @@ -1443,8 +1525,8 @@ private GenerateOrderSslRequest BuildOrderRequestFromLegacyEnrollRequest(EnrollC }, CertificateInformation = new CertificateInformation { - DomainName = ExtractCnFromSubject(request.Subject) ?? "unknown", - AdditionalDomains = BuildAdditionalDomains(request.Sans), + DomainName = domainName, + AdditionalDomains = BuildAdditionalDomains(request.Sans, domainName), AutoSecureWww = string.IsNullOrWhiteSpace(_config.AutoSecureWww) ? "0" : _config.AutoSecureWww }, @@ -1506,16 +1588,57 @@ private static string ExtractCnFromSubject(string subject) return null; } - private static List BuildAdditionalDomains(System.Collections.Generic.List sans) + /// + /// Projects the resolved SAN list onto certificateInformation.additionalDomains. + /// + /// Every requested SAN is submitted regardless of type. Filtering to DNS-only (the + /// original behaviour) issued certificates quietly missing names the subscriber had + /// requested, which is the worse failure; the caller warns about the non-DNS entries + /// before we get here. + /// + /// is the value already going out as the order's primary + /// domain, and Command normally includes the CN in the SAN set as well. On the US + /// sandbox CERTInext was measured to collapse that repetition itself + /// (SanSubmissionProbeTests: CN submitted twice came back registered once), but that is + /// undocumented and unverified against production — which is exactly why we exclude it + /// here rather than relying on CA-side de-duplication. It also keeps the submitted body + /// matching what we log. + /// + private List BuildAdditionalDomains( + System.Collections.Generic.List sans, + string domainName) { if (sans == null || sans.Count == 0) return null; + var domains = new List(); + var seen = new HashSet(StringComparer.OrdinalIgnoreCase); + + bool haveDomainName = !string.IsNullOrWhiteSpace(domainName); + if (haveDomainName) + seen.Add(domainName.Trim()); + + int duplicates = 0; foreach (var san in sans) { - if (string.Equals(san.Type, "dns", StringComparison.OrdinalIgnoreCase) && - !string.IsNullOrWhiteSpace(san.Value)) - domains.Add(san.Value); + if (san == null || string.IsNullOrWhiteSpace(san.Value)) continue; + + string value = san.Value.Trim(); + if (!seen.Add(value)) + { + duplicates++; + continue; + } + domains.Add(value); } + + if (duplicates > 0) + { + Logger.LogDebug( + "Collapsed {Count} duplicate SAN value(s) out of additionalDomains " + + "(already submitted as domainName '{DomainName}', or repeated in the SAN set).", + duplicates, LogSanitizer.Strip(domainName)); + } + return domains.Count > 0 ? domains : null; } diff --git a/CERTInext/Constants.cs b/CERTInext/Constants.cs index 4510286..e3dc989 100644 --- a/CERTInext/Constants.cs +++ b/CERTInext/Constants.cs @@ -20,6 +20,7 @@ public static class Config public const string AuthMode = "AuthMode"; public const string Enabled = "Enabled"; public const string IgnoreExpired = "IgnoreExpired"; + public const string SubmitNonDnsSans = "SubmitNonDnsSans"; public const string PageSize = "PageSize"; // Synchronous certificate pickup (parity with the legacy Sectigo connector). @@ -323,6 +324,17 @@ public static class Dcv // Override via the DcvTxtRecordTemplate connector config field. public const string DefaultTxtRecordTemplate = "_emsign-validation.{0}"; + // Independent bound for a single CleanupValidation (TXT-record removal) call. This is + // deliberately its own fixed ceiling, not a fraction of DcvTimeoutMinutes and not the + // ambient DCV-flow cancellation token: cleanup is a best-effort compensating action that + // must get a real chance to run even when the operation it's cleaning up after was + // itself cancelled (the ambient token would already be cancelled at that point), but it + // still must not be allowed to hang the calling gateway request forever if a DNS + // provider plugin's underlying network call stalls. 60s comfortably covers a single + // DELETE-shaped call under normal conditions (the reference CloudflareDomainValidator's + // HttpClient default alone is 100s) without risking an indefinite hang. + public const int CleanupValidationTimeoutSeconds = 60; + // Defaults for the DCV-during-sync bounds (issue 0002). public const int DefaultSyncMaxOrderAgeHours = 24; public const int DefaultSyncMaxPerPass = 50; diff --git a/CERTInext/Models/LogSanitizer.cs b/CERTInext/Models/LogSanitizer.cs new file mode 100644 index 0000000..d341f09 --- /dev/null +++ b/CERTInext/Models/LogSanitizer.cs @@ -0,0 +1,33 @@ +// Copyright 2026 Keyfactor +// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. +// At http://www.apache.org/licenses/LICENSE-2.0 + +namespace Keyfactor.Extensions.CAPlugin.CERTInext.Models +{ + /// + /// Neutralizes control characters before a requester-controlled value is interpolated into a + /// log message. + /// + /// SAN values reach the log from the CSR and from Command's SAN dictionary, i.e. from the + /// requester. Structured message templates stop format-string abuse but not embedded newlines, + /// and NLog's text layout does not escape them — so an unsanitized value can forge additional, + /// well-formed-looking records in the gateway log (CWE-117). That matters here specifically + /// because these log lines exist to make the submitted SAN set auditable; a forged line could + /// assert a different SAN set than the one actually sent. + /// + /// Shared between CERTInextCAPlugin and Client.CERTInextClient — both sanitize the + /// same kind of value at their respective log sinks, so this used to be defined twice, byte- + /// identical, one per class. + /// + internal static class LogSanitizer + { + internal static string Strip(string value) + { + if (string.IsNullOrEmpty(value)) return value; + return value + .Replace("\r", "\\r") + .Replace("\n", "\\n") + .Replace("\t", "\\t"); + } + } +} diff --git a/CHANGELOG.md b/CHANGELOG.md index d971478..11bd7b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,16 @@ # 1.0.1 ## Features -- **Faster enrollment for quickly-issued certificates.** Enrollment now waits briefly for the certificate and returns it in the same request when it issues fast (DV and already-approved orders), instead of always waiting for the next synchronization. Two new optional settings control the wait: `PickupRetries` (default 5; set to `0` to disable) and `PickupDelay` (default 10 seconds) — about a 55-second wait by default, with a built-in ceiling so it can't run long enough to time out the enrollment. Orders that don't issue in that window — including OV/EV, which CERTInext validates asynchronously over minutes to hours — return pending and are imported by a later sync, exactly as before. Works with or without DNS-based DCV. +- **Faster enrollment for quickly-issued certificates.** Enrollment now waits briefly and returns the certificate in the same request when it issues fast, instead of always waiting for the next sync. Configurable via `PickupRetries` (default 5, `0` disables) and `PickupDelay` (default 10s). Orders that don't issue in time (e.g. OV/EV) return pending and are picked up by the next sync, as before. ## Bug Fixes -- **No more duplicate or orphaned orders after a network timeout.** Order and CSR submissions are no longer retried after a network timeout. A timeout can happen *after* the CA has already accepted the request, so the automatic retry was being rejected as a duplicate — failing the enrollment and leaving an orphaned order behind. These requests now run once; if the order was created it is imported by the next synchronization, and duplicate responses are reported with clear, actionable guidance. (Read-only calls are unaffected and still retry.) +- **UCC certificates no longer come back with only the common name.** The gateway sends SANs under the key `dnsname`, which the plugin didn't recognize, so orders went out with an empty domain list. SANs are now read from every key the gateway sends, plus from the CSR itself. +- **Renewals no longer lose their SANs.** Renewals were submitted with no additional domains and the wrong primary domain; both now come from the certificate being renewed. +- **Enrollment no longer fails on an order CERTInext auto-approves before it finishes issuing.** The plugin used to report these as issued with no certificate attached, which the gateway rejected. It now returns pending and picks up the certificate once CERTInext finishes issuing it. + +## Upgrade Notes +- **Non-DNS SANs (IP, email, URI) are now submitted instead of silently dropped.** CERTInext can't validate them, so such an order won't issue until the SAN is removed. Set `SubmitNonDnsSans` to `false` to restore the old drop-silently behavior. +- **No more duplicate or orphaned orders after a network timeout.** Order/CSR submissions no longer auto-retry after a timeout, since the CA may have already created the order. If it was created, the next sync imports it. # 1.0.0 From 947ae686ebcdec5268d3083059cccbe745b05f8e Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Thu, 13 Aug 2026 10:27:12 -0700 Subject: [PATCH 07/10] docs: refresh docsource against current code docsource hadn't been touched since v1.0.0; a full freshness pass against current source found several stale/wrong claims and fixed them: - Order Lifecycle status-code table didn't match StatusMapper.cs (several codes were in the wrong bucket, several real codes weren't listed). - GroupNumber description omitted its per-order use (delegationInformation), contradicting a note three lines below it. - AutoApprove and DefaultProductCode were documented as doing things the code doesn't do; corrected to describe actual behavior (both filed as separate GitHub issues, not fixed here). - ~15 real config properties (OrganizationNumber, TechnicalContact*, SubmitNonDnsSans, PickupRetries/PickupDelay, DcvWaitFor*Seconds, DcvSyncMax*, etc.) existed in code with no mention anywhere in docs. - Enrollment/sync sequence diagrams in architecture.md were silent on DCV and synchronous certificate pickup entirely. - development.md's product test-coverage table cited a removed test file and hardcoded requestNumbers documented elsewhere as non-portable; replaced with a pointer to `make probe-products` and TESTING.md. --- docsource/architecture.md | 26 +++++++++++++++++++-- docsource/configuration.md | 46 ++++++++++++++++++++++++++++++-------- docsource/development.md | 30 +++++++------------------ 3 files changed, 69 insertions(+), 33 deletions(-) diff --git a/docsource/architecture.md b/docsource/architecture.md index 93ac459..7fc4135 100644 --- a/docsource/architecture.md +++ b/docsource/architecture.md @@ -113,6 +113,8 @@ sequenceDiagram **Expired certificates:** The `IgnoreExpired` connector setting controls whether expired certificates are included in synchronization. When enabled, expired certificates are silently skipped and will not appear in the Keyfactor Command inventory. +**DCV-during-sync:** on a DCV-enabled build, each sync pass also drives DNS-01 validation forward for pending DV orders that are still waiting on it, bounded by `DcvSyncMaxOrderAgeHours` (skip orders older than this) and `DcvSyncMaxPerPass` (cap how many are attempted per pass), so a large backlog of stalled pending orders can't slow down every sync. + --- ## Certificate Enrollment @@ -134,13 +136,27 @@ sequenceDiagram Plugin->>API: Place certificate order\n(CSR, domain, organization details,\nsubscriber agreement, requestor info) API-->>Plugin: Order accepted — order number assigned + opt DNS-01 DCV build, DCV enabled, and this order requires it + Plugin->>Plugin: Publish DNS TXT challenge\nvia the configured DNS provider plugin + Plugin->>API: Ask CERTInext to verify the record + API-->>Plugin: Domain validated (or still pending —\nfalls through to the pending path below) + end + Plugin->>API: Check order status API-->>Plugin: Order status and certificate details alt Certificate issued immediately Plugin-->>CMD: Certificate ready — PEM returned - else Certificate pending approval - Plugin-->>CMD: Pending — Command will pick it up\nduring the next synchronization + else Certificate pending or not yet downloadable + loop Certificate-pickup retries\n(bounded, ~55s by default — PickupRetries/PickupDelay) + Plugin->>API: Poll for the certificate + API-->>Plugin: Status and certificate, if ready + end + alt Certificate became available during pickup + Plugin-->>CMD: Certificate ready — PEM returned + else Still not available + Plugin-->>CMD: Pending — Command will pick it up\nduring the next synchronization + end else Order rejected by CERTInext Plugin-->>CMD: Enrollment failed — see gateway logs end @@ -148,12 +164,18 @@ sequenceDiagram Plugin->>Plugin: Record enrollment outcome in audit log\n(order number, serial number, status) ``` +**DCV:** on a DCV-enabled build, DNS-01 validation runs inline for DV orders that require it, bounded by `DcvTimeoutMinutes`. When DCV isn't enabled, isn't built into this host, or the order doesn't require it, this step is skipped entirely and the order proceeds straight to the pending/pickup path like any other asynchronously-issued order. + +**Synchronous certificate pickup:** if the certificate isn't available immediately (a fresh order, or DCV that just validated but hasn't finished generating the PEM), `Enroll()` polls CERTInext a bounded number of times (`PickupRetries` × `PickupDelay`, capped at a 180s ceiling) before giving up and returning pending. This lets a fast-issuing certificate (DV, or an already-approved order) come back in the same enrollment call instead of always waiting for the next sync. OV/EV orders validate asynchronously over minutes to hours and typically exhaust this window regardless. + ### Renewal When Command initiates a renewal, the plugin checks whether the existing certificate is within the configured renewal window. If it is, the prior order record is used as context for the new request. If it is outside the window (or the prior certificate cannot be located), the plugin falls back to issuing a new certificate. > **Note:** CERTInext does not have a dedicated certificate renewal endpoint. Both renewal and reissuance paths submit a new `GenerateOrderSSL` order. The distinction affects how Keyfactor Command tracks the certificate record, not what is sent to CERTInext. +> **Note:** If the prior-order lookup itself throws (rather than cleanly returning "not found" — e.g. a transient database error), the plugin falls back to issuing a new certificate rather than failing the enrollment. + ```mermaid flowchart TD A([Renewal requested]) --> B{Prior certificate\nserial number\nprovided?} diff --git a/docsource/configuration.md b/docsource/configuration.md index 41c872e..d80216b 100644 --- a/docsource/configuration.md +++ b/docsource/configuration.md @@ -9,6 +9,7 @@ The CERTInext AnyCA Gateway REST plugin extends the certificate lifecycle capabi * New certificate enrollment (new keys and certificate). * Certificate renewal — submits a new `GenerateOrderSSL` order when the prior certificate is within the configured renewal window (CERTInext has no dedicated renewal endpoint; the renewal-window check governs how Command tracks old→new, not which API is called). * Certificate reissuance (new keys with the same or updated subject/SANs) when outside the renewal window or no prior certificate is found. + * Synchronous certificate pickup — a fast-issuing order (DV, or already-approved) can return the certificate in the same enrollment call instead of always waiting for the next sync, via `PickupRetries`/`PickupDelay`. * Certificate Revocation: * Request revocation of a previously issued certificate using any RFC 5280 CRL reason code. * Supported authentication modes for calls to the CERTInext API: @@ -91,7 +92,9 @@ Before enrolling certificates, the Keyfactor Command server must trust the CERTI ## CA Configuration -The following fields are presented in the Keyfactor Command Management Portal when creating or editing the CERTInext CA connector. All fields marked **Required** must be provided before the connector can be saved in an enabled state. +The following fields are presented in the Keyfactor Command Management Portal when creating or editing the CERTInext CA connector. + +> Note: the connector's own save-time validation only enforces `ApiUrl`, `AccountNumber`, and the credential fields for the selected `AuthMode`. Other fields marked **Required** below are required by CERTInext for a successful order — the connector will save without them, but enrollment will fail or the order will be parked pending until they're set. | Field | Required / Optional | Description | Where to find it | Example | |---|---|---|---|---| @@ -108,15 +111,30 @@ The following fields are presented in the Keyfactor Command Management Portal wh | `RequestorMobileNumber` | Optional | Requestor mobile number (digits only, no country code). Included in the `requestorInformation` block. | N/A | `5551234567` | | `SignerPlace` | Required | City or location of the person accepting the subscriber agreement on behalf of your organization. Required by CERTInext for all orders. | Use the physical city where the signer is located. | `Austin` | | `SignerIp` | Required | Public IP address of the host accepting the subscriber agreement. Required by CERTInext for all orders. | Use the outbound IP of the AnyCA Gateway host, or the IP of the workstation from which the agreement was accepted. | `203.0.113.10` | -| `GroupNumber` | Optional | CERTInext group (delegation) number. When set, it is passed in the `productDetails.groupNumber` field of `GetProductDetails` requests. Some sandbox accounts return an empty product list from `GetProductDetails` unless this field is included. Available in the CERTInext portal under **Delegation → Groups**. | Portal → **Delegation → Groups**. | `2345678901` | -| `DefaultProductCode` | Optional | Default numeric product code to use when no product code is set on the certificate template. If omitted and the template also has no product code, enrollment will fail. Product codes are provisioned per account by eMudhra — contact your eMudhra account representative to obtain the numeric codes available to your account. | Call `GetProductDetails` against your account/environment (see product code table below). | `842` | +| `GroupNumber` | Optional | CERTInext group (delegation) number. When set, it is passed in the `productDetails.groupNumber` field of `GetProductDetails` requests *and* in `delegationInformation.groupNumber` on every SSL order. Some sandbox accounts return an empty product list from `GetProductDetails` unless this field is included. Available in the CERTInext portal under **Delegation → Groups**. | Portal → **Delegation → Groups**. | `2345678901` | +| `OrganizationNumber` | Optional, strongly recommended for OV/EV and faster DV | Numeric CERTInext organization number for a pre-vetted organization. When set, every SSL order is submitted with `organizationDetails.preVetting="1"` and this number, telling CERTInext to skip its manual organization-vetting queue. Without it, orders may sit in `Pending System RA` for extended manual review (observed: tens of hours). | Portal → **Organizations → Pre-vetted Organizations**. | `1234567` | +| `TechnicalContactName` / `TechnicalContactEmail` / `TechnicalContactIsdCode` / `TechnicalContactMobileNumber` | Optional | Populate `technicalPointOfContact` on every SSL order. Each defaults to the corresponding `Requestor*` field when blank. Some product configurations require a technical point of contact to be present; omitting it can cause CERTInext to park orders awaiting manual completion of the field. | N/A | *(defaults to Requestor fields)* | +| `AccountingModel` | Optional | CERTInext billing model sent in `orderDetails.accountingModel`. `2` = credit-based (most accounts). `1` = cash model. Default: `2`. | N/A | `2` | +| `EmailNotifications` | Optional | Whether CERTInext sends lifecycle-event emails to the requestor. `1` = enabled, `0` = silent (recommended for gateway-driven orders). Default: `0`. | N/A | `0` | +| `SubscriptionValidityYears` | Optional | Connector-level default validity in years for SSL orders (`1`, `2`, or `3`). Overridden per template by the `ValidityYears` enrollment parameter. Default: `1`. | N/A | `1` | +| `SubscriptionAutoRenew` | Optional | Whether CERTInext should auto-renew certificates issued through this connector. `0` = disabled (recommended — renewal is driven by Keyfactor Command), `1` = enabled. Default: `0`. | N/A | `0` | +| `SubscriptionRenewCriteriaDays` | Optional | Days before expiry at which CERTInext auto-renews. Only honored when `SubscriptionAutoRenew` is `1`. Default: `30`. | N/A | `30` | +| `AutoSecureWww` | Optional | If `1`, CERTInext automatically adds the `www.` variant of the primary domain as an additional SAN. Default: `0`. | N/A | `0` | +| `SubmitNonDnsSans` | Optional | If `true` (default), SANs that aren't DNS names (IP address, email, URI) are submitted to CERTInext instead of silently dropped. CERTInext can't validate them, so such an order won't issue until they're removed. Set to `false` to restore the pre-1.0.1 behavior of submitting DNS names only. Default: `true`. | N/A | `true` | +| `DefaultProductCode` | Optional, but effectively required if you use renewals | Numeric product code used for **renewals only** — CERTInext's `TrackOrder` doesn't return the prior order's product code, so the renewal path sends this value verbatim, ignoring the template's `ProductCode`/`ProfileId`. If left blank, renewals go out with an empty product code. Has **no effect on new enrollments** — the `ProductCode`/`ProfileId` template resolution never falls back to it. See [issue tracking this](https://github.com/Keyfactor/certinext-caplugin/issues/26). | Call `GetProductDetails` against your account/environment (see product code table below). | `842` | | `IgnoreExpired` | Optional | If `true`, expired certificates are skipped during synchronization and are not imported into Keyfactor Command. Default: `false`. | N/A | `false` | | `PageSize` | Optional | Number of orders to retrieve per page during synchronization. Default: `100`. Maximum: `500`. Reduce this value if synchronization requests time out. | N/A | `100` | | `Enabled` | Optional | Enables or disables the CA connector. Setting this to `false` allows the connector record to be created before all credentials are available, without triggering a live connectivity test. Default: `true`. | N/A | `true` | +| `PickupRetries` | Optional | Number of times `Enroll` polls CERTInext for the certificate after a successful order submission, before returning pending and leaving pickup to the next sync. Set to `0` to disable the wait. OV/EV orders validate asynchronously (minutes to hours) and typically exhaust this wait regardless of the value. Default: `5`. | N/A | `5` | +| `PickupDelay` | Optional | Seconds between certificate-pickup retries. `PickupRetries × PickupDelay` (plus a short initial delay) bounds how long an enrollment call occupies a Command worker thread — capped at a 180s ceiling regardless of how the two are set (aim for well under ~90s in practice, so the call doesn't run long enough to trip Command's own timeout). Default: `10` (a ~55s ceiling with default `PickupRetries`). | N/A | `10` | | `DcvEnabled` | Optional | When `true`, the gateway performs DNS-based Domain Control Validation (DCV) during enrollment for orders that require it. Requires a DNS provider plugin (e.g. `azure-azuredns-dnsplugin`) to be deployed on the gateway. Default: `false`. | N/A | `false` | | `DcvTxtRecordTemplate` | Optional | Format string for the DNS TXT record hostname published during DCV. `{0}` is replaced with the domain being validated. Default: `_emsign-validation.{0}`. | N/A | `_emsign-validation.{0}` | -| `DcvPropagationDelaySeconds` | Optional | Seconds to wait after publishing the DNS TXT record before asking CERTInext to verify it. Increase for zones with slow propagation. Default: `30`. | N/A | `30` | +| `DcvPropagationDelaySeconds` | Optional | Seconds to wait after publishing the DNS TXT record before asking CERTInext to verify it. Increase for zones with slow propagation. Applies only to the `Enroll()`-time DCV path — DCV driven during sync uses its own fixed 3-second delay. Default: `30`. | N/A | `30` | | `DcvTimeoutMinutes` | Optional | Maximum minutes to wait for the entire DCV flow (DNS publish + propagation + verify) before cancelling the enrollment. Can also be set via the `CERTINEXT_DCV_TIMEOUT_MINUTES` environment variable; the environment variable takes precedence when both are set. Default: `10`. | N/A | `10` | +| `DcvWaitForChallengeSeconds` | Optional | How long `Enroll()` waits for CERTInext to expose the DCV challenge after order placement, before giving up and deferring to the next sync. Set to `0` to disable the wait. Can also be set via `CERTINEXT_DCV_WAIT_FOR_CHALLENGE_SECONDS`. Default: `60`. | N/A | `60` | +| `DcvWaitForIssuanceSeconds` | Optional | How long `Enroll()` waits for CERTInext to finish generating the certificate after DCV verifies. Set to `0` to disable the wait. Can also be set via `CERTINEXT_DCV_WAIT_FOR_ISSUANCE_SECONDS`. Default: `60`. | N/A | `60` | +| `DcvSyncMaxOrderAgeHours` | Optional | During synchronization, only pending DV orders younger than this many hours are driven through DCV, so a large backlog of old/abandoned pending orders doesn't slow down every sync pass. Set to `0` to disable the age filter. Default: `24`. | N/A | `24` | +| `DcvSyncMaxPerPass` | Optional | Maximum number of pending DV orders driven through DCV in a single sync pass. Set to `0` to disable the cap. Default: `50`. | N/A | `50` | > Note: `AccountNumber` and group-level identifiers are distinct values. The `AccountNumber` is your top-level user account identifier. CERTInext groups (cost centers or departments) each have their own `groupNumber`, which is passed per-order and is separate from any organization number displayed on the Organizations page. @@ -130,11 +148,11 @@ In the Keyfactor Command Management Portal, navigate to **Certificate Templates* | Parameter | Required / Optional | Type | Description | Example / Default | |---|---|---|---|---| -| `ProductCode` | Optional | String | Override the numeric CERTInext product code for this template. Product codes are provisioned per account by eMudhra — obtain the correct code from `GetProductDetails` for your account. Set this explicitly when targeting the sandbox environment or when the connector `DefaultProductCode` should not apply to this template. See the [Product Codes](#product-codes) section for the sandbox/production lookup table. | DV SSL: `842` (sandbox) or `838` (production) | +| `ProductCode` | Optional | String | Override the numeric CERTInext product code for this template. Product codes are provisioned per account by eMudhra — obtain the correct code from `GetProductDetails` for your account. If omitted, the built-in default code for the selected product name is used (see [Product Codes](#product-codes)). Set this explicitly when targeting the sandbox environment or a non-standard code. | DV SSL: `842` (sandbox) or `838` (production) | | `ProfileId` | Deprecated | String | Legacy alias for `ProductCode`. Accepted for backward compatibility — if `ProductCode` is not set, `ProfileId` is used in its place. New templates should use `ProductCode`. | `838` | | `ValidityYears` | Optional | Number | Subscription validity period in years: `1`, `2`, or `3`. Default: `1`. CERTInext certificates are issued within a subscription term at up to 390 days per certificate, with free renewals within the term. | `1` | | `ValidityDays` | Deprecated | Number | Legacy validity field. If set, the value is divided by 365 and rounded up to derive a year count. New templates should use `ValidityYears`. | `365` | -| `AutoApprove` | Optional | Boolean | If `true`, the gateway will attempt automatic approval of certificates returned in a pending-approval state. Only set this if your CERTInext product is configured with automatic approval. Default: `false`. | `false` | +| `AutoApprove` | Optional | Boolean | **Currently has no effect** — reserved for future use. The plugin does not call any approval endpoint against CERTInext regardless of this setting. See [issue tracking this](https://github.com/Keyfactor/certinext-caplugin/issues/25). | `false` | | `RequesterName` | Optional | String | Per-template override for the requestor name. When set, overrides the connector-level `RequestorName` for orders using this template. | `Keyfactor Automation` | | `RequesterEmail` | Optional | String | Per-template override for the requestor email address. When set, overrides the connector-level `RequestorEmail` for orders using this template. | `pki-admin@example.com` | | `RenewalWindowDays` | Optional | Number | Number of days before certificate expiration within which a renewal is attempted instead of a reissue. Default: `90`. | `90` | @@ -226,6 +244,15 @@ authKey = SHA256(accessKey + requestTs + requestTxnId) Where `requestTs` is the ISO 8601 timestamp and `requestTxnId` is a unique transaction UUID generated per request. The raw access key is never transmitted — only the derived hash is sent. This computation happens automatically on every outbound call. When `AuthMode` is `OAuth`, the gateway obtains a bearer token via the configured client credentials flow and injects it into the `meta` block instead. +### HTTP Timeout + +Every CERTInext API call (enroll, sync, revoke) shares one HTTP client with a fixed 120-second +request timeout. This is hardcoded and is not exposed as a connector setting or environment +variable — it cannot be changed without modifying the plugin. If a call doesn't return within 120 +seconds, the plugin aborts it and the operation fails; a non-idempotent call (e.g. order placement) +is not retried afterward, since CERTInext may have already created the order — see +[Synchronization](#synchronization) to reconcile such orders on a later pass. + ### Enrollment Decision Logic When the gateway calls `Enroll`, the plugin selects between three paths based on the enrollment type and the age of the prior certificate: @@ -244,9 +271,10 @@ The `GenerateOrderSSL` API requires an `additionalInformation.remarks` field in CERTInext orders pass through several internal status stages before a certificate is issued. The plugin maps these to Keyfactor enrollment statuses as follows: -- **Issued** (status 9, 20) → certificate returned immediately. -- **Pending approval** (status 2, 8, 15, 24) → enrollment returns a pending status to Command. If `AutoApprove` is enabled on the template, the plugin attempts automatic approval before returning. -- **Rejected / cancelled** (status 4, 5, 13, 14) → enrollment fails with an error. +- **Issued** (status `7`, `9`, `12`, `15`, `20`, `23`) → certificate returned immediately (status `12`, expired, is retained in inventory as issued rather than treated as a failure). +- **Pending approval** (status `1`, `2`, `4`, `6`, `16`, `17`, `24`) → enrollment returns a pending status to Command. `Enroll()` polls briefly for the certificate (see `PickupRetries`/`PickupDelay`) before falling back to pending. +- **Revoked** (status `22`) → certificate marked revoked. +- **Rejected / cancelled** (status `3`, `5`, `8`, `13`, `14`, `18`, `19`, `21`, or any unrecognized code) → enrollment fails with an error. The gateway polls the `TrackOrder` endpoint during sync to pick up certificates that were approved after the initial enrollment call. diff --git a/docsource/development.md b/docsource/development.md index 14c6cff..2f7ab02 100644 --- a/docsource/development.md +++ b/docsource/development.md @@ -114,26 +114,12 @@ See `CERTInext.IntegrationTests/INTEGRATION_TESTING.md` for a full description o ## Product Integration Test Coverage -The table below records live draft-order results against the Production — India instance. Orders were placed with `saveAndHold:"1"` so no billing, DCV, or CA issuance was triggered. Tests are in `CERTInext.IntegrationTests/DraftOrderTests.cs`. +`DraftOrderTests.cs` (and `TrackOrderTests.cs`) previously recorded live draft-order results here, but both were removed: they asserted specific `requestNumber` values hardcoded from one developer's account, which don't exist on any other account and so failed everywhere else. Their intent — verifying draft-order and track-order semantics — is now covered by `LifecycleTests`, which creates its own order and asserts on it without relying on account-specific identifiers. -| Product | Code | Test Status | requestNumber | Notes | -|---|---|---|---|---| -| DV SSL | `838` | ✓ Tested | 4572531551 | Base domain; no extra fields required beyond base set | -| DV SSL Wildcard | `839` | ✓ Tested | 9149755266 | CSR CN must be `*.domain`; `domainName` must also use wildcard format | -| DV SSL UCC | `840` | ✓ Tested | 1611445122 | `certificateInformation.additionalDomains` array required | -| DV SSL Wildcard UCC | `841` | ✗ Blocked | — | EMS-918: "Additional Information cannot be empty" — required fields for this product not yet identified | -| OV SSL | `842` | ✓ Tested | 5546366498 | Requires `locality` and `postalCode` in `certificateInformation` | -| OV SSL Wildcard | `843` | ✗ Not tested | — | Draft order not yet placed | -| OV SSL UCC | `844` | ✗ Not tested | — | Draft order not yet placed | -| OV SSL Wildcard UCC | `845` | ✗ Blocked | — | EMS-918: "Additional Information cannot be empty" — required fields for this product not yet identified | -| EV SSL | `846` | ✓ Tested | 3932332114 | Requires `contractSignerInfo`, `certificateApproverInfo`, non-empty `streetAddress2`, `companyRegistrationNumber` | -| EV SSL UCC | `847` | ✗ Blocked | — | EMS-918: "Additional Information cannot be empty" — required fields for this product not yet identified | -| DV SSL 1 Month | N/A | ✗ Not supported | — | Visible in portal but not returned by `GetProductDetails` API; no product code available. Not supported by plugin. | -| DV SSL Wildcard 1 Month | N/A | ✗ Not supported | — | Visible in portal but not returned by `GetProductDetails` API; no product code available. Not supported by plugin. | -| emSign Intranet SSL | `100` | ✗ Not tested | — | EMS-1162: not provisioned on this account type | -| IGTF Host | `104` | ✗ Not tested | — | EMS-1162: not provisioned on this account type | -| S/MIME | `894` | ✗ Not tested | — | EMS-1162: not provisioned on this account type | -| Natural Person Doc Signer | `825` | ✗ Not tested | — | EMS-1162: not provisioned on this account type | -| Legal Entity Doc Signer | `819` | ✗ Not tested | — | EMS-1162: not provisioned on this account type | - -Products returning EMS-1162 require special provisioning by eMudhra that is not included on a standard SSL/TLS account. The plugin code supports submitting orders for any product code; whether the order is accepted depends on what is provisioned for your account. +Product codes are provisioned per account by eMudhra and are not portable across accounts (see the [Product Codes](configuration.md#product-codes) section in configuration.md). To discover which codes and required fields apply to *your* account: + +```bash +make probe-products +``` + +This places `saveAndHold=1` draft orders for all known SSL/TLS product codes and reports which return a `requestNumber` (valid/provisioned) versus an error (invalid or not provisioned). See `CERTInext.IntegrationTests/TESTING.md` for the current, account-specific findings and expected test results. From 5d2d15422b16c71d6df8d29f92a0d8db8ede90fd Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Thu, 13 Aug 2026 17:27:45 +0000 Subject: [PATCH 08/10] docs: auto-generate README and documentation [skip ci] --- README.md | 56 +++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index ff91ab1..f672ee1 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,7 @@ The CERTInext AnyCA Gateway REST plugin extends the certificate lifecycle capabi * New certificate enrollment (new keys and certificate). * Certificate renewal — submits a new `GenerateOrderSSL` order when the prior certificate is within the configured renewal window (CERTInext has no dedicated renewal endpoint; the renewal-window check governs how Command tracks old→new, not which API is called). * Certificate reissuance (new keys with the same or updated subject/SANs) when outside the renewal window or no prior certificate is found. + * Synchronous certificate pickup — a fast-issuing order (DV, or already-approved) can return the certificate in the same enrollment call instead of always waiting for the next sync, via `PickupRetries`/`PickupDelay`. * Certificate Revocation: * Request revocation of a previously issued certificate using any RFC 5280 CRL reason code. * Supported authentication modes for calls to the CERTInext API: @@ -159,11 +160,11 @@ In the Keyfactor Command Management Portal, navigate to **Certificate Templates* | Parameter | Required / Optional | Type | Description | Example / Default | |---|---|---|---|---| -| `ProductCode` | Optional | String | Override the numeric CERTInext product code for this template. Product codes are provisioned per account by eMudhra — obtain the correct code from `GetProductDetails` for your account. Set this explicitly when targeting the sandbox environment or when the connector `DefaultProductCode` should not apply to this template. See the [Product Codes](#product-codes) section for the sandbox/production lookup table. | DV SSL: `842` (sandbox) or `838` (production) | +| `ProductCode` | Optional | String | Override the numeric CERTInext product code for this template. Product codes are provisioned per account by eMudhra — obtain the correct code from `GetProductDetails` for your account. If omitted, the built-in default code for the selected product name is used (see [Product Codes](#product-codes)). Set this explicitly when targeting the sandbox environment or a non-standard code. | DV SSL: `842` (sandbox) or `838` (production) | | `ProfileId` | Deprecated | String | Legacy alias for `ProductCode`. Accepted for backward compatibility — if `ProductCode` is not set, `ProfileId` is used in its place. New templates should use `ProductCode`. | `838` | | `ValidityYears` | Optional | Number | Subscription validity period in years: `1`, `2`, or `3`. Default: `1`. CERTInext certificates are issued within a subscription term at up to 390 days per certificate, with free renewals within the term. | `1` | | `ValidityDays` | Deprecated | Number | Legacy validity field. If set, the value is divided by 365 and rounded up to derive a year count. New templates should use `ValidityYears`. | `365` | -| `AutoApprove` | Optional | Boolean | If `true`, the gateway will attempt automatic approval of certificates returned in a pending-approval state. Only set this if your CERTInext product is configured with automatic approval. Default: `false`. | `false` | +| `AutoApprove` | Optional | Boolean | **Currently has no effect** — reserved for future use. The plugin does not call any approval endpoint against CERTInext regardless of this setting. See [issue tracking this](https://github.com/Keyfactor/certinext-caplugin/issues/25). | `false` | | `RequesterName` | Optional | String | Per-template override for the requestor name. When set, overrides the connector-level `RequestorName` for orders using this template. | `Keyfactor Automation` | | `RequesterEmail` | Optional | String | Per-template override for the requestor email address. When set, overrides the connector-level `RequestorEmail` for orders using this template. | `pki-admin@example.com` | | `RenewalWindowDays` | Optional | Number | Number of days before certificate expiration within which a renewal is attempted instead of a reissue. Default: `90`. | `90` | @@ -238,7 +239,9 @@ If your CERTInext account has OAuth enabled, you can use OAuth client credential ## CA Configuration -The following fields are presented in the Keyfactor Command Management Portal when creating or editing the CERTInext CA connector. All fields marked **Required** must be provided before the connector can be saved in an enabled state. +The following fields are presented in the Keyfactor Command Management Portal when creating or editing the CERTInext CA connector. + +> Note: the connector's own save-time validation only enforces `ApiUrl`, `AccountNumber`, and the credential fields for the selected `AuthMode`. Other fields marked **Required** below are required by CERTInext for a successful order — the connector will save without them, but enrollment will fail or the order will be parked pending until they're set. | Field | Required / Optional | Description | Where to find it | Example | |---|---|---|---|---| @@ -255,15 +258,30 @@ The following fields are presented in the Keyfactor Command Management Portal wh | `RequestorMobileNumber` | Optional | Requestor mobile number (digits only, no country code). Included in the `requestorInformation` block. | N/A | `5551234567` | | `SignerPlace` | Required | City or location of the person accepting the subscriber agreement on behalf of your organization. Required by CERTInext for all orders. | Use the physical city where the signer is located. | `Austin` | | `SignerIp` | Required | Public IP address of the host accepting the subscriber agreement. Required by CERTInext for all orders. | Use the outbound IP of the AnyCA Gateway host, or the IP of the workstation from which the agreement was accepted. | `203.0.113.10` | -| `GroupNumber` | Optional | CERTInext group (delegation) number. When set, it is passed in the `productDetails.groupNumber` field of `GetProductDetails` requests. Some sandbox accounts return an empty product list from `GetProductDetails` unless this field is included. Available in the CERTInext portal under **Delegation → Groups**. | Portal → **Delegation → Groups**. | `2345678901` | -| `DefaultProductCode` | Optional | Default numeric product code to use when no product code is set on the certificate template. If omitted and the template also has no product code, enrollment will fail. Product codes are provisioned per account by eMudhra — contact your eMudhra account representative to obtain the numeric codes available to your account. | Call `GetProductDetails` against your account/environment (see product code table below). | `842` | +| `GroupNumber` | Optional | CERTInext group (delegation) number. When set, it is passed in the `productDetails.groupNumber` field of `GetProductDetails` requests *and* in `delegationInformation.groupNumber` on every SSL order. Some sandbox accounts return an empty product list from `GetProductDetails` unless this field is included. Available in the CERTInext portal under **Delegation → Groups**. | Portal → **Delegation → Groups**. | `2345678901` | +| `OrganizationNumber` | Optional, strongly recommended for OV/EV and faster DV | Numeric CERTInext organization number for a pre-vetted organization. When set, every SSL order is submitted with `organizationDetails.preVetting="1"` and this number, telling CERTInext to skip its manual organization-vetting queue. Without it, orders may sit in `Pending System RA` for extended manual review (observed: tens of hours). | Portal → **Organizations → Pre-vetted Organizations**. | `1234567` | +| `TechnicalContactName` / `TechnicalContactEmail` / `TechnicalContactIsdCode` / `TechnicalContactMobileNumber` | Optional | Populate `technicalPointOfContact` on every SSL order. Each defaults to the corresponding `Requestor*` field when blank. Some product configurations require a technical point of contact to be present; omitting it can cause CERTInext to park orders awaiting manual completion of the field. | N/A | *(defaults to Requestor fields)* | +| `AccountingModel` | Optional | CERTInext billing model sent in `orderDetails.accountingModel`. `2` = credit-based (most accounts). `1` = cash model. Default: `2`. | N/A | `2` | +| `EmailNotifications` | Optional | Whether CERTInext sends lifecycle-event emails to the requestor. `1` = enabled, `0` = silent (recommended for gateway-driven orders). Default: `0`. | N/A | `0` | +| `SubscriptionValidityYears` | Optional | Connector-level default validity in years for SSL orders (`1`, `2`, or `3`). Overridden per template by the `ValidityYears` enrollment parameter. Default: `1`. | N/A | `1` | +| `SubscriptionAutoRenew` | Optional | Whether CERTInext should auto-renew certificates issued through this connector. `0` = disabled (recommended — renewal is driven by Keyfactor Command), `1` = enabled. Default: `0`. | N/A | `0` | +| `SubscriptionRenewCriteriaDays` | Optional | Days before expiry at which CERTInext auto-renews. Only honored when `SubscriptionAutoRenew` is `1`. Default: `30`. | N/A | `30` | +| `AutoSecureWww` | Optional | If `1`, CERTInext automatically adds the `www.` variant of the primary domain as an additional SAN. Default: `0`. | N/A | `0` | +| `SubmitNonDnsSans` | Optional | If `true` (default), SANs that aren't DNS names (IP address, email, URI) are submitted to CERTInext instead of silently dropped. CERTInext can't validate them, so such an order won't issue until they're removed. Set to `false` to restore the pre-1.0.1 behavior of submitting DNS names only. Default: `true`. | N/A | `true` | +| `DefaultProductCode` | Optional, but effectively required if you use renewals | Numeric product code used for **renewals only** — CERTInext's `TrackOrder` doesn't return the prior order's product code, so the renewal path sends this value verbatim, ignoring the template's `ProductCode`/`ProfileId`. If left blank, renewals go out with an empty product code. Has **no effect on new enrollments** — the `ProductCode`/`ProfileId` template resolution never falls back to it. See [issue tracking this](https://github.com/Keyfactor/certinext-caplugin/issues/26). | Call `GetProductDetails` against your account/environment (see product code table below). | `842` | | `IgnoreExpired` | Optional | If `true`, expired certificates are skipped during synchronization and are not imported into Keyfactor Command. Default: `false`. | N/A | `false` | | `PageSize` | Optional | Number of orders to retrieve per page during synchronization. Default: `100`. Maximum: `500`. Reduce this value if synchronization requests time out. | N/A | `100` | | `Enabled` | Optional | Enables or disables the CA connector. Setting this to `false` allows the connector record to be created before all credentials are available, without triggering a live connectivity test. Default: `true`. | N/A | `true` | +| `PickupRetries` | Optional | Number of times `Enroll` polls CERTInext for the certificate after a successful order submission, before returning pending and leaving pickup to the next sync. Set to `0` to disable the wait. OV/EV orders validate asynchronously (minutes to hours) and typically exhaust this wait regardless of the value. Default: `5`. | N/A | `5` | +| `PickupDelay` | Optional | Seconds between certificate-pickup retries. `PickupRetries × PickupDelay` (plus a short initial delay) bounds how long an enrollment call occupies a Command worker thread — capped at a 180s ceiling regardless of how the two are set (aim for well under ~90s in practice, so the call doesn't run long enough to trip Command's own timeout). Default: `10` (a ~55s ceiling with default `PickupRetries`). | N/A | `10` | | `DcvEnabled` | Optional | When `true`, the gateway performs DNS-based Domain Control Validation (DCV) during enrollment for orders that require it. Requires a DNS provider plugin (e.g. `azure-azuredns-dnsplugin`) to be deployed on the gateway. Default: `false`. | N/A | `false` | | `DcvTxtRecordTemplate` | Optional | Format string for the DNS TXT record hostname published during DCV. `{0}` is replaced with the domain being validated. Default: `_emsign-validation.{0}`. | N/A | `_emsign-validation.{0}` | -| `DcvPropagationDelaySeconds` | Optional | Seconds to wait after publishing the DNS TXT record before asking CERTInext to verify it. Increase for zones with slow propagation. Default: `30`. | N/A | `30` | +| `DcvPropagationDelaySeconds` | Optional | Seconds to wait after publishing the DNS TXT record before asking CERTInext to verify it. Increase for zones with slow propagation. Applies only to the `Enroll()`-time DCV path — DCV driven during sync uses its own fixed 3-second delay. Default: `30`. | N/A | `30` | | `DcvTimeoutMinutes` | Optional | Maximum minutes to wait for the entire DCV flow (DNS publish + propagation + verify) before cancelling the enrollment. Can also be set via the `CERTINEXT_DCV_TIMEOUT_MINUTES` environment variable; the environment variable takes precedence when both are set. Default: `10`. | N/A | `10` | +| `DcvWaitForChallengeSeconds` | Optional | How long `Enroll()` waits for CERTInext to expose the DCV challenge after order placement, before giving up and deferring to the next sync. Set to `0` to disable the wait. Can also be set via `CERTINEXT_DCV_WAIT_FOR_CHALLENGE_SECONDS`. Default: `60`. | N/A | `60` | +| `DcvWaitForIssuanceSeconds` | Optional | How long `Enroll()` waits for CERTInext to finish generating the certificate after DCV verifies. Set to `0` to disable the wait. Can also be set via `CERTINEXT_DCV_WAIT_FOR_ISSUANCE_SECONDS`. Default: `60`. | N/A | `60` | +| `DcvSyncMaxOrderAgeHours` | Optional | During synchronization, only pending DV orders younger than this many hours are driven through DCV, so a large backlog of old/abandoned pending orders doesn't slow down every sync pass. Set to `0` to disable the age filter. Default: `24`. | N/A | `24` | +| `DcvSyncMaxPerPass` | Optional | Maximum number of pending DV orders driven through DCV in a single sync pass. Set to `0` to disable the cap. Default: `50`. | N/A | `50` | > Note: `AccountNumber` and group-level identifiers are distinct values. The `AccountNumber` is your top-level user account identifier. CERTInext groups (cost centers or departments) each have their own `groupNumber`, which is passed per-order and is separate from any organization number displayed on the Organizations page. @@ -453,6 +471,8 @@ sequenceDiagram **Expired certificates:** The `IgnoreExpired` connector setting controls whether expired certificates are included in synchronization. When enabled, expired certificates are silently skipped and will not appear in the Keyfactor Command inventory. +**DCV-during-sync:** on a DCV-enabled build, each sync pass also drives DNS-01 validation forward for pending DV orders that are still waiting on it, bounded by `DcvSyncMaxOrderAgeHours` (skip orders older than this) and `DcvSyncMaxPerPass` (cap how many are attempted per pass), so a large backlog of stalled pending orders can't slow down every sync. + --- ## Certificate Enrollment @@ -474,13 +494,27 @@ sequenceDiagram Plugin->>API: Place certificate order\n(CSR, domain, organization details,\nsubscriber agreement, requestor info) API-->>Plugin: Order accepted — order number assigned + opt DNS-01 DCV build, DCV enabled, and this order requires it + Plugin->>Plugin: Publish DNS TXT challenge\nvia the configured DNS provider plugin + Plugin->>API: Ask CERTInext to verify the record + API-->>Plugin: Domain validated (or still pending —\nfalls through to the pending path below) + end + Plugin->>API: Check order status API-->>Plugin: Order status and certificate details alt Certificate issued immediately Plugin-->>CMD: Certificate ready — PEM returned - else Certificate pending approval - Plugin-->>CMD: Pending — Command will pick it up\nduring the next synchronization + else Certificate pending or not yet downloadable + loop Certificate-pickup retries\n(bounded, ~55s by default — PickupRetries/PickupDelay) + Plugin->>API: Poll for the certificate + API-->>Plugin: Status and certificate, if ready + end + alt Certificate became available during pickup + Plugin-->>CMD: Certificate ready — PEM returned + else Still not available + Plugin-->>CMD: Pending — Command will pick it up\nduring the next synchronization + end else Order rejected by CERTInext Plugin-->>CMD: Enrollment failed — see gateway logs end @@ -488,12 +522,18 @@ sequenceDiagram Plugin->>Plugin: Record enrollment outcome in audit log\n(order number, serial number, status) ``` +**DCV:** on a DCV-enabled build, DNS-01 validation runs inline for DV orders that require it, bounded by `DcvTimeoutMinutes`. When DCV isn't enabled, isn't built into this host, or the order doesn't require it, this step is skipped entirely and the order proceeds straight to the pending/pickup path like any other asynchronously-issued order. + +**Synchronous certificate pickup:** if the certificate isn't available immediately (a fresh order, or DCV that just validated but hasn't finished generating the PEM), `Enroll()` polls CERTInext a bounded number of times (`PickupRetries` × `PickupDelay`, capped at a 180s ceiling) before giving up and returning pending. This lets a fast-issuing certificate (DV, or an already-approved order) come back in the same enrollment call instead of always waiting for the next sync. OV/EV orders validate asynchronously over minutes to hours and typically exhaust this window regardless. + ### Renewal When Command initiates a renewal, the plugin checks whether the existing certificate is within the configured renewal window. If it is, the prior order record is used as context for the new request. If it is outside the window (or the prior certificate cannot be located), the plugin falls back to issuing a new certificate. > **Note:** CERTInext does not have a dedicated certificate renewal endpoint. Both renewal and reissuance paths submit a new `GenerateOrderSSL` order. The distinction affects how Keyfactor Command tracks the certificate record, not what is sent to CERTInext. +> **Note:** If the prior-order lookup itself throws (rather than cleanly returning "not found" — e.g. a transient database error), the plugin falls back to issuing a new certificate rather than failing the enrollment. + ```mermaid flowchart TD A([Renewal requested]) --> B{Prior certificate\nserial number\nprovided?} From a890a7dd0f8e558d174a886abd319df810f2b045 Mon Sep 17 00:00:00 2001 From: spb <1661003+spbsoluble@users.noreply.github.com> Date: Thu, 13 Aug 2026 12:25:12 -0700 Subject: [PATCH 09/10] fix(enroll): renewal product code, AutoApprove UI text, config log visibility (#28) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix(enroll): renewals ignore template product code, correct AutoApprove UI text, log config presence Three independent fixes found during UCSD triage (issues #25, #26, #27): - RenewCertificateAsync built every renewal order from the connector's DefaultProductCode alone, ignoring the template's own ProductCode/ProfileId entirely. Threaded the template's code through RenewCertificateRequest.ProfileId, falling back to DefaultProductCode only when the template doesn't have one (using a blank-check, not ??, since EnrollmentParams.ProductCode never returns null — the same dead-fallback bug that made DefaultProductCode a no-op for new enrollments). - AutoApprove's UI text claimed the plugin attempts automatic approval of pending certificates; no such call exists anywhere in the code. Corrected to say so plainly. - OrganizationNumber, DefaultProductCode, and GroupNumber had zero log visibility, which is what made a stuck-pending-orders question undiagnosable from a support log. Added presence flags to the plugin-initialized log line. --- .../CERTInextCAPluginCoverageTests.cs | 54 +++++++++++++++++++ .../CERTInextClientRequestShapeTests.cs | 54 +++++++++++++++++++ CERTInext/API/CertificateRequest.cs | 9 ++++ CERTInext/CERTInextCAPlugin.cs | 8 +++ CERTInext/CERTInextCAPluginConfig.cs | 4 +- CERTInext/Client/CERTInextClient.cs | 12 +++-- 6 files changed, 136 insertions(+), 5 deletions(-) diff --git a/CERTInext.Tests/CERTInextCAPluginCoverageTests.cs b/CERTInext.Tests/CERTInextCAPluginCoverageTests.cs index f684f7d..1a9faa9 100644 --- a/CERTInext.Tests/CERTInextCAPluginCoverageTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginCoverageTests.cs @@ -259,6 +259,60 @@ public async Task RenewOrReissue_CallsRenewApi_WhenCertWithinRenewalWindow() It.IsAny()), Times.Never); } + // --------------------------------------------------------------------------- + // A1d-2: renewal within window carries the template's product code onto the + // RenewCertificateRequest, not just the connector-level DefaultProductCode. + // Regression for issue #26 / local issues/0012. + // --------------------------------------------------------------------------- + + [Fact] + public async Task RenewOrReissue_CallsRenewApi_UsesTemplateProductCode() + { + var clientMock = NewMock(); + var readerMock = NewReaderMock(); + + // Expiry is 30 days in the future, renewal window is 90 days → within window + DateTime expiry = DateTime.UtcNow.AddDays(30); + + readerMock + .Setup(r => r.GetRequestIDBySerialNumber(It.IsAny())) + .ReturnsAsync(MockCertificateData.CertId1); + + readerMock + .Setup(r => r.GetExpirationDateByRequestId(MockCertificateData.CertId1)) + .Returns(expiry); + + clientMock + .Setup(c => c.RenewCertificateAsync( + MockCertificateData.CertId1, + It.Is(r => r.ProfileId == MockCertificateData.ProfileIdClient), + It.IsAny())) + .ReturnsAsync(MockCertificateData.IssuedEnrollResponse("cert-renewed-002")); + + var plugin = new CERTInextCAPlugin(clientMock.Object, readerMock.Object); + + // ProfileId is a non-default value distinct from the connector's DefaultProductCode. + var productInfo = MakeProductInfo(profileId: MockCertificateData.ProfileIdClient, extras: new Dictionary + { + ["PriorCertSN"] = "AABBCCDDEEFF", + ["RenewalWindowDays"] = "90" + }); + + var result = await plugin.Enroll( + csr: MockCertificateData.FakeCsrPem, + subject: "CN=test.example.com", + san: null, + productInfo: productInfo, + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.RenewOrReissue); + + result.Status.Should().Be((int)EndEntityStatus.GENERATED); + clientMock.Verify(c => c.RenewCertificateAsync( + MockCertificateData.CertId1, + It.Is(r => r.ProfileId == MockCertificateData.ProfileIdClient), + It.IsAny()), Times.Once); + } + // --------------------------------------------------------------------------- // A1e: PriorCertSN present, cert already expired → new enroll // Semantics: useRenewalApi = expiry > now && expiry <= now + window. diff --git a/CERTInext.Tests/CERTInextClientRequestShapeTests.cs b/CERTInext.Tests/CERTInextClientRequestShapeTests.cs index 4e59495..fd61dfb 100644 --- a/CERTInext.Tests/CERTInextClientRequestShapeTests.cs +++ b/CERTInext.Tests/CERTInextClientRequestShapeTests.cs @@ -288,5 +288,59 @@ public async Task ValidityDays_OnRequest_OverridesConnectorDefault() CapturedOrderBody().GetProperty("subscriptionDetails") .GetProperty("validity").GetString().Should().Be("2"); } + + // ----------------------------------------------------------------------- + // RenewCertificateAsync — productCode resolution (issue #26 / local issues/0012) + // Renewals go out as a fresh GenerateOrderSSL order; the product code must + // come from the template (RenewCertificateRequest.ProfileId) when supplied, + // falling back to the connector's DefaultProductCode only when it is not. + // ----------------------------------------------------------------------- + + [Fact] + public async Task RenewCertificateAsync_ProfileIdSet_UsesTemplateProductCode() + { + StubHappyEnroll(); + var cfg = MinimalConfig(); + cfg.DefaultProductCode = "connector-default-code"; + + var renewReq = new RenewCertificateRequest + { + Csr = MockCertificateData.FakeCsrPem, + ProfileId = "template-product-code", + ValidityDays = 365, + Comment = "Renewal test" + }; + + await BuildClient(cfg).RenewCertificateAsync(MockCertificateData.OrderNumber1, renewReq); + + CapturedOrderBody().GetProperty("productCode").GetString() + .Should().Be("template-product-code", + "the template's own product code must win over the connector default"); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public async Task RenewCertificateAsync_ProfileIdBlank_FallsBackToConnectorDefault(string blankProfileId) + { + StubHappyEnroll(); + var cfg = MinimalConfig(); + cfg.DefaultProductCode = "connector-default-code"; + + var renewReq = new RenewCertificateRequest + { + Csr = MockCertificateData.FakeCsrPem, + ProfileId = blankProfileId, + ValidityDays = 365, + Comment = "Renewal test" + }; + + await BuildClient(cfg).RenewCertificateAsync(MockCertificateData.OrderNumber1, renewReq); + + CapturedOrderBody().GetProperty("productCode").GetString() + .Should().Be("connector-default-code", + "a blank ProfileId must fall back to the connector's DefaultProductCode, not an empty string"); + } } } diff --git a/CERTInext/API/CertificateRequest.cs b/CERTInext/API/CertificateRequest.cs index 043b4b5..c0da0d6 100644 --- a/CERTInext/API/CertificateRequest.cs +++ b/CERTInext/API/CertificateRequest.cs @@ -631,6 +631,15 @@ public class RenewCertificateRequest [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] public string Subject { get; set; } + /// + /// Template/enrollment product code to submit the renewal order under. Without it, the + /// renewal falls back to the connector-level default product code, which is often unset — + /// leaving renewals to go out under an empty product code regardless of the template used. + /// + [JsonPropertyName("profileId")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public string ProfileId { get; set; } + /// /// SANs to carry onto the renewal order. Renewals previously submitted none, so a /// renewed UCC certificate came back holding only its primary domain. diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index a631606..76c3cb4 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -240,6 +240,9 @@ public void Initialize(IAnyCAPluginConfigProvider configProvider, ICertificateDa bool hasClientId = !string.IsNullOrWhiteSpace(_config.OAuth2ClientId); bool hasClientSecret= !string.IsNullOrWhiteSpace(_config.OAuth2ClientSecret); bool hasTokenUrl = !string.IsNullOrWhiteSpace(_config.OAuth2TokenUrl); + bool hasOrganizationNumber = !string.IsNullOrWhiteSpace(_config.OrganizationNumber); + bool hasDefaultProductCode = !string.IsNullOrWhiteSpace(_config.DefaultProductCode); + bool hasGroupNumber = !string.IsNullOrWhiteSpace(_config.GroupNumber); _logger.LogInformation( "CERTInext plugin initialized. " + @@ -247,6 +250,8 @@ public void Initialize(IAnyCAPluginConfigProvider configProvider, ICertificateDa "ApiKeyPresent={ApiKeyPresent}, UsernamePresent={UsernamePresent}, " + "PasswordPresent={PasswordPresent}, OAuth2ClientIdPresent={OAuth2ClientIdPresent}, " + "OAuth2ClientSecretPresent={OAuth2ClientSecretPresent}, OAuth2TokenUrlPresent={OAuth2TokenUrlPresent}, " + + "OrganizationNumberPresent={OrganizationNumberPresent}, DefaultProductCodePresent={DefaultProductCodePresent}, " + + "GroupNumberPresent={GroupNumberPresent}, " + "PageSize={PageSize}, IgnoreExpired={IgnoreExpired}, SubmitNonDnsSans={SubmitNonDnsSans}, " + "DcvEnabled={DcvEnabled}, DcvTxtRecordTemplate={DcvTxtRecordTemplate}, " + "DomainValidatorFactoryInjected={FactoryInjected}", @@ -254,6 +259,8 @@ public void Initialize(IAnyCAPluginConfigProvider configProvider, ICertificateDa hasApiKey, hasUsername, hasPassword, hasClientId, hasClientSecret, hasTokenUrl, + hasOrganizationNumber, hasDefaultProductCode, + hasGroupNumber, _config.PageSize, _config.IgnoreExpired, _config.SubmitNonDnsSans, _config.DcvEnabled, _config.DcvTxtRecordTemplate, _domainValidatorFactory != null); @@ -1320,6 +1327,7 @@ private async Task RenewOrReissueAsync( // holding only its primary domain. Subject = subject, Sans = BuildSanList(san, csr, subject), + ProfileId = ep.ProductCode, ValidityDays = ep.ValidityDays > 0 ? ep.ValidityDays : (int?)null, RequesterName = string.IsNullOrWhiteSpace(ep.RequesterName) ? null : ep.RequesterName, RequesterEmail = string.IsNullOrWhiteSpace(ep.RequesterEmail) ? null : ep.RequesterEmail, diff --git a/CERTInext/CERTInextCAPluginConfig.cs b/CERTInext/CERTInextCAPluginConfig.cs index 980a26a..93fb26a 100644 --- a/CERTInext/CERTInextCAPluginConfig.cs +++ b/CERTInext/CERTInextCAPluginConfig.cs @@ -444,8 +444,8 @@ public static Dictionary GetTemplateParameterAnnotat }, [Constants.EnrollmentParam.AutoApprove] = new PropertyConfigInfo { - Comments = "OPTIONAL: If true, the gateway will attempt automatic approval of certificates " + - "that are returned in a pending-approval state. Default: false.", + Comments = "Currently has no effect — reserved for future use. The plugin does not call " + + "any approval endpoint against CERTInext regardless of this setting.", Hidden = false, DefaultValue = false, Type = "Boolean" diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index 9ecde2b..2fdcc18 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -810,14 +810,20 @@ public async Task RenewCertificateAsync( certificateId, LogSanitizer.Strip(renewalDomainName)); } - // We don't have the product code from TrackOrder — build an order using - // the config defaults and the CSR from the renewal request. + // Prefer the template's own product code (threaded through via request.ProfileId); + // only fall back to the connector-level default when the caller didn't supply one. + // EnrollmentParams.ProductCode never returns null (it returns string.Empty when it + // can't resolve a code), so this must be a blank check, not a null-coalesce — a + // null-coalesce here would make the DefaultProductCode fallback unreachable, the + // same dead-fallback bug that made DefaultProductCode a no-op for new enrollments. var orderReq = new GenerateOrderSslRequest { Meta = await BuildMetaAsync(ct), OrderDetails = new SslOrderDetails { - ProductCode = _config.DefaultProductCode ?? string.Empty, + ProductCode = string.IsNullOrWhiteSpace(request.ProfileId) + ? (_config.DefaultProductCode ?? string.Empty) + : request.ProfileId, SaveAndHold = "0", RequestorInformation = new RequestorInformation { From 00ccdbd60f8c3ee19363fcbc94699b77698d470a Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Thu, 13 Aug 2026 13:51:26 -0700 Subject: [PATCH 10/10] docs(changelog): add PR #28's renewal product-code, AutoApprove, and logging fixes to 1.0.1 --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11bd7b5..dff6588 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ - **UCC certificates no longer come back with only the common name.** The gateway sends SANs under the key `dnsname`, which the plugin didn't recognize, so orders went out with an empty domain list. SANs are now read from every key the gateway sends, plus from the CSR itself. - **Renewals no longer lose their SANs.** Renewals were submitted with no additional domains and the wrong primary domain; both now come from the certificate being renewed. - **Enrollment no longer fails on an order CERTInext auto-approves before it finishes issuing.** The plugin used to report these as issued with no certificate attached, which the gateway rejected. It now returns pending and picks up the certificate once CERTInext finishes issuing it. +- **Renewals now use the certificate template's product code.** Renewals previously always used the connector's `DefaultProductCode`, which could send an empty product code if that setting was never configured. Renewals now use the template's code, falling back to `DefaultProductCode` only when the template doesn't have one. + +## Chores +- **`OrganizationNumber`, `DefaultProductCode`, and `GroupNumber` are now visible in the startup log.** Whether each is set is now logged alongside the other connector settings, making a misconfigured connector easier to diagnose from logs alone. +- **Corrected the `AutoApprove` template setting's description.** It previously implied the plugin would attempt automatic approval of pending certificates; it does not currently do this. ## Upgrade Notes - **Non-DNS SANs (IP, email, URI) are now submitted instead of silently dropped.** CERTInext can't validate them, so such an order won't issue until the SAN is removed. Set `SubmitNonDnsSans` to `false` to restore the old drop-silently behavior.