RFE Review : JDK-5016517 - Replace plaintext passwords by hashed passwords for out-of-the-box JMX Agent

[Harsha Wardhana B]

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
>>>>
>>>
>>
>