busbey commented on a change in pull request #1798:
URL: https://github.com/apache/accumulo/pull/1798#discussion_r530373941
##########
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:
I think this comes down to wether we want to fail open or closed for
updating to a more secure hash. I personally prefer failing open in this case
-- essentially if there's a problem writing to ZK we did not consider here then
the system will keep working as before the upgrade but with a persistent log
message an operator could use to go fix things.
I could see at least one argument for failing closed: a person or system not
being able to talk to Accumulo will be a very loud noise that will ensure
whatever is preventing us from storing things more securely gets handled.
Presuming secure hashing of password data is an important system requirement in
a particular Accumulo deployment that is a good thing.
Does anyone else feel strongly?
----------------------------------------------------------------
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]
