auth: add client-only ACL #18730
Merged
auth: add client-only ACL #18730
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tgross
force-pushed
the
no-nil-acls-client-auth
branch
from
0e7e683 to
a30fb52
Compare
tgross
force-pushed
the
no-nil-acls-client-auth
branch
from
a30fb52 to
c1a2e97
Compare
tgross
force-pushed
the
no-nil-acls-client-auth
branch
from
c1a2e97 to
c69a556
Compare
The RPC handlers expect to see `nil` ACL objects whenever ACLs are disabled. By using `nil` as a sentinel value, we have the risk of nil pointer exceptions and improper handling of `nil` when returned from our various auth methods that can lead to privilege escalation bugs. This is the third in a series to eliminate the use of `nil` ACLs as a sentinel value for when ACLs are disabled. This patch involves creating a new "virtual" ACL object for checking permissions on client operations and a matching `AuthenticateClientOnly` method for client-only RPCs that can produce that ACL. Unlike the server ACLs PR, this also includes a special case for "legacy" client RPCs where the client was not previously sending the secret as it should (leaning on mTLS only). Those client RPCs were fixed in Nomad 1.6.0, but it'll take a while before we can guarantee they'll be present during upgrades. Ref: hashicorp/nomad-enterprise#1218 Ref: #18703 Ref: #18715 Ref: #16799
tgross
force-pushed
the
no-nil-acls-client-auth
branch
from
c69a556 to
3501515
Compare
shoenig
approved these changes
Oct 11, 2023
Comment on lines
+264
to
+266
| if err != nil { | ||
| s.logger.Error("could not determine remote address", "error", err) | ||
| } |
There was a problem hiding this comment.
just making sure we do not want to bail here?
There was a problem hiding this comment.
I'm not sure. I can't think of any case where we'd actually get an error when ctx != nil (and if it is we return no error), but I also didn't want to introduce a more serious bug if I was missing something, just for the sake of metrics.
nomad/auth/auth.go
Outdated
Comment on lines
281
to
289
| expected := fmt.Sprintf("client.%s.nomad", s.region) | ||
| _, err := validateCertificateForName(tlsCert, expected) | ||
| if err != nil { | ||
| expected := fmt.Sprintf("server.%s.nomad", s.region) | ||
| _, err := validateCertificateForName(tlsCert, expected) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
would it make sense to have validateCertificateForName accept a slice of expected values of which at least one must match?
There was a problem hiding this comment.
Done in b07a322. While doing that I noticed we had a straggler use of the cert check and after removing that we can remove the old cert check code in nomad/util.go entirely. Done in the same commit.
There was a problem hiding this comment.
And then that revealed that there was a bunch of places where we weren't authenticating with the node secret in test code, even though it's required in the RPC. Fixed that.
tgross
force-pushed
the
no-nil-acls-client-auth
branch
from
c06b7a2 to
262324c
Compare
tgross
added a commit
that referenced
this pull request
Oct 12, 2023
The RPC handlers expect to see `nil` ACL objects whenever ACLs are disabled. By using `nil` as a sentinel value, we have the risk of nil pointer exceptions and improper handling of `nil` when returned from our various auth methods that can lead to privilege escalation bugs. This is the third in a series to eliminate the use of `nil` ACLs as a sentinel value for when ACLs are disabled. This patch involves leveraging the refactored `auth` package to remove the weird "mixed auth" helper functions that only support the Variables read/list RPC handlers. Instead, pass the ACL object and claim together into the `AllowVariableOperations` method in the usual `acl` package. Ref: hashicorp/nomad-enterprise#1218 Ref: #18703 Ref: #18715 Ref: #16799 Ref: #18730 Fixes: #15875
tgross
added a commit
that referenced
this pull request
Oct 12, 2023
The RPC handlers expect to see `nil` ACL objects whenever ACLs are disabled. By using `nil` as a sentinel value, we have the risk of nil pointer exceptions and improper handling of `nil` when returned from our various auth methods that can lead to privilege escalation bugs. This is the third in a series to eliminate the use of `nil` ACLs as a sentinel value for when ACLs are disabled. This patch involves leveraging the refactored `auth` package to remove the weird "mixed auth" helper functions that only support the Variables read/list RPC handlers. Instead, pass the ACL object and claim together into the `AllowVariableOperations` method in the usual `acl` package. Ref: hashicorp/nomad-enterprise#1218 Ref: #18703 Ref: #18715 Ref: #16799 Ref: #18730 Fixes: #15875
tgross
added a commit
that referenced
this pull request
Oct 13, 2023
The RPC handlers expect to see `nil` ACL objects whenever ACLs are disabled. By using `nil` as a sentinel value, we have the risk of nil pointer exceptions and improper handling of `nil` when returned from our various auth methods that can lead to privilege escalation bugs. This is the final patch in a series to eliminate the use of `nil` ACLs as a sentinel value for when ACLs are disabled. This patch adds a new virtual ACL policy field for when ACLs are disabled and updates our authentication logic to use it. Included: * Extends auth package tests to demonstrate that nil ACLs are treated as failed auth and disabled ACLs succeed auth. * Adds a new `AllowDebug` ACL check for the weird special casing we have for pprof debugging when ACLs are disabled. * Removes the remaining unexported methods (and repeated tests) from the `nomad/acl.go` file. * Update the semgrep rules to detect improper nil ACL checking and remove the old invalid ACL checks. * Update the contributing guide for RPC authentication. Ref: hashicorp/nomad-enterprise#1218 Ref: #18703 Ref: #18715 Ref: #16799 Ref: #18730 Ref: #18744
tgross
added a commit
that referenced
this pull request
Oct 16, 2023
The RPC handlers expect to see `nil` ACL objects whenever ACLs are disabled. By using `nil` as a sentinel value, we have the risk of nil pointer exceptions and improper handling of `nil` when returned from our various auth methods that can lead to privilege escalation bugs. This is the final patch in a series to eliminate the use of `nil` ACLs as a sentinel value for when ACLs are disabled. This patch adds a new virtual ACL policy field for when ACLs are disabled and updates our authentication logic to use it. Included: * Extends auth package tests to demonstrate that nil ACLs are treated as failed auth and disabled ACLs succeed auth. * Adds a new `AllowDebug` ACL check for the weird special casing we have for pprof debugging when ACLs are disabled. * Removes the remaining unexported methods (and repeated tests) from the `nomad/acl.go` file. * Update the semgrep rules to detect improper nil ACL checking and remove the old invalid ACL checks. * Update the contributing guide for RPC authentication. Ref: hashicorp/nomad-enterprise#1218 Ref: #18703 Ref: #18715 Ref: #16799 Ref: #18730 Ref: #18744
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The RPC handlers expect to see
nilACL objects whenever ACLs are disabled. By usingnilas a sentinel value, we have the risk of nil pointer exceptions and improper handling ofnilwhen returned from our various auth methods that can lead to privilege escalation bugs. This is the third in a series to eliminate the use ofnilACLs as a sentinel value for when ACLs are disabled.This patch involves creating a new "virtual" ACL object for checking permissions on client operations and a matching
AuthenticateClientOnlymethod for client-only RPCs that can produce that ACL.Unlike the server ACLs PR, this also includes a special case for "legacy" client RPCs where the client was not previously sending the secret as it should (leaning on mTLS only). Those client RPCs were fixed in Nomad 1.6.0, but it'll take a while before we can guarantee they'll be present during upgrades.
Ref: https://github.com/hashicorp/nomad-enterprise/pull/1218
Ref: #18703
Ref: #18715
Ref: #16799