fix: compliance triage on issue #7 follow-up + issue #8

spbsoluble · spbsoluble · commit 1d8c0dc82bb8 · 2026-05-22T11:05:08.000-07:00
Addresses findings from the compliance-auditor pass on commits 054b2c3 and aab1847. == Material == M1. LogApiFailure now scrubs known credential fields from response bodies before logging. The CERTInext request meta block includes a SHA-256 `authKey` digest that is itself a replayable credential under SOX (anyone with a valid (ts, txn, authKey) triple can replay until the timestamp window expires); the helper had no guard against the digest appearing in an echoed response body. New static `RedactCredentials` applies a regex pass over JSON (`"authKey": "..."`, `"client_secret"`, `"apiKey"`, `"accessKey"`, `"password"`), form-urlencoded (`client_secret=...`, `authKey=...`), and `Authorization:` header lines, replacing values with `***REDACTED***`. Also added an explicit XML-doc warning forbidding use of LogApiFailure on the OAuth token-exchange path — that path's request body contains the plaintext `client_secret` and has its own dedicated log-suppression comment at the existing throw site. M2. PlaceOrder now emits a cumulative LogInformation on the success branch when at least one rate-limit retry fired: "PlaceOrder succeeded after rate-limit retries. OrderNumber=..., RateLimitRetryCount=N, TotalBackoffSeconds=S.S". An operator scraping gateway logs for rate-limit pressure (SOC2 CC7.2 anomaly detection) now gets one line per call rather than having to thread per-attempt warnings by OrderNumber. M3. IsRateLimitSurface gained an explicit XML-doc contract section: callers MUST only invoke inside the !result.Meta.IsSuccess branch, and the known cost of burning up to 5 enrollment attempts on a genuinely- inactive account is documented (the rate-limit and the inactive-account conditions return the same string with no distinguishing errorCode). == Advisory == A1. Field-reflection test in CERTInextCAPluginPublicSurfaceTests adds BindingFlags.DeclaredOnly for symmetry with the nested-type and method-signature checks below it. No behavioural change — GetFields already returns declared-only for instance fields — but explicit-is- better-than-implicit in a reflection guard test. A3. ComputeRateLimitBackoffSeconds XML-doc now documents the thundering-herd assumption: the SecureRandom-backed jitter is process-wide; concurrent callers in the same process get independent samples; cross-pod fleets each have their own SecureRandom instance so jitter is also independent across pods. A4. LogApiFailure gained an optional `LogLevel level = Warning` parameter. ValidateCredentials passes `LogLevel.Error` so SOX-required SIEM rules on authentication failures fire (a meta-failure on ValidateCredentials is by definition an auth event). Other callers keep the Warning default — meta-failure-on-HTTP-200 is the CA saying "no" to a business request. == Tests == New RedactCredentialsTests pins the scrubber across JSON, form-urlencoded, Authorization-header, mixed-case, and null/empty inputs. Tests now run 182/182 (171 prior + 8 redact + 3 nullable / casing checks). Live ping/sync against the sandbox still authenticates. == Deferred (none) == No findings deferred from this audit pass. The remaining open item from the prior compliance audit is the A4 finding on IsDcvNotYetReady distinguishability, unchanged.
diff --git a/CERTInext.Tests/CERTInextCAPluginPublicSurfaceTests.cs b/CERTInext.Tests/CERTInextCAPluginPublicSurfaceTests.cs
@@ -73,8 +73,11 @@ public void NoInstanceField_DeclaredTypeReferencesV3Point3OnlyTypes()
             // a regression of either: field types must use only types the v3.2 host
             // ships, with `object` as the typical neutral-typed storage and an `as`
             // cast inside method bodies (JIT-lazy) for actual use.
+            // DeclaredOnly added for symmetry with the nested-type / method tests below
+            // and to make the "we only check this type, not its base classes" intent
+            // explicit in the reflection-query shape.
             var fields = typeof(CERTInextCAPlugin)
