address various issues with the output-policy flag (#19160)

* update error message and properly handle list requests

* since we do agressive sanitizes we need to optionally check trailing slash

* added changelog record

* remove redundant path formating

* Update changelog/13106.txt

Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>

* addressed comments from review

* also remove code that duplicates efforts in kv_list

* abstracted helper func for testing

* added test cases for the policy builder

* updated the changelog to the correct one

* removed calls that apear not to do anything given test case results

* fixed spacing issue in output string

* remove const representation of list url param

* addressed comments for pr

---------

Co-authored-by: lursu <leland.ursu@hashicorp.com>
Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>

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
 	}

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)
 }

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)
+			}
+		})
+	}
+}

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/[^/]+$`),
 }
 

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.
+```

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)
 			}
 		}
 

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())

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))