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

Fri Oct 6 05:25:56 UTC 2017

Hi All,

Previously, for default agent, hashing of the passwords was done during 
the agent boot-up (ConnectorBootstrap.java). That was an error since 
login configuration could be different and is determined only when a 
login attempt is made. It would be then pointless to hash the password 
file. The fix for above and some off-list comments are incorporated in 
webrev below.

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

-Harsha

On Wednesday 04 October 2017 01:53 PM, Harsha Wardhana B wrote:
> Hi Roger,
>
> Below is the webrev incorporating changes suggested by you.
>
> http://cr.openjdk.java.net/~hb/5016517/webrev.02/
>
> -Harsha
>
> On Wednesday 04 October 2017 12:54 AM, Roger Riggs wrote:
>> 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).
> Ok.
>> 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
> Do you want to rename HashedPasswordManager class?
>>
>> 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
>>
> Will incorporate above comments.
>> 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).
> Static methods just to hash passwords can be created but 
> HashedPasswordManager class will have to be re-factored since almost 
> all methods are using instance variables. Not sure if we want instance 
> methods and look-alike static methods side-by-side. Wouldn't that be 
> more confusing than current implementation?
>>
>> line:770:  the string constant would be nicer as a final static 
>> string somewhere.
>>   "jmx.remote.x.password.file.hashpassword"
> All of "jmx.remote.x.*" don't have static strings. They are used 'as 
> is' all over the code to maintain isolation between pluggable login 
> authenticator and JDK code.
>>
>> Roger
>>
>>
> Harsha
>>
>> 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.
> Done.
>>>> 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."
> Done.
>>>> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>