From ebc493361e46b0201e4a561c2d781df243d1bc45 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 15:38:36 -0500 Subject: [PATCH] fix(secrets): audit malformed ${secret:} tokens, cap SecretName at 450, resolve local UNC shares (scadaproj#52) Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- ZB.MOM.WW.Secrets/CHANGELOG.md | 18 +++++ .../SecretName.cs | 12 +++- .../Interactive/Flows/ReferenceAuditFlow.cs | 9 ++- .../Interactive/ReferenceAuditor.cs | 55 +++++++++++++++- .../Configuration/SecretsStorePathRules.cs | 66 ++++++++++++++++++- .../Cli/Interactive/ReferenceAuditorTests.cs | 28 ++++++++ .../SecretsStorePathSpellingTests.cs | 44 +++++++++++++ .../SecretNameTests.cs | 10 +++ 8 files changed, 236 insertions(+), 6 deletions(-) diff --git a/ZB.MOM.WW.Secrets/CHANGELOG.md b/ZB.MOM.WW.Secrets/CHANGELOG.md index 78d02fe..c531d4f 100644 --- a/ZB.MOM.WW.Secrets/CHANGELOG.md +++ b/ZB.MOM.WW.Secrets/CHANGELOG.md @@ -10,6 +10,24 @@ one of them. Fixes the scadaproj#15 code-review findings (`code-reviews/Secrets/findings.md`): six P2s (scadaproj#30–#35), the P3 batch (scadaproj#45: Secrets-004–009, 011, 012, 014–016, 018, 019) and the scadaproj#49 follow-ups. +### scadaproj#52 follow-ups + +- **The CLI reference audit reports malformed `${secret:}` tokens.** New `ReferenceStatus.Malformed`: + an empty/blank name, an unterminated `${secret:name` or a nested marker — exactly the shapes + `SecretReferenceExpander` now refuses at startup — yields one finding (`${secret:...} (malformed)`) + listing every config path, and the audit flow tells the operator to fix it. Not seedable. +- **`SecretName` is capped at 450 characters** (`SecretName.MaxLength`, measured after trim/lowercase), + matching the hub's `NVARCHAR(450)` name column. Enforced in the constructor, so every store/API + entry point rejects an over-long name up front instead of the hub refusing the row later. + Behaviour change: an already-stored name longer than 450 can no longer be constructed. +- **Secrets-005 follow-up: an ordinary local SMB share.** On Windows, `\\localhost\share\...` / + `\\\share\...` for a non-administrative share is mapped to the directory the share + publishes (`NetShareGetInfo` level 2), so a store reached through a share that points back at the + content root is refused. Limits (documented in `SecretsStorePathRules`): the API needs + Administrators / Server Operators rights (denied => compared as written); a share on another machine + is never resolved; on non-Windows an NFS/SMB mount cannot be resolved to its export, so that case is + not caught. Test is Windows-only and skips when it cannot create a share. + ### Fixed - **SQL-Server and Akka registration no longer depends on call order** (scadaproj#30, Secrets-001). diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Abstractions/SecretName.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Abstractions/SecretName.cs index 7a8a0b4..82d4eb2 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Abstractions/SecretName.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Abstractions/SecretName.cs @@ -7,6 +7,9 @@ namespace ZB.MOM.WW.Secrets.Abstractions; /// public readonly record struct SecretName { + /// The longest permitted normalized name; matches the hub's NVARCHAR(450) name column. + public const int MaxLength = 450; + private const string AllowedChars = "abcdefghijklmnopqrstuvwxyz0123456789._/-"; /// @@ -17,7 +20,7 @@ public readonly record struct SecretName /// /// The value is null, empty, or whitespace; contains the path-traversal sequence /// ..; is rooted (starts or ends with /) or contains an empty path - /// segment (//); or contains any character outside [a-z0-9._/-] (after + /// segment (//); or is longer than (450) characters; or contains any character outside [a-z0-9._/-] (after /// trim and lowercasing). /// public SecretName(string value) @@ -31,6 +34,13 @@ public readonly record struct SecretName nameof(value)); } + if (normalized.Length > MaxLength) + { + throw new ArgumentException( + $"Secret name must be at most {MaxLength} characters (was {normalized.Length}).", + nameof(value)); + } + if (normalized.Contains("..", StringComparison.Ordinal)) { throw new ArgumentException( diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/ReferenceAuditFlow.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/ReferenceAuditFlow.cs index 0c3aa62..63db2ba 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/ReferenceAuditFlow.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/ReferenceAuditFlow.cs @@ -57,6 +57,13 @@ public sealed class ReferenceAuditFlow : IInteractiveFlow continue; } + if (finding.Status == ReferenceStatus.Malformed) + { + console.MarkupLineInterpolated( + $"[red]Malformed ${{secret:}} token[/] at {string.Join(", ", finding.ConfigPaths)} — the app refuses to start with it; give it a name and a closing '}}'."); + continue; + } + if (!IsSeedable(finding.Status)) { continue; // Ok (nothing to do) or PresentUnverified (unreachable on a KEK-capable session). @@ -132,7 +139,7 @@ public sealed class ReferenceAuditFlow : IInteractiveFlow { ReferenceStatus.Ok => "green", ReferenceStatus.PresentUnverified => "yellow", - _ => "red", // Missing / Tombstoned / Undecryptable / InvalidName + _ => "red", // Missing / Tombstoned / Undecryptable / InvalidName / Malformed }; // A deterministic "N Status" count-by-status roll-up, ordered by the status enum. diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/ReferenceAuditor.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/ReferenceAuditor.cs index e99e311..13d9fe2 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/ReferenceAuditor.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/ReferenceAuditor.cs @@ -30,6 +30,13 @@ public enum ReferenceStatus /// unfilled placeholder like REPLACE ME, and so on), so it cannot be looked up at all. /// InvalidName, + + /// + /// A ${secret: marker that does not start a complete, named reference (${secret:}, + /// ${secret: }, an unterminated ${secret:name, or a marker swallowed inside another + /// match). SecretReferenceExpander refuses to start with such a value, so the audit surfaces it. + /// + Malformed, } /// @@ -49,6 +56,12 @@ public sealed class ReferenceAuditor // the references the expander would resolve at startup. Do NOT diverge from the expander's regex. private static readonly Regex TokenPattern = new(@"\$\{secret:([^}]+)\}", RegexOptions.Compiled); + // Pinned to SecretReferenceExpander.TokenMarker. + private const string TokenMarker = "${secret:"; + + /// The used for a malformed-token finding. + public const string MalformedFindingName = "${secret:...} (malformed)"; + /// /// Walks every configuration value, collects each distinct ${secret:NAME} reference with the /// config key paths that use it, and classifies each name against the session's store. References are @@ -79,8 +92,29 @@ public sealed class ReferenceAuditor continue; } - foreach (Match match in TokenPattern.Matches(value)) + MatchCollection matches = TokenPattern.Matches(value); + + // Mirror the expander's fail-closed shape check: every marker must start one complete, + // non-blank reference, otherwise the app refuses to start. + if (CountMarkers(value) != matches.Count + || matches.Any(m => string.IsNullOrWhiteSpace(m.Groups[1].Value))) { + if (!groups.TryGetValue(MalformedFindingName, out ReferenceGroup? bad)) + { + bad = new ReferenceGroup(null) { Malformed = true }; + groups[MalformedFindingName] = bad; + } + + bad.Paths.Add(entry.Key); + } + + foreach (Match match in matches) + { + if (string.IsNullOrWhiteSpace(match.Groups[1].Value)) + { + continue; + } + string raw = match.Groups[1].Value.Trim(); ReferenceGroup group = ResolveGroup(groups, raw); group.Paths.Add(entry.Key); @@ -90,7 +124,9 @@ public sealed class ReferenceAuditor var findings = new List(groups.Count); foreach ((string key, ReferenceGroup group) in groups) { - ReferenceStatus status = group.Name is { } name + ReferenceStatus status = group.Malformed + ? ReferenceStatus.Malformed + : group.Name is { } name ? await ClassifyAsync(session, name, ct).ConfigureAwait(false) : ReferenceStatus.InvalidName; findings.Add(new ReferenceFinding(key, status, [.. group.Paths])); @@ -131,6 +167,19 @@ public sealed class ReferenceAuditor // A configuration key is a comment/documentation key when its leaf segment (after the last ':') starts // with '_'. Duplicated from SecretReferenceExpander.IsCommentKey — keep the two in lockstep. + private static int CountMarkers(string value) + { + int count = 0; + for (int at = value.IndexOf(TokenMarker, StringComparison.Ordinal); + at >= 0; + at = value.IndexOf(TokenMarker, at + TokenMarker.Length, StringComparison.Ordinal)) + { + count++; + } + + return count; + } + private static bool IsCommentKey(string key) { int lastSeparator = key.LastIndexOf(':'); @@ -177,5 +226,7 @@ public sealed class ReferenceAuditor public SortedSet Paths { get; } = new(StringComparer.Ordinal); public SecretName? Name { get; } = name; + + public bool Malformed { get; init; } } } diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretsStorePathRules.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretsStorePathRules.cs index 0f4d25b..567245a 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretsStorePathRules.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretsStorePathRules.cs @@ -46,8 +46,13 @@ namespace ZB.MOM.WW.Secrets.Configuration; /// leads elsewhere — the next deploy's tree has no such link), and on Windows the \\?\ / \\.\ device /// prefixes are removed, a loopback administrative share (\\localhost\C$\…, /// \\127.0.0.1\C$\…, \\<this machine>\C$\…) is mapped back to its drive, and 8.3 -/// short names (PROGRA~1) are expanded. A UNC path through any other share is compared as -/// written — what a share points at is server configuration this rule cannot see. +/// short names (PROGRA~1) are expanded. An ordinary (non-administrative) share on this +/// machine is mapped to the directory it publishes via NetShareGetInfo (Windows only; that API +/// needs Administrators / Server Operators rights, and when it is denied the path is compared as +/// written). A UNC path to another machine's share is always compared as written — what a +/// remote share points at is server configuration this rule cannot see. On non-Windows there are no +/// UNC shares and an NFS/SMB mount point cannot be resolved to its export, so a store on a mount whose +/// export is the content root is not caught (documented limit). /// /// public static class SecretsStorePathRules @@ -241,11 +246,68 @@ public static class SecretsStorePathRules { p = segments[1][0] + @":\" + string.Join('\\', segments[2..]); } + else if (segments.Length >= 2 && IsLoopbackHost(segments[0]) && TryGetLocalSharePath(segments[1]) is { } sharePath) + { + // An ordinary (non-administrative) share on THIS machine: map it to the directory it + // publishes so a store reached through a share that points back at the content root + // is compared by location. + p = sharePath.TrimEnd('\\') + (segments.Length > 2 ? @"\" + string.Join('\\', segments[2..]) : string.Empty); + } } return p; } + /// + /// Returns the local directory published by share on this machine, or + /// when it cannot be determined (no such share, or the caller lacks the + /// rights NetShareGetInfo level 2 demands — Administrators / Server Operators). A share on + /// ANOTHER machine is never resolved: what it points at is that server's configuration. + /// + private static string? TryGetLocalSharePath(string shareName) + { + try + { + if (NetShareGetInfo(null, shareName, 2, out IntPtr buffer) != 0) + { + return null; + } + + try + { + SHARE_INFO_2 info = Marshal.PtrToStructure(buffer); + return string.IsNullOrWhiteSpace(info.shi2_path) ? null : info.shi2_path; + } + finally + { + _ = NetApiBufferFree(buffer); + } + } + catch (Exception ex) when (ex is DllNotFoundException or EntryPointNotFoundException or SEHException) + { + return null; + } + } + + [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] + private struct SHARE_INFO_2 + { + public string shi2_netname; + public uint shi2_type; + public string shi2_remark; + public uint shi2_permissions; + public uint shi2_max_uses; + public uint shi2_current_uses; + public string shi2_path; + public string shi2_passwd; + } + + [DllImport("netapi32.dll", CharSet = CharSet.Unicode)] + private static extern uint NetShareGetInfo(string? serverName, string netName, uint level, out IntPtr bufptr); + + [DllImport("netapi32.dll")] + private static extern uint NetApiBufferFree(IntPtr buffer); + private static bool IsLoopbackHost(string host) => host.Equals("localhost", StringComparison.OrdinalIgnoreCase) || host.Equals("127.0.0.1", StringComparison.Ordinal) diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/ReferenceAuditorTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/ReferenceAuditorTests.cs index 1878f79..68c80aa 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/ReferenceAuditorTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/ReferenceAuditorTests.cs @@ -173,6 +173,34 @@ public sealed class ReferenceAuditorTests : IDisposable Assert.Equal(ReferenceStatus.Missing, findings.Single(f => f.SecretName == "api/key").Status); } + [Theory] + [InlineData("${secret:}")] + [InlineData("${secret: }")] + [InlineData("prefix ${secret:unterminated")] + [InlineData("${secret:a${secret:b}")] + public async Task Malformed_token_yields_Malformed_finding_matching_the_expander(string value) + { + SecretsSession session = await Open(EnvKey(), Config(("Bad:Ref", value), ("Good:Ref", "${secret:api/key}"))); + + IReadOnlyList findings = + await new ReferenceAuditor().AuditAsync(session, CancellationToken.None); + + ReferenceFinding bad = findings.Single(f => f.Status == ReferenceStatus.Malformed); + Assert.Equal(["Bad:Ref"], bad.ConfigPaths); + Assert.Equal(ReferenceStatus.Missing, findings.Single(f => f.SecretName == "api/key").Status); + } + + [Fact] + public async Task Well_formed_tokens_produce_no_Malformed_finding() + { + SecretsSession session = await Open(EnvKey(), Config(("A", "x ${secret:a/b} y ${secret:c}"))); + + IReadOnlyList findings = + await new ReferenceAuditor().AuditAsync(session, CancellationToken.None); + + Assert.DoesNotContain(findings, f => f.Status == ReferenceStatus.Malformed); + } + [Fact] public async Task Comment_keys_are_skipped_like_the_expander() { diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Configuration/SecretsStorePathSpellingTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Configuration/SecretsStorePathSpellingTests.cs index 25f1c99..846d75d 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Configuration/SecretsStorePathSpellingTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Configuration/SecretsStorePathSpellingTests.cs @@ -139,6 +139,50 @@ public sealed class SecretsStorePathSpellingTests : IDisposable Assert.True(Inside(Path.Combine(longDir, "secrets.db"), shortDir!)); } + [SkippableFact] + public void An_ordinary_local_share_pointing_at_the_content_root_is_inside_on_windows() + { + Skip.IfNot(OperatingSystem.IsWindows(), "Windows path semantics"); + string root = Path.Combine(_base, "app"); + string share = "zbtest" + Guid.NewGuid().ToString("N")[..8]; + Skip.IfNot(CreateShare(share, root), "cannot create an SMB share here (needs Administrators)"); + try + { + Assert.True(Inside($@"\\localhost\{share}\data\secrets.db", root)); + Assert.True(Inside($@"\\{Environment.MachineName}\{share}\secrets.db", root)); + Assert.False(Inside($@"\\localhost\{share}\secrets.db", Path.Combine(_base, "outside"))); + } + finally + { + _ = NetShareDel(null, share, 0); + } + } + + private static bool CreateShare(string name, string path) + { + var info = new ShareInfo2 { shi2_netname = name, shi2_type = 0, shi2_max_uses = uint.MaxValue, shi2_path = path }; + return NetShareAdd(null, 2, ref info, out _) == 0; + } + + [System.Runtime.InteropServices.StructLayout(System.Runtime.InteropServices.LayoutKind.Sequential, CharSet = System.Runtime.InteropServices.CharSet.Unicode)] + private struct ShareInfo2 + { + public string shi2_netname; + public uint shi2_type; + public string? shi2_remark; + public uint shi2_permissions; + public uint shi2_max_uses; + public uint shi2_current_uses; + public string shi2_path; + public string? shi2_passwd; + } + + [System.Runtime.InteropServices.DllImport("netapi32.dll", CharSet = System.Runtime.InteropServices.CharSet.Unicode)] + private static extern uint NetShareAdd(string? server, uint level, ref ShareInfo2 buf, out uint parmErr); + + [System.Runtime.InteropServices.DllImport("netapi32.dll", CharSet = System.Runtime.InteropServices.CharSet.Unicode)] + private static extern uint NetShareDel(string? server, string netName, uint reserved); + private static string? ShortName(string path) { char[] buffer = new char[1024]; diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/SecretNameTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/SecretNameTests.cs index ff9c8c0..970f33c 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/SecretNameTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/SecretNameTests.cs @@ -27,6 +27,16 @@ public class SecretNameTests public void RejectsRootedOrEmptySegment(string input) => Assert.Throws(() => new SecretName(input)); + [Fact] + public void AcceptsExactlyMaxLength_AndRejectsOneOver() + { + Assert.Equal(450, SecretName.MaxLength); + Assert.Equal(450, new SecretName(new string('a', 450)).Value.Length); + Assert.Throws(() => new SecretName(new string('a', 451))); + // Length is measured after trim/lowercase normalization. + Assert.Equal(450, new SecretName(" " + new string('A', 450) + " ").Value.Length); + } + [Fact] public void DefaultInstance_ThrowsOnValue() => Assert.Throws(() => _ = default(SecretName).Value);