-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Conversation
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.
171ca50
to
0a050f7
Compare
In #13606 the ACL check was refactored to better support the all namespaces wildcard (`*`). This commit applies the changes to the jobs and alloc list endpoints.
0307600
to
0b9ca17
Compare
acl/acl.go
Outdated
// If op is an empty string it returns true if any namespace allows any | ||
// operation. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 operationAllowNamespace
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!
There was a problem hiding this comment.
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
Don't rely on magic input (`""`) change the behaviour of anyNamespaceAllows. Use two different methods instead with the core logic implemented in a shared method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
allow := false | ||
|
||
checkFn := func(_ []byte, iv interface{}) bool { | ||
v := iv.(capabilitySet) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
In #13606 the ACL check was refactored to better support the all namespaces wildcard (`*`). This commit applies the changes to the jobs and alloc list endpoints.
api: apply new ACL check for wildcard namespace In #13606 the ACL check was refactored to better support the all namespaces wildcard (`*`). This commit applies the changes to the jobs and alloc list endpoints.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Improve how the all namespaces wildcard (
*
) is handled when checkingACL permissions. When using the wildcard namespace the
AllowNsOp
wouldreturn false since it looks for a namespace called
*
to match.This commit changes this behavior to return
true
when the queriednamespace 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 thisverification.
#13608 applies this changes to the existing Job and Alloc list endpoints.
#13530 applies this changes to the Eval list endpoint to fix a bug.
The work has been split to allow for easier backporting.