auth: use ACLsDisabledACL when ACLs are disabled (#18754)

The RPC handlers expect to see `nil` ACL objects whenever ACLs are disabled. By
using `nil` as a sentinel value, we have the risk of nil pointer exceptions and
improper handling of `nil` when returned from our various auth methods that can
lead to privilege escalation bugs. This is the final patch in a series to
eliminate the use of `nil` ACLs as a sentinel value for when ACLs are disabled.

This patch adds a new virtual ACL policy field for when ACLs are disabled and
updates our authentication logic to use it. Included:

* Extends auth package tests to demonstrate that nil ACLs are treated as failed
  auth and disabled ACLs succeed auth.
* Adds a new `AllowDebug` ACL check for the weird special casing we have for
  pprof debugging when ACLs are disabled.
* Removes the remaining unexported methods (and repeated tests) from the
  `nomad/acl.go` file.
* Update the semgrep rules to detect improper nil ACL checking and remove the
  old invalid ACL checks.
* Update the contributing guide for RPC authentication.

Ref: https://github.com/hashicorp/nomad-enterprise/pull/1218
Ref: https://github.com/hashicorp/nomad/pull/18703
Ref: https://github.com/hashicorp/nomad/pull/18715
Ref: https://github.com/hashicorp/nomad/pull/16799
Ref: https://github.com/hashicorp/nomad/pull/18730
Ref: https://github.com/hashicorp/nomad/pull/18744

acl/acl.go

@@ -78,9 +78,10 @@ type ACL struct {
 
 	// The attributes below detail a virtual policy that we never expose
 	// directly to the end user.
-	client   string
-	server   string
-	isLeader bool
+	client       string
+	server       string
+	isLeader     bool
+	aclsDisabled bool
 }
 
 // maxPrivilege returns the policy which grants the most privilege
@@ -98,6 +99,7 @@ func maxPrivilege(a, b string) string {
 	default:
 		return ""
 	}
+
 }
 
 // NewACL compiles a set of policies into an ACL object
@@ -305,6 +307,7 @@ func NewACL(management bool, policies []*Policy) (*ACL, error) {
 	acl.client = PolicyDeny
 	acl.server = PolicyDeny
 	acl.isLeader = false
+	acl.aclsDisabled = false
 
 	return acl, nil
 }
