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 2020/11/23 13:40:06 UTC
[GitHub] [accumulo] busbey commented on a change in pull request #1798: Hash upgrade
busbey commented on a change in pull request #1798:
URL: https://github.com/apache/accumulo/pull/1798#discussion_r528697020
##########
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:
log a DEBUG message with these details.
##########
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:
if we fail to update the password for some reason (like a transient zk write failure), at this point shouldn't we still return that they correctly authenticated?
##########
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:
We still need this constant updated because we rely on it for hashing the system credentials? won't that prevent a rolling upgrade?
Could we have system credentials fall back to SHA-256 with a warning? or require a configurable flag to switch it?
##########
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:
add a comment about why a new Charset instead of relying on `StandardCharsets.UTF_8`
##########
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:
changing this is also going to change some non-security uses, e.g. we optionally use it to obscure values printed from rfile metrics gathering. We'll need to enumerate these and release note the change in behavior. (or we could make something like a `NON_CRYPTO_USE_HASH_ALGORITHM` that we keep as SHA-256)
##########
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:
could we log a WARN message that this method has been used? that way it would show up in operator logs should we mistakenly use it in a non-test context.
##########
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.");
Review comment:
since we aren't including any details about which user(s) are impacted, I'd rather we not get a WARN for each user. could we move this to after we finish iterating with a summary of how many users?
If an operator needed to move towards eliminating these warn messages, how would they get the list of users that need to authenticate to the system?
##########
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:
just to make sure I understand the reasoning here, the removal of this check is because ZKSecurityTool is package private and all current calls ensure zkData isn't null?
if that's correct please add javadocs that say zkData can't be null. an alternative is to leave the check in place and rely on the jit to optimize it away.
----------------------------------------------------------------
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:
users@infra.apache.org