ctubbsii commented on a change in pull request #1798:
URL: https://github.com/apache/accumulo/pull/1798#discussion_r529827988
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthenticator.java
##########
@@ -54,6 +56,33 @@ public void initialize(ServerContext context) {
ZKUserPath = Constants.ZROOT + "/" + context.getInstanceID() + "/users";
}
+ /**
+ * Checks stored users and logs a warning containing the ones with outdated
hashes.
+ */
+ public boolean hasOutdatedHashes() {
Review comment:
If it makes more readable code in the callers (fewer negations), you
could also flip the return values, and call this method `areHashesCurrent()` or
similar.
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java
##########
@@ -47,6 +51,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 = UTF_8;
Review comment:
Could inline this constant, to avoid the extra `CRYPT_CHARSET` variable
and corresponding Charset import.
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/security/SystemCredentials.java
##########
@@ -103,7 +102,8 @@ private static SystemToken get(String instanceID,
SiteConfiguration siteConfig)
byte[] confChecksum;
MessageDigest md;
try {
- md = MessageDigest.getInstance(Constants.PW_HASH_ALGORITHM);
+ String hashAlgorithm = siteConfig.get(Property.SYSTEM_TOKEN_HASH_TYPE);
+ md = MessageDigest.getInstance(hashAlgorithm);
Review comment:
Since system credentials are not serialized anywhere, but we do want
them to be a strong hash, I think it might be best to make use of crypt(3) here
also, so we're using the default best hash available in the commons-codec
library.
We can use a fixed salt for this. We could make the salt configurable or
based on `instance.secret`, but there's no additional security added by doing
so, since `instance.secret` is configurable and is already included in the
message digest. So, a fixed salt would suffice here. If you're not interested
in pursuing this, here, it can be done as a follow-on issue. Just let me know
if that's the case, so I can create a new issue for it.
As you've identified, the one benefit to making it configurable and
preserving the current one is to support rolling restart. However, we don't
support rolling restarts very well already, and certainly not across major or
minor releases, and we've likely already broken compatibility of that sort
during 2.1's development (or will, if we upgrade Thrift). Also, I think having
a strong has for the system user is important enough to break that, even if it
did otherwise work. So, I'm inclined to favor not making it configurable.
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -83,6 +83,14 @@
RPC_SASL_QOP("rpc.sasl.qop", "auth", PropertyType.STRING,
"The quality of protection to be used with SASL. Valid values are
'auth', 'auth-int',"
+ " and 'auth-conf'"),
+ /**
+ * @since 2.1.0
+ */
+ SYSTEM_TOKEN_HASH_TYPE("system.token.hash.type",
Constants.PW_HASH_ALGORITHM_OUTDATED,
Review comment:
See my other comment about not making this configurable. However, since
it is configurable, it must be the same across all servers, or they won't be
able to talk to each other. In general, that would mean making this an
`instance.*` property. However, since the way we enforce these is through the
same-ness of the system credentials, if this is different across the cluster,
then they won't be able to talk to each other anyway. The other property prefix
we use for system-wide properties that can be different are `general.*`.
##########
File path:
core/src/main/java/org/apache/accumulo/core/file/rfile/VisMetricsGatherer.java
##########
@@ -136,12 +136,12 @@ public void printMetrics(boolean hash, String metricWord,
PrintStream out) {
if (hash) {
String encodedKey = "";
try {
- byte[] encodedBytes =
MessageDigest.getInstance(Constants.PW_HASH_ALGORITHM)
+ byte[] encodedBytes =
MessageDigest.getInstance(Constants.NON_CRYPTO_USE_HASH_ALGORITHM)
Review comment:
Since this use of the digest is specific to this specific metric
utility's serialization, we can probably just use a local constant, rather than
one in `Constants.java` (which I'd personally like to phase out over time).
##########
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:
Okay. Fair enough.
##########
File path:
shell/src/main/java/org/apache/accumulo/shell/commands/GetSplitsCommand.java
##########
@@ -113,7 +113,7 @@ private static String encode(final boolean encode, final
Text text) {
private static String obscuredTabletName(final KeyExtent extent) {
MessageDigest digester;
try {
- digester = MessageDigest.getInstance(Constants.PW_HASH_ALGORITHM);
+ digester =
MessageDigest.getInstance(Constants.NON_CRYPTO_USE_HASH_ALGORITHM);
Review comment:
It looks like we have this exact same algorithm for creating obscured
tablet identifiers implemented in at least two places. Creating a new issue
(#1805) to address that separately.
----------------------------------------------------------------
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]
