api: refactor ACL check for namespace wildcard

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.anyNamespaceAllows(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.anyNamespaceAllows("") {
+		return true
+	}
+
 	// Check for a matching capability set
 	capabilities, ok := a.matchingNamespaceCapabilitySet(ns)
 	if !ok {
@@ -307,6 +340,34 @@ func (a *ACL) matchingNamespaceCapabilitySet(ns string) (capabilitySet, bool) {
 	return a.findClosestMatchingGlob(a.wildcardNamespaces, ns)
 }
 
+// anyNamespaceAllows returns true if any namespace in the ACL object allows
+// the given operation.
+// If op is an empty string it returns true if any namespace allows any
+// operation.
+func (a *ACL) anyNamespaceAllows(op string) bool {
+	allow := false
+
+	checkFn := func(_ []byte, iv interface{}) bool {
+		v := iv.(capabilitySet)
+
+		allowAnyOp := op == "" && len(v) > 0 && !v.Check(PolicyDeny)
+		if allowAnyOp || v.Check(op) {
+			allow = true
+			return true
+		}
+
+		return false
+	}
+
+	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,
+			allow:     true,
+			namespace: "prod-api-services",
 		},
-		{ // The closest character match wins
-			Policy: `namespace "*-api-services" { policy = "deny" }
+		{
+			name: "closest character match wins - suffix",
+			policy: `namespace "*-api-services" { policy = "deny" }
 			         namespace "prod-api-*" { policy = "write" }`, // 4 vs 8 chars
-			Allow: false,
+			allow:     false,
+			namespace: "prod-api-services",
 		},
 		{
-			Policy: `namespace "prod-api-*" { policy = "write" }
+			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)
 		})
 	}
 }

nomad/alloc_endpoint.go

@@ -35,29 +35,16 @@ func (a *Alloc) List(args *structs.AllocListRequest, reply *structs.AllocListRes
 	defer metrics.MeasureSince([]string{"nomad", "alloc", "list"}, time.Now())
 
 	namespace := args.RequestNamespace()
-	var allow func(string) bool
 
 	// Check namespace read-job permissions
 	aclObj, err := a.srv.ResolveToken(args.AuthToken)
-
-	switch {
-	case err != nil:
+	if err != nil {
 		return err
-	case aclObj == nil:
-		allow = func(string) bool {
-			return true
-		}
-	case namespace == structs.AllNamespacesSentinel:
-		allow = func(ns string) bool {
-			return aclObj.AllowNsOp(ns, acl.NamespaceCapabilityReadJob)
-		}
-	case !aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityReadJob):
+	}
+	if !aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityReadJob) {
 		return structs.ErrPermissionDenied
-	default:
-		allow = func(string) bool {
-			return true
-		}
 	}
+	allow := aclObj.AllowNsOpFunc(acl.NamespaceCapabilityReadJob)
 
 	// Setup the blocking query
 	sort := state.SortOption(args.Reverse)

nomad/alloc_endpoint_test.go

@@ -1284,8 +1284,9 @@ func TestAllocEndpoint_List_AllNamespaces_ACL_OSS(t *testing.T) {
 		{
 			Label:     "all namespaces with insufficient token",
 			Namespace: "*",
-			Allocs:    []*structs.Allocation{},
 			Token:     ns1tokenInsufficient.SecretID,
+			Error:     true,
+			Message:   structs.ErrPermissionDenied.Error(),
 		},
 		{
 			Label:     "ns1 with ns1 token",