v1.0.1: synchronous certificate pickup (Sectigo parity) - #20
Open
spbsoluble wants to merge 10 commits into
Open
Conversation
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.
spbsoluble
force-pushed
the
hotfix/v1.0.1-sectigo-parity-pickup
branch
from
July 31, 2026 17:11
d49e2aa to
3e3d581
Compare
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).
…k timeout 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.
…dance 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).
…dit logging 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.
This was referenced Aug 12, 2026
…t empty (#21) * 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<T> 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.
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.
…sibility (#28) 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.
…logging fixes to 1.0.1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an in-call certificate pickup to enrollment. After an order is submitted, the plugin waits briefly for the certificate and returns it in the same enrollment request when it issues quickly — so DV and already-approved certs come back immediately instead of waiting for the next synchronization. Mirrors the legacy Sectigo connector's pickup behavior.
Behavior
New settings
PickupRetries— number of poll attempts (default5; set to0to disable the wait)PickupDelay— seconds between polls (default10)Also included
Fixes a duplicate/orphaned-order bug: order and CSR submissions are no longer retried after a network timeout. A timeout can occur after the CA has already accepted the request, so the retry was being rejected as a duplicate — failing the enrollment and leaving an orphaned order behind. These now run once and reconcile on the next sync.
Testing
-p:DcvSupport=falseand-p:DcvSupport=true).