acl: remove remaining unused nil ACL object handling (#20456)

As of #18754 which shipped in Nomad 1.7, we no longer need to nil-check the object returned by ResolveACL if there's no error return, because in the case where ACLs are disabled we return a special "ACLs disabled" ACL object. Checking nil is not a bug but should be discouraged because it opens us up to future bugs that would bypass ACLs. We fixed a bunch of these cases in #20150 but I didn't update the semgrep rule, which meant we missed a few more. Update the semgrep rule and fix the remaining cases.
hashicorp · Apr 18, 2024 · ea5f2f6 · ea5f2f6
1 parent 048f451
commit ea5f2f6
Show file tree

Hide file tree

Showing 7 changed files with 16 additions and 37 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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