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

classic Classic list List threaded Threaded
39 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Harsha Wardhana B

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

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




Reply | Threaded
Open this post in threaded view
|

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

Bernd Eckenfels-4
Hm, why introduce a new password hash format. Just use modular crypt() format (and iterations). This allows to use common tools (like htpasswd) to generate the hashes. It would use $5$ prefix for SHA256 but actually I would use $6$ for iterated SHA512 as it is the default on most recent Linux distributions.


From: serviceability-dev <[hidden email]> on behalf of Harsha Wardhana B <[hidden email]>
Sent: Sunday, April 23, 2017 12:20:57 PM
To: [hidden email]
Subject: RFE Review : JDK-5016517 - Replace plaintext passwords by hashed passwords for out-of-the-box JMX Agent
 

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

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




Reply | Threaded
Open this post in threaded view
|

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

Harsha Wardhana B

Hi Gruss,

Crypt format has additional params (param name and its value: hash complexity parameters, like rounds/iterations count ) which are not applicable to current implementation. Also,  hash algorithms shipped with JDK are applicable (MD5, SHA1, SHA256) and any other algorithms specified by crypt format will be ignored.

Crypt format can be used, but it is over-engineered for current requirement/implementation. I am not opposed to using it and would welcome input from other reviewers.

-Harsha

On Sunday 23 April 2017 08:10 PM, Bernd Eckenfels wrote:
Hm, why introduce a new password hash format. Just use modular crypt() format (and iterations). This allows to use common tools (like htpasswd) to generate the hashes. It would use $5$ prefix for SHA256 but actually I would use $6$ for iterated SHA512 as it is the default on most recent Linux distributions.


From: serviceability-dev [hidden email] on behalf of Harsha Wardhana B [hidden email]
Sent: Sunday, April 23, 2017 12:20:57 PM
To: [hidden email]
Subject: RFE Review : JDK-5016517 - Replace plaintext passwords by hashed passwords for out-of-the-box JMX Agent
 

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

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





Reply | Threaded
Open this post in threaded view
|

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

roger riggs
In reply to this post by Harsha Wardhana B
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
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

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

Harsha Wardhana B
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
>>
>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Harsha Wardhana B
Hello,

Could I get one more reviewer for this enhancement?

-Harsha


On Thursday 27 April 2017 12:06 PM, Harsha Wardhana B wrote:

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

Reply | Threaded
Open this post in threaded view
|

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

Harsha Wardhana B
In reply to this post by roger riggs

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

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


* 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






Reply | Threaded
Open this post in threaded view
|

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

roger riggs
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).

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

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

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

line:770:  the string constant would be nicer as a final static string
somewhere.
   "jmx.remote.x.password.file.hashpassword"

Roger



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

Reply | Threaded
Open this post in threaded view
|

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

Harsha Wardhana B
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
>>>>
>>>>
>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Harsha Wardhana B
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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs
Hi Harsha,

Good work!

 > http://cr.openjdk.java.net/~hb/5016517/webrev.03/

long standing typo in management.properties at line 90:

   measureRole => monitorRole

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.


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

I'm surprised no other tests had to be modified.
Is password hash disabled by default in the default agent?

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!

best regards,

-- daniel

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

Reply | Threaded
Open this post in threaded view
|

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

Harsha Wardhana B

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











Reply | Threaded
Open this post in threaded view
|

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

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

Reply | Threaded
Open this post in threaded view
|

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

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

Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung
In reply to this post by Harsha Wardhana B


On 10/8/17 10:34 PM, Harsha Wardhana B wrote:

Hi Daniel,

Below is the webrev addressing the review comments.

http://cr.openjdk.java.net/~hb/5016517/webrev.04/


This approach seems reasonable.   I only review management.properties and jmxremote.password.template file.
 304 # ################# Hash passwords in password file ##############
 305 # com.sun.management.jmxremote.password.hashpasswords = true|false
 306 #      Default for this property is true.
 307 #      Specifies if passswords in the above file should be hashed or not.

typo: passswords

s/above file/password file/
- it has been referred to as "password file" in many places.

I'm thinking any better alternative to the new property name??
   com.sun.management.jmxremote.password.hashes
   com.sun.management.jmxremote.password.asHashes
   com.sun.management.jmxremote.passowrd.toHashes

  49 #       https://docs.oracle.com/javase/7/docs/technotes/guides/security/StandardNames.html#MessageDigest
  50 #       MD5, SHA-1 and SHA-256 are supported algorithms.
  51 #       This is an optional field. If not specified SHA-256 will be assumed.

I would avoid the link to the documentation of a specific JDK release.
Maybe say:

Refer to "Java Security Standard Algorithm Names Specification"
for supported algorithm.


  53 # If passwords are in clear, they will be over-written by their hash if all of 

