From f1f684400fd1911aa9f19dc906e29079701a19ca Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 18 Oct 2022 16:43:59 -0400 Subject: [PATCH] variables: fix filter on List RPC The List RPC correctly authorized against the prefix argument. But when filtering results underneath the prefix, it only checked authorization for standard ACL tokens and not Workload Identity. This results in WI tokens being able to read List results (metadata only: variable paths and timestamps) for variables under the `nomad/` prefix that belong to other jobs in the same namespace. Fixes the filtering and split the `handleMixedAuthEndpoint` function into separate authentication and authorization steps so that we don't need to re-verify the claim token on each filtered object. Also includes: * update semgrep rule for mixed auth endpoints * variables: List returns empty set when all results are filtered --- .changelog/15012.txt | 7 + .semgrep/rpc_endpoint.yml | 9 ++ nomad/variables_endpoint.go | 136 +++++++++--------- nomad/variables_endpoint_test.go | 234 ++++++++++++++++++++++++++----- 4 files changed, 288 insertions(+), 98 deletions(-) create mode 100644 .changelog/15012.txt diff --git a/.changelog/15012.txt b/.changelog/15012.txt new file mode 100644 index 000000000..5010d2dd2 --- /dev/null +++ b/.changelog/15012.txt @@ -0,0 +1,7 @@ +```security +variables: Fixed a bug where non-sensitive variable metadata (paths and raft indexes) was exposed via the template `nomadVarList` function to other jobs in the same namespace. +``` + +```bug +variables: Fixed a bug where getting empty results from listing variables resulted in a permission denied error. +``` diff --git a/.semgrep/rpc_endpoint.yml b/.semgrep/rpc_endpoint.yml index 3100fa6fb..f32b327e7 100644 --- a/.semgrep/rpc_endpoint.yml +++ b/.semgrep/rpc_endpoint.yml @@ -45,6 +45,15 @@ rules: ... ... := $T.handleMixedAuthEndpoint(...) ... + # Pattern used by endpoints that support both normal ACLs and + # workload identity but break authentication and authorization up + - pattern-not-inside: | + if done, err := $A.$B.forward($METHOD, ...); done { + return err + } + ... + ... := $T.authorize(...) + ... # Pattern used by some Node endpoints. - pattern-not-inside: | if done, err := $A.$B.forward($METHOD, ...); done { diff --git a/nomad/variables_endpoint.go b/nomad/variables_endpoint.go index 5f159d05a..79f83b8af 100644 --- a/nomad/variables_endpoint.go +++ b/nomad/variables_endpoint.go @@ -218,7 +218,7 @@ func (sv *Variables) Read(args *structs.VariablesReadRequest, reply *structs.Var } defer metrics.MeasureSince([]string{"nomad", "variables", "read"}, time.Now()) - _, err := sv.handleMixedAuthEndpoint(args.QueryOptions, + _, _, err := sv.handleMixedAuthEndpoint(args.QueryOptions, acl.PolicyRead, args.Path) if err != nil { return err @@ -269,8 +269,7 @@ func (sv *Variables) List( return sv.listAllVariables(args, reply) } - aclObj, err := sv.handleMixedAuthEndpoint(args.QueryOptions, - acl.PolicyList, args.Prefix) + aclObj, claims, err := sv.authenticate(args.QueryOptions) if err != nil { return err } @@ -299,9 +298,12 @@ func (sv *Variables) List( filters := []paginator.Filter{ paginator.GenericFilter{ Allow: func(raw interface{}) (bool, error) { - sv := raw.(*structs.VariableEncrypted) - return strings.HasPrefix(sv.Path, args.Prefix) && - (aclObj == nil || aclObj.AllowVariableOperation(sv.Namespace, sv.Path, acl.PolicyList)), nil + v := raw.(*structs.VariableEncrypted) + if !strings.HasPrefix(v.Path, args.Prefix) { + return false, nil + } + err := sv.authorize(aclObj, claims, v.Namespace, acl.PolicyList, v.Path) + return err == nil, nil }, }, } @@ -345,43 +347,23 @@ func (sv *Variables) List( // listAllVariables is used to list variables held within // state where the caller has used the namespace wildcard identifier. -func (s *Variables) listAllVariables( +func (sv *Variables) listAllVariables( args *structs.VariablesListRequest, reply *structs.VariablesListResponse) error { // Perform token resolution. The request already goes through forwarding // and metrics setup before being called. - aclObj, err := s.srv.ResolveToken(args.AuthToken) + aclObj, claims, err := sv.authenticate(args.QueryOptions) if err != nil { return err } - // allowFunc checks whether the caller has the read-job capability on the - // passed namespace. - allowFunc := func(ns string) bool { - return aclObj.AllowVariableOperation(ns, "", acl.PolicyList) - } - // Set up and return the blocking query. - return s.srv.blockingRPC(&blockingOptions{ + return sv.srv.blockingRPC(&blockingOptions{ queryOpts: &args.QueryOptions, queryMeta: &reply.QueryMeta, run: func(ws memdb.WatchSet, stateStore *state.StateStore) error { - // 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. - _, err := allowedNSes(aclObj, stateStore, allowFunc) - switch err { - case structs.ErrPermissionDenied: - reply.Data = make([]*structs.VariableMetadata, 0) - return nil - case nil: - // Fallthrough. - default: - return err - } - // Get all the variables stored within state. iter, err := stateStore.Variables(ws) if err != nil { @@ -396,15 +378,17 @@ func (s *Variables) listAllVariables( paginator.StructsTokenizerOptions{ WithNamespace: true, WithID: true, - }, - ) + }) filters := []paginator.Filter{ paginator.GenericFilter{ Allow: func(raw interface{}) (bool, error) { - sv := raw.(*structs.VariableEncrypted) - return strings.HasPrefix(sv.Path, args.Prefix) && - (aclObj == nil || aclObj.AllowVariableOperation(sv.Namespace, sv.Path, acl.PolicyList)), nil + v := raw.(*structs.VariableEncrypted) + if !strings.HasPrefix(v.Path, args.Prefix) { + return false, nil + } + err := sv.authorize(aclObj, claims, v.Namespace, acl.PolicyList, v.Path) + return err == nil, nil }, }, } @@ -413,8 +397,8 @@ func (s *Variables) listAllVariables( // responsible for appending a variable to the stubs array. paginatorImpl, err := paginator.NewPaginator(iter, tokenizer, filters, args.QueryOptions, func(raw interface{}) error { - sv := raw.(*structs.VariableEncrypted) - svStub := sv.VariableMetadata + v := raw.(*structs.VariableEncrypted) + svStub := v.VariableMetadata svs = append(svs, &svStub) return nil }) @@ -437,7 +421,7 @@ func (s *Variables) listAllVariables( // Use the index table to populate the query meta as we have no way // of tracking the max index on deletes. - return s.srv.setReplyQueryMeta(stateStore, state.TableVariables, &reply.QueryMeta) + return sv.srv.setReplyQueryMeta(stateStore, state.TableVariables, &reply.QueryMeta) }, }) } @@ -475,24 +459,31 @@ func (sv *Variables) decrypt(v *structs.VariableEncrypted) (*structs.VariableDec // handleMixedAuthEndpoint is a helper to handle auth on RPC endpoints that can // either be called by external clients or by workload identity -func (sv *Variables) handleMixedAuthEndpoint(args structs.QueryOptions, cap, pathOrPrefix string) (*acl.ACL, error) { +func (sv *Variables) handleMixedAuthEndpoint(args structs.QueryOptions, cap, pathOrPrefix string) (*acl.ACL, *structs.IdentityClaims, error) { + + aclObj, claims, err := sv.authenticate(args) + if err != nil { + return aclObj, claims, err + } + err = sv.authorize(aclObj, claims, args.RequestNamespace(), cap, pathOrPrefix) + if err != nil { + return aclObj, claims, err + } + + return aclObj, claims, nil +} + +func (sv *Variables) authenticate(args structs.QueryOptions) (*acl.ACL, *structs.IdentityClaims, error) { // Perform the initial token resolution. aclObj, err := sv.srv.ResolveToken(args.AuthToken) if err == nil { - // Perform our ACL validation. If the object is nil, this means ACLs - // are not enabled, otherwise trigger the allowed namespace function. - if aclObj != nil { - if !aclObj.AllowVariableOperation(args.RequestNamespace(), pathOrPrefix, cap) { - return nil, structs.ErrPermissionDenied - } - } - return aclObj, nil + return aclObj, nil, nil } if helper.IsUUID(args.AuthToken) { // early return for ErrNotFound or other errors if it's formed // like an ACLToken.SecretID - return nil, err + return nil, nil, err } // Attempt to verify the token as a JWT with a workload @@ -502,27 +493,46 @@ func (sv *Variables) handleMixedAuthEndpoint(args structs.QueryOptions, cap, pat metrics.IncrCounter([]string{ "nomad", "variables", "invalid_allocation_identity"}, 1) sv.logger.Trace("allocation identity was not valid", "error", err) - return nil, structs.ErrPermissionDenied + return nil, nil, structs.ErrPermissionDenied + } + return nil, claims, nil +} + +func (sv *Variables) authorize(aclObj *acl.ACL, claims *structs.IdentityClaims, ns, cap, pathOrPrefix string) error { + + if aclObj == nil && claims == nil { + return nil // ACLs aren't enabled } - // 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 aclObj, nil + // Perform normal ACL validation. If the ACL object is nil, that means we're + // working with an identity claim. + if aclObj != nil { + if !aclObj.AllowVariableOperation(ns, pathOrPrefix, cap) { + return structs.ErrPermissionDenied + } + return 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 nil, err // this only returns an error when the state store has gone wrong + if claims != nil { + // The workload identity gets access to paths that match its + // identity, without having to go thru the ACL system + err := sv.authValidatePrefix(claims, ns, pathOrPrefix) + if err == nil { + return 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 + } + if aclObj != nil && aclObj.AllowVariableOperation( + ns, pathOrPrefix, cap) { + return nil + } } - if aclObj != nil && aclObj.AllowVariableOperation( - args.RequestNamespace(), pathOrPrefix, cap) { - return aclObj, nil - } - return nil, structs.ErrPermissionDenied + return structs.ErrPermissionDenied } // authValidatePrefix asserts that the requested path is valid for diff --git a/nomad/variables_endpoint_test.go b/nomad/variables_endpoint_test.go index a2c34e593..aa5989ab0 100644 --- a/nomad/variables_endpoint_test.go +++ b/nomad/variables_endpoint_test.go @@ -50,10 +50,15 @@ func TestVariablesEndpoint_auth(t *testing.T) { alloc3.Namespace = ns alloc3.Job.ParentID = jobID + alloc4 := mock.Alloc() + alloc4.ClientStatus = structs.AllocClientStatusRunning + alloc4.Job.Namespace = ns + alloc4.Namespace = ns + store := srv.fsm.State() must.NoError(t, store.UpsertNamespaces(1000, []*structs.Namespace{{Name: ns}})) must.NoError(t, store.UpsertAllocs( - structs.MsgTypeTestSetup, 1001, []*structs.Allocation{alloc1, alloc2, alloc3})) + structs.MsgTypeTestSetup, 1001, []*structs.Allocation{alloc1, alloc2, alloc3, alloc4})) claims1 := alloc1.ToTaskIdentityClaims(nil, "web") idToken, err := srv.encrypter.SignClaims(claims1) @@ -77,6 +82,10 @@ func TestVariablesEndpoint_auth(t *testing.T) { idTokenParts[2] = strings.Join(sig, "") invalidIDToken := strings.Join(idTokenParts, ".") + claims4 := alloc4.ToTaskIdentityClaims(alloc4.Job, "web") + wiOnlyToken, err := srv.encrypter.SignClaims(claims4) + must.NoError(t, err) + policy := mock.ACLPolicy() policy.Rules = `namespace "nondefault-namespace" { variables { @@ -98,8 +107,8 @@ func TestVariablesEndpoint_auth(t *testing.T) { must.NoError(t, err) t.Run("terminal alloc should be denied", func(t *testing.T) { - _, err = srv.staticEndpoints.Variables.handleMixedAuthEndpoint( - structs.QueryOptions{AuthToken: idToken, Namespace: ns}, "n/a", + _, _, err = srv.staticEndpoints.Variables.handleMixedAuthEndpoint( + structs.QueryOptions{AuthToken: idToken, Namespace: ns}, acl.PolicyList, fmt.Sprintf("nomad/jobs/%s/web/web", jobID)) must.EqError(t, err, structs.ErrPermissionDenied.Error()) }) @@ -110,8 +119,8 @@ func TestVariablesEndpoint_auth(t *testing.T) { structs.MsgTypeTestSetup, 1200, []*structs.Allocation{alloc1})) t.Run("wrong namespace should be denied", func(t *testing.T) { - _, err = srv.staticEndpoints.Variables.handleMixedAuthEndpoint( - structs.QueryOptions{AuthToken: idToken, Namespace: structs.DefaultNamespace}, "n/a", + _, _, err = srv.staticEndpoints.Variables.handleMixedAuthEndpoint( + structs.QueryOptions{AuthToken: idToken, Namespace: structs.DefaultNamespace}, acl.PolicyList, fmt.Sprintf("nomad/jobs/%s/web/web", jobID)) must.EqError(t, err, structs.ErrPermissionDenied.Error()) }) @@ -126,35 +135,35 @@ func TestVariablesEndpoint_auth(t *testing.T) { { name: "valid claim for path with task secret", token: idToken, - cap: "n/a", + cap: acl.PolicyRead, path: fmt.Sprintf("nomad/jobs/%s/web/web", jobID), expectedErr: nil, }, { name: "valid claim for path with group secret", token: idToken, - cap: "n/a", + cap: acl.PolicyRead, path: fmt.Sprintf("nomad/jobs/%s/web", jobID), expectedErr: nil, }, { name: "valid claim for path with job secret", token: idToken, - cap: "n/a", + cap: acl.PolicyRead, path: fmt.Sprintf("nomad/jobs/%s", jobID), expectedErr: nil, }, { name: "valid claim for path with dispatch job secret", token: idDispatchToken, - cap: "n/a", + cap: acl.PolicyRead, path: fmt.Sprintf("nomad/jobs/%s", jobID), expectedErr: nil, }, { name: "valid claim for path with namespace secret", token: idToken, - cap: "n/a", + cap: acl.PolicyRead, path: "nomad/jobs", expectedErr: nil, }, @@ -189,14 +198,14 @@ func TestVariablesEndpoint_auth(t *testing.T) { { name: "valid claim with no permissions denied by path", token: noPermissionsToken, - cap: "n/a", + cap: acl.PolicyList, path: fmt.Sprintf("nomad/jobs/%s/w", jobID), expectedErr: structs.ErrPermissionDenied, }, { name: "valid claim with no permissions allowed by namespace", token: noPermissionsToken, - cap: "n/a", + cap: acl.PolicyList, path: "nomad/jobs", expectedErr: nil, }, @@ -207,37 +216,23 @@ func TestVariablesEndpoint_auth(t *testing.T) { path: fmt.Sprintf("nomad/jobs/%s/w", jobID), expectedErr: structs.ErrPermissionDenied, }, - { - name: "extra trailing slash is denied", - token: idToken, - cap: "n/a", - path: fmt.Sprintf("nomad/jobs/%s/web/", jobID), - expectedErr: structs.ErrPermissionDenied, - }, - { - name: "invalid prefix is denied", - token: idToken, - cap: "n/a", - path: fmt.Sprintf("nomad/jobs/%s/w", jobID), - expectedErr: structs.ErrPermissionDenied, - }, { name: "missing auth token is denied", - cap: "n/a", + cap: acl.PolicyList, path: fmt.Sprintf("nomad/jobs/%s/web/web", jobID), expectedErr: structs.ErrPermissionDenied, }, { name: "invalid signature is denied", token: invalidIDToken, - cap: "n/a", + cap: acl.PolicyList, path: fmt.Sprintf("nomad/jobs/%s/web/web", jobID), expectedErr: structs.ErrPermissionDenied, }, { name: "invalid claim for dispatched ID", token: idDispatchToken, - cap: "n/a", + cap: acl.PolicyList, path: fmt.Sprintf("nomad/jobs/%s", alloc3.JobID), expectedErr: structs.ErrPermissionDenied, }, @@ -255,12 +250,106 @@ func TestVariablesEndpoint_auth(t *testing.T) { path: fmt.Sprintf("nomad/jobs/%s/web/web", jobID), expectedErr: structs.ErrPermissionDenied, }, + + { + name: "WI token can read own task", + token: wiOnlyToken, + cap: acl.PolicyRead, + path: fmt.Sprintf("nomad/jobs/%s/web/web", alloc4.JobID), + expectedErr: nil, + }, + { + name: "WI token can list own task", + token: wiOnlyToken, + cap: acl.PolicyList, + path: fmt.Sprintf("nomad/jobs/%s/web/web", alloc4.JobID), + expectedErr: nil, + }, + { + name: "WI token can read own group", + token: wiOnlyToken, + cap: acl.PolicyRead, + path: fmt.Sprintf("nomad/jobs/%s/web", alloc4.JobID), + expectedErr: nil, + }, + { + name: "WI token can list own group", + token: wiOnlyToken, + cap: acl.PolicyList, + path: fmt.Sprintf("nomad/jobs/%s/web", alloc4.JobID), + expectedErr: nil, + }, + + { + name: "WI token cannot read another task in group", + token: wiOnlyToken, + cap: acl.PolicyRead, + path: fmt.Sprintf("nomad/jobs/%s/web/other", alloc4.JobID), + expectedErr: structs.ErrPermissionDenied, + }, + { + name: "WI token cannot list another task in group", + token: wiOnlyToken, + cap: acl.PolicyList, + path: fmt.Sprintf("nomad/jobs/%s/web/other", alloc4.JobID), + expectedErr: structs.ErrPermissionDenied, + }, + { + name: "WI token cannot read another task in group", + token: wiOnlyToken, + cap: acl.PolicyRead, + path: fmt.Sprintf("nomad/jobs/%s/web/other", alloc4.JobID), + expectedErr: structs.ErrPermissionDenied, + }, + { + name: "WI token cannot list a task in another group", + token: wiOnlyToken, + cap: acl.PolicyRead, + path: fmt.Sprintf("nomad/jobs/%s/other/web", alloc4.JobID), + expectedErr: structs.ErrPermissionDenied, + }, + { + name: "WI token cannot read a task in another group", + token: wiOnlyToken, + cap: acl.PolicyRead, + path: fmt.Sprintf("nomad/jobs/%s/other/web", alloc4.JobID), + expectedErr: structs.ErrPermissionDenied, + }, + { + name: "WI token cannot read a group in another job", + token: wiOnlyToken, + cap: acl.PolicyRead, + path: "nomad/jobs/other/web/web", + expectedErr: structs.ErrPermissionDenied, + }, + { + name: "WI token cannot list a group in another job", + token: wiOnlyToken, + cap: acl.PolicyList, + path: "nomad/jobs/other/web/web", + expectedErr: structs.ErrPermissionDenied, + }, + + { + name: "WI token extra trailing slash is denied", + token: wiOnlyToken, + cap: acl.PolicyList, + path: fmt.Sprintf("nomad/jobs/%s/web/", alloc4.JobID), + expectedErr: structs.ErrPermissionDenied, + }, + { + name: "WI token invalid prefix is denied", + token: wiOnlyToken, + cap: acl.PolicyList, + path: fmt.Sprintf("nomad/jobs/%s/w", alloc4.JobID), + expectedErr: structs.ErrPermissionDenied, + }, } for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { - _, err := srv.staticEndpoints.Variables.handleMixedAuthEndpoint( + _, _, err := srv.staticEndpoints.Variables.handleMixedAuthEndpoint( structs.QueryOptions{AuthToken: tc.token, Namespace: ns}, tc.cap, tc.path) if tc.expectedErr == nil { must.NoError(t, err) @@ -453,6 +542,80 @@ func TestVariablesEndpoint_Apply_ACL(t *testing.T) { }) } +func TestVariablesEndpoint_ListFiltering(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) + + ns := "nondefault-namespace" + idx := uint64(1000) + + alloc := mock.Alloc() + alloc.Job.ID = "job1" + alloc.JobID = "job1" + alloc.TaskGroup = "group" + alloc.Job.TaskGroups[0].Name = "group" + alloc.ClientStatus = structs.AllocClientStatusRunning + alloc.Job.Namespace = ns + alloc.Namespace = ns + + store := srv.fsm.State() + must.NoError(t, store.UpsertNamespaces(idx, []*structs.Namespace{{Name: ns}})) + idx++ + must.NoError(t, store.UpsertAllocs( + structs.MsgTypeTestSetup, idx, []*structs.Allocation{alloc})) + + claims := alloc.ToTaskIdentityClaims(alloc.Job, "web") + token, err := srv.encrypter.SignClaims(claims) + must.NoError(t, err) + + writeVar := func(ns, path string) { + idx++ + sv := mock.VariableEncrypted() + sv.Namespace = ns + sv.Path = path + resp := store.VarSet(idx, &structs.VarApplyStateRequest{ + Op: structs.VarOpSet, + Var: sv, + }) + must.NoError(t, resp.Error) + } + + writeVar(ns, "nomad/jobs/job1/group/web") + writeVar(ns, "nomad/jobs/job1/group") + writeVar(ns, "nomad/jobs/job1") + + writeVar(ns, "nomad/jobs/job1/group/other") + writeVar(ns, "nomad/jobs/job1/other/web") + writeVar(ns, "nomad/jobs/job2/group/web") + + req := &structs.VariablesListRequest{ + QueryOptions: structs.QueryOptions{ + Namespace: ns, + Prefix: "nomad", + AuthToken: token, + Region: "global", + }, + } + var resp structs.VariablesListResponse + must.NoError(t, msgpackrpc.CallWithCodec(codec, "Variables.List", req, &resp)) + found := []string{} + for _, variable := range resp.Data { + found = append(found, variable.Path) + } + expect := []string{ + "nomad/jobs/job1", + "nomad/jobs/job1/group", + "nomad/jobs/job1/group/web", + } + must.Eq(t, expect, found) + +} + func TestVariablesEndpoint_ComplexACLPolicies(t *testing.T) { ci.Parallel(t) @@ -560,11 +723,12 @@ namespace "*" {} 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) + // list gives empty but no error! + testListPrefix("other", "system", 0, nil) + testListPrefix("other", "config/system", 0, nil) + testListPrefix("other", "config", 0, nil) + testListPrefix("other", "project", 0, nil) + testListPrefix("other", "", 0, nil) testListPrefix("*", "system", 1, nil) testListPrefix("*", "config/system", 1, nil)