auth: use ACLsDisabledACL when ACLs are disabled
#18754
Merged
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
This was referenced Oct 13, 2023
tgross
force-pushed
the
no-nil-acls-final
branch
from
10c2d59
to
32a3adb
Compare
tgross
force-pushed
the
no-nil-acls-final
branch
from
32a3adb
to
4ae6246
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 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
force-pushed
the
no-nil-acls-final
branch
from
4ae6246
to
b722882
Compare
jrasell
approved these changes
Oct 16, 2023
Oh, I bet we could. We just need to make sure the |
This was referenced Oct 16, 2023
Merged
tgross
added a commit
that referenced
this pull request
Dec 1, 2023
…o release/1.7.x #19278 Co-authored-by: Tim Gross <tgross@hashicorp.com>
nvanthao
pushed a commit
to nvanthao/nomad
that referenced
this pull request
Mar 1, 2024
…corp#19275) In hashicorp#18754 we accidentally fixed a bug that prevented poststop tasks from getting access to Variables. This was fixed in the 1.6.x branch in hashicorp#19270, at which point we discovered the fix had been done in main already as part of the auth refactor. Add a changelog entry for it.
nvanthao
pushed a commit
to nvanthao/nomad
that referenced
this pull request
Mar 1, 2024
…corp#19275) In hashicorp#18754 we accidentally fixed a bug that prevented poststop tasks from getting access to Variables. This was fixed in the 1.6.x branch in hashicorp#19270, at which point we discovered the fix had been done in main already as part of the auth refactor. Add a changelog entry for it.
tgross
added a commit
that referenced
this pull request
Mar 18, 2024
As of #18754 which shipped in Nomad 1.7, we no longer need to nil-check the object returned by `ResolveACL` if there's no error return, because in the case where ACLs are disabled we return a special "ACLs disabled" ACL object. Checking nil is not a bug but should be discouraged because it opens us up to future bugs that would bypass ACLs. While working on an unrelated feature @lindleywhite discovered that we missed removing the nil check from several endpoints with our semgrep linter. This changeset fixes that. Co-Author: Lindley <lindley@hashicorp.com>
tgross
added a commit
that referenced
this pull request
Mar 18, 2024
As of #18754 which shipped in Nomad 1.7, we no longer need to nil-check the object returned by `ResolveACL` if there's no error return, because in the case where ACLs are disabled we return a special "ACLs disabled" ACL object. Checking nil is not a bug but should be discouraged because it opens us up to future bugs that would bypass ACLs. While working on an unrelated feature @lindleywhite discovered that we missed removing the nil check from several endpoints with our semgrep linter. This changeset fixes that. Co-Author: Lindley <lindley@hashicorp.com>
philrenaud
pushed a commit
that referenced
this pull request
Apr 18, 2024
As of #18754 which shipped in Nomad 1.7, we no longer need to nil-check the object returned by `ResolveACL` if there's no error return, because in the case where ACLs are disabled we return a special "ACLs disabled" ACL object. Checking nil is not a bug but should be discouraged because it opens us up to future bugs that would bypass ACLs. While working on an unrelated feature @lindleywhite discovered that we missed removing the nil check from several endpoints with our semgrep linter. This changeset fixes that. Co-Author: Lindley <lindley@hashicorp.com>
tgross
added a commit
that referenced
this pull request
Apr 18, 2024
As of #18754 which shipped in Nomad 1.7, we no longer need to nil-check the object returned by ResolveACL if there's no error return, because in the case where ACLs are disabled we return a special "ACLs disabled" ACL object. Checking nil is not a bug but should be discouraged because it opens us up to future bugs that would bypass ACLs. We fixed a bunch of these cases in #20150 but I didn't update the semgrep rule, which meant we missed a few more. Update the semgrep rule and fix the remaining cases.
tgross
added a commit
that referenced
this pull request
Apr 18, 2024
As of #18754 which shipped in Nomad 1.7, we no longer need to nil-check the object returned by ResolveACL if there's no error return, because in the case where ACLs are disabled we return a special "ACLs disabled" ACL object. Checking nil is not a bug but should be discouraged because it opens us up to future bugs that would bypass ACLs. We fixed a bunch of these cases in #20150 but I didn't update the semgrep rule, which meant we missed a few more. Update the semgrep rule and fix the remaining cases.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 final patch in a series to eliminate the use ofnilACLs 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:
AllowDebugACL check for the weird special casing we have for pprof debugging when ACLs are disabled.nomad/acl.gofile.Ref: https://github.com/hashicorp/nomad-enterprise/pull/1218
Ref: #18703
Ref: #18715
Ref: #16799
Ref: #18730
Ref: #18744
Notes for reviewers:
if aclObj != nillines all over the RPC endpoints 😀if aclObj != nilfrom those RPC endpoints too.