From 35deb42868cb62ab47f2eb6b50352e7bfba1aade Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Wed, 12 Aug 2026 08:40:04 -0700 Subject: [PATCH 1/2] =?UTF-8?q?fix(enroll):=20UCC=20SANs=20never=20reached?= =?UTF-8?q?=20CERTInext=20=E2=80=94=20additionalDomains=20sent=20empty?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../SanSubmissionProbeTests.cs | 383 +++++++++++++++ CERTInext.Tests/SanSubmissionTests.cs | 443 ++++++++++++++++++ CERTInext/API/CertificateRequest.cs | 17 + CERTInext/CERTInextCAPlugin.cs | 239 +++++++++- CERTInext/Client/CERTInextClient.cs | 91 +++- 5 files changed, 1145 insertions(+), 28 deletions(-) create mode 100644 CERTInext.IntegrationTests/SanSubmissionProbeTests.cs create mode 100644 CERTInext.Tests/SanSubmissionTests.cs diff --git a/CERTInext.IntegrationTests/SanSubmissionProbeTests.cs b/CERTInext.IntegrationTests/SanSubmissionProbeTests.cs new file mode 100644 index 0000000..32df577 --- /dev/null +++ b/CERTInext.IntegrationTests/SanSubmissionProbeTests.cs @@ -0,0 +1,383 @@ +// Copyright 2026 Keyfactor +// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. +// At http://www.apache.org/licenses/LICENSE-2.0 +// +// Probe: establish empirically how CERTInext treats the SAN/domain fields on +// GenerateOrderSSL. Written because the plugin's original behaviour encoded three +// assumptions that were never measured: +// +// A. certificateInformation.additionalDomains is the field that puts extra names on +// the certificate (so a UCC order that omits it yields a CN-only certificate). +// B. additionalDomains accepts DNS names only, so a non-DNS SAN is rejected by the CA. +// C. Repeating the primary domainName inside additionalDomains is harmful (duplicate +// domain / consumes the UCC allowance), so it should be de-duplicated. +// +// None of these had a test. This probe answers them against the live API by placing one +// order per variant and reading back the domain set CERTInext actually registered, via +// TrackOrder's domainVerification block (keys are the domains on the order). That is +// ground truth for "which names did the CA put on this order" without waiting for DCV +// and issuance to complete. +// +// --------------------------------------------------------------------------------------- +// MEASURED RESULTS — sandbox-us, account 4951571271, product 844 (OV SSL UCC), 2026-08-12. +// (Product 840 / DV UCC is not enabled on that account: "Invalid Product Code".) +// +// A. CONFIRMED. additionalDomains is what puts extra names on the order. Submitting +// CN + extra1. registered BOTH domains. +// +// B. DISPROVEN. Non-DNS values are NOT rejected. An email address, an IPv4 literal and +// an https:// URI were each accepted at placement AND registered as order domains +// ("san-probe@example.com", "192.0.2.10", "https://san-probe.example.com/x" all came +// back as domainVerification keys). So the CA does not validate the field's contents +// at order time; such an order is created and then cannot pass DCV, rather than +// failing cleanly up front. +// +// C. PARTLY DISPROVEN. Repeating the primary domainName inside additionalDomains is +// accepted and CERTInext collapses it itself — the order came back with the CN +// registered once. De-duplicating on our side is therefore belt-and-braces, not a +// correctness requirement. +// +// Root cause of the customer-reported "UCC SANs not populating": CERTInext IGNORES the +// subjectAltName extension in the CSR. A CSR carrying CN + extra2., submitted with +// additionalDomains omitted, registered ONLY the CN. SANs must be sent in +// additionalDomains or they do not reach the certificate, no matter what the CSR says. +// --------------------------------------------------------------------------------------- +// +// Opt-in: this places real orders against whatever account ~/.env_certinext points at. +// +// set -a; . ~/.env_certinext; set +a +// export CERTINEXT_SAN_PROBE=1 +// dotnet test CERTInext.IntegrationTests/CERTInext.IntegrationTests.csproj -c Release \ +// --filter "FullyQualifiedName~SanSubmissionProbeTests" \ +// --logger "console;verbosity=detailed" > /tmp/sanprobe.log 2>&1 +// +// (xUnit buffers ITestOutputHelper output until the test ends — read the report at the tail.) + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using Keyfactor.Extensions.CAPlugin.CERTInext.API; +using Keyfactor.Extensions.CAPlugin.CERTInext.Client; +using Org.BouncyCastle.Asn1; +using Org.BouncyCastle.Asn1.Pkcs; +using Org.BouncyCastle.Asn1.X509; +using Org.BouncyCastle.Crypto; +using Org.BouncyCastle.Crypto.Generators; +using Org.BouncyCastle.Pkcs; +using Org.BouncyCastle.Security; +using Xunit; +using Xunit.Abstractions; + +namespace Keyfactor.Extensions.CAPlugin.CERTInext.IntegrationTests +{ + public class SanSubmissionProbeTests : IClassFixture + { + private const string OptInFlag = "CERTINEXT_SAN_PROBE"; + + /// + /// Comma-separated product codes to probe. Defaults to the Multi-Domain (UCC) codes, + /// because additional domains are only meaningful on a UCC product — a single-domain + /// product (e.g. 842 = OV SSL) registers the CN and nothing else no matter what + /// additionalDomains contains, which makes it useless as a probe target. + /// + private const string ProductCodesFlag = "CERTINEXT_SAN_PROBE_PRODUCTS"; + private const string DefaultProductCodes = "840,844"; + + private readonly IntegrationTestFixture _fixture; + private readonly ITestOutputHelper _out; + + public SanSubmissionProbeTests(IntegrationTestFixture fixture, ITestOutputHelper output) + { + _fixture = fixture; + _out = output; + } + + // ------------------------------------------------------------------------- + // CSR generation (BouncyCastle — project crypto policy) + // ------------------------------------------------------------------------- + + /// + /// Generates a PKCS#10 CSR for , optionally carrying a + /// subjectAltName extension (via the PKCS#9 extensionRequest attribute) holding + /// . The SAN-bearing form is what lets this probe ask + /// whether CERTInext reads SANs out of the CSR at all. + /// + private static string GenerateCsrPem(string cn, params string[] dnsSans) + { + var keyGen = new RsaKeyPairGenerator(); + keyGen.Init(new KeyGenerationParameters(new SecureRandom(), 2048)); + AsymmetricCipherKeyPair kp = keyGen.GenerateKeyPair(); + + Asn1Set attributes = null; + if (dnsSans != null && dnsSans.Length > 0) + { + var names = new GeneralNames( + dnsSans.Select(d => new GeneralName(GeneralName.DnsName, d)).ToArray()); + + var extGen = new X509ExtensionsGenerator(); + extGen.AddExtension(X509Extensions.SubjectAlternativeName, critical: false, extValue: names); + + attributes = new DerSet(new AttributePkcs( + PkcsObjectIdentifiers.Pkcs9AtExtensionRequest, + new DerSet(extGen.Generate()))); + } + + var csr = new Pkcs10CertificationRequest( + "SHA256withRSA", new X509Name($"CN={cn}"), kp.Public, attributes, kp.Private); + + return "-----BEGIN CERTIFICATE REQUEST-----\n" + + Convert.ToBase64String(csr.GetEncoded(), Base64FormattingOptions.InsertLineBreaks) + + "\n-----END CERTIFICATE REQUEST-----"; + } + + // ------------------------------------------------------------------------- + // One probe variant + // ------------------------------------------------------------------------- + + private sealed class ProbeOutcome + { + public string ProductCode; + public string Label; + public bool Accepted; + public string OrderNumber; + public string Detail; + /// Domains CERTInext registered on the order, per TrackOrder. + public List RegisteredDomains = new List(); + /// Names we asked CERTInext to put on the order, for comparison. + public List RequestedDomains = new List(); + + /// + /// True when the rejection was "Invalid Product Code" — the product simply is not + /// enabled on this account, which is not a data point about SAN handling. + /// + public bool ProductUnavailable; + } + + /// + /// Places one order and reads back the domain set CERTInext registered for it. + /// drives certificateInformation.additionalDomains; + /// drives the SAN extension inside the CSR. They are + /// varied independently on purpose — that separation is the whole point of the probe. + /// + private async Task ProbeAsync( + string productCode, + string label, + Func> sansFactory, + string[] csrSans) + { + var outcome = new ProbeOutcome { ProductCode = productCode, Label = label }; + + var client = new CERTInextClient(_fixture.Config); + string cn = $"sanprobe-{DateTime.UtcNow:yyyyMMddHHmmssfff}.{SafeLabel(label)}.example.com"; + + var sans = sansFactory?.Invoke(cn); + outcome.RequestedDomains = sans == null + ? new List() + : sans.Select(s => $"{s.Type}:{s.Value}").ToList(); + + var req = new EnrollCertificateRequest + { + Csr = GenerateCsrPem(cn, csrSans == null ? null : csrSans.Select(s => Format(s, cn)).ToArray()), + Subject = $"CN={cn}", + Sans = sans, + ProfileId = productCode, + RequesterName = _fixture.RequestorName, + RequesterEmail = _fixture.RequestorEmail + }; + + try + { + var resp = await client.EnrollCertificateAsync(req); + outcome.Accepted = true; + outcome.OrderNumber = resp?.Id; + outcome.Detail = $"OrderNumber={resp?.Id} Status={resp?.Status}"; + } + catch (Exception ex) + { + outcome.Accepted = false; + outcome.Detail = ex.Message; + outcome.ProductUnavailable = + ex.Message.IndexOf("Invalid Product Code", StringComparison.OrdinalIgnoreCase) >= 0; + return outcome; + } + + // Read back which domains the CA actually put on the order. + try + { + var track = await client.TrackOrderAsync(outcome.OrderNumber); + var entries = track.OrderDetails?.DomainVerification?.GetDomainEntries(); + if (entries != null) + outcome.RegisteredDomains = entries.Keys.OrderBy(k => k, StringComparer.OrdinalIgnoreCase).ToList(); + } + catch (Exception ex) + { + outcome.Detail += $" | TrackOrder failed: {ex.Message}"; + } + + return outcome; + } + + /// Substitutes the generated CN into a variant's placeholder template. + private static string Format(string template, string cn) => template.Replace("{cn}", cn); + + private static string SafeLabel(string label) => + new string(label.ToLowerInvariant().Select(c => char.IsLetterOrDigit(c) ? c : '-').ToArray()) + .Trim('-'); + + // ------------------------------------------------------------------------- + // The probe + // ------------------------------------------------------------------------- + + [SkippableFact] + public async Task Probe_SanSubmissionBehaviour() + { + IntegrationSkip.IfNotConfigured(_fixture); + Skip.IfNot( + Environment.GetEnvironmentVariable(OptInFlag) == "1", + $"Set {OptInFlag}=1 to run this probe — it places real orders on the configured account."); + + var variants = new List<(string Label, Func> Sans, string[] CsrSans)> + { + // 1. Assumption A, positive control: additionalDomains carries an extra DNS + // name. If the extra name comes back registered, additionalDomains works. + ("dns-extra-via-additionalDomains", + cn => new List + { + new SanEntry { Type = "dns", Value = cn }, + new SanEntry { Type = "dns", Value = $"extra1.{cn}" } + }, + new[] { "{cn}", "extra1.{cn}" }), + + // 2. Assumption A, the actual bug: CSR carries both names, additionalDomains + // is omitted entirely. This is what v1.0.1 sent for every UCC enrollment. + // If only the CN comes back registered, the CA does NOT read CSR SANs and + // the diagnosis is confirmed. + ("csr-sans-only-no-additionalDomains", + _ => null, + new[] { "{cn}", "extra2.{cn}" }), + + // 3. Assumption C: primary domainName repeated inside additionalDomains. + // Does the CA reject it, or silently collapse it? + ("cn-duplicated-in-additionalDomains", + cn => new List + { + new SanEntry { Type = "dns", Value = cn }, + new SanEntry { Type = "dns", Value = cn } + }, + new[] { "{cn}" }), + + // 4-6. Assumption B: non-DNS values in additionalDomains. Rejected, ignored, + // or accepted? Each is submitted alongside a valid DNS name so a rejection + // is attributable to the non-DNS value rather than an empty domain set. + ("nondns-email-in-additionalDomains", + cn => new List + { + new SanEntry { Type = "dns", Value = cn }, + new SanEntry { Type = "email", Value = "san-probe@example.com" } + }, + new[] { "{cn}" }), + + ("nondns-ip-in-additionalDomains", + cn => new List + { + new SanEntry { Type = "dns", Value = cn }, + new SanEntry { Type = "ip", Value = "192.0.2.10" } + }, + new[] { "{cn}" }), + + ("nondns-uri-in-additionalDomains", + cn => new List + { + new SanEntry { Type = "dns", Value = cn }, + new SanEntry { Type = "uri", Value = "https://san-probe.example.com/x" } + }, + new[] { "{cn}" }), + }; + + string[] productCodes = + (Environment.GetEnvironmentVariable(ProductCodesFlag) ?? DefaultProductCodes) + .Split(',', StringSplitOptions.RemoveEmptyEntries) + .Select(p => p.Trim()) + .Where(p => p.Length > 0) + .ToArray(); + + var results = new List(); + foreach (string productCode in productCodes) + { + bool unavailable = false; + foreach (var (label, sans, csrSans) in variants) + { + var outcome = await ProbeAsync(productCode, label, sans, csrSans); + results.Add(outcome); + + // Don't burn five more orders proving the same product code is not + // enabled on this account. + if (outcome.ProductUnavailable) + { + unavailable = true; + break; + } + + // Throttle: the sandbox rate-limits order bursts (~16 orders / 10 s). + await Task.Delay(1500); + } + + if (unavailable) + _out.WriteLine($"(product {productCode} is not enabled on this account — skipped)"); + } + + _out.WriteLine("=== CERTInext SAN submission probe ==="); + _out.WriteLine($"ProductCodes probed : {string.Join(", ", productCodes)}"); + _out.WriteLine($"(fixture default : {_fixture.ProductCode})"); + _out.WriteLine(""); + + foreach (var group in results.GroupBy(r => r.ProductCode)) + { + _out.WriteLine($"--- ProductCode {group.Key} ---"); + foreach (var r in group) + { + _out.WriteLine($"[{(r.Accepted ? "ACCEPTED" : "REJECTED")}] {r.Label}"); + _out.WriteLine($" requested (additionalDomains): {(r.RequestedDomains.Count > 0 ? string.Join(", ", r.RequestedDomains) : "(field omitted)")}"); + _out.WriteLine($" detail : {r.Detail}"); + _out.WriteLine($" registeredDomains (TrackOrder): {(r.RegisteredDomains.Count > 0 ? string.Join(", ", r.RegisteredDomains) : "(none reported)")}"); + _out.WriteLine(""); + } + } + + _out.WriteLine("=== How to read this ==="); + _out.WriteLine("registeredDomains is TrackOrder's domainVerification key set — the domains"); + _out.WriteLine("CERTInext put on the order. Compare it against 'requested':"); + _out.WriteLine("(1) vs (2): if (1) registers the extra name and (2) does not, then"); + _out.WriteLine(" additionalDomains is required and CSR SANs alone are ignored."); + _out.WriteLine("(3) : whether repeating the CN is rejected or collapsed."); + _out.WriteLine("(4)-(6) : whether non-DNS values are rejected, ignored, or accepted"); + _out.WriteLine(" AT PLACEMENT TIME. An order accepted here can still be"); + _out.WriteLine(" rejected later during validation/approval."); + + // The probe reports; it does not assert a specific CA behaviour, because its purpose + // is to discover what that behaviour is. What must hold is that at least one UCC + // product was actually exercised — otherwise the run proved nothing and should not + // read as a pass. + var usable = results + .Where(r => !r.ProductUnavailable) + .GroupBy(r => r.ProductCode) + .ToList(); + + Skip.If( + usable.Count == 0, + "None of the probed product codes are enabled on this account " + + $"({string.Join(", ", productCodes)}). Set {ProductCodesFlag} to a Multi-Domain (UCC) " + + "code this account can order."); + + foreach (var group in usable) + { + var control = group.First(r => r.Label == "dns-extra-via-additionalDomains"); + Assert.True( + control.Accepted, + $"Positive control failed on product {group.Key} — could not place even a " + + $"plain DNS UCC order: {control.Detail}"); + } + } + } +} diff --git a/CERTInext.Tests/SanSubmissionTests.cs b/CERTInext.Tests/SanSubmissionTests.cs new file mode 100644 index 0000000..84222f1 --- /dev/null +++ b/CERTInext.Tests/SanSubmissionTests.cs @@ -0,0 +1,443 @@ +// Copyright 2026 Keyfactor +// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. +// You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 +// Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions +// and limitations under the License. + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using FluentAssertions; +using Keyfactor.AnyGateway.Extensions; +using Keyfactor.Extensions.CAPlugin.CERTInext.API; +using Keyfactor.Extensions.CAPlugin.CERTInext.Client; +using Moq; +using Org.BouncyCastle.Asn1; +using Org.BouncyCastle.Asn1.Pkcs; +using Org.BouncyCastle.Asn1.X509; +using Org.BouncyCastle.Crypto; +using Org.BouncyCastle.Crypto.Generators; +using Org.BouncyCastle.Pkcs; +using Org.BouncyCastle.Security; +using WireMock.RequestBuilders; +using WireMock.ResponseBuilders; +using WireMock.Server; +using Xunit; + +namespace Keyfactor.Extensions.CAPlugin.CERTInext.Tests +{ + /// + /// Regression tests for UCC SAN submission. + /// + /// The defect these pin down: the AnyCA REST Gateway keys its SAN dictionary + /// dnsname, but MapSanType only recognized dns. Every DNS SAN was + /// therefore typed "dnsname", filtered out by a DNS-only test when building + /// certificateInformation.additionalDomains, and the order reached CERTInext with + /// no additional domains at all — yielding a certificate holding only the CN. Because + /// CERTInext ignores the CSR's subjectAltName extension entirely (measured; see + /// SanSubmissionProbeTests), SANs present on the CSR did not compensate. + /// + /// The end-to-end tests below drive a real against WireMock + /// so they assert on the JSON actually put on the wire, not on an intermediate object. + /// A test that only checked the mapping function would not have caught this bug, since + /// the mapping "worked" — it was the interaction with the downstream filter that lost + /// the names. + /// + public class SanSubmissionTests : IDisposable + { + private readonly WireMockServer _server; + + public SanSubmissionTests() + { + _server = WireMockServer.Start(); + StubHappyEnroll(); + } + + public void Dispose() => _server.Stop(); + + // ----------------------------------------------------------------------- + // Harness + // ----------------------------------------------------------------------- + + private void StubHappyEnroll() + { + _server.Given(Request.Create().WithPath("/GenerateOrderSSL").UsingPost()) + .RespondWith(Response.Create().WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody(MockCertificateData.GenerateOrderSuccessJson(MockCertificateData.OrderNumber1))); + + _server.Given(Request.Create().WithPath("/TrackOrder").UsingPost()) + .RespondWith(Response.Create().WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody(MockCertificateData.TrackOrderIssuedJson(MockCertificateData.OrderNumber1))); + + _server.Given(Request.Create().WithPath("/GetCertificate").UsingPost()) + .RespondWith(Response.Create().WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody(MockCertificateData.GetCertificateSuccessJson())); + } + + private CERTInextClient BuildRealClient() => new CERTInextClient(new CERTInextConfig + { + ApiUrl = _server.Urls[0], + AuthMode = "AccessKey", + ApiKey = "test-key", + AccountNumber = "12345", + RequestorName = "Default Requestor", + RequestorEmail = "default@example.com", + RequestorIsdCode = "1", + RequestorMobileNumber = "5550000000", + SignerPlace = "Austin", + SignerIp = "203.0.113.10", + PageSize = 100 + }); + + /// + /// Plugin wired to a real client pointed at WireMock, so the assertions below run + /// against the JSON actually serialized onto the wire. + /// + private CERTInextCAPlugin BuildPlugin() => new CERTInextCAPlugin(BuildRealClient()); + + private static EnrollmentProductInfo MakeProductInfo(string profileId = "842") => + new EnrollmentProductInfo + { + ProductID = profileId, + ProductParameters = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["ProfileId"] = profileId + } + }; + + /// The orderDetails.certificateInformation block actually POSTed. + private JsonElement CapturedCertificateInformation() + { + var posts = _server.LogEntries + .Where(e => e.RequestMessage.Path == "/GenerateOrderSSL") + .ToList(); + posts.Should().HaveCount(1, "exactly one GenerateOrderSSL POST should have been emitted"); + + string body = posts[0].RequestMessage.Body; + body.Should().NotBeNullOrEmpty(); + + return JsonDocument.Parse(body!).RootElement + .GetProperty("orderDetails") + .GetProperty("certificateInformation"); + } + + private static List AdditionalDomains(JsonElement certificateInformation) => + certificateInformation.TryGetProperty("additionalDomains", out var el) + ? el.EnumerateArray().Select(x => x.GetString()).ToList() + : null; + + // ----------------------------------------------------------------------- + // CSR generation (BouncyCastle — project crypto policy) + // ----------------------------------------------------------------------- + + /// + /// Builds a real PKCS#10 CSR for , optionally carrying a + /// subjectAltName extension holding . + /// + private static string GenerateCsrPem(string cn, params string[] dnsSans) + { + var keyGen = new RsaKeyPairGenerator(); + keyGen.Init(new KeyGenerationParameters(new SecureRandom(), 2048)); + AsymmetricCipherKeyPair kp = keyGen.GenerateKeyPair(); + + Asn1Set attributes = null; + if (dnsSans != null && dnsSans.Length > 0) + { + var names = new GeneralNames( + dnsSans.Select(d => new GeneralName(GeneralName.DnsName, d)).ToArray()); + + var extGen = new X509ExtensionsGenerator(); + extGen.AddExtension(X509Extensions.SubjectAlternativeName, critical: false, extValue: names); + + attributes = new DerSet(new AttributePkcs( + PkcsObjectIdentifiers.Pkcs9AtExtensionRequest, + new DerSet(extGen.Generate()))); + } + + var csr = new Pkcs10CertificationRequest( + "SHA256withRSA", new X509Name($"CN={cn}"), kp.Public, attributes, kp.Private); + + return "-----BEGIN CERTIFICATE REQUEST-----\n" + + Convert.ToBase64String(csr.GetEncoded(), Base64FormattingOptions.InsertLineBreaks) + + "\n-----END CERTIFICATE REQUEST-----"; + } + + // ======================================================================= + // End-to-end: Command's SAN dictionary → the JSON on the wire + // ======================================================================= + + /// + /// THE regression test. "dnsname" is the key the real gateway sends — verified against + /// a customer gateway log: + /// SANs=dnsname:CLAUDIOTEST20.ucsd.edu; dnsname:CLAUDIOTEST20.ad.ucsd.edu + /// Before the fix, additionalDomains was absent from the body entirely. + /// + [Fact] + public async Task GatewayDnsNameKey_ReachesAdditionalDomains() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "host.example.com", "alt.example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + var certInfo = CapturedCertificateInformation(); + certInfo.GetProperty("domainName").GetString().Should().Be("host.example.com"); + + AdditionalDomains(certInfo).Should().BeEquivalentTo(new[] { "alt.example.com" }, + "the extra SAN must reach additionalDomains, and the CN must not be repeated there"); + } + + /// + /// The short "dns" spelling must keep working — some callers and older hosts use it. + /// + [Fact] + public async Task ShortDnsKey_StillReachesAdditionalDomains() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dns"] = new[] { "alt.example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "alt.example.com" }); + } + + /// + /// SANs present only on the CSR must still reach additionalDomains. CERTInext does not + /// read the CSR's SAN extension, so if we don't forward these the names never appear + /// on the certificate. + /// + [Fact] + public async Task CsrSans_ReachAdditionalDomains_WhenGatewaySuppliesNone() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com", "host.example.com", "fromcsr.example.com"), + subject: "CN=host.example.com", + san: null, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "fromcsr.example.com" }); + } + + /// + /// Union, not either/or: names unique to each source survive and the overlap collapses. + /// + [Fact] + public async Task GatewayAndCsrSans_AreUnionedAndDeduplicated() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com", "shared.example.com", "csronly.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + // "shared" appears in both sources, and in different case, to prove the + // de-duplication is case-insensitive. + ["dnsname"] = new[] { "SHARED.example.com", "gatewayonly.example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + var domains = AdditionalDomains(CapturedCertificateInformation()); + + domains.Should().Contain("gatewayonly.example.com"); + domains.Should().Contain("csronly.example.com"); + domains.Count(d => d.Equals("shared.example.com", StringComparison.OrdinalIgnoreCase)) + .Should().Be(1, "the name present in both sources must appear exactly once"); + } + + /// + /// The CN is already submitted as domainName; repeating it in additionalDomains is + /// suppressed. CERTInext collapses it anyway (measured), so this keeps the body matching + /// what we log rather than relying on undocumented CA-side behaviour. + /// + [Fact] + public async Task Cn_IsNotRepeatedInAdditionalDomains() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com", "host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "host.example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + var certInfo = CapturedCertificateInformation(); + certInfo.GetProperty("domainName").GetString().Should().Be("host.example.com"); + AdditionalDomains(certInfo).Should().BeNull( + "with the CN as the only SAN there is nothing left to send, so the field is omitted"); + } + + /// + /// Non-DNS SANs are submitted rather than silently discarded. CERTInext accepts them + /// verbatim (measured) and the resulting order cannot pass validation — a visible + /// failure, deliberately preferred over issuing a certificate that quietly lacks names + /// the subscriber requested. + /// + [Fact] + public async Task NonDnsSans_AreSubmitted_NotDropped() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "alt.example.com" }, + ["ipaddress"] = new[] { "192.0.2.10" }, + ["rfc822name"] = new[] { "admin@example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "alt.example.com", "192.0.2.10", "admin@example.com" }); + } + + /// + /// A CSR we cannot parse must not break enrollment — the gateway-supplied SANs still go. + /// FakeCsrPem is deliberately truncated, so this also guards the many existing + /// tests that pass it. + /// + [Fact] + public async Task UnparseableCsr_DoesNotBlockGatewaySuppliedSans() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: MockCertificateData.FakeCsrPem, + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "alt.example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "alt.example.com" }); + } + + /// + /// No SANs from either source → the field is omitted rather than emitted as null/empty. + /// + [Fact] + public async Task NoSansAnywhere_OmitsAdditionalDomains() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: null, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()).Should().BeNull(); + } + + // ======================================================================= + // Renew path — previously submitted no SANs at all + // ======================================================================= + + /// + /// A renewal that goes through the CERTInext renew API must carry the same domain set + /// as a new enrollment, and must take its primary domain from the subject's CN rather + /// than from the prior order's requestor name. + /// + [Fact] + public async Task RenewalRequest_CarriesSubjectAndSans() + { + var clientMock = new Mock(MockBehavior.Loose); + RenewCertificateRequest captured = null; + + clientMock + .Setup(c => c.RenewCertificateAsync( + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Callback((_, req, __) => captured = req) + .ReturnsAsync(MockCertificateData.IssuedEnrollResponse()); + + var readerMock = new Mock(MockBehavior.Loose); + readerMock + .Setup(r => r.GetRequestIDBySerialNumber(It.IsAny())) + .ReturnsAsync("PRIOR-ORDER-1"); + + // The renewal-window decision reads expiry from the data reader, not from the CA. + // Put the prior cert 10 days out so it lands inside the 30-day window below and the + // renew API path is actually taken. + readerMock + .Setup(r => r.GetExpirationDateByRequestId(It.IsAny())) + .Returns(DateTime.UtcNow.AddDays(10)); + + var plugin = new CERTInextCAPlugin(clientMock.Object, readerMock.Object); + + var productInfo = MakeProductInfo(); + productInfo.ProductParameters["PriorCertSN"] = "AABBCCDDEEFF"; + productInfo.ProductParameters["RenewalWindowDays"] = "30"; + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com", "host.example.com", "alt.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "host.example.com", "alt.example.com" } + }, + productInfo: productInfo, + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.Renew); + + captured.Should().NotBeNull("the renew API path should have been taken"); + + // Bind to a local so the compiler's null-flow analysis is satisfied — a + // FluentAssertions NotBeNull() does not narrow the nullable reference. + RenewCertificateRequest renewReq = captured!; + + renewReq.Subject.Should().Be("CN=host.example.com", + "without the subject the renewal order has no usable primary domain"); + renewReq.Sans.Should().NotBeNull("renewals previously dropped every SAN"); + renewReq.Sans.Select(s => s.Value) + .Should().BeEquivalentTo(new[] { "host.example.com", "alt.example.com" }); + } + } +} diff --git a/CERTInext/API/CertificateRequest.cs b/CERTInext/API/CertificateRequest.cs index 336c8a1..c06046b 100644 --- a/CERTInext/API/CertificateRequest.cs +++ b/CERTInext/API/CertificateRequest.cs @@ -631,6 +631,23 @@ public class RenewCertificateRequest [JsonPropertyName("csr")] public string Csr { get; set; } + /// + /// Distinguished name of the certificate being renewed. Supplies the renewal order's + /// primary domain via its CN — without it the renewal falls back to the prior order's + /// requestor name, which is not a domain at all. + /// + [JsonPropertyName("subject")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public string Subject { get; set; } + + /// + /// SANs to carry onto the renewal order. Renewals previously submitted none, so a + /// renewed UCC certificate came back holding only its primary domain. + /// + [JsonPropertyName("sans")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public System.Collections.Generic.List Sans { get; set; } + [JsonPropertyName("validityDays")] [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] public int? ValidityDays { get; set; } diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index d6b384c..ecd41c1 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -1112,7 +1112,7 @@ private async Task EnrollNewAsync( ValidityYears = ep.ValidityYears > 0 ? ep.ValidityYears : (int?)null, ValidityDays = ep.ValidityDays > 0 ? ep.ValidityDays : (int?)null, Subject = subject, - Sans = BuildSanList(san), + Sans = BuildSanList(san, csr), RequesterName = string.IsNullOrWhiteSpace(ep.RequesterName) ? null : ep.RequesterName, RequesterEmail = string.IsNullOrWhiteSpace(ep.RequesterEmail) ? null : ep.RequesterEmail, KeyType = string.IsNullOrWhiteSpace(ep.KeyType) ? null : ep.KeyType, @@ -1400,6 +1400,11 @@ private async Task RenewOrReissueAsync( var renewReq = new RenewCertificateRequest { Csr = csr, + // Renewals go out as a fresh CERTInext order, so they need the same domain + // set as a new enrollment — otherwise a renewed UCC certificate comes back + // holding only its primary domain. + Subject = subject, + Sans = BuildSanList(san, csr), ValidityDays = ep.ValidityDays > 0 ? ep.ValidityDays : (int?)null, RequesterName = string.IsNullOrWhiteSpace(ep.RequesterName) ? null : ep.RequesterName, RequesterEmail = string.IsNullOrWhiteSpace(ep.RequesterEmail) ? null : ep.RequesterEmail, @@ -2114,46 +2119,242 @@ private static int MapRevocationReasonStringToCode(string reason) } /// - /// Converts the multi-valued SAN dictionary from the AnyCA gateway into the - /// list expected by the CERTInext API. + /// Builds the list submitted to CERTInext, from the union of + /// two sources: the multi-valued SAN dictionary the AnyCA gateway hands us, and the + /// subjectAltName extension carried inside the CSR itself. + /// + /// Both sources are needed. The gateway dictionary is authoritative when Command + /// populates it, but not every enrollment path does — and the CSR is the only place + /// the requested names are guaranteed to appear, because the client built it. Union + /// + de-duplicate rather than preferring one, so a name requested in either place + /// reaches the order. + /// + /// Parsing the CSR here is not redundant with sending the CSR to CERTInext. + /// CERTInext ignores the CSR's subjectAltName extension outright — measured in + /// SanSubmissionProbeTests: a CSR carrying two DNS names, submitted with + /// additionalDomains omitted, produced an order with only the CN registered. + /// Re-submitting the CSR's names through additionalDomains is the only way a + /// SAN that exists solely in the CSR reaches the issued certificate. + /// + /// History (UCC SANs silently dropped): the gateway keys this dictionary + /// dnsname, not dns. did not recognize + /// dnsname, so every DNS SAN was typed "dnsname", filtered out by the + /// DNS-only test in BuildAdditionalDomains, and the order went to CERTInext + /// with no additionalDomains at all. The certificate came back holding only + /// the CN, which reads as the CA stripping SANs supplied on the CSR. /// - private static List BuildSanList(Dictionary san) + private List BuildSanList(Dictionary san, string csr) { - if (san == null || san.Count == 0) - return null; - var result = new List(); + // Type+value identity, so the same name requested as two different SAN types is + // preserved while an exact repeat across the two sources collapses. + var seen = new HashSet(StringComparer.OrdinalIgnoreCase); - // AnyCA passes SANs keyed by type name (e.g. "Dns", "Ip", "Email", "Uri") - foreach (var kvp in san) + void Add(string type, string value) { - string sanType = MapSanType(kvp.Key); - if (kvp.Value == null) continue; + if (string.IsNullOrWhiteSpace(value)) return; + string trimmed = value.Trim(); + if (!seen.Add($"{type}|{trimmed}")) return; + result.Add(new SanEntry { Type = type, Value = trimmed }); + } - foreach (string value in kvp.Value) + // AnyCA passes SANs keyed by type name — the real gateway uses "dnsname", + // "rfc822name", "ipaddress"; MapSanType normalizes the spelling variants. + if (san != null) + { + foreach (var kvp in san) { - if (!string.IsNullOrWhiteSpace(value)) - result.Add(new SanEntry { Type = sanType, Value = value.Trim() }); + string sanType = MapSanType(kvp.Key); + if (kvp.Value == null) continue; + + foreach (string value in kvp.Value) + Add(sanType, value); } } - return result.Count > 0 ? result : null; + int fromGateway = result.Count; + + // Union in whatever the CSR asked for. Non-throwing: a malformed or unparseable + // CSR yields an empty list and leaves the gateway-supplied set untouched. + foreach (var csrSan in ExtractSanEntriesFromCsr(csr)) + Add(csrSan.Type, csrSan.Value); + + int fromCsrOnly = result.Count - fromGateway; + + if (result.Count == 0) + { + _logger.LogDebug( + "No SANs supplied by the gateway and none found in the CSR — submitting the order with domainName only."); + return null; + } + + // The blind spot that hid the original defect was that nothing logged what we + // resolved. Log the full resolved set and its provenance at Information. + _logger.LogInformation( + "Resolved {Total} SAN(s) for submission. FromGatewayRequest={FromGateway}, " + + "AddedFromCsr={FromCsr}, Sans={Sans}", + result.Count, fromGateway, fromCsrOnly, + string.Join("; ", result.Select(s => $"{s.Type}:{s.Value}"))); + + if (fromCsrOnly > 0) + { + // Worth a Warning, not Debug: it means Command did not hand us names the + // client actually requested, which is a gateway/template wiring smell even + // though we recover from it here. + _logger.LogWarning( + "{Count} SAN(s) were present in the CSR but absent from the SAN data supplied by Command; " + + "they have been added to the order. Review the enrollment pattern / template SAN configuration.", + fromCsrOnly); + } + + // CERTInext's certificateInformation.additionalDomains is a domain-name field, and + // non-DNS SANs are submitted into it deliberately rather than discarded: dropping + // them would issue a certificate silently missing names the subscriber asked for, + // which is the worse failure. + // + // Measured behaviour (SanSubmissionProbeTests, product 844, 2026-08-12): CERTInext + // does NOT reject these at order placement. It accepts the order and registers the + // value verbatim as an order domain — an email address, an IP literal and a URI all + // came back as domainVerification keys. The order then cannot pass domain validation, + // so it parks pending instead of failing fast. Say that plainly, because "the + // enrollment did not error but the order will never issue" is the confusing case. + var nonDns = result.Where(s => !string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)).ToList(); + if (nonDns.Count > 0) + { + _logger.LogWarning( + "{Count} requested SAN(s) are not DNS names: {Sans}. CERTInext's additionalDomains " + + "field takes domain names, and it accepts these verbatim rather than rejecting them — " + + "the order will be created but is not expected to pass domain validation, so it will " + + "sit pending rather than issue. They are submitted rather than dropped on purpose: a " + + "visibly stuck order is preferable to a certificate issued without names the subscriber " + + "requested. Remove them from the CSR or the enrollment pattern if the order should proceed.", + nonDns.Count, string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}"))); + } + + return result; } private static string MapSanType(string anyCAType) { switch (anyCAType?.ToLowerInvariant()) { - case "dns": return "dns"; + // "dnsname" is what the AnyCA REST Gateway actually sends; "dns"/"dnsnames" + // are kept for callers and older hosts that use the shorter spelling. + case "dns": + case "dnsname": + case "dnsnames": return "dns"; case "ip": - case "ipaddress": return "ip"; + case "ipaddress": + case "ipaddresses": return "ip"; case "email": - case "rfc822": return "email"; - case "uri": return "uri"; + case "rfc822": + case "rfc822name": return "email"; + case "uri": + case "uniformresourceidentifier": return "uri"; default: return anyCAType?.ToLowerInvariant() ?? "dns"; } } + /// + /// Extracts the subjectAltName entries from a PEM-encoded PKCS#10 CSR. + /// + /// Implemented with BouncyCastle (per the project's crypto policy: all certificate + /// and key handling goes through BouncyCastle, never BCL System.Security.Cryptography). + /// Never throws — an absent, truncated, or otherwise unparseable CSR returns an empty + /// list so enrollment continues on the gateway-supplied SAN data alone. + /// + private static List ExtractSanEntriesFromCsr(string csrPem) + { + var result = new List(); + if (string.IsNullOrWhiteSpace(csrPem)) + return result; + + try + { + string b64 = csrPem + .Replace("-----BEGIN CERTIFICATE REQUEST-----", string.Empty) + .Replace("-----END CERTIFICATE REQUEST-----", string.Empty) + .Replace("-----BEGIN NEW CERTIFICATE REQUEST-----", string.Empty) + .Replace("-----END NEW CERTIFICATE REQUEST-----", string.Empty) + .Replace("\r", string.Empty) + .Replace("\n", string.Empty) + .Trim(); + + if (string.IsNullOrWhiteSpace(b64)) + return result; + + var csr = new Org.BouncyCastle.Pkcs.Pkcs10CertificationRequest(Convert.FromBase64String(b64)); + + // SANs live in the PKCS#9 extensionRequest attribute, not the CSR body. + var extensions = csr.GetRequestedExtensions(); + var sanExtension = extensions?.GetExtension( + Org.BouncyCastle.Asn1.X509.X509Extensions.SubjectAlternativeName); + if (sanExtension == null) + return result; + + var names = Org.BouncyCastle.Asn1.X509.GeneralNames.GetInstance(sanExtension.GetParsedValue()); + foreach (var generalName in names.GetNames()) + { + string value = GeneralNameToValue(generalName); + if (!string.IsNullOrWhiteSpace(value)) + result.Add(new SanEntry { Type = SanTypeFromGeneralNameTag(generalName.TagNo), Value = value }); + } + } + catch (Exception ex) + { + // Enrollment must not fail because we could not read the CSR's SANs — the + // gateway-supplied set still applies, and CERTInext validates the CSR itself. + // Debug so an operator diagnosing a missing SAN can see the parse was skipped. + LogHandler.GetClassLogger(typeof(CERTInextCAPlugin)) + .LogDebug(ex, "ExtractSanEntriesFromCsr suppressed CSR parse failure"); + } + + return result; + } + + /// Maps an ASN.1 GeneralName tag to the SAN type string used by this plugin. + private static string SanTypeFromGeneralNameTag(int tagNo) + { + switch (tagNo) + { + case Org.BouncyCastle.Asn1.X509.GeneralName.DnsName: return "dns"; + case Org.BouncyCastle.Asn1.X509.GeneralName.Rfc822Name: return "email"; + case Org.BouncyCastle.Asn1.X509.GeneralName.UniformResourceIdentifier: return "uri"; + case Org.BouncyCastle.Asn1.X509.GeneralName.IPAddress: return "ip"; + case Org.BouncyCastle.Asn1.X509.GeneralName.DirectoryName: return "directoryname"; + case Org.BouncyCastle.Asn1.X509.GeneralName.RegisteredID: return "registeredid"; + case Org.BouncyCastle.Asn1.X509.GeneralName.OtherName: return "othername"; + default: return $"generalname-{tagNo}"; + } + } + + /// + /// Renders a GeneralName's value as the text CERTInext would need to see. Returns null + /// for a name whose value cannot be rendered meaningfully, so it is skipped rather than + /// submitted as ASN.1 debris. + /// + private static string GeneralNameToValue(Org.BouncyCastle.Asn1.X509.GeneralName generalName) + { + switch (generalName.TagNo) + { + case Org.BouncyCastle.Asn1.X509.GeneralName.DnsName: + case Org.BouncyCastle.Asn1.X509.GeneralName.Rfc822Name: + case Org.BouncyCastle.Asn1.X509.GeneralName.UniformResourceIdentifier: + return Org.BouncyCastle.Asn1.DerIA5String.GetInstance(generalName.Name).GetString(); + + case Org.BouncyCastle.Asn1.X509.GeneralName.IPAddress: + // Octet string → dotted-quad / RFC 5952 text, so what we submit and log is + // the address the subscriber asked for rather than its hex encoding. + byte[] octets = Org.BouncyCastle.Asn1.Asn1OctetString.GetInstance(generalName.Name).GetOctets(); + return octets.Length == 4 || octets.Length == 16 + ? new System.Net.IPAddress(octets).ToString() + : null; + + default: + return generalName.Name?.ToString(); + } + } + private static string GetStringValue( Dictionary dict, string key, string defaultValue = "") { diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index 2c604aa..d85e694 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -186,9 +186,20 @@ public async Task PlaceOrderAsync( if (request.Meta == null) request.Meta = await BuildMetaAsync(ct); + // The domain set is logged here, at the wire, not just where Command hands it to us. + // A UCC order that silently lost its SANs upstream of this point is otherwise + // indistinguishable in the gateway log from one the CA stripped — reconciling the + // enrollment-start "SANs=" line against this one localizes the loss immediately. + var certInfo = request.OrderDetails?.CertificateInformation; Logger.LogInformation( - "Submitting order to CERTInext. ProductCode={ProductCode}", - request.OrderDetails?.ProductCode); + "Submitting order to CERTInext. ProductCode={ProductCode}, DomainName={DomainName}, " + + "AdditionalDomainCount={AdditionalDomainCount}, AdditionalDomains={AdditionalDomains}", + request.OrderDetails?.ProductCode, + certInfo?.DomainName, + certInfo?.AdditionalDomains?.Count ?? 0, + certInfo?.AdditionalDomains != null && certInfo.AdditionalDomains.Count > 0 + ? string.Join("; ", certInfo.AdditionalDomains) + : "(none)"); GenerateOrderResponse result = null; RestResponse resp = null; @@ -710,6 +721,23 @@ public async Task RenewCertificateAsync( throw new KeyNotFoundException($"Cannot renew: prior order '{certificateId}' was not found in CERTInext."); } + // Primary domain for the renewal order. Prefer the CN of the subject Command gave + // us; the prior order's requestorName is only a last resort and is not a domain — + // it is retained solely so an old caller that sets no Subject behaves as before. + string renewalDomainName = + ExtractCnFromSubject(request.Subject) + ?? priorTrack.OrderDetails?.RequestorInformation?.RequestorName + ?? "unknown"; + + if (ExtractCnFromSubject(request.Subject) == null) + { + Logger.LogWarning( + "Renewal of order {PriorId} has no usable CN in its subject; falling back to " + + "DomainName='{DomainName}' from the prior order. Verify the renewed certificate's " + + "primary domain.", + certificateId, renewalDomainName); + } + // We don't have the product code from TrackOrder — build an order using // the config defaults and the CSR from the renewal request. var orderReq = new GenerateOrderSslRequest @@ -729,7 +757,8 @@ public async Task RenewCertificateAsync( SubscriptionDetails = new SubscriptionDetails { Validity = "1" }, CertificateInformation = new CertificateInformation { - DomainName = priorTrack.OrderDetails?.RequestorInformation?.RequestorName ?? "unknown" + DomainName = renewalDomainName, + AdditionalDomains = BuildAdditionalDomains(request.Sans, renewalDomainName) }, Csr = request.Csr, AgreementDetails = BuildDefaultAgreementDetails() @@ -1337,6 +1366,10 @@ private GenerateOrderSslRequest BuildOrderRequestFromLegacyEnrollRequest(EnrollC string requestorIsd = string.IsNullOrWhiteSpace(_config.RequestorIsdCode) ? "1" : _config.RequestorIsdCode; string requestorMobile = _config.RequestorMobileNumber ?? string.Empty; + // Hoisted: additionalDomains is de-duplicated against the primary domain, so both + // fields have to be built from the same value. + string domainName = ExtractCnFromSubject(request.Subject) ?? "unknown"; + return new GenerateOrderSslRequest { // Meta will be set by PlaceOrderAsync @@ -1382,8 +1415,8 @@ private GenerateOrderSslRequest BuildOrderRequestFromLegacyEnrollRequest(EnrollC }, CertificateInformation = new CertificateInformation { - DomainName = ExtractCnFromSubject(request.Subject) ?? "unknown", - AdditionalDomains = BuildAdditionalDomains(request.Sans), + DomainName = domainName, + AdditionalDomains = BuildAdditionalDomains(request.Sans, domainName), AutoSecureWww = string.IsNullOrWhiteSpace(_config.AutoSecureWww) ? "0" : _config.AutoSecureWww }, @@ -1445,16 +1478,56 @@ private static string ExtractCnFromSubject(string subject) return null; } - private static List BuildAdditionalDomains(System.Collections.Generic.List sans) + /// + /// Projects the resolved SAN list onto certificateInformation.additionalDomains. + /// + /// Every requested SAN is submitted regardless of type. Filtering to DNS-only (the + /// original behaviour) issued certificates quietly missing names the subscriber had + /// requested, which is the worse failure; the caller warns about the non-DNS entries + /// before we get here. + /// + /// is the value already going out as the order's primary + /// domain, and Command normally includes the CN in the SAN set as well. CERTInext was + /// measured to collapse that repetition itself (SanSubmissionProbeTests: CN submitted + /// twice came back registered once), so excluding it here is defence in depth rather + /// than a correctness requirement — it keeps the submitted body matching what we log + /// and avoids depending on undocumented CA-side de-duplication. + /// + private List BuildAdditionalDomains( + System.Collections.Generic.List sans, + string domainName) { if (sans == null || sans.Count == 0) return null; + var domains = new List(); + var seen = new HashSet(StringComparer.OrdinalIgnoreCase); + + bool haveDomainName = !string.IsNullOrWhiteSpace(domainName); + if (haveDomainName) + seen.Add(domainName.Trim()); + + int duplicates = 0; foreach (var san in sans) { - if (string.Equals(san.Type, "dns", StringComparison.OrdinalIgnoreCase) && - !string.IsNullOrWhiteSpace(san.Value)) - domains.Add(san.Value); + if (san == null || string.IsNullOrWhiteSpace(san.Value)) continue; + + string value = san.Value.Trim(); + if (!seen.Add(value)) + { + duplicates++; + continue; + } + domains.Add(value); + } + + if (duplicates > 0) + { + Logger.LogDebug( + "Collapsed {Count} duplicate SAN value(s) out of additionalDomains " + + "(already submitted as domainName '{DomainName}', or repeated in the SAN set).", + duplicates, domainName); } + return domains.Count > 0 ? domains : null; } From 281c624f8396deeaf4e0347f45f1b1ab563b348a Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Wed, 12 Aug 2026 08:58:30 -0700 Subject: [PATCH 2/2] docs(enroll): scope the CERTInext SAN measurements to the sandbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../SanSubmissionProbeTests.cs | 12 +++++- CERTInext/CERTInextCAPlugin.cs | 40 +++++++++++-------- CERTInext/Client/CERTInextClient.cs | 11 ++--- 3 files changed, 40 insertions(+), 23 deletions(-) diff --git a/CERTInext.IntegrationTests/SanSubmissionProbeTests.cs b/CERTInext.IntegrationTests/SanSubmissionProbeTests.cs index 32df577..23681d1 100644 --- a/CERTInext.IntegrationTests/SanSubmissionProbeTests.cs +++ b/CERTInext.IntegrationTests/SanSubmissionProbeTests.cs @@ -19,8 +19,16 @@ // and issuance to complete. // // --------------------------------------------------------------------------------------- -// MEASURED RESULTS — sandbox-us, account 4951571271, product 844 (OV SSL UCC), 2026-08-12. -// (Product 840 / DV UCC is not enabled on that account: "Invalid Product Code".) +// MEASURED RESULTS — SANDBOX ONLY: sandbox-us, account 4951571271, product 844 (OV SSL UCC), +// 2026-08-12. (Product 840 / DV UCC is not enabled on that account: "Invalid Product Code".) +// +// These are sandbox observations. Re-run against production before treating B or C as +// settled there — point ~/.env_certinext at the production account and set +// CERTINEXT_SAN_PROBE_PRODUCTS to a UCC code that account can actually order (product +// numbering is per-account; the codes in Constants.Products are defaults, not guarantees). +// Finding A and the CSR-SAN result below are separately corroborated by production: the +// customer report that prompted this work was a production UCC order whose CSR carried the +// SANs and whose issued certificate held only the CN. // // A. CONFIRMED. additionalDomains is what puts extra names on the order. Submitting // CN + extra1. registered BOTH domains. diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index ecd41c1..eabd07b 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -2130,11 +2130,14 @@ private static int MapRevocationReasonStringToCode(string reason) /// reaches the order. /// /// Parsing the CSR here is not redundant with sending the CSR to CERTInext. - /// CERTInext ignores the CSR's subjectAltName extension outright — measured in - /// SanSubmissionProbeTests: a CSR carrying two DNS names, submitted with - /// additionalDomains omitted, produced an order with only the CN registered. - /// Re-submitting the CSR's names through additionalDomains is the only way a - /// SAN that exists solely in the CSR reaches the issued certificate. + /// CERTInext ignores the CSR's subjectAltName extension outright — measured on the + /// US sandbox in SanSubmissionProbeTests: a CSR carrying two DNS names, + /// submitted with additionalDomains omitted, produced an order with only the + /// CN registered. Production behaves the same way: the customer report that prompted + /// this fix was a production UCC order whose CSR carried the SANs and whose issued + /// certificate held only the CN. Re-submitting the CSR's names through + /// additionalDomains is the only way a SAN that exists solely in the CSR + /// reaches the issued certificate. /// /// History (UCC SANs silently dropped): the gateway keys this dictionary /// dnsname, not dns. did not recognize @@ -2212,22 +2215,27 @@ void Add(string type, string value) // them would issue a certificate silently missing names the subscriber asked for, // which is the worse failure. // - // Measured behaviour (SanSubmissionProbeTests, product 844, 2026-08-12): CERTInext - // does NOT reject these at order placement. It accepts the order and registers the - // value verbatim as an order domain — an email address, an IP literal and a URI all - // came back as domainVerification keys. The order then cannot pass domain validation, - // so it parks pending instead of failing fast. Say that plainly, because "the - // enrollment did not error but the order will never issue" is the confusing case. + // Measured on the US SANDBOX only (SanSubmissionProbeTests, product 844, + // 2026-08-12): CERTInext did NOT reject these at order placement. It accepted the + // order and registered the value verbatim as an order domain — an email address, an + // IP literal and a URI all came back as domainVerification keys. The order then + // cannot pass domain validation, so it parks pending instead of failing fast. + // + // Production is UNVERIFIED for this case and may reject the order outright instead. + // The warning below therefore describes the sandbox outcome as the expected one + // without promising it: either way the operator is told which SANs are the problem, + // which is the part that matters for diagnosis. var nonDns = result.Where(s => !string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)).ToList(); if (nonDns.Count > 0) { _logger.LogWarning( "{Count} requested SAN(s) are not DNS names: {Sans}. CERTInext's additionalDomains " + - "field takes domain names, and it accepts these verbatim rather than rejecting them — " + - "the order will be created but is not expected to pass domain validation, so it will " + - "sit pending rather than issue. They are submitted rather than dropped on purpose: a " + - "visibly stuck order is preferable to a certificate issued without names the subscriber " + - "requested. Remove them from the CSR or the enrollment pattern if the order should proceed.", + "field takes domain names, so this order will either be rejected outright or be " + + "created and then fail domain validation and sit pending — on the US sandbox it was " + + "accepted verbatim and parked pending. They are submitted rather than dropped on " + + "purpose: a visible failure is preferable to a certificate issued without names the " + + "subscriber requested. Remove them from the CSR or the enrollment pattern if the " + + "order should proceed.", nonDns.Count, string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}"))); } diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index d85e694..4551529 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -1487,11 +1487,12 @@ private static string ExtractCnFromSubject(string subject) /// before we get here. /// /// is the value already going out as the order's primary - /// domain, and Command normally includes the CN in the SAN set as well. CERTInext was - /// measured to collapse that repetition itself (SanSubmissionProbeTests: CN submitted - /// twice came back registered once), so excluding it here is defence in depth rather - /// than a correctness requirement — it keeps the submitted body matching what we log - /// and avoids depending on undocumented CA-side de-duplication. + /// domain, and Command normally includes the CN in the SAN set as well. On the US + /// sandbox CERTInext was measured to collapse that repetition itself + /// (SanSubmissionProbeTests: CN submitted twice came back registered once), but that is + /// undocumented and unverified against production — which is exactly why we exclude it + /// here rather than relying on CA-side de-duplication. It also keeps the submitted body + /// matching what we log. /// private List BuildAdditionalDomains( System.Collections.Generic.List sans,