Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mount flag syntax to mitigate confusion from KV-v2 path discrepancies #14807

Merged
merged 11 commits into from
Apr 6, 2022
Merged

Mount flag syntax to mitigate confusion from KV-v2 path discrepancies #14807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
74c7dab
Add explanation to help text and flag usage text
digivava Mar 28, 2022
382b4a9
KV get with new mount flag
digivava Mar 29, 2022
13b4b23
Clearer naming
digivava Mar 30, 2022
d5682cc
KV Put, Patch, Metadata Get + corresponding tests
digivava Mar 31, 2022
b086046
KV Delete, Destroy, Rollback, Undelete, MetadataDelete, MetadataPatch…
digivava Mar 31, 2022
a097eb2
Update KV-v2 docs to use mount flag syntax
digivava Mar 31, 2022
a52bb1a
Add changelog
digivava Mar 31, 2022
b41613e
Run make fmt
digivava Mar 31, 2022
890ca5b
Clarify deprecation message in help string
digivava Apr 5, 2022
d8c8b40
Address style comments
digivava Apr 6, 2022
c1fe715
Fix merge conflict
digivava Apr 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/14807.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
cli: Alternative flag-based syntax for KV to mitigate confusion from automatically appended /data
```
14 changes: 10 additions & 4 deletions command/kv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,25 @@ Usage: vault kv <subcommand> [options] [args]
Create or update the key named "foo" in the "secret" mount with the value
"bar=baz":

$ vault kv put secret/foo bar=baz
$ vault kv put -mount=secret foo bar=baz

Read this value back:

$ vault kv get secret/foo
$ vault kv get -mount=secret foo

Get metadata for the key:

$ vault kv metadata get secret/foo
$ vault kv metadata get -mount=secret foo

Get a specific version of the key:

$ vault kv get -version=1 secret/foo
$ vault kv get -mount=secret -version=1 foo

The deprecated path-like syntax can also be used, but this should be avoided
for KV v2, as the fact that it is not actually the full API path to
the secret (secret/data/foo) can cause confusion:

$ vault kv get secret/foo

Please see the individual subcommand help for detailed usage information.
`
Expand Down
81 changes: 61 additions & 20 deletions command/kv_delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package command

import (
"fmt"
"path"
"strings"

"github.com/hashicorp/vault/api"
Expand All @@ -18,6 +19,7 @@ type KVDeleteCommand struct {
*BaseCommand

flagVersions []string
flagMount string
}

func (c *KVDeleteCommand) Synopsis() string {
Expand All @@ -34,11 +36,17 @@ Usage: vault kv delete [options] PATH

To delete the latest version of the key "foo":

$ vault kv delete -mount=secret foo

The deprecated path-like syntax can also be used, but this should be avoided
for KV v2, as the fact that it is not actually the full API path to
the secret (secret/data/foo) can cause confusion:

$ vault kv delete secret/foo

To delete version 3 of key foo:

$ vault kv delete -versions=3 secret/foo
$ vault kv delete -mount=secret -versions=3 foo

To delete all versions and metadata, see the "vault kv metadata" subcommand.

Expand All @@ -61,6 +69,17 @@ func (c *KVDeleteCommand) Flags() *FlagSets {
Usage: `Specifies the version numbers to delete.`,
})

f.StringVar(&StringVar{
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
v2 secrets.`,
})

return set
}

Expand Down Expand Up @@ -96,22 +115,54 @@ func (c *KVDeleteCommand) Run(args []string) int {
return 2
}

path := sanitizePath(args[0])
mountPath, v2, err := isKVv2(path, client)
if err != nil {
c.UI.Error(err.Error())
return 2
// If true, we're working with "-mount=secret foo" syntax.
// If false, we're using "secret/foo" syntax.
mountFlagSyntax := (c.flagMount != "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking how to refactor this a bit to reduce the code duplication. This is what I have so far, not sure if it will work, so feel free to ignore:

func splitPaths(...) (mountPath, partialPath, fullPath string) {
	if c.flagMount != "" {
		// In this case, this arg is the secret path (e.g. "foo").
		partialPath = sanitizePath(args[0])
		mountPath = sanitizePath(c.flagMount)
		fullPath = path.Join(mountPath, partialPath)
	} else {
		// In this case, this arg is a path-like combination of mountPath/secretPath.
		// (e.g. "secret/foo")
		partialPath = sanitizePath(args[0])
		mountPath = ""
                fullPath = partialPath
	}

	return mountPath, partialPath, fullPath 
}

Then in the function code:

      mountPath, partialPath, fullPath := splitPaths(...)
      
      mountPath, v2, err := isKVv2(path, client)
      if err != nil {
           c.UI.Error(err.Error())
           return 2
       }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for thinking about it! Unfortunately, the "fullPath" isn't so simple. fullPath is only equal to partialPath when it's KV v1, so we need the isKVv2 call to happen first to determine fullPath. Because of that I tried squishing the isKVv2 call into something like the splitPaths function before, but that failed me when it came to some of the other KV commands, which require fullPath to come from a different kind of logic. (Fun fact: There's more than just /data and /metadata.... see KV Delete, Destroy, and Undelete!)

I think I will go with what I've got for now and if something flexible suddenly comes to mind we can make a new PR for that.


var (
mountPath string
partialPath string
v2 bool
)

// Parse the paths and grab the KV version
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)
if err != nil {
c.UI.Error(err.Error())
return 2
}
} else {
// In this case, this arg is a path-like combination of mountPath/secretPath.
// (e.g. "secret/foo")
partialPath = sanitizePath(args[0])
mountPath, v2, err = isKVv2(partialPath, client)
if err != nil {
c.UI.Error(err.Error())
return 2
}
}

var secret *api.Secret
var fullPath string
if v2 {
secret, err = c.deleteV2(path, mountPath, client)
secret, err = c.deleteV2(partialPath, mountPath, client)
fullPath = addPrefixToKVPath(partialPath, mountPath, "data")
} else {
secret, err = client.Logical().Delete(path)
// v1
if mountFlagSyntax {
fullPath = path.Join(mountPath, partialPath)
} else {
fullPath = partialPath
}
secret, err = client.Logical().Delete(fullPath)
}

if err != nil {
c.UI.Error(fmt.Sprintf("Error deleting %s: %s", path, err))
c.UI.Error(fmt.Sprintf("Error deleting %s: %s", fullPath, err))
if secret != nil {
OutputSecret(c.UI, secret)
}
Expand All @@ -121,7 +172,7 @@ func (c *KVDeleteCommand) Run(args []string) int {
if secret == nil {
// Don't output anything unless using the "table" format
if Format(c.UI) == "table" {
c.UI.Info(fmt.Sprintf("Success! Data deleted (if it existed) at: %s", path))
c.UI.Info(fmt.Sprintf("Success! Data deleted (if it existed) at: %s", fullPath))
}
return 0
}
Expand All @@ -139,22 +190,12 @@ func (c *KVDeleteCommand) deleteV2(path, mountPath string, client *api.Client) (
switch {
case len(c.flagVersions) > 0:
path = addPrefixToKVPath(path, mountPath, "delete")
if err != nil {
return nil, err
}

data := map[string]interface{}{
"versions": kvParseVersionsFlags(c.flagVersions),
}

secret, err = client.Logical().Write(path, data)
default:

path = addPrefixToKVPath(path, mountPath, "data")
if err != nil {
return nil, err
}

secret, err = client.Logical().Delete(path)
}

Expand Down
62 changes: 53 additions & 9 deletions command/kv_destroy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type KVDestroyCommand struct {
*BaseCommand

flagVersions []string
flagMount string
}

func (c *KVDestroyCommand) Synopsis() string {
Expand All @@ -32,6 +33,12 @@ Usage: vault kv destroy [options] KEY

To destroy version 3 of key foo:

$ vault kv destroy -mount=secret -versions=3 foo

The deprecated path-like syntax can also be used, but this should be avoided
for KV v2, as the fact that it is not actually the full API path to
the secret (secret/data/foo) can cause confusion:

$ vault kv destroy -versions=3 secret/foo

Additional flags and more advanced use cases are detailed below.
Expand All @@ -53,6 +60,17 @@ func (c *KVDestroyCommand) Flags() *FlagSets {
Usage: `Specifies the version numbers to destroy.`,
})

f.StringVar(&StringVar{
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
v2 secrets.`,
})

return set
}

