fix(enroll): UCC SANs never reached CERTInext — additionalDomains sent empty - #21
Open
spbsoluble wants to merge 12 commits into
Open
fix(enroll): UCC SANs never reached CERTInext — additionalDomains sent empty#21spbsoluble wants to merge 12 commits into
spbsoluble wants to merge 12 commits into
Conversation
…t 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.
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.
…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.
…iant, 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.
…ging, 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.
…n, 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.
…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.
…oken, 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.
…bject 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.
…, 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.
…ggregate 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.
…ation 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.
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
Certificates enrolled through a UCC product came back holding only the CN, even though the requested SANs were on the CSR and in the SAN data Command supplied. A customer reported it as "UCC SANs not completely populating the certificate although SANs are included on the CSR" — reasonably read as the CA stripping them. The names were being dropped inside this plugin.
Affects all shipped versions — the SAN code is identical on
release-1.0,release-1.1andmain.Root cause
The AnyCA REST Gateway keys its SAN dictionary
dnsname, butMapSanTypeonly recognizeddns. So:"dnsname".BuildAdditionalDomainskept entries only whereType == "dns"→ all SANs filtered out.AdditionalDomainsbecamenull, and[JsonIgnore(WhenWritingNull)]removedadditionalDomainsfrom the order body entirely.domainName(the CN) alone.Confirmed in a customer gateway log — note the key:
The CSR did not compensate, because CERTInext ignores the CSR's
subjectAltNameextension outright (measured below).Why no test caught it: the only test touching this path used
Type = "DNS", which maps cleanly and works. No test used the real gateway key.Measured CERTInext behaviour
The old code encoded three assumptions that had never been tested.
SanSubmissionProbeTests(new, opt-in) measures them against the live API — sandbox-us, product 844 / OV SSL UCC — by placing one order per variant and reading back the domain set the CA registered viaTrackOrder'sdomainVerificationkeys.extra1.<cn>viaadditionalDomainsextra2.<cn>,additionalDomainsomittedadditionalDomainsemail:san-probe@example.comip:192.0.2.10uri:https://san-probe.example.com/xTwo assumptions were wrong:
Product 840 (DV UCC) is not enabled on the sandbox account (
Invalid Product Code), so 844 carried the probe.Changes
MapSanTyperecognizes what the gateway actually sends:dnsname,rfc822name,ipaddress,uniformresourceidentifier. Short forms still work.BuildSanListunions gateway-supplied SANs with SANs parsed from the CSR (BouncyCastle, per project crypto policy), de-duplicating on type+value case-insensitively. Non-throwing: an unparseable CSR falls back to the gateway set. Parsing the CSR is not redundant with sending it — the CA won't read those names, so re-submitting viaadditionalDomainsis the only way a CSR-only SAN reaches the certificate.BuildAdditionalDomainsno longer filters to DNS-only. Everything requested is submitted; silently dropping names the subscriber asked for is the worse failure. Excludes the value already going out asdomainName.domainNamefrom the prior order'srequestorName(not a domain).RenewCertificateRequestnow carriesSubject+Sans; the renewal order derivesdomainNamefrom the subject CN, with the old value as a logged fallback.PlaceOrderAsyncnow logsdomainNameandadditionalDomains. The absence of outbound domain logging is precisely what made this read as CA-side stripping — the log recorded what Command supplied and nothing about what went on the wire.Behaviour change to be aware of
Non-DNS SANs (IP / email / URI) previously vanished silently and the certificate issued without them. They are now submitted, so such an enrollment produces an order that parks pending instead of issuing and needs manual cancellation. This is intentional — a visibly stuck order beats a certificate quietly missing requested names — and the plugin logs a Warning naming the offending SANs. Called out because it turns a (lossy) success into a visible non-issuance.
Tests
SanSubmissionTests) driveplugin.Enrollthrough a realCERTInextClientagainst WireMock and assert on the JSON actually posted. A test of the mapping function alone would not have caught this bug — the mapping "worked"; the loss happened in its interaction with the downstream filter. Covers: thednsnamekey, the shortdnskey, CSR-only SANs, gateway∪CSR de-duplication, CN not repeated, non-DNS submitted not dropped, unparseable CSR, no-SANs-anywhere, and the renew request carrying subject + SANs.SanSubmissionProbeTests(integration, opt-in behindCERTINEXT_SAN_PROBE=1) records the live measurements above.Review notes
hotfix/v1.0.1-sectigo-parity-pickupso the diff is this one commit; GitHub retargets torelease-1.0when v1.0.1: synchronous certificate pickup (Sectigo parity) #20 merges. Merge after v1.0.1: synchronous certificate pickup (Sectigo parity) #20, or rebase if v1.0.1: synchronous certificate pickup (Sectigo parity) #20 changes.release-1.1in a separate PR.