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

[Harsha Wardhana B]

Hi Daniel,

Maintaining a warning or info log requires internationalization and 
since java.management module does not have the necessary resource 
bundles, it becomes complicated to take up that activity as a part of 
this enhancement.  Hence I have changed all the logs to debug. We can 
take up changing logging level and internationalization as a separate 
issue. I have updated the webrev in-place. Please review and let me know 
if you are okay with it.

Thanks
Harsha

On Friday 10 November 2017 07:50 PM, Daniel Fuchs wrote:
> 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
>>
>