BukrosSzabolcs commented on a change in pull request #1798:
URL: https://github.com/apache/accumulo/pull/1798#discussion_r528898191
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java
##########
@@ -47,6 +49,7 @@
class ZKSecurityTool {
private static final Logger log =
LoggerFactory.getLogger(ZKSecurityTool.class);
private static final int SALT_LENGTH = 8;
+ private static final Charset CRYPT_CHARSET = Charset.forName("UTF-8");
Review comment:
Switching to `StandardCharsets.UTF_8`
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthenticator.java
##########
@@ -180,18 +222,43 @@ public boolean authenticateUser(String principal,
AuthenticationToken token)
if (!(token instanceof PasswordToken))
throw new AccumuloSecurityException(principal,
SecurityErrorCode.INVALID_TOKEN);
PasswordToken pt = (PasswordToken) token;
- byte[] pass;
+ byte[] zkData;
String zpath = ZKUserPath + "/" + principal;
- pass = zooCache.get(zpath);
- boolean result = ZKSecurityTool.checkPass(pt.getPassword(), pass);
+ zkData = zooCache.get(zpath);
+ boolean result = authenticateUser(principal, pt, zkData);
if (!result) {
zooCache.clear(zpath);
- pass = zooCache.get(zpath);
- result = ZKSecurityTool.checkPass(pt.getPassword(), pass);
+ zkData = zooCache.get(zpath);
+ result = authenticateUser(principal, pt, zkData);
}
return result;
}
+ private boolean authenticateUser(String principal, PasswordToken pt, byte[]
zkData) {
+ if (zkData == null) {
+ return false;
+ }
+
+ // if the hash does not match the outdated format use Crypt to verify it
+ if (!ZKSecurityTool.isOutdatedPass(zkData)) {
+ return ZKSecurityTool.checkCryptPass(pt.getPassword(), zkData);
+ }
+
+ if (!ZKSecurityTool.checkPass(pt.getPassword(), zkData)) {
+ // if password does not match we are done
+ return false;
+ }
+
+ // if the password is correct we have to update the stored hash with new
algorithm
+ try {
+ changePassword(principal, pt);
+ return true;
+ } catch (AccumuloSecurityException e) {
+ log.error("Failed to update hashed user password for user: {}",
principal, e);
+ }
+ return false;
Review comment:
My reasoning was that we are trying to re-hash the password if possible
and on failure we are triggering retry and it should not consistently fail. I
guess the question is how aggressively are we trying to re-hash. It would be
fine for me either way. Please let me know if you would prefer it changed.
##########
File path:
server/base/src/test/java/org/apache/accumulo/server/security/handler/ZKAuthenticatorTest.java
##########
@@ -88,14 +90,40 @@ public void testTableConversion() {
@Test
public void testEncryption() {
+ byte[] rawPass = "myPassword".getBytes(Charset.forName("UTF-8"));
+ byte[] storedBytes;
+ try {
+ storedBytes = ZKSecurityTool.createPass(rawPass.clone());
+ assertTrue(ZKSecurityTool.checkCryptPass(rawPass.clone(), storedBytes));
+ } catch (AccumuloException e) {
+ log.error("{}", e.getMessage(), e);
+ fail();
+ }
Review comment:
Thank you for the suggestion!
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthenticator.java
##########
@@ -116,6 +133,31 @@ public void createUser(String principal,
AuthenticationToken token)
}
}
+ /**
+ * Creates user with outdated password hash for testing
+ *
+ * @deprecated since 2.1.0, only present for testing DO NOT USE!
+ */
Review comment:
Why do you feel a warning is necessary? It's not a widely used method
and the original name was re-used for the new functionality. But if we would
add a safeguard I would prefer to throw an AccumuloSecurityException at the end
of the method. We can catch it in test context and would be harder to miss for
a dev than a log msg. What do you think?
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthenticator.java
##########
@@ -52,6 +52,23 @@ public void initialize(ServerContext context) {
this.context = context;
zooCache = new ZooCache(context.getZooReaderWriter(), null);
ZKUserPath = Constants.ZROOT + "/" + context.getInstanceID() + "/users";
+ checkOutdatedHashes();
+ }
+
+ private void checkOutdatedHashes() {
+ try {
+ listUsers().forEach(user -> {
+ String zpath = ZKUserPath + "/" + user;
+ byte[] zkData = zooCache.get(zpath);
+ if (ZKSecurityTool.isOutdatedPass(zkData)) {
+ log.warn("Found user(s) with outdated password hash. These will be
re-hashed"
+ + " on successful authentication.");
+ return;
+ }
+ });
+ } catch (NullPointerException e) {
+ // initializeSecurity was not called yet, there could be no outdated
passwords stored
Review comment:
The zknode missing should be a corner case so I would prefer to keep the
try/catch. I'll add a debug log message.
##########
File path: core/src/main/java/org/apache/accumulo/core/Constants.java
##########
@@ -99,7 +99,8 @@
public static final long SCANNER_DEFAULT_READAHEAD_THRESHOLD = 3L;
// Security configuration
- public static final String PW_HASH_ALGORITHM = "SHA-256";
+ public static final String PW_HASH_ALGORITHM = "SHA-512";
Review comment:
You are right. I'll make the system credentials hash customizable while
defaulting to 256, and renaming this as suggested to reflect it's for unsecure
use.
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java
##########
@@ -56,16 +59,26 @@
return salt;
}
+ // only present for testing DO NOT USE!
+ @Deprecated(since = "2.1.0")
+ static byte[] createOutdatedPass(byte[] password) throws AccumuloException {
+ byte[] salt = generateSalt();
+ try {
+ return convertPass(password, salt);
+ } catch (NoSuchAlgorithmException e) {
+ log.error("Count not create hashed password", e);
+ throw new AccumuloException("Count not create hashed password", e);
+ }
+ }
+
private static byte[] hash(byte[] raw) throws NoSuchAlgorithmException {
- MessageDigest md = MessageDigest.getInstance(Constants.PW_HASH_ALGORITHM);
+ MessageDigest md =
MessageDigest.getInstance(Constants.PW_HASH_ALGORITHM_OUTDATED);
md.update(raw);
return md.digest();
}
+ @Deprecated(since = "2.1.0")
public static boolean checkPass(byte[] password, byte[] zkData) {
- if (zkData == null)
- return false;
-
Review comment:
I re-added the check to be on the safe side, but and made it package
private as I should have done already.
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java
##########
@@ -99,6 +102,24 @@ public static boolean checkPass(byte[] password, byte[]
zkData) {
return saltedHash; // contains salt+hash(password+salt)
}
+ public static byte[] createPass(byte[] password) throws AccumuloException {
+ // we rely on default algorithm and hash length (SHA-512 and 8 byte)
+ String cryptHash = Crypt.crypt(password);
+ return cryptHash.getBytes(CRYPT_CHARSET);
+ }
+
+ public static boolean checkCryptPass(byte[] password, byte[] zkData) {
+ String zkDataString = new String(zkData, CRYPT_CHARSET);
+ String cryptHash;
+ try {
+ cryptHash = Crypt.crypt(password, zkDataString);
+ } catch (IllegalArgumentException e) {
+ log.error("Unrecognized hash format", e);
+ return false;
+ }
+ return MessageDigest.isEqual(zkData, cryptHash.getBytes(CRYPT_CHARSET));
Review comment:
String comparisons is not secure against timing attacks,
`MessageDigest.isEqual` is.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
