Fix potential NPE in UsersTool #37660
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
I found the first UserException thrown in #execute marked as dead code when accidentally switching this from a warning to an error. It looks like the output of FileUserPasswdStore.parseFile a line above shouldn't be wrapped into another map since its output can be null. Looking at Commit fab5e21 I wonder what changed this back? The history of this file is a bit unclear to me, but I think L225 should be changed.
cbuescher
added
>bug
v7.0.0
:Security/Security
|
Pinging @elastic/es-security |
|
@elasticmachine run the gradle build tests 2 |
cbuescher
commented
Jan 21, 2019
| @@ -221,7 +222,7 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th | |||
|
|
|||
| Path file = FileUserPasswdStore.resolveFile(env); | |||
| FileAttributesChecker attributesChecker = new FileAttributesChecker(file); | |||
| Map<String, char[]> users = new HashMap<>(FileUserPasswdStore.parseFile(file, null, env.settings())); | |||
| Map<String, char[]> users = FileUserPasswdStore.parseFile(file, null, env.settings()); | |||
| if (users == null) { | |||
| throw new UserException(ExitCodes.CONFIG, "Configuration file [" + file + "] is missing"); | |||
There was a problem hiding this comment.
Currently this looks dead. Furthermore, FileUserPasswdStore.parseFile can return null which I think will then create an NPE in the HashMap ctor.
cbuescher
commented
Jan 22, 2019
| if (users == null) { | ||
| throw new UserException(ExitCodes.CONFIG, "Configuration file [" + file + "] is missing"); | ||
| } | ||
| if (users.containsKey(username) == false) { | ||
| throw new UserException(ExitCodes.NO_USER, "User [" + username + "] doesn't exist"); | ||
| } | ||
| final Hasher hasher = Hasher.resolve(XPackSettings.PASSWORD_HASHING_ALGORITHM.get(env.settings())); | ||
| users = new HashMap<>(users); // make modifiable |
There was a problem hiding this comment.
same approach as in L129
|
@elasticmachine run elasticsearch-ci/2 |
|
@cbuescher this also looks like a issue in 6.x. Do you mind backporting to 6.x and 6.6? |
cbuescher
pushed a commit
that referenced
this pull request
Jan 22, 2019
It looks like the output of FileUserPasswdStore.parseFile shouldn't be wrapped into another map since its output can be null. Doing this wrapping after the null check (which potentially raises an exception) instead.
cbuescher
pushed a commit
that referenced
this pull request
Jan 22, 2019
It looks like the output of FileUserPasswdStore.parseFile shouldn't be wrapped into another map since its output can be null. Doing this wrapping after the null check (which potentially raises an exception) instead.
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.
I found the first UserException thrown in #execute marked as dead code when
accidentally switching this from a warning to an error. It looks like the output
of FileUserPasswdStore.parseFile a line above shouldn't be wrapped into another
map since its output can be null. Looking at Commit fab5e21 I wonder what
changed this back? The history of this file is a bit unclear to me, but I think
L225 should be changed.