-
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
consul: correctly check consul acl token namespace when using consul oss #10720
Conversation
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.
Few questions; approach seems reasonable.
nomad/consul_policy.go
Outdated
case namespace == token.Namespace: | ||
// ACLs enabled, namespaces are the same | ||
return nil |
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.
I find it a bit easier if the common cases are grouped together, and special cases are together. I'd suggest moving this case to be the second.
// provide a more informative error message. | ||
return errors.Errorf("consul ACL token requires using namespace %q", token.Namespace) | ||
|
||
default: |
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.
I found it a tad harder to reason through conditions, maybe because of ordering. I would probably find it easier if the conditions were order the following way:
namespace == token.Namespace
: either ACL not enabled, or it's enabled and they matchtoken.Namespace == "default"
: special casetoken.Namespace != "default" && namespace == ""
<-token.Namespace != "default"
is redundant here but included for explicitnessdefault
: the mismatch case
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.
Thanks for this suggestion, this is easier to read 🙂
nomad/consul_policy.go
Outdated
// ACLs enabled, must defer to per-object checking, since the token could | ||
// have namespace_prefix blocks allowing the 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.
I'm not familiar with namespace_prefix. Is it only applicable if token's namespace is default? Why doesn't it apply in the default or token.Namespace != "default"
cases? Is a special case for handling legacy apps only? Might be useful to add more context.
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.
Yep, Consul token ACL policies can supply namespace
or namespace_prefix
blocks which give the token permission beyond the "default" namespace. Those blocks are only allowable in tokens in the "default" namespace.
https://www.consul.io/docs/security/acl/acl-rules#namespace-rules
Expanded on the comment to clarify.
This PR fixes the Nomad Object Namespace <-> Consul ACL Token relationship check when using Consul OSS (or Consul ENT without namespace support). Nomad v1.1.0 introduced a regression where Nomad would fail the validation when submitting Connect jobs and allow_unauthenticated set to true, with Consul OSS - because it would do the namespace check against the Consul ACL token assuming the "default" namespace, which does not work because Consul OSS does not have namespaces. Instead of making the bad assumption, expand the namespace check to handle each special case explicitly. Fixes #10718
@notnoop I applied your recommendations, thanks for the suggestion. I also fixed the failing tests, which then made me realize we need to split the tests between oss and ent - they were originally written around the same bad assumption that caused this bug in the first place. So now we also have new files
which are oriented around the fact that Nomad OSS does not consider the |
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.
Looks great. Awesome tests. LGTM.
t.Run("check-permissions kv read", func(t *testing.T) { | ||
t.Run("uses kv has permission", func(t *testing.T) { |
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.
I don't know how I personally feel about this style of nested t.Run
: When the assertions are simply delegation to try
, it feels a simple table tests might read better and is more concise.
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, for tests like these it's hard to find the right balance between readability of the code and readability of the description of the test. By composing t.Run
we get a free way of composing the description, making it obvious what the difference is between one particular test from the tests surrounding it. OTOH it's a lot of lines of code that aren't actually doing anything. I'll take a look at refactoring these tests some more after the bugfix release.
Spot checking (oss Nomad + oss Consul)
|
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. |
This PR fixes the Nomad Object Namespace <-> Consul ACL Token relationship
check when using Consul OSS (or Consul ENT without namespace support).
Nomad v1.1.0 introduced a regression where Nomad would fail the validation
when submitting Connect jobs and allow_unauthenticated set to true, with
Consul OSS - because it would do the namespace check against the Consul ACL
token assuming the "default" namespace, which does not work because Consul OSS
does not have namespaces.
Instead of making the bad assumption, expand the namespace check to handle
each special case explicitly.
Fixes #10718