From dfbfe80cfd9d12cd3df555cbef11cb7361f1eeb3 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 01/14] =?UTF-8?q?fix(enroll):=20UCC=20SANs=20never=20reach?= =?UTF-8?q?ed=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. --- .../SanSubmissionProbeTests.cs | 383 +++++++++++++++ CERTInext.Tests/SanSubmissionTests.cs | 446 ++++++++++++++++++ CERTInext/API/CertificateRequest.cs | 17 + CERTInext/CERTInextCAPlugin.cs | 239 +++++++++- CERTInext/Client/CERTInextClient.cs | 91 +++- 5 files changed, 1148 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..14f00f3 --- /dev/null +++ b/CERTInext.Tests/SanSubmissionTests.cs @@ -0,0 +1,446 @@ +// 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. PickupRetries = 0 is set on + /// the plugin's own config (not the client's) — that is where the synchronous-pickup + /// budget is read, and leaving it at the default would make every test here sit in a + /// polling loop. + /// + private CERTInextCAPlugin BuildPlugin() => + new CERTInextCAPlugin(BuildRealClient(), new CERTInextConfig { PickupRetries = 0 }); + + private static EnrollmentProductInfo MakeProductInfo(string profileId = "842") => + new EnrollmentProductInfo + { + ProductID = profileId, + ProductParameters = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["ProfileId"] = profileId + } + }; + + /// The orderDetails.certificateInformation block actually POSTed. + private JsonElement CapturedCertificateInformation() + { + var posts = _server.LogEntries + .Where(e => e.RequestMessage.Path == "/GenerateOrderSSL") + .ToList(); + posts.Should().HaveCount(1, "exactly one GenerateOrderSSL POST should have been emitted"); + + string body = posts[0].RequestMessage.Body; + body.Should().NotBeNullOrEmpty(); + + return JsonDocument.Parse(body!).RootElement + .GetProperty("orderDetails") + .GetProperty("certificateInformation"); + } + + private static List AdditionalDomains(JsonElement certificateInformation) => + certificateInformation.TryGetProperty("additionalDomains", out var el) + ? el.EnumerateArray().Select(x => x.GetString()).ToList() + : null; + + // ----------------------------------------------------------------------- + // CSR generation (BouncyCastle — project crypto policy) + // ----------------------------------------------------------------------- + + /// + /// Builds a real PKCS#10 CSR for , 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 7f02df0..043b4b5 100644 --- a/CERTInext/API/CertificateRequest.cs +++ b/CERTInext/API/CertificateRequest.cs @@ -622,6 +622,23 @@ public class RenewCertificateRequest [JsonPropertyName("csr")] public string Csr { get; set; } + /// + /// Distinguished name of the certificate being renewed. Supplies the renewal order's + /// primary domain via its CN — without it the renewal falls back to the prior order's + /// requestor name, which is not a domain at all. + /// + [JsonPropertyName("subject")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public string Subject { get; set; } + + /// + /// SANs to carry onto the renewal order. Renewals previously submitted none, so a + /// renewed UCC certificate came back holding only its primary domain. + /// + [JsonPropertyName("sans")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public System.Collections.Generic.List Sans { get; set; } + [JsonPropertyName("validityDays")] [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] public int? ValidityDays { get; set; } diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index b04c051..a0481cd 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -1096,7 +1096,7 @@ private async Task EnrollNewAsync( Csr = csr, 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, @@ -1313,6 +1313,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, @@ -2183,46 +2188,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 c6ad56b..8b46ad8 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; @@ -775,6 +786,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 @@ -794,7 +822,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() @@ -1398,6 +1427,10 @@ private GenerateOrderSslRequest BuildOrderRequestFromLegacyEnrollRequest(EnrollC string requestorIsd = string.IsNullOrWhiteSpace(_config.RequestorIsdCode) ? "1" : _config.RequestorIsdCode; string requestorMobile = _config.RequestorMobileNumber ?? string.Empty; + // Hoisted: additionalDomains is de-duplicated against the primary domain, so both + // fields have to be built from the same value. + string domainName = ExtractCnFromSubject(request.Subject) ?? "unknown"; + return new GenerateOrderSslRequest { // Meta will be set by PlaceOrderAsync @@ -1443,8 +1476,8 @@ private GenerateOrderSslRequest BuildOrderRequestFromLegacyEnrollRequest(EnrollC }, CertificateInformation = new CertificateInformation { - DomainName = ExtractCnFromSubject(request.Subject) ?? "unknown", - AdditionalDomains = BuildAdditionalDomains(request.Sans), + DomainName = domainName, + AdditionalDomains = BuildAdditionalDomains(request.Sans, domainName), AutoSecureWww = string.IsNullOrWhiteSpace(_config.AutoSecureWww) ? "0" : _config.AutoSecureWww }, @@ -1506,16 +1539,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 5348f728a1cceb3a7b7bcf8d6ac8d5255acb4a70 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 02/14] 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 a0481cd..4796d10 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -2199,11 +2199,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 @@ -2281,22 +2284,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 8b46ad8..aa98d0c 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -1548,11 +1548,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, From f6fbc80129e8bad4776fcb717061eb0d0e5f6c72 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Wed, 12 Aug 2026 09:27:06 -0700 Subject: [PATCH 03/14] =?UTF-8?q?fix(enroll):=20address=20full-review=20ro?= =?UTF-8?q?und=201=20=E2=80=94=20DCV=20strand,=20ASN.1=20debris,=20log=20i?= =?UTF-8?q?njection?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CERTInext.Tests/CERTInextCAPluginDcvTests.cs | 153 +++++++++++++++ CERTInext.Tests/SanSubmissionTests.cs | 177 +++++++++++++++++ CERTInext/CERTInextCAPlugin.cs | 190 ++++++++++++++++--- CERTInext/CERTInextCAPluginConfig.cs | 30 +++ CERTInext/Client/CERTInextClient.cs | 31 ++- CERTInext/Constants.cs | 1 + CHANGELOG.md | 6 + 7 files changed, 560 insertions(+), 28 deletions(-) diff --git a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs index d812074..7861618 100644 --- a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs @@ -824,5 +824,158 @@ public async Task Dcv_WaitsForIssuance_AfterDcvVerifies() mock.Verify(c => c.GetCertificateAsync(MockCertificateData.DcvOrderId, It.IsAny()), Times.AtLeast(2), "plugin should have polled at least twice for issuance"); } + + // --------------------------------------------------------------------------- + // Undrainable pending domains must not strand the valid ones on the same order + // --------------------------------------------------------------------------- + + /// + /// Builds a TrackOrder response whose domainVerification block lists several pending + /// domains, so tests can mix validatable and unvalidatable keys on one order. + /// + private static TrackOrderResponse DcvPendingTrackResponseMultiDomain( + string orderNumber, params string[] domains) + { + var detail = System.Text.Json.JsonSerializer.SerializeToElement(new DomainVerificationDetail + { + DcvMethod = Constants.Dcv.MethodDnsTxt, + DcvStatus = Constants.Dcv.StatusPending, + Status = "1" + }); + + var raw = new Dictionary(); + foreach (string d in domains) + raw[d] = detail; + + return new TrackOrderResponse + { + OrderDetails = new TrackOrderResponseDetails + { + OrderStatusId = "1", + CertificateStatusId = "1", + DomainVerification = new TrackOrderDomainVerification + { + Status = Constants.Dcv.StatusPending, + RawDomainEntries = raw + } + } + }; + } + + /// + /// A non-FQDN pending domain must be skipped, not thrown on. + /// + /// Regression: non-DNS SANs are now submitted to CERTInext, which registers them verbatim + /// as order domains, so an email/URI SAN turns up as a domainVerification key that fails the + /// FQDN check. That check used to throw for the whole order — escaping Enroll (which has no + /// catch) after the order was already placed, so the enrollment failed with an orphaned + /// order and no TXT record was staged for the *valid* domains beside it. Every sync retry + /// re-threw and TryRunDcvDuringSyncAsync swallowed it, so the order could never progress. + /// + [Fact] + public async Task Dcv_NonFqdnPendingDomain_IsSkipped_AndValidDomainStillStaged() + { + const string order = MockCertificateData.DcvOrderId; + const string good = MockCertificateData.DcvDomain; + const string bad = "admin@example.com"; // what an rfc822 SAN comes back as + + var mock = NewMock(); + + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); + + mock.SetupSequence(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, good, bad)) + .ReturnsAsync(MockCertificateData.DcvVerifiedTrackResponse(order, good)); + + // Only the valid domain should ever reach GetDcv/VerifyDcv. MockBehavior.Strict means + // an unexpected call for `bad` fails the test on its own. + mock.Setup(c => c.GetDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse(MockCertificateData.DcvToken)); + mock.Setup(c => c.VerifyDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .Returns(Task.CompletedTask); + mock.Setup(c => c.GetCertificateAsync(order, It.IsAny())) + .ReturnsAsync(MockCertificateData.IssuedCertRecord(order)); + + var validator = new FakeDomainValidator(); + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator), + DcvConfig(dcvWaitForIssuanceSeconds: 10)); + + // Must not throw — that is the regression. + var result = await Enroll(plugin); + + string expectedHostname = string.Format(Constants.Dcv.DefaultTxtRecordTemplate, good); + validator.StagedRecords.Should().ContainSingle( + "the valid DNS domain must still be staged even though a co-tenant domain is unusable") + .Which.Should().Be((expectedHostname, MockCertificateData.DcvToken)); + + mock.Verify(c => c.GetDcvAsync(order, bad, It.IsAny(), It.IsAny()), + Times.Never, "a non-FQDN domain must never be sent to GetDcv"); + result.Status.Should().Be((int)EndEntityStatus.GENERATED); + } + + /// + /// A pending domain that resolves no DNS provider (an IP-literal SAN passes the FQDN regex + /// but no zone can match it) must likewise be skipped rather than failing the whole order. + /// + [Fact] + public async Task Dcv_DomainWithNoResolvableValidator_IsSkipped_AndValidDomainStillStaged() + { + const string order = MockCertificateData.DcvOrderId; + const string good = MockCertificateData.DcvDomain; + const string ip = "192.0.2.10"; // what an iPAddress SAN comes back as + + var mock = NewMock(); + + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); + + mock.SetupSequence(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, good, ip)) + .ReturnsAsync(MockCertificateData.DcvVerifiedTrackResponse(order, good)); + + // The IP literal clears the FQDN filter, so GetDcv IS called for it; the dead end is + // that no validator resolves. Stub it so reaching that point is legitimate. + mock.Setup(c => c.GetDcvAsync(order, It.IsAny(), Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse(MockCertificateData.DcvToken)); + mock.Setup(c => c.VerifyDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .Returns(Task.CompletedTask); + mock.Setup(c => c.GetCertificateAsync(order, It.IsAny())) + .ReturnsAsync(MockCertificateData.IssuedCertRecord(order)); + + var validator = new FakeDomainValidator(); + var plugin = BuildPlugin( + mock.Object, + new SelectiveDomainValidatorFactory(validator, resolvableDomain: good), + DcvConfig(dcvWaitForIssuanceSeconds: 10)); + + var result = await Enroll(plugin); + + string expectedHostname = string.Format(Constants.Dcv.DefaultTxtRecordTemplate, good); + validator.StagedRecords.Should().ContainSingle( + "only the domain with a resolvable provider should be staged, and it must still be staged") + .Which.Should().Be((expectedHostname, MockCertificateData.DcvToken)); + result.Status.Should().Be((int)EndEntityStatus.GENERATED); + } + + /// Factory that resolves a validator for exactly one domain and null for all others. + private sealed class SelectiveDomainValidatorFactory : IDomainValidatorFactory + { + private readonly IDomainValidator _validator; + private readonly string _resolvableDomain; + + public SelectiveDomainValidatorFactory(IDomainValidator validator, string resolvableDomain) + { + _validator = validator; + _resolvableDomain = resolvableDomain; + } + + public IDomainValidator ResolveDomainValidator(string domain, string validationType) => + string.Equals(domain, _resolvableDomain, StringComparison.OrdinalIgnoreCase) ? _validator : null; + + public IDomainValidator PrimaryValidator => _validator; + } } } diff --git a/CERTInext.Tests/SanSubmissionTests.cs b/CERTInext.Tests/SanSubmissionTests.cs index 14f00f3..fafb247 100644 --- a/CERTInext.Tests/SanSubmissionTests.cs +++ b/CERTInext.Tests/SanSubmissionTests.cs @@ -8,6 +8,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Reflection; using System.Text.Json; using System.Threading; using System.Threading.Tasks; @@ -140,6 +141,33 @@ private static List AdditionalDomains(JsonElement certificateInformation // CSR generation (BouncyCastle — project crypto policy) // ----------------------------------------------------------------------- + /// + /// Builds a real PKCS#10 CSR for carrying arbitrary + /// in its subjectAltName extension — used to exercise + /// GeneralName types that have no domain-name rendering. + /// + private static string GenerateCsrPemWithGeneralNames(string cn, params GeneralName[] names) + { + var keyGen = new RsaKeyPairGenerator(); + keyGen.Init(new KeyGenerationParameters(new SecureRandom(), 2048)); + AsymmetricCipherKeyPair kp = keyGen.GenerateKeyPair(); + + var extGen = new X509ExtensionsGenerator(); + extGen.AddExtension(X509Extensions.SubjectAlternativeName, critical: false, + extValue: new GeneralNames(names)); + + var attributes = new DerSet(new AttributePkcs( + PkcsObjectIdentifiers.Pkcs9AtExtensionRequest, + new DerSet(extGen.Generate()))); + + var csr = new Pkcs10CertificationRequest( + "SHA256withRSA", new X509Name($"CN={cn}"), kp.Public, attributes, kp.Private); + + return "-----BEGIN CERTIFICATE REQUEST-----\n" + + Convert.ToBase64String(csr.GetEncoded(), Base64FormattingOptions.InsertLineBreaks) + + "\n-----END CERTIFICATE REQUEST-----"; + } + /// /// Builds a real PKCS#10 CSR for , optionally carrying a /// subjectAltName extension holding . @@ -378,6 +406,155 @@ await plugin.Enroll( AdditionalDomains(CapturedCertificateInformation()).Should().BeNull(); } + // ======================================================================= + // GeneralName types with no domain-name rendering + // ======================================================================= + + /// + /// A UPN otherName and a directoryName must NOT be submitted. + /// + /// Regression: GeneralNameToValue's default branch returned BouncyCastle's ASN.1 + /// stringification, so a Windows-generated CSR carrying a UPN otherName put + /// "[1.3.6.1.4.1.311.20.2.3, [CONTEXT 0]svc@corp.example.com]" into additionalDomains as if + /// it were a domain name — breaking orders that previously succeeded, and contradicting the + /// method's own doc comment. These types cannot become a certificate SAN via a domain-name + /// field at all, which is why they are skipped (with a Warning) rather than submitted the way + /// well-formed IP/email/URI SANs are. + /// + [Fact] + public async Task CsrOtherNameAndDirectoryName_AreNotSubmittedAsDomains() + { + // UPN otherName, as emitted by Windows/AD certificate tooling. + var upn = new GeneralName(GeneralName.OtherName, new DerSequence( + new DerObjectIdentifier("1.3.6.1.4.1.311.20.2.3"), + new DerTaggedObject(true, 0, new DerUtf8String("svc@corp.example.com")))); + + var directoryName = new GeneralName( + GeneralName.DirectoryName, new X509Name("CN=host.example.com,O=Acme")); + + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPemWithGeneralNames( + "host.example.com", + new GeneralName(GeneralName.DnsName, "alt.example.com"), + upn, + directoryName), + subject: "CN=host.example.com", + san: null, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + var domains = AdditionalDomains(CapturedCertificateInformation()); + + domains.Should().BeEquivalentTo(new[] { "alt.example.com" }, + "only the renderable DNS name may be submitted"); + domains.Should().NotContain(d => d.Contains("1.3.6.1.4.1.311.20.2.3"), + "an otherName must never be submitted as an ASN.1 dump"); + domains.Should().NotContain(d => d.Contains("CONTEXT"), + "BouncyCastle ASN.1 debris must never reach the wire"); + domains.Should().NotContain(d => d.StartsWith("CN=", StringComparison.OrdinalIgnoreCase), + "a directoryName must never be submitted as a domain"); + } + + // ======================================================================= + // Log-injection hardening (CWE-117) + // ======================================================================= + + /// + /// SAN values reach the log from the CSR and from Command's SAN dictionary — i.e. from the + /// requester. Structured message templates stop format-string abuse but not embedded + /// newlines, so a value carrying CRLF could forge audit records in the very log lines added + /// to make the submitted SAN set auditable. Pins the scrub on the private helper directly, + /// following ExtractSerialFromPemTests' pattern. + /// + [Theory] + [InlineData("evil.example.com\r\nINFO forged record", "evil.example.com\\r\\nINFO forged record")] + [InlineData("a\nb", "a\\nb")] + [InlineData("a\tb", "a\\tb")] + [InlineData("plain.example.com", "plain.example.com")] + [InlineData("", "")] + [InlineData(null, null)] + public void SanitizeForLog_NeutralizesControlCharacters(string input, string expected) + { + var method = typeof(CERTInextCAPlugin) + .GetMethod("SanitizeForLog", BindingFlags.NonPublic | BindingFlags.Static); + method.Should().NotBeNull("log sinks depend on this scrub existing"); + + var actual = (string)method!.Invoke(null, new object[] { input }); + + actual.Should().Be(expected); + } + + /// + /// A CRLF-bearing SAN must not break enrollment, and the value is still submitted verbatim — + /// the scrub is a logging concern and deliberately does not mutate the payload sent to the CA. + /// + [Fact] + public async Task SanValueWithCrLf_DoesNotBreakEnrollment() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "alt.example.com\r\nforged log line" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().ContainSingle().Which.Should().Contain("alt.example.com"); + } + + // ======================================================================= + // SubmitNonDnsSans escape hatch + // ======================================================================= + + /// + /// Submitting non-DNS SANs flips affected enrollments from "issues, silently missing the + /// name" to "parks pending". SubmitNonDnsSans=false restores the pre-1.0.1 behaviour so an + /// upgraded host has a way back that isn't a plugin downgrade. + /// + [Fact] + public async Task SubmitNonDnsSansFalse_SubmitsDnsNamesOnly() + { + var plugin = new CERTInextCAPlugin( + BuildRealClient(), + new CERTInextConfig { PickupRetries = 0, SubmitNonDnsSans = false }); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "alt.example.com" }, + ["ipaddress"] = new[] { "192.0.2.10" }, + ["rfc822name"] = new[] { "admin@example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "alt.example.com" }, + "with the switch off, only DNS names are submitted"); + } + + /// + /// The switch defaults to true, so the documented default behaviour is pinned independently + /// of any test that sets it explicitly. + /// + [Fact] + public void SubmitNonDnsSans_DefaultsToTrue() + { + new CERTInextConfig().SubmitNonDnsSans.Should().BeTrue(); + } + // ======================================================================= // Renew path — previously submitted no SANs at all // ======================================================================= diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index 4796d10..6335033 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -1607,27 +1607,54 @@ private async Task PerformDcvIfNeededAsync( // SOX CC6.1: validate domain names before passing them to the DNS provider plugin // or the CERTInext API. A malformed domain (empty, whitespace, or containing // characters outside the FQDN alphabet) could cause log injection or unexpected - // DNS plugin behaviour. Invalid entries are rejected loudly rather than silently - // skipped so the condition is visible in the audit trail. - foreach (var (domain, _) in pendingDomains) + // DNS plugin behaviour. Invalid entries are rejected loudly — LogError, so the + // condition is visible in the audit trail — but they are EXCLUDED rather than + // thrown on. + // + // Throwing here would fail the whole order: the exception escapes Enroll (which has + // no catch) after the order was already placed at the CA, so the enrollment reports + // failure with an orphaned order, and no TXT record is staged for the *valid* domains + // on the same order. Worse, it is unrecoverable — every later Synchronize / + // GetSingleRecord retry re-enters here, hits the same undrainable domain, and + // TryRunDcvDuringSyncAsync swallows the exception and returns false, so the order sits + // at EXTERNALVALIDATION forever. + // + // This is reachable in normal operation now that non-DNS SANs are submitted to + // CERTInext (see BuildSanList): the CA registers an email/URI SAN verbatim as an order + // domain, and that key is not an FQDN. One such SAN must not strand the DNS names + // alongside it. Same principle the EMS-956 branch below states explicitly: do not throw + // out of DCV for a condition that leaves the order legitimately pending. + var invalidDomains = new List(); + var validPendingDomains = new List>(); + + foreach (var entry in pendingDomains) { - if (string.IsNullOrWhiteSpace(domain)) - throw new InvalidOperationException( - $"TrackOrder returned a blank domain key in domainVerification for order '{orderNumber}'. " + - "Cannot proceed with DCV."); + string domain = entry.Key; // Allow standard FQDN characters plus wildcard prefix (*.example.com) - if (!System.Text.RegularExpressions.Regex.IsMatch(domain, @"^(\*\.)?[a-zA-Z0-9]([a-zA-Z0-9\-\.]*[a-zA-Z0-9])?$")) - { - _logger.LogError( - "DCV domain name failed validation and will not be processed. OrderNumber={OrderNumber}, Domain={Domain}", - orderNumber, domain); - throw new InvalidOperationException( - $"TrackOrder returned an invalid domain name '{domain}' in domainVerification for order '{orderNumber}'. " + - "Domain names must conform to FQDN syntax."); - } + bool valid = !string.IsNullOrWhiteSpace(domain) + && System.Text.RegularExpressions.Regex.IsMatch( + domain, @"^(\*\.)?[a-zA-Z0-9]([a-zA-Z0-9\-\.]*[a-zA-Z0-9])?$"); + + if (valid) + validPendingDomains.Add(entry); + else + invalidDomains.Add(string.IsNullOrWhiteSpace(domain) ? "(blank)" : domain); + } + + if (invalidDomains.Count > 0) + { + _logger.LogError( + "{Count} domain(s) on order {OrderNumber} are not valid FQDNs and cannot be DNS-01 validated: " + + "[{Domains}]. They are skipped so the remaining {ValidCount} domain(s) can still be validated. " + + "This order cannot be issued by CERTInext until these are removed — they usually come from a " + + "non-DNS SAN (IP address, email, URI) that was requested on the enrollment.", + invalidDomains.Count, orderNumber, SanitizeForLog(string.Join(", ", invalidDomains)), + validPendingDomains.Count); } + pendingDomains = validPendingDomains; + if (pendingDomains.Count == 0) return false; @@ -1637,6 +1664,11 @@ private async Task PerformDcvIfNeededAsync( var stagedValidations = new List<(string domain, string hostname, Keyfactor.AnyGateway.Extensions.IDomainValidator validator)>(); + // Domains for which no DNS provider plugin resolved. Used after the staging loop to tell + // "this one name is unvalidatable" (skip, keep going) apart from "no DNS provider is + // deployed at all" (a gateway misconfiguration that must still fail loudly). + var unresolvedDomains = new List(); + // Stage DNS TXT records for all pending domains foreach (var (domain, _) in pendingDomains) { @@ -1679,9 +1711,31 @@ private async Task PerformDcvIfNeededAsync( var validator = DomainValidatorFactory.ResolveDomainValidator(domain, "dns-01"); if (validator == null) - throw new InvalidOperationException( - $"No DNS provider plugin is configured for domain '{domain}'. " + - "Ensure the appropriate DNS provider plugin is deployed and configured on the gateway."); + { + // Two different conditions land here, and they want different handling: + // + // * This *particular* name is unvalidatable while others on the order are fine — + // an IP-literal SAN is the canonical case (it satisfies the FQDN regex above, + // but no DNS zone can ever match it). Skip it, so its co-tenant domains still + // get staged. Throwing would fail the whole Enroll after the order was already + // placed and leave the order permanently unable to progress. + // + // * No DNS provider is deployed/configured at all — a real gateway + // misconfiguration. That is still raised, after the loop, when nothing on the + // order staged (see below), preserving the loud operator-facing failure. + // + // Trade-off accepted: an order whose *only* pending domain is unresolvable still + // throws. In practice the order's primary domain (the CN) is always a pending + // domain too, so this is the misconfiguration case rather than the bad-SAN case. + _logger.LogError( + "No DNS provider plugin resolved for domain '{Domain}' on order {OrderNumber}. " + + "If this is a real domain, ensure the appropriate DNS provider plugin is deployed and " + + "configured on the gateway; if it came from a non-DNS SAN (e.g. an IP address), remove " + + "it from the request.", + SanitizeForLog(domain), orderNumber); + unresolvedDomains.Add(domain); + continue; + } _logger.LogInformation( "Staging DNS TXT record for DCV. OrderNumber={OrderNumber}, Domain={Domain}, Hostname={Hostname}", @@ -1695,6 +1749,14 @@ private async Task PerformDcvIfNeededAsync( stagedValidations.Add((domain, hostname, validator)); } + // Nothing staged and every pending domain failed provider resolution => no DNS provider + // plugin is usable on this gateway at all. That is a deployment misconfiguration, not bad + // request data, so it still fails loudly rather than silently parking the order. + if (stagedValidations.Count == 0 && unresolvedDomains.Count == pendingDomains.Count && unresolvedDomains.Count > 0) + throw new InvalidOperationException( + $"No DNS provider plugin is configured for domain '{unresolvedDomains[0]}'. " + + "Ensure the appropriate DNS provider plugin is deployed and configured on the gateway."); + if (stagedValidations.Count == 0) return false; @@ -2248,11 +2310,27 @@ void Add(string type, string value) // 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)) + var csrSans = ExtractSanEntriesFromCsr(csr, out List skippedCsrTags); + foreach (var csrSan in csrSans) Add(csrSan.Type, csrSan.Value); int fromCsrOnly = result.Count - fromGateway; + if (skippedCsrTags.Count > 0) + { + // GeneralName types with no domain-name rendering (otherName — e.g. a UPN from a + // Windows-generated CSR — directoryName, x400Address, ediPartyName, registeredID). + // They cannot be expressed in additionalDomains, so they are not forwarded. Warn + // rather than drop silently: the operator needs to know the CSR asked for something + // the certificate will not carry. + _logger.LogWarning( + "{Count} SAN(s) in the CSR use a type that cannot be represented as a domain name " + + "and were not submitted (ASN.1 GeneralName tag(s): {Tags}). CERTInext's " + + "additionalDomains field carries domain names only, so these cannot appear on the " + + "issued certificate. Remove them from the CSR if they are required.", + skippedCsrTags.Count, string.Join(", ", skippedCsrTags)); + } + if (result.Count == 0) { _logger.LogDebug( @@ -2266,7 +2344,7 @@ void Add(string type, string value) "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}"))); + SanitizeForLog(string.Join("; ", result.Select(s => $"{s.Type}:{s.Value}")))); if (fromCsrOnly > 0) { @@ -2295,6 +2373,27 @@ void Add(string type, string value) // 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(); + + // Escape hatch (SubmitNonDnsSans, default true). On a host that was previously issuing + // certificates for requests carrying an IP or email SAN, the default behaviour flips + // those enrollments from "issues, silently missing the name" to "parks pending", and an + // operator needs a way back that isn't a plugin downgrade. Off => pre-1.0.1 behaviour. + if (nonDns.Count > 0 && !_config.SubmitNonDnsSans) + { + _logger.LogWarning( + "{Count} requested SAN(s) are not DNS names and are being DROPPED because " + + "SubmitNonDnsSans is false: {Sans}. The order will issue, but the certificate will " + + "NOT contain these names. Set SubmitNonDnsSans back to true to submit them and have " + + "CERTInext surface the problem instead.", + nonDns.Count, SanitizeForLog(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}")))); + + result = result + .Where(s => string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)) + .ToList(); + + return result.Count > 0 ? result : null; + } + if (nonDns.Count > 0) { _logger.LogWarning( @@ -2305,7 +2404,7 @@ void Add(string type, string value) "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}"))); + nonDns.Count, SanitizeForLog(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}")))); } return result; @@ -2340,9 +2439,17 @@ private static string MapSanType(string anyCAType) /// 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) + /// PEM-encoded PKCS#10 request, or null/garbage. + /// + /// ASN.1 GeneralName tag numbers present in the CSR that have no domain-name rendering and + /// were therefore not returned (otherName, directoryName, x400Address, ediPartyName, + /// registeredID, and any malformed IPAddress). Reported so the caller can warn instead of + /// dropping them silently. + /// + private static List ExtractSanEntriesFromCsr(string csrPem, out List skippedTagNumbers) { var result = new List(); + skippedTagNumbers = new List(); if (string.IsNullOrWhiteSpace(csrPem)) return result; @@ -2375,6 +2482,8 @@ private static List ExtractSanEntriesFromCsr(string csrPem) string value = GeneralNameToValue(generalName); if (!string.IsNullOrWhiteSpace(value)) result.Add(new SanEntry { Type = SanTypeFromGeneralNameTag(generalName.TagNo), Value = value }); + else + skippedTagNumbers.Add(generalName.TagNo); } } catch (Exception ex) @@ -2428,10 +2537,43 @@ private static string GeneralNameToValue(Org.BouncyCastle.Asn1.X509.GeneralName : null; default: - return generalName.Name?.ToString(); + // otherName, directoryName, x400Address, ediPartyName, registeredID. + // + // Deliberately null, not Name.ToString(). BouncyCastle renders these as an + // ASN.1 dump — a UPN otherName from a Windows-generated CSR stringifies to + // "[1.3.6.1.4.1.311.20.2.3, [CONTEXT 0]svc@corp.example.com]" and a + // directoryName to "CN=host.example.com,O=Acme". Submitting that as an entry in + // additionalDomains is not "forwarding the name the subscriber asked for" — it + // is putting ASN.1 debris in a domain-name field, which cannot become a + // certificate SAN under any circumstances and only breaks the order. That is + // different from a well-formed non-DNS SAN (IP/email/URI), which we do submit + // on purpose so nothing the subscriber requested is dropped silently. + // + // Skipped is not silent: BuildSanList warns with the tag numbers so the + // operator can see a SAN was present and not forwarded. + return null; } } + /// + /// Strips CR, LF, and tab from a value before it is interpolated into a log message. + /// + /// SAN values reach the log from the CSR and from Command's SAN dictionary, i.e. from the + /// requester. Structured message templates stop format-string abuse but not embedded + /// newlines, and NLog's text layout does not escape them — so an unsanitized value can + /// forge additional, well-formed-looking records in the gateway log (CWE-117). That matters + /// here specifically because these log lines exist to make the submitted SAN set auditable; + /// a forged line could assert a different SAN set than the one actually sent. + /// + private static string SanitizeForLog(string value) + { + if (string.IsNullOrEmpty(value)) return value; + return value + .Replace("\r", "\\r") + .Replace("\n", "\\n") + .Replace("\t", "\\t"); + } + private static string GetStringValue( Dictionary dict, string key, string defaultValue = "") { diff --git a/CERTInext/CERTInextCAPluginConfig.cs b/CERTInext/CERTInextCAPluginConfig.cs index e77ac68..980a26a 100644 --- a/CERTInext/CERTInextCAPluginConfig.cs +++ b/CERTInext/CERTInextCAPluginConfig.cs @@ -256,6 +256,19 @@ public static Dictionary GetCAConnectorAnnotations() DefaultValue = false, Type = "Boolean" }, + [Constants.Config.SubmitNonDnsSans] = new PropertyConfigInfo + { + Comments = "If true (default), SANs that are not DNS names (IP address, email, URI) are " + + "submitted to CERTInext in additionalDomains along with the DNS names. CERTInext " + + "registers them verbatim as order domains and they cannot pass domain validation, " + + "so such an order will not issue until they are removed — but nothing the " + + "subscriber requested is dropped silently. Set to false to submit DNS names only, " + + "which restores the pre-1.0.1 behaviour: the order issues, but the certificate " + + "will not contain the non-DNS names. Default: true.", + Hidden = false, + DefaultValue = true, + Type = "Boolean" + }, [Constants.Config.PageSize] = new PropertyConfigInfo { Comments = "Number of orders to fetch per page during synchronization. " + @@ -691,6 +704,23 @@ public class CERTInextConfig [JsonPropertyName("IgnoreExpired")] public bool IgnoreExpired { get; set; } = false; + /// + /// Whether non-DNS SANs (IP address, email, URI) are submitted to CERTInext. + /// + /// Defaults to true: nothing the subscriber requested is dropped silently. CERTInext + /// registers such values verbatim as order domains, and they cannot pass domain validation, + /// so the order will not issue until they are removed — a visible failure, deliberately + /// preferred over a certificate quietly missing requested names. + /// + /// Set to false to submit DNS names only, restoring the pre-1.0.1 behaviour where the + /// order issues but the non-DNS names are absent from the certificate. This exists as an + /// upgrade escape hatch: on a host that was issuing certificates for requests carrying an IP + /// or email SAN, the default flips those enrollments from "issues (incomplete)" to "parks + /// pending", and an operator needs a way back that does not involve downgrading the plugin. + /// + [JsonPropertyName("SubmitNonDnsSans")] + public bool SubmitNonDnsSans { get; set; } = true; + [JsonPropertyName("PageSize")] public int PageSize { get; set; } = Constants.Api.DefaultPageSize; diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index aa98d0c..f0c4b3f 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -195,10 +195,10 @@ public async Task PlaceOrderAsync( "Submitting order to CERTInext. ProductCode={ProductCode}, DomainName={DomainName}, " + "AdditionalDomainCount={AdditionalDomainCount}, AdditionalDomains={AdditionalDomains}", request.OrderDetails?.ProductCode, - certInfo?.DomainName, + SanitizeForLog(certInfo?.DomainName), certInfo?.AdditionalDomains?.Count ?? 0, certInfo?.AdditionalDomains != null && certInfo.AdditionalDomains.Count > 0 - ? string.Join("; ", certInfo.AdditionalDomains) + ? SanitizeForLog(string.Join("; ", certInfo.AdditionalDomains)) : "(none)"); GenerateOrderResponse result = null; @@ -789,12 +789,16 @@ public async Task RenewCertificateAsync( // Primary domain for the renewal order. Prefer the CN of the subject Command gave // us; the prior order's requestorName is only a last resort and is not a domain — // it is retained solely so an old caller that sets no Subject behaves as before. + // Hoisted: the same parse drives both the domain and the "did we get a CN?" warning, + // mirroring BuildOrderRequestFromLegacyEnrollRequest. + string subjectCn = ExtractCnFromSubject(request.Subject); + string renewalDomainName = - ExtractCnFromSubject(request.Subject) + subjectCn ?? priorTrack.OrderDetails?.RequestorInformation?.RequestorName ?? "unknown"; - if (ExtractCnFromSubject(request.Subject) == null) + if (subjectCn == null) { Logger.LogWarning( "Renewal of order {PriorId} has no usable CN in its subject; falling back to " + @@ -1765,6 +1769,25 @@ internal static string RedactCredentials(string body) return body; } + /// + /// Strips CR, LF, and tab from a value before it is interpolated into a log message. + /// + /// Domain values logged at the wire originate from the requester (Command's SAN dictionary + /// or the CSR). Structured message templates stop format-string abuse but not embedded + /// newlines, and NLog's text layout does not escape them, so an unsanitized value could + /// forge additional well-formed-looking records (CWE-117) in the very log line added to make + /// the submitted domain set auditable. The plugin sanitizes its own SAN log sinks the same + /// way; this covers the order-submission sink. + /// + private static string SanitizeForLog(string value) + { + if (string.IsNullOrEmpty(value)) return value; + return value + .Replace("\r", "\\r") + .Replace("\n", "\\n") + .Replace("\t", "\\t"); + } + /// /// Writes a structured log capturing every diagnostic field available for a /// non-success CERTInext API response — HTTP status, the CERTInext-side error diff --git a/CERTInext/Constants.cs b/CERTInext/Constants.cs index 4510286..22da587 100644 --- a/CERTInext/Constants.cs +++ b/CERTInext/Constants.cs @@ -20,6 +20,7 @@ public static class Config public const string AuthMode = "AuthMode"; public const string Enabled = "Enabled"; public const string IgnoreExpired = "IgnoreExpired"; + public const string SubmitNonDnsSans = "SubmitNonDnsSans"; public const string PageSize = "PageSize"; // Synchronous certificate pickup (parity with the legacy Sectigo connector). diff --git a/CHANGELOG.md b/CHANGELOG.md index d971478..d3ca570 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ - **Faster enrollment for quickly-issued certificates.** Enrollment now waits briefly for the certificate and returns it in the same request when it issues fast (DV and already-approved orders), instead of always waiting for the next synchronization. Two new optional settings control the wait: `PickupRetries` (default 5; set to `0` to disable) and `PickupDelay` (default 10 seconds) — about a 55-second wait by default, with a built-in ceiling so it can't run long enough to time out the enrollment. Orders that don't issue in that window — including OV/EV, which CERTInext validates asynchronously over minutes to hours — return pending and are imported by a later sync, exactly as before. Works with or without DNS-based DCV. ## Bug Fixes +- **UCC certificates no longer come back with only the common name.** SANs requested on a Multi-Domain (UCC) enrollment were not reaching CERTInext at all: the gateway supplies them under the key `dnsname`, which the plugin did not recognize, so the order was submitted with an empty additional-domains list and the CA issued a certificate containing just the CN. SANs supplied on the CSR did not compensate, because CERTInext does not read the CSR's subjectAltName extension — names must be submitted explicitly. The plugin now recognizes every SAN key the gateway sends, and additionally reads SANs out of the CSR itself so a name requested only there still reaches the certificate. The submitted domain list is now logged at the point it goes on the wire, so a missing SAN can be traced without guesswork. +- **Renewals no longer lose their SANs.** Certificates renewed through the CA's renew path were submitted with no additional domains and took their primary domain from the previous order's requestor name rather than the certificate subject, so a renewed UCC certificate came back holding a single, possibly wrong, domain. + +## Upgrade Notes +- **Requests carrying a non-DNS SAN (IP address, email, URI) now behave differently.** Previously these were silently discarded and the certificate issued without them. They are now submitted, because issuing a certificate that quietly omits names the subscriber asked for is the worse outcome. CERTInext accepts such a value as an order domain but it cannot pass domain validation, so the order will not issue until the SAN is removed from the request — the gateway log names the offending SANs. If you need the previous behavior while you clean up your templates, set the new connector setting **`SubmitNonDnsSans`** to `false` (default `true`) to submit DNS names only. Public TLS certificates cannot contain IP or email SANs in the first place, so most deployments are unaffected. + - **No more duplicate or orphaned orders after a network timeout.** Order and CSR submissions are no longer retried after a network timeout. A timeout can happen *after* the CA has already accepted the request, so the automatic retry was being rejected as a duplicate — failing the enrollment and leaving an orphaned order behind. These requests now run once; if the order was created it is imported by the next synchronization, and duplicate responses are reported with clear, actionable guidance. (Read-only calls are unaffected and still retry.) # 1.0.0 From 0caaa1bd42e9176ce7ae288ca3a16878ecb76351 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Wed, 12 Aug 2026 11:45:35 -0700 Subject: [PATCH 04/14] =?UTF-8?q?fix(enroll):=20address=20full-review=20ro?= =?UTF-8?q?und=202=20=E2=80=94=20false=20pending-domain=20invariant,=20TXT?= =?UTF-8?q?=20leak?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CERTInext.Tests/CERTInextCAPluginDcvTests.cs | 175 ++++++++++ CERTInext.Tests/SanSubmissionTests.cs | 10 +- CERTInext/CERTInextCAPlugin.cs | 322 +++++++++++-------- CERTInext/Client/CERTInextClient.cs | 25 +- CERTInext/Models/LogSanitizer.cs | 33 ++ 5 files changed, 399 insertions(+), 166 deletions(-) create mode 100644 CERTInext/Models/LogSanitizer.cs diff --git a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs index 7861618..a51128f 100644 --- a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs @@ -862,6 +862,94 @@ private static TrackOrderResponse DcvPendingTrackResponseMultiDomain( }; } + /// + /// Builds a TrackOrder response with one already-validated domain (dcvStatus=1) and one + /// still-pending, unresolvable domain (dcvStatus=0) — the shape CERTInext produces when it + /// has cached a prior DCV validation for the CN while a non-DNS SAN on the same order is + /// still outstanding. + /// + private static TrackOrderResponse DcvMixedStatusTrackResponse( + string validatedDomain, string pendingDomain) + { + var validated = System.Text.Json.JsonSerializer.SerializeToElement(new DomainVerificationDetail + { + DcvMethod = Constants.Dcv.MethodDnsTxt, + DcvStatus = Constants.Dcv.StatusValidated, + Status = "1" + }); + var pending = System.Text.Json.JsonSerializer.SerializeToElement(new DomainVerificationDetail + { + DcvMethod = Constants.Dcv.MethodDnsTxt, + DcvStatus = Constants.Dcv.StatusPending, + Status = "1" + }); + + return new TrackOrderResponse + { + OrderDetails = new TrackOrderResponseDetails + { + OrderStatusId = "1", + CertificateStatusId = "1", + DomainVerification = new TrackOrderDomainVerification + { + // Aggregate stays pending because one domain still is — this must not take + // the early "already validated" return at the top of the method. + Status = Constants.Dcv.StatusPending, + RawDomainEntries = new Dictionary + { + [validatedDomain] = validated, + [pendingDomain] = pending + } + } + } + }; + } + + /// + /// Regression for the false invariant behind the round-1 fix's own misconfiguration check: + /// "the CN is always a pending domain too" is untrue whenever CERTInext has cached a prior + /// DCV validation for it (a case this same file's cached-validation branch documents), so a + /// non-DNS SAN sharing the order with an already-validated CN must not throw — it must defer + /// to the next sync cycle exactly like the single-domain case does. + /// + [Fact] + public async Task Dcv_CachedCnPlusUnresolvableSan_DefersWithoutThrowing() + { + const string order = MockCertificateData.DcvOrderId; + const string cn = MockCertificateData.DcvDomain; + const string ip = "192.0.2.10"; + + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending" }); + + mock.Setup(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvMixedStatusTrackResponse(validatedDomain: cn, pendingDomain: ip)); + + // The IP clears the FQDN regex and reaches GetDcv, per the sandbox-measured shape. + mock.Setup(c => c.GetDcvAsync(order, ip, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse(MockCertificateData.DcvToken)); + + var validator = new FakeDomainValidator(); + // Resolves for the CN (a real, working DNS provider) but not for the IP literal — the + // scenario that must prove "a provider IS deployed" rather than "nothing is deployed". + var plugin = BuildPlugin( + mock.Object, + new SelectiveDomainValidatorFactory(validator, resolvableDomain: cn), + DcvConfig()); + + Func act = () => Enroll(plugin); + + await act.Should().NotThrowAsync( + "an unresolvable non-DNS SAN must defer the order to the next sync cycle, not fail " + + "the enrollment — the CN having cached DCV proves a provider is deployed and working, " + + "so this is not the 'nothing is deployed' misconfiguration case"); + + validator.StagedRecords.Should().BeEmpty( + "the only pending domain is unresolvable, so nothing should have been staged"); + } + /// /// A non-FQDN pending domain must be skipped, not thrown on. /// @@ -977,5 +1065,92 @@ public IDomainValidator ResolveDomainValidator(string domain, string validationT public IDomainValidator PrimaryValidator => _validator; } + + /// + /// Like , but StageValidation fails only for hostnames + /// matching — needed to prove that a failure on one + /// domain of a multi-domain order still cleans up what was already staged for the others. + /// + private sealed class PartiallyFailingDomainValidator : IDomainValidator + { + private readonly string _failingHostnameSubstring; + + public PartiallyFailingDomainValidator(string failingHostnameSubstring) => + _failingHostnameSubstring = failingHostnameSubstring; + + public List<(string key, string value)> StagedRecords { get; } = new(); + public List CleanedUpKeys { get; } = new(); + + public void Initialize(IDomainValidatorConfigProvider configProvider) { } + + public Task StageValidation(string key, string value, CancellationToken cancellationToken) + { + bool shouldFail = key.Contains(_failingHostnameSubstring, StringComparison.OrdinalIgnoreCase); + if (!shouldFail) + StagedRecords.Add((key, value)); + + return Task.FromResult(new DomainValidationResult + { + Success = !shouldFail, + ErrorMessage = shouldFail ? "Stage failed (test stub, selective)" : null + }); + } + + public Task CleanupValidation(string key, CancellationToken cancellationToken) + { + CleanedUpKeys.Add(key); + return Task.FromResult(new DomainValidationResult { Success = true }); + } + + public Task ValidateConfiguration(Dictionary configuration) => Task.CompletedTask; + public Dictionary GetDomainValidatorAnnotations() => new(); + public string GetValidationType() => "dns-01"; + } + + /// + /// Regression: a StageValidation failure on one domain of a multi-domain order must not + /// leave the TXT records already published for the earlier domains orphaned. Before the + /// fix, the staging loop's throw sites were outside the try/finally that owns cleanup, so + /// this was reachable only by accident (pre-fix, a UCC order's SANs never reached CERTInext + /// at all, so an order rarely had more than one pending domain to stage). Submitting every + /// requested SAN makes multi-domain staging the normal case, so this must hold now. + /// + [Fact] + public async Task Dcv_StageFailureOnSecondDomain_CleansUpFirstDomainsTxtRecord() + { + const string order = MockCertificateData.DcvOrderId; + const string good = "a.example.com"; + const string bad = "b.example.com"; + + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); + + mock.Setup(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, good, bad)); + + mock.Setup(c => c.GetDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse("token-a")); + mock.Setup(c => c.GetDcvAsync(order, bad, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse("token-b")); + + var validator = new PartiallyFailingDomainValidator(failingHostnameSubstring: bad); + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator)); + + Func act = () => Enroll(plugin); + + await act.Should().ThrowAsync() + .WithMessage("*Failed to stage DNS validation*"); + + string goodHostname = string.Format(Constants.Dcv.DefaultTxtRecordTemplate, good); + validator.StagedRecords.Should().ContainSingle( + "only the domain staged before the failure should have a recorded StageValidation call") + .Which.key.Should().Be(goodHostname); + validator.CleanedUpKeys.Should().Contain(goodHostname, + "the TXT record already published for the domain that succeeded must be removed " + + "when a later domain in the same order fails to stage — otherwise it is orphaned in " + + "the customer's DNS zone with nothing else in the codebase to ever clean it up"); + } } } diff --git a/CERTInext.Tests/SanSubmissionTests.cs b/CERTInext.Tests/SanSubmissionTests.cs index fafb247..637aa72 100644 --- a/CERTInext.Tests/SanSubmissionTests.cs +++ b/CERTInext.Tests/SanSubmissionTests.cs @@ -466,8 +466,8 @@ await plugin.Enroll( /// SAN values reach the log from the CSR and from Command's SAN dictionary — i.e. from the /// requester. Structured message templates stop format-string abuse but not embedded /// newlines, so a value carrying CRLF could forge audit records in the very log lines added - /// to make the submitted SAN set auditable. Pins the scrub on the private helper directly, - /// following ExtractSerialFromPemTests' pattern. + /// to make the submitted SAN set auditable. LogSanitizer is internal (not private) and + /// shared between the plugin and the client, so this is a direct call, not reflection. /// [Theory] [InlineData("evil.example.com\r\nINFO forged record", "evil.example.com\\r\\nINFO forged record")] @@ -478,11 +478,7 @@ await plugin.Enroll( [InlineData(null, null)] public void SanitizeForLog_NeutralizesControlCharacters(string input, string expected) { - var method = typeof(CERTInextCAPlugin) - .GetMethod("SanitizeForLog", BindingFlags.NonPublic | BindingFlags.Static); - method.Should().NotBeNull("log sinks depend on this scrub existing"); - - var actual = (string)method!.Invoke(null, new object[] { input }); + var actual = Keyfactor.Extensions.CAPlugin.CERTInext.Models.LogSanitizer.Strip(input); actual.Should().Be(expected); } diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index 6335033..f6102d4 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -1649,7 +1649,7 @@ private async Task PerformDcvIfNeededAsync( "[{Domains}]. They are skipped so the remaining {ValidCount} domain(s) can still be validated. " + "This order cannot be issued by CERTInext until these are removed — they usually come from a " + "non-DNS SAN (IP address, email, URI) that was requested on the enrollment.", - invalidDomains.Count, orderNumber, SanitizeForLog(string.Join(", ", invalidDomains)), + invalidDomains.Count, orderNumber, LogSanitizer.Strip(string.Join(", ", invalidDomains)), validPendingDomains.Count); } @@ -1669,93 +1669,151 @@ private async Task PerformDcvIfNeededAsync( // deployed at all" (a gateway misconfiguration that must still fail loudly). var unresolvedDomains = new List(); - // Stage DNS TXT records for all pending domains - foreach (var (domain, _) in pendingDomains) + // Set instead of an immediate `return false` inside the loop below, so a not-yet-ready + // domain that is deferred mid-loop still goes through the same cleanup as every other + // exit path — see the try/catch around the loop. + bool deferToNextSyncCycle = false; + + // Removes whatever TXT records were already published before an early exit from the + // staging loop (an exception, or a not-yet-ready deferral). Nothing else in this method + // cleans up mid-loop: the try/finally further down only runs once every pending domain + // has been staged, so without this, a later domain's failure orphans every TXT record + // already published for the earlier domains in the *same* order — permanently, since + // nothing else in the codebase calls CleanupValidation for them. + async Task CleanupPartialStagingAsync() { - GetDcvResponse dcvResp; - try - { - dcvResp = await _client.GetDcvAsync(orderNumber, domain, Constants.Dcv.MethodDnsTxt, ct); - } - catch (Exception ex) when (IsDcvNotYetReady(ex)) + foreach (var (domain, hostname, validator) in stagedValidations) { - // CERTInext occasionally exposes the DCV slot in TrackOrder (so - // domainVerification is populated and dcvStatus="0") before the GetDcv - // endpoint will accept calls for that order — observed as EMS-956 - // "Invalid Request for this API" for several hours after enrollment. - // Treat this as "DCV not ready yet": skip the DCV ceremony for now and - // let the sync-driven retry pick it up on a later cycle. We must NOT - // throw, because that would fail the entire Enroll call and prevent the - // gateway from recording the pending order at all. - _logger.LogInformation( - "GetDcv not yet accepting calls for order {OrderNumber} domain {Domain} ({Error}). " + - "Deferring DCV to the next sync cycle.", - orderNumber, domain, ex.Message); - return false; + try + { + await validator.CleanupValidation(hostname, ct); + _logger.LogInformation( + "DNS TXT record cleaned up after a later domain on the same order could not be " + + "staged. Domain={Domain}, Hostname={Hostname}", domain, hostname); + } + catch (Exception ex) + { + _logger.LogWarning(ex, + "Failed to clean up DNS TXT record after a partial staging failure. " + + "Domain={Domain}, Hostname={Hostname}. May require manual removal.", domain, hostname); + } } - catch (Exception ex) + } + + try + { + // Stage DNS TXT records for all pending domains + foreach (var (domain, _) in pendingDomains) { - _logger.LogError(ex, "GetDcv failed for order {OrderNumber} domain {Domain}", orderNumber, domain); - throw; - } + GetDcvResponse dcvResp; + try + { + dcvResp = await _client.GetDcvAsync(orderNumber, domain, Constants.Dcv.MethodDnsTxt, ct); + } + catch (Exception ex) when (IsDcvNotYetReady(ex)) + { + // CERTInext occasionally exposes the DCV slot in TrackOrder (so + // domainVerification is populated and dcvStatus="0") before the GetDcv + // endpoint will accept calls for that order — observed as EMS-956 + // "Invalid Request for this API" for several hours after enrollment. + // Treat this as "DCV not ready yet": skip the DCV ceremony for now and + // let the sync-driven retry pick it up on a later cycle. We must NOT + // throw, because that would fail the entire Enroll call and prevent the + // gateway from recording the pending order at all. + _logger.LogInformation( + "GetDcv not yet accepting calls for order {OrderNumber} domain {Domain} ({Error}). " + + "Deferring DCV to the next sync cycle.", + orderNumber, domain, ex.Message); + deferToNextSyncCycle = true; + break; + } + catch (Exception ex) + { + _logger.LogError(ex, "GetDcv failed for order {OrderNumber} domain {Domain}", orderNumber, domain); + throw; + } - string token = dcvResp.DcvDetails?.Token; - if (string.IsNullOrWhiteSpace(token)) - throw new InvalidOperationException( - $"GetDcv returned no token for order '{orderNumber}' domain '{domain}'."); + string token = dcvResp.DcvDetails?.Token; + if (string.IsNullOrWhiteSpace(token)) + throw new InvalidOperationException( + $"GetDcv returned no token for order '{orderNumber}' domain '{domain}'."); - string template = string.IsNullOrWhiteSpace(_config.DcvTxtRecordTemplate) - ? Constants.Dcv.DefaultTxtRecordTemplate - : _config.DcvTxtRecordTemplate; - string hostname = string.Format(template, domain); + string template = string.IsNullOrWhiteSpace(_config.DcvTxtRecordTemplate) + ? Constants.Dcv.DefaultTxtRecordTemplate + : _config.DcvTxtRecordTemplate; + string hostname = string.Format(template, domain); - var validator = DomainValidatorFactory.ResolveDomainValidator(domain, "dns-01"); - if (validator == null) - { - // Two different conditions land here, and they want different handling: - // - // * This *particular* name is unvalidatable while others on the order are fine — - // an IP-literal SAN is the canonical case (it satisfies the FQDN regex above, - // but no DNS zone can ever match it). Skip it, so its co-tenant domains still - // get staged. Throwing would fail the whole Enroll after the order was already - // placed and leave the order permanently unable to progress. - // - // * No DNS provider is deployed/configured at all — a real gateway - // misconfiguration. That is still raised, after the loop, when nothing on the - // order staged (see below), preserving the loud operator-facing failure. - // - // Trade-off accepted: an order whose *only* pending domain is unresolvable still - // throws. In practice the order's primary domain (the CN) is always a pending - // domain too, so this is the misconfiguration case rather than the bad-SAN case. - _logger.LogError( - "No DNS provider plugin resolved for domain '{Domain}' on order {OrderNumber}. " + - "If this is a real domain, ensure the appropriate DNS provider plugin is deployed and " + - "configured on the gateway; if it came from a non-DNS SAN (e.g. an IP address), remove " + - "it from the request.", - SanitizeForLog(domain), orderNumber); - unresolvedDomains.Add(domain); - continue; - } + var validator = DomainValidatorFactory.ResolveDomainValidator(domain, "dns-01"); + if (validator == null) + { + // Two different conditions land here, and they want different handling: + // + // * This *particular* name is unvalidatable while others on the order are fine — + // an IP-literal SAN is the canonical case (it satisfies the FQDN regex above, + // but no DNS zone can ever match it). Skip it, so its co-tenant domains still + // get staged. Throwing would fail the whole Enroll after the order was already + // placed and leave the order permanently unable to progress. + // + // * No DNS provider is deployed/configured at all — a real gateway + // misconfiguration. That is still raised, after the loop, when nothing on the + // order staged and no domain anywhere on the order resolves a provider (see + // below), preserving the loud operator-facing failure. + _logger.LogError( + "No DNS provider plugin resolved for domain '{Domain}' on order {OrderNumber}. " + + "If this is a real domain, ensure the appropriate DNS provider plugin is deployed and " + + "configured on the gateway; if it came from a non-DNS SAN (e.g. an IP address), remove " + + "it from the request.", + LogSanitizer.Strip(domain), orderNumber); + unresolvedDomains.Add(domain); + continue; + } - _logger.LogInformation( - "Staging DNS TXT record for DCV. OrderNumber={OrderNumber}, Domain={Domain}, Hostname={Hostname}", - orderNumber, domain, hostname); + _logger.LogInformation( + "Staging DNS TXT record for DCV. OrderNumber={OrderNumber}, Domain={Domain}, Hostname={Hostname}", + orderNumber, domain, hostname); + + var stageResult = await validator.StageValidation(hostname, token, ct); + if (!stageResult.Success) + throw new InvalidOperationException( + $"Failed to stage DNS validation for '{domain}': {stageResult.ErrorMessage}"); - var stageResult = await validator.StageValidation(hostname, token, ct); - if (!stageResult.Success) - throw new InvalidOperationException( - $"Failed to stage DNS validation for '{domain}': {stageResult.ErrorMessage}"); + stagedValidations.Add((domain, hostname, validator)); + } + } + catch + { + await CleanupPartialStagingAsync(); + throw; + } - stagedValidations.Add((domain, hostname, validator)); + if (deferToNextSyncCycle) + { + await CleanupPartialStagingAsync(); + return false; } - // Nothing staged and every pending domain failed provider resolution => no DNS provider - // plugin is usable on this gateway at all. That is a deployment misconfiguration, not bad - // request data, so it still fails loudly rather than silently parking the order. - if (stagedValidations.Count == 0 && unresolvedDomains.Count == pendingDomains.Count && unresolvedDomains.Count > 0) + // Nothing staged, at least one pending domain failed provider resolution, and — this is + // the part that must hold across the WHOLE order, not just the still-pending domains — + // no domain anywhere on the order, pending or already validated, resolves a provider + // either. That combination means this gateway has no usable DNS provider deployed at + // all: a real deployment always has at least one legitimate domain somewhere on the + // order. That is a misconfiguration, not bad request data, so it still fails loudly + // instead of silently parking the order. + // + // Checking only pendingDomains here was the bug: CERTInext can cache a prior DCV + // validation for a parent domain (see the aggregate/per-domain check earlier in this + // method), which removes the CN from pendingDomains even though it resolves a provider + // just fine. A prior version of this check assumed the CN is always pending, which this + // method's own cached-validation branch above proves false — so it threw on an ordinary + // non-DNS SAN sharing an order with an already-validated domain, reopening the exact + // orphaned-order failure this whole restructuring exists to prevent. + if (stagedValidations.Count == 0 && unresolvedDomains.Count > 0 + && !allDomainEntries.Keys.Any(d => DomainValidatorFactory.ResolveDomainValidator(d, "dns-01") != null)) + { throw new InvalidOperationException( $"No DNS provider plugin is configured for domain '{unresolvedDomains[0]}'. " + "Ensure the appropriate DNS provider plugin is deployed and configured on the gateway."); + } if (stagedValidations.Count == 0) return false; @@ -2344,7 +2402,7 @@ void Add(string type, string value) "Resolved {Total} SAN(s) for submission. FromGatewayRequest={FromGateway}, " + "AddedFromCsr={FromCsr}, Sans={Sans}", result.Count, fromGateway, fromCsrOnly, - SanitizeForLog(string.Join("; ", result.Select(s => $"{s.Type}:{s.Value}")))); + LogSanitizer.Strip(string.Join("; ", result.Select(s => $"{s.Type}:{s.Value}")))); if (fromCsrOnly > 0) { @@ -2374,28 +2432,31 @@ void Add(string type, string value) // which is the part that matters for diagnosis. var nonDns = result.Where(s => !string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)).ToList(); - // Escape hatch (SubmitNonDnsSans, default true). On a host that was previously issuing - // certificates for requests carrying an IP or email SAN, the default behaviour flips - // those enrollments from "issues, silently missing the name" to "parks pending", and an - // operator needs a way back that isn't a plugin downgrade. Off => pre-1.0.1 behaviour. - if (nonDns.Count > 0 && !_config.SubmitNonDnsSans) + if (nonDns.Count > 0) { - _logger.LogWarning( - "{Count} requested SAN(s) are not DNS names and are being DROPPED because " + - "SubmitNonDnsSans is false: {Sans}. The order will issue, but the certificate will " + - "NOT contain these names. Set SubmitNonDnsSans back to true to submit them and have " + - "CERTInext surface the problem instead.", - nonDns.Count, SanitizeForLog(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}")))); + string nonDnsRendered = LogSanitizer.Strip(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}"))); - result = result - .Where(s => string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)) - .ToList(); + // Escape hatch (SubmitNonDnsSans, default true). On a host that was previously + // issuing certificates for requests carrying an IP or email SAN, the default + // behaviour flips those enrollments from "issues, silently missing the name" to + // "parks pending", and an operator needs a way back that isn't a plugin downgrade. + // Off => pre-1.0.1 behaviour: drop them and issue. + if (!_config.SubmitNonDnsSans) + { + _logger.LogWarning( + "{Count} requested SAN(s) are not DNS names and are being DROPPED because " + + "SubmitNonDnsSans is false: {Sans}. The order will issue, but the certificate will " + + "NOT contain these names. Set SubmitNonDnsSans back to true to submit them and have " + + "CERTInext surface the problem instead.", + nonDns.Count, nonDnsRendered); - return result.Count > 0 ? result : null; - } + result = result + .Where(s => string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)) + .ToList(); + + return result.Count > 0 ? result : null; + } - if (nonDns.Count > 0) - { _logger.LogWarning( "{Count} requested SAN(s) are not DNS names: {Sans}. CERTInext's additionalDomains " + "field takes domain names, so this order will either be rejected outright or be " + @@ -2404,7 +2465,7 @@ void Add(string type, string value) "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, SanitizeForLog(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}")))); + nonDns.Count, nonDnsRendered); } return result; @@ -2479,9 +2540,9 @@ private static List ExtractSanEntriesFromCsr(string csrPem, out List ExtractSanEntriesFromCsr(string csrPem, out ListMaps 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. + /// Maps a GeneralName to the this plugin would submit for it, or null + /// for a name whose value cannot be rendered meaningfully — skipped rather than submitted as + /// ASN.1 debris. One switch, not two: a separate tag→type mapping alongside this one used to + /// assign a type string ("directoryname", "registeredid", ...) to tags that always return a + /// null value here anyway, so those branches were dead — the type never reached a caller + /// with no value to pair it with. /// - private static string GeneralNameToValue(Org.BouncyCastle.Asn1.X509.GeneralName generalName) + private static SanEntry GeneralNameToSanEntry(Org.BouncyCastle.Asn1.X509.GeneralName generalName) { + string type; + string value; + switch (generalName.TagNo) { case Org.BouncyCastle.Asn1.X509.GeneralName.DnsName: + type = "dns"; + value = Org.BouncyCastle.Asn1.DerIA5String.GetInstance(generalName.Name).GetString(); + break; + case Org.BouncyCastle.Asn1.X509.GeneralName.Rfc822Name: + type = "email"; + value = Org.BouncyCastle.Asn1.DerIA5String.GetInstance(generalName.Name).GetString(); + break; + case Org.BouncyCastle.Asn1.X509.GeneralName.UniformResourceIdentifier: - return Org.BouncyCastle.Asn1.DerIA5String.GetInstance(generalName.Name).GetString(); + type = "uri"; + value = Org.BouncyCastle.Asn1.DerIA5String.GetInstance(generalName.Name).GetString(); + break; case Org.BouncyCastle.Asn1.X509.GeneralName.IPAddress: + type = "ip"; // Octet string → dotted-quad / RFC 5952 text, so what we submit and log is // the address the subscriber asked for rather than its hex encoding. byte[] octets = Org.BouncyCastle.Asn1.Asn1OctetString.GetInstance(generalName.Name).GetOctets(); - return octets.Length == 4 || octets.Length == 16 + value = octets.Length == 4 || octets.Length == 16 ? new System.Net.IPAddress(octets).ToString() : null; + break; default: // otherName, directoryName, x400Address, ediPartyName, registeredID. @@ -2551,27 +2614,12 @@ private static string GeneralNameToValue(Org.BouncyCastle.Asn1.X509.GeneralName // // Skipped is not silent: BuildSanList warns with the tag numbers so the // operator can see a SAN was present and not forwarded. - return null; + type = null; + value = null; + break; } - } - /// - /// Strips CR, LF, and tab from a value before it is interpolated into a log message. - /// - /// SAN values reach the log from the CSR and from Command's SAN dictionary, i.e. from the - /// requester. Structured message templates stop format-string abuse but not embedded - /// newlines, and NLog's text layout does not escape them — so an unsanitized value can - /// forge additional, well-formed-looking records in the gateway log (CWE-117). That matters - /// here specifically because these log lines exist to make the submitted SAN set auditable; - /// a forged line could assert a different SAN set than the one actually sent. - /// - private static string SanitizeForLog(string value) - { - if (string.IsNullOrEmpty(value)) return value; - return value - .Replace("\r", "\\r") - .Replace("\n", "\\n") - .Replace("\t", "\\t"); + return string.IsNullOrWhiteSpace(value) ? null : new SanEntry { Type = type, Value = value }; } private static string GetStringValue( diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index f0c4b3f..8a3572a 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -195,10 +195,10 @@ public async Task PlaceOrderAsync( "Submitting order to CERTInext. ProductCode={ProductCode}, DomainName={DomainName}, " + "AdditionalDomainCount={AdditionalDomainCount}, AdditionalDomains={AdditionalDomains}", request.OrderDetails?.ProductCode, - SanitizeForLog(certInfo?.DomainName), + LogSanitizer.Strip(certInfo?.DomainName), certInfo?.AdditionalDomains?.Count ?? 0, certInfo?.AdditionalDomains != null && certInfo.AdditionalDomains.Count > 0 - ? SanitizeForLog(string.Join("; ", certInfo.AdditionalDomains)) + ? LogSanitizer.Strip(string.Join("; ", certInfo.AdditionalDomains)) : "(none)"); GenerateOrderResponse result = null; @@ -1591,7 +1591,7 @@ private List BuildAdditionalDomains( Logger.LogDebug( "Collapsed {Count} duplicate SAN value(s) out of additionalDomains " + "(already submitted as domainName '{DomainName}', or repeated in the SAN set).", - duplicates, domainName); + duplicates, LogSanitizer.Strip(domainName)); } return domains.Count > 0 ? domains : null; @@ -1769,25 +1769,6 @@ internal static string RedactCredentials(string body) return body; } - /// - /// Strips CR, LF, and tab from a value before it is interpolated into a log message. - /// - /// Domain values logged at the wire originate from the requester (Command's SAN dictionary - /// or the CSR). Structured message templates stop format-string abuse but not embedded - /// newlines, and NLog's text layout does not escape them, so an unsanitized value could - /// forge additional well-formed-looking records (CWE-117) in the very log line added to make - /// the submitted domain set auditable. The plugin sanitizes its own SAN log sinks the same - /// way; this covers the order-submission sink. - /// - private static string SanitizeForLog(string value) - { - if (string.IsNullOrEmpty(value)) return value; - return value - .Replace("\r", "\\r") - .Replace("\n", "\\n") - .Replace("\t", "\\t"); - } - /// /// Writes a structured log capturing every diagnostic field available for a /// non-success CERTInext API response — HTTP status, the CERTInext-side error diff --git a/CERTInext/Models/LogSanitizer.cs b/CERTInext/Models/LogSanitizer.cs new file mode 100644 index 0000000..d341f09 --- /dev/null +++ b/CERTInext/Models/LogSanitizer.cs @@ -0,0 +1,33 @@ +// Copyright 2026 Keyfactor +// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. +// At http://www.apache.org/licenses/LICENSE-2.0 + +namespace Keyfactor.Extensions.CAPlugin.CERTInext.Models +{ + /// + /// Neutralizes control characters before a requester-controlled value is interpolated into a + /// log message. + /// + /// SAN values reach the log from the CSR and from Command's SAN dictionary, i.e. from the + /// requester. Structured message templates stop format-string abuse but not embedded newlines, + /// and NLog's text layout does not escape them — so an unsanitized value can forge additional, + /// well-formed-looking records in the gateway log (CWE-117). That matters here specifically + /// because these log lines exist to make the submitted SAN set auditable; a forged line could + /// assert a different SAN set than the one actually sent. + /// + /// Shared between CERTInextCAPlugin and Client.CERTInextClient — both sanitize the + /// same kind of value at their respective log sinks, so this used to be defined twice, byte- + /// identical, one per class. + /// + internal static class LogSanitizer + { + internal static string Strip(string value) + { + if (string.IsNullOrEmpty(value)) return value; + return value + .Replace("\r", "\\r") + .Replace("\n", "\\n") + .Replace("\t", "\\t"); + } + } +} From 7388e752b86273a63bde49b7f40fe2d4017961a4 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:27:10 -0700 Subject: [PATCH 05/14] =?UTF-8?q?fix(enroll):=20address=20full-review=20ro?= =?UTF-8?q?und=203=20=E2=80=94=20never=20throw=20out=20of=20DCV=20staging,?= =?UTF-8?q?=20CSR=20fallback=20not=20union?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CERTInext.Tests/CERTInextCAPluginDcvTests.cs | 82 +++-- CERTInext.Tests/SanSubmissionTests.cs | 56 +++- CERTInext/CERTInextCAPlugin.cs | 305 +++++++++++-------- CERTInext/Client/CERTInextClient.cs | 2 +- 4 files changed, 277 insertions(+), 168 deletions(-) diff --git a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs index a51128f..98f7382 100644 --- a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs @@ -527,7 +527,7 @@ public async Task SyncDcvRetry_DoesSingleShotTrackOrder_WhenChallengeNotReady() // --------------------------------------------------------------------------- [Fact] - public async Task Dcv_Throws_WhenNoProviderForDomain() + public async Task Dcv_SkipsAndDefers_WhenNoProviderForDomain() { var mock = NewMock(); mock.Setup(c => c.EnrollCertificateAsync(It.IsAny(), It.IsAny())) @@ -539,17 +539,19 @@ public async Task Dcv_Throws_WhenNoProviderForDomain() mock.Setup(c => c.GetDcvAsync(MockCertificateData.DcvOrderId, MockCertificateData.DcvDomain, Constants.Dcv.MethodDnsTxt, It.IsAny())) .ReturnsAsync(MockCertificateData.DcvTokenResponse()); - // Factory returns null → no DNS provider configured + // Factory returns null → no DNS provider configured. Regression: this used to throw and + // fail the whole order — including when the "unresolvable" domain was actually just a + // non-DNS Subject CN with no config-level way to prevent the throw (SubmitNonDnsSans only + // filters the SAN list, not the subject). Now it is logged loudly and deferred instead. var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator: null)); Func act = () => Enroll(plugin); - await act.Should().ThrowAsync() - .WithMessage("*No DNS provider plugin is configured*"); + await act.Should().NotThrowAsync(); } [Fact] - public async Task Dcv_Throws_WhenStageValidationFails() + public async Task Dcv_SkipsAndDefers_WhenStageValidationFails() { var mock = NewMock(); mock.Setup(c => c.EnrollCertificateAsync(It.IsAny(), It.IsAny())) @@ -566,10 +568,12 @@ public async Task Dcv_Throws_WhenStageValidationFails() Func act = () => Enroll(plugin); - await act.Should().ThrowAsync() - .WithMessage("*Failed to stage DNS validation*DNS zone not writable*"); + // Regression: a StageValidation failure used to throw and fail the whole order. Now it + // is logged loudly and the domain is skipped/deferred — this is the only pending domain, + // so nothing gets staged and the order defers to the next sync cycle. + await act.Should().NotThrowAsync(); - // No VerifyDcv call — failed before reaching that step + // No VerifyDcv call — nothing was staged to verify mock.Verify(c => c.VerifyDcvAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); } @@ -602,7 +606,7 @@ public async Task Dcv_CleanupAlwaysCalled_EvenWhenVerifyDcvThrows() } [Fact] - public async Task Dcv_Throws_WhenGetDcvReturnsNoToken() + public async Task Dcv_SkipsAndDefers_WhenGetDcvReturnsNoToken() { var mock = NewMock(); mock.Setup(c => c.EnrollCertificateAsync(It.IsAny(), It.IsAny())) @@ -619,8 +623,11 @@ public async Task Dcv_Throws_WhenGetDcvReturnsNoToken() Func act = () => Enroll(plugin); - await act.Should().ThrowAsync() - .WithMessage("*GetDcv returned no token*"); + // Regression: an empty token used to throw and fail the whole order. It is now logged + // loudly (LogError) and the domain is skipped — the order defers to the next sync cycle + // rather than failing Enroll with an order already placed at the CA. + await act.Should().NotThrowAsync(); + validator.StagedRecords.Should().BeEmpty("the only pending domain returned no token, so nothing should have been staged"); } // --------------------------------------------------------------------------- @@ -689,11 +696,16 @@ public async Task Dcv_Defers_When_GetDcv_ReturnsInvalidRequestMessage_WithoutEms } [Fact] - public async Task Dcv_Rethrows_When_GetDcv_FailsWithUnrelatedError() + public async Task Dcv_SkipsAndDefers_WhenGetDcvFailsWithUnrelatedError() { - // Tolerance is narrow: a genuine server error (5xx, transport, auth) must still - // bubble up so the gateway treats the enrollment as failed and the operator can - // diagnose. This guards against accidentally swallowing every GetDcv exception. + // Regression: this test used to assert the opposite — that a genuine server error (5xx, + // transport, auth) must bubble up and fail the whole enrollment. That is exactly the + // orphaned-order failure mode: GetDcv's live behavior for a non-DNS order-domain is + // unmeasured (see BuildSanList's sandbox-only caveat), so treating any unrecognized + // GetDcv error as fatal risks failing perfectly good co-tenant DNS domains on the same + // order over one domain's transient or CA-side issue, with the enrollment already + // placed at CERTInext and no catch anywhere above this call. The failure is still loud + // (LogError, with the underlying exception) — it just no longer fails the call. var mock = NewMock(); mock.Setup(c => c.EnrollCertificateAsync(It.IsAny(), It.IsAny())) .ReturnsAsync(new EnrollCertificateResponse { Id = MockCertificateData.DcvOrderId, Status = "pending_dcv" }); @@ -708,8 +720,8 @@ public async Task Dcv_Rethrows_When_GetDcv_FailsWithUnrelatedError() var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator)); Func act = () => Enroll(plugin); - await act.Should().ThrowAsync() - .WithMessage("*HTTP 500*"); + await act.Should().NotThrowAsync(); + validator.StagedRecords.Should().BeEmpty("the only pending domain's GetDcv call failed, so nothing should have been staged"); } // --------------------------------------------------------------------------- @@ -1116,7 +1128,7 @@ public Task CleanupValidation(string key, CancellationTo /// requested SAN makes multi-domain staging the normal case, so this must hold now. /// [Fact] - public async Task Dcv_StageFailureOnSecondDomain_CleansUpFirstDomainsTxtRecord() + public async Task Dcv_StageFailureOnSecondDomain_DoesNotAbortTheGoodDomain() { const string order = MockCertificateData.DcvOrderId; const string good = "a.example.com"; @@ -1127,30 +1139,46 @@ public async Task Dcv_StageFailureOnSecondDomain_CleansUpFirstDomainsTxtRecord() It.IsAny(), It.IsAny())) .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); - mock.Setup(c => c.TrackOrderAsync(order, It.IsAny())) - .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, good, bad)); + // First TrackOrder call (inside PerformDcvIfNeededAsync) sees both domains pending; + // the second (WaitForDcvVerificationAsync's poll after staging/VerifyDcv) sees the one + // domain that actually got staged — 'good' — as verified. + mock.SetupSequence(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, good, bad)) + .ReturnsAsync(MockCertificateData.DcvVerifiedTrackResponse(order, good)); mock.Setup(c => c.GetDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) .ReturnsAsync(MockCertificateData.DcvTokenResponse("token-a")); mock.Setup(c => c.GetDcvAsync(order, bad, Constants.Dcv.MethodDnsTxt, It.IsAny())) .ReturnsAsync(MockCertificateData.DcvTokenResponse("token-b")); + mock.Setup(c => c.VerifyDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .Returns(Task.CompletedTask); + mock.Setup(c => c.GetCertificateAsync(order, It.IsAny())) + .ReturnsAsync(MockCertificateData.IssuedCertRecord(order)); + var validator = new PartiallyFailingDomainValidator(failingHostnameSubstring: bad); - var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator)); + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator), + DcvConfig(dcvWaitForIssuanceSeconds: 10)); Func act = () => Enroll(plugin); - await act.Should().ThrowAsync() - .WithMessage("*Failed to stage DNS validation*"); + // Regression: a StageValidation failure on one domain of a multi-domain order must not + // abort the whole order any more — it did before this fix, which both failed the + // enrollment with an orphaned CERTInext order AND (before an earlier round's fix) + // orphaned the 'good' domain's already-published TXT record. Now the bad domain is + // skipped (logged loudly) and the good domain proceeds through the normal DCV lifecycle. + await act.Should().NotThrowAsync(); string goodHostname = string.Format(Constants.Dcv.DefaultTxtRecordTemplate, good); + string badHostname = string.Format(Constants.Dcv.DefaultTxtRecordTemplate, bad); + validator.StagedRecords.Should().ContainSingle( - "only the domain staged before the failure should have a recorded StageValidation call") + "only the domain that did not fail to stage should ever have been staged") .Which.key.Should().Be(goodHostname); validator.CleanedUpKeys.Should().Contain(goodHostname, - "the TXT record already published for the domain that succeeded must be removed " + - "when a later domain in the same order fails to stage — otherwise it is orphaned in " + - "the customer's DNS zone with nothing else in the codebase to ever clean it up"); + "the good domain completes its normal verify-then-cleanup lifecycle"); + validator.CleanedUpKeys.Should().NotContain(badHostname, + "the bad domain was never staged, so there is nothing to clean up for it"); } } } diff --git a/CERTInext.Tests/SanSubmissionTests.cs b/CERTInext.Tests/SanSubmissionTests.cs index 637aa72..a0ec1c8 100644 --- a/CERTInext.Tests/SanSubmissionTests.cs +++ b/CERTInext.Tests/SanSubmissionTests.cs @@ -282,18 +282,23 @@ await plugin.Enroll( /// Union, not either/or: names unique to each source survive and the overlap collapses. /// [Fact] - public async Task GatewayAndCsrSans_AreUnionedAndDeduplicated() + public async Task CsrOnlySans_AreIgnored_WhenGatewaySuppliesAnyEntries() { + // Regression: this test used to assert the CSR was unioned in on top of whatever the + // gateway supplied. Full-review's security lens found that risky: Command's SAN + // dictionary is how an enrollment pattern's SAN policy is expressed, and a signed CSR — + // usually generated by the subscriber's own tooling, not by Command — can legitimately + // carry more names than that policy allows. Unioning them in would re-introduce a name + // the policy excluded. The CSR is now consulted only as a fallback when the gateway + // supplies nothing at all (see CsrSans_ReachAdditionalDomains_WhenGatewaySuppliesNone). var plugin = BuildPlugin(); await plugin.Enroll( - csr: GenerateCsrPem("host.example.com", "shared.example.com", "csronly.example.com"), + csr: GenerateCsrPem("host.example.com", "host.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" } + ["dnsname"] = new[] { "gatewayonly.example.com" } }, productInfo: MakeProductInfo(), requestFormat: RequestFormat.PKCS10, @@ -301,10 +306,9 @@ await plugin.Enroll( 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"); + domains.Should().BeEquivalentTo(new[] { "gatewayonly.example.com" }, + "the gateway supplied a (non-empty) SAN set, so the CSR's own SAN extension must be " + + "ignored entirely, not merged in on top of it"); } /// @@ -551,6 +555,40 @@ public void SubmitNonDnsSans_DefaultsToTrue() new CERTInextConfig().SubmitNonDnsSans.Should().BeTrue(); } + /// + /// Regression for a self-contradicting audit record: BuildSanList used to log "N SAN(s) + /// ... have been added to the order" for CSR-fallback entries, then filter exactly those + /// entries back out two lines later when SubmitNonDnsSans is false — a false claim in the + /// same call. The fix reordered the method to filter first and log the final result, which + /// this test exercises functionally: with the gateway supplying nothing (so the CSR fallback + /// engages) and a non-DNS CSR SAN present, SubmitNonDnsSans=false must still result in that + /// name being genuinely absent from the wire, not merely mis-described in the log. + /// + [Fact] + public async Task CsrFallbackNonDnsSan_IsExcluded_WhenSubmitNonDnsSansFalse() + { + var plugin = new CERTInextCAPlugin( + BuildRealClient(), + new CERTInextConfig { PickupRetries = 0, SubmitNonDnsSans = false }); + + await plugin.Enroll( + csr: GenerateCsrPemWithGeneralNames( + "host.example.com", + new GeneralName(GeneralName.DnsName, "host.example.com"), + new GeneralName(GeneralName.DnsName, "alt.example.com"), + new GeneralName(GeneralName.Rfc822Name, "admin@example.com")), + subject: "CN=host.example.com", + san: null, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "alt.example.com" }, + "the CSR-fallback email SAN must be genuinely absent from the order, not just " + + "misreported as present"); + } + // ======================================================================= // Renew path — previously submitted no SANs at all // ======================================================================= diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index f6102d4..ad3fb2b 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -1664,22 +1664,24 @@ private async Task PerformDcvIfNeededAsync( var stagedValidations = new List<(string domain, string hostname, Keyfactor.AnyGateway.Extensions.IDomainValidator validator)>(); - // Domains for which no DNS provider plugin resolved. Used after the staging loop to tell - // "this one name is unvalidatable" (skip, keep going) apart from "no DNS provider is - // deployed at all" (a gateway misconfiguration that must still fail loudly). - var unresolvedDomains = new List(); + // Domains this pass could not stage, with why — purely for the summary LogError after + // the loop. Every failure mode below is loud (its own LogError, sanitized) before being + // skipped, so nothing here is silent; this list just avoids repeating that detail twice. + var skippedDomains = new List<(string domain, string reason)>(); // Set instead of an immediate `return false` inside the loop below, so a not-yet-ready - // domain that is deferred mid-loop still goes through the same cleanup as every other - // exit path — see the try/catch around the loop. + // deferral goes through the same cleanup as every other exit path — see the try/catch + // around the loop. bool deferToNextSyncCycle = false; // Removes whatever TXT records were already published before an early exit from the - // staging loop (an exception, or a not-yet-ready deferral). Nothing else in this method - // cleans up mid-loop: the try/finally further down only runs once every pending domain - // has been staged, so without this, a later domain's failure orphans every TXT record - // already published for the earlier domains in the *same* order — permanently, since - // nothing else in the codebase calls CleanupValidation for them. + // staging loop. Nothing else in this method cleans up mid-loop: the try/finally further + // down only runs once every pending domain has been staged, so without this, an early + // exit orphans every TXT record already published for the earlier domains in the *same* + // order — permanently, since nothing else in the codebase calls CleanupValidation for + // them. Kept even though every per-domain failure below is now skip-and-continue rather + // than throw: it is the safety net for a genuinely unexpected exception (cancellation, a + // bug, a validator implementation that throws instead of returning a failure result). async Task CleanupPartialStagingAsync() { foreach (var (domain, hostname, validator) in stagedValidations) @@ -1688,13 +1690,13 @@ async Task CleanupPartialStagingAsync() { await validator.CleanupValidation(hostname, ct); _logger.LogInformation( - "DNS TXT record cleaned up after a later domain on the same order could not be " + - "staged. Domain={Domain}, Hostname={Hostname}", domain, hostname); + "DNS TXT record cleaned up after an early exit from DCV staging. " + + "Domain={Domain}, Hostname={Hostname}", domain, hostname); } catch (Exception ex) { _logger.LogWarning(ex, - "Failed to clean up DNS TXT record after a partial staging failure. " + + "Failed to clean up DNS TXT record after an early exit from DCV staging. " + "Domain={Domain}, Hostname={Hostname}. May require manual removal.", domain, hostname); } } @@ -1702,7 +1704,23 @@ async Task CleanupPartialStagingAsync() try { - // Stage DNS TXT records for all pending domains + // Stage DNS TXT records for all pending domains. Every failure below is scoped to + // the one domain that hit it — logged loudly (LogError, so the audit trail carries + // the reason before the domain is dropped) and skipped, never thrown. A throw here + // would abort the WHOLE order after Enroll already placed it at the CA — Enroll has + // no catch around this call, so the exception would escape as a failed enrollment + // with an orphaned CERTInext order, and TryRunDcvDuringSyncAsync would swallow the + // same exception on every later sync retry, leaving the order stuck at + // EXTERNALVALIDATION forever. That is worse than parking the order pending with a + // clear log entry, for EVERY failure shape here — not just the ones distinguishable + // as "bad input" — because nothing downstream ever gets to see or act on the + // exception anyway. This directly caused three real regressions across the first two + // rounds of fixing this file: a GetDcv error or an empty token for a non-DNS SAN + // (submitted on purpose — see BuildSanList) aborted co-tenant DNS domains on the same + // order; a StageValidation failure on domain N+1 orphaned domain N's TXT record; and + // a misconfiguration-detection throw fired on an ordinary non-DNS Subject CN, which + // no setting could prevent since SubmitNonDnsSans only filters the SAN list, not the + // subject. There is no longer a "this must still throw" case in this loop at all. foreach (var (domain, _) in pendingDomains) { GetDcvResponse dcvResp; @@ -1715,28 +1733,38 @@ async Task CleanupPartialStagingAsync() // CERTInext occasionally exposes the DCV slot in TrackOrder (so // domainVerification is populated and dcvStatus="0") before the GetDcv // endpoint will accept calls for that order — observed as EMS-956 - // "Invalid Request for this API" for several hours after enrollment. - // Treat this as "DCV not ready yet": skip the DCV ceremony for now and - // let the sync-driven retry pick it up on a later cycle. We must NOT - // throw, because that would fail the entire Enroll call and prevent the - // gateway from recording the pending order at all. + // "Invalid Request for this API" for several hours after enrollment. This is + // an order-readiness condition, not a per-domain one, so unlike every other + // case in this loop it defers the whole pass rather than skipping one domain. _logger.LogInformation( "GetDcv not yet accepting calls for order {OrderNumber} domain {Domain} ({Error}). " + "Deferring DCV to the next sync cycle.", - orderNumber, domain, ex.Message); + orderNumber, LogSanitizer.Strip(domain), ex.Message); deferToNextSyncCycle = true; break; } catch (Exception ex) { - _logger.LogError(ex, "GetDcv failed for order {OrderNumber} domain {Domain}", orderNumber, domain); - throw; + // Any other GetDcv failure — genuinely unmeasured against the live API for a + // non-DNS order-domain, which is exactly why this must not be allowed to fail + // the whole order on a guess. Skip just this domain. + _logger.LogError(ex, + "GetDcv failed for order {OrderNumber} domain {Domain}; skipping this domain so the " + + "rest of the order can still be validated.", orderNumber, LogSanitizer.Strip(domain)); + skippedDomains.Add((domain, "GetDcv failed")); + continue; } string token = dcvResp.DcvDetails?.Token; if (string.IsNullOrWhiteSpace(token)) - throw new InvalidOperationException( - $"GetDcv returned no token for order '{orderNumber}' domain '{domain}'."); + { + _logger.LogError( + "GetDcv returned no token for order {OrderNumber} domain {Domain}; skipping this " + + "domain so the rest of the order can still be validated.", + orderNumber, LogSanitizer.Strip(domain)); + skippedDomains.Add((domain, "no DCV token returned")); + continue; + } string template = string.IsNullOrWhiteSpace(_config.DcvTxtRecordTemplate) ? Constants.Dcv.DefaultTxtRecordTemplate @@ -1746,25 +1774,16 @@ async Task CleanupPartialStagingAsync() var validator = DomainValidatorFactory.ResolveDomainValidator(domain, "dns-01"); if (validator == null) { - // Two different conditions land here, and they want different handling: - // - // * This *particular* name is unvalidatable while others on the order are fine — - // an IP-literal SAN is the canonical case (it satisfies the FQDN regex above, - // but no DNS zone can ever match it). Skip it, so its co-tenant domains still - // get staged. Throwing would fail the whole Enroll after the order was already - // placed and leave the order permanently unable to progress. - // - // * No DNS provider is deployed/configured at all — a real gateway - // misconfiguration. That is still raised, after the loop, when nothing on the - // order staged and no domain anywhere on the order resolves a provider (see - // below), preserving the loud operator-facing failure. + // The canonical case: an IP-literal SAN (or a non-DNS Subject CN) satisfies + // the FQDN regex above but no DNS zone can ever match it. _logger.LogError( - "No DNS provider plugin resolved for domain '{Domain}' on order {OrderNumber}. " + - "If this is a real domain, ensure the appropriate DNS provider plugin is deployed and " + - "configured on the gateway; if it came from a non-DNS SAN (e.g. an IP address), remove " + - "it from the request.", + "No DNS provider plugin resolved for domain '{Domain}' on order {OrderNumber}; " + + "skipping this domain so the rest of the order can still be validated. If this is " + + "a real domain, ensure the appropriate DNS provider plugin is deployed and " + + "configured on the gateway; if it came from a non-DNS SAN (e.g. an IP address) or a " + + "non-DNS Subject CN, remove it from the request.", LogSanitizer.Strip(domain), orderNumber); - unresolvedDomains.Add(domain); + skippedDomains.Add((domain, "no DNS provider resolved")); continue; } @@ -1772,16 +1791,38 @@ async Task CleanupPartialStagingAsync() "Staging DNS TXT record for DCV. OrderNumber={OrderNumber}, Domain={Domain}, Hostname={Hostname}", orderNumber, domain, hostname); - var stageResult = await validator.StageValidation(hostname, token, ct); + DomainValidationResult stageResult; + try + { + stageResult = await validator.StageValidation(hostname, token, ct); + } + catch (Exception ex) + { + _logger.LogError(ex, + "DNS provider plugin threw while staging '{Domain}' for order {OrderNumber}; " + + "skipping this domain so the rest of the order can still be validated.", + LogSanitizer.Strip(domain), orderNumber); + skippedDomains.Add((domain, "DNS provider plugin threw")); + continue; + } + if (!stageResult.Success) - throw new InvalidOperationException( - $"Failed to stage DNS validation for '{domain}': {stageResult.ErrorMessage}"); + { + _logger.LogError( + "Failed to stage DNS validation for '{Domain}' on order {OrderNumber}: {Error}. " + + "Skipping this domain so the rest of the order can still be validated.", + LogSanitizer.Strip(domain), orderNumber, LogSanitizer.Strip(stageResult.ErrorMessage)); + skippedDomains.Add((domain, $"stage failed: {stageResult.ErrorMessage}")); + continue; + } stagedValidations.Add((domain, hostname, validator)); } } catch { + // Nothing in the loop above throws for a per-domain reason any more — this is the + // safety net for a genuinely unexpected failure (cancellation, a bug). await CleanupPartialStagingAsync(); throw; } @@ -1792,27 +1833,13 @@ async Task CleanupPartialStagingAsync() return false; } - // Nothing staged, at least one pending domain failed provider resolution, and — this is - // the part that must hold across the WHOLE order, not just the still-pending domains — - // no domain anywhere on the order, pending or already validated, resolves a provider - // either. That combination means this gateway has no usable DNS provider deployed at - // all: a real deployment always has at least one legitimate domain somewhere on the - // order. That is a misconfiguration, not bad request data, so it still fails loudly - // instead of silently parking the order. - // - // Checking only pendingDomains here was the bug: CERTInext can cache a prior DCV - // validation for a parent domain (see the aggregate/per-domain check earlier in this - // method), which removes the CN from pendingDomains even though it resolves a provider - // just fine. A prior version of this check assumed the CN is always pending, which this - // method's own cached-validation branch above proves false — so it threw on an ordinary - // non-DNS SAN sharing an order with an already-validated domain, reopening the exact - // orphaned-order failure this whole restructuring exists to prevent. - if (stagedValidations.Count == 0 && unresolvedDomains.Count > 0 - && !allDomainEntries.Keys.Any(d => DomainValidatorFactory.ResolveDomainValidator(d, "dns-01") != null)) + if (skippedDomains.Count > 0) { - throw new InvalidOperationException( - $"No DNS provider plugin is configured for domain '{unresolvedDomains[0]}'. " + - "Ensure the appropriate DNS provider plugin is deployed and configured on the gateway."); + _logger.LogError( + "{Count} domain(s) on order {OrderNumber} could not be staged for DCV and were skipped: " + + "[{Domains}]. This order cannot be issued by CERTInext until they are resolved.", + skippedDomains.Count, orderNumber, + LogSanitizer.Strip(string.Join(", ", skippedDomains.Select(d => $"{d.domain} ({d.reason})")))); } if (stagedValidations.Count == 0) @@ -2308,25 +2335,25 @@ private static int MapRevocationReasonStringToCode(string reason) } /// - /// 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. + /// Builds the list submitted to CERTInext: the multi-valued SAN + /// dictionary the AnyCA gateway hands us, falling back to the subjectAltName extension + /// carried inside the CSR itself only when the gateway supplies nothing at all. /// - /// 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. + /// Fallback, not union, deliberately: Command's SAN dictionary is the channel through + /// which an enrollment pattern's SAN policy is expressed for this request, and a signed + /// CSR — typically generated by the subscriber's own tooling, not by Command — can + /// legitimately carry more names than that policy allows. Unioning them in would + /// re-introduce a name the policy excluded. The CSR is only consulted when the dictionary + /// is empty, which is the one case where there is no policy-derived set to defer to. /// - /// Parsing the CSR here is not redundant with sending the CSR to CERTInext. - /// 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. + /// The CSR still matters even though CERTInext ignores its subjectAltName extension + /// outright — measured on the US sandbox in SanSubmissionProbeTests: a CSR + /// carrying two DNS names, submitted with additionalDomains omitted, produced an + /// order with only the CN registered. Production behaves the same way: the customer + /// report that prompted this fix was a production UCC order whose CSR carried the SANs + /// and whose issued certificate held only the CN. So on whichever path populates the + /// gateway dictionary — or, in the fallback case, the CSR — this method is the only way + /// those names reach additionalDomains and therefore the certificate. /// /// History (UCC SANs silently dropped): the gateway keys this dictionary /// dnsname, not dns. did not recognize @@ -2341,13 +2368,19 @@ private List BuildSanList(Dictionary san, string csr // Type+value identity, so the same name requested as two different SAN types is // preserved while an exact repeat across the two sources collapses. var seen = new HashSet(StringComparer.OrdinalIgnoreCase); + // "type|value" keys of entries that came from the CSR fallback, not the gateway + // dictionary — used only to word the provenance log accurately once the final, + // possibly-filtered result is known (see below). + var fromCsrKeys = new HashSet(StringComparer.OrdinalIgnoreCase); - void Add(string type, string value) + void Add(string type, string value, bool fromCsr = false) { if (string.IsNullOrWhiteSpace(value)) return; string trimmed = value.Trim(); - if (!seen.Add($"{type}|{trimmed}")) return; + string key = $"{type}|{trimmed}"; + if (!seen.Add(key)) return; result.Add(new SanEntry { Type = type, Value = trimmed }); + if (fromCsr) fromCsrKeys.Add(key); } // AnyCA passes SANs keyed by type name — the real gateway uses "dnsname", @@ -2366,13 +2399,15 @@ void Add(string type, string value) 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. - var csrSans = ExtractSanEntriesFromCsr(csr, out List skippedCsrTags); - foreach (var csrSan in csrSans) - Add(csrSan.Type, csrSan.Value); - - int fromCsrOnly = result.Count - fromGateway; + // CSR fallback — only when the gateway supplied nothing. Non-throwing: a malformed + // or unparseable CSR yields an empty list, and this whole block is then a no-op. + var skippedCsrTags = new List(); + if (fromGateway == 0) + { + var csrSans = ExtractSanEntriesFromCsr(csr, out skippedCsrTags); + foreach (var csrSan in csrSans) + Add(csrSan.Type, csrSan.Value, fromCsr: true); + } if (skippedCsrTags.Count > 0) { @@ -2396,25 +2431,6 @@ void Add(string type, string value) 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, - LogSanitizer.Strip(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, @@ -2430,33 +2446,60 @@ void Add(string type, string value) // The warning below therefore describes the sandbox outcome as the expected one // without promising it: either way the operator is told which SANs are the problem, // which is the part that matters for diagnosis. + // + // This filtering runs BEFORE any of the logging below, and all of that logging is + // computed from `result` as it stands afterward — not from the pre-filter set. A + // prior version of this method logged "resolved" and "added to the order" against the + // pre-filter set and only THEN applied this filter, so with SubmitNonDnsSans=false the + // audit trail could claim a SAN was added when it had in fact just been dropped two + // lines later — a self-contradicting record for the same enrollment. The fix is + // ordering, not new logic: decide what is actually being submitted first, describe + // that. var nonDns = result.Where(s => !string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)).ToList(); - if (nonDns.Count > 0) + if (nonDns.Count > 0 && !_config.SubmitNonDnsSans) { - string nonDnsRendered = LogSanitizer.Strip(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}"))); + _logger.LogWarning( + "{Count} requested SAN(s) are not DNS names and are being DROPPED because " + + "SubmitNonDnsSans is false: {Sans}. The order will issue, but the certificate will " + + "NOT contain these names. Set SubmitNonDnsSans back to true to submit them and have " + + "CERTInext surface the problem instead.", + nonDns.Count, LogSanitizer.Strip(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}")))); - // Escape hatch (SubmitNonDnsSans, default true). On a host that was previously - // issuing certificates for requests carrying an IP or email SAN, the default - // behaviour flips those enrollments from "issues, silently missing the name" to - // "parks pending", and an operator needs a way back that isn't a plugin downgrade. - // Off => pre-1.0.1 behaviour: drop them and issue. - if (!_config.SubmitNonDnsSans) - { - _logger.LogWarning( - "{Count} requested SAN(s) are not DNS names and are being DROPPED because " + - "SubmitNonDnsSans is false: {Sans}. The order will issue, but the certificate will " + - "NOT contain these names. Set SubmitNonDnsSans back to true to submit them and have " + - "CERTInext surface the problem instead.", - nonDns.Count, nonDnsRendered); + result = result + .Where(s => string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)) + .ToList(); + nonDns = new List(); - result = result - .Where(s => string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)) - .ToList(); + if (result.Count == 0) + return null; + } - return result.Count > 0 ? result : null; - } + // The blind spot that hid the original defect was that nothing logged what we + // resolved. Log the final, post-filter resolved set and its provenance at Information. + int fromCsrKept = result.Count(s => fromCsrKeys.Contains($"{s.Type}|{s.Value}")); + _logger.LogInformation( + "Resolved {Total} SAN(s) for submission. FromGatewayRequest={FromGateway}, " + + "AddedFromCsrFallback={FromCsr}, Sans={Sans}", + result.Count, fromGateway, fromCsrKept, + LogSanitizer.Strip(string.Join("; ", result.Select(s => $"{s.Type}:{s.Value}")))); + if (fromCsrKept > 0) + { + // Worth a Warning, not Debug: it means Command handed us no SAN data at all for + // this enrollment, which is a gateway/template wiring smell even though the CSR + // fallback recovers it here. + _logger.LogWarning( + "Command supplied no SAN data for this enrollment; {Count} SAN(s) present in the CSR " + + "have been added to the order instead. Review the enrollment pattern / template SAN " + + "configuration.", + fromCsrKept); + } + + if (nonDns.Count > 0) + { + // Reaching this line means SubmitNonDnsSans is true (the false case already + // returned above), so these are being submitted, not dropped. _logger.LogWarning( "{Count} requested SAN(s) are not DNS names: {Sans}. CERTInext's additionalDomains " + "field takes domain names, so this order will either be rejected outright or be " + @@ -2465,7 +2508,7 @@ void Add(string type, string value) "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, nonDnsRendered); + nonDns.Count, LogSanitizer.Strip(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}")))); } return result; diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index 8a3572a..7c30c7f 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -804,7 +804,7 @@ public async Task RenewCertificateAsync( "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); + certificateId, LogSanitizer.Strip(renewalDomainName)); } // We don't have the product code from TrackOrder — build an order using From e7715c9622a8f937ccf37eb86d9a799af8ffc514 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:57:35 -0700 Subject: [PATCH 06/14] =?UTF-8?q?fix(enroll):=20address=20full-review=20ro?= =?UTF-8?q?und=204=20=E2=80=94=20regex=20$=20quirk,=20cancellation,=20CSR-?= =?UTF-8?q?fallback=20edge=20case?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CERTInext.Tests/CERTInextCAPluginDcvTests.cs | 85 +++++++++++++++++++ CERTInext.Tests/SanSubmissionTests.cs | 33 ++++++++ CERTInext/CERTInextCAPlugin.cs | 89 +++++++++++++++----- 3 files changed, 188 insertions(+), 19 deletions(-) diff --git a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs index 98f7382..4286f39 100644 --- a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs @@ -1015,6 +1015,91 @@ public async Task Dcv_NonFqdnPendingDomain_IsSkipped_AndValidDomainStillStaged() result.Status.Should().Be((int)EndEntityStatus.GENERATED); } + /// + /// Regression: the FQDN validation regex used ^...$ , and in .NET's default (non-Multiline) + /// mode $ matches immediately before a single trailing '\n', not only at the true end of the + /// string. A domain value ending in '\n' therefore passed as "valid" and reached several log + /// sinks unsanitized further down this same method — a CWE-117 log-injection route into the + /// DCV audit trail, reachable via any order visible through Synchronize/GetSingleRecord (not + /// just ones this plugin's own Enroll call placed, since TrackOrder's domainVerification keys + /// for an externally-created order are never trimmed by this plugin). The regex now anchors + /// with \A/\z, which are absolute string-start/end regardless of trailing newlines. + /// + [Fact] + public async Task Dcv_DomainWithTrailingNewline_IsRejectedAsInvalid_AndValidDomainStillStaged() + { + const string order = MockCertificateData.DcvOrderId; + const string good = MockCertificateData.DcvDomain; + const string bad = "evil.example.com\n"; + + var mock = NewMock(); + + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); + + mock.SetupSequence(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, good, bad)) + .ReturnsAsync(MockCertificateData.DcvVerifiedTrackResponse(order, good)); + + // MockBehavior.Strict: an unexpected GetDcv call for `bad` fails the test on its own — + // if the regex fix regressed, this domain would reach GetDcv instead of being rejected + // by the FQDN check before the staging loop even starts. + mock.Setup(c => c.GetDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse(MockCertificateData.DcvToken)); + mock.Setup(c => c.VerifyDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .Returns(Task.CompletedTask); + mock.Setup(c => c.GetCertificateAsync(order, It.IsAny())) + .ReturnsAsync(MockCertificateData.IssuedCertRecord(order)); + + var validator = new FakeDomainValidator(); + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator), + DcvConfig(dcvWaitForIssuanceSeconds: 10)); + + var result = await Enroll(plugin); + + string expectedHostname = string.Format(Constants.Dcv.DefaultTxtRecordTemplate, good); + validator.StagedRecords.Should().ContainSingle( + "the valid domain must still be staged even though a co-tenant domain carries a " + + "trailing newline") + .Which.Should().Be((expectedHostname, MockCertificateData.DcvToken)); + + mock.Verify(c => c.GetDcvAsync(order, bad, It.IsAny(), It.IsAny()), + Times.Never, "a domain with a trailing newline must never be sent to GetDcv"); + result.Status.Should().Be((int)EndEntityStatus.GENERATED); + } + + /// + /// Regression: the generic per-domain catch blocks around GetDcvAsync and StageValidation + /// used to catch OperationCanceledException along with genuine GetDcv/DNS-provider failures, + /// logging and skipping the domain as an ordinary per-domain failure. A cancellation (the + /// shared DcvTimeoutMinutes-bound token expiring mid-loop) is not that — it must propagate to + /// the outer catch instead, which is the only place that logs it correctly and is the + /// intended timeout-handling path documented at the top of this method's DCV timeout setup. + /// + [Fact] + public async Task Dcv_CancellationDuringGetDcv_PropagatesRatherThanBeingSkippedAsPerDomainFailure() + { + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = MockCertificateData.DcvOrderId, Status = "pending_dcv" }); + + mock.Setup(c => c.TrackOrderAsync(MockCertificateData.DcvOrderId, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvPendingTrackResponse()); + + mock.Setup(c => c.GetDcvAsync(MockCertificateData.DcvOrderId, MockCertificateData.DcvDomain, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ThrowsAsync(new OperationCanceledException("DCV timeout budget exceeded")); + + var validator = new FakeDomainValidator(); + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator)); + + Func act = () => Enroll(plugin); + + // Must propagate as a cancellation, not be swallowed and reported as "GetDcv failed" in + // the skipped-domains summary while Enroll completes normally. + await act.Should().ThrowAsync(); + } + /// /// A pending domain that resolves no DNS provider (an IP-literal SAN passes the FQDN regex /// but no zone can match it) must likewise be skipped rather than failing the whole order. diff --git a/CERTInext.Tests/SanSubmissionTests.cs b/CERTInext.Tests/SanSubmissionTests.cs index a0ec1c8..541fde3 100644 --- a/CERTInext.Tests/SanSubmissionTests.cs +++ b/CERTInext.Tests/SanSubmissionTests.cs @@ -311,6 +311,39 @@ await plugin.Enroll( "ignored entirely, not merged in on top of it"); } + /// + /// Regression: the CSR-fallback trigger used to be "the gateway dictionary computed to zero + /// added entries", which cannot distinguish "Command never populated SAN data" (the case the + /// fallback exists for) from "Command's enrollment pattern ran and deliberately computed + /// zero SANs for this request" (an explicit policy decision this plugin must respect). A + /// non-null dictionary whose only key maps to an empty array is the latter — the fallback + /// must not engage, even though it computes to the same "0 SANs added" outcome as a null + /// dictionary would. + /// + [Fact] + public async Task CsrSans_AreIgnored_WhenGatewaySuppliesNonNullDictWithOnlyEmptyValues() + { + var plugin = BuildPlugin(); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com", "host.example.com", "csronly.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + // Non-null dictionary, but the key maps to no values — computes to zero added + // SANs, same as san == null would, but it must NOT be treated the same way. + ["dnsname"] = Array.Empty() + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()).Should().BeNull( + "a non-null gateway SAN dictionary that computes to zero entries must be respected " + + "as Command's own decision, not treated as 'Command supplied nothing' and " + + "backfilled from the CSR"); + } + /// /// The CN is already submitted as domainName; repeating it in additionalDomains is /// suppressed. CERTInext collapses it anyway (measured), so this keeps the body matching diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index ad3fb2b..c44ea74 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -247,14 +247,14 @@ public void Initialize(IAnyCAPluginConfigProvider configProvider, ICertificateDa "ApiKeyPresent={ApiKeyPresent}, UsernamePresent={UsernamePresent}, " + "PasswordPresent={PasswordPresent}, OAuth2ClientIdPresent={OAuth2ClientIdPresent}, " + "OAuth2ClientSecretPresent={OAuth2ClientSecretPresent}, OAuth2TokenUrlPresent={OAuth2TokenUrlPresent}, " + - "PageSize={PageSize}, IgnoreExpired={IgnoreExpired}, " + + "PageSize={PageSize}, IgnoreExpired={IgnoreExpired}, SubmitNonDnsSans={SubmitNonDnsSans}, " + "DcvEnabled={DcvEnabled}, DcvTxtRecordTemplate={DcvTxtRecordTemplate}, " + "DomainValidatorFactoryInjected={FactoryInjected}", _config.ApiUrl, _config.AuthMode, _config.Enabled, hasApiKey, hasUsername, hasPassword, hasClientId, hasClientSecret, hasTokenUrl, - _config.PageSize, _config.IgnoreExpired, + _config.PageSize, _config.IgnoreExpired, _config.SubmitNonDnsSans, _config.DcvEnabled, _config.DcvTxtRecordTemplate, _domainValidatorFactory != null); @@ -1631,10 +1631,17 @@ private async Task PerformDcvIfNeededAsync( { string domain = entry.Key; - // Allow standard FQDN characters plus wildcard prefix (*.example.com) + // Allow standard FQDN characters plus wildcard prefix (*.example.com). + // + // \A/\z, not ^/$: in .NET's default (non-Multiline) mode, $ matches immediately + // before a single trailing '\n', not only at the true end of the string — so + // "evil.com\n" passes a ^...$ version of this regex. \A and \z are absolute + // start/end-of-string anchors regardless of RegexOptions, so a value with any + // trailing control character is correctly rejected here rather than reaching the + // unsanitized-looking-safe domain this validation exists to guarantee. bool valid = !string.IsNullOrWhiteSpace(domain) && System.Text.RegularExpressions.Regex.IsMatch( - domain, @"^(\*\.)?[a-zA-Z0-9]([a-zA-Z0-9\-\.]*[a-zA-Z0-9])?$"); + domain, @"\A(\*\.)?[a-zA-Z0-9]([a-zA-Z0-9\-\.]*[a-zA-Z0-9])?\z"); if (valid) validPendingDomains.Add(entry); @@ -1691,13 +1698,15 @@ async Task CleanupPartialStagingAsync() await validator.CleanupValidation(hostname, ct); _logger.LogInformation( "DNS TXT record cleaned up after an early exit from DCV staging. " + - "Domain={Domain}, Hostname={Hostname}", domain, hostname); + "Domain={Domain}, Hostname={Hostname}", + LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); } catch (Exception ex) { _logger.LogWarning(ex, "Failed to clean up DNS TXT record after an early exit from DCV staging. " + - "Domain={Domain}, Hostname={Hostname}. May require manual removal.", domain, hostname); + "Domain={Domain}, Hostname={Hostname}. May require manual removal.", + LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); } } } @@ -1743,6 +1752,15 @@ async Task CleanupPartialStagingAsync() deferToNextSyncCycle = true; break; } + catch (OperationCanceledException) + { + // The shared, DcvTimeoutMinutes-bound cancellation firing mid-loop. This is + // NOT a per-domain CA/DNS-provider failure — it must not be caught by the + // generic clause below, which would mislabel it as "GetDcv failed" for + // whichever domain happened to be in flight and send an operator chasing the + // wrong cause. Propagate to the outer catch, which logs and cleans up. + throw; + } catch (Exception ex) { // Any other GetDcv failure — genuinely unmeasured against the live API for a @@ -1789,13 +1807,19 @@ async Task CleanupPartialStagingAsync() _logger.LogInformation( "Staging DNS TXT record for DCV. OrderNumber={OrderNumber}, Domain={Domain}, Hostname={Hostname}", - orderNumber, domain, hostname); + orderNumber, LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); DomainValidationResult stageResult; try { stageResult = await validator.StageValidation(hostname, token, ct); } + catch (OperationCanceledException) + { + // Same reasoning as the GetDcv cancellation catch above: not a per-domain + // failure, must reach the outer catch rather than the generic clause below. + throw; + } catch (Exception ex) { _logger.LogError(ex, @@ -1819,10 +1843,20 @@ async Task CleanupPartialStagingAsync() stagedValidations.Add((domain, hostname, validator)); } } - catch + catch (Exception ex) { // Nothing in the loop above throws for a per-domain reason any more — this is the - // safety net for a genuinely unexpected failure (cancellation, a bug). + // safety net for a genuinely unexpected failure: cancellation (the shared + // DcvTimeoutMinutes-bound token expiring mid-loop — explicitly re-thrown past the + // per-domain catches above rather than mislabeled as a per-domain failure) or a bug. + // Log before rethrowing: neither caller (EnrollNewAsync's try/finally, or Enroll + // itself) adds a catch, so without a log line here an unanticipated failure on the + // synchronous Enroll-time DCV path would leave no plugin-emitted record at all + // identifying the order or cause — only whatever the gateway host's own unhandled- + // exception logging happens to capture. + _logger.LogError(ex, + "Unexpected failure during DCV staging for order {OrderNumber}; cleaning up any " + + "already-staged TXT records before this propagates.", orderNumber); await CleanupPartialStagingAsync(); throw; } @@ -1860,7 +1894,8 @@ async Task CleanupPartialStagingAsync() foreach (var (domain, hostname, _) in stagedValidations) { _logger.LogInformation( - "Triggering CERTInext DCV verification. OrderNumber={OrderNumber}, Domain={Domain}", orderNumber, domain); + "Triggering CERTInext DCV verification. OrderNumber={OrderNumber}, Domain={Domain}", + orderNumber, LogSanitizer.Strip(domain)); await _client.VerifyDcvAsync(orderNumber, domain, Constants.Dcv.MethodDnsTxt, ct); } @@ -1878,12 +1913,14 @@ async Task CleanupPartialStagingAsync() { await validator.CleanupValidation(hostname, ct); _logger.LogInformation( - "DNS TXT record cleaned up. Domain={Domain}, Hostname={Hostname}", domain, hostname); + "DNS TXT record cleaned up. Domain={Domain}, Hostname={Hostname}", + LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); } catch (Exception ex) { _logger.LogWarning(ex, - "Failed to clean up DNS TXT record. Domain={Domain}, Hostname={Hostname}", domain, hostname); + "Failed to clean up DNS TXT record. Domain={Domain}, Hostname={Hostname}", + LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); } } } @@ -2012,7 +2049,8 @@ private async Task WaitForDcvVerificationAsync(string orderNumber, IReadOnlyList "DCV verification poll exceeded its internal deadline ({Minutes}min). " + "OrderNumber={OrderNumber}, StillPendingDomains=[{Pending}]. " + "Exiting and leaving TXT records for the caller's finally block to clean up.", - _config.GetEffectiveDcvTimeoutMinutes(), orderNumber, string.Join(",", pending)); + _config.GetEffectiveDcvTimeoutMinutes(), orderNumber, + LogSanitizer.Strip(string.Join(",", pending))); return; } @@ -2045,12 +2083,14 @@ private async Task WaitForDcvVerificationAsync(string orderNumber, IReadOnlyList if (string.Equals(detail.DcvStatus, Constants.Dcv.StatusValidated, StringComparison.Ordinal)) { - _logger.LogInformation("DCV verified by CERTInext. OrderNumber={OrderNumber}, Domain={Domain}", orderNumber, domain); + _logger.LogInformation("DCV verified by CERTInext. OrderNumber={OrderNumber}, Domain={Domain}", + orderNumber, LogSanitizer.Strip(domain)); pending.Remove(domain); } else if (string.Equals(detail.DcvStatus, Constants.Dcv.StatusRejected, StringComparison.Ordinal)) { - _logger.LogWarning("DCV rejected by CERTInext. OrderNumber={OrderNumber}, Domain={Domain}", orderNumber, domain); + _logger.LogWarning("DCV rejected by CERTInext. OrderNumber={OrderNumber}, Domain={Domain}", + orderNumber, LogSanitizer.Strip(domain)); pending.Remove(domain); } } @@ -2344,7 +2384,10 @@ private static int MapRevocationReasonStringToCode(string reason) /// CSR — typically generated by the subscriber's own tooling, not by Command — can /// legitimately carry more names than that policy allows. Unioning them in would /// re-introduce a name the policy excluded. The CSR is only consulted when the dictionary - /// is empty, which is the one case where there is no policy-derived set to defer to. + /// argument is null — not merely empty or all-empty-arrays. A non-null dictionary, + /// even one that computes to zero names, means Command's enrollment pattern ran and + /// deliberately produced no SANs for this request; only its literal absence means no + /// policy-derived set exists to defer to. /// /// The CSR still matters even though CERTInext ignores its subjectAltName extension /// outright — measured on the US sandbox in SanSubmissionProbeTests: a CSR @@ -2399,10 +2442,18 @@ void Add(string type, string value, bool fromCsr = false) int fromGateway = result.Count; - // CSR fallback — only when the gateway supplied nothing. Non-throwing: a malformed - // or unparseable CSR yields an empty list, and this whole block is then a no-op. + // CSR fallback — only when the gateway dictionary is itself absent (san == null), NOT + // merely "computed to zero SAN entries" (fromGateway == 0). Those are different things: + // a non-null dictionary — even an empty one, or one whose keys all map to empty arrays + // — means Command's enrollment pattern ran and deliberately produced no SANs for this + // request, which the CSR fallback must respect rather than override. san == null means + // Command never populated SAN data for this enrollment path at all, which is the one + // case this fallback exists for. Checking fromGateway instead of san itself would let an + // enrollment pattern that explicitly computes zero SANs still have CSR-derived names + // spliced back in — reopening the policy-reintroduction risk the fallback-over-union + // redesign exists to close. var skippedCsrTags = new List(); - if (fromGateway == 0) + if (san == null) { var csrSans = ExtractSanEntriesFromCsr(csr, out skippedCsrTags); foreach (var csrSan in csrSans) From d69c4cdcb35a92ccd2cc9c662b848dbb7e4f120f Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:23:53 -0700 Subject: [PATCH 07/14] =?UTF-8?q?fix(enroll):=20address=20full-review=20ro?= =?UTF-8?q?und=205=20=E2=80=94=20cancellation=20swallowed=20by=20RestSharp?= =?UTF-8?q?,=20unsanitized=20DomainName=20log?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CERTInext.Tests/CERTInextClientTests.cs | 36 ++++++++++++++ CERTInext/CERTInextCAPlugin.cs | 64 ++++++++++++------------- CERTInext/Client/CERTInextClient.cs | 21 +++++++- 3 files changed, 87 insertions(+), 34 deletions(-) diff --git a/CERTInext.Tests/CERTInextClientTests.cs b/CERTInext.Tests/CERTInextClientTests.cs index e473e89..a0ade72 100644 --- a/CERTInext.Tests/CERTInextClientTests.cs +++ b/CERTInext.Tests/CERTInextClientTests.cs @@ -790,6 +790,42 @@ await act.Should().ThrowAsync() .WithMessage("*GetDcv failed*"); } + /// + /// Regression: this client is built with ThrowOnAnyError=false, so RestSharp catches a + /// cancelled HttpClient.SendAsync internally and returns a non-throwing, unsuccessful + /// RestResponse instead of propagating OperationCanceledException. Before this fix, + /// ExecuteWithRetryAsync passed that response straight to DeserializeOrThrow, which wrapped + /// it in a plain Exception — indistinguishable from a genuine API failure. A caller such as + /// PerformDcvIfNeededAsync's per-domain "catch (OperationCanceledException) { throw; }" guard + /// (added specifically to stop a DCV timeout from being mislabeled as an ordinary per-domain + /// failure) could never actually see the real cancellation, because it never arrived as + /// OperationCanceledException in the first place — a gap a Moq-level test of the plugin alone + /// cannot expose, since a mock can be told to throw whatever type is asked for. This test + /// exercises the real client against a real (if local) HTTP call, which is the only way to + /// pin the actual failure mode. + /// + [Fact] + public async Task GetDcvAsync_ThrowsOperationCanceled_WhenCancellationTokenIsCancelled() + { + _server + .Given(Request.Create().WithPath("/GetDcv").UsingPost()) + .RespondWith(Response.Create() + .WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody(MockCertificateData.GetDcvSuccessJson())); + + var client = BuildClient(); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + Func act = () => client.GetDcvAsync( + MockCertificateData.OrderNumber1, "example.com", Constants.Dcv.MethodDnsTxt, cts.Token); + + await act.Should().ThrowAsync( + "a cancelled token must surface as a genuine cancellation, not get wrapped into a " + + "plain Exception that a caller's cancellation-specific catch clause cannot recognize"); + } + [Fact] public async Task GetDcvAsync_Throws_WhenServerReturns401() { diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index c44ea74..dea7003 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -1689,25 +1689,39 @@ private async Task PerformDcvIfNeededAsync( // them. Kept even though every per-domain failure below is now skip-and-continue rather // than throw: it is the safety net for a genuinely unexpected exception (cancellation, a // bug, a validator implementation that throws instead of returning a failure result). + // + // Shares its per-entry cleanup logic with the try/finally's own cleanup loop further + // down via CleanupOneStagedValidation — the two call sites differ only in when they run + // (an early exit here vs. always-run-at-the-end there), not in what "clean up one TXT + // record" means. async Task CleanupPartialStagingAsync() { - foreach (var (domain, hostname, validator) in stagedValidations) + foreach (var entry in stagedValidations) + await CleanupOneStagedValidationAsync(entry, " after an early exit from DCV staging"); + } + + // Shared by CleanupPartialStagingAsync above and the try/finally's own cleanup loop + // below — both mean "remove one already-published TXT record", just at different times + // (an early exit vs. always-run-at-the-end). `context` distinguishes the two in the log + // text without duplicating the try/catch/log structure itself. + async Task CleanupOneStagedValidationAsync( + (string domain, string hostname, Keyfactor.AnyGateway.Extensions.IDomainValidator validator) entry, + string context) + { + var (domain, hostname, validator) = entry; + try { - try - { - await validator.CleanupValidation(hostname, ct); - _logger.LogInformation( - "DNS TXT record cleaned up after an early exit from DCV staging. " + - "Domain={Domain}, Hostname={Hostname}", - LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); - } - catch (Exception ex) - { - _logger.LogWarning(ex, - "Failed to clean up DNS TXT record after an early exit from DCV staging. " + - "Domain={Domain}, Hostname={Hostname}. May require manual removal.", - LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); - } + await validator.CleanupValidation(hostname, ct); + _logger.LogInformation( + "DNS TXT record cleaned up{Context}. Domain={Domain}, Hostname={Hostname}", + context, LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); + } + catch (Exception ex) + { + _logger.LogWarning(ex, + "Failed to clean up DNS TXT record{Context}. Domain={Domain}, Hostname={Hostname}. " + + "May require manual removal.", + context, LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); } } @@ -1907,22 +1921,8 @@ async Task CleanupPartialStagingAsync() finally { // Always clean up staged DNS records — even on failure - foreach (var (domain, hostname, validator) in stagedValidations) - { - try - { - await validator.CleanupValidation(hostname, ct); - _logger.LogInformation( - "DNS TXT record cleaned up. Domain={Domain}, Hostname={Hostname}", - LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); - } - catch (Exception ex) - { - _logger.LogWarning(ex, - "Failed to clean up DNS TXT record. Domain={Domain}, Hostname={Hostname}", - LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); - } - } + foreach (var entry in stagedValidations) + await CleanupOneStagedValidationAsync(entry, ""); } return true; diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index 7c30c7f..cf6a4da 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -259,7 +259,8 @@ public async Task PlaceOrderAsync( "PlaceOrder received no usable response (DomainName={Domain}, HttpStatus={Status}, LatencyMs={Latency}). " + "Not retrying to avoid a duplicate order (EMS-947). If CERTInext created the order it " + "will be imported by the next synchronization.", - request.OrderDetails?.CertificateInformation?.DomainName, (int)resp.StatusCode, sw.ElapsedMilliseconds); + LogSanitizer.Strip(request.OrderDetails?.CertificateInformation?.DomainName), + (int)resp.StatusCode, sw.ElapsedMilliseconds); throw new Exception( "CERTInext did not return a usable response to the order submission. If the order was " + "created it will be imported by the next synchronization — do not resubmit immediately. " + @@ -309,7 +310,9 @@ public async Task PlaceOrderAsync( "PlaceOrder classified {ErrorCode} as a duplicate transaction (not a hard failure). " + "DomainName={Domain}, Path={Path}, HttpStatus={Status}, LatencyMs={Latency}. If an order exists " + "for this transaction it will be imported by the next synchronization.", - result.Meta.ErrorCode, request.OrderDetails?.CertificateInformation?.DomainName, Constants.Api.GenerateOrderSslPath, (int)resp.StatusCode, sw.ElapsedMilliseconds); + result.Meta.ErrorCode, + LogSanitizer.Strip(request.OrderDetails?.CertificateInformation?.DomainName), + Constants.Api.GenerateOrderSslPath, (int)resp.StatusCode, sw.ElapsedMilliseconds); throw new Exception( "CERTInext reported a duplicate order transaction (EMS-947). If an order was created " + "for this transaction it will be imported by the next synchronization — do not resubmit " + @@ -1331,6 +1334,20 @@ private async Task ExecuteWithRetryAsync( { resp = await _http.ExecuteAsync(req, ct); + // This client is built with ThrowOnAnyError=false (see the constructor), so a + // cancelled ct does not surface as OperationCanceledException from ExecuteAsync — + // RestSharp catches HttpClient.SendAsync's cancellation internally and returns a + // non-throwing, unsuccessful RestResponse instead. Left unchecked, that response + // reaches DeserializeOrThrow and becomes a plain Exception indistinguishable from a + // genuine API failure — which is exactly how a caller such as + // PerformDcvIfNeededAsync's shared DCV-timeout cancellation was still landing in a + // generic "GetDcv failed" per-domain catch instead of the cancellation-specific one, + // even after that method was hardened to re-throw a real OperationCanceledException + // past its per-domain catches. Surface the true cancellation here, at the one place + // in the client that actually holds `ct`, before any retry or error-wrapping logic + // sees the response. + ct.ThrowIfCancellationRequested(); + // Success or 4xx client error — return immediately bool isClientError = (int)resp.StatusCode >= 400 && (int)resp.StatusCode < 500; if (resp.IsSuccessful || isClientError) From 8fb4e88c9037bedd1c4f3276028505428c11abbd Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:55:41 -0700 Subject: [PATCH 08/14] =?UTF-8?q?fix(enroll):=20address=20full-review=20ro?= =?UTF-8?q?und=206=20=E2=80=94=20cleanup=20reuses=20cancelled=20token,=20n?= =?UTF-8?q?o=20correlation=20ID?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CERTInext.Tests/CERTInextCAPluginDcvTests.cs | 73 ++++++++++++++++++++ CERTInext/CERTInextCAPlugin.cs | 43 ++++++++---- 2 files changed, 101 insertions(+), 15 deletions(-) diff --git a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs index 4286f39..912c98c 100644 --- a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs @@ -1204,6 +1204,79 @@ public Task CleanupValidation(string key, CancellationTo public string GetValidationType() => "dns-01"; } + /// Records the CancellationToken it was actually called with, for each method. + private sealed class TokenCapturingDomainValidator : IDomainValidator + { + public List<(string key, string value)> StagedRecords { get; } = new(); + public List CleanupTokens { get; } = new(); + + public void Initialize(IDomainValidatorConfigProvider configProvider) { } + + public Task StageValidation(string key, string value, CancellationToken cancellationToken) + { + StagedRecords.Add((key, value)); + return Task.FromResult(new DomainValidationResult { Success = true }); + } + + public Task CleanupValidation(string key, CancellationToken cancellationToken) + { + CleanupTokens.Add(cancellationToken); + return Task.FromResult(new DomainValidationResult { Success = true }); + } + + public Task ValidateConfiguration(Dictionary configuration) => Task.CompletedTask; + public Dictionary GetDomainValidatorAnnotations() => new(); + public string GetValidationType() => "dns-01"; + } + + /// + /// Regression: the compensating cleanup call after an early exit from staging (chiefly the + /// shared DcvTimeoutMinutes-bound token firing mid-loop, which is what this scenario + /// simulates via a domain whose GetDcv call raises OperationCanceledException) must not reuse + /// the same token the operation was cancelled by — CleanupValidation must receive + /// CancellationToken.None, not the ambient `ct`. A cooperative IDomainValidator that forwards + /// its token into its own HTTP calls (the reference CloudflareDomainValidator in this repo + /// does exactly that) would otherwise throw immediately on an already-cancelled token and + /// never even attempt the delete, silently leaving the TXT record published. + /// + [Fact] + public async Task Dcv_CleanupAfterCancellation_UsesCancellationTokenNone_NotTheAmbientToken() + { + const string order = MockCertificateData.DcvOrderId; + const string good = "a.example.com"; + const string bad = "b.example.com"; + + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); + + mock.Setup(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, good, bad)); + + mock.Setup(c => c.GetDcvAsync(order, good, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse("token-a")); + // Domain 'good' is processed first (Dictionary enumeration order matches insertion order + // in practice for the small dictionaries this test builds); 'bad' then throws, driving the + // outer catch's cleanup of the already-staged 'good' entry. + mock.Setup(c => c.GetDcvAsync(order, bad, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ThrowsAsync(new OperationCanceledException("DCV timeout budget exceeded")); + + var validator = new TokenCapturingDomainValidator(); + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator)); + + Func act = () => Enroll(plugin); + await act.Should().ThrowAsync(); + + validator.StagedRecords.Should().ContainSingle( + "'good' must have staged before 'bad' threw, for this test to exercise cleanup at all"); + validator.CleanupTokens.Should().ContainSingle( + "the staged entry must go through the cancellation cleanup path exactly once") + .Which.Should().Be(CancellationToken.None, + "cleanup is a best-effort compensating action and must run with its own token, " + + "not the token that was just cancelled"); + } + /// /// Regression: a StageValidation failure on one domain of a multi-domain order must not /// leave the TXT records already published for the earlier domains orphaned. Before the diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index dea7003..261dbbd 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -1096,7 +1096,7 @@ private async Task EnrollNewAsync( Csr = csr, ValidityDays = ep.ValidityDays > 0 ? ep.ValidityDays : (int?)null, Subject = subject, - Sans = BuildSanList(san, csr), + Sans = BuildSanList(san, csr, subject), RequesterName = string.IsNullOrWhiteSpace(ep.RequesterName) ? null : ep.RequesterName, RequesterEmail = string.IsNullOrWhiteSpace(ep.RequesterEmail) ? null : ep.RequesterEmail, KeyType = string.IsNullOrWhiteSpace(ep.KeyType) ? null : ep.KeyType, @@ -1317,7 +1317,7 @@ private async Task RenewOrReissueAsync( // set as a new enrollment — otherwise a renewed UCC certificate comes back // holding only its primary domain. Subject = subject, - Sans = BuildSanList(san, csr), + Sans = BuildSanList(san, csr, subject), ValidityDays = ep.ValidityDays > 0 ? ep.ValidityDays : (int?)null, RequesterName = string.IsNullOrWhiteSpace(ep.RequesterName) ? null : ep.RequesterName, RequesterEmail = string.IsNullOrWhiteSpace(ep.RequesterEmail) ? null : ep.RequesterEmail, @@ -1711,7 +1711,17 @@ async Task CleanupOneStagedValidationAsync( var (domain, hostname, validator) = entry; try { - await validator.CleanupValidation(hostname, ct); + // CancellationToken.None, not `ct`: this is a best-effort compensating action — + // removing a TXT record we already published — and it must run regardless of + // WHY we are cleaning up, including the case where `ct` itself is the reason. + // The dominant real trigger for this exact call site is the shared + // DcvTimeoutMinutes-bound token firing mid-loop (see the outer catch's comment + // below), which means `ct` is guaranteed already cancelled here. A cooperative + // IDomainValidator that forwards its token into its own HTTP calls — the + // reference CloudflareDomainValidator in this repo does exactly that — would + // throw immediately on an already-cancelled token and never even attempt the + // delete, silently leaving the record published with only a Warning logged. + await validator.CleanupValidation(hostname, CancellationToken.None); _logger.LogInformation( "DNS TXT record cleaned up{Context}. Domain={Domain}, Hostname={Hostname}", context, LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); @@ -2405,7 +2415,7 @@ private static int MapRevocationReasonStringToCode(string reason) /// 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 List BuildSanList(Dictionary san, string csr) + private List BuildSanList(Dictionary san, string csr, string subject) { var result = new List(); // Type+value identity, so the same name requested as two different SAN types is @@ -2471,14 +2481,15 @@ void Add(string type, string value, bool fromCsr = false) "{Count} SAN(s) in the CSR use a type that cannot be represented as a domain name " + "and were not submitted (ASN.1 GeneralName tag(s): {Tags}). CERTInext's " + "additionalDomains field carries domain names only, so these cannot appear on the " + - "issued certificate. Remove them from the CSR if they are required.", - skippedCsrTags.Count, string.Join(", ", skippedCsrTags)); + "issued certificate. Remove them from the CSR if they are required. Subject={Subject}", + skippedCsrTags.Count, string.Join(", ", skippedCsrTags), subject); } if (result.Count == 0) { _logger.LogDebug( - "No SANs supplied by the gateway and none found in the CSR — submitting the order with domainName only."); + "No SANs supplied by the gateway and none found in the CSR — submitting the order " + + "with domainName only. Subject={Subject}", subject); return null; } @@ -2514,8 +2525,9 @@ void Add(string type, string value, bool fromCsr = false) "{Count} requested SAN(s) are not DNS names and are being DROPPED because " + "SubmitNonDnsSans is false: {Sans}. The order will issue, but the certificate will " + "NOT contain these names. Set SubmitNonDnsSans back to true to submit them and have " + - "CERTInext surface the problem instead.", - nonDns.Count, LogSanitizer.Strip(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}")))); + "CERTInext surface the problem instead. Subject={Subject}", + nonDns.Count, LogSanitizer.Strip(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}"))), + subject); result = result .Where(s => string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)) @@ -2531,9 +2543,9 @@ void Add(string type, string value, bool fromCsr = false) int fromCsrKept = result.Count(s => fromCsrKeys.Contains($"{s.Type}|{s.Value}")); _logger.LogInformation( "Resolved {Total} SAN(s) for submission. FromGatewayRequest={FromGateway}, " + - "AddedFromCsrFallback={FromCsr}, Sans={Sans}", + "AddedFromCsrFallback={FromCsr}, Sans={Sans}, Subject={Subject}", result.Count, fromGateway, fromCsrKept, - LogSanitizer.Strip(string.Join("; ", result.Select(s => $"{s.Type}:{s.Value}")))); + LogSanitizer.Strip(string.Join("; ", result.Select(s => $"{s.Type}:{s.Value}"))), subject); if (fromCsrKept > 0) { @@ -2543,8 +2555,8 @@ void Add(string type, string value, bool fromCsr = false) _logger.LogWarning( "Command supplied no SAN data for this enrollment; {Count} SAN(s) present in the CSR " + "have been added to the order instead. Review the enrollment pattern / template SAN " + - "configuration.", - fromCsrKept); + "configuration. Subject={Subject}", + fromCsrKept, subject); } if (nonDns.Count > 0) @@ -2558,8 +2570,9 @@ void Add(string type, string value, bool fromCsr = false) "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, LogSanitizer.Strip(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}")))); + "order should proceed. Subject={Subject}", + nonDns.Count, LogSanitizer.Strip(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}"))), + subject); } return result; From 8a3ac172e0e8f7a36477ccd66a9ad3a89a7a4bbf Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:28:39 -0700 Subject: [PATCH 09/14] =?UTF-8?q?fix(enroll):=20address=20full-review=20ro?= =?UTF-8?q?und=207=20=E2=80=94=20cleanup=20call=20unbounded,=20subject=20u?= =?UTF-8?q?nsanitized=20in=20BuildSanList?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CERTInext.Tests/CERTInextCAPluginDcvTests.cs | 54 ++++++++++---------- CERTInext/CERTInextCAPlugin.cs | 43 ++++++++++------ CERTInext/Constants.cs | 11 ++++ 3 files changed, 66 insertions(+), 42 deletions(-) diff --git a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs index 912c98c..624759c 100644 --- a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs @@ -841,6 +841,15 @@ public async Task Dcv_WaitsForIssuance_AfterDcvVerifies() // Undrainable pending domains must not strand the valid ones on the same order // --------------------------------------------------------------------------- + /// Builds a DomainVerificationDetail JsonElement for the given dcvStatus. + private static System.Text.Json.JsonElement DcvDetail(string dcvStatus) => + System.Text.Json.JsonSerializer.SerializeToElement(new DomainVerificationDetail + { + DcvMethod = Constants.Dcv.MethodDnsTxt, + DcvStatus = dcvStatus, + Status = "1" + }); + /// /// Builds a TrackOrder response whose domainVerification block lists several pending /// domains, so tests can mix validatable and unvalidatable keys on one order. @@ -848,13 +857,7 @@ public async Task Dcv_WaitsForIssuance_AfterDcvVerifies() private static TrackOrderResponse DcvPendingTrackResponseMultiDomain( string orderNumber, params string[] domains) { - var detail = System.Text.Json.JsonSerializer.SerializeToElement(new DomainVerificationDetail - { - DcvMethod = Constants.Dcv.MethodDnsTxt, - DcvStatus = Constants.Dcv.StatusPending, - Status = "1" - }); - + var detail = DcvDetail(Constants.Dcv.StatusPending); var raw = new Dictionary(); foreach (string d in domains) raw[d] = detail; @@ -883,18 +886,8 @@ private static TrackOrderResponse DcvPendingTrackResponseMultiDomain( private static TrackOrderResponse DcvMixedStatusTrackResponse( string validatedDomain, string pendingDomain) { - var validated = System.Text.Json.JsonSerializer.SerializeToElement(new DomainVerificationDetail - { - DcvMethod = Constants.Dcv.MethodDnsTxt, - DcvStatus = Constants.Dcv.StatusValidated, - Status = "1" - }); - var pending = System.Text.Json.JsonSerializer.SerializeToElement(new DomainVerificationDetail - { - DcvMethod = Constants.Dcv.MethodDnsTxt, - DcvStatus = Constants.Dcv.StatusPending, - Status = "1" - }); + var validated = DcvDetail(Constants.Dcv.StatusValidated); + var pending = DcvDetail(Constants.Dcv.StatusPending); return new TrackOrderResponse { @@ -1233,14 +1226,17 @@ public Task CleanupValidation(string key, CancellationTo /// Regression: the compensating cleanup call after an early exit from staging (chiefly the /// shared DcvTimeoutMinutes-bound token firing mid-loop, which is what this scenario /// simulates via a domain whose GetDcv call raises OperationCanceledException) must not reuse - /// the same token the operation was cancelled by — CleanupValidation must receive - /// CancellationToken.None, not the ambient `ct`. A cooperative IDomainValidator that forwards + /// the same token the operation was cancelled by. A cooperative IDomainValidator that forwards /// its token into its own HTTP calls (the reference CloudflareDomainValidator in this repo /// does exactly that) would otherwise throw immediately on an already-cancelled token and /// never even attempt the delete, silently leaving the TXT record published. + /// + /// CancellationToken.None would fix that but removes the cleanup call's timeout bound + /// entirely — a second, adversarially-found regression on top of the first — so the correct + /// fix is a fresh token with its OWN short timeout: not cancelled going in, but still bounded. /// [Fact] - public async Task Dcv_CleanupAfterCancellation_UsesCancellationTokenNone_NotTheAmbientToken() + public async Task Dcv_CleanupAfterCancellation_UsesAFreshBoundedToken_NotTheAmbientToken() { const string order = MockCertificateData.DcvOrderId; const string good = "a.example.com"; @@ -1270,11 +1266,15 @@ public async Task Dcv_CleanupAfterCancellation_UsesCancellationTokenNone_NotTheA validator.StagedRecords.Should().ContainSingle( "'good' must have staged before 'bad' threw, for this test to exercise cleanup at all"); - validator.CleanupTokens.Should().ContainSingle( - "the staged entry must go through the cancellation cleanup path exactly once") - .Which.Should().Be(CancellationToken.None, - "cleanup is a best-effort compensating action and must run with its own token, " + - "not the token that was just cancelled"); + var cleanupToken = validator.CleanupTokens.Should().ContainSingle( + "the staged entry must go through the cancellation cleanup path exactly once").Subject; + + cleanupToken.IsCancellationRequested.Should().BeFalse( + "cleanup is a best-effort compensating action and must run with its own token, " + + "not the already-cancelled ambient one"); + cleanupToken.CanBeCanceled.Should().BeTrue( + "the cleanup call must still be bounded by its own timeout, not unbounded " + + "(CancellationToken.None) — a hanging DNS-provider call must not block forever"); } /// diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index 261dbbd..db88321 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -1711,17 +1711,29 @@ async Task CleanupOneStagedValidationAsync( var (domain, hostname, validator) = entry; try { - // CancellationToken.None, not `ct`: this is a best-effort compensating action — - // removing a TXT record we already published — and it must run regardless of - // WHY we are cleaning up, including the case where `ct` itself is the reason. - // The dominant real trigger for this exact call site is the shared - // DcvTimeoutMinutes-bound token firing mid-loop (see the outer catch's comment - // below), which means `ct` is guaranteed already cancelled here. A cooperative - // IDomainValidator that forwards its token into its own HTTP calls — the - // reference CloudflareDomainValidator in this repo does exactly that — would + // A fresh, independently-bounded token — deliberately neither `ct` nor + // CancellationToken.None. + // + // Not `ct`: this is a best-effort compensating action — removing a TXT record we + // already published — and it must run regardless of WHY we are cleaning up, + // including the case where `ct` itself is the reason (the dominant real trigger + // for the early-exit call site is the shared DcvTimeoutMinutes-bound token firing + // mid-loop, which means `ct` is guaranteed already cancelled there). A + // cooperative IDomainValidator that forwards its token into its own HTTP calls — + // the reference CloudflareDomainValidator in this repo does exactly that — would // throw immediately on an already-cancelled token and never even attempt the // delete, silently leaving the record published with only a Warning logged. - await validator.CleanupValidation(hostname, CancellationToken.None); + // + // Not CancellationToken.None either: this method's own SOX CC7.3 guarantee is + // that the whole DCV flow is hard-timeout-bounded so a stuck DNS provider cannot + // hold a gateway worker thread indefinitely. That bound has to come from + // somewhere for THIS call too — including the routine, always-runs finally-block + // cleanup on the ordinary successful-DCV path, which was never cancellation- + // related to begin with and would otherwise hang forever on a DNS provider + // plugin whose underlying network call stalls. + using var cleanupCts = new CancellationTokenSource( + TimeSpan.FromSeconds(Constants.Dcv.CleanupValidationTimeoutSeconds)); + await validator.CleanupValidation(hostname, cleanupCts.Token); _logger.LogInformation( "DNS TXT record cleaned up{Context}. Domain={Domain}, Hostname={Hostname}", context, LogSanitizer.Strip(domain), LogSanitizer.Strip(hostname)); @@ -2482,14 +2494,14 @@ void Add(string type, string value, bool fromCsr = false) "and were not submitted (ASN.1 GeneralName tag(s): {Tags}). CERTInext's " + "additionalDomains field carries domain names only, so these cannot appear on the " + "issued certificate. Remove them from the CSR if they are required. Subject={Subject}", - skippedCsrTags.Count, string.Join(", ", skippedCsrTags), subject); + skippedCsrTags.Count, string.Join(", ", skippedCsrTags), LogSanitizer.Strip(subject)); } if (result.Count == 0) { _logger.LogDebug( "No SANs supplied by the gateway and none found in the CSR — submitting the order " + - "with domainName only. Subject={Subject}", subject); + "with domainName only. Subject={Subject}", LogSanitizer.Strip(subject)); return null; } @@ -2527,7 +2539,7 @@ void Add(string type, string value, bool fromCsr = false) "NOT contain these names. Set SubmitNonDnsSans back to true to submit them and have " + "CERTInext surface the problem instead. Subject={Subject}", nonDns.Count, LogSanitizer.Strip(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}"))), - subject); + LogSanitizer.Strip(subject)); result = result .Where(s => string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)) @@ -2545,7 +2557,8 @@ void Add(string type, string value, bool fromCsr = false) "Resolved {Total} SAN(s) for submission. FromGatewayRequest={FromGateway}, " + "AddedFromCsrFallback={FromCsr}, Sans={Sans}, Subject={Subject}", result.Count, fromGateway, fromCsrKept, - LogSanitizer.Strip(string.Join("; ", result.Select(s => $"{s.Type}:{s.Value}"))), subject); + LogSanitizer.Strip(string.Join("; ", result.Select(s => $"{s.Type}:{s.Value}"))), + LogSanitizer.Strip(subject)); if (fromCsrKept > 0) { @@ -2556,7 +2569,7 @@ void Add(string type, string value, bool fromCsr = false) "Command supplied no SAN data for this enrollment; {Count} SAN(s) present in the CSR " + "have been added to the order instead. Review the enrollment pattern / template SAN " + "configuration. Subject={Subject}", - fromCsrKept, subject); + fromCsrKept, LogSanitizer.Strip(subject)); } if (nonDns.Count > 0) @@ -2572,7 +2585,7 @@ void Add(string type, string value, bool fromCsr = false) "subscriber requested. Remove them from the CSR or the enrollment pattern if the " + "order should proceed. Subject={Subject}", nonDns.Count, LogSanitizer.Strip(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}"))), - subject); + LogSanitizer.Strip(subject)); } return result; diff --git a/CERTInext/Constants.cs b/CERTInext/Constants.cs index 22da587..e3dc989 100644 --- a/CERTInext/Constants.cs +++ b/CERTInext/Constants.cs @@ -324,6 +324,17 @@ public static class Dcv // Override via the DcvTxtRecordTemplate connector config field. public const string DefaultTxtRecordTemplate = "_emsign-validation.{0}"; + // Independent bound for a single CleanupValidation (TXT-record removal) call. This is + // deliberately its own fixed ceiling, not a fraction of DcvTimeoutMinutes and not the + // ambient DCV-flow cancellation token: cleanup is a best-effort compensating action that + // must get a real chance to run even when the operation it's cleaning up after was + // itself cancelled (the ambient token would already be cancelled at that point), but it + // still must not be allowed to hang the calling gateway request forever if a DNS + // provider plugin's underlying network call stalls. 60s comfortably covers a single + // DELETE-shaped call under normal conditions (the reference CloudflareDomainValidator's + // HttpClient default alone is 100s) without risking an indefinite hang. + public const int CleanupValidationTimeoutSeconds = 60; + // Defaults for the DCV-during-sync bounds (issue 0002). public const int DefaultSyncMaxOrderAgeHours = 24; public const int DefaultSyncMaxPerPass = 50; From 41a71ae566864c9800bcffb7c03698e3c2e7a269 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:59:52 -0700 Subject: [PATCH 10/14] =?UTF-8?q?fix(enroll):=20address=20full-review=20ro?= =?UTF-8?q?und=208=20=E2=80=94=20stale=20gateway=20count=20in=20log,=20can?= =?UTF-8?q?cellation=20swallows=20audit=20line?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CERTInext.Tests/SanSubmissionTests.cs | 79 ++++++++++++++++----------- CERTInext/CERTInextCAPlugin.cs | 23 +++++--- CERTInext/Client/CERTInextClient.cs | 17 ++++++ 3 files changed, 80 insertions(+), 39 deletions(-) diff --git a/CERTInext.Tests/SanSubmissionTests.cs b/CERTInext.Tests/SanSubmissionTests.cs index 541fde3..c305665 100644 --- a/CERTInext.Tests/SanSubmissionTests.cs +++ b/CERTInext.Tests/SanSubmissionTests.cs @@ -152,40 +152,12 @@ private static string GenerateCsrPemWithGeneralNames(string cn, params GeneralNa keyGen.Init(new KeyGenerationParameters(new SecureRandom(), 2048)); AsymmetricCipherKeyPair kp = keyGen.GenerateKeyPair(); - var extGen = new X509ExtensionsGenerator(); - extGen.AddExtension(X509Extensions.SubjectAlternativeName, critical: false, - extValue: new GeneralNames(names)); - - var attributes = new DerSet(new AttributePkcs( - PkcsObjectIdentifiers.Pkcs9AtExtensionRequest, - new DerSet(extGen.Generate()))); - - var csr = new Pkcs10CertificationRequest( - "SHA256withRSA", new X509Name($"CN={cn}"), kp.Public, attributes, kp.Private); - - return "-----BEGIN CERTIFICATE REQUEST-----\n" - + Convert.ToBase64String(csr.GetEncoded(), Base64FormattingOptions.InsertLineBreaks) - + "\n-----END CERTIFICATE REQUEST-----"; - } - - /// - /// Builds a real PKCS#10 CSR for , optionally carrying a - /// subjectAltName extension holding . - /// - private static string GenerateCsrPem(string cn, params string[] dnsSans) - { - var keyGen = new RsaKeyPairGenerator(); - keyGen.Init(new KeyGenerationParameters(new SecureRandom(), 2048)); - AsymmetricCipherKeyPair kp = keyGen.GenerateKeyPair(); - Asn1Set attributes = null; - if (dnsSans != null && dnsSans.Length > 0) + if (names != null && names.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); + extGen.AddExtension(X509Extensions.SubjectAlternativeName, critical: false, + extValue: new GeneralNames(names)); attributes = new DerSet(new AttributePkcs( PkcsObjectIdentifiers.Pkcs9AtExtensionRequest, @@ -200,6 +172,14 @@ private static string GenerateCsrPem(string cn, params string[] dnsSans) + "\n-----END CERTIFICATE REQUEST-----"; } + /// + /// Builds a real PKCS#10 CSR for , optionally carrying a + /// subjectAltName extension holding . + /// + private static string GenerateCsrPem(string cn, params string[] dnsSans) => + GenerateCsrPemWithGeneralNames( + cn, (dnsSans ?? Array.Empty()).Select(d => new GeneralName(GeneralName.DnsName, d)).ToArray()); + // ======================================================================= // End-to-end: Command's SAN dictionary → the JSON on the wire // ======================================================================= @@ -399,6 +379,43 @@ await plugin.Enroll( .Should().BeEquivalentTo(new[] { "alt.example.com", "192.0.2.10", "admin@example.com" }); } + /// + /// Regression for a stale-count bug in BuildSanList's own audit log: with a mixed + /// DNS/non-DNS gateway SAN dictionary and SubmitNonDnsSans=false, the "Resolved N SAN(s)" + /// log line reported the pre-filter gateway count (3) alongside the post-filter total (1) — + /// an arithmetic impossibility ("Resolved 1 ... FromGatewayRequest=3"). There is no log- + /// capture seam in this codebase (ILogger comes from a fixed LogHandler.GetClassLogger() + /// field, not an injectable dependency), so this pins the payload-level data the log line is + /// computed from instead: with the non-DNS entries filtered out, exactly the one DNS name + /// must reach additionalDomains — proving the surviving gateway-sourced count is 1, not the + /// pre-filter 3 the stale log line used to claim. + /// + [Fact] + public async Task MixedGatewaySans_SubmitNonDnsSansFalse_OnlyDnsNameSurvivesFiltering() + { + var plugin = new CERTInextCAPlugin( + BuildRealClient(), + new CERTInextConfig { PickupRetries = 0, SubmitNonDnsSans = false }); + + await plugin.Enroll( + csr: GenerateCsrPem("host.example.com"), + subject: "CN=host.example.com", + san: new Dictionary + { + ["dnsname"] = new[] { "alt.example.com" }, + ["ipaddress"] = new[] { "192.0.2.10" }, + ["rfc822name"] = new[] { "admin@example.com" } + }, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + AdditionalDomains(CapturedCertificateInformation()) + .Should().BeEquivalentTo(new[] { "alt.example.com" }, + "only the DNS entry should survive the SubmitNonDnsSans=false filter, out of " + + "3 the gateway supplied"); + } + /// /// A CSR we cannot parse must not break enrollment — the gateway-supplied SANs still go. /// FakeCsrPem is deliberately truncated, so this also guards the many existing diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index db88321..2e84a73 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -2462,18 +2462,17 @@ void Add(string type, string value, bool fromCsr = false) } } - int fromGateway = result.Count; - // CSR fallback — only when the gateway dictionary is itself absent (san == null), NOT - // merely "computed to zero SAN entries" (fromGateway == 0). Those are different things: + // merely "computed to zero SAN entries" (i.e. result.Count == 0 at this point). Those + // are different things: // a non-null dictionary — even an empty one, or one whose keys all map to empty arrays // — means Command's enrollment pattern ran and deliberately produced no SANs for this // request, which the CSR fallback must respect rather than override. san == null means // Command never populated SAN data for this enrollment path at all, which is the one - // case this fallback exists for. Checking fromGateway instead of san itself would let an - // enrollment pattern that explicitly computes zero SANs still have CSR-derived names - // spliced back in — reopening the policy-reintroduction risk the fallback-over-union - // redesign exists to close. + // case this fallback exists for. Checking "computed to zero" instead of "san is null" + // would let an enrollment pattern that explicitly computes zero SANs still have + // CSR-derived names spliced back in — reopening the policy-reintroduction risk the + // fallback-over-union redesign exists to close. var skippedCsrTags = new List(); if (san == null) { @@ -2553,10 +2552,18 @@ void Add(string type, string value, bool fromCsr = false) // The blind spot that hid the original defect was that nothing logged what we // resolved. Log the final, post-filter resolved set and its provenance at Information. int fromCsrKept = result.Count(s => fromCsrKeys.Contains($"{s.Type}|{s.Value}")); + // Post-filter, not the pre-filter `fromGateway` snapshot: gateway- and CSR-sourced + // entries are mutually exclusive by construction (the CSR fallback only ever runs when + // the gateway supplied nothing at all), so whatever's left in `result` and isn't + // fromCsrKept must be gateway-sourced. Using the pre-filter count here reproduced the + // exact self-contradicting-audit-trail bug this method was already restructured once to + // fix — with SubmitNonDnsSans=false this line could read e.g. "Resolved 1 SAN(s) ... + // FromGatewayRequest=3", an arithmetic impossibility for anyone reconciling counts. + int fromGatewayKept = result.Count - fromCsrKept; _logger.LogInformation( "Resolved {Total} SAN(s) for submission. FromGatewayRequest={FromGateway}, " + "AddedFromCsrFallback={FromCsr}, Sans={Sans}, Subject={Subject}", - result.Count, fromGateway, fromCsrKept, + result.Count, fromGatewayKept, fromCsrKept, LogSanitizer.Strip(string.Join("; ", result.Select(s => $"{s.Type}:{s.Value}"))), LogSanitizer.Strip(subject)); diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index cf6a4da..ceccf25 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -1330,6 +1330,7 @@ private async Task ExecuteWithRetryAsync( { int attempts = idempotent ? maxAttempts : 1; RestResponse resp = null; + var sw = System.Diagnostics.Stopwatch.StartNew(); for (int attempt = 1; attempt <= attempts; attempt++) { resp = await _http.ExecuteAsync(req, ct); @@ -1346,6 +1347,22 @@ private async Task ExecuteWithRetryAsync( // past its per-domain catches. Surface the true cancellation here, at the one place // in the client that actually holds `ct`, before any retry or error-wrapping logic // sees the response. + // + // Throwing here means every caller's own per-call audit line (Method/Path/HttpStatus/ + // LatencyMs, logged after ExecuteWithRetryAsync returns) never executes for the + // cancelled call — that specific attempt would otherwise vanish from the audit trail + // entirely, leaving only a coarser, order-level "unexpected failure" log with no + // domain/endpoint/status/latency. Log that record here instead, at the one place that + // reliably sees every cancellation regardless of which of ExecuteWithRetryAsync's ~10 + // callers is in flight. + if (ct.IsCancellationRequested) + { + Logger.LogWarning( + "CERTInext API call cancelled: Method={Method}, Path={Path}, HttpStatus={Status}, " + + "ResponseStatus={ResponseStatus}, LatencyMs={Latency}, Attempt={Attempt}/{Max}.", + req.Method, req.Resource, (int)resp.StatusCode, resp.ResponseStatus, + sw.ElapsedMilliseconds, attempt, attempts); + } ct.ThrowIfCancellationRequested(); // Success or 4xx client error — return immediately From 5d5e2b33c54e443f614e2df8b2210a6030d3224a Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Wed, 12 Aug 2026 15:41:23 -0700 Subject: [PATCH 11/14] =?UTF-8?q?fix(enroll):=20address=20full-review=20ro?= =?UTF-8?q?und=209=20=E2=80=94=20TXT=20cleanup=20unbounded=20in=20aggregat?= =?UTF-8?q?e=20across=20domains?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One confirmed root cause (found independently by two lenses), continuing the round 6→7 pattern: round 7 fixed each cleanup call's own timeout bound, but running those calls one after another meant the bound was per-call, not in aggregate. Both the early-exit cleanup (CleanupPartialStagingAsync) and the routine, always-runs finally-block cleanup iterated staged domains sequentially. A UCC/multi-SAN order (the exact feature this diff exists to support) with N staged domains could hold the calling request open for up to N x CleanupValidationTimeoutSeconds if the DNS provider was merely slow — not even hung — on every delete: a realistic degraded-provider condition, not a contrived one. For a large SAN count this can exceed DcvTimeoutMinutes itself, contradicting the method's own SOX CC7.3 "the entire DCV flow is hard-timeout-bounded" comment for exactly the multi-domain case this whole fix chain has been hardening. Fixed by running the per-domain cleanup calls concurrently (Task.WhenAll) instead of sequentially, at both call sites. Each call keeps its own independent 60s bound from round 7; running them concurrently means the wall-clock time for the whole batch is bounded by the slowest single call, not the sum — these are independent per-domain operations on different hostnames/records with no shared mutable state, so there is nothing for concurrent execution to race on. Also applied both endorsed advisories: three new test-only IDomainValidator/ IDomainValidatorFactory implementations (PartiallyFailingDomainValidator, TokenCapturingDomainValidator, SelectiveDomainValidatorFactory) each re-implemented boilerplate the pre-existing FakeDomainValidator/ FakeDomainValidatorFactory already provided. Extended those two shared fakes instead (ShouldFail predicate + CleanupTokens capture on the validator; an optional resolvableDomain filter on the factory) and deleted the three duplicates, updating call sites. Tests: added a timing-based regression test proving cleanup for 3 domains completes in close to one cleanup delay's worth of wall time, not three — verified it actually catches the regression by temporarily reverting the fix locally (confirmed FAIL at ~4.5s) before restoring it (confirmed PASS at ~3s), so the threshold is proven discriminating, not just a number that happens to pass. Extended FakeDomainValidator with a configurable CleanupDelay to make this possible; its two List fields needed a lock now that cleanup calls can genuinely run concurrently (StagedRecords did not, since nothing awaits with a real yield point before writing to it). Release, no-DCV: 199/199. Release, DCV: 230/230. Zero code warnings in both. --- CERTInext.Tests/CERTInextCAPluginDcvTests.cs | 169 +++++++++---------- CERTInext.Tests/FakeDomainValidator.cs | 61 +++++-- CERTInext/CERTInextCAPlugin.cs | 24 ++- 3 files changed, 151 insertions(+), 103 deletions(-) diff --git a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs index 624759c..b45f0ac 100644 --- a/CERTInext.Tests/CERTInextCAPluginDcvTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginDcvTests.cs @@ -941,7 +941,7 @@ public async Task Dcv_CachedCnPlusUnresolvableSan_DefersWithoutThrowing() // scenario that must prove "a provider IS deployed" rather than "nothing is deployed". var plugin = BuildPlugin( mock.Object, - new SelectiveDomainValidatorFactory(validator, resolvableDomain: cn), + new FakeDomainValidatorFactory(validator, resolvableDomain: cn), DcvConfig()); Func act = () => Enroll(plugin); @@ -1126,7 +1126,7 @@ public async Task Dcv_DomainWithNoResolvableValidator_IsSkipped_AndValidDomainSt var validator = new FakeDomainValidator(); var plugin = BuildPlugin( mock.Object, - new SelectiveDomainValidatorFactory(validator, resolvableDomain: good), + new FakeDomainValidatorFactory(validator, resolvableDomain: good), DcvConfig(dcvWaitForIssuanceSeconds: 10)); var result = await Enroll(plugin); @@ -1138,90 +1138,6 @@ public async Task Dcv_DomainWithNoResolvableValidator_IsSkipped_AndValidDomainSt result.Status.Should().Be((int)EndEntityStatus.GENERATED); } - /// Factory that resolves a validator for exactly one domain and null for all others. - private sealed class SelectiveDomainValidatorFactory : IDomainValidatorFactory - { - private readonly IDomainValidator _validator; - private readonly string _resolvableDomain; - - public SelectiveDomainValidatorFactory(IDomainValidator validator, string resolvableDomain) - { - _validator = validator; - _resolvableDomain = resolvableDomain; - } - - public IDomainValidator ResolveDomainValidator(string domain, string validationType) => - string.Equals(domain, _resolvableDomain, StringComparison.OrdinalIgnoreCase) ? _validator : null; - - public IDomainValidator PrimaryValidator => _validator; - } - - /// - /// Like , but StageValidation fails only for hostnames - /// matching — needed to prove that a failure on one - /// domain of a multi-domain order still cleans up what was already staged for the others. - /// - private sealed class PartiallyFailingDomainValidator : IDomainValidator - { - private readonly string _failingHostnameSubstring; - - public PartiallyFailingDomainValidator(string failingHostnameSubstring) => - _failingHostnameSubstring = failingHostnameSubstring; - - public List<(string key, string value)> StagedRecords { get; } = new(); - public List CleanedUpKeys { get; } = new(); - - public void Initialize(IDomainValidatorConfigProvider configProvider) { } - - public Task StageValidation(string key, string value, CancellationToken cancellationToken) - { - bool shouldFail = key.Contains(_failingHostnameSubstring, StringComparison.OrdinalIgnoreCase); - if (!shouldFail) - StagedRecords.Add((key, value)); - - return Task.FromResult(new DomainValidationResult - { - Success = !shouldFail, - ErrorMessage = shouldFail ? "Stage failed (test stub, selective)" : null - }); - } - - public Task CleanupValidation(string key, CancellationToken cancellationToken) - { - CleanedUpKeys.Add(key); - return Task.FromResult(new DomainValidationResult { Success = true }); - } - - public Task ValidateConfiguration(Dictionary configuration) => Task.CompletedTask; - public Dictionary GetDomainValidatorAnnotations() => new(); - public string GetValidationType() => "dns-01"; - } - - /// Records the CancellationToken it was actually called with, for each method. - private sealed class TokenCapturingDomainValidator : IDomainValidator - { - public List<(string key, string value)> StagedRecords { get; } = new(); - public List CleanupTokens { get; } = new(); - - public void Initialize(IDomainValidatorConfigProvider configProvider) { } - - public Task StageValidation(string key, string value, CancellationToken cancellationToken) - { - StagedRecords.Add((key, value)); - return Task.FromResult(new DomainValidationResult { Success = true }); - } - - public Task CleanupValidation(string key, CancellationToken cancellationToken) - { - CleanupTokens.Add(cancellationToken); - return Task.FromResult(new DomainValidationResult { Success = true }); - } - - public Task ValidateConfiguration(Dictionary configuration) => Task.CompletedTask; - public Dictionary GetDomainValidatorAnnotations() => new(); - public string GetValidationType() => "dns-01"; - } - /// /// Regression: the compensating cleanup call after an early exit from staging (chiefly the /// shared DcvTimeoutMinutes-bound token firing mid-loop, which is what this scenario @@ -1258,7 +1174,7 @@ public async Task Dcv_CleanupAfterCancellation_UsesAFreshBoundedToken_NotTheAmbi mock.Setup(c => c.GetDcvAsync(order, bad, Constants.Dcv.MethodDnsTxt, It.IsAny())) .ThrowsAsync(new OperationCanceledException("DCV timeout budget exceeded")); - var validator = new TokenCapturingDomainValidator(); + var validator = new FakeDomainValidator(); var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator)); Func act = () => Enroll(plugin); @@ -1277,6 +1193,80 @@ public async Task Dcv_CleanupAfterCancellation_UsesAFreshBoundedToken_NotTheAmbi "(CancellationToken.None) — a hanging DNS-provider call must not block forever"); } + /// + /// Regression: the routine, always-runs finally-block cleanup used to iterate staged domains + /// sequentially. Each cleanup call already has its own independent + /// CleanupValidationTimeoutSeconds bound, but running them one after another meant that + /// bound was per-call, not in aggregate — a UCC order with N staged domains could hold the + /// calling request open for up to N x the per-call ceiling if the DNS provider was merely + /// slow (not even hung) on every delete, which can exceed DcvTimeoutMinutes itself for a + /// realistic multi-SAN count. Proven here by timing: three domains each with an artificial + /// cleanup delay must complete in close to ONE delay's worth of wall time, not three. + /// + [Fact] + public async Task Dcv_CleanupOfMultipleDomains_RunsConcurrently_NotSequentially() + { + const string order = MockCertificateData.DcvOrderId; + string[] domains = { "a.example.com", "b.example.com", "c.example.com" }; + var cleanupDelay = TimeSpan.FromMilliseconds(800); + + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), It.IsAny())) + .ReturnsAsync(new EnrollCertificateResponse { Id = order, Status = "pending_dcv" }); + + var verifiedDetail = DcvDetail(Constants.Dcv.StatusValidated); + var verifiedRaw = new Dictionary(); + foreach (string d in domains) verifiedRaw[d] = verifiedDetail; + + mock.SetupSequence(c => c.TrackOrderAsync(order, It.IsAny())) + .ReturnsAsync(DcvPendingTrackResponseMultiDomain(order, domains)) + .ReturnsAsync(new TrackOrderResponse + { + OrderDetails = new TrackOrderResponseDetails + { + OrderStatusId = "1", + CertificateStatusId = "1", + DomainVerification = new TrackOrderDomainVerification + { + Status = Constants.Dcv.StatusValidated, + RawDomainEntries = verifiedRaw + } + } + }); + + foreach (string d in domains) + { + mock.Setup(c => c.GetDcvAsync(order, d, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .ReturnsAsync(MockCertificateData.DcvTokenResponse($"token-{d}")); + mock.Setup(c => c.VerifyDcvAsync(order, d, Constants.Dcv.MethodDnsTxt, It.IsAny())) + .Returns(Task.CompletedTask); + } + mock.Setup(c => c.GetCertificateAsync(order, It.IsAny())) + .ReturnsAsync(MockCertificateData.IssuedCertRecord(order)); + + var validator = new FakeDomainValidator { CleanupDelay = cleanupDelay }; + var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator), + DcvConfig(dcvWaitForIssuanceSeconds: 10)); + + var sw = System.Diagnostics.Stopwatch.StartNew(); + await Enroll(plugin); + sw.Stop(); + + validator.CleanedUpKeys.Should().HaveCount(3, "all three staged domains must be cleaned up"); + + // This flow carries ~2s of fixed overhead unrelated to cleanup (DcvPropagationDelaySeconds + // and WaitForDcvVerificationAsync's poll interval both floor at 1s each — DcvConfig's + // propagationDelaySeconds default is deliberately 1, since 0 falls back to a 30s default + // in PerformDcvIfNeededAsync, not "no delay"). An 800ms-per-domain cleanup delay makes the + // concurrent-vs-sequential gap (≈800ms vs ≈2400ms of cleanup time) large relative to that + // fixed cost and to CI jitter. 4000ms sits well above "fixed overhead + one 800ms delay" + // and well below "fixed overhead + three 800ms delays run one after another". + sw.ElapsedMilliseconds.Should().BeLessThan(4000, + "cleanup for independent domains must run concurrently, not sequentially — " + + "3 domains x 800ms sequential would add roughly 3x this call's actual cleanup time"); + } + /// /// Regression: a StageValidation failure on one domain of a multi-domain order must not /// leave the TXT records already published for the earlier domains orphaned. Before the @@ -1314,7 +1304,10 @@ public async Task Dcv_StageFailureOnSecondDomain_DoesNotAbortTheGoodDomain() mock.Setup(c => c.GetCertificateAsync(order, It.IsAny())) .ReturnsAsync(MockCertificateData.IssuedCertRecord(order)); - var validator = new PartiallyFailingDomainValidator(failingHostnameSubstring: bad); + var validator = new FakeDomainValidator + { + ShouldFail = key => key.Contains(bad, StringComparison.OrdinalIgnoreCase) + }; var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator), DcvConfig(dcvWaitForIssuanceSeconds: 10)); diff --git a/CERTInext.Tests/FakeDomainValidator.cs b/CERTInext.Tests/FakeDomainValidator.cs index 6b42475..d3917ec 100644 --- a/CERTInext.Tests/FakeDomainValidator.cs +++ b/CERTInext.Tests/FakeDomainValidator.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. // At http://www.apache.org/licenses/LICENSE-2.0 +using System; using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; @@ -21,10 +22,20 @@ internal sealed class FakeDomainValidator : IDomainValidator /// All keys passed to . public List CleanedUpKeys { get; } = new(); + /// All CancellationTokens passed to . + public List CleanupTokens { get; } = new(); + /// When false, returns a failure result. public bool StageSucceeds { get; init; } = true; - /// Error message returned when is false. + /// + /// When set, overrides on a per-key basis — e.g. + /// key => key.Contains("bad", StringComparison.OrdinalIgnoreCase) to fail only a + /// specific hostname in a multi-domain test while the others still stage successfully. + /// + public Func ShouldFail { get; init; } + + /// Error message returned when a StageValidation call fails. public string StageError { get; init; } = "Stage failed (test stub)"; public void Initialize(IDomainValidatorConfigProvider configProvider) { } @@ -32,18 +43,39 @@ public void Initialize(IDomainValidatorConfigProvider configProvider) { } public Task StageValidation(string key, string value, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); - StagedRecords.Add((key, value)); + bool fail = ShouldFail?.Invoke(key) ?? !StageSucceeds; + if (!fail) + StagedRecords.Add((key, value)); + return Task.FromResult(new DomainValidationResult { - Success = StageSucceeds, - ErrorMessage = StageSucceeds ? null : StageError + Success = !fail, + ErrorMessage = fail ? StageError : null }); } - public Task CleanupValidation(string key, CancellationToken cancellationToken) + /// + /// Artificial delay applied inside before completing — lets + /// tests distinguish "cleanup calls run concurrently" (wall time ~= one delay) from + /// "cleanup calls run sequentially" (wall time ~= N x delay). + /// + public TimeSpan CleanupDelay { get; init; } = TimeSpan.Zero; + + // Cleanup calls can genuinely run concurrently (that's what CleanupDelay exists to prove), + // so the two List fields below need a lock — unlike StagedRecords above, which only ever + // sees synchronously-completing calls in practice. + private readonly object _cleanupLock = new(); + + public async Task CleanupValidation(string key, CancellationToken cancellationToken) { - CleanedUpKeys.Add(key); - return Task.FromResult(new DomainValidationResult { Success = true }); + if (CleanupDelay > TimeSpan.Zero) + await Task.Delay(CleanupDelay, cancellationToken); + lock (_cleanupLock) + { + CleanedUpKeys.Add(key); + CleanupTokens.Add(cancellationToken); + } + return new DomainValidationResult { Success = true }; } public Task ValidateConfiguration(Dictionary configuration) => Task.CompletedTask; @@ -53,15 +85,24 @@ public Task CleanupValidation(string key, CancellationTo /// /// Factory that returns a single pre-configured for every - /// domain. Pass null as the validator to simulate "no DNS provider configured". + /// domain, or only for if set. Pass null as the + /// validator to simulate "no DNS provider configured". /// internal sealed class FakeDomainValidatorFactory : IDomainValidatorFactory { private readonly IDomainValidator _validator; + private readonly string _resolvableDomain; - public FakeDomainValidatorFactory(IDomainValidator validator = null) => _validator = validator; + public FakeDomainValidatorFactory(IDomainValidator validator = null, string resolvableDomain = null) + { + _validator = validator; + _resolvableDomain = resolvableDomain; + } - public IDomainValidator ResolveDomainValidator(string domain, string validationType) => _validator; + public IDomainValidator ResolveDomainValidator(string domain, string validationType) => + (_resolvableDomain == null || string.Equals(domain, _resolvableDomain, StringComparison.OrdinalIgnoreCase)) + ? _validator + : null; /// The validator this factory returns; exposed for assertions in tests. public IDomainValidator PrimaryValidator => _validator; diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index 2e84a73..467aa2a 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -1696,8 +1696,19 @@ private async Task PerformDcvIfNeededAsync( // record" means. async Task CleanupPartialStagingAsync() { - foreach (var entry in stagedValidations) - await CleanupOneStagedValidationAsync(entry, " after an early exit from DCV staging"); + // Concurrent, not sequential: each cleanup call already has its own independent + // CleanupValidationTimeoutSeconds bound (see CleanupOneStagedValidationAsync), but + // running them one after another meant that bound was per-call, not in aggregate — a + // UCC order with N staged domains could hold the calling request open for up to + // N × CleanupValidationTimeoutSeconds if the DNS provider was merely slow (not even + // hung) on every delete, which can exceed DcvTimeoutMinutes itself and defeats the + // "entire DCV flow is hard-timeout-bounded" guarantee for exactly the multi-SAN case + // this diff exists to support. Running them concurrently bounds the wall-clock time + // for the whole batch to the slowest single call, regardless of domain count — these + // are independent per-domain operations (different hostnames/records) with no shared + // mutable state, so there is nothing for concurrent execution to race on. + await Task.WhenAll(stagedValidations.Select(entry => + CleanupOneStagedValidationAsync(entry, " after an early exit from DCV staging"))); } // Shared by CleanupPartialStagingAsync above and the try/finally's own cleanup loop @@ -1942,9 +1953,12 @@ async Task CleanupOneStagedValidationAsync( } finally { - // Always clean up staged DNS records — even on failure - foreach (var entry in stagedValidations) - await CleanupOneStagedValidationAsync(entry, ""); + // Always clean up staged DNS records — even on failure. Concurrent, not sequential + // — see CleanupPartialStagingAsync's comment above for why: sequential cleanup made + // the aggregate wall-clock time for this block scale with the number of staged SAN + // domains, unbounded relative to DcvTimeoutMinutes, on this ordinary success path too. + await Task.WhenAll(stagedValidations.Select(entry => + CleanupOneStagedValidationAsync(entry, ""))); } return true; From 0cd840a793e16e09000cb74fd15599ff984db369 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Wed, 12 Aug 2026 18:44:18 -0700 Subject: [PATCH 12/14] =?UTF-8?q?fix(enroll):=20address=20full-review=20ro?= =?UTF-8?q?und=2011=20=E2=80=94=20check-after-await=20cancellation=20race,?= =?UTF-8?q?=20Subject=20sanitization=20(issue=200008)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CERTInext/CERTInextCAPlugin.cs | 42 ++++++++++++++------------- CERTInext/Client/CERTInextClient.cs | 45 ++++++++++++++++++----------- 2 files changed, 50 insertions(+), 37 deletions(-) diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index 467aa2a..360fe29 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -576,15 +576,15 @@ public async Task Enroll( "EnrollmentType={EnrollmentType}, RequestFormat={RequestFormat}, Subject={Subject}, " + "ProfileId={ProfileId}, SANs={SANs}, " + "RequesterName={RequesterName}, RequesterEmail={RequesterEmail}", - enrollmentType, requestFormat, subject, - ep.ProfileId, sanSummary, + enrollmentType, requestFormat, LogSanitizer.Strip(subject), + ep.ProfileId, LogSanitizer.Strip(sanSummary), ep.RequesterName, ep.RequesterEmail); if (string.IsNullOrWhiteSpace(ep.ProfileId)) { _logger.LogError( "Enrollment rejected — ProfileId parameter is missing. Subject={Subject}, EnrollmentType={EnrollmentType}", - subject, enrollmentType); + LogSanitizer.Strip(subject), enrollmentType); throw new Exception($"Template parameter '{Constants.EnrollmentParam.ProfileId}' is required."); } @@ -605,7 +605,7 @@ public async Task Enroll( default: _logger.LogError( "Enrollment rejected — unsupported enrollment type. EnrollmentType={EnrollmentType}, Subject={Subject}", - enrollmentType, subject); + enrollmentType, LogSanitizer.Strip(subject)); throw new NotSupportedException($"Enrollment type '{enrollmentType}' is not supported."); } @@ -617,7 +617,7 @@ public async Task Enroll( "SerialNumber={SerialNumber}, Subject={Subject}, ProfileId={ProfileId}", enrollmentType, result.CARequestID, result.Status, result.Certificate != null ? ExtractSerialFromPem(result.Certificate) : "(pending)", - subject, ep.ProfileId); + LogSanitizer.Strip(subject), ep.ProfileId); _logger.MethodExit(LogLevel.Debug); return result; } @@ -727,7 +727,7 @@ public async Task Revoke(string caRequestID, string hexSerialNumber, uint r _logger.LogWarning( "Revocation skipped — certificate is already revoked. " + "CARequestID={Id}, HexSerialNumber={Serial}, Subject={Subject}", - caRequestID, hexSerialNumber, current.Subject); + caRequestID, hexSerialNumber, LogSanitizer.Strip(current.Subject)); return (int)EndEntityStatus.REVOKED; } @@ -756,7 +756,7 @@ public async Task Revoke(string caRequestID, string hexSerialNumber, uint r "Revocation complete. " + "CARequestID={Id}, HexSerialNumber={Serial}, Subject={Subject}, " + "ReasonCode={ReasonCode}, ReasonString={ReasonString}", - caRequestID, hexSerialNumber, current.Subject, + caRequestID, hexSerialNumber, LogSanitizer.Strip(current.Subject), revocationReason, reasonString); _logger.MethodExit(LogLevel.Debug); return (int)EndEntityStatus.REVOKED; @@ -937,7 +937,8 @@ public async Task Synchronize( status = StatusMapper.ToRequestDisposition(current.Status); _logger.LogDebug( "Sync: refetched order Id={Id} — status={Status}, certBytes={Bytes}, subject={Subject}.", - current.Id, status, current.Certificate?.Length ?? 0, current.Subject); + current.Id, status, current.Certificate?.Length ?? 0, + LogSanitizer.Strip(current.Subject)); } catch (Exception fetchEx) { @@ -970,7 +971,8 @@ public async Task Synchronize( } _logger.LogDebug( "Sync emit: CARequestID={Id}, Status={Status}, CertBytes={CertBytes}, Subject={Subject}", - record.CARequestID, record.Status, record.Certificate?.Length ?? 0, current.Subject); + record.CARequestID, record.Status, record.Certificate?.Length ?? 0, + LogSanitizer.Strip(current.Subject)); blockingBuffer.Add(record, cancelToken); synced++; @@ -1229,7 +1231,7 @@ private async Task RenewOrReissueAsync( _logger.LogInformation( "Renewal/reissue probe — read PriorCertSN from EnrollmentProductInfo. " + "Subject={Subject}, PriorCertSN={PriorCertSN}, RenewalWindowDays={WindowDays}", - subject, string.IsNullOrWhiteSpace(priorCertSn) ? "(none)" : priorCertSn, + LogSanitizer.Strip(subject), string.IsNullOrWhiteSpace(priorCertSn) ? "(none)" : priorCertSn, ep.RenewalWindowDays); if (string.IsNullOrWhiteSpace(priorCertSn)) @@ -1238,7 +1240,7 @@ private async Task RenewOrReissueAsync( // production log filters and are available for anomaly detection. _logger.LogInformation( "Renewal/reissue has no PriorCertSN — treating as new enrollment. Subject={Subject}", - subject); + LogSanitizer.Strip(subject)); return await EnrollNewAsync(csr, subject, san, ep); } @@ -1259,7 +1261,7 @@ private async Task RenewOrReissueAsync( { _logger.LogInformation( "CARequestID for serial '{SN}' is empty — falling back to new enrollment. Subject={Subject}", - priorCertSn, subject); + priorCertSn, LogSanitizer.Strip(subject)); return await EnrollNewAsync(csr, subject, san, ep); } @@ -1308,7 +1310,7 @@ private async Task RenewOrReissueAsync( _logger.LogInformation( "Renewal via CERTInext renew API started. " + "PriorCARequestID={PriorId}, Subject={Subject}, ProfileId={ProfileId}", - priorCaRequestId, subject, ep.ProfileId); + priorCaRequestId, LogSanitizer.Strip(subject), ep.ProfileId); var renewReq = new RenewCertificateRequest { @@ -1345,7 +1347,7 @@ private async Task RenewOrReissueAsync( { _logger.LogInformation( "Certificate '{Id}' is outside the renewal window ({Window} days) — issuing new certificate. Subject={Subject}", - priorCaRequestId, ep.RenewalWindowDays, subject); + priorCaRequestId, ep.RenewalWindowDays, LogSanitizer.Strip(subject)); return await EnrollNewAsync(csr, subject, san, ep); } } @@ -2462,6 +2464,9 @@ void Add(string type, string value, bool fromCsr = false) if (fromCsr) fromCsrKeys.Add(key); } + string FormatSans(IEnumerable sans) => + LogSanitizer.Strip(string.Join("; ", sans.Select(s => $"{s.Type}:{s.Value}"))); + // AnyCA passes SANs keyed by type name — the real gateway uses "dnsname", // "rfc822name", "ipaddress"; MapSanType normalizes the spelling variants. if (san != null) @@ -2551,8 +2556,7 @@ void Add(string type, string value, bool fromCsr = false) "SubmitNonDnsSans is false: {Sans}. The order will issue, but the certificate will " + "NOT contain these names. Set SubmitNonDnsSans back to true to submit them and have " + "CERTInext surface the problem instead. Subject={Subject}", - nonDns.Count, LogSanitizer.Strip(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}"))), - LogSanitizer.Strip(subject)); + nonDns.Count, FormatSans(nonDns), LogSanitizer.Strip(subject)); result = result .Where(s => string.Equals(s.Type, "dns", StringComparison.OrdinalIgnoreCase)) @@ -2577,8 +2581,7 @@ void Add(string type, string value, bool fromCsr = false) _logger.LogInformation( "Resolved {Total} SAN(s) for submission. FromGatewayRequest={FromGateway}, " + "AddedFromCsrFallback={FromCsr}, Sans={Sans}, Subject={Subject}", - result.Count, fromGatewayKept, fromCsrKept, - LogSanitizer.Strip(string.Join("; ", result.Select(s => $"{s.Type}:{s.Value}"))), + result.Count, fromGatewayKept, fromCsrKept, FormatSans(result), LogSanitizer.Strip(subject)); if (fromCsrKept > 0) @@ -2605,8 +2608,7 @@ void Add(string type, string value, bool fromCsr = false) "purpose: a visible failure is preferable to a certificate issued without names the " + "subscriber requested. Remove them from the CSR or the enrollment pattern if the " + "order should proceed. Subject={Subject}", - nonDns.Count, LogSanitizer.Strip(string.Join("; ", nonDns.Select(s => $"{s.Type}:{s.Value}"))), - LogSanitizer.Strip(subject)); + nonDns.Count, FormatSans(nonDns), LogSanitizer.Strip(subject)); } return result; diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs index ceccf25..9ecde2b 100644 --- a/CERTInext/Client/CERTInextClient.cs +++ b/CERTInext/Client/CERTInextClient.cs @@ -1335,18 +1335,34 @@ private async Task ExecuteWithRetryAsync( { resp = await _http.ExecuteAsync(req, ct); - // This client is built with ThrowOnAnyError=false (see the constructor), so a - // cancelled ct does not surface as OperationCanceledException from ExecuteAsync — - // RestSharp catches HttpClient.SendAsync's cancellation internally and returns a - // non-throwing, unsuccessful RestResponse instead. Left unchecked, that response - // reaches DeserializeOrThrow and becomes a plain Exception indistinguishable from a - // genuine API failure — which is exactly how a caller such as - // PerformDcvIfNeededAsync's shared DCV-timeout cancellation was still landing in a - // generic "GetDcv failed" per-domain catch instead of the cancellation-specific one, - // even after that method was hardened to re-throw a real OperationCanceledException - // past its per-domain catches. Surface the true cancellation here, at the one place - // in the client that actually holds `ct`, before any retry or error-wrapping logic - // sees the response. + // Success or 4xx client error — return immediately, checked BEFORE the + // cancellation check below. `_http.ExecuteAsync` already ran to completion by the + // time control reaches this line; whether `ct` has *since* flipped to cancelled is + // a separate, unsynchronized fact (a check-after-await race, not a fabricated one — + // a CancellationTokenSource(TimeSpan) callback and this awaited Task's completion + // are not mutually exclusive events). A deadline (the shared DcvTimeoutMinutes + // budget) firing at essentially the same instant a call genuinely succeeded must not + // discard that success: for VerifyDcv specifically, discarding it here would abort + // PerformDcvIfNeededAsync's loop before WaitForDcvVerificationAsync ever ran, and + // its finally block would delete the just-staged TXT record even though CERTInext + // had genuinely received the verify trigger — turning a real CA-side success into a + // self-inflicted DCV failure. + bool isClientError = (int)resp.StatusCode >= 400 && (int)resp.StatusCode < 500; + if (resp.IsSuccessful || isClientError) + return resp; + + // Only for a call that did NOT succeed: this client is built with + // ThrowOnAnyError=false (see the constructor), so a cancelled ct does not surface as + // OperationCanceledException from ExecuteAsync — RestSharp catches + // HttpClient.SendAsync's cancellation internally and returns a non-throwing, + // unsuccessful RestResponse instead. Left unchecked, that response reaches + // DeserializeOrThrow and becomes a plain Exception indistinguishable from a genuine + // API failure — which is exactly how a caller such as PerformDcvIfNeededAsync's + // shared DCV-timeout cancellation was still landing in a generic "GetDcv failed" + // per-domain catch instead of the cancellation-specific one, even after that method + // was hardened to re-throw a real OperationCanceledException past its per-domain + // catches. Surface the true cancellation here, at the one place in the client that + // actually holds `ct`, before any retry or error-wrapping logic sees the response. // // Throwing here means every caller's own per-call audit line (Method/Path/HttpStatus/ // LatencyMs, logged after ExecuteWithRetryAsync returns) never executes for the @@ -1365,11 +1381,6 @@ private async Task ExecuteWithRetryAsync( } ct.ThrowIfCancellationRequested(); - // Success or 4xx client error — return immediately - bool isClientError = (int)resp.StatusCode >= 400 && (int)resp.StatusCode < 500; - if (resp.IsSuccessful || isClientError) - return resp; - if (attempt < attempts) { Logger.LogWarning( From d37b93543f4562fc10dfe2e39e6d367558a8f5a4 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Thu, 13 Aug 2026 09:02:13 -0700 Subject: [PATCH 13/14] fix(enroll): don't report GENERATED with no certificate body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CERTInext can mark an order auto-approved (certificateStatusId 15) before the certificate bytes are actually generated. The immediate GetCertificate after order placement then fails, but the legacy client still reported Status=issued with Certificate=null, and BuildEnrollmentResult trusted that status over the missing body — handing the gateway framework a GENERATED result with no PEM, which crashes CertificateConverterFactory.FromPEM downstream (confirmed against a live support escalation, UCSD order 5435716354). Demote GENERATED to EXTERNALVALIDATION whenever the certificate body is missing, matching the invariant PickUpEnrolledCertificateAsync already enforces on its own GENERATED branch. --- CERTInext.Tests/CERTInextCAPluginTests.cs | 28 +++++++++++++++++++++++ CERTInext.Tests/MockCertificateData.cs | 14 ++++++++++++ CERTInext/CERTInextCAPlugin.cs | 10 ++++++++ CHANGELOG.md | 1 + 4 files changed, 53 insertions(+) diff --git a/CERTInext.Tests/CERTInextCAPluginTests.cs b/CERTInext.Tests/CERTInextCAPluginTests.cs index 7064b44..5154146 100644 --- a/CERTInext.Tests/CERTInextCAPluginTests.cs +++ b/CERTInext.Tests/CERTInextCAPluginTests.cs @@ -357,6 +357,34 @@ public async Task Enroll_New_ReturnsPendingStatus_WhenCaReturnsPendingApproval() result.Status.Should().Be((int)EndEntityStatus.EXTERNALVALIDATION); } + [Fact] + public async Task Enroll_New_ReturnsPendingStatus_WhenCaReportsIssuedButBodyMissing() + { + // CERTInext can report an "issued"/auto-approved certificateStatusId before the + // certificate bytes actually exist — the immediate GetCertificate download fails + // and the legacy client returns Status="issued" with Certificate=null. Reporting + // GENERATED with no PEM crashes the gateway framework's PEM parser downstream, so + // the plugin must demote this to pending rather than trust the raw status string. + var mock = NewMock(); + mock.Setup(c => c.EnrollCertificateAsync( + It.IsAny(), + It.IsAny())) + .ReturnsAsync(MockCertificateData.AutoApprovedNoBodyEnrollResponse()); + + var plugin = BuildPluginWithPickup(mock.Object, retries: 0); + + var result = await plugin.Enroll( + csr: MockCertificateData.FakeCsrPem, + subject: "CN=test.example.com", + san: null, + productInfo: MakeProductInfo(), + requestFormat: RequestFormat.PKCS10, + enrollmentType: EnrollmentType.New); + + result.Status.Should().Be((int)EndEntityStatus.EXTERNALVALIDATION); + result.Certificate.Should().BeNullOrEmpty(); + } + // --------------------------------------------------------------------------- // Synchronous certificate pickup (Sectigo parity) // --------------------------------------------------------------------------- diff --git a/CERTInext.Tests/MockCertificateData.cs b/CERTInext.Tests/MockCertificateData.cs index ee6644b..7714152 100644 --- a/CERTInext.Tests/MockCertificateData.cs +++ b/CERTInext.Tests/MockCertificateData.cs @@ -294,6 +294,20 @@ public static EnrollCertificateResponse PendingEnrollResponse(string id = null) Message = "Awaiting approval." }; + // Reproduces the CERTInext "auto-approved" race: TrackOrder reports a + // certificateStatusId the client legacy-maps to "issued", but the immediate + // GetCertificate download failed (cert bytes not generated yet), so no PEM + // ever arrived. See issue 0009. + public static EnrollCertificateResponse AutoApprovedNoBodyEnrollResponse(string id = null) => + new EnrollCertificateResponse + { + Id = id ?? CertId1, + Status = "issued", + Certificate = null, + ProfileId = ProfileIdTls, + Message = "Order auto-approved." + }; + // ----------------------------------------------------------------------- // GetCertificate response (object helpers — used by Moq-based plugin tests) // These use the legacy inferred type (LegacyGetCertificateResponse). diff --git a/CERTInext/CERTInextCAPlugin.cs b/CERTInext/CERTInextCAPlugin.cs index 360fe29..a631606 100644 --- a/CERTInext/CERTInextCAPlugin.cs +++ b/CERTInext/CERTInextCAPlugin.cs @@ -2333,6 +2333,16 @@ private EnrollmentResult BuildEnrollmentResult(EnrollCertificateResponse resp, b throw new Exception("CERTInext returned a null enrollment response."); int status = StatusMapper.ToRequestDisposition(resp.Status); + + // CertiNext's "auto-approved"/"downloadable" statuses can arrive before the + // certificate bytes are actually generated — GetCertificate right after order + // placement then fails, leaving resp.Certificate null while resp.Status still + // says issued. Never hand Command a GENERATED result with no PEM (it crashes + // CertificateConverterFactory.FromPEM downstream); demote to pending instead, + // matching the same invariant PickUpEnrolledCertificateAsync already enforces. + if (status == (int)EndEntityStatus.GENERATED && string.IsNullOrWhiteSpace(resp.Certificate)) + status = (int)EndEntityStatus.EXTERNALVALIDATION; + string message; switch (status) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3ca570..10e9c1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ ## Bug Fixes - **UCC certificates no longer come back with only the common name.** SANs requested on a Multi-Domain (UCC) enrollment were not reaching CERTInext at all: the gateway supplies them under the key `dnsname`, which the plugin did not recognize, so the order was submitted with an empty additional-domains list and the CA issued a certificate containing just the CN. SANs supplied on the CSR did not compensate, because CERTInext does not read the CSR's subjectAltName extension — names must be submitted explicitly. The plugin now recognizes every SAN key the gateway sends, and additionally reads SANs out of the CSR itself so a name requested only there still reaches the certificate. The submitted domain list is now logged at the point it goes on the wire, so a missing SAN can be traced without guesswork. - **Renewals no longer lose their SANs.** Certificates renewed through the CA's renew path were submitted with no additional domains and took their primary domain from the previous order's requestor name rather than the certificate subject, so a renewed UCC certificate came back holding a single, possibly wrong, domain. +- **Enrollment no longer fails on an order CERTInext approves before it finishes issuing.** CERTInext can mark an order auto-approved before the certificate itself is ready for download; when this happened, the plugin reported the enrollment as issued anyway with no certificate attached, which the gateway could not process and the enrollment failed outright. Enrollment now recognizes this case and returns pending instead, exactly as it does for any other certificate not yet ready — it is picked up automatically once CERTInext finishes issuing it. ## Upgrade Notes - **Requests carrying a non-DNS SAN (IP address, email, URI) now behave differently.** Previously these were silently discarded and the certificate issued without them. They are now submitted, because issuing a certificate that quietly omits names the subscriber asked for is the worse outcome. CERTInext accepts such a value as an order domain but it cannot pass domain validation, so the order will not issue until the SAN is removed from the request — the gateway log names the offending SANs. If you need the previous behavior while you clean up your templates, set the new connector setting **`SubmitNonDnsSans`** to `false` (default `true`) to submit DNS names only. Public TLS certificates cannot contain IP or email SANs in the first place, so most deployments are unaffected. From 21a06604ff3f0f955e0ae06dd279e67086f02d54 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Thu, 13 Aug 2026 09:04:32 -0700 Subject: [PATCH 14/14] docs(changelog): trim 1.0.1 entries to plain, concise bullets The 1.0.1 section had ballooned into multi-sentence paragraphs per bullet. Cut each down to the essential fact for a customer skimming release notes; no information dropped, just the padding. --- CHANGELOG.md | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10e9c1d..11bd7b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,17 +1,16 @@ # 1.0.1 ## Features -- **Faster enrollment for quickly-issued certificates.** Enrollment now waits briefly for the certificate and returns it in the same request when it issues fast (DV and already-approved orders), instead of always waiting for the next synchronization. Two new optional settings control the wait: `PickupRetries` (default 5; set to `0` to disable) and `PickupDelay` (default 10 seconds) — about a 55-second wait by default, with a built-in ceiling so it can't run long enough to time out the enrollment. Orders that don't issue in that window — including OV/EV, which CERTInext validates asynchronously over minutes to hours — return pending and are imported by a later sync, exactly as before. Works with or without DNS-based DCV. +- **Faster enrollment for quickly-issued certificates.** Enrollment now waits briefly and returns the certificate in the same request when it issues fast, instead of always waiting for the next sync. Configurable via `PickupRetries` (default 5, `0` disables) and `PickupDelay` (default 10s). Orders that don't issue in time (e.g. OV/EV) return pending and are picked up by the next sync, as before. ## Bug Fixes -- **UCC certificates no longer come back with only the common name.** SANs requested on a Multi-Domain (UCC) enrollment were not reaching CERTInext at all: the gateway supplies them under the key `dnsname`, which the plugin did not recognize, so the order was submitted with an empty additional-domains list and the CA issued a certificate containing just the CN. SANs supplied on the CSR did not compensate, because CERTInext does not read the CSR's subjectAltName extension — names must be submitted explicitly. The plugin now recognizes every SAN key the gateway sends, and additionally reads SANs out of the CSR itself so a name requested only there still reaches the certificate. The submitted domain list is now logged at the point it goes on the wire, so a missing SAN can be traced without guesswork. -- **Renewals no longer lose their SANs.** Certificates renewed through the CA's renew path were submitted with no additional domains and took their primary domain from the previous order's requestor name rather than the certificate subject, so a renewed UCC certificate came back holding a single, possibly wrong, domain. -- **Enrollment no longer fails on an order CERTInext approves before it finishes issuing.** CERTInext can mark an order auto-approved before the certificate itself is ready for download; when this happened, the plugin reported the enrollment as issued anyway with no certificate attached, which the gateway could not process and the enrollment failed outright. Enrollment now recognizes this case and returns pending instead, exactly as it does for any other certificate not yet ready — it is picked up automatically once CERTInext finishes issuing it. +- **UCC certificates no longer come back with only the common name.** The gateway sends SANs under the key `dnsname`, which the plugin didn't recognize, so orders went out with an empty domain list. SANs are now read from every key the gateway sends, plus from the CSR itself. +- **Renewals no longer lose their SANs.** Renewals were submitted with no additional domains and the wrong primary domain; both now come from the certificate being renewed. +- **Enrollment no longer fails on an order CERTInext auto-approves before it finishes issuing.** The plugin used to report these as issued with no certificate attached, which the gateway rejected. It now returns pending and picks up the certificate once CERTInext finishes issuing it. ## Upgrade Notes -- **Requests carrying a non-DNS SAN (IP address, email, URI) now behave differently.** Previously these were silently discarded and the certificate issued without them. They are now submitted, because issuing a certificate that quietly omits names the subscriber asked for is the worse outcome. CERTInext accepts such a value as an order domain but it cannot pass domain validation, so the order will not issue until the SAN is removed from the request — the gateway log names the offending SANs. If you need the previous behavior while you clean up your templates, set the new connector setting **`SubmitNonDnsSans`** to `false` (default `true`) to submit DNS names only. Public TLS certificates cannot contain IP or email SANs in the first place, so most deployments are unaffected. - -- **No more duplicate or orphaned orders after a network timeout.** Order and CSR submissions are no longer retried after a network timeout. A timeout can happen *after* the CA has already accepted the request, so the automatic retry was being rejected as a duplicate — failing the enrollment and leaving an orphaned order behind. These requests now run once; if the order was created it is imported by the next synchronization, and duplicate responses are reported with clear, actionable guidance. (Read-only calls are unaffected and still retry.) +- **Non-DNS SANs (IP, email, URI) are now submitted instead of silently dropped.** CERTInext can't validate them, so such an order won't issue until the SAN is removed. Set `SubmitNonDnsSans` to `false` to restore the old drop-silently behavior. +- **No more duplicate or orphaned orders after a network timeout.** Order/CSR submissions no longer auto-retry after a timeout, since the CA may have already created the order. If it was created, the next sync imports it. # 1.0.0