@@ -324,13 +327,12 @@ func (a *ACL) AllowNsOpFunc(ops ...string) func(string) bool {
 
 // AllowNamespaceOperation checks if a given operation is allowed for a namespace.
 func (a *ACL) AllowNamespaceOperation(ns string, op string) bool {
-	// Hot path if ACL is not enabled.
 	if a == nil {
-		return true
+		return false
 	}
 
-	// Hot path management tokens
-	if a.management {
+	// Hot path management tokens or when ACLs are disabled
+	if a.aclsDisabled || a.management {
 		return true
 	}
 
@@ -357,13 +359,12 @@ func (a *ACL) AllowNamespaceOperation(ns string, op string) bool {
 
 // AllowNamespace checks if any operations are allowed for a namespace
 func (a *ACL) AllowNamespace(ns string) bool {
-	// Hot path if ACL is not enabled.
 	if a == nil {
-		return true
+		return false
 	}
 
-	// Hot path management tokens
-	if a.management {
+	// Hot path management tokens or when ACLs are disabled
+	if a.aclsDisabled || a.management {
 		return true
 	}
 
@@ -390,8 +391,12 @@ func (a *ACL) AllowNamespace(ns string) bool {
 // AllowNodePoolOperation returns true if the given operation is allowed in the
 // node pool specified.
 func (a *ACL) AllowNodePoolOperation(pool string, op string) bool {
-	// Hot path if ACL is not enabled or if it's a management token.
-	if a == nil || a.management {
+	if a == nil {
+		return false
+	}
+
+	// Hot path management tokens or when ACLs are disabled
+	if a.aclsDisabled || a.management {
 		return true
 	}
 
@@ -407,8 +412,12 @@ func (a *ACL) AllowNodePoolOperation(pool string, op string) bool {
 
 // AllowNodePool returns true if any operation is allowed for the node pool.
 func (a *ACL) AllowNodePool(pool string) bool {
-	// Hot path if ACL is not enabled or if it's a management token.
-	if a == nil || a.management {
+	if a == nil {
+		return false
+	}
+
+	// Hot path management tokens or when ACLs are disabled
+	if a.aclsDisabled || a.management {
 		return true
 	}
 
@@ -431,8 +440,12 @@ func (a *ACL) AllowNodePool(pool string) bool {
 // This is a very loose check and is expected that callers perform more precise
 // verification later.
 func (a *ACL) AllowNodePoolSearch() bool {
-	// Hot path if ACL is not enabled or token is management.
-	if a == nil || a.management {
+	if a == nil {
+		return false
+	}
+
+	// Hot path management tokens or when ACLs are disabled
+	if a.aclsDisabled || a.management {
 		return true
 	}
 
@@ -456,8 +469,12 @@ func (a *ACL) AllowNodePoolSearch() bool {
 
 // AllowHostVolumeOperation checks if a given operation is allowed for a host volume
 func (a *ACL) AllowHostVolumeOperation(hv string, op string) bool {
-	// Hot path management tokens
-	if a.management {
+	if a == nil {
+		return false
+	}
+
+	// Hot path management tokens or when ACLs are disabled
+	if a.aclsDisabled || a.management {
 		return true
 	}
 
@@ -473,8 +490,12 @@ func (a *ACL) AllowHostVolumeOperation(hv string, op string) bool {
 
 // AllowHostVolume checks if any operations are allowed for a HostVolume
 func (a *ACL) AllowHostVolume(ns string) bool {
-	// Hot path management tokens
-	if a.management {
+	if a == nil {
+		return false
+	}
+
+	// Hot path management tokens or when ACLs are disabled
+	if a.aclsDisabled || a.management {
 		return true
 	}
 
@@ -494,11 +515,11 @@ func (a *ACL) AllowHostVolume(ns string) bool {
 
 func (a *ACL) AllowVariableOperation(ns, path, op string, claim *ACLClaim) bool {
 	if a == nil {
-		// ACL is nil only if ACLs are disabled
-		// TODO(tgross): return false when there are no nil ACLs
-		return true
+		return false
 	}
-	if a.management {
+
+	// Hot path management tokens or when ACLs are disabled
+	if a.aclsDisabled || a.management {
 		return true
 	}
 
@@ -523,13 +544,14 @@ type ACLClaim struct {
 // search result will be filtered by specific paths
 func (a *ACL) AllowVariableSearch(ns string) bool {
 	if a == nil {
-		// ACL is nil only if ACLs are disabled
-		// TODO(tgross): return false when there are no nil ACLs
-		return true
-	}
-	if a.management {
+		return false
+	}
+
+	// Hot path management tokens or when ACLs are disabled
+	if a.aclsDisabled || a.management {
 		return true
 	}
+
 	if ns == "*" {
 		return a.variables.Len() > 0 || a.wildcardVariables.Len() > 0
 	}
@@ -713,7 +735,9 @@ func findAllMatchingWildcards(radix *iradix.Tree[capabilitySet], name string) []
 // AllowAgentRead checks if read operations are allowed for an agent
 func (a *ACL) AllowAgentRead() bool {
 	switch {
-	case a.management:
+	case a == nil:
+		return false
+	case a.aclsDisabled, a.management:
 		return true
 	case a.agent == PolicyWrite:
 		return true
@@ -727,7 +751,9 @@ func (a *ACL) AllowAgentRead() bool {
 // AllowAgentWrite checks if write operations are allowed for an agent
 func (a *ACL) AllowAgentWrite() bool {
 	switch {
-	case a.management:
+	case a == nil:
+		return false
+	case a.aclsDisabled, a.management:
 		return true
 	case a.agent == PolicyWrite:
 		return true
@@ -736,13 +762,32 @@ func (a *ACL) AllowAgentWrite() bool {
 	}
 }
 
+// AllowAgentDebug checks if debug operations are allowed for an agent. This is
+// a special case of AllowAgentRead because we don't allow debug if ACLs are
+// disabled unless the debug flag is set in the agent config.
+func (a *ACL) AllowAgentDebug(isDebugEnabled bool) bool {
+	switch {
+	case a == nil:
+		return false
+	case a.management:
+		return true
+	case a.agent == PolicyWrite:
+		return true
+	case a.agent == PolicyRead:
+		return true
+	case a.aclsDisabled:
+		return isDebugEnabled
+	default:
+		return false
+	}
+}
+
 // AllowNodeRead checks if read operations are allowed for a node
 func (a *ACL) AllowNodeRead() bool {
 	switch {
-	// a is nil if ACLs are disabled.
 	case a == nil:
-		return true
-	case a.management:
+		return false
+	case a.aclsDisabled, a.management:
 		return true
 	case a.node == PolicyWrite:
 		return true
@@ -764,7 +809,9 @@ func (a *ACL) AllowNodeRead() bool {
 // AllowNodeWrite checks if write operations are allowed for a node
 func (a *ACL) AllowNodeWrite() bool {
 	switch {
-	case a.management:
+	case a == nil:
+		return false
+	case a.aclsDisabled, a.management:
 		return true
 	case a.node == PolicyWrite:
 		return true
@@ -776,7 +823,9 @@ func (a *ACL) AllowNodeWrite() bool {
 // AllowOperatorRead checks if read operations are allowed for a operator
 func (a *ACL) AllowOperatorRead() bool {
 	switch {
-	case a.management:
+	case a == nil:
+		return false
+	case a.aclsDisabled, a.management:
 		return true
 	case a.operator == PolicyWrite:
 		return true
@@ -790,7 +839,9 @@ func (a *ACL) AllowOperatorRead() bool {
 // AllowOperatorWrite checks if write operations are allowed for a operator
 func (a *ACL) AllowOperatorWrite() bool {
 	switch {
-	case a.management:
+	case a == nil:
+		return false
+	case a.aclsDisabled, a.management:
 		return true
 	case a.operator == PolicyWrite:
 		return true
@@ -802,7 +853,9 @@ func (a *ACL) AllowOperatorWrite() bool {
 // AllowQuotaRead checks if read operations are allowed for all quotas
 func (a *ACL) AllowQuotaRead() bool {
 	switch {
-	case a.management:
+	case a == nil:
+		return false
+	case a.aclsDisabled, a.management:
 		return true
 	case a.quota == PolicyWrite:
 		return true
@@ -816,7 +869,9 @@ func (a *ACL) AllowQuotaRead() bool {
 // AllowQuotaWrite checks if write operations are allowed for quotas
 func (a *ACL) AllowQuotaWrite() bool {
 	switch {
-	case a.management:
+	case a == nil:
+		return false
+	case a.aclsDisabled, a.management:
 		return true
 	case a.quota == PolicyWrite:
 		return true
@@ -828,10 +883,9 @@ func (a *ACL) AllowQuotaWrite() bool {
 // AllowPluginRead checks if read operations are allowed for all plugins
 func (a *ACL) AllowPluginRead() bool {
 	switch {
-	// ACL is nil only if ACLs are disabled
 	case a == nil:
-		return true
-	case a.management:
+		return false
+	case a.aclsDisabled, a.management:
 		return true
 	case a.client == PolicyRead,
 		a.client == PolicyWrite:
@@ -846,10 +900,9 @@ func (a *ACL) AllowPluginRead() bool {
 // AllowPluginList checks if list operations are allowed for all plugins
 func (a *ACL) AllowPluginList() bool {
 	switch {
-	// ACL is nil only if ACLs are disabled
 	case a == nil:
-		return true
-	case a.management:
+		return false
+	case a.aclsDisabled, a.management:
 		return true
 	case a.client == PolicyRead,
 		a.client == PolicyWrite:
@@ -864,9 +917,10 @@ func (a *ACL) AllowPluginList() bool {
 }
 
 func (a *ACL) AllowServiceRegistrationReadList(ns string, isWorkload bool) bool {
-	if a == nil {
-		// ACL is nil only if ACLs are disabled
-		// TODO(tgross): return false when there are no nil ACLs
+	switch {
+	case a == nil:
+		return false
+	case a.aclsDisabled, a.management:
 		return true
 	}
 	return isWorkload || a.AllowNsOp(ns, NamespaceCapabilityReadJob)
@@ -875,25 +929,24 @@ func (a *ACL) AllowServiceRegistrationReadList(ns string, isWorkload bool) bool
 // AllowServerOp checks if server-only operations are allowed
 func (a *ACL) AllowServerOp() bool {
 	if a == nil {
-		// ACL is nil only if ACLs are disabled
-		// TODO(tgross): return false when there are no nil ACLs
-		return true
+		return false
 	}
 	return a.server != PolicyDeny || a.isLeader
 }
 
 func (a *ACL) AllowClientOp() bool {
 	if a == nil {
-		// ACL is nil only if ACLs are disabled
-		// TODO(tgross): return false when there are no nil ACLs
-		return true
+		return false
 	}
 	return a.client != PolicyDeny
 }
 
 // IsManagement checks if this represents a management token
 func (a *ACL) IsManagement() bool {
-	return a.management
+	if a == nil {
+		return false
+	}
+	return a.management || a.aclsDisabled
 }
 
 // NamespaceValidator returns a func that wraps ACL.AllowNamespaceOperation in
@@ -901,10 +954,14 @@ func (a *ACL) IsManagement() bool {
 // *any* capabilities match.
 func NamespaceValidator(ops ...string) func(*ACL, string) bool {
 	return func(a *ACL, ns string) bool {
-		// Always allow if ACLs are disabled.
 		if a == nil {
+			return false
+		}
+		// Hot path for management tokens or when ACLs are disabled
+		if a.aclsDisabled || a.management {
 			return true
 		}
+
 		// Clients need to be able to read namespaced objects
 		if a.client != PolicyDeny {
 			return true

acl/acl_test.go

@@ -1030,3 +1030,68 @@ func TestACL_matchingCapabilitySet_difference(t *testing.T) {
 	}
 
 }
+
+func TestAgentDebug(t *testing.T) {
+	ci.Parallel(t)
+
+	testCases := []struct {
+		name           string
+		policy         string
+		aclsDisabled   bool
+		isDebugEnabled bool
+		expect         bool
+	}{
+		{
+			name:           "policy read debug not enabled",
+			policy:         `agent { policy = "read" }`,
+			isDebugEnabled: false,
+			expect:         true,
+		},
+		{
+			name:           "policy read debug enabled",
+			policy:         `agent { policy = "read" }`,
+			isDebugEnabled: true,
+			expect:         true,
+		},
+		{
+			name:           "policy no read debug enabled",
+			policy:         `node { policy = "read" }`,
+			isDebugEnabled: true,
+			expect:         false,
+		},
+		{
+			name:           "policy no read debug not enabled",
+			policy:         `node { policy = "read" }`,
+			isDebugEnabled: false,
+			expect:         false,
+		},
+		{
+			name:           "no acls debug enabled",
+			aclsDisabled:   true,
+			isDebugEnabled: true,
+			expect:         true,
+		},
+		{
+			name:           "no acls debug not enabled",
+			aclsDisabled:   true,
+			isDebugEnabled: false,
+			expect:         false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+
+			acl := ACLsDisabledACL
+			if !tc.aclsDisabled {
+				policy, err := Parse(tc.policy)
+				must.NoError(t, err)
+
+				acl, err = NewACL(false, []*Policy{policy})
+				must.NoError(t, err)
+			}
+
+			must.Eq(t, tc.expect, acl.AllowAgentDebug(tc.isDebugEnabled))
+		})
+	}
+}

acl/virtual.go

@@ -5,6 +5,7 @@ package acl
 
 var ClientACL = initClientACL()
 var ServerACL = initServerACL()
+var ACLsDisabledACL = initACLsDisabledACL()
 
 func initClientACL() *ACL {
 	aclObj, err := NewACL(false, []*Policy{})
@@ -26,3 +27,12 @@ func initServerACL() *ACL {
 	aclObj.server = PolicyWrite
 	return aclObj
 }
+
+func initACLsDisabledACL() *ACL {
+	aclObj, err := NewACL(false, []*Policy{})
+	if err != nil {
+		panic(err)
+	}
+	aclObj.aclsDisabled = true
+	return aclObj
+}