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

Wed Oct 4 08:23:34 UTC 2017

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