From b9b72ec3214c4e34556161c206eae25dc249a5e3 Mon Sep 17 00:00:00 2001 From: Nick Downs Date: Tue, 31 Oct 2023 06:08:12 -0700 Subject: [PATCH] Fix undelete for mount paths that are deeper than one level (#19811) * Fix for undelete for mount paths * Pulled in a fix from kv_delete.go that adds partialPath to the mountPath to support paths that are multiple levels deep. * Added undelete tests to validate that KV secrets can be successfully undeleted when mounted at a multi-level mount path. * Added changelog txt file * Update changelog to specify KV impact --------- Co-authored-by: Violet Hynes --- changelog/19811.txt | 3 + command/kv_test.go | 149 +++++++++++++++++++++++++++++++++++++++++ command/kv_undelete.go | 25 ++++--- 3 files changed, 169 insertions(+), 8 deletions(-) create mode 100644 changelog/19811.txt diff --git a/changelog/19811.txt b/changelog/19811.txt new file mode 100644 index 0000000000..49af10cceb --- /dev/null +++ b/changelog/19811.txt @@ -0,0 +1,3 @@ +```release-note:bug +cli/kv: Undelete now properly handles KV-V2 mount paths that are more than one layer deep. +``` diff --git a/command/kv_test.go b/command/kv_test.go index 1471b009a1..7e5a927df0 100644 --- a/command/kv_test.go +++ b/command/kv_test.go @@ -1522,6 +1522,155 @@ func TestPadEqualSigns(t *testing.T) { }) } } +func testKVUndeleteCommand(tb testing.TB) (*cli.MockUi, *KVUndeleteCommand) { + tb.Helper() + + ui := cli.NewMockUi() + return ui, &KVUndeleteCommand{ + BaseCommand: &BaseCommand{ + UI: ui, + }, + } +} + +func TestKVUndeleteCommand(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + args []string + outStrings []string + code int + }{ + { + "not_enough_args", + []string{}, + []string{"Not enough arguments"}, + 1, + }, + { + "too_many_args", + []string{"foo", "bar"}, + []string{"Too many arguments"}, + 1, + }, + { + "no_versions", + []string{"-mount", "kv", "/read/foo"}, + []string{"No versions provided"}, + 1, + }, + { + "v2_mount_flag_syntax", + []string{"-versions", "1", "-mount", "kv", "read/foo"}, + []string{"Success! Data written to: kv/undelete/read/foo"}, + 0, + }, + { + "v2_mount_flag_syntax_complex_1", + []string{"-versions", "1", "-mount", "secrets/testapp", "test"}, + []string{"Success! Data written to: secrets/testapp/undelete/test"}, + 0, + }, + { + "v2_mount_flag_syntax_complex_2", + []string{"-versions", "1", "-mount", "secrets/x/testapp", "test"}, + []string{"Success! Data written to: secrets/x/testapp/undelete/test"}, + 0, + }, + } + + t.Run("validations", func(t *testing.T) { + t.Parallel() + + for _, tc := range cases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + client, closer := testVaultServer(t) + defer closer() + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatal(err) + } + + if err := client.Sys().Mount("secrets/testapp", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatal(err) + } + + // Additional layer of mount path + if err := client.Sys().Mount("secrets/x/testapp", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatal(err) + } + + // Give time for the upgrade code to run/finish + time.Sleep(time.Second) + + if _, err := client.Logical().Write("kv/data/read/foo", map[string]interface{}{ + "data": map[string]interface{}{ + "foo": "bar", + }, + }); err != nil { + t.Fatal(err) + } + + // Delete the entry so we can undelete it + if _, err := client.Logical().Delete("kv/data/read/foo"); err != nil { + t.Fatal(err) + } + + if _, err := client.Logical().Write("secrets/testapp/data/test", map[string]interface{}{ + "data": map[string]interface{}{ + "complex": "yes", + }, + }); err != nil { + t.Fatal(err) + } + + if _, err := client.Logical().Write("secrets/x/testapp/data/test", map[string]interface{}{ + "data": map[string]interface{}{ + "complex": "yes", + }, + }); err != nil { + t.Fatal(err) + } + + // Delete the entry so we can undelete it + if _, err := client.Logical().Delete("secrets/x/testapp/data/test"); err != nil { + t.Fatal(err) + } + + // Delete the entry so we can undelete it + if _, err := client.Logical().Delete("secrets/testapp/data/test"); err != nil { + t.Fatal(err) + } + + ui, cmd := testKVUndeleteCommand(t) + cmd.client = client + + code := cmd.Run(tc.args) + if code != tc.code { + t.Errorf("expected %d to be %d", code, tc.code) + } + + combined := ui.OutputWriter.String() + ui.ErrorWriter.String() + + for _, str := range tc.outStrings { + if !strings.Contains(combined, str) { + t.Errorf("expected %q to contain %q", combined, str) + } + } + }) + } + }) +} func createTokenForPolicy(t *testing.T, client *api.Client, policy string) (*api.SecretAuth, error) { t.Helper() diff --git a/command/kv_undelete.go b/command/kv_undelete.go index 838f8cb211..abc90879e6 100644 --- a/command/kv_undelete.go +++ b/command/kv_undelete.go @@ -5,6 +5,7 @@ package command import ( "fmt" + "path" "strings" "github.com/mitchellh/cli" @@ -35,12 +36,12 @@ Usage: vault kv undelete [options] KEY This restores the data, allowing it to be returned on get requests. To undelete version 3 of key "foo": - + $ vault kv undelete -mount=secret -versions=3 foo - The deprecated path-like syntax can also be used, but this should be avoided, - as the fact that it is not actually the full API path to - the secret (secret/data/foo) can cause confusion: + The deprecated path-like syntax can also be used, but this should be avoided, + as the fact that it is not actually the full API path to + the secret (secret/data/foo) can cause confusion: $ vault kv undelete -versions=3 secret/foo @@ -67,10 +68,10 @@ func (c *KVUndeleteCommand) Flags() *FlagSets { Name: "mount", Target: &c.flagMount, Default: "", // no default, because the handling of the next arg is determined by whether this flag has a value - Usage: `Specifies the path where the KV backend is mounted. If specified, - the next argument will be interpreted as the secret path. If this flag is - not specified, the next argument will be interpreted as the combined mount - path and secret path, with /data/ automatically appended between KV + Usage: `Specifies the path where the KV backend is mounted. If specified, + the next argument will be interpreted as the secret path. If this flag is + not specified, the next argument will be interpreted as the combined mount + path and secret path, with /data/ automatically appended between KV v2 secrets.`, }) @@ -134,6 +135,14 @@ func (c *KVUndeleteCommand) Run(args []string) int { c.UI.Error(err.Error()) return 2 } + if v2 { + // Without this join, mountPaths that are deeper + // than the root path E.G. secrets/myapp will get + // pruned down to myapp/undelete/ which + // is incorrect. + // This technique was lifted from kv_delete.go. + partialPath = path.Join(mountPath, partialPath) + } } else { // In this case, this arg is a path-like combination of mountPath/secretPath. // (e.g. "secret/foo")