Add cache for password hash in druid-basic-security#15648
Add cache for password hash in druid-basic-security#15648abhishekagarwal87 merged 3 commits intoapache:masterfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @Test | ||
| public void testHashPassword() | ||
| { | ||
| char[] password = "HELLO".toCharArray(); |
Check failure
Code scanning / CodeQL
Hard-coded credential in API call
| { | ||
| final PasswordHashGenerator hashGenerator = new PasswordHashGenerator(); | ||
|
|
||
| final char[] password = "this_is_a_long_password".toCharArray(); |
Check failure
Code scanning / CodeQL
Hard-coded credential in API call
| { | ||
| final byte[] passwordSha; | ||
| final byte[] salt; | ||
| final int numIterations; |
There was a problem hiding this comment.
do we need numIterations in CacheKey?
There was a problem hiding this comment.
Yes, that is because the final hash value can change if numIterations changes. Also, it doesn't seem like we use the same number of iterations for every credential as it is configurable. But let me double check to be sure.
| /** | ||
| * Hashes the given password using the {@link #HASH_ALGORITHM}. | ||
| */ | ||
| public byte[] getOrComputePasswordHash(char[] password, byte[] salt, int numIterations) |
There was a problem hiding this comment.
did you run some local test to how much runs-per-second you get with "getOrComputePasswordHash" and with "computePasswordHash"? Just to make sure that we do see perf gains. By benchmark, I really mean just a simple test method calling these functions in a loop
There was a problem hiding this comment.
Yes, the newly added test PasswordHashGeneratorTest does a bit of comparison of the two methods.
| // Verify that the first computation takes a few ms | ||
| stopwatch.restart(); | ||
| hashGenerator.getOrComputePasswordHash(password, salt, iterations); | ||
| long firstComputeTimeMillis = stopwatch.millisElapsed(); | ||
| Assert.assertTrue(firstComputeTimeMillis > 50); | ||
|
|
||
| // Verify that each subsequent computation takes less than 1ms | ||
| for (int i = 0; i < 10; ++i) { | ||
| stopwatch.restart(); | ||
| hashGenerator.getOrComputePasswordHash(password, salt, iterations); | ||
| long recomputeTimeMillis = stopwatch.millisElapsed(); | ||
| Assert.assertTrue(recomputeTimeMillis <= 1); | ||
| } |
There was a problem hiding this comment.
This is not a robust test though at least the perf is verified. Instead you could expose a method to access the cache and then access the CacheStats through getStats and verify hit count.
There was a problem hiding this comment.
Yeah, I had similar thoughts, let me try adding that.
|
Yay! |
5 participants
Changes
PasswordHashGenerator. Move hashing logic fromBasicAuthUtilsto this new class.Pending changes
Release note
The computed hash values of passwords are now cached to boost authentication validator performance.
This PR has: