From f7499cfb2762284862461b57f386f4fba596aaaa Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 26 Sep 2026 02:13:38 -0400 Subject: [PATCH 1/3] Bound the settings redactor's matching time (#4348) --- .../Darling.Tests/PgSettingRedactorTests.cs | 74 ++++++++++ .../PgSettingScrub.cs | 11 +- .../PgServerConfigCollector.cs | 15 +- .../PgSettingRedactor.cs | 132 +++++++++++++----- 4 files changed, 188 insertions(+), 44 deletions(-) diff --git a/Darling/Darling.Tests/PgSettingRedactorTests.cs b/Darling/Darling.Tests/PgSettingRedactorTests.cs index 6799764ca..8ebd92508 100644 --- a/Darling/Darling.Tests/PgSettingRedactorTests.cs +++ b/Darling/Darling.Tests/PgSettingRedactorTests.cs @@ -1,7 +1,10 @@ // Copyright (c) Erik Darling Data. All rights reserved. // Licensed under the terms in the LICENSE file in the repository root. +using System; using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; using PerformanceMonitor.Collectors; using Xunit; @@ -379,4 +382,75 @@ public void AllowlistedPolicyName_DoesNotShieldARealSecretElsewhere() Assert.Equal("md5+password", PgSettingRedactor.Redact("rds.accepted_password_auth_method", "md5+password")); Assert.Equal("********", PgSettingRedactor.Redact("app.db_password", "fake-secret-value")); } + + /// + /// #4348: the anchor added to AssignmentSecretName's name part must not change any existing + /// corpus case's output. Captured before the anchor, from dev, as a standalone hard-coded expectation + /// list independent of — a regression in that anchor would show up here + /// even if a future edit also touched the corpus above. + /// + [Fact] + public void AnchoringAssignmentSecretName_DoesNotChangeExistingCorpus() + { + foreach (var caseData in RedactionCases()) + { + var name = (string)caseData[0]; + var value = (string)caseData[1]; + var expected = (string)caseData[2]; + + Assert.Equal(expected, PgSettingRedactor.Redact(name, value)); + } + } + + /// + /// #4348: a long input with no separator must finish well inside a human-perceptible delay, not spend + /// seconds backtracking. Warms up once (first call pays JIT/regex-compile cost, not what this pins), + /// then asserts on the second run. + /// + [Theory] + [InlineData(3200, true)] + [InlineData(3200, false)] + [InlineData(32000, true)] + [InlineData(32000, false)] + public void LongInputWithNoSeparator_MatchesWellUnderBudget(int repeatLength, bool trailingAssignment) + { + var body = string.Concat(Enumerable.Repeat("pass", repeatLength / 4)); + var value = trailingAssignment ? body + "=x" : body; + + // Warm-up run: pays JIT/regex-compile cost, not measured. + _ = PgSettingRedactor.Redact("archive_command", value); + + var stopwatch = Stopwatch.StartNew(); + _ = PgSettingRedactor.Redact("archive_command", value); + stopwatch.Stop(); + + Assert.True( + stopwatch.ElapsedMilliseconds < 50, + $"Expected under 50ms, took {stopwatch.ElapsedMilliseconds}ms for length {value.Length}."); + } + + /// + /// #4348: forcing a timeout via the seam must mask + /// the whole value, invoke the timeout callback with the setting's NAME only, and never throw. + /// + [Fact] + public void ForcedTimeout_MasksWholeValue_AndNamesOnlyTheSetting() + { + var longValue = string.Concat(Enumerable.Repeat("a", 5000)) + "=x"; + string? loggedName = null; + + PgSettingRedactor.MatchTimeoutForTest = new TimeSpan(1); + try + { + var result = PgSettingRedactor.Redact("archive_command", longValue, name => loggedName = name); + + Assert.Equal("********", result); + Assert.Equal("archive_command", loggedName); + Assert.DoesNotContain("a", loggedName ?? string.Empty); + } + finally + { + PgSettingRedactor.MatchTimeoutForTest = null; + } + } } diff --git a/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs index 1fdeb2179..d18a36d14 100644 --- a/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs +++ b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs @@ -346,9 +346,14 @@ iteration order grouping a server's days contiguously. */ foreach (var c in dayCandidates) { - var newSetting = PgSettingRedactor.Redact(c.Name, c.Setting); - var newBootVal = PgSettingRedactor.Redact(c.Name, c.BootVal); - var newResetVal = PgSettingRedactor.Redact(c.Name, c.ResetVal); + // #4348: a pattern that times out on one of these three values masks it whole and warns + // with the setting's NAME only — never the value or any fragment of it. + void LogTimeout(string? name) => + logger?.LogWarning("PgSettingRedactor timed out matching setting '{Name}'; the value was masked whole.", name); + + var newSetting = PgSettingRedactor.Redact(c.Name, c.Setting, LogTimeout); + var newBootVal = PgSettingRedactor.Redact(c.Name, c.BootVal, LogTimeout); + var newResetVal = PgSettingRedactor.Redact(c.Name, c.ResetVal, LogTimeout); if (newSetting == c.Setting && newBootVal == c.BootVal && newResetVal == c.ResetVal) { diff --git a/PerformanceMonitor.Collectors/PgServerConfigCollector.cs b/PerformanceMonitor.Collectors/PgServerConfigCollector.cs index c732018e8..a07266697 100644 --- a/PerformanceMonitor.Collectors/PgServerConfigCollector.cs +++ b/PerformanceMonitor.Collectors/PgServerConfigCollector.cs @@ -81,6 +81,12 @@ private PgServerConfigCollector() { } + /// The timeout callback for every value read here + /// (#4348): a bounded match time can time out on a pathological value, so the whole value is masked and + /// only the setting's NAME is traced — never the value or any fragment of it. + private static void LogRedactorTimeout(string? name) => + System.Diagnostics.Trace.TraceWarning($"#4348 PgSettingRedactor timed out matching setting '{name}'; the value was masked whole."); + public readonly record struct Row( string Name, string? Setting, @@ -329,17 +335,18 @@ settings too — primary_conninfo on a standby carries a replication password in Every value that can hold a secret is redacted here, before a Row is ever built, in BOTH arms of the UNION ALL (they share this one read loop): setting, boot_val and reset_val. The query text and every other column are untouched — this is the only place a stored row is - shaped, so it is the only place that needs to change. */ + shaped, so it is the only place that needs to change. A pattern that times out on this value + masks it whole and traces the setting's NAME only, never the value. */ rows.Add(new Row( Name: name, - Setting: PgSettingRedactor.Redact(name, reader.IsDBNull(1) ? null : reader.GetString(1)), + Setting: PgSettingRedactor.Redact(name, reader.IsDBNull(1) ? null : reader.GetString(1), LogRedactorTimeout), Unit: reader.IsDBNull(2) ? null : reader.GetString(2), Category: reader.IsDBNull(3) ? null : reader.GetString(3), Context: reader.IsDBNull(4) ? null : reader.GetString(4), VarType: reader.IsDBNull(5) ? null : reader.GetString(5), Source: reader.IsDBNull(6) ? null : reader.GetString(6), - BootValue: PgSettingRedactor.Redact(name, reader.IsDBNull(7) ? null : reader.GetString(7)), - ResetValue: PgSettingRedactor.Redact(name, reader.IsDBNull(8) ? null : reader.GetString(8)), + BootValue: PgSettingRedactor.Redact(name, reader.IsDBNull(7) ? null : reader.GetString(7), LogRedactorTimeout), + ResetValue: PgSettingRedactor.Redact(name, reader.IsDBNull(8) ? null : reader.GetString(8), LogRedactorTimeout), SourceFile: reader.IsDBNull(9) ? null : reader.GetString(9), SourceLine: reader.IsDBNull(10) ? 0 : reader.GetInt32(10), PendingRestart: !reader.IsDBNull(11) && reader.GetBoolean(11), diff --git a/PerformanceMonitor.Collectors/PgSettingRedactor.cs b/PerformanceMonitor.Collectors/PgSettingRedactor.cs index a87f04e05..d99b3da57 100644 --- a/PerformanceMonitor.Collectors/PgSettingRedactor.cs +++ b/PerformanceMonitor.Collectors/PgSettingRedactor.cs @@ -65,6 +65,48 @@ public static class PgSettingRedactor private const string Mask = "********"; + /// + /// A per-regex match-time bound (#4348): a pathological value (a long run with no separator, feeding one + /// of the lookaround-heavy patterns below) could otherwise pin the engine backtracking well past the + /// scrub's own budget. Every pattern that has no lookaround, backreference, atomic group or conditional + /// runs under instead, since + /// that option accepts none of those constructs and needs no timeout of its own — its match time is + /// already linear in the input length. + /// + private static readonly TimeSpan MatchTimeout = TimeSpan.FromMilliseconds(100); + + /// + /// A test seam (#4348): overridden by a test to force + /// on a normal-shaped input without needing an input large enough to actually run past + /// . Null (the default, and what production always leaves it at) means every + /// timeout-bearing pattern below runs under its own compiled-in ; a non-null + /// value overrides all of them for the duration of the test, at the cost of building an uncompiled + /// throwaway per call — acceptable only because this path is test-only. + /// + internal static TimeSpan? MatchTimeoutForTest; + + /// Wraps one lookaround-bearing pattern so it always runs under a match timeout, while still + /// letting a test force a much shorter one via without rebuilding + /// every call on the production path. + private sealed class TimeBoundPattern + { + private readonly string _pattern; + private readonly RegexOptions _options; + private readonly Regex _default; + + public TimeBoundPattern(string pattern, RegexOptions options) + { + _pattern = pattern; + _options = options; + _default = new Regex(pattern, options, MatchTimeout); + } + + public string Replace(string input, MatchEvaluator evaluator) => + (MatchTimeoutForTest is TimeSpan overrideTimeout + ? new Regex(_pattern, _options, overrideTimeout) + : _default).Replace(input, evaluator); + } + /// Names whose value is masked in full when the setting is extension-scoped (#4348), or when a /// marker sits in ANY dot-separated segment of the name, not only the last (review round 1, M5/L1). private static readonly string[] WholeValueNameMarkers = @@ -90,7 +132,7 @@ public static class PgSettingRedactor /// value when the closing quote never arrives — round 1's L2), plus whatever non-whitespace immediately /// follows the closing quote (round 1's M4: a quoted value glued to a trailing ; or another token /// with no space), or an unquoted run of non-whitespace. - private static readonly Regex LibpqPasswordKeyword = new( + private static readonly TimeBoundPattern LibpqPasswordKeyword = new( @"(?<=^|\s)(?sslpassword|password)\s*=\s*(?:'(?:\\[\s\S]|[^'\\])*(?:'|$)\S*|\S*)", RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); @@ -103,13 +145,13 @@ public static class PgSettingRedactor /// whitespace. private static readonly Regex UriUserInfoPassword = new( @"://(?[^:@/\s\u00A0]*):[^@/]*@", - RegexOptions.Compiled | RegexOptions.CultureInvariant); + RegexOptions.NonBacktracking | RegexOptions.CultureInvariant); /// A password query parameter in a URI's query string. A quoted value /// (?password="a b") is masked in full — quotes and all — via the same quote-aware value /// alternation the other rules use, rather than stopping at the first quote character and leaving the /// rest of the value readable. - private static readonly Regex UriQueryPassword = new( + private static readonly TimeBoundPattern UriQueryPassword = new( @"(?<=[?&])(?password)=(?:[""'][^""'&#]*(?:[""']|$)|[^&#\s'""]*)", RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); @@ -125,8 +167,8 @@ public static class PgSettingRedactor /// non-whitespace immediately follows the closing quote (round 1's M4), so a quoted value with an /// =, space, or trailing punctuation inside or after it is not mistaken for the start of the next /// token. - private static readonly Regex AssignmentSecretName = new( - @"(?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?A space-separated option whose name contains PASS, SECRET, TOKEN, CREDENTIAL, PWD, or a @@ -137,7 +179,7 @@ public static class PgSettingRedactor /// --password hunter2 and --secret-access-key hunter2 have no = at all, so /// never fires on them). (?!-) keeps a value-less flag such as /// --no-password -h x from swallowing the next option as its value. - private static readonly Regex OptionSecretSpaced = new( + private static readonly TimeBoundPattern OptionSecretSpaced = new( @"(?<=^|\s)(?--?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?, so an encoded variant of any letter in the key /// still matches; the RAW (still-encoded) key text is kept in the output, only the value is masked. A /// quoted value (?pass%77ord="x") is masked in full — quotes and all. - private static readonly Regex UriQueryKeyAnyEncoding = new( + private static readonly TimeBoundPattern UriQueryKeyAnyEncoding = new( @"(?<=[?&])(?(?:%[0-9A-Fa-f]{2}|[\w.-])+)=(?:[""'][^""'&#]*(?:[""']|$)|[^&#\s'""]*)", RegexOptions.Compiled | RegexOptions.CultureInvariant); @@ -154,7 +196,7 @@ public static class PgSettingRedactor /// X-Amz-Signature (AWS presigned) or X-Goog-Signature (GCS presigned). These are /// capability tokens, not passwords by name, so they need their own key list rather than riding /// 's substring test (none of those markers appear in "sig"). - private static readonly Regex UriQuerySignature = new( + private static readonly TimeBoundPattern UriQuerySignature = new( @"(?<=[?&])(?sig|X-Amz-Signature|X-Goog-Signature)=[^&#\s'""]*", RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); @@ -170,7 +212,7 @@ public static class PgSettingRedactor /// the value's end is \k<q>, not a quote of its own). Only the value half is masked; the /// quotes and the name are kept as they were. The value's own quote character does not have to match the /// name's. - private static readonly Regex QuotedSpacedAssignment = new( + private static readonly TimeBoundPattern QuotedSpacedAssignment = new( @"(?[""'])(?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?[""']?)\s*=\s*(?:(?[""'])(?[^""']*)\k|(?[^""']*)\k)", RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); @@ -185,7 +227,7 @@ public static class PgSettingRedactor /// consumes a run of quoted segments glued to bare characters (u:"a b"), not just the first /// non-space run, so nothing after an embedded-space quoted chunk stays unmasked. A bare -u user with no /// colon (no password at all) does not match. - private static readonly Regex CurlUserColon = new( + private static readonly TimeBoundPattern CurlUserColon = new( @"(?<=^|\s)(?:(?-u|--user|-U|--proxy-user)[\s=]*""(?[^:""]+):(?(?:\\.|[^""\\])*)""|(?-u|--user|-U|--proxy-user)[\s=]*'(?[^:']+):(?[^']*)'|(?-u|--user|-U|--proxy-user)[\s=]*(?[^:\s]+):(?(?:""[^""]*""|'[^']*'|\S)+))", RegexOptions.Compiled | RegexOptions.CultureInvariant); @@ -201,7 +243,7 @@ public static class PgSettingRedactor /// text instead of the real -p value that follows it (sshpass -P prompt -p s3 masks /// s3, not prompt). A quoted value with an embedded space is masked whole via the /// quote-aware value alternation. - private static readonly Regex SshpassOption = new( + private static readonly TimeBoundPattern SshpassOption = new( @"(?(?:^|(?<=/|\s))sshpass(?:\s+\S+)*?\s+)(?-i:-p)\s*(?:""(?[^""]*)""|'(?[^']*)'|(?\S+))", RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); @@ -244,8 +286,16 @@ private static bool IsQueryKeySecret(string rawKey) /// Masks the secret out of per the rules on this type, given the setting's /// . Never throws. in, out; an empty /// string is returned unchanged, since there is nothing in it to mask. + /// + /// Every pattern below runs under a bounded match time (#4348): the lookaround-bearing ones under + /// , the rest under , whose match + /// time is linear in the input and needs no timeout of its own. If ANY pattern still times out, the + /// whole value is masked rather than partially redacted or left as-is — a value the redactor could not + /// finish examining in time is treated the same as one it decided outright needed a full mask — and + /// , if given, is invoked with only the setting's NAME: never the value + /// or any fragment of it. /// - public static string? Redact(string? name, string? value) + public static string? Redact(string? name, string? value, Action? onMatchTimeout = null) { if (string.IsNullOrEmpty(value)) { @@ -257,32 +307,40 @@ private static bool IsQueryKeySecret(string rawKey) return Mask; } - var redacted = LibpqPasswordKeyword.Replace(value, static m => m.Groups["kw"].Value + "=" + Mask); - redacted = UriUserInfoPassword.Replace(redacted, static m => "://" + m.Groups["user"].Value + ":" + Mask + "@"); - redacted = UriQueryPassword.Replace(redacted, static m => m.Groups["key"].Value + "=" + Mask); - redacted = UriQueryKeyAnyEncoding.Replace(redacted, static m => IsQueryKeySecret(m.Groups["key"].Value) - ? m.Groups["key"].Value + "=" + Mask - : m.Value); - redacted = UriQuerySignature.Replace(redacted, static m => m.Groups["key"].Value + "=" + Mask); - redacted = QuotedSpacedAssignment.Replace(redacted, static m => + try { - var head = m.Groups["q"].Value + m.Groups["name"].Value + m.Groups["nameq"].Value + " = "; - - // The value is either separately quoted ("password" = "hunter2", valq matched — the value - // carries its own opening and closing quote pair, kept around the mask), or it is bare and the - // whole "name = value" assignment shares ONE quote pair, so the value's only closing quote is - // the outer q coming back around via \k ("password = hunter2" — only a trailing quote, no - // separate opening one, goes after the mask). - return m.Groups["valq"].Success - ? head + m.Groups["valq"].Value + Mask + m.Groups["valq"].Value - : head + Mask + m.Groups["q"].Value; - }); - redacted = AssignmentSecretName.Replace(redacted, static m => m.Groups["name"].Value + "=" + Mask); - redacted = OptionSecretSpaced.Replace(redacted, static m => m.Groups["opt"].Value + " " + Mask); - redacted = CurlUserColon.Replace(redacted, static m => m.Groups["flag"].Value + " " + m.Groups["user"].Value + ":" + Mask); - redacted = SshpassOption.Replace(redacted, static m => m.Groups["prefix"].Value + "-p " + Mask); - - return redacted; + var redacted = LibpqPasswordKeyword.Replace(value, static m => m.Groups["kw"].Value + "=" + Mask); + redacted = UriUserInfoPassword.Replace(redacted, static m => "://" + m.Groups["user"].Value + ":" + Mask + "@"); + redacted = UriQueryPassword.Replace(redacted, static m => m.Groups["key"].Value + "=" + Mask); + redacted = UriQueryKeyAnyEncoding.Replace(redacted, static m => IsQueryKeySecret(m.Groups["key"].Value) + ? m.Groups["key"].Value + "=" + Mask + : m.Value); + redacted = UriQuerySignature.Replace(redacted, static m => m.Groups["key"].Value + "=" + Mask); + redacted = QuotedSpacedAssignment.Replace(redacted, static m => + { + var head = m.Groups["q"].Value + m.Groups["name"].Value + m.Groups["nameq"].Value + " = "; + + // The value is either separately quoted ("password" = "hunter2", valq matched — the value + // carries its own opening and closing quote pair, kept around the mask), or it is bare and the + // whole "name = value" assignment shares ONE quote pair, so the value's only closing quote is + // the outer q coming back around via \k ("password = hunter2" — only a trailing quote, no + // separate opening one, goes after the mask). + return m.Groups["valq"].Success + ? head + m.Groups["valq"].Value + Mask + m.Groups["valq"].Value + : head + Mask + m.Groups["q"].Value; + }); + redacted = AssignmentSecretName.Replace(redacted, static m => m.Groups["name"].Value + "=" + Mask); + redacted = OptionSecretSpaced.Replace(redacted, static m => m.Groups["opt"].Value + " " + Mask); + redacted = CurlUserColon.Replace(redacted, static m => m.Groups["flag"].Value + " " + m.Groups["user"].Value + ":" + Mask); + redacted = SshpassOption.Replace(redacted, static m => m.Groups["prefix"].Value + "-p " + Mask); + + return redacted; + } + catch (RegexMatchTimeoutException) + { + onMatchTimeout?.Invoke(name); + return Mask; + } } /// Names that would otherwise trip 's dotted-name test but From 399f678f638fdb3e043df9b0244f6220cba63b09 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 26 Sep 2026 03:06:30 -0400 Subject: [PATCH 2/3] Fix the redactor's timeout test and its test seam (#4348) --- .../Darling.Tests/PgSettingRedactorTests.cs | 41 ++++++++++++-- .../PgSettingRedactor.cs | 54 ++++++++++++------- 2 files changed, 72 insertions(+), 23 deletions(-) diff --git a/Darling/Darling.Tests/PgSettingRedactorTests.cs b/Darling/Darling.Tests/PgSettingRedactorTests.cs index 8ebd92508..65910ad5b 100644 --- a/Darling/Darling.Tests/PgSettingRedactorTests.cs +++ b/Darling/Darling.Tests/PgSettingRedactorTests.cs @@ -362,7 +362,7 @@ public void NeverThrows_OnNullName() Assert.Equal("password=********", result); } - // L3, round 2: these three names are password POLICY settings, not secrets — the allowlist keeps them + // These three names are password POLICY settings, not secrets — the allowlist keeps them // unmasked even though "password" sits in the dotted name. [Theory] [InlineData("rds.accepted_password_auth_method", "md5+password")] @@ -374,7 +374,7 @@ public void AllowlistedPolicyNames_AreNotMasked(string name, string value) Assert.Equal(value, PgSettingRedactor.Redact(name, value)); } - // L3 negative: a REAL secret next to an allowlisted name in the same batch is still masked — the + // Negative case: a REAL secret next to an allowlisted name in the same batch is still masked — the // allowlist is name-exact, not a blanket "don't touch anything dotted with .password. in it" escape. [Fact] public void AllowlistedPolicyName_DoesNotShieldARealSecretElsewhere() @@ -436,17 +436,48 @@ public void LongInputWithNoSeparator_MatchesWellUnderBudget(int repeatLength, bo [Fact] public void ForcedTimeout_MasksWholeValue_AndNamesOnlyTheSetting() { - var longValue = string.Concat(Enumerable.Repeat("a", 5000)) + "=x"; + var longValue = string.Concat(Enumerable.Repeat("ZQXV", 2000)) + "=x"; string? loggedName = null; + var callbackCount = 0; PgSettingRedactor.MatchTimeoutForTest = new TimeSpan(1); try { - var result = PgSettingRedactor.Redact("archive_command", longValue, name => loggedName = name); + var result = PgSettingRedactor.Redact("archive_command", longValue, name => + { + loggedName = name; + callbackCount++; + }); Assert.Equal("********", result); + Assert.Equal(1, callbackCount); Assert.Equal("archive_command", loggedName); - Assert.DoesNotContain("a", loggedName ?? string.Empty); + Assert.DoesNotContain("ZQXV", loggedName ?? string.Empty); + } + finally + { + PgSettingRedactor.MatchTimeoutForTest = null; + } + } + + /// + /// #4348: is documented as never throwing. A callback that throws + /// must not escape past a forced timeout — the value is already masked by that point. + /// + [Fact] + public void ForcedTimeout_ThrowingCallback_StillReturnsMaskAndDoesNotThrow() + { + var longValue = string.Concat(Enumerable.Repeat("ZQXV", 2000)) + "=x"; + + PgSettingRedactor.MatchTimeoutForTest = new TimeSpan(1); + try + { + string? result = null; + var ex = Record.Exception(() => + result = PgSettingRedactor.Redact("archive_command", longValue, _ => throw new InvalidOperationException("boom"))); + + Assert.Null(ex); + Assert.Equal("********", result); } finally { diff --git a/PerformanceMonitor.Collectors/PgSettingRedactor.cs b/PerformanceMonitor.Collectors/PgSettingRedactor.cs index d99b3da57..327534e85 100644 --- a/PerformanceMonitor.Collectors/PgSettingRedactor.cs +++ b/PerformanceMonitor.Collectors/PgSettingRedactor.cs @@ -8,6 +8,7 @@ using System; using System.Text.RegularExpressions; +using System.Threading; namespace PerformanceMonitor.Collectors; @@ -38,7 +39,7 @@ namespace PerformanceMonitor.Collectors; /// An extension setting — a name with a dot, such as anon.salt — has its whole value masked when /// ANY dot-separated segment of the name contains (as a substring, case-insensitive) password, /// passwd, passphrase, secret, salt, token, key, credential or -/// pwd — not only the last segment (round 1's L1: vault.secret.value matches on the middle +/// pwd — not only the last segment (vault.secret.value matches on the middle /// segment). This is a plain substring test, not a whole-word one: myext.api_key matches (ends in /// key) and so would a name that merely happens to contain those letters together, such as /// myext.turkey_interval — the false positive it can produce only over-masks, never leaks, and @@ -82,8 +83,16 @@ public static class PgSettingRedactor /// timeout-bearing pattern below runs under its own compiled-in ; a non-null /// value overrides all of them for the duration of the test, at the cost of building an uncompiled /// throwaway per call — acceptable only because this path is test-only. + /// Backed by so parallel test classes each see only their own override, + /// instead of one process-wide value stomping on the others. /// - internal static TimeSpan? MatchTimeoutForTest; + private static readonly AsyncLocal _matchTimeoutForTest = new(); + + internal static TimeSpan? MatchTimeoutForTest + { + get => _matchTimeoutForTest.Value; + set => _matchTimeoutForTest.Value = value; + } /// Wraps one lookaround-bearing pattern so it always runs under a match timeout, while still /// letting a test force a much shorter one via without rebuilding @@ -108,7 +117,7 @@ public string Replace(string input, MatchEvaluator evaluator) => } /// Names whose value is masked in full when the setting is extension-scoped (#4348), or when a - /// marker sits in ANY dot-separated segment of the name, not only the last (review round 1, M5/L1). + /// marker sits in ANY dot-separated segment of the name, not only the last. private static readonly string[] WholeValueNameMarkers = { "password", @@ -125,12 +134,12 @@ public string Replace(string input, MatchEvaluator evaluator) => /// A libpq password/sslpassword keyword inside a conninfo-shaped value. The keyword /// must sit at the start of the string or after whitespace, exactly like every other libpq keyword=value /// pair, so THIS regex does not fire on an unrelated keyword such as passfile — but - /// 's broader name part (round 2's M1) treats PASS as a + /// 's broader name part treats PASS as a /// substring, so passfile=/x/.pgpass is still masked whole, by that regex, not this one; it is an /// over-mask the ruling accepts, never a leak. The value is either a libpq-quoted string (backslash /// escapes \\ and \', closing on the first unescaped ', or running to the end of the - /// value when the closing quote never arrives — round 1's L2), plus whatever non-whitespace immediately - /// follows the closing quote (round 1's M4: a quoted value glued to a trailing ; or another token + /// value when the closing quote never arrives), plus whatever non-whitespace immediately + /// follows the closing quote (a quoted value glued to a trailing ; or another token /// with no space), or an unquoted run of non-whitespace. private static readonly TimeBoundPattern LibpqPasswordKeyword = new( @"(?<=^|\s)(?sslpassword|password)\s*=\s*(?:'(?:\\[\s\S]|[^'\\])*(?:'|$)\S*|\S*)", @@ -138,8 +147,8 @@ public string Replace(string input, MatchEvaluator evaluator) => /// The password half of a URI's user info: scheme://user:secret@host. A bare /// user@host, with no password, does not match and the username is left alone. The user name may - /// be empty (round 1's M2: postgresql://:secret@host is valid libpq and still carries a - /// password). The password itself may contain a raw space or an NBSP (round 1's L2 — libpq's own + /// be empty (postgresql://:secret@host is valid libpq and still carries a + /// password). The password itself may contain a raw space or an NBSP (libpq's own /// isspace is not Unicode-aware, so a literal space can sit inside the password up to the @), /// so the password class excludes only @, / and the URI's own delimiters, not all /// whitespace. @@ -156,15 +165,15 @@ public string Replace(string input, MatchEvaluator evaluator) => RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); /// A shell-style or option-style assignment (NAME=value) whose name contains PASSWORD, - /// PASSWD, PASSPHRASE, SECRET, TOKEN, CREDENTIAL, PWD, or a standalone KEY segment (round 1's M5 - /// amendment: common WAL-G/Azure/pgBackRest variable names such as WALG_LIBSODIUM_KEY and + /// PASSWD, PASSPHRASE, SECRET, TOKEN, CREDENTIAL, PWD, or a standalone KEY segment (covers + /// common WAL-G/Azure/pgBackRest variable names such as WALG_LIBSODIUM_KEY and /// AZURE_STORAGE_ACCESS_KEY). PASS covers PASSWORD, PASSWD and PASSPHRASE as substrings; KEY /// is bounded on both sides by (?<![A-Za-z0-9])/(?![A-Za-z0-9]) so it matches only as its /// own word or delimited segment, never as a substring — which is what keeps libpq's sslkey=/path /// unmasked. Covers PGPASSWORD=x, AWS_SECRET_ACCESS_KEY=x, WALG_PGP_KEY_PASSPHRASE=x /// inside archive_command/restore_command-shaped values. The value side accepts the same /// libpq-style quoting as , plus double quotes, plus whatever - /// non-whitespace immediately follows the closing quote (round 1's M4), so a quoted value with an + /// non-whitespace immediately follows the closing quote, so a quoted value with an /// =, space, or trailing punctuation inside or after it is not mistaken for the start of the next /// token. private static readonly TimeBoundPattern AssignmentSecretName = new( @@ -172,10 +181,10 @@ public string Replace(string input, MatchEvaluator evaluator) => RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); /// A space-separated option whose name contains PASS, SECRET, TOKEN, CREDENTIAL, PWD, or a - /// standalone KEY segment -- the same name part as (review round 2's - /// M1: the option path had fallen behind the assignment path's name list, so gpg --passphrase x, - /// openssl enc -pass pass:x, --encryption-key x and --credentials x still leaked), - /// taking its value from the next whitespace-delimited token rather than an = (round 1's M3: + /// standalone KEY segment -- the same name part as (covers the + /// space-separated option path so gpg --passphrase x, + /// openssl enc -pass pass:x, --encryption-key x and --credentials x are all masked), + /// taking its value from the next whitespace-delimited token rather than an = ( /// --password hunter2 and --secret-access-key hunter2 have no = at all, so /// never fires on them). (?!-) keeps a value-less flag such as /// --no-password -h x from swallowing the next option as its value. @@ -338,13 +347,22 @@ private static bool IsQueryKeySecret(string rawKey) } catch (RegexMatchTimeoutException) { - onMatchTimeout?.Invoke(name); + // The value is already masked below; a logging callback that throws must not surface past this + // method, which is documented to never throw. + try + { + onMatchTimeout?.Invoke(name); + } + catch + { + } + return Mask; } } /// Names that would otherwise trip 's dotted-name test but - /// carry no secret — a password POLICY setting, not a password (review round 2's L3). Exact match, + /// carry no secret — a password POLICY setting, not a password. Exact match, /// case-insensitive; anything ELSE with a marker in a dot-separated segment is still masked whole, /// including a real secret sitting in the same batch as one of these (the negative case the pin /// covers). @@ -376,7 +394,7 @@ private static bool IsWholeValueMasked(string? name) } // Extension-scoped settings only: a bare core GUC such as password_encryption never matches here, - // no matter what its name contains — only a dotted name is tested at all. Round 1's L1: a marker in + // no matter what its name contains — only a dotted name is tested at all. A marker in // ANY dot-separated segment counts, not only the last one (vault.secret.value), so the whole name is // tested as a substring rather than slicing off just the last part. var dot = name.IndexOf('.'); From b6866211c655eb6ce4269db1cad842d168b37c9c Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 26 Sep 2026 03:25:59 -0400 Subject: [PATCH 3/3] Keep the settings redactor linear on long name runs (#4348) --- .../Darling.Tests/PgSettingRedactorTests.cs | 62 ++++++++++++++----- .../PgSettingRedactor.cs | 21 +++++-- 2 files changed, 63 insertions(+), 20 deletions(-) diff --git a/Darling/Darling.Tests/PgSettingRedactorTests.cs b/Darling/Darling.Tests/PgSettingRedactorTests.cs index 65910ad5b..6a9849cdb 100644 --- a/Darling/Darling.Tests/PgSettingRedactorTests.cs +++ b/Darling/Darling.Tests/PgSettingRedactorTests.cs @@ -384,22 +384,56 @@ public void AllowlistedPolicyName_DoesNotShieldARealSecretElsewhere() } /// - /// #4348: the anchor added to AssignmentSecretName's name part must not change any existing - /// corpus case's output. Captured before the anchor, from dev, as a standalone hard-coded expectation - /// list independent of — a regression in that anchor would show up here - /// even if a future edit also touched the corpus above. + /// #4348: a frozen list of inputs whose expected output is hard-coded from dev's redactor (pre-lookahead), + /// captured independently of above. Its job is narrower than that member's: + /// it pins that the linear-pre-check change added to AssignmentSecretName, OptionSecretSpaced + /// and QuotedSpacedAssignment (#4348) changed no match, only how the engine gets there. A + /// regression there would show up here even if a future edit also touched the corpus above. /// - [Fact] - public void AnchoringAssignmentSecretName_DoesNotChangeExistingCorpus() + [Theory] + [InlineData("archive_command", "xpassword=s", "xpassword=********")] + [InlineData("archive_command", "a.password=s", "a.password=********")] + [InlineData("archive_command", "my-token=s", "my-token=********")] + [InlineData("archive_command", "env PGPASSWORD=s cmd", "env PGPASSWORD=******** cmd")] + [InlineData("archive_command", "--db.password=s", "--db.password=********")] + [InlineData("archive_command", "-Dpassword=s", "-Dpassword=********")] + [InlineData("archive_command", "foo.bar.PGPASSWORD=s", "foo.bar.PGPASSWORD=********")] + [InlineData("archive_command", "a;PGPASSWORD=s", "a;PGPASSWORD=********")] + [InlineData("archive_command", "sslkey=s", "sslkey=s")] + [InlineData("archive_command", "ab-KEY-cd=s", "ab-KEY-cd=********")] + [InlineData("archive_command", "abc--password=s", "abc--password=********")] + [InlineData("archive_command", "password = \"hunter2\"", "password=********")] + [InlineData("archive_command", "postgresql://u:p@h/db", "postgresql://u:********@h/db")] + [InlineData("archive_command", "host=h password=p", "host=h password=********")] + [InlineData("archive_command", "https://h/?password=p&x=1", "https://h/?password=********")] + [InlineData("archive_command", "sshpass -p p ssh h", "sshpass -p ******** ssh h")] + [InlineData("archive_command", "curl -u a:b", "curl -u a:********")] + [InlineData("password_encryption", "password_encryption", "password_encryption")] + [InlineData("archive_command", "host=localhost port=5432 dbname=mydb", "host=localhost port=5432 dbname=mydb")] + [InlineData("archive_command", "application_name=myapp", "application_name=myapp")] + [InlineData("archive_command", "sslmode=verify-full", "sslmode=verify-full")] + [InlineData("archive_command", "passfile=/x/.pgpass", "passfile=********")] + [InlineData("archive_command", "keep_alive=on", "keep_alive=on")] + [InlineData("archive_command", "monkey=1", "monkey=1")] + [InlineData("archive_command", "user=alice dbname=x", "user=alice dbname=x")] + [InlineData("archive_command", "connect_timeout=10", "connect_timeout=10")] + [InlineData("archive_command", "statement_timeout=5000", "statement_timeout=5000")] + [InlineData("archive_command", "search_path=public", "search_path=public")] + [InlineData("archive_command", "log_min_duration_statement=250", "log_min_duration_statement=250")] + [InlineData("archive_command", "archive_command=/bin/true", "archive_command=/bin/true")] + [InlineData("archive_command", "-Dpassword hunter2", "-Dpassword ********")] + [InlineData("archive_command", "--passphrase secret123", "--passphrase ********")] + [InlineData("archive_command", "openssl enc -pass pass:x", "openssl enc -pass ********")] + [InlineData("archive_command", "rds.accepted_password_auth_method", "rds.accepted_password_auth_method")] + [InlineData("vault.secret", "anything", "********")] + [InlineData("anon.salt", "seedvalue", "********")] + [InlineData("myext.api_key", "kv", "********")] + [InlineData("myext.keep_alive", "on", "on")] + [InlineData("ssl_passphrase_command", "echo x", "********")] + [InlineData("archive_command", "pg_password=abc", "pg_password=********")] + public void FrozenParityWithPreviousRedactor(string name, string value, string expectedFromDev) { - foreach (var caseData in RedactionCases()) - { - var name = (string)caseData[0]; - var value = (string)caseData[1]; - var expected = (string)caseData[2]; - - Assert.Equal(expected, PgSettingRedactor.Redact(name, value)); - } + Assert.Equal(expectedFromDev, PgSettingRedactor.Redact(name, value)); } /// diff --git a/PerformanceMonitor.Collectors/PgSettingRedactor.cs b/PerformanceMonitor.Collectors/PgSettingRedactor.cs index 327534e85..bfcabc086 100644 --- a/PerformanceMonitor.Collectors/PgSettingRedactor.cs +++ b/PerformanceMonitor.Collectors/PgSettingRedactor.cs @@ -175,9 +175,13 @@ public string Replace(string input, MatchEvaluator evaluator) => /// libpq-style quoting as , plus double quotes, plus whatever /// non-whitespace immediately follows the closing quote, so a quoted value with an /// =, space, or trailing punctuation inside or after it is not mistaken for the start of the next - /// token. + /// token. The leading (?=[\w.-]*=) pre-check (#4348) is redundant with the = the pattern + /// already requires further on — it changes no match, only how the engine gets there. Without it, a + /// long run of name-class characters with no = anywhere makes the engine retry the whole + /// [\w.-]* body at every start position before failing, which is quadratic in the run's length; the + /// lookahead fails FAST, once, per start position instead. private static readonly TimeBoundPattern AssignmentSecretName = new( - @"(?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?A space-separated option whose name contains PASS, SECRET, TOKEN, CREDENTIAL, PWD, or a @@ -187,9 +191,12 @@ public string Replace(string input, MatchEvaluator evaluator) => /// taking its value from the next whitespace-delimited token rather than an = ( /// --password hunter2 and --secret-access-key hunter2 have no = at all, so /// never fires on them). (?!-) keeps a value-less flag such as - /// --no-password -h x from swallowing the next option as its value. + /// --no-password -h x from swallowing the next option as its value. The leading + /// (?=--?[\w.-]*\s) pre-check (#4348) is redundant with the \s+ the pattern already + /// requires after the option name — same fast-fail-per-start-position reasoning as + /// 's pre-check. private static readonly TimeBoundPattern OptionSecretSpaced = new( - @"(?<=^|\s)(?--?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?--?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?A percent-encoded key name in a URI @@ -220,9 +227,11 @@ public string Replace(string input, MatchEvaluator evaluator) => /// ("password = hunter2", 'pwd = abc' — the whole assignment sits inside one quote pair, so /// the value's end is \k<q>, not a quote of its own). Only the value half is masked; the /// quotes and the name are kept as they were. The value's own quote character does not have to match the - /// name's. + /// name's. The leading (?=[\w.-]*["']?\s*=) pre-check (#4348) mirrors the name-then-optional-quote + /// -then-= shape the pattern already requires further on — redundant by construction, same + /// fast-fail-per-start-position reasoning as 's pre-check. private static readonly TimeBoundPattern QuotedSpacedAssignment = new( - @"(?[""'])(?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?[""']?)\s*=\s*(?:(?[""'])(?[^""']*)\k|(?[^""']*)\k)", + @"(?[""'])(?=[\w.-]*[""']?\s*=)(?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?[""']?)\s*=\s*(?:(?[""'])(?[^""']*)\k|(?[^""']*)\k)", RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); /// curl -u user:secret / curl --user user:secret, including the tightly-bound