api: refactor ACL check for namespace wildcard (#13606)

Improve how the all namespaces wildcard (`*`) is handled when checking
ACL permissions. When using the wildcard namespace the `AllowNsOp` would
return false since it looks for a namespace called `*` to match.

This commit changes this behavior to return `true` when the queried
namespace is `*` and the token allows the operation in _any_ namespace.

Actual permission must be checked per object. The helper function
`AllowNsOpFunc` returns a function that can be used to make this
verification.

acl/acl.go

@@ -9,6 +9,9 @@ import (
 	glob "github.com/ryanuber/go-glob"
 )
 
+// Redefine this value from structs to avoid circular dependency.
+const AllNamespacesSentinel = "*"
+
 // ManagementACL is a singleton used for management tokens
 var ManagementACL *ACL
 
@@ -215,13 +218,32 @@ func (a *ACL) AllowNsOp(ns string, op string) bool {
 	return a.AllowNamespaceOperation(ns, op)
 }
 
-// AllowNamespaceOperation checks if a given operation is allowed for a namespace
+// AllowNsOpFunc is a helper that returns a function that can be used to check
+// namespace permissions.
+func (a *ACL) AllowNsOpFunc(ops ...string) func(string) bool {
+	return func(ns string) bool {
+		return NamespaceValidator(ops...)(a, ns)
+	}
+}
+
+// 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
+	}
+
 	// Hot path management tokens
 	if a.management {
 		return true
 	}
 
+	// If using the all namespaces wildcard, allow if any namespace allows the
+	// operation.
+	if ns == AllNamespacesSentinel && a.anyNamespaceAllowsOp(op) {
+		return true
+	}
+
 	// Check for a matching capability set
 	capabilities, ok := a.matchingNamespaceCapabilitySet(ns)
 	if !ok {
@@ -234,11 +256,22 @@ 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
+	}
+
 	// Hot path management tokens
 	if a.management {
 		return true
 	}
 
