diff --git a/api/client.go b/api/client.go index ef8a74f1a..e42808c89 100644 --- a/api/client.go +++ b/api/client.go @@ -1362,6 +1362,7 @@ START: LastOutputPolicyError = &OutputPolicyError{ method: req.Method, path: strings.TrimPrefix(req.URL.Path, "/v1"), + params: req.URL.Query(), } return nil, LastOutputPolicyError } diff --git a/api/output_policy.go b/api/output_policy.go index 85d1617e5..57545fafd 100644 --- a/api/output_policy.go +++ b/api/output_policy.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" "net/url" + "strconv" "strings" ) @@ -16,6 +17,7 @@ var LastOutputPolicyError *OutputPolicyError type OutputPolicyError struct { method string path string + params url.Values finalHCLString string } @@ -44,8 +46,22 @@ func (d *OutputPolicyError) HCLString() (string, error) { // Builds a sample policy document from the request func (d *OutputPolicyError) buildSamplePolicy() (string, error) { + operation := d.method + // List is often defined as a URL param instead of as an http.Method + // this will check for the header and properly switch off of the intended functionality + if d.params.Has("list") { + isList, err := strconv.ParseBool(d.params.Get("list")) + if err != nil { + return "", fmt.Errorf("the value of the list url param is not a bool: %v", err) + } + + if isList { + operation = "LIST" + } + } + var capabilities []string - switch d.method { + switch operation { case http.MethodGet, "": capabilities = append(capabilities, "read") case http.MethodPost, http.MethodPut: @@ -59,17 +75,15 @@ func (d *OutputPolicyError) buildSamplePolicy() (string, error) { capabilities = append(capabilities, "list") } - // sanitize, then trim the Vault address and v1 from the front of the path - path, err := url.PathUnescape(d.path) - if err != nil { - return "", fmt.Errorf("failed to unescape request URL characters: %v", err) - } - // determine whether to add sudo capability - if IsSudoPath(path) { + if IsSudoPath(d.path) { capabilities = append(capabilities, "sudo") } + return formatOutputPolicy(d.path, capabilities), nil +} + +func formatOutputPolicy(path string, capabilities []string) string { // the OpenAPI response has a / in front of each path, // but policies need the path without that leading slash path = strings.TrimLeft(path, "/") @@ -78,5 +92,5 @@ func (d *OutputPolicyError) buildSamplePolicy() (string, error) { return fmt.Sprintf( `path "%s" { capabilities = ["%s"] -}`, path, capStr), nil +}`, path, capStr) } diff --git a/api/output_policy_test.go b/api/output_policy_test.go new file mode 100644 index 000000000..8ea035603 --- /dev/null +++ b/api/output_policy_test.go @@ -0,0 +1,80 @@ +package api + +import ( + "net/http" + "net/url" + "testing" +) + +func TestBuildSamplePolicy(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + req *OutputPolicyError + expected string + err error + }{ + { + "happy path", + &OutputPolicyError{ + method: http.MethodGet, + path: "/something", + }, + formatOutputPolicy("/something", []string{"read"}), + nil, + }, + { // test included to clear up some confusion around the sanitize comment + "demonstrate that this function does not format fully", + &OutputPolicyError{ + method: http.MethodGet, + path: "http://vault.test/v1/something", + }, + formatOutputPolicy("http://vault.test/v1/something", []string{"read"}), + nil, + }, + { // test that list is properly returned + "list over read returned", + &OutputPolicyError{ + method: http.MethodGet, + path: "/something", + params: url.Values{ + "list": []string{"true"}, + }, + }, + formatOutputPolicy("/something", []string{"list"}), + nil, + }, + { + "valid protected path", + &OutputPolicyError{ + method: http.MethodGet, + path: "/sys/config/ui/headers/", + }, + formatOutputPolicy("/sys/config/ui/headers/", []string{"read", "sudo"}), + nil, + }, + { // ensure that a formatted path that trims the trailing slash as the code does still works for recognizing a sudo path + "valid protected path no trailing /", + &OutputPolicyError{ + method: http.MethodGet, + path: "/sys/config/ui/headers", + }, + formatOutputPolicy("/sys/config/ui/headers", []string{"read", "sudo"}), + nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result, err := tc.req.buildSamplePolicy() + if tc.err != err { + t.Fatalf("expected for the error to be %v instead got %v\n", tc.err, err) + } + + if tc.expected != result { + t.Fatalf("expected for the policy string to be %v instead got %v\n", tc.expected, result) + } + }) + } +} diff --git a/api/plugin_helpers.go b/api/plugin_helpers.go index 6602a044b..6dc9e0803 100644 --- a/api/plugin_helpers.go +++ b/api/plugin_helpers.go @@ -37,7 +37,7 @@ const ( // path matches that path or not (useful specifically for the paths that // contain templated fields.) var sudoPaths = map[string]*regexp.Regexp{ - "/auth/token/accessors/": regexp.MustCompile(`^/auth/token/accessors/$`), + "/auth/token/accessors/": regexp.MustCompile(`^/auth/token/accessors/?$`), "/pki/root": regexp.MustCompile(`^/pki/root$`), "/pki/root/sign-self-issued": regexp.MustCompile(`^/pki/root/sign-self-issued$`), "/sys/audit": regexp.MustCompile(`^/sys/audit$`), @@ -47,10 +47,10 @@ var sudoPaths = map[string]*regexp.Regexp{ "/sys/config/auditing/request-headers": regexp.MustCompile(`^/sys/config/auditing/request-headers$`), "/sys/config/auditing/request-headers/{header}": regexp.MustCompile(`^/sys/config/auditing/request-headers/.+$`), "/sys/config/cors": regexp.MustCompile(`^/sys/config/cors$`), - "/sys/config/ui/headers/": regexp.MustCompile(`^/sys/config/ui/headers/$`), + "/sys/config/ui/headers/": regexp.MustCompile(`^/sys/config/ui/headers/?$`), "/sys/config/ui/headers/{header}": regexp.MustCompile(`^/sys/config/ui/headers/.+$`), "/sys/leases": regexp.MustCompile(`^/sys/leases$`), - "/sys/leases/lookup/": regexp.MustCompile(`^/sys/leases/lookup/$`), + "/sys/leases/lookup/": regexp.MustCompile(`^/sys/leases/lookup/?$`), "/sys/leases/lookup/{prefix}": regexp.MustCompile(`^/sys/leases/lookup/.+$`), "/sys/leases/revoke-force/{prefix}": regexp.MustCompile(`^/sys/leases/revoke-force/.+$`), "/sys/leases/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/leases/revoke-prefix/.+$`), @@ -70,7 +70,7 @@ var sudoPaths = map[string]*regexp.Regexp{ "/sys/replication/performance/primary/secondary-token": regexp.MustCompile(`^/sys/replication/performance/primary/secondary-token$`), "/sys/replication/primary/secondary-token": regexp.MustCompile(`^/sys/replication/primary/secondary-token$`), "/sys/replication/reindex": regexp.MustCompile(`^/sys/replication/reindex$`), - "/sys/storage/raft/snapshot-auto/config/": regexp.MustCompile(`^/sys/storage/raft/snapshot-auto/config/$`), + "/sys/storage/raft/snapshot-auto/config/": regexp.MustCompile(`^/sys/storage/raft/snapshot-auto/config/?$`), "/sys/storage/raft/snapshot-auto/config/{name}": regexp.MustCompile(`^/sys/storage/raft/snapshot-auto/config/[^/]+$`), } diff --git a/changelog/19160.txt b/changelog/19160.txt new file mode 100644 index 000000000..66a3baa15 --- /dev/null +++ b/changelog/19160.txt @@ -0,0 +1,3 @@ +```release-note:bug +api: Addressed a couple of issues that arose as edge cases for the -output-policy flag. Specifically around properly handling list commands, distinguishing kv V1/V2, and correctly recognizing protected paths. +``` \ No newline at end of file diff --git a/command/kv_helpers.go b/command/kv_helpers.go index b362c3bb0..3a9dd786a 100644 --- a/command/kv_helpers.go +++ b/command/kv_helpers.go @@ -75,7 +75,9 @@ func kvPreflightVersionRequest(client *api.Client, path string) (string, int, er err = fmt.Errorf( `This output flag requires the success of a preflight request to determine the version of a KV secrets engine. Please -re-run this command with a token with read access to %s`, path) +re-run this command with a token with read access to %s. +Note that if the path you are trying to reach is a KV v2 path, your token's policy must +allow read access to that path in the format 'mount-path/data/foo', not just 'mount-path/foo'.`, path) } } diff --git a/command/kv_list.go b/command/kv_list.go index b6b665c6f..3afbcd42a 100644 --- a/command/kv_list.go +++ b/command/kv_list.go @@ -75,14 +75,8 @@ func (c *KVListCommand) Run(args []string) int { return 2 } - // Append trailing slash - path := args[0] - if !strings.HasSuffix(path, "/") { - path += "/" - } - // Sanitize path - path = sanitizePath(path) + path := sanitizePath(args[0]) mountPath, v2, err := isKVv2(path, client) if err != nil { c.UI.Error(err.Error()) diff --git a/command/list.go b/command/list.go index 9831b6633..8a79e56fc 100644 --- a/command/list.go +++ b/command/list.go @@ -78,13 +78,7 @@ func (c *ListCommand) Run(args []string) int { return 2 } - // Append trailing slash - path := args[0] - if !strings.HasSuffix(path, "/") { - path += "/" - } - - path = sanitizePath(path) + path := sanitizePath(args[0]) secret, err := client.Logical().List(path) if err != nil { c.UI.Error(fmt.Sprintf("Error listing %s: %s", path, err))