fix+chore: audit triage (dev-review + SOX/SOC2 compliance pass)

Combined triage of two parallel audit reports on the PR-#2 change set since
commit 6a2db50. Both ran against the live codebase, not just the diff.

== ca-rest-plugin-dev (technical review) ==

Blocker B1 — ExtractSerialFromPem hex format parity:
  BouncyCastle BigInteger.ToString(16) drops the leading-zero nibble in the
  most-significant byte, producing "A123456" where X509Certificate2.SerialNumber
  returns "0A123456". That mismatch breaks audit-log correlation against the
  serial Keyfactor Command stores at issue time. Switched to
  Convert.ToHexString(cert.SerialNumber.ToByteArrayUnsigned()) which preserves
  every byte exactly. New ExtractSerialFromPemTests pins five cases including
  the leading-zero-byte regression.

Blocker B2 — cached-DCV path missing OrderStatusId terminal guard:
  PerformDcvIfNeededAsync's new "return true on Status=1" cached-DCV path
  could fire on a cancelled or rejected order whose domainVerification.Status
  was carried over from a prior validated round — sending the caller into a
  wasted DcvWaitForIssuanceSeconds-long GetCertificate poll. Added the same
  OrderStatusId "4"/"5" check that WaitForDcvVerificationAsync uses, ahead of
  the AlreadyValidated branch. New Dcv_Skipped_WhenOrderStatusIdIsTerminal
  Theory pins both states.

Should-fix S1: _domainValidatorFactory marked `volatile` so cross-thread
  SetDomainValidatorFactory writes are immediately visible to Enroll/Sync.

Should-fix S2: README compatibility note rewritten — `SetDomainValidatorFactory`
  is the gateway host's integration point, not something operators call.

Should-fix S3: footer "Production codes 838–847" note replaced with a
  pointer to the per-product sandbox/production table above; the range
  notation was confusing because some codes (e.g. 842) appear in both
  columns for different products.

Nit N1: clarifying comment on Times.Exactly(1) expectation in the
  single-shot test.

Nit N2: SetDomainValidatorFactory_SecondCall_OverridesFirst converted from
  reflection-on-field to a behavioural test driving Enroll and asserting
  which factory's validator was actually used.

== compliance-auditor (SOX / SOC1 / SOC2) ==

Critical C1: WaitForDcvVerificationAsync gained a defense-in-depth absolute
  deadline so a future caller passing CancellationToken.None can't make
  the loop unbounded (SOX CC7.3).

Critical C3 (verify-and-document): added an explicit comment at the OAuth2
  token-acquisition site forbidding logging of tokenResp.Content / .ErrorMessage
  / .ErrorException — RestSharp echoes the original request body on failure,
  which contains the client_secret (SOX CC6.1).

Material M1: SetDomainValidatorFactory now logs every call (offered type +
  whether the cast succeeded) so an auditor can confirm which DNS provider
  drove DCV (SOX change-management).

Material M2: ValidateCAConnectionInfo and ValidateProductInfo blank out
  ApiKey/OAuthClientSecret/Password on the transient tempConfig after the
  validate call so they aren't reachable from the still-rooted tempClient
  instance (SOC2 CC6.1 best-effort).

Material M3: Synchronize gained an error-rate threshold — aborts and throws
  when error rate exceeds 25% over a sample of ≥50 records, so a CA-side
  outage cannot silently 'complete' a sync with zero useful records (SOC1
  completeness/accuracy).

Material M4: Revoke log line now includes ManagedThreadId for correlation
  against any RequestingUser scope the gateway host enriches (SOX
  segregation-of-duties evidence).

Material M5: RenewOrReissueAsync logs the PriorCertSN probe at Information
  (SOC2 CC6.1 logical-access event).

Material M6: BuildDefaultAgreementDetails logs a Warning when SignerIp
  falls back to 127.0.0.1 — submitting that to a public CA is a
  misrepresentation in the legal audit record (SOC1 accuracy-of-processing).

Material M7: ExtractErrorMessage caps the response-body size at 64 KB
  before JsonDocument.Parse to prevent memory-exhaustion DoS via a hostile
  CA response (SOC2 CC7.2).

Advisory A1: GenerateTxnId switched from Random.Shared.NextDouble to
  Org.BouncyCastle.Security.SecureRandom.NextLong — txn is part of the
  authKey HMAC input, so cryptographic randomness is appropriate. Matches
  the project's BouncyCastle-only crypto policy.

