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

Harsha Wardhana B harsha.wardhana.b at oracle.com
Mon Oct 9 05:34:52 UTC 2017 

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20171009/8828badc/attachment-0001.html>

More information about the serviceability-dev mailing list