From ed55b97f30ca2c8297f91e31fa976ec24c4d9641 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Mon, 15 Aug 2022 11:38:20 -0400 Subject: [PATCH] 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 | 25 ++++ acl/policy.go | 5 +- nomad/secure_variables_endpoint.go | 61 +++++---- nomad/secure_variables_endpoint_test.go | 128 +++++++++++++++++- .../docs/other-specifications/acl-policy.mdx | 1 + 5 files changed, 188 insertions(+), 32 deletions(-) diff --git a/acl/acl_test.go b/acl/acl_test.go index 57c3cfa2b..51c1a6189 100644 --- a/acl/acl_test.go +++ b/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 { diff --git a/acl/policy.go b/acl/policy.go index d74df4437..d52d4ee15 100644 --- a/acl/policy.go +++ b/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: diff --git a/nomad/secure_variables_endpoint.go b/nomad/secure_variables_endpoint.go index d68238c3c..c1b15446b 100644 --- a/nomad/secure_variables_endpoint.go +++ b/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 diff --git a/nomad/secure_variables_endpoint_test.go b/nomad/secure_variables_endpoint_test.go index 15b2a1db2..58aafd892 100644 --- a/nomad/secure_variables_endpoint_test.go +++ b/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) + +} diff --git a/website/content/docs/other-specifications/acl-policy.mdx b/website/content/docs/other-specifications/acl-policy.mdx index 3375e005b..0e052be2a 100644 --- a/website/content/docs/other-specifications/acl-policy.mdx +++ b/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