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.

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

FileRealmTests#testAuthenticateCaching fails with an assertion error. #31697

Closed
jtibshirani opened this issue Jun 29, 2018 · 4 comments
Closed
Assignees
Labels
:Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) >test-failure Triaged test failures from CI

Comments

@jtibshirani
Copy link
Contributor

The failure reliably reproduces for me locally, and may be related to the recent change to password hashing: #31234.


Link to the build: https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+g1gc/6006/console

Command to reproduce:

./gradlew :x-pack:plugin:security:test \
  -Dtests.seed=B944BC5A5B893131 \
  -Dtests.class=org.elasticsearch.xpack.security.authc.file.FileRealmTests \
  -Dtests.method="testAuthenticateCaching" \
  -Dtests.security.manager=true \
  -Dtests.jvm.argline="-XX:-UseConcMarkSweepGC -XX:+UseG1GC" \
  -Dtests.locale=fr-BE \
  -Dtests.timezone=Antarctica/Troll

Relevant excerpt from the logs:

FAILURE 3.40s | FileRealmTests.testAuthenticateCaching <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: 
   > Expected: sameInstance(<User[username=user1,roles=[role1,role2],fullName=null,email=null,metadata={}]>)
   >      but: was <User[username=user1,roles=[role1,role2],fullName=null,email=null,metadata={}]>
   >    at __randomizedtesting.SeedInfo.seed([B944BC5A5B893131:52FC433DA47C405A]:0)
   >    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
   >    at org.elasticsearch.xpack.security.authc.file.FileRealmTests.testAuthenticateCaching(FileRealmTests.java:103)
   >    at java.lang.Thread.run(Thread.java:748)
@jtibshirani jtibshirani added >test-failure Triaged test failures from CI :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Jun 29, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@jkakavas jkakavas self-assigned this Jun 29, 2018
@jkakavas
Copy link
Member

This reproduces with ./gradlew but not when I run the test in Intellij ( I get the same instance of the User object ). Not sure if sameInstance makes sense here, but that indeed doesn't explain why this fails now as it was passing forever.

@bizybot
Copy link
Contributor

bizybot commented Jun 30, 2018

Hi @jkakavas its happening because of NOOP hasher, looks like when we are verifying hash and trying to find the hasher to verify it can't find it from the hash as its plaintext. Hope this helps. I had this test fail in my environment so was looking at it.

@jkakavas
Copy link
Member

jkakavas commented Jun 30, 2018

Thanks @bizybot you're right. We probably need to log that warning too for easier troubleshooting. This is also an actual bug in the implementation as the NOOP hasher can't be used as is. I will address this

jkakavas added a commit to jkakavas/elasticsearch that referenced this issue Jul 2, 2018
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 added a commit that referenced this issue Jul 3, 2018
* Default resolveFromHash to Hasher.NOOP

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.

Resolves #31697
Reverts 58cf95a
jkakavas added a commit to jkakavas/elasticsearch that referenced this issue Jul 13, 2018
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) >test-failure Triaged test failures from CI
Projects
None yet
Development

No branches or pull requests

4 participants