DefaultUserInfo empty password hash #64018
Merged
Conversation
This file contains hidden or 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
Use an empty char array in place of the hash of the empty string for defaultEnabledReservedUser, defaultDisabledReservedUser and bootstrapUserInfo ReservedUserInfo objects, when the password is empty. In these cases, the hasEmptyPassword property is set to true and so the actual password hash is not ever checked or verified in ReservedRealm#doAuthenticate so this change has no functional effect on existing logic. If anything, it is more consistent with the logic in NativeUsersStore#setReservedUserEnabled when the user document didn't already exist. Even if there is / will be code that attempts to verify the hash, this would still work as intended as Hasher#resolve would resolve the NOOP Hasher from the empty string and its verify method would compare the char arrays. The reason for this change is that when in FIPS 140 approved only mode, the security provider's implementation of PBKDF2 might forbid operations when the password is smaller than 112 bits so the existing implementation would cause elasticsearch to fail.
jkakavas
added
>bug
:Security/Authentication
|
Pinging @elastic/es-security (:Security/Authentication) |
ywangd
reviewed
Oct 21, 2020
| @@ -697,5 +691,13 @@ ReservedUserInfo deepClone() { | |||
| boolean verifyPassword(SecureString data) { | |||
| return hasher.verify(data, this.passwordHash); | |||
| } | |||
|
|
|||
| static ReservedUserInfo defaultEnabledUserInfo(){ | |||
| return new ReservedUserInfo(new char[0], true, true); | |||
There was a problem hiding this comment.
With this PR, the empty passwordHash now has a deterministic value, new char[0]. It would be better to remove hasEmptyPassword from the constructor parameters of ReservedUserInfo because it can be safely derived. It is more consistent to derive it than manually passing it into the constructor. We can also replace the hasEmptyPassword field with a method call which internally returns passwordHash.length == 0.
tvernum
approved these changes
Oct 22, 2020
| @@ -697,5 +691,13 @@ ReservedUserInfo deepClone() { | |||
| boolean verifyPassword(SecureString data) { | |||
| return hasher.verify(data, this.passwordHash); | |||
| } | |||
|
|
|||
| static ReservedUserInfo defaultEnabledUserInfo(){ | |||
There was a problem hiding this comment.
Nit:
Suggested change
| static ReservedUserInfo defaultEnabledUserInfo(){ | |
| static ReservedUserInfo defaultEnabledUserInfo() { |
jkakavas
added a commit
to jkakavas/elasticsearch
that referenced
this pull request
Dec 1, 2020
Use an empty char array in place of the hash of the empty string for defaultEnabledReservedUser, defaultDisabledReservedUser and bootstrapUserInfo ReservedUserInfo objects, when the password is empty. In these cases, the hasEmptyPassword property is set to true and so the actual password hash is not ever checked or verified in ReservedRealm#doAuthenticate so this change has no functional effect on existing logic. If anything, it is more consistent with the logic in NativeUsersStore#setReservedUserEnabled when the user document didn't already exist. Even if there is / will be code that attempts to verify the hash, this would still work as intended as Hasher#resolve would resolve the NOOP Hasher from the empty string and its verify method would compare the char arrays. The reason for this change is that when in FIPS 140 approved only mode, the security provider's implementation of PBKDF2 might forbid operations when the password is smaller than 112 bits so the existing implementation would cause elasticsearch to fail.
jkakavas
added a commit
that referenced
this pull request
Dec 1, 2020
Use an empty char array in place of the hash of the empty string for defaultEnabledReservedUser, defaultDisabledReservedUser and bootstrapUserInfo ReservedUserInfo objects, when the password is empty. In these cases, the hasEmptyPassword property is set to true and so the actual password hash is not ever checked or verified in ReservedRealm#doAuthenticate so this change has no functional effect on existing logic. If anything, it is more consistent with the logic in NativeUsersStore#setReservedUserEnabled when the user document didn't already exist. Even if there is / will be code that attempts to verify the hash, this would still work as intended as Hasher#resolve would resolve the NOOP Hasher from the empty string and its verify method would compare the char arrays. The reason for this change is that when in FIPS 140 approved only mode, the security provider's implementation of PBKDF2 might forbid operations when the password is smaller than 112 bits so the existing implementation would cause elasticsearch to fail.
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.
Use an empty char array in place of the hash of the empty string
for defaultEnabledReservedUser, defaultDisabledReservedUser and
bootstrapUserInfo ReservedUserInfo objects, when the password is
empty. In these cases, the hasEmptyPassword property is set to
true and so the actual password hash is not ever checked or
verified in ReservedRealm#doAuthenticate so this change has no
functional effect on existing logic. If anything, it is more
consistent with the logic in NativeUsersStore#setReservedUserEnabled
when the user document didn't already exist.
Even if there is / will be code that attempts to verify the hash,
this would still work as intended as Hasher#resolve would resolve
the NOOP Hasher from the empty string and its verify method would
compare the char arrays.
The reason for this change is that when in FIPS 140 approved only
mode, the security provider's implementation of PBKDF2 might
forbid operations when the password is smaller than 112 bits so
the existing implementation would cause elasticsearch to fail.