Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: refactor ACL check for namespace wildcard #13606

Merged
merged 6 commits into from
Jul 6, 2022
Merged

api: refactor ACL check for namespace wildcard #13606

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0a050f7
api: refactor ACL check for namespace wildcard
lgfa29 Jul 6, 2022
0b9ca17
changelog: add entry for #13606
lgfa29 Jul 6, 2022
a7a06b8
api: refactor anyNamespaceAllows method
lgfa29 Jul 6, 2022
de40f4d
changelog: remove unnecessary entry
lgfa29 Jul 6, 2022
00ec288
fix linter and simplify anyNamespaceAllows
lgfa29 Jul 6, 2022
b092818
fix namespace acl check logic
lgfa29 Jul 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/13606.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
api: refactor ACL check when using the all namespaces wildcard
```
lgfa29 marked this conversation as resolved.
Show resolved Hide resolved
63 changes: 62 additions & 1 deletion acl/acl.go
View file
Open in desktop
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.anyNamespaceAllows(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.anyNamespaceAllows("") {
return true
}

// Check for a matching capability set
capabilities, ok := a.matchingNamespaceCapabilitySet(ns)
if !ok {
Expand Down Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like passing an empty op is a programmer error, not a condition we intentionally support. Should this return ErrPermissionDenied so that we can catch that in testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I didn't like this very much but figured it was OK given it's a private method. The key is that it needs to differentiate between two scenarios:

  • AllowNamespaceOperation needs to know if any namespace allows a specific operation
  • AllowNamespace needs to know if any namespace allows any operation (i.e., all don't deny)

Having a "magic" input (empty string in this case) serves as that switch between the two cases.

But looking at the code I found a better way to do it, so thanks for raising it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New approach in commit a7a06b8

func (a *ACL) anyNamespaceAllows(op string) bool {
allow := false

checkFn := func(_ []byte, iv interface{}) bool {
v := iv.(capabilitySet)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we not need to check this is okay? if we can statically assert, shouldn't the checkFn parameter be a capabilitySet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkFn needs to be a WalkFn, so the signature is pretty set.

This iterates over the internal tree of namespaces, if there's data that is not a capabilitySet then something pretty wrong happened 😅

findAllMatchingWildcards doesn't have the check either, so I think it's safe to assume this would always work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see, yup just ignore me!


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.
Expand Down
158 changes: 113 additions & 45 deletions acl/acl_test.go
View file
Open in desktop
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
Open in desktop
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