Hi Daniel,
Below is the webrev addressing the review comments.
http://cr.openjdk.java.net/~hb/5016517/webrev.04/
On Friday 06 October 2017 03:38 PM, Daniel Fuchs wrote:
Hi Harsha,
Good work!
> http://cr.openjdk.java.net/~hb/5016517/webrev.03/
long standing typo in management.properties at line 90:
measureRole => monitorRole
Done.
HashedPasswordManager.java: loadPasswords()
It seems this function will add the header to the file even
if it already contains the header.
So every time a user/administrator wants to change/add a password,
the header will be inserted again.
Yes. It is fixed in the new webrev.
HashedPasswordFileTest should probably have a test for this
scenario as well:
generate password file with clear text password
load it, then verify passwords have been hashed (properly)
add some new user/name password to the same file
load it again, verify all passwords are hashed
(do this a number of times - to make sure it doesn't
break the second or third time)
and finally verify the header is only present once ;-)
Done. Added a testcase for the same.
I'm surprised no other tests had to be modified.
Is password hash disabled by default in the default agent?
Password hashing is enabled by default. But it is only the
implementation that is changed. The pluggable JAAS mechanism isolates
interfaces from implementation. So in theory, all tests should pass.
If not then you should try (locally) running jtreg
more than once over the default agent tests.
Just make sure running the same test twice doesn't
make the legacy tests that use password files failing the
second time when they discover that passwords have been
hashed under their feet (the client part of the test
might be reading the password file too to see which
password it should send to the agent).
Otherwise I think it looks good to me - provided all
tests are passing!
Done. Had a few test failures but nothing related to this enhancement.
best regards,
-- daniel
Thanks
Harsha
On 06/10/2017 06:25, Harsha Wardhana B wrote:
Hi All,
Previously, for default agent, hashing of the passwords was done
during the agent boot-up (ConnectorBootstrap.java). That was an error
since login configuration could be different and is determined only
when a login attempt is made. It would be then pointless to hash the
password file. The fix for above and some off-list comments are
incorporated in webrev below.
http://cr.openjdk.java.net/~hb/5016517/webrev.03/
-Harsha
On Wednesday 04 October 2017 01:53 PM, Harsha Wardhana B wrote:
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
