From db205c9e331b4aaf71c60b48c14fca4cb61af675 Mon Sep 17 00:00:00 2001 From: Chris Capurso <1036769+ccapurso@users.noreply.github.com> Date: Wed, 26 Oct 2022 16:26:05 -0400 Subject: [PATCH 1/4] fix mount flag behavior for kv subcommands --- command/kv_delete.go | 9 ++++++--- command/kv_destroy.go | 10 +++++++--- command/kv_get.go | 9 ++++++--- command/kv_patch.go | 10 +++++++--- command/kv_put.go | 9 ++++++--- command/kv_rollback.go | 10 +++++++--- 6 files changed, 39 insertions(+), 18 deletions(-) diff --git a/command/kv_delete.go b/command/kv_delete.go index 65e0927630ad3..5555b9c71a895 100644 --- a/command/kv_delete.go +++ b/command/kv_delete.go @@ -117,7 +117,7 @@ func (c *KVDeleteCommand) Run(args []string) int { // If true, we're working with "-mount=secret foo" syntax. // If false, we're using "secret/foo" syntax. - mountFlagSyntax := (c.flagMount != "") + mountFlagSyntax := c.flagMount != "" var ( mountPath string @@ -129,12 +129,15 @@ func (c *KVDeleteCommand) Run(args []string) int { if mountFlagSyntax { // In this case, this arg is the secret path (e.g. "foo"). partialPath = sanitizePath(args[0]) - mountPath = sanitizePath(c.flagMount) - _, v2, err = isKVv2(mountPath, client) + mountPath, v2, err = isKVv2(sanitizePath(c.flagMount), client) if err != nil { c.UI.Error(err.Error()) return 2 } + + if v2 { + partialPath = path.Join(mountPath, partialPath) + } } else { // In this case, this arg is a path-like combination of mountPath/secretPath. // (e.g. "secret/foo") diff --git a/command/kv_destroy.go b/command/kv_destroy.go index 33f5452a74e0e..45cbca02518bc 100644 --- a/command/kv_destroy.go +++ b/command/kv_destroy.go @@ -2,6 +2,7 @@ package command import ( "fmt" + "path" "strings" "github.com/mitchellh/cli" @@ -115,7 +116,7 @@ func (c *KVDestroyCommand) Run(args []string) int { // If true, we're working with "-mount=secret foo" syntax. // If false, we're using "secret/foo" syntax. - mountFlagSyntax := (c.flagMount != "") + mountFlagSyntax := c.flagMount != "" var ( mountPath string @@ -127,12 +128,15 @@ func (c *KVDestroyCommand) Run(args []string) int { if mountFlagSyntax { // In this case, this arg is the secret path (e.g. "foo"). partialPath = sanitizePath(args[0]) - mountPath = sanitizePath(c.flagMount) - _, v2, err = isKVv2(mountPath, client) + mountPath, v2, err = isKVv2(sanitizePath(c.flagMount), client) if err != nil { c.UI.Error(err.Error()) return 2 } + + if v2 { + partialPath = path.Join(mountPath, partialPath) + } } else { // In this case, this arg is a path-like combination of mountPath/secretPath. // (e.g. "secret/foo") diff --git a/command/kv_get.go b/command/kv_get.go index 391efaf882cec..057a787c4e286 100644 --- a/command/kv_get.go +++ b/command/kv_get.go @@ -113,7 +113,7 @@ func (c *KVGetCommand) Run(args []string) int { // If true, we're working with "-mount=secret foo" syntax. // If false, we're using "secret/foo" syntax. - mountFlagSyntax := (c.flagMount != "") + mountFlagSyntax := c.flagMount != "" var ( mountPath string @@ -126,12 +126,15 @@ func (c *KVGetCommand) Run(args []string) int { // Parse the paths and grab the KV version if mountFlagSyntax { // In this case, this arg is the secret path (e.g. "foo"). - mountPath = sanitizePath(c.flagMount) - _, v2, err = isKVv2(mountPath, client) + mountPath, v2, err = isKVv2(sanitizePath(c.flagMount), client) if err != nil { c.UI.Error(err.Error()) return 2 } + + if v2 { + partialPath = path.Join(mountPath, partialPath) + } } else { // In this case, this arg is a path-like combination of mountPath/secretPath. // (e.g. "secret/foo") diff --git a/command/kv_patch.go b/command/kv_patch.go index 5368d9e0dbd03..6a736fa247f1e 100644 --- a/command/kv_patch.go +++ b/command/kv_patch.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "os" + "path" "strings" "github.com/hashicorp/vault/api" @@ -167,7 +168,7 @@ func (c *KVPatchCommand) Run(args []string) int { // If true, we're working with "-mount=secret foo" syntax. // If false, we're using "secret/foo" syntax. - mountFlagSyntax := (c.flagMount != "") + mountFlagSyntax := c.flagMount != "" var ( mountPath string @@ -179,12 +180,15 @@ func (c *KVPatchCommand) Run(args []string) int { if mountFlagSyntax { // In this case, this arg is the secret path (e.g. "foo"). partialPath = sanitizePath(args[0]) - mountPath = sanitizePath(c.flagMount) - _, v2, err = isKVv2(mountPath, client) + mountPath, v2, err = isKVv2(sanitizePath(c.flagMount), client) if err != nil { c.UI.Error(err.Error()) return 2 } + + if v2 { + partialPath = path.Join(mountPath, partialPath) + } } else { // In this case, this arg is a path-like combination of mountPath/secretPath. // (e.g. "secret/foo") diff --git a/command/kv_put.go b/command/kv_put.go index fe991711a56b5..5cc7b6fbc67b8 100644 --- a/command/kv_put.go +++ b/command/kv_put.go @@ -143,7 +143,7 @@ func (c *KVPutCommand) Run(args []string) int { // If true, we're working with "-mount=secret foo" syntax. // If false, we're using "secret/foo" syntax. - mountFlagSyntax := (c.flagMount != "") + mountFlagSyntax := c.flagMount != "" var ( mountPath string @@ -155,12 +155,15 @@ func (c *KVPutCommand) Run(args []string) int { if mountFlagSyntax { // In this case, this arg is the secret path (e.g. "foo"). partialPath = sanitizePath(args[0]) - mountPath = sanitizePath(c.flagMount) - _, v2, err = isKVv2(mountPath, client) + mountPath, v2, err = isKVv2(sanitizePath(c.flagMount), client) if err != nil { c.UI.Error(err.Error()) return 2 } + + if v2 { + partialPath = path.Join(mountPath, partialPath) + } } else { // In this case, this arg is a path-like combination of mountPath/secretPath. // (e.g. "secret/foo") diff --git a/command/kv_rollback.go b/command/kv_rollback.go index e69dba6f6c804..0d782619a8320 100644 --- a/command/kv_rollback.go +++ b/command/kv_rollback.go @@ -3,6 +3,7 @@ package command import ( "flag" "fmt" + "path" "strings" "github.com/mitchellh/cli" @@ -123,7 +124,7 @@ func (c *KVRollbackCommand) Run(args []string) int { // If true, we're working with "-mount=secret foo" syntax. // If false, we're using "secret/foo" syntax. - mountFlagSyntax := (c.flagMount != "") + mountFlagSyntax := c.flagMount != "" var ( mountPath string @@ -135,12 +136,15 @@ func (c *KVRollbackCommand) Run(args []string) int { if mountFlagSyntax { // In this case, this arg is the secret path (e.g. "foo"). partialPath = sanitizePath(args[0]) - mountPath = sanitizePath(c.flagMount) - _, v2, err = isKVv2(mountPath, client) + mountPath, v2, err = isKVv2(sanitizePath(c.flagMount), client) if err != nil { c.UI.Error(err.Error()) return 2 } + + if v2 { + partialPath = path.Join(mountPath, partialPath) + } } else { // In this case, this arg is a path-like combination of mountPath/secretPath. // (e.g. "secret/foo") From cd65485505d4819b14511a3be1a763e1fbf6c4e3 Mon Sep 17 00:00:00 2001 From: Chris Capurso <1036769+ccapurso@users.noreply.github.com> Date: Wed, 26 Oct 2022 16:26:22 -0400 Subject: [PATCH 2/4] fix mount flag behavior for kv metadata subcommands --- command/kv_metadata_delete.go | 10 +++++++--- command/kv_metadata_get.go | 10 +++++++--- command/kv_metadata_patch.go | 10 +++++++--- command/kv_metadata_put.go | 10 +++++++--- 4 files changed, 28 insertions(+), 12 deletions(-) diff --git a/command/kv_metadata_delete.go b/command/kv_metadata_delete.go index 911f00117e762..cff16f21c6fe2 100644 --- a/command/kv_metadata_delete.go +++ b/command/kv_metadata_delete.go @@ -2,6 +2,7 @@ package command import ( "fmt" + "path" "strings" "github.com/mitchellh/cli" @@ -97,7 +98,7 @@ func (c *KVMetadataDeleteCommand) Run(args []string) int { // If true, we're working with "-mount=secret foo" syntax. // If false, we're using "secret/foo" syntax. - mountFlagSyntax := (c.flagMount != "") + mountFlagSyntax := c.flagMount != "" var ( mountPath string @@ -109,12 +110,15 @@ func (c *KVMetadataDeleteCommand) Run(args []string) int { if mountFlagSyntax { // In this case, this arg is the secret path (e.g. "foo"). partialPath = sanitizePath(args[0]) - mountPath = sanitizePath(c.flagMount) - _, v2, err = isKVv2(mountPath, client) + mountPath, v2, err = isKVv2(sanitizePath(c.flagMount), client) if err != nil { c.UI.Error(err.Error()) return 2 } + + if v2 { + partialPath = path.Join(mountPath, partialPath) + } } else { // In this case, this arg is a path-like combination of mountPath/secretPath. // (e.g. "secret/foo") diff --git a/command/kv_metadata_get.go b/command/kv_metadata_get.go index 08e401374a700..8920340752d5a 100644 --- a/command/kv_metadata_get.go +++ b/command/kv_metadata_get.go @@ -2,6 +2,7 @@ package command import ( "fmt" + "path" "sort" "strconv" "strings" @@ -99,7 +100,7 @@ func (c *KVMetadataGetCommand) Run(args []string) int { // If true, we're working with "-mount=secret foo" syntax. // If false, we're using "secret/foo" syntax. - mountFlagSyntax := (c.flagMount != "") + mountFlagSyntax := c.flagMount != "" var ( mountPath string @@ -111,12 +112,15 @@ func (c *KVMetadataGetCommand) Run(args []string) int { if mountFlagSyntax { // In this case, this arg is the secret path (e.g. "foo"). partialPath = sanitizePath(args[0]) - mountPath = sanitizePath(c.flagMount) - _, v2, err = isKVv2(mountPath, client) + mountPath, v2, err = isKVv2(sanitizePath(c.flagMount), client) if err != nil { c.UI.Error(err.Error()) return 2 } + + if v2 { + partialPath = path.Join(mountPath, partialPath) + } } else { // In this case, this arg is a path-like combination of mountPath/secretPath. // (e.g. "secret/foo") diff --git a/command/kv_metadata_patch.go b/command/kv_metadata_patch.go index d3550e6450b82..b63da2369a2cd 100644 --- a/command/kv_metadata_patch.go +++ b/command/kv_metadata_patch.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "path" "strings" "time" @@ -159,7 +160,7 @@ func (c *KVMetadataPatchCommand) Run(args []string) int { // If true, we're working with "-mount=secret foo" syntax. // If false, we're using "secret/foo" syntax. - mountFlagSyntax := (c.flagMount != "") + mountFlagSyntax := c.flagMount != "" var ( mountPath string @@ -171,12 +172,15 @@ func (c *KVMetadataPatchCommand) Run(args []string) int { if mountFlagSyntax { // In this case, this arg is the secret path (e.g. "foo"). partialPath = sanitizePath(args[0]) - mountPath = sanitizePath(c.flagMount) - _, v2, err = isKVv2(mountPath, client) + mountPath, v2, err = isKVv2(sanitizePath(c.flagMount), client) if err != nil { c.UI.Error(err.Error()) return 2 } + + if v2 { + partialPath = path.Join(mountPath, partialPath) + } } else { // In this case, this arg is a path-like combination of mountPath/secretPath. // (e.g. "secret/foo") diff --git a/command/kv_metadata_put.go b/command/kv_metadata_put.go index 57aa3ebd9b5d7..5196b1c79a0a6 100644 --- a/command/kv_metadata_put.go +++ b/command/kv_metadata_put.go @@ -3,6 +3,7 @@ package command import ( "fmt" "io" + "path" "strings" "time" @@ -158,7 +159,7 @@ func (c *KVMetadataPutCommand) Run(args []string) int { // If true, we're working with "-mount=secret foo" syntax. // If false, we're using "secret/foo" syntax. - mountFlagSyntax := (c.flagMount != "") + mountFlagSyntax := c.flagMount != "" var ( mountPath string @@ -170,12 +171,15 @@ func (c *KVMetadataPutCommand) Run(args []string) int { if mountFlagSyntax { // In this case, this arg is the secret path (e.g. "foo"). partialPath = sanitizePath(args[0]) - mountPath = sanitizePath(c.flagMount) - _, v2, err = isKVv2(mountPath, client) + mountPath, v2, err = isKVv2(sanitizePath(c.flagMount), client) if err != nil { c.UI.Error(err.Error()) return 2 } + + if v2 { + partialPath = path.Join(mountPath, partialPath) + } } else { // In this case, this arg is a path-like combination of mountPath/secretPath. // (e.g. "secret/foo") From 6d811494e3735d8afdd70f4125384b59a7235ac3 Mon Sep 17 00:00:00 2001 From: Chris Capurso <1036769+ccapurso@users.noreply.github.com> Date: Wed, 26 Oct 2022 16:26:39 -0400 Subject: [PATCH 3/4] add tests --- command/kv_test.go | 74 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 2 deletions(-) diff --git a/command/kv_test.go b/command/kv_test.go index eba5910435db4..830c9cc30770e 100644 --- a/command/kv_test.go +++ b/command/kv_test.go @@ -129,6 +129,12 @@ func TestKVPutCommand(t *testing.T) { []string{"Success!"}, 0, }, + { + "v1_mount_flag_syntax_key_same_as_mount", + []string{"-mount", "secret", "secret", "foo=bar"}, + []string{"Success!"}, + 0, + }, { "v2_single_value", []string{"kv/write/foo", "foo=bar"}, @@ -153,6 +159,12 @@ func TestKVPutCommand(t *testing.T) { v2ExpectedFields, 0, }, + { + "v2_mount_flag_syntax_key_same_as_mount", + []string{"-mount", "kv", "kv", "foo=bar"}, + v2ExpectedFields, + 0, + }, { "v2_single_value_backslash", []string{"kv/write/foo", "foo=\\"}, @@ -464,6 +476,18 @@ func TestKVGetCommand(t *testing.T) { append(baseV2ExpectedFields, "foo"), 0, }, + { + "v1_mount_flag_syntax_key_same_as_mount", + []string{"-mount", "kv", "kv"}, + append(baseV2ExpectedFields, "foo"), + 0, + }, + { + "v2_mount_flag_syntax_key_same_as_mount", + []string{"-mount", "kv", "kv"}, + append(baseV2ExpectedFields, "foo"), + 0, + }, { "v2_not_found", []string{"kv/nope/not/once/never"}, @@ -524,6 +548,21 @@ func TestKVGetCommand(t *testing.T) { t.Fatal(err) } + // create KV entries to test -mount flag where secret key is same as mount path + if _, err := client.Logical().Write("secret/secret", map[string]interface{}{ + "foo": "bar", + }); err != nil { + t.Fatal(err) + } + + if _, err := client.Logical().Write("kv/data/kv", map[string]interface{}{ + "data": map[string]interface{}{ + "foo": "bar", + }, + }); err != nil { + t.Fatal(err) + } + ui, cmd := testKVGetCommand(t) cmd.client = client @@ -613,6 +652,12 @@ func TestKVMetadataGetCommand(t *testing.T) { expectedTopLevelFields, 0, }, + { + "mount_flag_syntax_key_same_as_mount", + []string{"-mount", "kv", "kv"}, + expectedTopLevelFields, + 0, + }, } t.Run("validations", func(t *testing.T) { @@ -643,6 +688,15 @@ func TestKVMetadataGetCommand(t *testing.T) { t.Fatal(err) } + // create KV entry to test -mount flag where secret key is same as mount path + if _, err := client.Logical().Write("kv/data/kv", map[string]interface{}{ + "data": map[string]interface{}{ + "foo": "bar", + }, + }); err != nil { + t.Fatal(err) + } + ui, cmd := testKVMetadataGetCommand(t) cmd.client = client @@ -973,6 +1027,7 @@ func TestKVPatchCommand_RWMethodSucceeds(t *testing.T) { func TestKVPatchCommand_CAS(t *testing.T) { cases := []struct { name string + key string args []string expected string outStrings []string @@ -980,6 +1035,7 @@ func TestKVPatchCommand_CAS(t *testing.T) { }{ { "right version", + "foo", []string{"-cas", "1", "kv/foo", "bar=quux"}, "quux", expectedPatchFields(), @@ -987,6 +1043,7 @@ func TestKVPatchCommand_CAS(t *testing.T) { }, { "wrong version", + "foo", []string{"-cas", "2", "kv/foo", "bar=wibble"}, "baz", []string{"check-and-set parameter did not match the current version"}, @@ -994,11 +1051,20 @@ func TestKVPatchCommand_CAS(t *testing.T) { }, { "mount_flag_syntax", + "foo", []string{"-mount", "kv", "-cas", "1", "foo", "bar=quux"}, "quux", expectedPatchFields(), 0, }, + { + "v2_mount_flag_syntax_key_same_as_mount", + "kv", + []string{"-mount", "kv", "-cas", "1", "kv", "bar=quux"}, + "quux", + expectedPatchFields(), + 0, + }, } for _, tc := range cases { @@ -1030,7 +1096,11 @@ func TestKVPatchCommand_CAS(t *testing.T) { kvClient.SetToken(secretAuth.ClientToken) - _, err = kvClient.Logical().Write("kv/data/foo", map[string]interface{}{"data": map[string]interface{}{"bar": "baz"}}) + data := map[string]interface{}{ + "bar": "baz", + } + + _, err = kvClient.Logical().Write("kv/data/"+tc.key, map[string]interface{}{"data": data}) if err != nil { t.Fatal(err) } @@ -1047,7 +1117,7 @@ func TestKVPatchCommand_CAS(t *testing.T) { } } - secret, err := kvClient.Logical().ReadWithContext(context.Background(), "kv/data/foo") + secret, err := kvClient.Logical().ReadWithContext(context.Background(), "kv/data/"+tc.key) if err != nil { t.Fatal(err) } From cac03e4fa3ff8902c61812397cde2b624d1cb1fd Mon Sep 17 00:00:00 2001 From: Chris Capurso <1036769+ccapurso@users.noreply.github.com> Date: Wed, 26 Oct 2022 16:40:43 -0400 Subject: [PATCH 4/4] add changelog entry --- changelog/17679.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/17679.txt diff --git a/changelog/17679.txt b/changelog/17679.txt new file mode 100644 index 0000000000000..b77f631eaff51 --- /dev/null +++ b/changelog/17679.txt @@ -0,0 +1,3 @@ +```release-note:bug +cli: Fix issue preventing kv commands from executing properly when the mount path provided by `-mount` flag and secret key path are the same. +```