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

[Roger Riggs]

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