Hi Daniel,
On Tuesday 07 November 2017 10:43 PM, Daniel Fuchs wrote:
Hi Harsha,
HashedPasswordManager.java:
I'd suggest to use java.nio.charset.StandardCharset.UTF_8 instead of
Charset.forName("UTF-8");
ok.
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.
I think reading a file while it is being written to might give us
incomplete data (depending on how often we flush the stream) but not
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.
I think using a local lock (synchronized block or r/w lock) in
conjunction with FileLock for both read/write operations should solve
this problem. I am not sure why we need to keep track of FileLocks. I
will send a modified webrev.
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.
Now also the file gets reloaded for each login. Not clearing the
userCredentialMap might leave stale entries (which should be cleared),
but new entries will get updated and hence it will not break existing
functionality. The test-cases validate the above use-case.
best regards,
-- daniel
-Harsha
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 inserted
287 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,
-- daniel
Mandy
