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

[Harsha Wardhana B]

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