Add cache for Bcrypt password #11904
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11904 +/- ##
============================================
+ Coverage 61.42% 61.44% +0.01%
Complexity 1129 1129
============================================
Files 2385 2385
Lines 129188 129193 +5
Branches 20000 20002 +2
============================================
+ Hits 79355 79378 +23
+ Misses 44082 44059 -23
- Partials 5751 5756 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 15 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
|
What if user changed the auth, how long will it reflect? I feel a better solution is to use zookeeper listener to update the cache, so it's up to date and you just need to init it once during startup time. |
Hi @xiangfu0 . If users change their password, it will take effect immediately. Because encrypted password is used as key of cache entry. When user change password, it will get null from cache entries, so it will execute Brypt.checkpw to check and put it to cache right now. The reason why we set expire time is that cache will save all old password before delete. |
| @@ -135,7 +135,8 @@ private Optional<ZkBasicAuthPrincipal> getPrincipalAuth(RequesterIdentity reques | |||
|
|
|||
| Optional<ZkBasicAuthPrincipal> principalOpt = | |||
| password2principal.entrySet().stream() | |||
| .filter(entry -> BcryptUtils.checkpw(entry.getKey(), entry.getValue().getPassword())) | |||
| .filter(entry -> BcryptUtils.checkpwWithCache(entry.getKey(), entry.getValue().getPassword(), | |||
| _userCache.getUserPasswordAuthCache())) | |||
There was a problem hiding this comment.
(code format) Can you apply Pinot Style and reformat all the changes in this PR
There was a problem hiding this comment.
Hi @Jackie-Jiang , I have reformat all changes about files in this PR, Please review again.
| boolean isMatch = true; | ||
| String cachedPassword = userPasswordAuthCache.getIfPresent(encryptedPassword); | ||
| if (cachedPassword == null || !cachedPassword.equals(password)) { | ||
| isMatch = checkpw(password, encryptedPassword); |
There was a problem hiding this comment.
This means wrong credential will force the checkpw every time right?
Since we already have a zk subscribe, is it necessary to check the password?
There was a problem hiding this comment.
Hi @xiangfu0 , You are right, each wrong credential will force the checkpw every time. In my opnion, If we have n users, the time complexity for executing checkpw is almost O(n) in one day. And I think this should be tolerable for pinot users.
I think it is necessary to check to password. I will give an example. Such we have a user: admin, password: admin. The path /PinotCluster/PROPERTYSTORE/CONFIGS/USER/admin_CONTROLLER in zookeeper will store below details which save password as encrypted password: "$2a$10$DoCs2UyjBeBk9H7pvZw8kehCa9ot7ofdcF8uKx30PNHdyTyvG4Tiq" other than plain text password "admin".
{
"id" : "admin",
"simpleFields" : {
"password" : "$2a$10$DoCs2UyjBeBk9H7pvZw8kehCa9ot7ofdcF8uKx30PNHdyTyvG4Tiq",
"component" : "CONTROLLER",
"role" : "ADMIN",
"username" : "admin"
},
"mapFields" : { },
"listFields" : { }
}
Cisco Wap Storage Team add cache to optimize query logic. Bcrypt decrypt is a time-consuming operator. So we could add cache to reduce query time. And We set expire time of each password is 24h.
Fix: Issue 11902
Hi @xiangfu0 ， please help to review this pr.