diff --git a/.semgrep/rpc_endpoint.yml b/.semgrep/rpc_endpoint.yml index 8d64dc5a87d..5080dade719 100644 --- a/.semgrep/rpc_endpoint.yml +++ b/.semgrep/rpc_endpoint.yml @@ -112,13 +112,9 @@ rules: # authorization, as nil ACLs are always programmer errors. - id: "rpc-authz-bypass" patterns: - # Pattern that will accidentally bypass authorization checks. + # Pattern that may accidentally bypass authorization checks. - pattern: | - ... - if aclObj != nil && aclObj.$ACL_CHECK(...) { - ... - } - ... + aclObj == nil message: "RPC method ACL check $ACL_CHECK appears to bypass authorization by first checking for nil ACLs" languages: diff --git a/nomad/csi_endpoint.go b/nomad/csi_endpoint.go index 26cfa9432a2..c51d2a3120d 100644 --- a/nomad/csi_endpoint.go +++ b/nomad/csi_endpoint.go @@ -1795,8 +1795,7 @@ func (v *CSIPlugin) Get(args *structs.CSIPluginGetRequest, reply *structs.CSIPlu return structs.ErrPermissionDenied } - withAllocs := aclObj == nil || - aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) + withAllocs := aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) if args.ID == "" { return fmt.Errorf("missing plugin ID") diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 8e3e80f7791..59c40b2e6c4 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -1367,7 +1367,7 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, // Returns `nil` set if the token has access to all namespaces // and ErrPermissionDenied if the token has no capabilities on any namespace. func allowedNSes(aclObj *acl.ACL, state *state.StateStore, allow func(ns string) bool) (map[string]bool, error) { - if aclObj == nil || aclObj.IsManagement() { + if aclObj.IsManagement() { return nil, nil } diff --git a/nomad/namespace_endpoint.go b/nomad/namespace_endpoint.go index 6e1268faa2c..9061107e66a 100644 --- a/nomad/namespace_endpoint.go +++ b/nomad/namespace_endpoint.go @@ -276,7 +276,7 @@ func (n *Namespace) ListNamespaces(args *structs.NamespaceListRequest, reply *st ns := raw.(*structs.Namespace) // Only return namespaces allowed by acl - if aclObj == nil || aclObj.AllowNamespace(ns.Name) { + if aclObj.AllowNamespace(ns.Name) { reply.Namespaces = append(reply.Namespaces, ns) } } diff --git a/nomad/operator_endpoint.go b/nomad/operator_endpoint.go index 3524534549a..ae9b0af71bf 100644 --- a/nomad/operator_endpoint.go +++ b/nomad/operator_endpoint.go @@ -369,8 +369,7 @@ func (op *Operator) AutopilotGetConfiguration(args *structs.GenericRequest, repl aclObj, err := op.srv.ResolveACL(args) if err != nil { return err - } - if aclObj != nil && !aclObj.AllowOperatorRead() { + } else if !aclObj.AllowOperatorRead() { return structs.ErrPermissionDenied } @@ -404,8 +403,7 @@ func (op *Operator) AutopilotSetConfiguration(args *structs.AutopilotSetConfigRe aclObj, err := op.srv.ResolveACL(args) if err != nil { return err - } - if aclObj != nil && !aclObj.AllowOperatorWrite() { + } else if !aclObj.AllowOperatorWrite() { return structs.ErrPermissionDenied } @@ -447,8 +445,7 @@ func (op *Operator) ServerHealth(args *structs.GenericRequest, reply *structs.Op aclObj, err := op.srv.ResolveACL(args) if err != nil { return err - } - if aclObj != nil && !aclObj.AllowOperatorRead() { + } else if !aclObj.AllowOperatorRead() { return structs.ErrPermissionDenied } @@ -482,7 +479,7 @@ func (op *Operator) SchedulerSetConfiguration(args *structs.SchedulerSetConfigRe aclObj, err := op.srv.ResolveACL(args) if err != nil { return err - } else if aclObj != nil && !aclObj.AllowOperatorWrite() { + } else if !aclObj.AllowOperatorWrite() { return structs.ErrPermissionDenied } @@ -536,7 +533,7 @@ func (op *Operator) SchedulerGetConfiguration(args *structs.GenericRequest, repl aclObj, err := op.srv.ResolveACL(args) if err != nil { return err - } else if aclObj != nil && !aclObj.AllowOperatorRead() { + } else if !aclObj.AllowOperatorRead() { return structs.ErrPermissionDenied } @@ -807,7 +804,7 @@ func (op *Operator) UpgradeCheckVaultWorkloadIdentity( aclObj, err := op.srv.ResolveACL(args) if err != nil { return err - } else if aclObj != nil && !aclObj.AllowOperatorRead() { + } else if !aclObj.AllowOperatorRead() { return structs.ErrPermissionDenied } diff --git a/nomad/search_endpoint.go b/nomad/search_endpoint.go index ef40d8431e2..4a66e939238 100644 --- a/nomad/search_endpoint.go +++ b/nomad/search_endpoint.go @@ -393,7 +393,7 @@ func getResourceIter(context structs.Context, aclObj *acl.ACL, namespace, prefix if err != nil { return nil, err } - if aclObj == nil || aclObj.IsManagement() { + if aclObj.IsManagement() { return iter, nil } return memdb.NewFilterIterator(iter, nodePoolCapFilter(aclObj)), nil @@ -410,18 +410,12 @@ func getResourceIter(context structs.Context, aclObj *acl.ACL, namespace, prefix if err != nil { return nil, err } - if aclObj == nil { - return iter, nil - } return memdb.NewFilterIterator(iter, nsCapFilter(aclObj)), nil case structs.Variables: iter, err := store.GetVariablesByPrefix(ws, prefix) if err != nil { return nil, err } - if aclObj == nil { - return iter, nil - } return memdb.NewFilterIterator(iter, nsCapFilter(aclObj)), nil default: return getEnterpriseResourceIter(context, aclObj, namespace, prefix, ws, store) @@ -471,7 +465,7 @@ func getFuzzyResourceIterator(context structs.Context, aclObj *acl.ACL, namespac return nil, err } - if aclObj == nil || aclObj.IsManagement() { + if aclObj.IsManagement() { return iter, nil } return memdb.NewFilterIterator(iter, nodePoolCapFilter(aclObj)), nil @@ -499,9 +493,6 @@ func nsCapIterFilter(iter memdb.ResultIterator, err error, aclObj *acl.ACL) (mem if err != nil { return nil, err } - if aclObj == nil { - return iter, nil - } return memdb.NewFilterIterator(iter, nsCapFilter(aclObj)), nil } @@ -667,7 +658,7 @@ func (s *Search) PrefixSearch(args *structs.SearchRequest, reply *structs.Search // // Returns true if aclObj is nil or is for a management token func sufficientSearchPerms(aclObj *acl.ACL, namespace string, context structs.Context) bool { - if aclObj == nil || aclObj.IsManagement() { + if aclObj.IsManagement() { return true } @@ -893,10 +884,6 @@ func sufficientFuzzySearchPerms(aclObj *acl.ACL, namespace string, context struc func filteredSearchContexts(aclObj *acl.ACL, namespace string, context structs.Context) []structs.Context { desired := expandContext(context) - // If ACLs aren't enabled return all contexts - if aclObj == nil { - return desired - } if aclObj.IsManagement() { return desired } diff --git a/nomad/stream/event_broker.go b/nomad/stream/event_broker.go index db24e7e2c65..41d52f064e4 100644 --- a/nomad/stream/event_broker.go +++ b/nomad/stream/event_broker.go @@ -207,7 +207,7 @@ func (e *EventBroker) handleACLUpdates(ctx context.Context) { } aclObj, expiryTime, err := aclObjFromSnapshotForTokenSecretID(e.aclDelegate.TokenProvider(), e.aclCache, tokenSecretID) - if err != nil || aclObj == nil { + if err != nil { e.logger.Error("failed resolving ACL for secretID, closing subscriptions", "error", err) e.subscriptions.closeSubscriptionsForTokens([]string{tokenSecretID}) continue @@ -255,7 +255,7 @@ func (e *EventBroker) checkSubscriptionsAgainstACLChange() { } aclObj, expiryTime, err := aclObjFromSnapshotForTokenSecretID(aclSnapshot, e.aclCache, tokenSecretID) - if err != nil || aclObj == nil { + if err != nil { e.logger.Debug("failed resolving ACL for secretID, closing subscriptions", "error", err) e.subscriptions.closeSubscriptionsForTokens([]string{tokenSecretID}) continue