[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-04-27 6:48:24
Message-ID: 5dded394-ea92-687f-cbb0-3347936c1eec () oracle ! com
[Download RAW message or body]

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]