You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/05/16 13:45:45 UTC
[GitHub] [accumulo] dlmarion opened a new pull request, #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
dlmarion opened a new pull request, #2707:
URL: https://github.com/apache/accumulo/pull/2707
Apache Commons Codec Crypt.crypt() can be expensive as it creates
numerous MessageDigest objects. As an optimization, cache the last
16 unique inputs to the ZKSecurityTool.checkCryptPass method that
result in a positive return value.
Related to #2700
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r873878195
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +120,27 @@ public static byte[] createPass(byte[] password) throws AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Map<List<Byte>,String> CHECKED_CRYPT_PASSWORDS =
Review Comment:
Maybe the Bytes.asList() function makes everything efficient under the covers. I did not really look at that. It would depend on its hashcode and equals functions. If this are just using byte[], then that would address my concerns.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2707: Cached last 64 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1134973368
> I haven't looked into them being stored in the Session you're referring to, so I don't have an opinion about that.
@ctubbsii - TCredentials are referenced in [Session](https://github.com/apache/accumulo/blob/main/server/tserver/src/main/java/org/apache/accumulo/tserver/session/Session.java#L34) and stored in the SessionManager.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r873811513
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +120,27 @@ public static byte[] createPass(byte[] password) throws AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Map<List<Byte>,String> CHECKED_CRYPT_PASSWORDS =
Review Comment:
Hadoop's Text or Accumulo's ByteSequence would probably be more efficient for the key than List<Byte>.
```suggestion
private static final Map<ByteSequence,String> CHECKED_CRYPT_PASSWORDS =
```
Also thinking a guava cache w/ extremely short TTL (like 1 min) would be better than fixed size lrumap. An extremely short duration should still amortize the cost nicely.
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +120,27 @@ public static byte[] createPass(byte[] password) throws AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Map<List<Byte>,String> CHECKED_CRYPT_PASSWORDS =
+ Collections.synchronizedMap(new LRUMap<>(16));
+
public static boolean checkCryptPass(byte[] password, byte[] zkData) {
+ List<Byte> key = Bytes.asList(password);
String zkDataString = new String(zkData, UTF_8);
+ if (CHECKED_CRYPT_PASSWORDS.getOrDefault(key, "").equals(zkDataString)) {
+ return true;
+ }
String cryptHash;
try {
cryptHash = Crypt.crypt(password, zkDataString);
Review Comment:
Feels more safe to me if we explicitly cache the input and output of the cryptHash function. So something like the followin psuedocode (not sure of the best way to write it w/o looking at cache APIs)
```
var key = password + zkData
cryptHash = getFromCache(key)
if(cryptHash == null) {
cryptHash = Crypt.crypt(password, zkDataString);
putInCache(key, cryptHash);
}
boolean matches = MessageDigest.isEqual(zkData, cryptHash.getBytes(UTF_8));
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2707: Cached last 64 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r882241112
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -157,6 +157,13 @@ public enum Property {
"The permission handler class that accumulo will use to determine if a "
+ "user has privilege to perform an action",
"1.5.0"),
+ INSTANCE_SECURITY_ZK_AUTH_CACHE_ENABLED("instance.security.authenticator.zk.cache.enabled",
+ "true", PropertyType.BOOLEAN,
+ "Enables the temporary caching of successfully authenticated"
+ + " user passwords in org.apache.accumulo.server.security.handler.ZKAuthenticator to"
+ + " mitigate the performance penalties of having to compute the password hash"
+ + " on every API call",
+ "2.1.0"),
Review Comment:
Since this is an internal optimization, I don't want to bloat users with extra configuration. If in future, we evaluate this again, and it's no longer needed due to improvements in the JDK or in commons-codec, then this property will become OBE, and we'll have churn removing it.
Also, it only applies to ZKAuthenticator, which is itself configurable. If they don't want this, they can just replace ZKAuthenticator with a different custom authenticator. So, there's already a control knob for this that is available to users. This option is effectively redundant, and it's confusing if they've used a custom authenticator already.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r874711539
##########
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:
> 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.
I think that is what is described in #2700 as part of a test. I could see real work implications, typically on application startup where you have a thundering herd type situation.
> 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.
This was based on an earlier comment about have really short times in the cache. I think at this point it could be longer and based on access rather than write.
> I'm not sure this is worth it. The underlying issue seems highly dependent on specific JVM versions, and specific JCE providers and their native instruction hardware optimizations.
I would agree, but I would also say that we are doing work that we might not have to do.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r873873046
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +120,27 @@ public static byte[] createPass(byte[] password) throws AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Map<List<Byte>,String> CHECKED_CRYPT_PASSWORDS =
+ Collections.synchronizedMap(new LRUMap<>(16));
+
public static boolean checkCryptPass(byte[] password, byte[] zkData) {
+ List<Byte> key = Bytes.asList(password);
String zkDataString = new String(zkData, UTF_8);
+ if (CHECKED_CRYPT_PASSWORDS.getOrDefault(key, "").equals(zkDataString)) {
+ return true;
+ }
String cryptHash;
try {
cryptHash = Crypt.crypt(password, zkDataString);
Review Comment:
Was just thinking very literally and trying to avoid shortcuts since its security related. The function takes two inputs and gives a single output, so was thinking of caching that functions input and output exactly rather than caching a partial was more secure. Caching only a single input parameter (the password) felt kind dicey. Like what if two account have the same password with different salts, then what will happen with 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r873841405
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +120,27 @@ public static byte[] createPass(byte[] password) throws AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Map<List<Byte>,String> CHECKED_CRYPT_PASSWORDS =
Review Comment:
I'll wait to hear back from @ctubbsii before I update this. Your approach may be better in the case of a password change.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r874734586
##########
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:
I changed the key type from String to ByteBuffer in 67e6332
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2707: Cached last 64 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1135631769
> > I haven't looked into them being stored in the Session you're referring to, so I don't have an opinion about that.
>
> @ctubbsii - TCredentials are referenced in [Session](https://github.com/apache/accumulo/blob/main/server/tserver/src/main/java/org/apache/accumulo/tserver/session/Session.java#L34) and stored in the SessionManager.
Yeah, it looks like the credentials are kept there to re-authenticate a user and check their permissions for each new table the user attempts to send mutations to during a batch write session.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r874076518
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +119,38 @@ 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;
- try {
- cryptHash = Crypt.crypt(password, zkDataString);
- } catch (IllegalArgumentException e) {
- log.error("Unrecognized hash format", e);
- return false;
+ String key = new String(password, UTF_8) + zkDataString;
+ String cryptHash = CRYPT_PASSWORD_CACHE.getIfPresent(key);
+ boolean matches = false;
+ if (cryptHash != null) {
+ matches = MessageDigest.isEqual(zkData, cryptHash.getBytes(UTF_8));
+ // If matches then zkData has not changed from when it was put into the cache
+ if (matches) {
+ return true;
+ } else {
+ // remove the non-matching entry from the cache
+ CRYPT_PASSWORD_CACHE.invalidate(key);
+ }
+ }
+ // Either !matches or was not cached
+ if (!matches) {
+ try {
+ cryptHash = Crypt.crypt(password, zkDataString);
+ } catch (IllegalArgumentException e) {
+ log.error("Unrecognized hash format", e);
+ return false;
+ }
+ }
+ matches = MessageDigest.isEqual(zkData, cryptHash.getBytes(UTF_8));
+ if (matches) {
+ CRYPT_PASSWORD_CACHE.put(key, cryptHash);
Review Comment:
Looking at the `if (!matches) {` case it seems like if execution got to that point in the code that it would always be true. If that is the case, then could remove `if (!matches) {`.
```suggestion
if (cryptHash != null) {
// If matches then zkData has not changed from when it was put into the cache
if ( MessageDigest.isEqual(zkData, cryptHash.getBytes(UTF_8))) {
return true;
} else {
// remove the non-matching entry from the cache
CRYPT_PASSWORD_CACHE.invalidate(key);
}
}
try {
cryptHash = Crypt.crypt(password, zkDataString);
} catch (IllegalArgumentException e) {
log.error("Unrecognized hash format", e);
return false;
}
boolean matches = MessageDigest.isEqual(zkData, cryptHash.getBytes(UTF_8));
if (matches) {
CRYPT_PASSWORD_CACHE.put(key, cryptHash);
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r874885492
##########
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:
@keith-turner thinks he has a solution for the other locking issues he saw in #2700 in the ScanServer. I'll suggest to him that he test with his solution first to see if this is still an issue, then test with this to see if it solves it. I'll wait to merge this until we hear back.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r873881837
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +120,27 @@ public static byte[] createPass(byte[] password) throws AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Map<List<Byte>,String> CHECKED_CRYPT_PASSWORDS =
+ Collections.synchronizedMap(new LRUMap<>(16));
+
public static boolean checkCryptPass(byte[] password, byte[] zkData) {
+ List<Byte> key = Bytes.asList(password);
String zkDataString = new String(zkData, UTF_8);
+ if (CHECKED_CRYPT_PASSWORDS.getOrDefault(key, "").equals(zkDataString)) {
+ return true;
+ }
String cryptHash;
try {
cryptHash = Crypt.crypt(password, zkDataString);
Review Comment:
> Ah, I see. I didn't think about the password reuse case. I'll adjust.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r874762778
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +120,36 @@ public static byte[] createPass(byte[] password) throws AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Cache<ByteBuffer,String> CRYPT_PASSWORD_CACHE =
+ Caffeine.newBuilder().scheduler(Scheduler.systemScheduler())
+ .expireAfterAccess(Duration.ofMinutes(1)).initialCapacity(4).maximumSize(64).build();
+
public static boolean checkCryptPass(byte[] password, byte[] zkData) {
Review Comment:
A brief comment stating the overall intent would be nice. The logic makes sense now, but I realized that in future, somebody could wonder why we're bothering to do this.
```suggestion
// This uses a cache to avoid repeated expensive calls to Crypt.crypt for recent inputs
public static boolean checkCryptPass(byte[] password, byte[] zkData) {
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1129467314
There's probably a bit of a difference between not yet garbage collected objects and a well-known data structure containing them in a central place for a period of time. I haven't looked into them being stored in the Session you're referring to, so I don't have an opinion about that. It's just that the relatively casual approach to hanging onto credentials here for performance seems to contrast with the aggressive approach to clearing security-related fields that you proposed in #2616, and I'm not sure how to reconcile the two approaches with a single coherent "security" principle.
Overall, I'm more fine than not with these changes... just not `+1` enthusiastic about it. I'm only comfortable enough to give it a `+0` from me.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
keith-turner commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1130599393
I circled back and did my test and these changes resulted in a really significant improvement. I was running [this bit of code](https://github.com/keith-turner/accumulo-testing/blob/scan-server-testing/src/main/java/org/apache/accumulo/testing/continuous/ContinuousQuery.java) that I created to do random queries against CI data. I ran the code with parameters such that it would do small random queries in a single tablet using 500 threads. Below are the results which show the average times and entries returned for every 1000 scans done. With these changes and 500 concurrent queries the average time is 400ms and the max times are around 1000ms (ignoring the first few lines because JIT probably has not kicked in scan servers). Without these changes the average times are around 1600ms to 1700ms and the max times 15,000ms to 20,000ms.
```
With 2 scan server lock contention fixes, with the sha512 cache :
hadoop@manager:/opt/accumulo-testing/sources/accumulo-testing-repo$ ./bin/cingest query 733333333333333e 79999999999999a5 16 500 2>&1 | grep STAT
2022-05-18T21:41:01,226 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:7263 avgTime:1717.988 minResults:0 maxResults:98 avgResults:22.314
2022-05-18T21:41:02,333 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:8380 avgTime:871.568 minResults:0 maxResults:98 avgResults:23.173
2022-05-18T21:41:03,221 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:9268 avgTime:1399.021 minResults:0 maxResults:97 avgResults:23.728
2022-05-18T21:41:04,003 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:25 maxTime:9967 avgTime:620.181 minResults:0 maxResults:94 avgResults:22.873
2022-05-18T21:41:04,736 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:33 maxTime:10187 avgTime:480.048 minResults:0 maxResults:101 avgResults:22.382
2022-05-18T21:41:05,482 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:38 maxTime:10965 avgTime:533.736 minResults:0 maxResults:106 avgResults:22.628
2022-05-18T21:41:06,250 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:30 maxTime:11534 avgTime:374.372 minResults:0 maxResults:94 avgResults:22.656
2022-05-18T21:41:07,038 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:18 maxTime:999 avgTime:402.28 minResults:0 maxResults:94 avgResults:23.957
2022-05-18T21:41:07,857 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:22 maxTime:962 avgTime:381.623 minResults:0 maxResults:95 avgResults:22.933
2022-05-18T21:41:08,688 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:15 maxTime:951 avgTime:420.283 minResults:0 maxResults:93 avgResults:21.669
2022-05-18T21:41:09,494 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:25 maxTime:1133 avgTime:417.134 minResults:0 maxResults:91 avgResults:22.704
2022-05-18T21:41:10,262 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:18 maxTime:998 avgTime:379.985 minResults:0 maxResults:103 avgResults:22.826
2022-05-18T21:41:11,063 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:15 maxTime:1174 avgTime:391.562 minResults:0 maxResults:98 avgResults:22.415
2022-05-18T21:41:11,861 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:21 maxTime:1005 avgTime:405.976 minResults:0 maxResults:91 avgResults:21.523
2022-05-18T21:41:12,669 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:34 maxTime:1061 avgTime:403.049 minResults:0 maxResults:105 avgResults:22.965
2022-05-18T21:41:13,430 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:17 maxTime:1077 avgTime:377.985 minResults:0 maxResults:98 avgResults:22.559
2022-05-18T21:41:14,223 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:21 maxTime:1003 avgTime:392.969 minResults:0 maxResults:112 avgResults:22.2
2022-05-18T21:41:14,986 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:18 maxTime:923 avgTime:383.816 minResults:0 maxResults:93 avgResults:22.59
2022-05-18T21:41:15,759 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:15 maxTime:970 avgTime:385.021 minResults:0 maxResults:97 avgResults:21.352
2022-05-18T21:41:16,562 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:11 maxTime:1098 avgTime:401.247 minResults:0 maxResults:104 avgResults:23.041
2022-05-18T21:41:17,336 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:11 maxTime:1074 avgTime:387.938 minResults:0 maxResults:105 avgResults:23.56
2022-05-18T21:41:18,131 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:19 maxTime:1522 avgTime:400.732 minResults:0 maxResults:104 avgResults:23.852
2022-05-18T21:41:18,890 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:24 maxTime:1347 avgTime:377.983 minResults:0 maxResults:90 avgResults:22.291
2022-05-18T21:41:19,669 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:16 maxTime:2075 avgTime:379.598 minResults:0 maxResults:97 avgResults:24.757
With 2 scan server lock contention fixes, Without the sha512 cache :
hadoop@manager:/opt/accumulo-testing/sources/accumulo-testing-repo$ ./bin/cingest query 733333333333333e 79999999999999a5 16 500 2>&1 | grep STAT
2022-05-18T21:13:07,128 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:9294 avgTime:2934.887 minResults:0 maxResults:95 avgResults:21.559
2022-05-18T21:13:11,032 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:12401 avgTime:2558.46 minResults:0 maxResults:95 avgResults:22.671
2022-05-18T21:13:14,814 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:15932 avgTime:1787.61 minResults:0 maxResults:106 avgResults:22.142
2022-05-18T21:13:18,361 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:19075 avgTime:1757.055 minResults:0 maxResults:95 avgResults:22.1
2022-05-18T21:13:21,627 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:14242 avgTime:1787.193 minResults:0 maxResults:108 avgResults:23.424
2022-05-18T21:13:24,935 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:15593 avgTime:1668.114 minResults:0 maxResults:92 avgResults:22.891
2022-05-18T21:13:28,543 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:14493 avgTime:1664.027 minResults:0 maxResults:94 avgResults:24.173
2022-05-18T21:13:31,744 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:3 maxTime:14075 avgTime:1452.26 minResults:0 maxResults:93 avgResults:22.112
2022-05-18T21:13:35,330 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:15807 avgTime:1659.093 minResults:0 maxResults:103 avgResults:23.123
2022-05-18T21:13:38,536 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:19485 avgTime:1626.846 minResults:0 maxResults:103 avgResults:22.99
2022-05-18T21:13:42,135 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:21982 avgTime:1709.725 minResults:0 maxResults:103 avgResults:22.758
2022-05-18T21:13:45,235 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:17912 avgTime:1797.937 minResults:0 maxResults:91 avgResults:23.651
2022-05-18T21:13:48,547 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:16882 avgTime:1488.331 minResults:0 maxResults:104 avgResults:23.036
2022-05-18T21:13:51,931 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:12888 avgTime:1337.462 minResults:0 maxResults:107 avgResults:23.906
2022-05-18T21:13:55,143 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:20292 avgTime:1833.693 minResults:0 maxResults:100 avgResults:22.504
2022-05-18T21:13:58,369 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:18291 avgTime:1550.546 minResults:0 maxResults:100 avgResults:23.073
2022-05-18T21:14:01,646 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:19390 avgTime:1840.628 minResults:0 maxResults:93 avgResults:21.93
2022-05-18T21:14:05,563 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:16744 avgTime:2057.819 minResults:0 maxResults:85 avgResults:21.94
2022-05-18T21:14:08,630 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:17751 avgTime:1918.439 minResults:0 maxResults:94 avgResults:23.095
2022-05-18T21:14:12,124 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:24605 avgTime:1785.754 minResults:0 maxResults:106 avgResults:22.681
2022-05-18T21:14:15,330 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:12315 avgTime:1558.236 minResults:0 maxResults:100 avgResults:23.369
Without 2 scan server lock contention fixes, Without the sha512 cache :
hadoop@manager:/opt/accumulo-testing/sources/accumulo-testing-repo$ ./bin/cingest query 733333333333333e 79999999999999a5 16 500 2>&1 | grep STAT
2022-05-18T21:26:54,209 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:9955 avgTime:3377.346 minResults:0 maxResults:104 avgResults:24.251
2022-05-18T21:26:58,112 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:12942 avgTime:2398.26 minResults:0 maxResults:101 avgResults:22.639
2022-05-18T21:27:01,732 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:16860 avgTime:1872.668 minResults:0 maxResults:93 avgResults:22.595
2022-05-18T21:27:05,113 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:12454 avgTime:1858.168 minResults:0 maxResults:89 avgResults:22.467
2022-05-18T21:27:08,592 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:13245 avgTime:1816.611 minResults:0 maxResults:109 avgResults:23.52
2022-05-18T21:27:11,941 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:10074 avgTime:1577.603 minResults:0 maxResults:98 avgResults:23.139
2022-05-18T21:27:15,537 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:10367 avgTime:1617.97 minResults:0 maxResults:99 avgResults:23.271
2022-05-18T21:27:18,940 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:16847 avgTime:1717.488 minResults:0 maxResults:103 avgResults:23.523
2022-05-18T21:27:22,121 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:3 maxTime:11905 avgTime:1754.433 minResults:0 maxResults:96 avgResults:23.501
2022-05-18T21:27:25,418 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:11685 avgTime:1628.917 minResults:0 maxResults:107 avgResults:22.256
2022-05-18T21:27:28,822 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:10381 avgTime:1533.018 minResults:0 maxResults:98 avgResults:21.994
2022-05-18T21:27:31,979 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:13414 avgTime:1715.61 minResults:0 maxResults:105 avgResults:22.88
2022-05-18T21:27:35,447 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:12916 avgTime:1939.198 minResults:0 maxResults:105 avgResults:22.622
2022-05-18T21:27:38,921 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:15182 avgTime:1690.404 minResults:0 maxResults:113 avgResults:23.795
2022-05-18T21:27:42,426 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:13339 avgTime:1672.008 minResults:0 maxResults:96 avgResults:21.484
2022-05-18T21:27:46,246 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:10578 avgTime:1716.462 minResults:0 maxResults:96 avgResults:22.72
2022-05-18T21:27:49,048 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:13267 avgTime:1575.544 minResults:0 maxResults:92 avgResults:23.481
2022-05-18T21:27:52,529 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:10375 avgTime:1833.692 minResults:0 maxResults:99 avgResults:23.189
2022-05-18T21:27:55,927 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:11099 avgTime:1505.09 minResults:0 maxResults:101 avgResults:22.699
2022-05-18T21:27:59,122 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:12778 avgTime:1692.452 minResults:0 maxResults:93 avgResults:22.965
2022-05-18T21:28:02,413 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:19010 avgTime:1612.813 minResults:0 maxResults:98 avgResults:22.725
2022-05-18T21:28:05,514 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:12986 avgTime:1662.435 minResults:0 maxResults:89 avgResults:22.258
2022-05-18T21:28:08,849 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:13979 avgTime:1559.41 minResults:0 maxResults:95 avgResults:22.965
2022-05-18T21:28:12,458 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:4 maxTime:13302 avgTime:1701.877 minResults:0 maxResults:93 avgResults:22.371
2022-05-18T21:28:15,937 [testing.continuous.ContinuousScanner] INFO : STATS count:1000 minTime:5 maxTime:11030 avgTime:1794.829 minResults:0 maxResults:97 avgResults:21.826
```
Also another interesting data point is that when I profile without these changes all of the hottest methods are sha512 related in addition to the lock contention problem with getting the sha512 provider, do not see any Accumulo scan code in the hot methods. With these changes the hottest methods in profiling are all Accumulo scan code and there is not much lock contention (just a little bit around the secure random used to generate session ids).
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r874753972
##########
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:
> > 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.
>
> I think that is what is described in #2700 as part of a test. I could see real work implications, typically on application startup where you have a thundering herd type situation.
I suppose. I'm just not sure how well the observations of lock contention or brief periods of high CPU usage translate to end user experience, or overall user application performance. This isn't much complexity, but in general, I'm resistant to specialized code in our application that tries to work around performance limitations in user hardware/external libraries, just because of separation of responsibilities. I'm also resistant to workarounds that cater to specific deployment use cases when better deployment decisions (like choosing a better JCE library or deploying using hardware with crypto extensions) could also address the problem. This hits on both those points, and it's not clear it's even noticeable to users. But, the workaround is simple enough, so maybe it's okay.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r873869514
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +120,27 @@ public static byte[] createPass(byte[] password) throws AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Map<List<Byte>,String> CHECKED_CRYPT_PASSWORDS =
Review Comment:
> Hadoop's Text or Accumulo's ByteSequence would probably be more efficient for the key than List.
Why do you say that? LRUMap is based on a hash map, and the list returned by `Bytes.asList()` implements `hashCode()`.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1129859495
> It's just that the relatively casual approach to hanging onto credentials here for performance seems to contrast with the aggressive approach to clearing security-related fields that you proposed in https://github.com/apache/accumulo/pull/2616, and I'm not sure how to reconcile the two approaches with a single coherent "security" principle.
#2616 is about the encryption keys, but your point is fair. If @keith-turner's solution to the locking problem in #2700 is enough to solve the problem in the ScanServer and this is not needed, then I think we should not merge it. If this does help, I don't see it as any worse than storing the credentials in the likely longer-lived Session objects.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
keith-turner commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1128047593
> No, I didn't. Everything in ZKSecurityTool is static and I wanted to encapsulate this cache as much as possible
I don't like adding more static state, but I do agree with trying to encapsulate this cache as much as possible. Those are contradictory goals, and I am not sure which is better overall.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r874081618
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +119,38 @@ 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;
- try {
- cryptHash = Crypt.crypt(password, zkDataString);
- } catch (IllegalArgumentException e) {
- log.error("Unrecognized hash format", e);
- return false;
+ String key = new String(password, UTF_8) + zkDataString;
+ String cryptHash = CRYPT_PASSWORD_CACHE.getIfPresent(key);
+ boolean matches = false;
+ if (cryptHash != null) {
+ matches = MessageDigest.isEqual(zkData, cryptHash.getBytes(UTF_8));
+ // If matches then zkData has not changed from when it was put into the cache
+ if (matches) {
+ return true;
+ } else {
+ // remove the non-matching entry from the cache
+ CRYPT_PASSWORD_CACHE.invalidate(key);
+ }
+ }
+ // Either !matches or was not cached
+ if (!matches) {
+ try {
+ cryptHash = Crypt.crypt(password, zkDataString);
+ } catch (IllegalArgumentException e) {
+ log.error("Unrecognized hash format", e);
+ return false;
+ }
+ }
+ matches = MessageDigest.isEqual(zkData, cryptHash.getBytes(UTF_8));
+ if (matches) {
+ CRYPT_PASSWORD_CACHE.put(key, cryptHash);
Review Comment:
I think that's true. I'll apply your changes manually, it doesn't look like it will pass the formatter check
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r873878195
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +120,27 @@ public static byte[] createPass(byte[] password) throws AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Map<List<Byte>,String> CHECKED_CRYPT_PASSWORDS =
Review Comment:
Maybe the Bytes.asList() function makes everything efficient under the covers. I did not really look at that. It would depend on its hashcode and equals functions. If those are just using byte[], then that would address my concerns.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2707: Cached last 64 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1131610002
@milleruntime -`ZKSecurityTool` is used by `ZKAuthenticator.authenticateUser`, which is called by `SecurityOperation.authenticateUser`. It does not appear to call this if the user is the "system" user.
> If it is just for users, then will this cache only be effective if you have about 16 users?
The description was out of date, it currently caches up to 64 passwords for 1 minute.
> What about the use case where the query application uses the same app user?
This should still work. Not sure I understand the concern or question.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2707: Cached last 64 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1131716539
> I am thinking of Datawave, which I thought uses the same Accumulo user for all scans. But I don't remember how the internals work.
So, Datawave does use the same user for all scans, but each connection is still authenticated. I think where this change will help is with application startup when you potentially have a thundering herd effect of a bunch of connections being created at once. Same is true with an application that uses short-lived on-demand connections.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r873841405
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +120,27 @@ public static byte[] createPass(byte[] password) throws AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Map<List<Byte>,String> CHECKED_CRYPT_PASSWORDS =
Review Comment:
I'll wait to hear back from @ctubbsii before I update this. The short TTL approach may be better in the case of a password change.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1127857175
> @dlmarion did you look into making the cache non-static? Not sure of the overall context, wondering if its possible to pass in a server context that may have the cache. Not sure if that is a good thing to do.
No, I didn't. Everything in ZKSecurityTool is static and I wanted to encapsulate this cache as much as possible
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1128959546
> I'm still uncertain that caching recently received passwords is a great idea... but implementation seems fine.
I thought about it too, from a security perspective. I became ok with it when I thought about the fact that they were likely already in memory anyway, at least until all of the unreferenced objects from the authenticateUser call were garbage collected. Not to mention user credentials are also stored in all Session objects.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1131582897
It looks like this tool is only used for authenticating users? Any idea if this is also used for servers? If it is just for users, then will this cache only be effective if you have about 16 users? What about the use case where the query application uses the same app user?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion merged pull request #2707: Cached last 64 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion merged PR #2707:
URL: https://github.com/apache/accumulo/pull/2707
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r873839467
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +120,27 @@ public static byte[] createPass(byte[] password) throws AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Map<List<Byte>,String> CHECKED_CRYPT_PASSWORDS =
+ Collections.synchronizedMap(new LRUMap<>(16));
+
public static boolean checkCryptPass(byte[] password, byte[] zkData) {
+ List<Byte> key = Bytes.asList(password);
String zkDataString = new String(zkData, UTF_8);
+ if (CHECKED_CRYPT_PASSWORDS.getOrDefault(key, "").equals(zkDataString)) {
+ return true;
+ }
String cryptHash;
try {
cryptHash = Crypt.crypt(password, zkDataString);
Review Comment:
I was thinking that we would not want to store the output of any crypt function. Instead, I was just capturing the fact that two pieces of information had returned a true result in the recent past, and using it to short circuit the method. Is there any benefit to recalculating whether the two hashes are equal?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r874736575
##########
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:
In 67e6332 I changed the expiration criteria from afterWrite to afterAccess, put a max size on the cache to 64, and changed the expiration time from 3s to 1m based on a comment earlier by @keith-turner
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r873876190
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java:
##########
@@ -114,16 +120,27 @@ public static byte[] createPass(byte[] password) throws AccumuloException {
return cryptHash.getBytes(UTF_8);
}
+ private static final Map<List<Byte>,String> CHECKED_CRYPT_PASSWORDS =
Review Comment:
> Why do you say that?
An array of Byte is an array of object pointers to Byte objects, so unless the JVM does something to optimize it then its much less efficient than an array of byte primitives w/o any pointers. Text and ByteSequence just wrap byte[].
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #2707: Cached last 16 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
keith-turner commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1127835426
@dlmarion did you look into making the cache non-static? Not sure of the overall context, wondering if its possible to pass in a server context that may have the cache. Not sure if that is a good thing to do.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2707: Cached last 64 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#issuecomment-1131614539
> > What about the use case where the query application uses the same app user?
>
> This should still work. Not sure I understand the concern or question.
I am thinking of Datawave, which I thought uses the same Accumulo user for all scans. But I don't remember how the internals work.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2707: Cached last 64 successful ZKSecurityTool.checkCryptPass password checks
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r882570081
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -157,6 +157,13 @@ public enum Property {
"The permission handler class that accumulo will use to determine if a "
+ "user has privilege to perform an action",
"1.5.0"),
+ INSTANCE_SECURITY_ZK_AUTH_CACHE_ENABLED("instance.security.authenticator.zk.cache.enabled",
+ "true", PropertyType.BOOLEAN,
+ "Enables the temporary caching of successfully authenticated"
+ + " user passwords in org.apache.accumulo.server.security.handler.ZKAuthenticator to"
+ + " mitigate the performance penalties of having to compute the password hash"
+ + " on every API call",
+ "2.1.0"),
Review Comment:
I reverted the commit that added this property
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org