Skip to content
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.

Sign up for GitHub

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

Jump to bottom

resolveHasher defaults to NOOP #31723

Merged
merged 3 commits into from
Jul 3, 2018
Merged

resolveHasher defaults to NOOP #31723

merged 3 commits into from
Jul 3, 2018

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented Jul 2, 2018

This changes the default behavior when resolving the hashing
algorithm from unrecognised hash strings, which was introduced in
#31234

A hash string that doesn't start with an algorithm identifier can
either be a malformed/corrupted hash or a plaintext password when
Hasher.NOOP is used(against warnings).
Do not make assumptions about which of the two is true for such
strings and default to Hasher.NOOP. This covers the case for
malformed hashes also as hash verification will subsequently
fail.
Finally, do not log the potentially malformed hash as this can very
well be a plaintext password.

Resolves #31697
Reverts 58cf95a

jkakavas added 3 commits July 2, 2018 12:02
@jkakavas
Default resolveFromHash to Hasher.NOOP
0434be4 
This changes the default behavior when resolving the hashing
algorithm from unrecognised hash strings, which was introduced in
 elastic#31234

A hash string that doesn't start with an algorithm identifier can
either be a malformed/corrupted hash or a plaintext password when
Hasher.NOOP is used(against warnings).
Do not make assumptions about which of the two is true for such
strings and default to Hasher.NOOP. Hash verification will subsequently
fail for malformed hashes.
Finally, do not log the potentially malformed hash as this can very
well be a plaintext password.

Resolves elastic#31697
Reverts 58cf95a
@jkakavas
Fix line length
705e945
@jkakavas
Merge remote-tracking branch 'origin/master' into hasher-default-tonoop
666b5cf
@jkakavas jkakavas added >bug review v7.0.0 :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v6.4.0 labels Jul 2, 2018
@jkakavas jkakavas requested a review from jaymode July 2, 2018 10:12
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@albertzaharovits
Copy link
Contributor

@jkakavas I think it might be better to have a tag for the NOOP hashing algo as well, and not rely on the leniency that any hash string is valid and must be a NOOP hash? Although it will not be NOOP anymore, since the op adds a prefix? I think there might be exceptional debugging cases where you might wonder if a password hash entry is valid or is NOOP since it was randomly generated in a failing test.

I don't see my suggestion impactful in any way, since NOOP hash will be used exclusively for debugging purposes (as only secure options are allowed:

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java

Line 552 in 1dd10fe

public static List<String> getAvailableAlgoStoredHash() {
)

@jkakavas
Copy link
Member Author

jkakavas commented Jul 2, 2018

Thanks @albertzaharovits. I would like to propose to deprecate the NOOP hasher in a future release and thus didn't want to make any further changes to related behavior but I have no objection going forward with your suggestions. As you said, I don't see this impacting functionality

@albertzaharovits
Copy link
Contributor

Gotcha. Thanks for the FYI.

jaymode
jaymode approved these changes Jul 2, 2018
View reviewed changes
Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jkakavas
Copy link
Member Author

jkakavas commented Jul 3, 2018

@albertzaharovits I will merge this as is and depending on the outcome of the discussion on the NOOP hasher, I will raise a new one if deemed necessary. Is this ok with you?

@albertzaharovits
Copy link
Contributor

Sure, it was only a thought.

