From ca34a2d65da6452e05a9fc79e9f139f14c73b23d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 12:17:27 -0400 Subject: [PATCH] fix(logging): fail-closed bearer redaction; hoist per-request logger creation --- docs/Diagnostics.md | 64 ++++------ .../Diagnostics/GatewayLogRedactor.cs | 118 +++++++++++++----- ...tewayRequestLoggingMiddlewareExtensions.cs | 12 +- .../Diagnostics/GatewayLogRedactorTests.cs | 61 +++++++++ 4 files changed, 182 insertions(+), 73 deletions(-) diff --git a/docs/Diagnostics.md b/docs/Diagnostics.md index 70c24d0..bec160c 100644 --- a/docs/Diagnostics.md +++ b/docs/Diagnostics.md @@ -84,42 +84,32 @@ The names match the MXAccess command list in `gateway.md` exactly. `Write` and ` ### API key redaction -`RedactApiKey` is built around the `mxgw_` API key format issued by the gateway. It preserves the bearer scheme and the key id segment so that operators can correlate a log entry to a specific principal, but always strips the secret tail: +`RedactClientIdentity` is the single redaction path for identity-bearing values; `RedactApiKey` is a +name-preserving alias for it. Redaction **fails closed**: the only value that survives with any of its +content is a gateway-issued `mxgw__` key, whose key id is kept so operators can +correlate a log entry to a specific principal. -```csharp -public static string? RedactApiKey(string? authorizationHeader) -{ - if (string.IsNullOrWhiteSpace(authorizationHeader)) - { - return authorizationHeader; - } +| Input | Output | Why | +|-------|--------|-----| +| `Bearer mxgw_operator01_super-secret` | `Bearer mxgw_operator01_[redacted]` | Recognized gateway key; key id identifies the principal | +| `Bearer eyJhbGciOi…` (any foreign token) | `Bearer [redacted]` | Structure is unknown, so the whole credential goes | +| `Basic dXNlcjpwYXNz` | `Basic [redacted]` | Same, for any recognized scheme | +| `Bearer mxgw_operator01` (no secret separator) | `Bearer mxgw_[redacted]` | No trustworthy key-id boundary | +| `Bearer` (scheme only), `anonymous`, `some junk` | `[redacted]` | No scheme/credential split that can be trusted | +| `null`, `""`, whitespace | unchanged | Nothing to redact | - const string bearerPrefix = "Bearer "; - if (!authorizationHeader.StartsWith(bearerPrefix, StringComparison.OrdinalIgnoreCase)) - { - return RedactedValue; - } +A scheme word survives only when it is one of the recognized authorization schemes (`Bearer`, +`Basic`, `Digest`, `Negotiate`, `NTLM`, `ApiKey`, `Token`). An unrecognized leading word is as likely +to be credential material as it is to be a scheme, so it is dropped along with the rest. The key id is +also dropped when it runs longer than 64 characters, which no issued key id does — a long run before +the first `_` is secret material, not an identifier. - string token = authorizationHeader[bearerPrefix.Length..].Trim(); +The parse is span-based (no regex, no `Split` allocation): the value is split once at the first space, +and the key id is read up to the first `_` of the remainder. - if (!token.StartsWith("mxgw_", StringComparison.OrdinalIgnoreCase)) - { - return $"{bearerPrefix}{RedactedValue}"; - } - - string[] tokenParts = token.Split('_', 3, StringSplitOptions.RemoveEmptyEntries); - if (tokenParts.Length < 2) - { - return $"{bearerPrefix}mxgw_{RedactedValue}"; - } - - return $"{bearerPrefix}mxgw_{tokenParts[1]}_{RedactedValue}"; -} -``` - -The split uses `count: 3` because the secret portion may itself contain underscores; only the first two segments (`mxgw` and the key id) are kept verbatim. Authorization headers that are not bearer tokens are reduced to `[redacted]` rather than passed through, since the gateway cannot reason about their structure. - -`RedactClientIdentity` is the entry point used by `GatewayLogScope` and `DashboardRedactor`. It only invokes `RedactApiKey` when the input contains the `mxgw_` marker, leaving non-key identities (for example, Windows account names) untouched. +The consequence for callers is that a non-key identity (for example a Windows account name) reaching +`RedactClientIdentity` is now replaced rather than passed through. `DashboardRedactor` routes only +values containing the `mxgw_` marker here, so dashboard display names are unaffected. ### Command value redaction @@ -160,12 +150,12 @@ public static IApplicationBuilder UseGatewayRequestLoggingScope(this IApplicatio { ArgumentNullException.ThrowIfNull(app); + ILogger logger = app.ApplicationServices + .GetRequiredService() + .CreateLogger("MxGateway.Request"); + return app.Use(async (context, next) => { - ILogger logger = context.RequestServices - .GetRequiredService() - .CreateLogger("ZB.MOM.WW.MxGateway.Request"); - using IDisposable? scope = logger.BeginGatewayScope(new GatewayLogScope( SessionId: ReadHeader(context, SessionIdHeaderName), WorkerProcessId: ReadInt32Header(context, WorkerProcessIdHeaderName), @@ -190,7 +180,7 @@ The scope is keyed off four custom headers and the standard `authorization` head The numeric headers use `int.TryParse` and `ulong.TryParse`; missing or unparseable values become `null` and are dropped by `GatewayLogScope.ToDictionary`. This keeps the middleware tolerant of clients that do not yet emit every header, which matters because the earliest call in a session (`OpenSession`) has no `SessionId` to send. -The logger category is `ZB.MOM.WW.MxGateway.Request`, which lets operators filter the request scope events independently from per-component categories. +The logger category is `MxGateway.Request`, which lets operators filter the request scope events independently from per-component categories. The logger is resolved once at registration rather than per request: the category is fixed, so a per-request `IServiceProvider` resolve and `ILoggerFactory.CreateLogger` (which takes the factory lock) bought nothing. Scope construction itself stays unconditional — gating it on `ILogger.IsEnabled` would drop scope state for providers and scope consumers registered after startup. ### Pipeline ordering diff --git a/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayLogRedactor.cs b/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayLogRedactor.cs index b22b081..bc517d9 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayLogRedactor.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayLogRedactor.cs @@ -15,6 +15,28 @@ public static class GatewayLogRedactor "WriteSecured2" }; + /// + /// Authorization schemes whose name may survive redaction. Anything outside this list is + /// dropped whole: an unrecognized leading word is as likely to be credential material as it + /// is to be a scheme, so it is not worth the leak. + /// + private static readonly string[] KnownAuthorizationSchemes = + [ + "Bearer", + "Basic", + "Digest", + "Negotiate", + "NTLM", + "ApiKey", + "Token", + ]; + + /// Prefix identifying a gateway-issued API key. + private const string GatewayKeyPrefix = "mxgw_"; + + /// Upper bound on a key id kept in the clear; a longer run is treated as secret material. + private const int MaxKeyIdLength = 64; + /// /// Determines whether a command method bears credentials. /// @@ -27,44 +49,24 @@ public static class GatewayLogRedactor } /// - /// Redacts the API key secret portion of a Bearer authorization header. + /// Redacts the credential portion of an authorization header value. /// /// The authorization header value to redact. - /// The header with the secret portion redacted, or the original value when it is null, blank, or not a Bearer header. + /// The header with the credential redacted, or the original value when it is null or blank. public static string? RedactApiKey(string? authorizationHeader) { - if (string.IsNullOrWhiteSpace(authorizationHeader)) - { - return authorizationHeader; - } - - const string bearerPrefix = "Bearer "; - if (!authorizationHeader.StartsWith(bearerPrefix, StringComparison.OrdinalIgnoreCase)) - { - return RedactedValue; - } - - string token = authorizationHeader[bearerPrefix.Length..].Trim(); - - if (!token.StartsWith("mxgw_", StringComparison.OrdinalIgnoreCase)) - { - return $"{bearerPrefix}{RedactedValue}"; - } - - string[] tokenParts = token.Split('_', 3, StringSplitOptions.RemoveEmptyEntries); - if (tokenParts.Length < 2) - { - return $"{bearerPrefix}mxgw_{RedactedValue}"; - } - - return $"{bearerPrefix}mxgw_{tokenParts[1]}_{RedactedValue}"; + return RedactClientIdentity(authorizationHeader); } /// - /// Redacts the client identity if it contains an API key. + /// Redacts the credential carried by a client identity. Redaction fails closed: only a + /// gateway-issued API key keeps its mxgw_<key-id>_ shape (so operators can tell keys + /// apart in logs), and only a recognized scheme keeps its name. Every other value — a foreign + /// bearer token, a scheme-less string, junk — is replaced whole, because nothing that reaches + /// this method is known to be safe to log. /// /// The client identity string to redact. - /// The redacted client identity, or the original value when it contains no API key. + /// The redacted client identity, or the original value when it is null or blank. public static string? RedactClientIdentity(string? clientIdentity) { if (string.IsNullOrWhiteSpace(clientIdentity)) @@ -72,9 +74,61 @@ public static class GatewayLogRedactor return clientIdentity; } - return clientIdentity.Contains("mxgw_", StringComparison.OrdinalIgnoreCase) - ? RedactApiKey(clientIdentity) - : clientIdentity; + ReadOnlySpan value = clientIdentity.AsSpan().Trim(); + int separatorIndex = value.IndexOf(' '); + + if (separatorIndex < 0) + { + // A single token carries no scheme, so the token itself is the credential. + return RedactedValue; + } + + ReadOnlySpan scheme = value[..separatorIndex]; + ReadOnlySpan credential = value[(separatorIndex + 1)..].Trim(); + + if (credential.IsEmpty || !IsKnownAuthorizationScheme(scheme)) + { + return RedactedValue; + } + + return credential.StartsWith(GatewayKeyPrefix, StringComparison.OrdinalIgnoreCase) + ? $"{scheme} {GatewayKeyPrefix}{RedactKeyId(credential)}" + : $"{scheme} {RedactedValue}"; + } + + /// + /// Renders the trailing portion of a gateway API key: the key id when the key is well formed, + /// otherwise nothing but the placeholder. + /// + /// The credential, known to start with the gateway key prefix. + /// The <key-id>_[redacted] tail, or just the placeholder. + private static string RedactKeyId(ReadOnlySpan credential) + { + ReadOnlySpan remainder = credential[GatewayKeyPrefix.Length..]; + int secretIndex = remainder.IndexOf('_'); + + // No separator means no secret boundary to trust, so the whole remainder is treated as secret. + return secretIndex is <= 0 or > MaxKeyIdLength + ? RedactedValue + : $"{remainder[..secretIndex]}_{RedactedValue}"; + } + + /// + /// Determines whether a leading word is a recognized authorization scheme. + /// + /// The candidate scheme word. + /// when the word may survive redaction; otherwise . + private static bool IsKnownAuthorizationScheme(ReadOnlySpan scheme) + { + foreach (string knownScheme in KnownAuthorizationSchemes) + { + if (scheme.Equals(knownScheme, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + + return false; } /// diff --git a/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayRequestLoggingMiddlewareExtensions.cs b/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayRequestLoggingMiddlewareExtensions.cs index 2b0e11b..2ae0de3 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayRequestLoggingMiddlewareExtensions.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayRequestLoggingMiddlewareExtensions.cs @@ -24,12 +24,16 @@ public static class GatewayRequestLoggingMiddlewareExtensions { ArgumentNullException.ThrowIfNull(app); + // Resolved once at registration: the logger is keyed by category, not by request, so the + // per-request DI resolve and logger-factory lock bought nothing. + ILogger logger = app.ApplicationServices + .GetRequiredService() + .CreateLogger("MxGateway.Request"); + return app.Use(async (context, next) => { - ILogger logger = context.RequestServices - .GetRequiredService() - .CreateLogger("MxGateway.Request"); - + // Scope construction is deliberately unconditional: gating it on IsEnabled would drop + // scope state for providers (and scope consumers) registered after startup. using IDisposable? scope = logger.BeginGatewayScope(new GatewayLogScope( SessionId: ReadHeader(context, SessionIdHeaderName), WorkerProcessId: ReadInt32Header(context, WorkerProcessIdHeaderName), diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs index eedd129..70df716 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs @@ -24,6 +24,67 @@ public sealed class GatewayLogRedactorTests Assert.DoesNotContain("super_secret_value", redacted); } + /// + /// Verifies that a bearer credential the gateway does not issue is redacted too. A client that + /// pastes a JWT (or any other token) into the authorization header must not have it logged. + /// + [Fact] + public void RedactClientIdentity_RedactsNonGatewayBearerCredential() + { + const string token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxIn0.c2lnbmF0dXJl"; + + string? redacted = GatewayLogRedactor.RedactClientIdentity($"Bearer {token}"); + + Assert.Equal("Bearer [redacted]", redacted); + Assert.DoesNotContain(token, redacted, StringComparison.Ordinal); + Assert.DoesNotContain("eyJ", redacted, StringComparison.Ordinal); + } + + /// Verifies that a gateway API key keeps its key-id shape so an operator can still tell keys apart. + [Fact] + public void RedactClientIdentity_PreservesGatewayKeyIdShape() + { + string? redacted = GatewayLogRedactor.RedactClientIdentity("Bearer mxgw_operator01_super-secret"); + + Assert.Equal("Bearer mxgw_operator01_[redacted]", redacted); + Assert.DoesNotContain("super-secret", redacted, StringComparison.Ordinal); + } + + /// + /// Verifies that anything not recognized as a gateway API key fails closed: the scheme word + /// survives only when it looks like an auth scheme, and the credential never does. + /// + /// The raw client identity value. + /// The expected redacted value. + [Theory] + [InlineData("Bearer", "[redacted]")] + [InlineData("Bearer ", "[redacted]")] + [InlineData("Basic dXNlcjpwYXNzd29yZA==", "Basic [redacted]")] + [InlineData("Negotiate YIIJvwYGKwYBBQUCoIIJ", "Negotiate [redacted]")] + [InlineData("mxgw_operator01_super-secret", "[redacted]")] + [InlineData("mxgw_operator01_super-secret trailing", "[redacted]")] + [InlineData("Bearer mxgw_operator01", "Bearer mxgw_[redacted]")] + [InlineData("Bearer mxgw_", "Bearer mxgw_[redacted]")] + [InlineData("Bearer mxgw__super-secret", "Bearer mxgw_[redacted]")] + [InlineData("bearer mxgw_operator01_super-secret", "bearer mxgw_operator01_[redacted]")] + [InlineData("anonymous", "[redacted]")] + [InlineData("some random junk", "[redacted]")] + public void RedactClientIdentity_FailsClosedForUnrecognizedCredentials(string clientIdentity, string expected) + { + Assert.Equal(expected, GatewayLogRedactor.RedactClientIdentity(clientIdentity)); + } + + /// Verifies that a blank client identity is passed through — there is nothing to redact. + /// The raw client identity value. + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void RedactClientIdentity_PassesThroughBlankValues(string? clientIdentity) + { + Assert.Equal(clientIdentity, GatewayLogRedactor.RedactClientIdentity(clientIdentity)); + } + /// Verifies that IsCredentialBearingCommand identifies credential-bearing MXAccess commands. /// Name of the MXAccess command method. [Theory]