[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFE Review : JDK-5016517 - Replace plaintext passwords by hashed passwords for out-of-the-box JM
From:       Harsha Wardhana B <harsha.wardhana.b () oracle ! com>
Date:       2017-05-08 5:58:13
Message-ID: b717fbc9-6f9c-be89-f888-7380117d23ec () oracle ! com
[Download RAW message or body]

Hello,

Could I get one more reviewer for this enhancement?

-Harsha


On Thursday 27 April 2017 12:06 PM, Harsha Wardhana B wrote:
> Hi Roger,
>
> Thanks for the detailed review. I will wait for more review comments 
> before incorporating the ones below.
>
> -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'.
>>
>> 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)
>>
>> * 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"
>>
>>
>> * 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
>> 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
>>
>> 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.
>> Also, can this be NOT duplicated here and in ConnectorBootStrap.java?
>>
>>
>> * ConnectorBootStrap.java:
>>  482: Add space after ","s; no spaces before.
>>
>> 770: use the same name for the option/property if possible to avoid 
>> confusion.
>>
>> 770:  if the HASH_PASSWORDS static is appropriate use it instead of 
>> literal "true".
>>
>> * 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.
>>
>> 200: loadPasswords() - should this confirm the access to the file is 
>> allowed and it has
>> the correct file access before reading?
>>
>> Is the re-writing of the passwords intended to be done by a 
>> 'priveleged' system.
>> Does this need doPrivileged?
>>
>> * HashedPasswordFileTest:
>>
>> 88: should use the TestLibrary Utils.getRandomInstance so it logs the 
>> seed and can be replayed if necessary.
>>
>>
>> Thanks, Roger
>>
>>
>> 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
>>>
>>>
>>>
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]