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

Tue Oct 3 19:24:57 UTC 2017

Hi Harsha,

FileLoginModule.java:  104:  Add a period at the end of the the sentence.

JMXPluggableAuthenticator.java: line 306:  Is the difference between 
singular and plural significant?
   It would be less confusing if both were plural (hashPasswords).

ConnectorBootstrap:
134: ...password.file.hash" and HashedPasswordManager disagree on the 
exact string.
I would propose 'hashpasswords' as the suffix in all places to be consistent
in ConnectorBootstrap.java, HashedPasswordManager (except for 
capitalization),
jmxremote.password.template, and management.properties

As is you have a mix of "...password.hash", "...password.file.hash", 
"...hashpassword";
that's not good for knowing there is only one semantic.

line 482:  " ," -> ", "  space after comma, not before

line: 771: is it intentional to discard the reference to the new 
HashedPasswordManager?
If the intention is only to use the side effect of loadPasswords, then 
please
create a static method in HashedPasswordManager for that purpose.
(Even if just does the same code; it would be clear that's the purpose).
(It probably also implies that the password file will be read a second 
time somewhere else in the initialization).

line:770:  the string constant would be nicer as a final static string 
somewhere.
   "jmx.remote.x.password.file.hashpassword"

Roger

On 10/3/2017 3:47 PM, Harsha Wardhana B wrote:
>
> 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.
the suffix should be the same in all places since it is a single semantic.
>> 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.
right
>>
>> 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".
drop the "file."
>> 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.
The differing prefix'es are fine as is; no change except to make the new 
keys consistent.

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