@jkakavas jkakavas merged commit 49b977b into elastic:master Jul 3, 2018
dnhatn added a commit that referenced this pull request Jul 4, 2018
@dnhatn
Merge branch 'master' into ccr
f736205 
* master:
  [ML] Rate limit established model memory updates (#31768)
  [Docs] Correct default window_size (#31582)
  S3 fixture should report 404 on unknown bucket (#31782)
  Detach Transport from TransportService (#31727)
  [ML] Limit ML filter items to 10K (#31731)
  [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647)
  Fixture for Minio testing (#31688)
  [DOCS] Add missing get mappings docs to HLRC (#31765)
  [DOCS] Starting Elasticsearch (#31701)
  Painless: Complete Removal of Painless Type (#31699)
  Fix not waiting for Netty ThreadDeathWatcher in IT (#31758)
  Consolidate watcher setting update registration (#31762)
  Build: re-enabled bwc (#31769)
  ingest: Introduction of a bytes processor (#31733)
  Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747)
  Add analyze API to high-level rest client (#31577)
  [DOCS] Typos
  DOC: Add examples to the SQL docs (#31633)
  Add support for AWS session tokens (#30414)
  Watcher: Reenable start/stop yaml tests (#31754)
  Implemented XContent serialisation for GetIndexResponse (#31675)
  JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735)
  resolveHasher defaults to NOOP (#31723)
  Account for XContent overhead in in-flight breaker
  Split CircuitBreaker-related tests (#31659)
  Add write*Blob option to replace existing blob (#31729)
  Painless: Add Context Docs (#31190)
  Watcher: Fix chain input toXcontent serialization (#31721)
  Docs: Match the examples in the description (#31710)
  rest-high-level: added get cluster settings (#31706)
  [Docs] Correct typos (#31720)
  Clean up double semicolon code typos (#31687)
  [DOCS] Check for Windows and *nix file paths (#31648)
  [ML] Validate ML filter_id (#31535)
  Revert long lines
  Fix TransportChangePasswordActionTests
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Jul 13, 2018
@jkakavas
resolveHasher defaults to NOOP (elastic#31723)
0d5b172 
* Default resolveFromHash to Hasher.NOOP

This changes the default behavior when resolving the hashing
algorithm from unrecognised hash strings, which was introduced in
 elastic#31234

A hash string that doesn't start with an algorithm identifier can
either be a malformed/corrupted hash or a plaintext password when
Hasher.NOOP is used(against warnings).
Do not make assumptions about which of the two is true for such
strings and default to Hasher.NOOP. Hash verification will subsequently
fail for malformed hashes.
Finally, do not log the potentially malformed hash as this can very
well be a plaintext password.

Resolves elastic#31697
Reverts 58cf95a
@jkakavas jkakavas mentioned this pull request Jul 16, 2018
Merged
jkakavas added a commit that referenced this pull request Jul 18, 2018
@jkakavas
Configurable password hashing algorithm/cost(#31234) (#32092)
ba0ecae 
* Configurable password hashing algorithm/cost (#31234)

Make password hashing algorithm/cost configurable for the
stored passwords of users for the realms that this applies
(native, reserved). Replaces predefined choice of bcrypt with
cost factor 10.
This also introduces PBKDF2 with configurable cost
(number of iterations) as an algorithm option for password hashing
both for storing passwords and for the user cache.
Password hash validation algorithm selection takes into
consideration the stored hash prefix and only a specific number
of algorithnm and cost factor options for brypt and pbkdf2 are
whitelisted and can be selected in the relevant setting.

* resolveHasher defaults to NOOP (#31723)

This changes the default behavior when resolving the hashing
algorithm from unrecognised hash strings, which was introduced in
 #31234

A hash string that doesn't start with an algorithm identifier can
either be a malformed/corrupted hash or a plaintext password when
Hasher.NOOP is used(against warnings).
Do not make assumptions about which of the two is true for such
strings and default to Hasher.NOOP. Hash verification will subsequently
fail for malformed hashes.
Finally, do not log the potentially malformed hash as this can very
well be a plaintext password.

* Fix RealmInteg test failures

As part of the changes in #31234,the password verification logic
determines the algorithm used for hashing the password from the
format of the stored password hash itself. Thus, it is generally
possible to validate a password even if it's associated stored hash
was not created with the same algorithm than the one currently set
in the settings.
At the same time, we introduced a check for incoming client change
password requests to make sure that the request's password is hashed
with the same algorithm that is configured to be used in the node
settings.
In the spirit of randomizing the algorithms used, the
{@code SecurityClient} used in the {@code NativeRealmIntegTests} and
{@code ReservedRealmIntegTests} would send all requests dealing with
user passwords by randomly selecting a hashing algorithm each time.
This meant that some change password requests were using a different
password hashing algorithm than the one used for the node and the
request would fail.
This commit changes this behavior in the two aforementioned Integ
tests to use the same password hashing algorithm for the node and the
clients, no matter what the request is.
@jkakavas jkakavas deleted the hasher-default-tonoop branch September 14, 2018 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaymode jaymode jaymode approved these changes

Assignees
No one assigned
Labels
>bug :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jkakavas @elasticmachine @albertzaharovits @jaymode @colings86