fix(enroll): UCC SANs never reached CERTInext — additionalDomains sent empty (1.1 port) - #22
fix(enroll): UCC SANs never reached CERTInext — additionalDomains sent empty (1.1 port)#22spbsoluble wants to merge 2 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.
Port note (release-1.1): the unit-test helper drops the PickupRetries=0 config
used on the 1.0.1 branch. Synchronous certificate pickup is a 1.0.x hotfix
feature and CERTInextConfig has no PickupRetries on this line, so the plugin is
constructed with the injected client alone. The fix itself is unchanged.
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.
Status update: this PR has the base fix only, not the full-review hardeningSince this PR was opened, the companion PR into This branch does not have those fixes. Porting them turned out to be more than a mechanical cherry-pick: I also found that porting the surrounding support code alone (the What this PR currently has: the original UCC-SAN fix ( Recommendation: land #21 first, then redo this port as its own focused task once release-1.0 is settled — the merge may be more tractable then, or worth doing as a deliberate rebase rather than a cherry-pick chain. |
1 participant
Summary
Port of the UCC SAN fix to the 1.1 line. Original: #21 (stacked on #20, targeting
release-1.0).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.
The SAN code was identical across
release-1.0,release-1.1andmain, so 1.1 has the bug in full.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:
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.Difference from #21
One adaptation, in test code only — the fix itself is byte-identical:
SanSubmissionTests.BuildPlugin()drops thePickupRetries = 0config. Synchronous certificate pickup is a 1.0.x hotfix feature andCERTInextConfighas noPickupRetrieson this line, so the plugin is constructed with the injected client alone.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. Intentional — a visibly stuck order beats a certificate quietly missing requested names — and the plugin logs a Warning naming the offending SANs.
Tests
9 new unit tests drive
plugin.Enrollthrough a realCERTInextClientagainst WireMock and assert on the JSON actually posted — a test of the mapping function alone would not have caught this bug, since the mapping "worked" and the loss happened in its interaction with the downstream filter. The live probe is opt-in behindCERTINEXT_SAN_PROBE=1.Review notes
release-1.1as requested; nothing committed tomain.mainandrelease-1.1are source-identical acrossCERTInext/,CERTInext.Tests/andCERTInext.IntegrationTests/(empty diff), so this covers themaincontent too and can flow forward on the normal release merge.