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