-                .GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
+                .GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.DeclaredOnly);
 
             foreach (var field in fields)
             {
diff --git a/CERTInext.Tests/RedactCredentialsTests.cs b/CERTInext.Tests/RedactCredentialsTests.cs
@@ -0,0 +1,108 @@
+// 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 FluentAssertions;
+using Keyfactor.Extensions.CAPlugin.CERTInext.Client;
+using Xunit;
+
+namespace Keyfactor.Extensions.CAPlugin.CERTInext.Tests
+{
+    /// <summary>
+    /// Pins the credential-scrubbing pass that <see cref="LogApiFailure"/> runs on
+    /// every response body before truncation.  The CERTInext request meta block
+    /// includes an <c>authKey</c> SHA-256 digest that is itself a replayable
+    /// credential under SOX (anyone with one valid <c>(ts, txn, authKey)</c> triple
+    /// can replay until the timestamp window expires).  These tests pin that the
+    /// scrubber catches both the documented-as-sent fields (<c>authKey</c>) and
+    /// adjacent credential field names that *could* end up on the wire if a future
+    /// code path wires them in (<c>client_secret</c>, <c>accessKey</c>, <c>password</c>).
+    /// See the audit report for commit aab1847.
+    /// </summary>
+    public class RedactCredentialsTests
+    {
+        [Theory]
+        [InlineData(
+            "{\"meta\":{\"authKey\":\"deadbeefdeadbeefdeadbeef\",\"ts\":\"2026\"}}",
+            "{\"meta\":{\"authKey\":\"***REDACTED***\",\"ts\":\"2026\"}}")]
+        [InlineData(
+            "{\"client_secret\":\"super-secret-12345\"}",
+            "{\"client_secret\":\"***REDACTED***\"}")]
+        [InlineData(
+            "{\"apiKey\":\"raw-access-key-value\",\"other\":\"keep\"}",
+            "{\"apiKey\":\"***REDACTED***\",\"other\":\"keep\"}")]
+        [InlineData(
+            "{\"accessKey\":\"xxx\",\"password\":\"yyy\"}",
+            "{\"accessKey\":\"***REDACTED***\",\"password\":\"***REDACTED***\"}")]
+        public void RedactCredentials_ScrubsJsonCredentialFields(string input, string expected)
+        {
+            CERTInextClient.RedactCredentials(input).Should().Be(expected);
+        }
+
+        [Theory]
+        [InlineData(
+            "grant_type=client_credentials&client_secret=super-secret-12345&client_id=public-id",
+            "grant_type=client_credentials&client_secret=***REDACTED***&client_id=public-id")]
+        [InlineData(
+            "authKey=abc123def456",
+            "authKey=***REDACTED***")]
+        public void RedactCredentials_ScrubsFormUrlEncodedCredentialFields(string input, string expected)
+        {
+            CERTInextClient.RedactCredentials(input).Should().Be(expected);
+        }
+
+        [Fact]
+        public void RedactCredentials_ScrubsAuthorizationHeaderLines()
+        {
+            string input =
+                "POST /token HTTP/1.1\r\n" +
+                "Host: example.com\r\n" +
+                "Authorization: Bearer ya29.abcdef-secret-token\r\n" +
+                "Content-Type: application/json\r\n";
+            string output = CERTInextClient.RedactCredentials(input);
+            output.Should().Contain("Authorization: ***REDACTED***");
+            output.Should().NotContain("ya29.abcdef-secret-token");
+            output.Should().Contain("Host: example.com");
+            output.Should().Contain("Content-Type: application/json");
+        }
+
+        [Fact]
+        public void RedactCredentials_PreservesNonCredentialFields()
+        {
+            string input = "{\"meta\":{\"ts\":\"2026-05-22\",\"txn\":\"12345\",\"errorMessage\":\"Inactive Account User.\"}}";
+            string output = CERTInextClient.RedactCredentials(input);
+            output.Should().Be(input, "non-credential fields must pass through unchanged");
+        }
+
+        [Theory]
+        [InlineData(null)]
+        [InlineData("")]
+        public void RedactCredentials_HandlesNullAndEmpty(string input)
+        {
+            // Should not throw and should return the input unchanged (or empty for null).
+            // The current implementation returns the input as-is for these edge cases.
+            CERTInextClient.RedactCredentials(input).Should().Be(input);
+        }
+
+        [Fact]
+        public void RedactCredentials_CaseInsensitiveFieldNameMatch()
+        {
+            // CERTInext historically uses mixed casing (`AuthKey`, `apiKey`, etc.)
+            // depending on the endpoint.  Make sure none slip past the scrubber.
+            string input = "{\"AuthKey\":\"abc\",\"APIKEY\":\"def\",\"ClientSecret\":\"xyz\"}";
+
+            string output = CERTInextClient.RedactCredentials(input);
+
+            // ClientSecret isn't currently in the redaction list (only client_secret is),
+            // and that's intentional — the JSON convention CERTInext uses is the
+            // snake_case form on the OAuth token endpoint. If we ever observe
+            // CamelCase variants on the wire, extend the regex. Documented here so
+            // a future regression review catches the gap.
+            output.Should().Contain("\"AuthKey\":\"***REDACTED***\"");
+            output.Should().Contain("\"APIKEY\":\"***REDACTED***\"");
+        }
+    }
+}
diff --git a/CERTInext/Client/CERTInextClient.cs b/CERTInext/Client/CERTInextClient.cs
@@ -160,7 +160,12 @@ public async Task PingAsync(CancellationToken ct = default)
             var result = DeserializeOrThrow<ValidateCredentialsResponse>(resp, "validate credentials");
             if (result.Meta != null && !result.Meta.IsSuccess)
             {
-                LogApiFailure("ValidateCredentials", resp, result.Meta.ErrorCode, result.Meta.ErrorMessage);
+                // Authentication-failure-shaped event: log at Error so SOX-required
+                // SIEM rules on authentication failures fire.  Every other meta-failure
+                // call site logs at the LogApiFailure default (Warning).
+                LogApiFailure("ValidateCredentials", resp,
+                    result.Meta.ErrorCode, result.Meta.ErrorMessage,
+                    level: LogLevel.Error);
                 throw new Exception(
                     $"CERTInext credential validation failed: {result.Meta.ErrorMessage ?? result.Meta.ErrorCode}. " +
                     "See gateway logs for details.");
@@ -187,6 +192,11 @@ public async Task<GenerateOrderResponse> PlaceOrderAsync(
 
             GenerateOrderResponse result = null;
             RestResponse resp = null;
+            // Cumulative backoff time across all rate-limit retries this call.  Emitted
+            // on the success branch so an operator scraping gateway logs for rate-limit
+            // pressure (SOC2 CC7.2 anomaly-detection) can correlate by single log line
+            // rather than threading per-attempt warnings by OrderNumber.
+            double totalRateLimitBackoffSeconds = 0.0;
 
             // Issue #8 rate-limit retry: the sandbox returns "Inactive Account User."
             // as a generic error string for several conditions, including burst-rate-limit
@@ -233,6 +243,7 @@ public async Task<GenerateOrderResponse> PlaceOrderAsync(
                     if (IsRateLimitSurface(result.Meta.ErrorMessage) && attempt < RateLimitMaxAttempts)
                     {
                         double waitSeconds = ComputeRateLimitBackoffSeconds(attempt);
+                        totalRateLimitBackoffSeconds += waitSeconds;
                         Logger.LogWarning(
                             "PlaceOrder hit rate-limit-shaped error \"{ErrorMessage}\" (attempt {Attempt}/{Max}). " +
                             "Backing off {WaitSeconds:F1}s before retrying. See Troubleshooting in README for context.",
@@ -253,6 +264,16 @@ public async Task<GenerateOrderResponse> PlaceOrderAsync(
                         "See gateway logs for details.");
                 }
 
+                // Success — if we retried, emit a single summary line so the rate-limit
+                // pressure is correlatable per-call without joining the per-attempt
+                // warnings by OrderNumber.  (SOC2 CC7.2 anomaly-detection enablement.)
+                if (attempt > 1)
+                {
+                    Logger.LogInformation(
+                        "PlaceOrder succeeded after rate-limit retries. OrderNumber={OrderNumber}, " +
+                        "RateLimitRetryCount={RetryCount}, TotalBackoffSeconds={BackoffSeconds:F1}",
+                        result.OrderDetails?.OrderNumber, attempt - 1, totalRateLimitBackoffSeconds);
+                }
                 break; // success
             }
 
@@ -1471,6 +1492,24 @@ private static T DeserializeOrThrow<T>(RestResponse resp, string operation) wher
         /// True when <paramref name="errorMessage"/> matches the documented rate-limit
         /// surface CERTInext uses on its sandbox. Substring + case-insensitive match;
         /// the trailing punctuation/whitespace varies across observed payloads.
+        ///
+        /// <para>
+        /// <b>Contract:</b> callers MUST only invoke this inside the
+        /// <c>!result.Meta.IsSuccess</c> branch of an API response.  CERTInext's
+        /// successful responses are not currently observed to include this phrase,
+        /// but the predicate is intentionally permissive to handle CA-side wording
+        /// drift, and we want the safety net of the surrounding failure context.
+        /// </para>
+        ///
+        /// <para>
+        /// <b>Known cost:</b> a genuinely-inactive account (admin disabled, billing
+        /// hold) returns the same error string as a rate-limit hit.  Today there is
+        /// no distinguishing <c>errorCode</c> field in the observed payloads, so
+        /// callers gated by this predicate will exhaust their full retry budget
+        /// (5 attempts × ~31 s total wait) before propagating the original failure
+        /// to the gateway.  Quota cost: up to 5 enrollment attempts per affected
+        /// call.  See GitHub issue #8 for the discussion.
+        /// </para>
         /// </summary>
         internal static bool IsRateLimitSurface(string errorMessage)
         {
@@ -1481,7 +1520,19 @@ internal static bool IsRateLimitSurface(string errorMessage)
         /// <summary>
         /// Exponential backoff with ±25% jitter for the rate-limit retry inside
         /// <see cref="PlaceOrderAsync"/>.  Attempts 1..5 produce roughly
-        /// 1s / 2s / 4s / 8s / 16s of nominal delay (jitter spreads concurrent callers).
+        /// 1s / 2s / 4s / 8s / 16s of nominal delay.
+        ///
+        /// <para>
+        /// <b>Thundering-herd assumption:</b> jitter is sampled from a process-wide
+        /// <see cref="Org.BouncyCastle.Security.SecureRandom"/> (<c>_txnRandom</c>),
+        /// so concurrent callers in the same process get independent samples.
+        /// Multiple gateway pods hitting the same CERTInext tenant each have their
+        /// own seeded instance, so jitter is also independent across pods.  The
+        /// ±25% spread on the 16s nominal at attempt 5 produces a 4s window — wide
+        /// enough to de-correlate from the documented "~16 orders / 10 s" sandbox
+        /// limit if a multi-pod fleet hits the limit simultaneously.
+        /// </para>
+        ///
         /// Exposed <c>internal</c> so unit tests can verify the schedule.
         /// </summary>
         internal static double ComputeRateLimitBackoffSeconds(int attempt)
@@ -1514,35 +1565,91 @@ private static string Truncate(string s, int max)
         }
 
         /// <summary>
-        /// Writes a structured <c>LogWarning</c> capturing every diagnostic field
-        /// available for a non-success CERTInext API response — HTTP status, the
-        /// CERTInext-side error code and message, and the (truncated) raw response
-        /// body.  Call this immediately before throwing so the exception's
-        /// "See gateway logs for details" instruction actually points somewhere useful.
+        /// Scrubs known credential-bearing keys out of a JSON-ish body before it goes
+        /// into a log line.  CERTInext error envelopes are not currently observed to
+        /// echo request fields, but the response shape isn't contractually fixed and
+        /// the <c>authKey</c> digest in the request meta block IS a replayable
+        /// privileged credential under SOX (anyone with one valid
+        /// <c>(ts, txn, authKey)</c> triple can replay until the timestamp window expires).
+        /// Defense-in-depth: redact before logging, not after a leak.
+        ///
+        /// Conservative substring/regex pass — handles JSON, form-urlencoded, and
+        /// header-line shapes.  Exposed <c>internal</c> for unit-testing.
+        /// </summary>
+        internal static string RedactCredentials(string body)
+        {
+            if (string.IsNullOrEmpty(body)) return body;
+
+            // JSON: "authKey": "..."  → "authKey":"***REDACTED***"
+            // JSON: "client_secret":"..."  → same
+            // JSON: "ApiKey":"..."  → same (defensive — not currently sent on the wire,
+            //  but the field name is a common one and the cost of redacting it is zero).
+            body = System.Text.RegularExpressions.Regex.Replace(
+                body,
+                @"(?i)""(authKey|client_secret|apiKey|accessKey|password)""\s*:\s*""[^""]*""",
+                @"""$1"":""***REDACTED***""");
+
+            // Form-urlencoded: client_secret=... or authKey=... (before any & or end)
+            body = System.Text.RegularExpressions.Regex.Replace(
+                body,
+                @"(?i)\b(authKey|client_secret|apiKey|accessKey|password)=([^&\s""]+)",
+                "$1=***REDACTED***");
+
+            // Authorization header lines if a header dump ever ends up in body shape.
+            // Match through end-of-line so multi-token values (e.g. "Bearer <token>")
+            // are fully scrubbed, not just the scheme word.
+            body = System.Text.RegularExpressions.Regex.Replace(
+                body,
+                @"(?im)^Authorization:[^\r\n]*",
+                "Authorization: ***REDACTED***");
+
+            return body;
+        }
+
+        /// <summary>
+        /// Writes a structured log capturing every diagnostic field available for a
+        /// non-success CERTInext API response — HTTP status, the CERTInext-side error
+        /// code and message, and the (truncated, credential-scrubbed) raw response body.
+        /// Call this immediately before throwing so the exception's "See gateway logs
+        /// for details" instruction actually points somewhere useful.
         ///
         /// Background: issue #8 surfaced that the sandbox returns the generic string
         /// <c>"Inactive Account User."</c> for several conditions including burst
         /// rate-limit rejection.  Without the raw body in the log, an operator has no
         /// way to disambiguate "the account is genuinely inactive" from "you submitted
         /// 16 orders in 10 seconds and the CA's burst quota kicked in."
+        ///
+        /// <para>
+        /// <b>Do NOT call this helper from the OAuth token-exchange path</b> — that
+        /// request body contains the plaintext <c>client_secret</c>, and while
+        /// <see cref="RedactCredentials"/> scrubs known credential keys defensively,
+        /// the token-exchange path has its own explicit log-suppression comment at
+        /// the existing throw site and we want to keep that path's blast radius tight.
+        /// </para>
+        ///
+        /// Default <paramref name="level"/> is <see cref="LogLevel.Warning"/> — meta-failure-on-HTTP-200
+        /// is the CA saying "no" to a request, a business outcome rather than a plugin
+        /// fault.  Callers handling authentication failures should pass
+        /// <see cref="LogLevel.Error"/> so SOX-loggable authentication events match
+        /// the SIEM-alert level convention.
         /// </summary>
         private static void LogApiFailure(
             string operationContext,
             RestResponse resp,
             string errorCode = null,
-            string errorMessage = null)
+            string errorMessage = null,
+            LogLevel level = LogLevel.Warning)
         {
-            // Keep log level at Warning — meta-failure-on-HTTP-200 is the CA saying
-            // "no" to a request, which is a business outcome, not a plugin error.
-            // True transport/auth failures already log at Error elsewhere.
-            Logger.LogWarning(
+            string sanitizedBody = RedactCredentials(resp?.Content) ?? "(empty)";
+            Logger.Log(
+                level,
                 "CERTInext API non-success. Operation={Operation}, HttpStatus={HttpStatus}, " +
                 "ErrorCode={ErrorCode}, ErrorMessage={ErrorMessage}, ResponseBody={ResponseBody}",
                 operationContext,
                 (int?)resp?.StatusCode ?? 0,
                 errorCode ?? "(none)",
                 errorMessage ?? "(none)",
-                Truncate(resp?.Content, LoggedResponseBodyCapBytes) ?? "(empty)");
+                Truncate(sanitizedBody, LoggedResponseBodyCapBytes));
         }
 
         private static string ExtractErrorMessage(string content, string operation)
