You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@accumulo.apache.org by dl...@apache.org on 2023/06/07 20:33:58 UTC
[accumulo] branch 2.1 updated: Use Text instead of ByteBuffer for Cache key
This is an automated email from the ASF dual-hosted git repository.
dlmarion pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/2.1 by this push:
new 5b85dab6a2 Use Text instead of ByteBuffer for Cache key
5b85dab6a2 is described below
commit 5b85dab6a2db4dc2c4a741a913982cd49bb9f31d
Author: Dave Marion <dl...@apache.org>
AuthorDate: Wed Jun 7 20:33:20 2023 +0000
Use Text instead of ByteBuffer for Cache key
---
.../server/security/handler/ZKSecurityTool.java | 35 ++++++---
.../security/handler/ZKSecurityToolTest.java | 86 ++++++++++++++++++++++
2 files changed, 109 insertions(+), 12 deletions(-)
diff --git a/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java b/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java
index 6e13beed6b..156d29fbdd 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java
@@ -25,7 +25,6 @@ import java.io.ByteArrayOutputStream;
import java.io.DataInputStream;
import java.io.DataOutputStream;
import java.io.IOException;
-import java.nio.ByteBuffer;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
@@ -41,12 +40,14 @@ import org.apache.accumulo.core.security.NamespacePermission;
import org.apache.accumulo.core.security.SystemPermission;
import org.apache.accumulo.core.security.TablePermission;
import org.apache.commons.codec.digest.Crypt;
+import org.apache.hadoop.io.Text;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.Scheduler;
+import com.google.common.annotations.VisibleForTesting;
/**
* All the static too methods used for this class, so that we can separate out stuff that isn't
@@ -121,18 +122,17 @@ class ZKSecurityTool {
return cryptHash.getBytes(UTF_8);
}
- private static final Cache<ByteBuffer,String> CRYPT_PASSWORD_CACHE =
+ private static final Cache<Text,String> CRYPT_PASSWORD_CACHE =
Caffeine.newBuilder().scheduler(Scheduler.systemScheduler())
.expireAfterAccess(Duration.ofMinutes(1)).initialCapacity(4).maximumSize(64).build();
// This uses a cache to avoid repeated expensive calls to Crypt.crypt for recent inputs
- public static boolean checkCryptPass(byte[] password, byte[] zkData) {
- final ByteBuffer key = ByteBuffer.allocate(password.length + zkData.length);
- key.put(password);
- key.put(zkData);
- String cryptHash = CRYPT_PASSWORD_CACHE.getIfPresent(key);
- if (cryptHash != null) {
- if (MessageDigest.isEqual(zkData, cryptHash.getBytes(UTF_8))) {
+ public static boolean checkCryptPass(final byte[] password, final byte[] cryptHashInZkToTest) {
+ final Text key = new Text(password);
+ key.append(cryptHashInZkToTest, 0, cryptHashInZkToTest.length);
+ String cachedCryptHash = CRYPT_PASSWORD_CACHE.getIfPresent(key);
+ if (cachedCryptHash != null) {
+ if (MessageDigest.isEqual(cryptHashInZkToTest, cachedCryptHash.getBytes(UTF_8))) {
// If matches then zkData has not changed from when it was put into the cache
return true;
} else {
@@ -141,19 +141,30 @@ class ZKSecurityTool {
}
}
// Either !matches or was not cached
+ String cryptHashToCache;
try {
- cryptHash = Crypt.crypt(password, new String(zkData, UTF_8));
+ cryptHashToCache = Crypt.crypt(password, new String(cryptHashInZkToTest, UTF_8));
} catch (IllegalArgumentException e) {
log.error("Unrecognized hash format", e);
return false;
}
- boolean matches = MessageDigest.isEqual(zkData, cryptHash.getBytes(UTF_8));
+ boolean matches = MessageDigest.isEqual(cryptHashInZkToTest, cryptHashToCache.getBytes(UTF_8));
if (matches) {
- CRYPT_PASSWORD_CACHE.put(key, cryptHash);
+ CRYPT_PASSWORD_CACHE.put(key, cryptHashToCache);
}
return matches;
}
+ @VisibleForTesting
+ static long getCryptPasswordCacheSize() {
+ return CRYPT_PASSWORD_CACHE.estimatedSize();
+ }
+
+ @VisibleForTesting
+ static void clearCryptPasswordCache() {
+ CRYPT_PASSWORD_CACHE.invalidateAll();
+ }
+
public static Authorizations convertAuthorizations(byte[] authorizations) {
return new Authorizations(authorizations);
}
diff --git a/server/base/src/test/java/org/apache/accumulo/server/security/handler/ZKSecurityToolTest.java b/server/base/src/test/java/org/apache/accumulo/server/security/handler/ZKSecurityToolTest.java
new file mode 100644
index 0000000000..a7699404c8
--- /dev/null
+++ b/server/base/src/test/java/org/apache/accumulo/server/security/handler/ZKSecurityToolTest.java
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.server.security.handler;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.apache.commons.codec.digest.Crypt;
+import org.junit.jupiter.api.Test;
+
+public class ZKSecurityToolTest {
+
+ private final String good = "GOODPASSWORD";
+ private final String goodHash = Crypt.crypt(good.getBytes(UTF_8));
+
+ private final String good2 = "GOODPASSWORD2";
+ private final String goodHash2 = Crypt.crypt(good2.getBytes(UTF_8));
+
+ private final String bad = "BADPASSWORD";
+
+ @Test
+ public void testCheckCryptPass() {
+ assertTrue(ZKSecurityTool.checkCryptPass(good.getBytes(UTF_8), goodHash.getBytes(UTF_8)));
+ assertEquals(1L, ZKSecurityTool.getCryptPasswordCacheSize());
+ assertFalse(ZKSecurityTool.checkCryptPass(bad.getBytes(UTF_8), goodHash.getBytes(UTF_8)));
+ assertEquals(1L, ZKSecurityTool.getCryptPasswordCacheSize());
+ assertTrue(ZKSecurityTool.checkCryptPass(good.getBytes(UTF_8), goodHash.getBytes(UTF_8)));
+ assertEquals(1L, ZKSecurityTool.getCryptPasswordCacheSize());
+ assertFalse(ZKSecurityTool.checkCryptPass(bad.getBytes(UTF_8), goodHash.getBytes(UTF_8)));
+ assertEquals(1L, ZKSecurityTool.getCryptPasswordCacheSize());
+
+ // test two things in password cache
+ assertFalse(ZKSecurityTool.checkCryptPass(bad.getBytes(UTF_8), goodHash2.getBytes(UTF_8)));
+ assertEquals(1L, ZKSecurityTool.getCryptPasswordCacheSize());
+ assertTrue(ZKSecurityTool.checkCryptPass(good2.getBytes(UTF_8), goodHash2.getBytes(UTF_8)));
+ assertEquals(2L, ZKSecurityTool.getCryptPasswordCacheSize());
+ assertTrue(ZKSecurityTool.checkCryptPass(good2.getBytes(UTF_8), goodHash2.getBytes(UTF_8)));
+ assertEquals(2L, ZKSecurityTool.getCryptPasswordCacheSize());
+ assertFalse(ZKSecurityTool.checkCryptPass(bad.getBytes(UTF_8), goodHash2.getBytes(UTF_8)));
+ assertEquals(2L, ZKSecurityTool.getCryptPasswordCacheSize());
+ assertTrue(ZKSecurityTool.checkCryptPass(good.getBytes(UTF_8), goodHash.getBytes(UTF_8)));
+ assertEquals(2L, ZKSecurityTool.getCryptPasswordCacheSize());
+
+ // ensure nothing is stored in cache when password does not match what is in ZK
+ ZKSecurityTool.clearCryptPasswordCache();
+ assertEquals(0L, ZKSecurityTool.getCryptPasswordCacheSize());
+ assertFalse(ZKSecurityTool.checkCryptPass(bad.getBytes(UTF_8), goodHash.getBytes(UTF_8)));
+ assertEquals(0L, ZKSecurityTool.getCryptPasswordCacheSize());
+
+ }
+
+ @Test
+ public void testMatchingPasswordNoHash() {
+ ZKSecurityTool.clearCryptPasswordCache();
+ assertEquals(0L, ZKSecurityTool.getCryptPasswordCacheSize());
+ assertTrue(ZKSecurityTool.checkCryptPass(good.getBytes(UTF_8), goodHash.getBytes(UTF_8)));
+ assertEquals(1L, ZKSecurityTool.getCryptPasswordCacheSize());
+
+ String passwordPlusHash = good + goodHash;
+ assertFalse(ZKSecurityTool.checkCryptPass(passwordPlusHash.getBytes(UTF_8), new byte[0]));
+ // This invalidates the existing key because the cache key matches but the hashes do not
+ assertEquals(0L, ZKSecurityTool.getCryptPasswordCacheSize());
+
+ assertTrue(ZKSecurityTool.checkCryptPass(good.getBytes(UTF_8), goodHash.getBytes(UTF_8)));
+ assertEquals(1L, ZKSecurityTool.getCryptPasswordCacheSize());
+
+ }
+}