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

[Daniel Fuchs]

Hi Harsha,

On 10/11/2017 12:38, Harsha Wardhana B wrote:
> Hi,
> 
> Please find the below webrev with the following changes.
> 
> 1. All the reads/writes into the password file are synchronized w.r.t 
> threads within the JVM and across multiple JVM processes. It is possible 
> that some edits made to file while the agent is running might be lost 
> and hence added a cautionary note in jmxremote.password.template.

OK - given the complexity of the alternative I believe what you
have now should be considered "good enough". We can always revisit
later if it proves to cause issues. Thanks for adding the note
to the jmxremote.password.template.

> 2. Added a new test-case 'testMultipleClients' that validates concurrent 
> read/writes

Thanks for doing that!

> 
> 3. Added an info log when the password file is over-written.
> 
> http://cr.openjdk.java.net/~hb/5016517/webrev.08/
> 
> Please review the latest webrev.

HashPasswordManager:

  204             } catch (NoSuchAlgorithmException ex) {
  205                 if (logger.warningOn()) {
  206                     logger.warning("authenticate", "Unrecognized 
hash algorithm : "
  207                             + 
userCredentialsMap.get(userName).hashAlgorithm
  208                             + " - for user : " + userName);
  209                 }
  210                 return false;
  211             }
  212         } else {
  213             if (logger.warningOn()) {
  214                 logger.warning("authenticate", "Unknown user : " + 
userName);
  215             }
  216             return false;
  217         }

and elsewhere in this file: these should not be warnings: debug at
the most.

  318                 if (logger.infoOn()) {
  319                     logger.info("loadPasswords",
  320                             "Wrote hashed passwords to file : " + 
passwordFile);
  321                 }

I think this should be debug as well.


The last one might probably stay as a warning but if so:

    1. it should be printed only once (and not for every
       new client that comes)
    2. It might need to be internationalized.

  322             } else if (logger.warningOn()) {
  323                 logger.warning("loadPasswords",
  324                         "Passwords in " + passwordFile + " are in 
clear and password file is read-only. "
  325                                 + "Passwords cannot be hashed !!!!");
  326             }

The rest looks good to me!

best regards,

-- daniel

> 
> Thanks
> Harsha
> 
> On Wednesday 08 November 2017 09:29 AM, mandy chung wrote:
>>
>>
>> On 11/7/17 9:04 AM, Harsha Wardhana B wrote:
>>>
>>> Hi Mandy,
>>>
>>> To summarize the changes,
>>>
>>> 1. The header will not contain the file modification timestamp. 
>>> Instead when the password file is modified, a debug log will be 
>>> printed. The log will contain the timestamp.
>>>
>>> 2. The password file is now protected from concurrent writes from 
>>> within the JVM.
>>>
>>> 3. HashedPasswordManager.authenticate accepts char[] for password 
>>> instead of String.
>>>
>>
>> Thanks for this. That helps.
>>> Header will be inserted. Apart from that all the comments will be 
>>> retained.
>>
>> I think this header can also be taken out.  The comment may already be 
>> copied from the template or deleted on purpose.
>>
>>>> Also log a message when the file is overridden - we didn't discuss 
>>>> the format but I think it should include the pathname of the file 
>>>> and the role name of the overridden entries (should it be info 
>>>> level?).  line 308-311 is debug message - is that the one?
>>> I guess this wasn't discussed. We just output a debug log saying the 
>>> file is overwritten. File name can be mentioned in the log.
>>
>> INFO log message seems more appropriate.
>>
>> Mandy
>