Advisory A2: Initialize now warns when DcvEnabled=true but no factory is
  injected — surfaces a silent functional downgrade.

Advisory A3: ExtractSerialFromPem catch-all logs the suppression at Debug
  (audit visibility without breaking the never-throw contract).

Advisory A5: Initialize log line gained DcvEnabled, DcvTxtRecordTemplate,
  and DomainValidatorFactoryInjected so DCV configuration is visible on
  every plugin restart.

Advisory A6: in-flight DCV collision log promoted from Debug to Information
  — concurrent attempts are security-relevant events.

== Tests ==

153/153 unit tests pass. Live Ping/Sync still authenticate against the
sandbox (the BouncyCastle SecureRandom-based GenerateTxnId produces a
valid authKey end-to-end).

== Deferred follow-ups (not in this commit) ==

C2 (auditor): generic CERTInext error-message passthrough flagged as a
  theoretical leak vector. Current behaviour matches every other CERTInext
  API consumer; rewriting all throw sites to drop the underlying message
  would noticeably hurt diagnostics. Worth a follow-up issue if the
  compliance team wants tighter belt-and-braces.

A4 (auditor): make IsDcvNotYetReady deferral distinguishable from
  "no DCV required" in the return code so audit trail shows deferral cause.
  Worth doing but requires touching every PerformDcvIfNeededAsync caller.

Author: spbsoluble

CERTInext.Tests/CERTInextCAPluginDcvTests.cs

@@ -371,28 +371,85 @@ public async Task SetDomainValidatorFactory_AfterConstruction_WiresFactoryForSub
         }
 
         [Fact]
-        public void SetDomainValidatorFactory_SecondCall_OverridesFirst()
+        public async Task SetDomainValidatorFactory_SecondCall_OverridesFirst()
         {
             // Property-style setter semantics: the most recent SetDomainValidatorFactory
             // call wins. Important for gateway hosts that may resolve a fresh factory
-            // per-initialize cycle.
-            var plugin = new CERTInextCAPlugin();
+            // per-initialize cycle.  Tested behaviorally — drive Enroll() and assert
+            // the SECOND factory's validator received the TXT staging call (no reflection
+            // on internal fields).
+            var (mock, _) = HappyPathMocks();
             var firstValidator = new FakeDomainValidator();
             var secondValidator = new FakeDomainValidator();
 
+            var plugin = new CERTInextCAPlugin(
+                mock.Object,
+                domainValidatorFactory: null,
+                DcvConfig(dcvWaitForIssuanceSeconds: 10));
+
+            // First setter call is ignored by the override; only the second factory's
+            // validator should ever see traffic.
             plugin.SetDomainValidatorFactory(new FakeDomainValidatorFactory(firstValidator));
             plugin.SetDomainValidatorFactory(new FakeDomainValidatorFactory(secondValidator));
 
-            // The plugin uses _domainValidatorFactory through internal methods; we reach
-            // the field via reflection to assert the second factory is the one stored.
-            var field = typeof(CERTInextCAPlugin)
-                .GetField("_domainValidatorFactory",
-                    System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
-            field.Should().NotBeNull();
-            var stored = field!.GetValue(plugin) as FakeDomainValidatorFactory;
-            stored.Should().NotBeNull();
-            stored!.PrimaryValidator.Should().BeSameAs(secondValidator,
-                "the most recent SetDomainValidatorFactory call must replace the earlier one");
+            var result = await Enroll(plugin);
+
+            result.Status.Should().Be((int)EndEntityStatus.GENERATED);
+            firstValidator.StagedRecords.Should().BeEmpty(
+                "the first factory must be replaced — its validator should never be called");
+            secondValidator.StagedRecords.Should().NotBeEmpty(
+                "the second SetDomainValidatorFactory call must replace the first; its validator drives DCV");
+        }
+
+        // ---------------------------------------------------------------------------
+        // Cancelled/rejected orders short-circuit even with validated DCV state
+        // ---------------------------------------------------------------------------
+
+        [Theory]
+        [InlineData("4")] // OrderStatusId 4 = Order Cancelled
+        [InlineData("5")] // OrderStatusId 5 = Order Rejected
+        public async Task Dcv_Skipped_WhenOrderStatusIdIsTerminal_EvenIfDcvValidated(string terminalOrderStatusId)
+        {
+            // Regression guard for the cached-DCV path: a cancelled or rejected order
+            // can still have domainVerification.Status="1" carried over from a prior
+            // validated round. Without this guard the plugin would return true from
+            // PerformDcvIfNeededAsync and the caller would spend the full
+            // DcvWaitForIssuanceSeconds budget polling GetCertificate for a cert that
+            // is never going to issue. Per audit report B2 on PR #2.
+            var mock = NewMock();
+            mock.Setup(c => c.EnrollCertificateAsync(It.IsAny<EnrollCertificateRequest>(), It.IsAny<CancellationToken>()))
+                .ReturnsAsync(new EnrollCertificateResponse { Id = MockCertificateData.DcvOrderId, Status = "pending" });
+
+            mock.Setup(c => c.TrackOrderAsync(MockCertificateData.DcvOrderId, It.IsAny<CancellationToken>()))
+                .ReturnsAsync(new TrackOrderResponse
+                {
+                    OrderDetails = new TrackOrderResponseDetails
+                    {
+                        OrderStatusId       = terminalOrderStatusId,
+                        CertificateStatusId = "1",
+                        // Validated DCV state — without the OrderStatusId guard this would
+                        // erroneously trigger the issuance-wait path.
+                        DomainVerification  = new TrackOrderDomainVerification
+                        {
+                            Status = Constants.Dcv.StatusValidated
+                        }
+                    }
+                });
+
+            var validator = new FakeDomainValidator();
+            // Issuance-wait budget > 0 so a wrong-path entry would manifest as a
+            // GetCertificate call we DON'T expect.
+            var plugin = BuildPlugin(mock.Object, new FakeDomainValidatorFactory(validator),
+                DcvConfig(dcvWaitForIssuanceSeconds: 10));
+
+            await Enroll(plugin);
+
+            mock.Verify(c => c.GetCertificateAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()),
+                Times.Never,
+                "Enroll must not enter WaitForIssuanceAfterDcvAsync when the order is " +
+                "cancelled/rejected, even if DCV happens to be in a 'validated' state");
+            validator.StagedRecords.Should().BeEmpty(
+                "DCV staging must not run for a cancelled/rejected order");
         }
 
         // ---------------------------------------------------------------------------
