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

Roger Riggs Roger.Riggs at Oracle.com
Wed Oct 11 15:51:25 UTC 2017 

Hi Harsha,

conf/management.properties: - typo line 307: pa*sss*words


HashedPasswordManager.java:
  - line 46: "classes" -> "class"

- line 84-87 "private" and 'static" come before "final" in declarations.

  - 158 and everywhere: add space after "if"  before "("

  - line 202: add "the" before password file.

  - line 287:  why a separate canWriteToFile(); it does the same check 
as newFileWriter(passwordFile);
    instead catch the exception and log then ignore.

Looking good

Regards, Roger

On 10/11/2017 5:00 AM, Daniel Fuchs wrote:
> Hi Harsha,
>
> Your changes look good. However I have still a nagging doubt:
>
> What happens if two Java process share the same password file,
> and it needs hashing? Are there any protection in place
> to prevent the two processes from writing to the same
> file concurrently?
>
> best regards,
>
> -- daniel
>
> On 09/10/2017 06:34, Harsha Wardhana B wrote:
>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

More information about the serviceability-dev mailing list