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

Tue Oct 3 19:47:12 UTC 2017

Hi Roger,

Thanks for the detailed review. Below is the webrev addressing all the 
review comments.

http://cr.openjdk.java.net/~hb/5016517/webrev.01/

-Harsha

On Tuesday 25 April 2017 10:56 PM, Roger Riggs wrote:
> Hi Harsha,
>
> Thanks for this important improvement. Comments:
>
>
> * jmxremote.password.template:
>   "Passwords will be hashed by server if they are in clear." Perhaps 
> should be more explicit:
>
>    "The jmxremote.passwords file will be re-written by the server to 
> replace all plain text passwords with hashed passwords when the file 
> is read by the server."
>
> line 35: "Base64 encoded hash"  -> drop the "Base64" in this line 
> isn't needed and
> make it seems like it should appear as 1 field instead of 2 or 3.
>
> 37+: The syntax of the file may be clearer if it includes the complete 
> syntax in (line 39) not
> just the password/hash fragment.
>
> Line 41:  "W = spaces"; above "tabs" are allowed as a delimiter; it 
> would be good to be consistent
> and include the usualy white-space characters in the set, be as 
> specific as possible.
> Is this the same set of whitespace used by Regex '\\s'.
Only spaces and tabs are allowed. '\s' matches newline as well hence not 
allowed.
>
> 45: "java platform""  ->   "MD5, SDA-1, SHA-256 are supported 
> algorithms."
>
> 49: be more specific about 'hashing is requested'  how?  Refer to the 
> management.properties
>   com.sun.management.jmxremote.password.hash value.
>
>
>
> 51:  "replace hashed" -> "replace *the *hashed"
> 52: "with clear text or new" -> "with the clear text or the new"
> 52: "If new password" -> "If the new password"
> 53: "when new login" -> "when a new login"
>
> 60: "User generated" -> "A User generated"
>
> 67: Will the file be ignored if it has the wrong permissions. (With a 
> logged message)
Addressed all the above review comments.
>
> * management.properties
>
> 306: "(Case for true/false ignored)"  - what does this mean; I think 
> it can be removed.
>
> 307: missing period at the end of the sentence.
> 309: "in password file" -> "in the password file"
>
Done.
>
> * FileLoginModule.java
>
> 102: can this match better the similar name in the 
> management.properties if it has the same function:
>     com.sun.management.jmxremote.password.hash
Are you suggesting that 'hashPassword' be renamed to something similar 
to com.sun.management.jmxremote.password.hash? Variable names cannot be 
similar to property names since property names are long and provide 
complete context which local variables need not have to do.
> 103: "replaces clear text passwords" -> "replaces each clear text 
> password"
> 104: indent to match previous <dd> enteries.
>
> * JMXPluggableAuthenticator.java
>
> 119: There is no need to copy the password to a new local
It is required since variables accessed from inner class must be final 
or effectively final.
>
> 128: add a space after ","
>
> 256 private static final String HASH_PASSWORDS =
> 257 "jmx.remote.x.password.file.hash";
>
> The name ".hash" part does not clearly communicate that passwords are 
> to be hashed.
> "hashPasswords" might be more self explanatory.
Changed it to "jmx.remote.x.password.file.hashpassword".
> Also, can this be NOT duplicated here and in ConnectorBootStrap.java?
The property names used in ConnectorBootStrap follows the convention 
used in management.properties file - 'com.sun.management.*'. For 
environment variables for a JMXConnector "jmx.remote.x.*" convention is 
used . Hence they cannot be duplicated.
>
>
> * ConnectorBootStrap.java:
>  482: Add space after ","s; no spaces before.
>
> 770: use the same name for the option/property if possible to avoid 
> confusion.
Not possible as explained above.
>
> 770:  if the HASH_PASSWORDS static is appropriate use it instead of 
> literal "true".
DefaultValues.HASH_PASSWORDS static is set to 'true' and can be used. 
However using literal "true" is more readable than using the static.
>
> * HashedPasswordManager
>
> 80-83: The fields can be final and use the constructor to initialize 
> in all cases and make the class final
> to avoid unintentional subclassing.
>
>
> 113: canWriteToFile:   It should be made clear in the template that 
> *both* the Security policy
>    and the file access value are used to check that the file can be 
> updated.
Made it explicit in template as well as code comments.
>
> 200: loadPasswords() - should this confirm the access to the file is 
> allowed and it has
> the correct file access before reading?
Not really required. Appropriate exceptions are thrown if file cannot be 
accessed.
>
> Is the re-writing of the passwords intended to be done by a 
> 'priveleged' system.
> Does this need doPrivileged?
I am not sure. Maybe it will be covered in the security review.
>
> * HashedPasswordFileTest:
>
> 88: should use the TestLibrary Utils.getRandomInstance so it logs the 
> seed and can be replayed if necessary.
>
>
Done
> Thanks, Roger
>
Thanks
Harsha
>
> On 4/23/2017 6:20 AM, Harsha Wardhana B wrote:
>>
>> Hi All,
>>
>> Please review this enhancement to replace plain-text password for JMX 
>> agent with SHA-256 hash.
>>
>> Issue: https://bugs.openjdk.java.net/browse/JDK-5016517
>> <https://bugs.openjdk.java.net/browse/JDK-5016517>
>>
>> webrev: http://cr.openjdk.java.net/~hb/5016517/webrev.00/
>>
>> Overview of implementation:
>>
>> Currently, the JMX agent password file used to authenticate user, 
>> stores user name and password as clear text. Though system level 
>> restrictions are recommended for jmx password file, passwords are 
>> vulnerable since they are stored in clear. The current RFE proposes 
>> to store passwords as SHA256 hash instead of clear text.
>>
>> In current implementation, if password file is writable, and if 
>> passwords are in clear, they will be replaced by SHA256 hash upon 
>> agent boot-up or when login attempt is made.
>>
>> The file, 
>> src/jdk.management.agent/share/conf/jmxremote.password.template 
>> contains more details about the implementation.
>>
>> - Harsha
>>
>>
>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20171004/0025ff3e/attachment.html>