s/over-written/overwritten

  67 # If multiple entries are found for the same role name, then the last one
  68 # is used.

If there are multiple entries of the same role, will all entries
be overridden with hash value?  It may be better to detect as an
error when there are more than one entries of the same role?
HashedPasswordFileTest.java
@bug is missing

Mandy
Reply | Threaded
Open this post in threaded view
|

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

Harsha Wardhana B
In reply to this post by Daniel Fuchs
Hi Daniel,

The contents written into the password file are identical and written at
the same offset. Hence the order of the writes should not matter.
However there is a possibility that file could be read in the midst of
password change and different file contents could be read by different
processes. Since multiple writes could be done on the file, it is
possible for differing contents to be interleaved into the file, thereby
corrupting the entire file.

I will add FileLock from the nio package to avoid the above race condition.

Thanks

Harsha




On Wednesday 11 October 2017 02:30 PM, 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Harsha Wardhana B
In reply to this post by roger riggs
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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Harsha Wardhana B
In reply to this post by Mandy Chung

Hi Mandy,


On Wednesday 11 October 2017 11:48 PM, mandy chung wrote:


On 10/8/17 10:34 PM, Harsha Wardhana B wrote:

Hi Daniel,

Below is the webrev addressing the review comments.

http://cr.openjdk.java.net/~hb/5016517/webrev.04/


This approach seems reasonable.   I only review management.properties and jmxremote.password.template file.
 304 # ################# Hash passwords in password file ##############
 305 # com.sun.management.jmxremote.password.hashpasswords = true|false
 306 #      Default for this property is true.
 307 #      Specifies if passswords in the above file should be hashed or not.

typo: passswords

s/above file/password file/
- it has been referred to as "password file" in many places.
Done.


I'm thinking any better alternative to the new property name??
   com.sun.management.jmxremote.password.hashes
   com.sun.management.jmxremote.password.asHashes
   com.sun.management.jmxremote.passowrd.toHashes

  49 #       https://docs.oracle.com/javase/7/docs/technotes/guides/security/StandardNames.html#MessageDigest
  50 #       MD5, SHA-1 and SHA-256 are supported algorithms.
  51 #       This is an optional field. If not specified SHA-256 will be assumed.

I would avoid the link to the documentation of a specific JDK release.
Maybe say:

Refer to "Java Security Standard Algorithm Names Specification"
for supported algorithm.
Will modify the file appropriately.


  53 # If passwords are in clear, they will be over-written by their hash if all of 

s/over-written/overwritten

  67 # If multiple entries are found for the same role name, then the last one
  68 # is used.

If there are multiple entries of the same role, will all entries
be overridden with hash value?  It may be better to detect as an
error when there are more than one entries of the same role?
It would be better to log a warning. Throwing an error would seem a bit extreme.

HashedPasswordFileTest.java
@bug is missing

Mandy
-Harsha
Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung


On 10/12/17 1:16 AM, Harsha Wardhana B wrote:



I'm thinking any better alternative to the new property name??
   com.sun.management.jmxremote.password.hashes
   com.sun.management.jmxremote.password.asHashes
   com.sun.management.jmxremote.passowrd.toHashes

I suggest to rename com.sun.management.jmxremote.password.hashpasswords to  com.sun.management.jmxremote.password.hashes.

What do you think?

  67 # If multiple entries are found for the same role name, then the last one
  68 # is used.

If there are multiple entries of the same role, will all entries
be overridden with hash value?  It may be better to detect as an
error when there are more than one entries of the same role?
It would be better to log a warning. Throwing an error would seem a bit extreme.

What happen to the duplicated entries?  The clear password will stay?  Warning is fine.

Mandy

Reply | Threaded
Open this post in threaded view
|

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

Harsha Wardhana B



On Thursday 12 October 2017 08:40 PM, mandy chung wrote:


On 10/12/17 1:16 AM, Harsha Wardhana B wrote:



I'm thinking any better alternative to the new property name??
   com.sun.management.jmxremote.password.hashes
   com.sun.management.jmxremote.password.asHashes
   com.sun.management.jmxremote.passowrd.toHashes

I suggest to rename com.sun.management.jmxremote.password.hashpasswords to  com.sun.management.jmxremote.password.hashes.

What do you think?
We want the property to suggest an action and hence *.toHashes would be better than *.hashes.

  67 # If multiple entries are found for the same role name, then the last one
  68 # is used.

If there are multiple entries of the same role, will all entries
be overridden with hash value?  It may be better to detect as an
error when there are more than one entries of the same role?
It would be better to log a warning. Throwing an error would seem a bit extreme.

What happen to the duplicated entries?  The clear password will stay?  Warning is fine.
The duplicated entries will be removed. The last entry for a given role along with its hashed password will be written into the file.

Mandy

Harsha
12