secure vars: filter by path in List RPCs (#14036)

The List RPCs only checked the ACL for the Prefix argument of the request. Add
an ACL filter to the paginator for the List RPC.

Extend test coverage of ACLs in the List RPC and in the `acl` package, and add a
"deny" capability so that operators can deny specific paths or prefixes below an
allowed path.

acl/acl_test.go

@@ -586,6 +586,31 @@ func TestSecureVariablesMatching(t *testing.T) {
 			op:    "read",
 			allow: false,
 		},
+		{
+			name: "wildcard with more specific denied path",
+			policy: `namespace "ns" {
+					secure_variables {
+					path "*" { capabilities = ["list"] }
+					path "system/*" { capabilities = ["deny"] }}}`,
+			ns:    "ns",
+			path:  "system/not-allowed",
+			op:    "list",
+			allow: false,
+		},
+		{
+			name: "multiple namespace with overlapping paths",
+			policy: `namespace "ns" {
+						secure_variables {
+  						path "*" { capabilities = ["list"] }
+						path "system/*" { capabilities = ["deny"] }}}
+					namespace "prod" {
+						secure_variables {
+						path "*" { capabilities = ["list"]}}}`,
+			ns:    "prod",
+			path:  "system/is-allowed",
+			op:    "list",
+			allow: true,
+		},
 	}
 
 	for _, tc := range tests {

acl/policy.go

@@ -75,6 +75,7 @@ const (
 	SecureVariablesCapabilityRead    = "read"
 	SecureVariablesCapabilityWrite   = "write"
 	SecureVariablesCapabilityDestroy = "destroy"
+	SecureVariablesCapabilityDeny    = "deny"
 )
 
 // Policy represents a parsed HCL or JSON policy.
@@ -187,7 +188,7 @@ func isNamespaceCapabilityValid(cap string) bool {
 func isPathCapabilityValid(cap string) bool {
 	switch cap {
 	case SecureVariablesCapabilityWrite, SecureVariablesCapabilityRead,
-		SecureVariablesCapabilityList, SecureVariablesCapabilityDestroy:
+		SecureVariablesCapabilityList, SecureVariablesCapabilityDestroy, SecureVariablesCapabilityDeny:
 		return true
 	default:
 		return false
@@ -269,6 +270,8 @@ func expandSecureVariablesCapabilities(caps []string) []string {
 	var foundRead, foundList bool
 	for _, cap := range caps {
 		switch cap {
+		case SecureVariablesCapabilityDeny:
+			return []string{SecureVariablesCapabilityDeny}
 		case SecureVariablesCapabilityRead:
 			foundRead = true
 		case SecureVariablesCapabilityList:

nomad/secure_variables_endpoint.go

@@ -210,7 +210,7 @@ func (sv *SecureVariables) Read(args *structs.SecureVariablesReadRequest, reply
 	}
 	defer metrics.MeasureSince([]string{"nomad", "secure_variables", "read"}, time.Now())
 
-	err := sv.handleMixedAuthEndpoint(args.QueryOptions,
+	_, err := sv.handleMixedAuthEndpoint(args.QueryOptions,
 		acl.PolicyRead, args.Path)
 	if err != nil {
 		return err
@@ -261,7 +261,7 @@ func (sv *SecureVariables) List(
 		return sv.listAllSecureVariables(args, reply)
 	}
 
-	err := sv.handleMixedAuthEndpoint(args.QueryOptions,
+	aclObj, err := sv.handleMixedAuthEndpoint(args.QueryOptions,
 		acl.PolicyList, args.Prefix)
 	if err != nil {
 		return err
@@ -291,13 +291,23 @@ func (sv *SecureVariables) List(
 				},
 			)
 
+			filters := []paginator.Filter{
+				paginator.GenericFilter{
+					Allow: func(raw interface{}) (bool, error) {
+						sv := raw.(*structs.SecureVariableEncrypted)
+						return strings.HasPrefix(sv.Path, args.Prefix) &&
+							(aclObj == nil || aclObj.AllowSecureVariableOperation(sv.Namespace, sv.Path, acl.PolicyList)), nil
+					},
+				},
+			}
+
 			// Set up our output after we have checked the error.
 			var svs []*structs.SecureVariableMetadata
 
 			// Build the paginator. This includes the function that is
 			// responsible for appending a variable to the secure variables
 			// stubs slice.
-			paginatorImpl, err := paginator.NewPaginator(iter, tokenizer, nil, args.QueryOptions,
+			paginatorImpl, err := paginator.NewPaginator(iter, tokenizer, filters, args.QueryOptions,
 				func(raw interface{}) error {
 					sv := raw.(*structs.SecureVariableEncrypted)
 					svStub := sv.SecureVariableMetadata
@@ -356,7 +366,7 @@ func (s *SecureVariables) listAllSecureVariables(
 			// Identify which namespaces the caller has access to. If they do
 			// not have access to any, send them an empty response. Otherwise,
 			// handle any error in a traditional manner.
-			allowedNSes, err := allowedNSes(aclObj, stateStore, allowFunc)
+			_, err := allowedNSes(aclObj, stateStore, allowFunc)
 			switch err {
 			case structs.ErrPermissionDenied:
 				reply.Data = make([]*structs.SecureVariableMetadata, 0)
@@ -384,24 +394,19 @@ func (s *SecureVariables) listAllSecureVariables(
 				},
 			)
 
-			// Wrap the SecureVariables iterator with a FilterIterator to
-			// eliminate invalid values before sending them to the paginator.
-			fltrIter := memdb.NewFilterIterator(iter, func(raw interface{}) bool {
-
-				// Values are filtered when the func returns true.
-				sv := raw.(*structs.SecureVariableEncrypted)
-				if allowedNSes != nil && !allowedNSes[sv.Namespace] {
-					return true
-				}
-				if !strings.HasPrefix(sv.Path, args.Prefix) {
-					return true
-				}
-				return false
-			})
+			filters := []paginator.Filter{
+				paginator.GenericFilter{
+					Allow: func(raw interface{}) (bool, error) {
+						sv := raw.(*structs.SecureVariableEncrypted)
+						return strings.HasPrefix(sv.Path, args.Prefix) &&
+							(aclObj == nil || aclObj.AllowSecureVariableOperation(sv.Namespace, sv.Path, acl.PolicyList)), nil
+					},
+				},
+			}
 
 			// Build the paginator. This includes the function that is
 			// responsible for appending a variable to the stubs array.
-			paginatorImpl, err := paginator.NewPaginator(fltrIter, tokenizer, nil, args.QueryOptions,
+			paginatorImpl, err := paginator.NewPaginator(iter, tokenizer, filters, args.QueryOptions,
 				func(raw interface{}) error {
 					sv := raw.(*structs.SecureVariableEncrypted)
 					svStub := sv.SecureVariableMetadata
@@ -465,7 +470,7 @@ func (sv *SecureVariables) decrypt(v *structs.SecureVariableEncrypted) (*structs
 
 // handleMixedAuthEndpoint is a helper to handle auth on RPC endpoints that can
 // either be called by external clients or by workload identity
-func (sv *SecureVariables) handleMixedAuthEndpoint(args structs.QueryOptions, cap, pathOrPrefix string) error {
+func (sv *SecureVariables) handleMixedAuthEndpoint(args structs.QueryOptions, cap, pathOrPrefix string) (*acl.ACL, error) {
 
 	// Perform the initial token resolution.
 	aclObj, err := sv.srv.ResolveToken(args.AuthToken)
@@ -474,15 +479,15 @@ func (sv *SecureVariables) handleMixedAuthEndpoint(args structs.QueryOptions, ca
 		// are not enabled, otherwise trigger the allowed namespace function.
 		if aclObj != nil {
 			if !aclObj.AllowSecureVariableOperation(args.RequestNamespace(), pathOrPrefix, cap) {
-				return structs.ErrPermissionDenied
+				return nil, structs.ErrPermissionDenied
 			}
 		}
-		return nil
+		return aclObj, nil
 	}
 	if helper.IsUUID(args.AuthToken) {
 		// early return for ErrNotFound or other errors if it's formed
 		// like an ACLToken.SecretID
-		return err
+		return nil, err
 	}
 
 	// Attempt to verify the token as a JWT with a workload
@@ -492,27 +497,27 @@ func (sv *SecureVariables) handleMixedAuthEndpoint(args structs.QueryOptions, ca
 		metrics.IncrCounter([]string{
 			"nomad", "secure_variables", "invalid_allocation_identity"}, 1)
 		sv.logger.Trace("allocation identity was not valid", "error", err)
-		return structs.ErrPermissionDenied
+		return nil, structs.ErrPermissionDenied
 	}
 
 	// The workload identity gets access to paths that match its
 	// identity, without having to go thru the ACL system
 	err = sv.authValidatePrefix(claims, args.RequestNamespace(), pathOrPrefix)
 	if err == nil {
-		return nil
+		return aclObj, nil
 	}
 
 	// If the workload identity doesn't match the implicit permissions
 	// given to paths, check for its attached ACL policies
 	aclObj, err = sv.srv.ResolveClaims(claims)
 	if err != nil {
-		return err // this only returns an error when the state store has gone wrong
+		return nil, err // this only returns an error when the state store has gone wrong
 	}
 	if aclObj != nil && aclObj.AllowSecureVariableOperation(
 		args.RequestNamespace(), pathOrPrefix, cap) {
-		return nil
+		return aclObj, nil
 	}
-	return structs.ErrPermissionDenied
+	return nil, structs.ErrPermissionDenied
 }
 
 // authValidatePrefix asserts that the requested path is valid for

nomad/secure_variables_endpoint_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
+	"github.com/shoenig/test"
 	"github.com/shoenig/test/must"
 
 	"github.com/hashicorp/nomad/acl"
@@ -91,7 +92,7 @@ func TestSecureVariablesEndpoint_auth(t *testing.T) {
 	must.NoError(t, err)
 
 	t.Run("terminal alloc should be denied", func(t *testing.T) {
-		err = srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint(
+		_, err = srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint(
 			structs.QueryOptions{AuthToken: idToken, Namespace: ns}, "n/a",
 			fmt.Sprintf("nomad/jobs/%s/web/web", jobID))
 		must.EqError(t, err, structs.ErrPermissionDenied.Error())
@@ -103,7 +104,7 @@ func TestSecureVariablesEndpoint_auth(t *testing.T) {
 		structs.MsgTypeTestSetup, 1200, []*structs.Allocation{alloc1}))
 
 	t.Run("wrong namespace should be denied", func(t *testing.T) {
-		err = srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint(
+		_, err = srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint(
 			structs.QueryOptions{AuthToken: idToken, Namespace: structs.DefaultNamespace}, "n/a",
 			fmt.Sprintf("nomad/jobs/%s/web/web", jobID))
 		must.EqError(t, err, structs.ErrPermissionDenied.Error())
@@ -246,7 +247,7 @@ func TestSecureVariablesEndpoint_auth(t *testing.T) {
 	for _, tc := range testCases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
-			err := srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint(
+			_, err := srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint(
 				structs.QueryOptions{AuthToken: tc.token, Namespace: ns}, tc.cap, tc.path)
 			if tc.expectedErr == nil {
 				must.NoError(t, err)
@@ -438,3 +439,124 @@ func TestSecureVariablesEndpoint_Apply_ACL(t *testing.T) {
 		must.Equals(t, sv.Items, applyResp.Output.Items)
 	})
 }
+
+func TestSecureVariablesEndpoint_ComplexACLPolicies(t *testing.T) {
+
+	ci.Parallel(t)
+	srv, _, shutdown := TestACLServer(t, func(c *Config) {
+		c.NumSchedulers = 0 // Prevent automatic dequeue
+	})
+	defer shutdown()
+	testutil.WaitForLeader(t, srv.RPC)
+	codec := rpcClient(t, srv)
+
+	idx := uint64(1000)
+
+	policyRules := `
+namespace "dev" {
+  secure_variables {
+    path "*" { capabilities = ["list", "read"] }
+    path "system/*" { capabilities = ["deny"] }
+    path "config/system/*" { capabilities = ["deny"] }
+  }
+}
+
+namespace "prod" {
+  secure_variables {
+    path  "*" {
+    capabilities = ["list"]
+    }
+  }
+}
+
+namespace "*" {}
+`
+
+	store := srv.fsm.State()
+
+	must.NoError(t, store.UpsertNamespaces(1000, []*structs.Namespace{
+		{Name: "dev"}, {Name: "prod"}, {Name: "other"}}))
+
+	idx++
+	token := mock.CreatePolicyAndToken(t, store, idx, "developer", policyRules)
+
+	writeVar := func(ns, path string) {
+		idx++
+		sv := mock.SecureVariableEncrypted()
+		sv.Namespace = ns
+		sv.Path = path
+		resp := store.SVESet(idx, &structs.SVApplyStateRequest{
+			Op:  structs.SVOpSet,
+			Var: sv,
+		})
+		must.NoError(t, resp.Error)
+	}
+
+	writeVar("dev", "system/never-list")
+	writeVar("dev", "config/system/never-list")
+	writeVar("dev", "config/can-read")
+	writeVar("dev", "project/can-read")
+
+	writeVar("prod", "system/can-list")
+	writeVar("prod", "config/system/can-list")
+	writeVar("prod", "config/can-list")
+	writeVar("prod", "project/can-list")
+
+	writeVar("other", "system/never-list")
+	writeVar("other", "config/system/never-list")
+	writeVar("other", "config/never-list")
+	writeVar("other", "project/never-list")
+
+	testListPrefix := func(ns, prefix string, expectedCount int, expectErr error) {
+		t.Run(fmt.Sprintf("ns=%s-prefix=%s", ns, prefix), func(t *testing.T) {
+			req := &structs.SecureVariablesListRequest{
+				QueryOptions: structs.QueryOptions{
+					Namespace: ns,
+					Prefix:    prefix,
+					AuthToken: token.SecretID,
+					Region:    "global",
+				},
+			}
+			var resp structs.SecureVariablesListResponse
+
+			if expectErr != nil {
+				must.EqError(t,
+					msgpackrpc.CallWithCodec(codec, "SecureVariables.List", req, &resp),
+					expectErr.Error())
+				return
+			}
+			must.NoError(t, msgpackrpc.CallWithCodec(codec, "SecureVariables.List", req, &resp))
+
+			found := "found:\n"
+			for _, sv := range resp.Data {
+				found += fmt.Sprintf(" ns=%s path=%s\n", sv.Namespace, sv.Path)
+			}
+			must.Len(t, expectedCount, resp.Data, test.Sprintf("%s", found))
+		})
+	}
+
+	testListPrefix("dev", "system", 0, nil)
+	testListPrefix("dev", "config/system", 0, nil)
+	testListPrefix("dev", "config", 1, nil)
+	testListPrefix("dev", "project", 1, nil)
+	testListPrefix("dev", "", 2, nil)
+
+	testListPrefix("prod", "system", 1, nil)
+	testListPrefix("prod", "config/system", 1, nil)
+	testListPrefix("prod", "config", 2, nil)
+	testListPrefix("prod", "project", 1, nil)
+	testListPrefix("prod", "", 4, nil)
+
+	testListPrefix("other", "system", 0, structs.ErrPermissionDenied)
+	testListPrefix("other", "config/system", 0, structs.ErrPermissionDenied)
+	testListPrefix("other", "config", 0, structs.ErrPermissionDenied)
+	testListPrefix("other", "project", 0, structs.ErrPermissionDenied)
+	testListPrefix("other", "", 0, structs.ErrPermissionDenied)
+
+	testListPrefix("*", "system", 1, nil)
+	testListPrefix("*", "config/system", 1, nil)
+	testListPrefix("*", "config", 3, nil)
+	testListPrefix("*", "project", 2, nil)
+	testListPrefix("*", "", 6, nil)
+
+}

website/content/docs/other-specifications/acl-policy.mdx

@@ -202,6 +202,7 @@ Variables are as follows:
 | read       | Read the decrypted contents of Secure Variables at this path. Also includes the "list" capability                            |
 | list       | List the metadata but not contents of Secure Variables at this path.                                                         |
 | destroy    | Delete Secure Variables at this path.                                                                                        |
+| deny       | No permissions at this path. Deny takes precedence over other capabilities.                                                  |
 
 For example, the policy below allows full access to secure variables at all
 paths in the "dev" namespace that are prefixed with "project/", but only read