From 04018aab6e225d7b355e3fcb400756b09c637d4b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 19 Jul 2026 10:00:59 -0400 Subject: [PATCH] =?UTF-8?q?fix(secrets-cli):=20bundle=20import=20=E2=80=94?= =?UTF-8?q?=20source-key=20verification,=20format=20gate=20up=20front,=20a?= =?UTF-8?q?ccess-denied=20containment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Interactive/Flows/BundleImportFlow.cs | 38 ++++++++++-- .../Cli/Interactive/Flows/BundleFlowTests.cs | 60 ++++++++++++++++++- 2 files changed, 91 insertions(+), 7 deletions(-) diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/BundleImportFlow.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/BundleImportFlow.cs index 37a1473..7cff49b 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/BundleImportFlow.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/BundleImportFlow.cs @@ -45,19 +45,29 @@ public sealed class BundleImportFlow : IInteractiveFlow return; } - // Read + parse FIRST so the source KEK can be inspected before any write is attempted. + // Read + parse FIRST so the format version and source KEK can be inspected before any write — + // or any prompting — is attempted. SecretBundle bundle; try { string json = await File.ReadAllTextAsync(path, ct).ConfigureAwait(false); bundle = SecretBundleCodec.Deserialize(json); } - catch (Exception ex) when (ex is InvalidOperationException or JsonException or IOException) + catch (Exception ex) when (ex is InvalidOperationException or JsonException or IOException or UnauthorizedAccessException) { console.MarkupLineInterpolated($"[red]Could not read bundle '{path}': {ex.Message}[/]"); return; } + // Format gate up front: reject an unreadable version before consuming any operator input. The + // service applies the same gate as defense-in-depth, but failing here keeps the flow honest. + if (bundle.FormatVersion != SecretBundle.CurrentFormatVersion) + { + console.MarkupLineInterpolated( + $"[red]Bundle '{path}' has unsupported format version {bundle.FormatVersion}; this build supports version {SecretBundle.CurrentFormatVersion}.[/]"); + return; + } + // Foreign-KEK detection: a bundle exported under another KEK must be re-wrapped, which needs the // source key. Prompt for it up front; without it every row would be skipped as foreign-KEK. IMasterKeyProvider? sourceKek = null; @@ -74,6 +84,15 @@ public sealed class BundleImportFlow : IInteractiveFlow console.MarkupLine("[yellow]Import cancelled.[/]"); return; } + + // A valid-but-wrong key would otherwise silently land every row in SkippedForeignKek. Verify + // the supplied key actually derives the bundle's source KEK id before importing. + if (!string.Equals(sourceKek.KekId, bundle.SourceKekId, StringComparison.Ordinal)) + { + console.MarkupLineInterpolated( + $"[red]That key does not match this bundle's source KEK ('{bundle.SourceKekId}' expected, got '{sourceKek.KekId}').[/]"); + return; + } } bool perConflict = console.Prompt( @@ -97,7 +116,7 @@ public sealed class BundleImportFlow : IInteractiveFlow return; } - RenderReport(console, report); + RenderReport(console, report, bundle.SourceKekId); } // Builds the source-KEK provider the operator selects: a pasted base64 key (masked), an environment @@ -166,16 +185,23 @@ public sealed class BundleImportFlow : IInteractiveFlow return console.Prompt(new ConfirmationPrompt(" Take the bundle row?") { DefaultValue = false }); } - // A compact tally table of the import outcome. - private static void RenderReport(IAnsiConsole console, BundleImportReport report) + // A compact tally table of the import outcome. When rows were skipped as foreign-KEK, the bundle's + // source KEK id is appended so a wrong-key paste is self-diagnosable from the report alone. + private static void RenderReport(IAnsiConsole console, BundleImportReport report, string sourceKekId) { var table = new Table() .AddColumn("Result") .AddColumn("Rows"); + string foreign = report.SkippedForeignKek.ToString(CultureInfo.InvariantCulture); + if (report.SkippedForeignKek > 0) + { + foreign += $" (source KEK {Markup.Escape(sourceKekId)})"; + } + table.AddRow("Imported", report.Imported.ToString(CultureInfo.InvariantCulture)); table.AddRow("Skipped (older)", report.SkippedOlder.ToString(CultureInfo.InvariantCulture)); - table.AddRow("Skipped (foreign KEK)", report.SkippedForeignKek.ToString(CultureInfo.InvariantCulture)); + table.AddRow("Skipped (foreign KEK)", foreign); table.AddRow("Conflicts", report.Conflicts.ToString(CultureInfo.InvariantCulture)); console.Write(table); diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/Flows/BundleFlowTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/Flows/BundleFlowTests.cs index 727b81c..3b888e3 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/Flows/BundleFlowTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/Flows/BundleFlowTests.cs @@ -100,7 +100,7 @@ public sealed class BundleFlowTests : IDisposable Assert.True(new BundleExportFlow().RequiresKek); Assert.True(File.Exists(outPath)); - Assert.Contains("2", console.Output, StringComparison.Ordinal); + Assert.Contains("Exported 2 row(s)", console.Output, StringComparison.Ordinal); // The offered default filename carries the target name and the .json extension. Assert.Contains($"secrets-bundle-{session.Target.Name}", console.Output, StringComparison.Ordinal); Assert.Contains(".json", console.Output, StringComparison.Ordinal); @@ -201,4 +201,62 @@ public sealed class BundleFlowTests : IDisposable Assert.DoesNotContain(base64A, console.Output, StringComparison.Ordinal); // pasted key never echoed } + + [Fact] + public async Task Import_wrong_source_key_is_rejected_without_importing() + { + LiteralMasterKeyProvider kekA = NewKek(out _); + LiteralMasterKeyProvider kekB = NewKek(out _); + LiteralMasterKeyProvider wrong = NewKek(out string wrongBase64); // valid format, unrelated key + Assert.NotEqual(kekA.KekId, wrong.KekId); + + SecretsSession source = await NewSessionAsync("a.db", kekA); + await SealAsync(source, "svc/cross", "cross-value"); + + var exportConsole = NewConsole(); + exportConsole.Input.PushTextWithEnter(BundlePath); + exportConsole.Input.PushTextWithEnter("n"); + await new BundleExportFlow().RunAsync(exportConsole, source, CancellationToken.None); + + SecretsSession target = await NewSessionAsync("b.db", kekB); + + TestConsole console = NewConsole(); + console.Input.PushTextWithEnter(BundlePath); // path + console.Input.PushKey(ConsoleKey.Enter); // source-key selection: paste base64 + console.Input.PushTextWithEnter(wrongBase64); // a valid-format but wrong key + + await new BundleImportFlow().RunAsync(console, target, CancellationToken.None); + + Assert.Contains("does not match this bundle's source KEK", console.Output, StringComparison.Ordinal); + Assert.DoesNotContain("Imported", console.Output, StringComparison.Ordinal); // no report rendered + Assert.Null(await target.Store.GetAsync(new SecretName("svc/cross"), CancellationToken.None)); // untouched + } + + [Fact] + public async Task Import_rejects_unknown_format_version_before_prompting() + { + LiteralMasterKeyProvider kek = NewKek(out _); + SecretsSession target = await NewSessionAsync("b.db", kek); + + // A hand-built bundle envelope on an unreadable format version. Only the path is scripted: if the + // flow prompted for anything past the format gate, the missing input would fail the test. + const string json = """ + { + "FormatVersion": 99, + "ExportedUtc": "2026-01-01T00:00:00+00:00", + "SourceKekId": "some-kek", + "Entries": [] + } + """; + await File.WriteAllTextAsync(BundlePath, json, CancellationToken.None); + + TestConsole console = NewConsole(); + console.Input.PushTextWithEnter(BundlePath); // path only — no further prompt must be consumed + + await new BundleImportFlow().RunAsync(console, target, CancellationToken.None); + + Assert.Contains("99", console.Output, StringComparison.Ordinal); + Assert.Contains("format version", console.Output, StringComparison.Ordinal); + Assert.DoesNotContain("Imported", console.Output, StringComparison.Ordinal); // no report rendered + } }