Expand Down Expand Up @@ -86,25 +104,51 @@ func (c *KVDestroyCommand) Run(args []string) int {
c.UI.Error("No versions provided, use the \"-versions\" flag to specify the version to destroy.")
return 1
}

var err error
path := sanitizePath(args[0])

client, err := c.Client()
if err != nil {
c.UI.Error(err.Error())
return 2
}

mountPath, v2, err := isKVv2(path, client)
if err != nil {
c.UI.Error(err.Error())
return 2
// If true, we're working with "-mount=secret foo" syntax.
// If false, we're using "secret/foo" syntax.
mountFlagSyntax := (c.flagMount != "")

var (
mountPath string
partialPath string
v2 bool
)

// Parse the paths and grab the KV version
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)
if err != nil {
c.UI.Error(err.Error())
return 2
}
} else {
// In this case, this arg is a path-like combination of mountPath/secretPath.
// (e.g. "secret/foo")
partialPath = sanitizePath(args[0])
mountPath, v2, err = isKVv2(partialPath, client)
if err != nil {
c.UI.Error(err.Error())
return 2
}
}

if !v2 {
c.UI.Error("Destroy not supported on KV Version 1")
return 1
}
path = addPrefixToKVPath(path, mountPath, "destroy")
destroyPath := addPrefixToKVPath(partialPath, mountPath, "destroy")
if err != nil {
c.UI.Error(err.Error())
return 2
Expand All @@ -114,9 +158,9 @@ func (c *KVDestroyCommand) Run(args []string) int {
"versions": kvParseVersionsFlags(c.flagVersions),
}

secret, err := client.Logical().Write(path, data)
secret, err := client.Logical().Write(destroyPath, data)
if err != nil {
c.UI.Error(fmt.Sprintf("Error writing data to %s: %s", path, err))
c.UI.Error(fmt.Sprintf("Error writing data to %s: %s", destroyPath, err))
if secret != nil {
OutputSecret(c.UI, secret)
}
Expand All @@ -125,7 +169,7 @@ func (c *KVDestroyCommand) Run(args []string) int {
if secret == nil {
// Don't output anything unless using the "table" format
if Format(c.UI) == "table" {
c.UI.Info(fmt.Sprintf("Success! Data written to: %s", path))
c.UI.Info(fmt.Sprintf("Success! Data written to: %s", destroyPath))
}
return 0
}
Expand Down
Loading