Hi Mandy,
Below is the new webrev incorporating below review comments.
http://cr.openjdk.java.net/~hb/5016517/webrev.06/
On Monday 30 October 2017 11:34 PM, mandy chung wrote:
http://cr.openjdk.java.net/~hb/5016517/webrev.05/
Looks okay in general. Daniel is closer to the FileLoginModule
implementation that I will count on his review.
FileLoginModule.java
225 if(hashPwdMgr == null) {
226 hashPwdMgr = new HashedPasswordManager(passwordFile, hashPasswords);
227 } else { 228 hashPwdMgr.loadPasswords(); 229 } Will hashPwdMgr be
initialized multiple threads concurrently? Does this need to be
synchronized?
Without synchronization, it would leave an orphan instance of
HashedPasswordManager. Fixed it.
243 ace.setStackTrace(e.getStackTrace()); I think ace.initCause(e)
instead of replacing the stack trace would help debugging. ACE should
have been rev'ed to take the cause parameter (a separate issue).
jmxremote.password.template 49 #
https://docs.oracle.com/javase/7/docs/technotes/guides/security/StandardNames.html#MessageDigest
Please refer to JDK 9 docs.
255 "jmx.remote.x.password.tohashes";
305 # com.sun.management.jmxremote.password.tohashes = true|falseSince
"to hashes" are two words, capitalize "H" is a recommended convention.
HashedPasswordManager.java
214 Stream<String> lines = Files.lines(Paths.get(passwordFile));
This should be called with try-with-resource.
Done.
It would be useful to record the timestamp of when the password
file is updated with the hashed passwords.
Added it as a part of the header. The header now looks like below.
# The passwords in this file are hashed.
# In order to change password for a role, replace hashed password entry
# with clear text password or new hashed password. If new password is in
clear,
# it will be replaced with its hash when new login attempt is made.
# file last updated on - 10/31/2017 21:23:52
-Harsha
