Hi Harsha, HashedPasswordManager.java:
I'd suggest to use java.nio.charset.StandardCharset.UTF_8 instead of
Charset.forName("UTF-8");
The reading of the password file should probably not be allowed if
some process (this one or another one) is currently writing to it,
because you could just read some garbage.
Maybe you'd need a ReadWriteLock here or something to avoid
having this become a bottleneck - but then it probably means
you'll need to keep track of which file is protected by
a FileLock too.
Also previously it was possible to update the
password file while the agent was running, as the file was
read with each login().
I'm suspecting your change will break this partly as it
looks as if reloading the file does not clear the
new userCredentialsMap.
best regards,
-- daniel
On 07/11/2017 16:26, Harsha Wardhana B wrote:
Hi, Please find below the webrev addressing Daniel and Mandy's comments. http://cr.openjdk.java.net/~hb/5016517/webrev.07/ -Harsha On Wednesday 01 November 2017 09:42 PM, Daniel Fuchs wrote:On 31/10/2017 17:07, mandy chung wrote:On 10/31/17 8:55 AM, Harsha Wardhana B wrote:Hi Mandy, Below is the new webrev incorporating below review comments. http://cr.openjdk.java.net/~hb/5016517/webrev.06/Looks okay in general except this: 286 // Check if header needs to be inserted287 if (sbuf.indexOf("# The passwords in this file are hashed") != 0) {288 String lastUpdated = "# file last updated on - "289 + new SimpleDateFormat("MM/dd/yyyy HH:mm:ss").format(new Date()) + "\n\n";290 sbuf.insert(0, header + lastUpdated); 291 } Relying on matching the partial header string is fragile. Also the timestamp is not updated if the file contains such heading but the file is re-written again. You should probably drop the header (auto-inserted), not add it to sbuf, and always add the header when updating the password file.Sorry Mandy, that was my demand. The header is several lines long. It will look strange if it's added several times to the password file every time the user/administrator adds/changes a password. Concerning FileLock - I'm not an expert there, but the Javadoc says: https://docs.oracle.com/javase/9/docs/api/java/nio/channels/FileLock.html "File locks are held on behalf of the entire Java virtual machine. They are not suitable for controlling access to a file by multiple threads within the same virtual machine." Which means you need to also protect against concurrent writes to the same file from within the same JVM by some other means. Also I wonder if HashedPasswordManager.authenticate should take a char[] array rather than a String for the password. best regards, -- danielMandy
