Skip to content

Commit

Permalink
api: refactor ACL check for namespace wildcard (#13606)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@lgfa29
lgfa29 authored Jul 6, 2022
1 parent e193217 commit 9278730
Show file tree
Hide file tree
Showing 3 changed files with 186 additions and 46 deletions.
71 changes: 70 additions & 1 deletion acl/acl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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.
Expand Down
158 changes: 113 additions & 45 deletions acl/acl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
})
}
}
Expand All @@ -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)
})
}
}
Expand Down
3 changes: 3 additions & 0 deletions nomad/structs/structs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ const (

// AllNamespacesSentinel is the value used as a namespace RPC value
// to indicate that endpoints must search in all namespaces
//
// Also defined in acl/acl.go to avoid circular dependencies. If modified
// it should be updated there as well.
AllNamespacesSentinel = "*"

// maxNamespaceDescriptionLength limits a namespace description length
Expand Down

0 comments on commit 9278730

Please sign in to comment.