@@ -426,7 +483,10 @@ public async Task SyncDcvRetry_DoesSingleShotTrackOrder_WhenChallengeNotReady()
 
             // GetSingleRecord calls GetCertificateAsync first to materialize the record;
             // the sync-DCV-retry kicks in afterwards.  The pending response keeps the
-            // retry path engaged so we exercise the override.
+            // retry path engaged so we exercise the override.  The assertion below pins
+            // Times.Exactly(1) on TrackOrderAsync: with override=0, the polling loop
+            // takes one TrackOrder call, sees domainVerification null, and bails — no
+            // further polls inside the 60s budget the config nominally allows.
             mock.Setup(c => c.GetCertificateAsync(MockCertificateData.DcvOrderId, It.IsAny<CancellationToken>()))
                 .ReturnsAsync(MockCertificateData.PendingCertRecord(MockCertificateData.DcvOrderId));
 

CERTInext.Tests/ExtractSerialFromPemTests.cs

@@ -0,0 +1,131 @@
+// Copyright 2024 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.Reflection;
+using FluentAssertions;
+using Org.BouncyCastle.Asn1.X509;
+using Org.BouncyCastle.Crypto;
+using Org.BouncyCastle.Crypto.Generators;
+using Org.BouncyCastle.Crypto.Operators;
+using Org.BouncyCastle.Math;
+using Org.BouncyCastle.Security;
+using Org.BouncyCastle.X509;
+using Xunit;
+
+namespace Keyfactor.Extensions.CAPlugin.CERTInext.Tests
+{
+    /// <summary>
+    /// Regression tests for the private <c>CERTInextCAPlugin.ExtractSerialFromPem</c>
+    /// helper, which feeds the audit-log SerialNumber field.  After the BouncyCastle
+    /// migration (replacing <c>X509Certificate2.SerialNumber</c>) we need to pin the
+    /// format invariants — particularly the leading-zero-byte case where the old BCL
+    /// behaviour and a naive <c>BigInteger.ToString(16)</c> diverge.
+    /// </summary>
+    public class ExtractSerialFromPemTests
+    {
+        private static string InvokeExtractSerialFromPem(string pem)
+        {
+            var method = typeof(CERTInextCAPlugin)
+                .GetMethod("ExtractSerialFromPem", BindingFlags.NonPublic | BindingFlags.Static);
+            method.Should().NotBeNull("test pins the format produced by ExtractSerialFromPem");
+            return (string)method!.Invoke(null, new object[] { pem })!;
+        }
+
+        /// <summary>
+        /// Generates a self-signed PEM cert with the specified serial number.  Uses
+        /// BouncyCastle throughout — no BCL crypto — per the project's crypto policy.
+        /// </summary>
+        private static string GeneratePemWithSerial(BigInteger serial)
+        {
+            var keyGen = new RsaKeyPairGenerator();
+            keyGen.Init(new KeyGenerationParameters(new SecureRandom(), 2048));
+            AsymmetricCipherKeyPair keyPair = keyGen.GenerateKeyPair();
+
+            var subject = new X509Name("CN=test-serial-parity");
+            var notBefore = DateTime.UtcNow.AddMinutes(-1);
+            var notAfter = notBefore.AddDays(1);
+
+            var builder = new X509V3CertificateGenerator();
+            builder.SetSerialNumber(serial);
+            builder.SetIssuerDN(subject);
+            builder.SetSubjectDN(subject);
+            builder.SetNotBefore(notBefore);
+            builder.SetNotAfter(notAfter);
+            builder.SetPublicKey(keyPair.Public);
+
+            var signerFactory = new Asn1SignatureFactory("SHA256withRSA", keyPair.Private);
+            X509Certificate cert = builder.Generate(signerFactory);
+
+            return "-----BEGIN CERTIFICATE-----\n"
+                + Convert.ToBase64String(cert.GetEncoded(), Base64FormattingOptions.InsertLineBreaks)
+                + "\n-----END CERTIFICATE-----";
+        }
+
+        [Fact]
+        public void ExtractSerialFromPem_PreservesLeadingZeroByte()
+        {
+            // Serial bytes 0x00 0x0A 0xFF 0xFF as an unsigned big-endian integer = 720895
+            // X509Certificate2.SerialNumber would produce "0AFFFF" (sign byte stripped,
+            // remaining bytes hex-encoded, leading-zero NIBBLE preserved within byte boundary).
+            // A naive BigInteger.ToString(16) would produce "afff" (a 4-digit hex, dropping
+            // the leading zero nibble), which mis-correlates with Command's stored serial.
+            //
+            // Use a serial that has a leading-zero nibble in its first non-zero byte:
+            // 0x0A123456 → unsigned hex "0A123456" (8 nibbles). Anything that drops the
+            // leading zero produces "A123456" (7 nibbles).
+            var serial = new BigInteger("0A123456", 16);
+            string pem = GeneratePemWithSerial(serial);
+
+            string result = InvokeExtractSerialFromPem(pem);
+
+            result.Should().Be("0A123456",
+                "the serial must preserve the leading-zero nibble within its first byte " +
+                "so audit-log correlation against Command's stored serial succeeds");
+        }
+
+        [Fact]
+        public void ExtractSerialFromPem_NormalSerial_UppercaseHexNoLeadingZero()
+        {
+            // Plain mid-range serial; just confirms format is uppercase hex without separators.
+            var serial = new BigInteger("DEADBEEFCAFE", 16);
+            string pem = GeneratePemWithSerial(serial);
+
+            string result = InvokeExtractSerialFromPem(pem);
+
+            result.Should().Be("DEADBEEFCAFE");
+        }
+
+        [Fact]
+        public void ExtractSerialFromPem_LongSerial_AllBytesPreservedUppercase()
+        {
+            // 20-byte serial (the max CA/B Forum permits).  Each byte must be uppercase
+            // hex, no separators, no leading-zero loss.
+            var serial = new BigInteger("01020304050607080910111213141516171819FA", 16);
+            string pem = GeneratePemWithSerial(serial);
+
+            string result = InvokeExtractSerialFromPem(pem);
+
+            result.Should().Be("01020304050607080910111213141516171819FA");
+        }
+
+        [Fact]
+        public void ExtractSerialFromPem_GarbageInput_ReturnsParseError()
+        {
+            // Robustness — audit-log path must never throw, only mark the failure.
+            InvokeExtractSerialFromPem("not a pem")
+                .Should().Be("(parse-error)");
+        }
+
+        [Fact]
+        public void ExtractSerialFromPem_EmptyBody_ReturnsEmptyPem()
+        {
+            InvokeExtractSerialFromPem("-----BEGIN CERTIFICATE-----\n-----END CERTIFICATE-----")
+                .Should().Be("(empty-pem)");
+        }
+    }
+}