+	// If using the all namespaces wildcard, allow if any namespace allows any
+	// operation.
+	if ns == AllNamespacesSentinel && a.anyNamespaceAllowsAnyOp() {
+		return true
+	}
+
 	// Check for a matching capability set
 	capabilities, ok := a.matchingNamespaceCapabilitySet(ns)
 	if !ok {
@@ -307,6 +340,42 @@ func (a *ACL) matchingNamespaceCapabilitySet(ns string) (capabilitySet, bool) {
 	return a.findClosestMatchingGlob(a.wildcardNamespaces, ns)
 }
 
+// anyNamespaceAllowsOp returns true if any namespace in ACL object allows the
+// given operation.
+func (a *ACL) anyNamespaceAllowsOp(op string) bool {
+	return a.anyNamespaceAllows(func(c capabilitySet) bool {
+		return c.Check(op)
+	})
+}
+
+// anyNamespaceAllowsAnyOp returns true if any namespace in ACL object allows
+// at least one operation.
+func (a *ACL) anyNamespaceAllowsAnyOp() bool {
+	return a.anyNamespaceAllows(func(c capabilitySet) bool {
+		return len(c) > 0 && !c.Check(PolicyDeny)
+	})
+}
+
+// anyNamespaceAllows returns true if the callback cb returns true for any
+// namespace operation of the ACL object.
+func (a *ACL) anyNamespaceAllows(cb func(capabilitySet) bool) bool {
+	allow := false
+
+	checkFn := func(_ []byte, iv interface{}) bool {
+		v := iv.(capabilitySet)
+		allow = cb(v)
+		return allow
+	}
+
+	a.namespaces.Root().Walk(checkFn)
+	if allow {
+		return true
+	}
+
+	a.wildcardNamespaces.Root().Walk(checkFn)
+	return allow
+}
+
 // matchingHostVolumeCapabilitySet looks for a capabilitySet that matches the host volume name,
 // if no concrete definitions are found, then we return the closest matching
 // glob.

acl/acl_test.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/hashicorp/nomad/ci"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestCapabilitySet(t *testing.T) {
@@ -234,42 +235,89 @@ func TestAllowNamespace(t *testing.T) {
 	ci.Parallel(t)
 
 	tests := []struct {
-		Policy string
-		Allow  bool
+		name      string
+		policy    string
+		allow     bool
+		namespace string
 	}{
 		{
-			Policy: `namespace "foo" {}`,
-			Allow:  false,
+			name:      "foo namespace - no capabilities",
+			policy:    `namespace "foo" {}`,
+			allow:     false,
+			namespace: "foo",
 		},
 		{
-			Policy: `namespace "foo" { policy = "deny" }`,
-			Allow:  false,
+			name:      "foo namespace - deny policy",
+			policy:    `namespace "foo" { policy = "deny" }`,
+			allow:     false,
+			namespace: "foo",
 		},
 		{
-			Policy: `namespace "foo" { capabilities = ["deny"] }`,
-			Allow:  false,
+			name:      "foo namespace - deny capability",
+			policy:    `namespace "foo" { capabilities = ["deny"] }`,
+			allow:     false,
+			namespace: "foo",
 		},
 		{
-			Policy: `namespace "foo" { capabilities = ["list-jobs"] }`,
-			Allow:  true,
+			name:      "foo namespace - with capability",
+			policy:    `namespace "foo" { capabilities = ["list-jobs"] }`,
+			allow:     true,
+			namespace: "foo",
 		},
 		{
-			Policy: `namespace "foo" { policy = "read" }`,
-			Allow:  true,
+			name:      "foo namespace - with policy",
+			policy:    `namespace "foo" { policy = "read" }`,
+			allow:     true,
+			namespace: "foo",
+		},
+		{
+			name:      "wildcard namespace - no capabilities",
+			policy:    `namespace "foo" {}`,
+			allow:     false,
+			namespace: "*",
+		},
+		{
+			name:      "wildcard namespace - deny policy",
+			policy:    `namespace "foo" { policy = "deny" }`,
+			allow:     false,
+			namespace: "*",
+		},
+		{
+			name:      "wildcard namespace - deny capability",
+			policy:    `namespace "foo" { capabilities = ["deny"] }`,
+			allow:     false,
+			namespace: "*",
+		},
+		{
+			name:      "wildcard namespace - with capability",
+			policy:    `namespace "foo" { capabilities = ["list-jobs"] }`,
+			allow:     true,
+			namespace: "*",
+		},
+		{
+			name:      "wildcard namespace - with policy",
+			policy:    `namespace "foo" { policy = "read" }`,
+			allow:     true,
+			namespace: "*",
+		},
+		{
+			name:      "wildcard namespace - no namespace rule",
+			policy:    `agent { policy = "read" }`,
+			allow:     false,
+			namespace: "*",
 		},
 	}
 
 	for _, tc := range tests {
-		t.Run(tc.Policy, func(t *testing.T) {
-			assert := assert.New(t)
-
-			policy, err := Parse(tc.Policy)
-			assert.Nil(err)
+		t.Run(tc.name, func(t *testing.T) {
+			policy, err := Parse(tc.policy)
+			require.NoError(t, err)
 
 			acl, err := NewACL(false, []*Policy{policy})
-			assert.Nil(err)
+			require.NoError(t, err)
 
-			assert.Equal(tc.Allow, acl.AllowNamespace("foo"))
+			got := acl.AllowNamespace(tc.namespace)
+			require.Equal(t, tc.allow, got)
 		})
 	}
 }
@@ -278,51 +326,71 @@ func TestWildcardNamespaceMatching(t *testing.T) {
 	ci.Parallel(t)
 
 	tests := []struct {
-		Policy string
-		Allow  bool
+		name      string
+		policy    string
+		allow     bool
+		namespace string
 	}{
-		{ // Wildcard matches
-			Policy: `namespace "prod-api-*" { policy = "write" }`,
-			Allow:  true,
+		{
+			name:      "wildcard matches",
+			policy:    `namespace "prod-api-*" { policy = "write" }`,
+			allow:     true,
+			namespace: "prod-api-services",
 		},
-		{ // Non globbed namespaces are not wildcards
-			Policy: `namespace "prod-api" { policy = "write" }`,
-			Allow:  false,
+		{
+			name:      "non globbed namespaces are not wildcards",
+			policy:    `namespace "prod-api" { policy = "write" }`,
+			allow:     false,
+			namespace: "prod-api-services",
 		},
-		{ // Concrete matches take precedence
-			Policy: `namespace "prod-api-services" { policy = "deny" }
+		{
+			name: "concrete matches take precedence",
+			policy: `namespace "prod-api-services" { policy = "deny" }
 			         namespace "prod-api-*" { policy = "write" }`,
-			Allow: false,
+			allow:     false,
+			namespace: "prod-api-services",
 		},
 		{
-			Policy: `namespace "prod-api-*" { policy = "deny" }
+			name: "glob match",
+			policy: `namespace "prod-api-*" { policy = "deny" }
 			         namespace "prod-api-services" { policy = "write" }`,
-			Allow: true,
-		},
-		{ // The closest character match wins
-			Policy: `namespace "*-api-services" { policy = "deny" }
-			         namespace "prod-api-*" { policy = "write" }`, // 4 vs 8 chars
-			Allow: false,
+			allow:     true,
+			namespace: "prod-api-services",
 		},
 		{
-			Policy: `namespace "prod-api-*" { policy = "write" }
+			name: "closest character match wins - suffix",
+			policy: `namespace "*-api-services" { policy = "deny" }
+			         namespace "prod-api-*" { policy = "write" }`, // 4 vs 8 chars
+			allow:     false,
+			namespace: "prod-api-services",
+		},
+		{
+			name: "closest character match wins - prefix",
+			policy: `namespace "prod-api-*" { policy = "write" }
                namespace "*-api-services" { policy = "deny" }`, // 4 vs 8 chars
-			Allow: false,
+			allow:     false,
+			namespace: "prod-api-services",
+		},
+		{
+			name: "wildcard namespace with glob match",
+			policy: `namespace "prod-api-*" { policy = "deny" }
+			         namespace "prod-api-services" { policy = "write" }`,
+			allow:     true,
+			namespace: "*",
 		},
 	}
 
 	for _, tc := range tests {
-		t.Run(tc.Policy, func(t *testing.T) {
-			assert := assert.New(t)
-
-			policy, err := Parse(tc.Policy)
-			assert.NoError(err)
-			assert.NotNil(policy.Namespaces)
+		t.Run(tc.name, func(t *testing.T) {
+			policy, err := Parse(tc.policy)
+			require.NoError(t, err)
+			require.NotNil(t, policy.Namespaces)
 
 			acl, err := NewACL(false, []*Policy{policy})
-			assert.Nil(err)
+			require.NoError(t, err)
 
-			assert.Equal(tc.Allow, acl.AllowNamespace("prod-api-services"))
+			got := acl.AllowNamespace(tc.namespace)
+			require.Equal(t, tc.allow, got)
 		})
 	}
 }