You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "INNOCENT-BOY (via GitHub)" <gi...@apache.org> on 2023/10/30 07:52:30 UTC
[PR] Add cache for Bcrypt password [pinot]
INNOCENT-BOY opened a new pull request, #11904:
URL: https://github.com/apache/pinot/pull/11904
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.
Fix: [Issue 11902](https://github.com/apache/pinot/issues/11902)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add cache for Bcrypt password [pinot]
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #11904:
URL: https://github.com/apache/pinot/pull/11904#discussion_r1382732504
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -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()))
Review Comment:
Hi Jackie, I have reformat all changes about files in this PR, Please review again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add cache for Bcrypt password [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11904:
URL: https://github.com/apache/pinot/pull/11904#discussion_r1387466511
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/BcryptUtils.java:
##########
@@ -18,33 +18,48 @@
*/
package org.apache.pinot.common.utils;
+import com.google.common.cache.Cache;
import org.mindrot.jbcrypt.BCrypt;
+
public class BcryptUtils {
- private static final int DEFALUT_LOG_ROUNDS = 10;
- private static String _bcryptPassword = null;
+ private static final int DEFALUT_LOG_ROUNDS = 10;
+ private static String _bcryptPassword = null;
- private BcryptUtils() {
- }
+ private BcryptUtils() {
+ }
- public static String encrypt(String password) {
- return encrypt(password, DEFALUT_LOG_ROUNDS);
- }
+ public static String encrypt(String password) {
+ return encrypt(password, DEFALUT_LOG_ROUNDS);
+ }
+
+ public static String encrypt(String password, int saltLogRrounds) {
+ _bcryptPassword = BCrypt.hashpw(password, BCrypt.gensalt(saltLogRrounds));
+ return _bcryptPassword;
+ }
- public static String encrypt(String password, int saltLogRrounds) {
- _bcryptPassword = BCrypt.hashpw(password, BCrypt.gensalt(saltLogRrounds));
- return _bcryptPassword;
+ public static boolean checkpw(String pasword, String encrypedPassword) {
+ boolean isMatch = false;
+ try {
+ isMatch = BCrypt.checkpw(pasword, encrypedPassword);
+ } catch (Exception e) {
+ System.out.println(e.getMessage());
+ } finally {
+ return isMatch;
}
+ }
- public static boolean checkpw(String pasword, String encrypedPassword) {
- boolean isMatch = false;
- try {
- isMatch = BCrypt.checkpw(pasword, encrypedPassword);
- } catch (Exception e) {
- System.out.println(e.getMessage());
- } finally {
- return isMatch;
- }
+ public static boolean checkpwWithCache(String password, String encryptedPassword,
+ Cache<String, String> userPasswordAuthCache) {
+ boolean isMatch = true;
+ String cachedPassword = userPasswordAuthCache.getIfPresent(encryptedPassword);
+ if (cachedPassword == null || !cachedPassword.equals(password)) {
+ isMatch = checkpw(password, encryptedPassword);
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add cache for Bcrypt password [pinot]
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on PR #11904:
URL: https://github.com/apache/pinot/pull/11904#issuecomment-1786311905
> 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.
If a user changes their password, it will take effect immediately. Because encrypted password is used as key of cache entry. When user change password, it will execute Brypt.checkpw to check and put it to cache again. The reason why we set expire time is that cache save old password in the cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add cache for Bcrypt password [pinot]
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #11904:
URL: https://github.com/apache/pinot/pull/11904#discussion_r1387499908
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/BcryptUtils.java:
##########
@@ -18,33 +18,48 @@
*/
package org.apache.pinot.common.utils;
+import com.google.common.cache.Cache;
import org.mindrot.jbcrypt.BCrypt;
+
public class BcryptUtils {
- private static final int DEFALUT_LOG_ROUNDS = 10;
- private static String _bcryptPassword = null;
+ private static final int DEFALUT_LOG_ROUNDS = 10;
+ private static String _bcryptPassword = null;
- private BcryptUtils() {
- }
+ private BcryptUtils() {
+ }
- public static String encrypt(String password) {
- return encrypt(password, DEFALUT_LOG_ROUNDS);
- }
+ public static String encrypt(String password) {
+ return encrypt(password, DEFALUT_LOG_ROUNDS);
+ }
+
+ public static String encrypt(String password, int saltLogRrounds) {
+ _bcryptPassword = BCrypt.hashpw(password, BCrypt.gensalt(saltLogRrounds));
+ return _bcryptPassword;
+ }
- public static String encrypt(String password, int saltLogRrounds) {
- _bcryptPassword = BCrypt.hashpw(password, BCrypt.gensalt(saltLogRrounds));
- return _bcryptPassword;
+ public static boolean checkpw(String pasword, String encrypedPassword) {
+ boolean isMatch = false;
+ try {
+ isMatch = BCrypt.checkpw(pasword, encrypedPassword);
+ } catch (Exception e) {
+ System.out.println(e.getMessage());
+ } finally {
+ return isMatch;
}
+ }
- public static boolean checkpw(String pasword, String encrypedPassword) {
- boolean isMatch = false;
- try {
- isMatch = BCrypt.checkpw(pasword, encrypedPassword);
- } catch (Exception e) {
- System.out.println(e.getMessage());
- } finally {
- return isMatch;
- }
+ public static boolean checkpwWithCache(String password, String encryptedPassword,
+ Cache<String, String> userPasswordAuthCache) {
+ boolean isMatch = true;
+ String cachedPassword = userPasswordAuthCache.getIfPresent(encryptedPassword);
+ if (cachedPassword == null || !cachedPassword.equals(password)) {
+ isMatch = checkpw(password, encryptedPassword);
Review 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 "password".
```json
{
"id" : "admin",
"simpleFields" : {
"password" : "$2a$10$DoCs2UyjBeBk9H7pvZw8kehCa9ot7ofdcF8uKx30PNHdyTyvG4Tiq",
"component" : "CONTROLLER",
"role" : "ADMIN",
"username" : "admin"
},
"mapFields" : { },
"listFields" : { }
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add cache for Bcrypt password [pinot]
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #11904:
URL: https://github.com/apache/pinot/pull/11904#discussion_r1382732504
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -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()))
Review Comment:
Hi @Jackie-Jiang , I have reformat all changes about files in this PR, Please review again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add cache for Bcrypt password [pinot]
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #11904:
URL: https://github.com/apache/pinot/pull/11904#discussion_r1387499908
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/BcryptUtils.java:
##########
@@ -18,33 +18,48 @@
*/
package org.apache.pinot.common.utils;
+import com.google.common.cache.Cache;
import org.mindrot.jbcrypt.BCrypt;
+
public class BcryptUtils {
- private static final int DEFALUT_LOG_ROUNDS = 10;
- private static String _bcryptPassword = null;
+ private static final int DEFALUT_LOG_ROUNDS = 10;
+ private static String _bcryptPassword = null;
- private BcryptUtils() {
- }
+ private BcryptUtils() {
+ }
- public static String encrypt(String password) {
- return encrypt(password, DEFALUT_LOG_ROUNDS);
- }
+ public static String encrypt(String password) {
+ return encrypt(password, DEFALUT_LOG_ROUNDS);
+ }
+
+ public static String encrypt(String password, int saltLogRrounds) {
+ _bcryptPassword = BCrypt.hashpw(password, BCrypt.gensalt(saltLogRrounds));
+ return _bcryptPassword;
+ }
- public static String encrypt(String password, int saltLogRrounds) {
- _bcryptPassword = BCrypt.hashpw(password, BCrypt.gensalt(saltLogRrounds));
- return _bcryptPassword;
+ public static boolean checkpw(String pasword, String encrypedPassword) {
+ boolean isMatch = false;
+ try {
+ isMatch = BCrypt.checkpw(pasword, encrypedPassword);
+ } catch (Exception e) {
+ System.out.println(e.getMessage());
+ } finally {
+ return isMatch;
}
+ }
- public static boolean checkpw(String pasword, String encrypedPassword) {
- boolean isMatch = false;
- try {
- isMatch = BCrypt.checkpw(pasword, encrypedPassword);
- } catch (Exception e) {
- System.out.println(e.getMessage());
- } finally {
- return isMatch;
- }
+ public static boolean checkpwWithCache(String password, String encryptedPassword,
+ Cache<String, String> userPasswordAuthCache) {
+ boolean isMatch = true;
+ String cachedPassword = userPasswordAuthCache.getIfPresent(encryptedPassword);
+ if (cachedPassword == null || !cachedPassword.equals(password)) {
+ isMatch = checkpw(password, encryptedPassword);
Review 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 is 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 "password".
```json
{
"id" : "admin",
"simpleFields" : {
"password" : "$2a$10$DoCs2UyjBeBk9H7pvZw8kehCa9ot7ofdcF8uKx30PNHdyTyvG4Tiq",
"component" : "CONTROLLER",
"role" : "ADMIN",
"username" : "admin"
},
"mapFields" : { },
"listFields" : { }
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add cache for Bcrypt password [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #11904:
URL: https://github.com/apache/pinot/pull/11904
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add cache for Bcrypt password [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #11904:
URL: https://github.com/apache/pinot/pull/11904#issuecomment-1785663880
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add cache for Bcrypt password [pinot]
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #11904:
URL: https://github.com/apache/pinot/pull/11904#discussion_r1387499908
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/BcryptUtils.java:
##########
@@ -18,33 +18,48 @@
*/
package org.apache.pinot.common.utils;
+import com.google.common.cache.Cache;
import org.mindrot.jbcrypt.BCrypt;
+
public class BcryptUtils {
- private static final int DEFALUT_LOG_ROUNDS = 10;
- private static String _bcryptPassword = null;
+ private static final int DEFALUT_LOG_ROUNDS = 10;
+ private static String _bcryptPassword = null;
- private BcryptUtils() {
- }
+ private BcryptUtils() {
+ }
- public static String encrypt(String password) {
- return encrypt(password, DEFALUT_LOG_ROUNDS);
- }
+ public static String encrypt(String password) {
+ return encrypt(password, DEFALUT_LOG_ROUNDS);
+ }
+
+ public static String encrypt(String password, int saltLogRrounds) {
+ _bcryptPassword = BCrypt.hashpw(password, BCrypt.gensalt(saltLogRrounds));
+ return _bcryptPassword;
+ }
- public static String encrypt(String password, int saltLogRrounds) {
- _bcryptPassword = BCrypt.hashpw(password, BCrypt.gensalt(saltLogRrounds));
- return _bcryptPassword;
+ public static boolean checkpw(String pasword, String encrypedPassword) {
+ boolean isMatch = false;
+ try {
+ isMatch = BCrypt.checkpw(pasword, encrypedPassword);
+ } catch (Exception e) {
+ System.out.println(e.getMessage());
+ } finally {
+ return isMatch;
}
+ }
- public static boolean checkpw(String pasword, String encrypedPassword) {
- boolean isMatch = false;
- try {
- isMatch = BCrypt.checkpw(pasword, encrypedPassword);
- } catch (Exception e) {
- System.out.println(e.getMessage());
- } finally {
- return isMatch;
- }
+ public static boolean checkpwWithCache(String password, String encryptedPassword,
+ Cache<String, String> userPasswordAuthCache) {
+ boolean isMatch = true;
+ String cachedPassword = userPasswordAuthCache.getIfPresent(encryptedPassword);
+ if (cachedPassword == null || !cachedPassword.equals(password)) {
+ isMatch = checkpw(password, encryptedPassword);
Review 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".
```json
{
"id" : "admin",
"simpleFields" : {
"password" : "$2a$10$DoCs2UyjBeBk9H7pvZw8kehCa9ot7ofdcF8uKx30PNHdyTyvG4Tiq",
"component" : "CONTROLLER",
"role" : "ADMIN",
"username" : "admin"
},
"mapFields" : { },
"listFields" : { }
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add cache for Bcrypt password [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11904:
URL: https://github.com/apache/pinot/pull/11904#issuecomment-1784717078
We're currently processing your upload. This comment will be updated when the results are available.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add cache for Bcrypt password [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11904:
URL: https://github.com/apache/pinot/pull/11904#discussion_r1382342626
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -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()))
Review Comment:
(code format) Can you apply [Pinot Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#set-up-ide) and reformat all the changes in this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org