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)