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
Thu Oct 12 06:49:16 UTC 2017
Hi Roger,
On Wednesday 11 October 2017 09:21 PM, Roger Riggs wrote:
> 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.
>
Done.
> - line 287: why a separate canWriteToFile(); it does the same check
> as newFileWriter(passwordFile);
> instead catch the exception and log then ignore.
It will be replaced by nio APIs.
Thanks
Harsha
>
> 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