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

Ensure SHA256 is not used in tests #42289

Merged
merged 6 commits into from
May 22, 2019

Conversation

jkakavas
Copy link
Member

SHA256 was recently added to the Hasher class in order to be used
in the TokenService. A few tests were still using values() to get
the available algorithms from the Enum and it could happen that
SHA256 would be picked up by these.
This change adds an extra convenience method
(Hasher#getAvailableAlgoCacheHash) and enures that only this and
Hasher#getAvailableAlgoStoredHash are used for getting the list of
available password hashing algorithms in our tests.

Resolves #42267

SHA256 was recently added to the Hasher class in order to be used
in the TokenService. A few tests were still using values() to get
the available algorithms from the Enum and it could happen that
SHA256 would be picked up by these.
This change adds an extra convenience method
(Hasher#getAvailableAlgoCacheHash) and enures that only this and
Hasher#getAvailableAlgoStoredHash are used for getting the list of
available password hashing algorithms in our tests.
@jkakavas jkakavas added >test-failure Triaged test failures from CI :Security/Security Security issues without another label v8.0.0 v7.2.0 labels May 21, 2019
@jkakavas jkakavas requested a review from tvernum May 21, 2019 12:45
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@jaymode
Copy link
Member

jaymode commented May 21, 2019

my two cents is that we should probably make values for hasher a forbidden API or just change Hasher so that it is not an enum. It seems like it would be too easy for this to pop back up.

@jkakavas
Copy link
Member Author

I contemplated other options too. I attempted to change this from an Enum when doing #31234 but circled back, not really sure what was the issue though. I also thought about moving the SHA256 to the TokenService as this is the only option with no salt ( well, NOOP too ).
I see your point and I think making values() a forbidden API makes sense. I don't worry that this will make it into a password hashing code since we use resolve and SHA256 is not available there, but it might well creep up to a test case with someone getting a random from Hasher.values() .

@jaymode
Copy link
Member

jaymode commented May 21, 2019

but it might well creep up to a test case with someone getting a random from Hasher.values() .

This is what I was thinking of.

talevy added a commit to talevy/elasticsearch that referenced this pull request May 21, 2019
some tests are failing after the introduction of elastic#41792.

relates elastic#42267 and elastic#42289.
talevy added a commit that referenced this pull request May 21, 2019
some tests are failing after the introduction of #41792.

relates #42267 and #42289.
talevy added a commit that referenced this pull request May 21, 2019
some tests are failing after the introduction of #41792.

relates #42267 and #42289.
@talevy
Copy link
Contributor

talevy commented May 21, 2019

fyi, I found another test that relates to this and muted two related tests in #42304

Copy link
Contributor

@tvernum tvernum 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 jkakavas merged commit 18bff0c into elastic:master May 22, 2019
jkakavas added a commit that referenced this pull request May 22, 2019
SHA256 was recently added to the Hasher class in order to be used
in the TokenService. A few tests were still using values() to get
the available algorithms from the Enum and it could happen that
SHA256 would be picked up by these.
This change adds an extra convenience method
(Hasher#getAvailableAlgoCacheHash) and enures that only this and
Hasher#getAvailableAlgoStoredHash are used for getting the list of
available password hashing algorithms in our tests.
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
some tests are failing after the introduction of elastic#41792.

relates elastic#42267 and elastic#42289.
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
SHA256 was recently added to the Hasher class in order to be used
in the TokenService. A few tests were still using values() to get
the available algorithms from the Enum and it could happen that
SHA256 would be picked up by these.
This change adds an extra convenience method
(Hasher#getAvailableAlgoCacheHash) and enures that only this and
Hasher#getAvailableAlgoStoredHash are used for getting the list of
available password hashing algorithms in our tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Security/Security Security issues without another label >test-failure Triaged test failures from CI v7.2.0 v8.0.0-alpha1
Projects
None yet
6 participants