diff --git a/ZB.MOM.WW.Secrets/CHANGELOG.md b/ZB.MOM.WW.Secrets/CHANGELOG.md index fc3c9fd..588813e 100644 --- a/ZB.MOM.WW.Secrets/CHANGELOG.md +++ b/ZB.MOM.WW.Secrets/CHANGELOG.md @@ -51,6 +51,18 @@ Fixes six scadaproj#15 code-review findings (`code-reviews/Secrets/findings.md`) failure is logged as a warning (`ILogger` / `ILogger`, when the host registers logging). +### Changed + +- **CLI `set`/`rotate` take the value on stdin or a masked prompt** (scadaproj#35, Secrets-017). + `secret set -` / `--value-stdin` reads stdin (one trailing newline dropped); `secret set ` + reads piped stdin, or prompts twice (masked) at a terminal. The positional `` still works but + prints a deprecation warning to stderr. The CLI host is no longer built from argv, so a value starting + with `-` and containing `=` no longer crashes it with the value in an unhandled `FormatException`, + and argv can no longer override configuration (`--Secrets:SqlitePath=…`, `--environment …`: use + environment variables). A startup failure now prints `secret: startup failed: ` and exits 1 + instead of an unhandled stack trace. An unexpected trailing argument is reported by position, not + echoed. + ## 0.6.5 — unreleased (NOT published to the feed) ### Changed diff --git a/ZB.MOM.WW.Secrets/README.md b/ZB.MOM.WW.Secrets/README.md index b8579c1..073ae38 100644 --- a/ZB.MOM.WW.Secrets/README.md +++ b/ZB.MOM.WW.Secrets/README.md @@ -16,6 +16,10 @@ plaintext back on demand — from application code, from configuration, or from | `ZB.MOM.WW.Secrets.Replicator.Grpc` | Central-hosted `secrets_hub.v1` gRPC hub, pull-only (sites sync from central; central never accepts a write over this channel) — for topologies where sites talk only to central, never a shared DB. | A `secret` CLI (`set` / `get` / `list` / `rm` / `rotate` / `rewrap-all`) ships in the repo (not packed). +`set` and `rotate` take the value on **stdin** (`printf '%s' "$PW" | secret set db/pw -`) or, at a +terminal, from a masked prompt (`secret set db/pw`). The old `secret set ` form still +works but warns, because a value on the command line lands in shell history and the process list. +The CLI reads its configuration from `appsettings.json` and the environment only — never from argv. ### Interactive console diff --git a/ZB.MOM.WW.Secrets/docs/operations/clustered-secrets.md b/ZB.MOM.WW.Secrets/docs/operations/clustered-secrets.md index c890dd3..c9091dc 100644 --- a/ZB.MOM.WW.Secrets/docs/operations/clustered-secrets.md +++ b/ZB.MOM.WW.Secrets/docs/operations/clustered-secrets.md @@ -78,7 +78,7 @@ secret, or an environment variable. Never commit it. ### Verify ```bash -secret set smoke/test hello --description "delete me" # on node A +echo hello | secret set smoke/test - --description "delete me" # on node A secret get smoke/test # on node B — expect: hello secret rm smoke/test ``` diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Program.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Program.cs index a4a474d..ddbb9aa 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Program.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Program.cs @@ -18,18 +18,36 @@ using ZB.MOM.WW.Secrets.MasterKey; if (InteractiveEntry.ShouldEnterInteractive(args, Console.IsInputRedirected, Console.IsOutputRedirected)) return await InteractiveEntry.CreateShell(AnsiConsole.Console).RunAsync(CancellationToken.None); -HostApplicationBuilder builder = Host.CreateApplicationBuilder(args); -builder.Configuration - .AddJsonFile("appsettings.json", optional: true) - .AddEnvironmentVariables(); -builder.Services.AddZbSecrets(builder.Configuration, "Secrets"); +// argv is deliberately NOT handed to the host builder (scadaproj#35). Host.CreateApplicationBuilder(args) +// feeds it to the command-line configuration provider, which turned a secret value containing '=' +// into a configuration key and crashed on one starting with '-' — with the whole argument, i.e. the +// secret, in the unhandled FormatException's message. Configuration comes from appsettings.json and +// the environment only. +IHost host; +try +{ + HostApplicationBuilder builder = Host.CreateApplicationBuilder(new HostApplicationBuilderSettings()); + builder.Configuration + .AddJsonFile("appsettings.json", optional: true) + .AddEnvironmentVariables(); + builder.Services.AddZbSecrets(builder.Configuration, "Secrets"); -using IHost host = builder.Build(); + host = builder.Build(); -// Ensure the schema exists before any store operation. Resolved through the ISecretsStoreMigrator -// seam so the CLI provisions whichever store the app configured (local SQLite or shared SQL Server). -await host.Services.GetRequiredService() - .MigrateAsync(CancellationToken.None); + // Ensure the schema exists before any store operation. Resolved through the ISecretsStoreMigrator + // seam so the CLI provisions whichever store the app configured (local SQLite or shared SQL Server). + await host.Services.GetRequiredService() + .MigrateAsync(CancellationToken.None); +} +catch (Exception ex) +{ + // No argv reaches configuration, so this message cannot carry a secret value; print it without + // the stack trace an unhandled crash would dump into CI logs and deploy transcripts. + Console.Error.WriteLine($"secret: startup failed: {ex.Message}"); + return 1; +} + +using IHost ownedHost = host; var commands = new SecretCommands( host.Services.GetRequiredService(), @@ -50,19 +68,21 @@ try switch (args[0]) { case "set": - { - if (args.Length < 3) - return Usage("set requires and ."); - (SecretContentType contentType, string? description) = ParseSetOptions(args, startIndex: 3); - return await commands.SetAsync(args[1], args[2], contentType, description, actor, ct); - } - case "rotate": { - if (args.Length < 3) - return Usage("rotate requires and ."); - (SecretContentType contentType, string? description) = ParseSetOptions(args, startIndex: 3); - return await commands.RotateAsync(args[1], args[2], contentType, description, actor, ct); + // The value comes from stdin or a masked prompt; the positional form still works but + // warns (scadaproj#35). See SetCommandLine for the accepted forms. + if (args.Length < 2) + return Usage($"{args[0]} requires ."); + SetCommandLine line = SetCommandLine.Parse(args); + string value = line.ReadValue( + Console.In, + Console.IsInputRedirected, + label => AnsiConsole.Prompt(new TextPrompt(label).Secret().AllowEmpty()), + Console.Error); + return args[0] == "set" + ? await commands.SetAsync(line.Name, value, line.ContentType, line.Description, actor, ct) + : await commands.RotateAsync(line.Name, value, line.ContentType, line.Description, actor, ct); } case "get": @@ -134,48 +154,6 @@ catch (ArgumentException ex) // --- helpers --------------------------------------------------------------- -// Parses the optional flags shared by `set` and `rotate`: -// [--content-type text|connection-string|json|binary-base64] [--description "..."] -static (SecretContentType contentType, string? description) ParseSetOptions(string[] args, int startIndex) -{ - SecretContentType contentType = SecretContentType.Text; - string? description = null; - - for (int i = startIndex; i < args.Length; i++) - { - switch (args[i]) - { - case "--content-type": - if (i + 1 >= args.Length) - throw new ArgumentException("--content-type requires a value."); - contentType = ParseContentType(args[++i]); - break; - - case "--description": - if (i + 1 >= args.Length) - throw new ArgumentException("--description requires a value."); - description = args[++i]; - break; - - default: - throw new ArgumentException($"Unknown option '{args[i]}'."); - } - } - - return (contentType, description); -} - -// Maps the CLI content-type token (kebab-case) to the enum. -static SecretContentType ParseContentType(string value) => value switch -{ - "text" => SecretContentType.Text, - "connection-string" => SecretContentType.ConnectionString, - "json" => SecretContentType.Json, - "binary-base64" => SecretContentType.BinaryBase64, - _ => throw new ArgumentException( - $"Unknown --content-type '{value}'; expected text|connection-string|json|binary-base64."), -}; - // Reads the value that must follow an option flag at args[i], advancing i past it. static string RequireValue(string[] args, ref int i) { @@ -206,14 +184,19 @@ int Usage(string? error = null) """ Usage: secret [args] - set [--content-type text|connection-string|json|binary-base64] [--description "..."] - rotate [--content-type ...] [--description "..."] + set [-|--value-stdin] [--content-type text|connection-string|json|binary-base64] [--description "..."] + rotate [-|--value-stdin] [--content-type ...] [--description "..."] get list [--include-deleted] rm rewrap-all --old-key-env |--old-key-file [--old-key-id ] [--new-key-env |--new-key-file ] [--new-key-id ] + set/rotate read the value from stdin when it is piped (or with `-` / --value-stdin), and + otherwise prompt for it, masked, twice. One trailing newline on stdin is dropped. The old + `set ` form still works but prints a warning: a value on the command line is + kept in shell history and visible in the process list. + rewrap-all rotates the master KEK: it re-wraps every stored secret's data key from the old KEK to the new one (bodies are never re-encrypted, no plaintext is exposed). The new KEK defaults to the configured Secrets:MasterKey provider. Supply key material only by env-var diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/SetCommandLine.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/SetCommandLine.cs new file mode 100644 index 0000000..67a067e --- /dev/null +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/SetCommandLine.cs @@ -0,0 +1,218 @@ +using ZB.MOM.WW.Secrets.Abstractions; + +namespace ZB.MOM.WW.Secrets.Cli; + +/// Where a set/rotate invocation takes the secret value from. +public enum SecretValueSource +{ + /// + /// No value on the command line: read standard input when it is redirected, otherwise prompt + /// (masked) at the terminal. + /// + Implicit, + + /// Read standard input, requested explicitly with - or --value-stdin. + Stdin, + + /// + /// The legacy positional <value>. Still accepted, with a warning: it lands in shell + /// history and is visible in the process list for the duration of the run. + /// + Argument, +} + +/// +/// Parsing and value acquisition for the headless set and rotate verbs (scadaproj#35). +/// +/// +/// +/// Accepted forms, for set and rotate alike: +/// +/// +/// secret set <name> [options] # value from piped stdin, else a masked prompt +/// secret set <name> - [options] # value from stdin +/// secret set <name> --value-stdin [options] +/// secret set <name> <value> [options] # DEPRECATED: warns, value is exposed on argv +/// +/// +/// The third argument is a positional value unless it is exactly - or one of the option +/// flags, so a value that happens to equal --content-type, --description or +/// --value-stdin must be supplied through stdin instead. +/// +/// +public sealed record SetCommandLine( + string Name, + SecretValueSource Source, + string? ArgumentValue, + SecretContentType ContentType, + string? Description) +{ + /// The flag that requests the value from standard input. + public const string ValueStdinFlag = "--value-stdin"; + + /// Printed to stderr whenever the deprecated positional value form is used. + public const string ArgumentValueWarning = + "warning: the secret value was passed on the command line, where it is kept in shell history " + + "and visible in the process list. Pipe it on stdin (`secret set -`) or omit it to be " + + "prompted; the positional form will be removed in a future release."; + + private static readonly HashSet Flags = + new(StringComparer.Ordinal) { "--content-type", "--description", ValueStdinFlag }; + + /// + /// Parses <verb> <name> [<value>|-] [options]. The value itself is + /// never part of an exception message. + /// + /// The full argument vector, verb first. + /// The parsed command line. + /// A missing name, an unknown option, or conflicting value sources. + public static SetCommandLine Parse(string[] args) + { + ArgumentNullException.ThrowIfNull(args); + if (args.Length < 2 || Flags.Contains(args[1])) + { + throw new ArgumentException($"{(args.Length > 0 ? args[0] : "set")} requires ."); + } + + string name = args[1]; + SecretValueSource source = SecretValueSource.Implicit; + string? argumentValue = null; + int optionsStart = 2; + + if (args.Length > 2 && !Flags.Contains(args[2])) + { + optionsStart = 3; + if (args[2] == "-") + { + source = SecretValueSource.Stdin; + } + else + { + source = SecretValueSource.Argument; + argumentValue = args[2]; + } + } + + SecretContentType contentType = SecretContentType.Text; + string? description = null; + for (int i = optionsStart; i < args.Length; i++) + { + switch (args[i]) + { + case "--content-type": + contentType = ParseContentType(RequireValue(args, ref i)); + break; + + case "--description": + description = RequireValue(args, ref i); + break; + + case ValueStdinFlag: + if (source == SecretValueSource.Argument) + { + throw new ArgumentException( + $"{ValueStdinFlag} cannot be combined with a value on the command line."); + } + + source = SecretValueSource.Stdin; + break; + + default: + // Deliberately not echoed: a mistyped invocation can put the secret value here. + throw new ArgumentException( + $"Unexpected argument at position {i}; expected --content-type, --description or {ValueStdinFlag}."); + } + } + + return new SetCommandLine(name, source, argumentValue, contentType, description); + } + + /// + /// Obtains the secret value from the source this command line names. + /// + /// Standard input. + /// Whether standard input is redirected (piped or a file). + /// Masked terminal prompt: takes a label, returns what was typed. + /// Where the deprecation warning goes. + /// The plaintext value. + /// Stdin was empty, or the two prompted entries differ. + public string ReadValue(TextReader stdin, bool stdinRedirected, Func prompt, TextWriter stderr) + { + ArgumentNullException.ThrowIfNull(stdin); + ArgumentNullException.ThrowIfNull(prompt); + ArgumentNullException.ThrowIfNull(stderr); + + switch (Source) + { + case SecretValueSource.Argument: + stderr.WriteLine(ArgumentValueWarning); + return ArgumentValue!; + + case SecretValueSource.Stdin: + return ReadStdin(stdin); + + default: + if (stdinRedirected) + { + return ReadStdin(stdin); + } + + string first = prompt("Secret value:"); + string second = prompt("Confirm value:"); + if (!string.Equals(first, second, StringComparison.Ordinal)) + { + throw new ArgumentException("The two entries do not match; nothing was written."); + } + + if (first.Length == 0) + { + throw new ArgumentException("An empty value was entered; nothing was written."); + } + + return first; + } + } + + // The whole of stdin, less ONE trailing line terminator — what `echo`/`printf '%s\n'`/a heredoc + // append, and never part of the secret. Any other whitespace is the operator's and is kept. + private static string ReadStdin(TextReader stdin) + { + string value = stdin.ReadToEnd(); + if (value.EndsWith("\r\n", StringComparison.Ordinal)) + { + value = value[..^2]; + } + else if (value.EndsWith('\n')) + { + value = value[..^1]; + } + + if (value.Length == 0) + { + throw new ArgumentException("No secret value was read from stdin; nothing was written."); + } + + return value; + } + + private static string RequireValue(string[] args, ref int i) + { + if (i + 1 >= args.Length) + { + throw new ArgumentException($"{args[i]} requires a value."); + } + + return args[++i]; + } + + // Maps the CLI content-type token (kebab-case) to the enum. + private static SecretContentType ParseContentType(string value) => value switch + { + "text" => SecretContentType.Text, + "connection-string" => SecretContentType.ConnectionString, + "json" => SecretContentType.Json, + "binary-base64" => SecretContentType.BinaryBase64, + _ => throw new ArgumentException( + $"Unknown --content-type '{value}'; expected text|connection-string|json|binary-base64."), + }; +} diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/SetCommandLineTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/SetCommandLineTests.cs new file mode 100644 index 0000000..4daadbf --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/SetCommandLineTests.cs @@ -0,0 +1,224 @@ +using System.Diagnostics; +using System.Security.Cryptography; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.Cli; + +namespace ZB.MOM.WW.Secrets.Tests.Cli; + +/// +/// scadaproj#35 (Secrets-017): secret set/rotate take the value from stdin or a masked +/// prompt; the positional form still works but warns; and argv never reaches configuration. +/// +public sealed class SetCommandLineTests +{ + private static string NoPrompt(string label) => + throw new InvalidOperationException("the prompt must not be used here"); + + // ---------- parsing ---------- + + [Fact] + public void A_bare_name_reads_the_value_implicitly() + { + SetCommandLine line = SetCommandLine.Parse(["set", "db/pw", "--content-type", "json"]); + + Assert.Equal("db/pw", line.Name); + Assert.Equal(SecretValueSource.Implicit, line.Source); + Assert.Null(line.ArgumentValue); + Assert.Equal(SecretContentType.Json, line.ContentType); + } + + [Theory] + [InlineData("-")] + [InlineData("--value-stdin")] + public void Dash_and_the_flag_both_select_stdin(string marker) + { + SetCommandLine line = SetCommandLine.Parse(["rotate", "db/pw", marker, "--description", "d"]); + + Assert.Equal(SecretValueSource.Stdin, line.Source); + Assert.Equal("d", line.Description); + } + + [Fact] + public void The_positional_value_still_parses_for_compatibility() + { + SetCommandLine line = SetCommandLine.Parse(["set", "db/pw", "hunter2", "--content-type", "text"]); + + Assert.Equal(SecretValueSource.Argument, line.Source); + Assert.Equal("hunter2", line.ArgumentValue); + } + + [Fact] + public void A_positional_value_and_the_stdin_flag_conflict() + { + Assert.Throws(() => SetCommandLine.Parse(["set", "db/pw", "v", "--value-stdin"])); + } + + [Fact] + public void An_unexpected_argument_is_rejected_without_echoing_it() + { + ArgumentException ex = Assert.Throws( + () => SetCommandLine.Parse(["set", "db/pw", "-", "Hunter2Secret"])); + + Assert.DoesNotContain("Hunter2Secret", ex.Message, StringComparison.Ordinal); + } + + // ---------- value acquisition ---------- + + [Fact] + public void The_positional_value_is_used_but_warns() + { + var stderr = new StringWriter(); + SetCommandLine line = SetCommandLine.Parse(["set", "db/pw", "hunter2"]); + + string value = line.ReadValue(TextReader.Null, stdinRedirected: false, NoPrompt, stderr); + + Assert.Equal("hunter2", value); + Assert.Contains("warning:", stderr.ToString(), StringComparison.Ordinal); + Assert.DoesNotContain("hunter2", stderr.ToString(), StringComparison.Ordinal); + } + + [Theory] + [InlineData("s3cret\n", "s3cret")] + [InlineData("s3cret\r\n", "s3cret")] + [InlineData("s3cret", "s3cret")] + [InlineData(" two lines\nkept \n", " two lines\nkept ")] + public void Stdin_drops_exactly_one_trailing_newline(string piped, string expected) + { + SetCommandLine line = SetCommandLine.Parse(["set", "db/pw", "-"]); + + string value = line.ReadValue(new StringReader(piped), stdinRedirected: true, NoPrompt, TextWriter.Null); + + Assert.Equal(expected, value); + } + + [Fact] + public void Implicit_source_reads_piped_stdin_without_prompting() + { + SetCommandLine line = SetCommandLine.Parse(["set", "db/pw"]); + + Assert.Equal("v", line.ReadValue(new StringReader("v\n"), stdinRedirected: true, NoPrompt, TextWriter.Null)); + } + + [Fact] + public void Implicit_source_at_a_terminal_prompts_twice_and_requires_a_match() + { + SetCommandLine line = SetCommandLine.Parse(["set", "db/pw"]); + var answers = new Queue(["typed", "typed", "typed", "typo"]); + var labels = new List(); + string Prompt(string label) + { + labels.Add(label); + return answers.Dequeue(); + } + + Assert.Equal("typed", line.ReadValue(TextReader.Null, stdinRedirected: false, Prompt, TextWriter.Null)); + Assert.Equal(2, labels.Count); + Assert.Throws( + () => line.ReadValue(TextReader.Null, stdinRedirected: false, Prompt, TextWriter.Null)); + } + + [Fact] + public void Empty_stdin_is_refused() + { + SetCommandLine line = SetCommandLine.Parse(["set", "db/pw", "--value-stdin"]); + + Assert.Throws( + () => line.ReadValue(new StringReader("\n"), stdinRedirected: true, NoPrompt, TextWriter.Null)); + } + + // ---------- the real executable ---------- + + [Fact] + public async Task A_dash_prefixed_positional_value_no_longer_crashes_the_host_or_leaks() + { + // THE REGRESSION. Before the fix the CLI handed argv to Host.CreateApplicationBuilder, whose + // command-line configuration provider threw an unhandled FormatException for "-p=..." with the + // whole argument — the secret — in the message on stderr. + using var rig = new CliRig(); + + CliResult set = await rig.RunAsync(["set", "db/pw", "-p=Hunter2Secret"]); + Assert.DoesNotContain("Hunter2Secret", set.Stderr, StringComparison.Ordinal); + Assert.DoesNotContain("FormatException", set.Stderr, StringComparison.Ordinal); + Assert.Equal(0, set.ExitCode); + Assert.Contains("warning:", set.Stderr, StringComparison.Ordinal); + + CliResult get = await rig.RunAsync(["get", "db/pw"]); + Assert.Equal("-p=Hunter2Secret", get.Stdout.Trim()); + } + + [Fact] + public async Task A_value_piped_on_stdin_is_stored() + { + using var rig = new CliRig(); + + CliResult set = await rig.RunAsync(["set", "db/pw", "--value-stdin"], stdin: "pw=with=equals\n"); + Assert.Equal(0, set.ExitCode); + Assert.DoesNotContain("warning:", set.Stderr, StringComparison.Ordinal); + + CliResult rotate = await rig.RunAsync(["rotate", "db/pw"], stdin: "rotated\n"); + Assert.Equal(0, rotate.ExitCode); + + CliResult get = await rig.RunAsync(["get", "db/pw"]); + Assert.Equal("rotated", get.Stdout.Trim()); + } + + private sealed record CliResult(int ExitCode, string Stdout, string Stderr); + + // Runs the built CLI against a throwaway store with a throwaway key, from an empty working + // directory so no stray appsettings.json is picked up. + private sealed class CliRig : IDisposable + { + private readonly string _dir = Path.Combine(Path.GetTempPath(), $"zb-cli-{Guid.NewGuid():N}"); + private readonly string _key = Convert.ToBase64String(RandomNumberGenerator.GetBytes(32)); + + public CliRig() => Directory.CreateDirectory(_dir); + + public async Task RunAsync(string[] args, string? stdin = null) + { + string dll = Path.Combine(AppContext.BaseDirectory, "ZB.MOM.WW.Secrets.Cli.dll"); + var psi = new ProcessStartInfo(Environment.GetEnvironmentVariable("DOTNET_HOST_PATH") ?? "dotnet") + { + WorkingDirectory = _dir, + RedirectStandardInput = true, + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + }; + psi.ArgumentList.Add(dll); + foreach (string arg in args) + { + psi.ArgumentList.Add(arg); + } + + psi.Environment["Secrets__SqlitePath"] = Path.Combine(_dir, "store", "secrets.db"); + psi.Environment["Secrets__MasterKey__Source"] = "Environment"; + psi.Environment["Secrets__MasterKey__EnvVarName"] = "ZB_SECRETS_CLI_TEST_KEY"; + psi.Environment["ZB_SECRETS_CLI_TEST_KEY"] = _key; + + using Process process = Process.Start(psi)!; + if (stdin is not null) + { + await process.StandardInput.WriteAsync(stdin); + } + + process.StandardInput.Close(); + Task stdout = process.StandardOutput.ReadToEndAsync(); + Task stderr = process.StandardError.ReadToEndAsync(); + using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(60)); + await process.WaitForExitAsync(timeout.Token); + return new CliResult(process.ExitCode, await stdout, await stderr); + } + + public void Dispose() + { + try + { + Directory.Delete(_dir, recursive: true); + } + catch (IOException) + { + // Best effort: a child process may still be releasing the SQLite file on Windows. + } + } + } +} diff --git a/code-reviews/README.md b/code-reviews/README.md index d854509..e7125dd 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 25 pending findings across all libraries. +10 of 10 libraries reviewed. 24 pending findings across all libraries. Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health, Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also @@ -50,9 +50,9 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 6 | +| Medium | 5 | | Low | 19 | -| **Total** | **25** | +| **Total** | **24** | ## Library Status @@ -65,7 +65,7 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 7 | | [LocalDb](LocalDb/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/4/3 | 7 | 7 | | [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 5 | -| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/1/13 | 14 | 19 | +| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/13 | 13 | 19 | | [Telemetry](Telemetry/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 11 | | [Theme](Theme/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 8 | @@ -84,7 +84,7 @@ _None open._ _None open._ -### Medium (6) +### Medium (5) | ID | Library | Title | |----|---------|-------| @@ -93,7 +93,6 @@ _None open._ | LocalDb-002 | [LocalDb](LocalDb/findings.md) | Snapshot streams tables by name and rows by `pk_json` text, so FK children land before parents and are dead-lettered for good | | LocalDb-003 | [LocalDb](LocalDb/findings.md) | `INSERT OR REPLACE` apply silently deletes a different row on a secondary `UNIQUE` conflict, so the pair diverges | | LocalDb-004 | [LocalDb](LocalDb/findings.md) | `FailClosedOnDrift` is bypassed on the snapshot path, so a future-stamped snapshot row moves the local HLC forward permanently | -| Secrets-017 | [Secrets](Secrets/findings.md) | `secret set`/`rotate` take the plaintext on argv; the host builder re-parses argv as configuration and can crash with the value in the error | ### Low (19) diff --git a/code-reviews/Secrets/findings.md b/code-reviews/Secrets/findings.md index 45e5787..261537c 100644 --- a/code-reviews/Secrets/findings.md +++ b/code-reviews/Secrets/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-11-09 | | Reviewer | Claude (scadaproj#15) | | Commit reviewed | `91fb2b4` | -| Open findings | 14 | +| Open findings | 13 | ## Summary @@ -570,7 +570,7 @@ _Unresolved._ | Severity | Medium | | Priority | P2 | | Category | Security & secret handling | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Program.cs:21` (values consumed at `:57`, `:65`) | **Description** @@ -604,7 +604,7 @@ Even without the crash, every `secret set` leaves the plaintext in `~/.zsh_histo **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#35)` (Secrets `0.6.6`, unpublished). All three recommendations: (1) new `SetCommandLine` parses `set|rotate [-|--value-stdin] [options]`; with no value it reads piped stdin, or prompts twice with a masked Spectre `TextPrompt.Secret()` at a terminal; stdin drops exactly one trailing newline and an empty value is refused. The positional `` form is kept for compatibility and prints a deprecation warning (never the value) to stderr. (2) The host is built with `Host.CreateApplicationBuilder(new HostApplicationBuilderSettings())`, so argv never reaches the command-line configuration provider. (3) Host build + migration are wrapped and print `secret: startup failed: ` with exit 1. Unexpected trailing arguments are reported by position, not echoed. Tests `Cli/SetCommandLineTests`: parsing/acquisition unit tests plus two that run the built CLI as a child process — `A_dash_prefixed_positional_value_no_longer_crashes_the_host_or_leaks` (the finding's `-p=Hunter2Secret` probe) and `A_value_piped_on_stdin_is_stored`; against the old `Program.cs` the first fails on the `FormatException` stderr containing `Hunter2Secret`, the second on the missing stdin support. ### Secrets-018 — Bundle import aborts mid-run on one bad entry, leaving earlier rows applied but reporting only "Import failed"