ctubbsii commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r874147597
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +119,34 @@ public static byte[] createPass(byte[] password) throws
AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Cache<String,String> CRYPT_PASSWORD_CACHE =
Caffeine.newBuilder()
+ .scheduler(Scheduler.systemScheduler()).expireAfterWrite(3,
TimeUnit.SECONDS).build();
+
public static boolean checkCryptPass(byte[] password, byte[] zkData) {
- String zkDataString = new String(zkData, UTF_8);
- String cryptHash;
+ final String zkDataString = new String(zkData, UTF_8);
+ final String key = new String(password, UTF_8) + zkDataString;
Review Comment:
Is there a risk of a contrived password manipulating this cache key? If so,
can it be avoided by swapping the structured zkDataString and the password?
What happens if the password byte array can't be encoded as UTF-8? Would it be
better to use a key based on a byte buffer or Text object, or a fast
hex-encoding of the byte array?
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +119,34 @@ public static byte[] createPass(byte[] password) throws
AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Cache<String,String> CRYPT_PASSWORD_CACHE =
Caffeine.newBuilder()
+ .scheduler(Scheduler.systemScheduler()).expireAfterWrite(3,
TimeUnit.SECONDS).build();
Review Comment:
Wouldn't we want expireAfterAccess rather than write? If it's still
frequently being accessed, it'd be good to keep it around, regardless of the
last time it was written to the cache.
It also might be good to put a cap on the size of this cache.
For this to be of benefit, we'd have to be creating a lot of connections
pretty quickly. I'm not sure how realistic that would be. Does 3 seconds cover
a realistic scenario? Should it be 10? More? Maybe a size-limited LRU cache
would be better?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
