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

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

2017-11-12 Thread Harsha Wardhana B


Hi Daniel,

Maintaining a warning or info log requires internationalization and 
since java.management module does not have the necessary resource 
bundles, it becomes complicated to take up that activity as a part of 
this enhancement.  Hence I have changed all the logs to debug. We can 
take up changing logging level and internationalization as a separate 
issue. I have updated the webrev in-place. Please review and let me know 
if you are okay with it.


Thanks
Harsha

On Friday 10 November 2017 07:50 PM, Daniel Fuchs wrote:

Hi Harsha,

On 10/11/2017 12:38, Harsha Wardhana B wrote:

Hi,

Please find the below webrev with the following changes.

1. All the reads/writes into the password file are synchronized w.r.t 
threads within the JVM and across multiple JVM processes. It is 
possible that some edits made to file while the agent is running 
might be lost and hence added a cautionary note in 
jmxremote.password.template.


OK - given the complexity of the alternative I believe what you
have now should be considered "good enough". We can always revisit
later if it proves to cause issues. Thanks for adding the note
to the jmxremote.password.template.

2. Added a new test-case 'testMultipleClients' that validates 
concurrent read/writes


Thanks for doing that!



3. Added an info log when the password file is over-written.

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

Please review the latest webrev.


HashPasswordManager:

 204 } catch (NoSuchAlgorithmException ex) {
 205 if (logger.warningOn()) {
 206 logger.warning("authenticate", "Unrecognized 
hash algorithm : "
 207 + 
userCredentialsMap.get(userName).hashAlgorithm

 208 + " - for user : " + userName);
 209 }
 210 return false;
 211 }
 212 } else {
 213 if (logger.warningOn()) {
 214 logger.warning("authenticate", "Unknown user : " 
+ userName);

 215 }
 216 return false;
 217 }

and elsewhere in this file: these should not be warnings: debug at
the most.

 318 if (logger.infoOn()) {
 319 logger.info("loadPasswords",
 320 "Wrote hashed passwords to file : " + 
passwordFile);

 321 }

I think this should be debug as well.


The last one might probably stay as a warning but if so:

   1. it should be printed only once (and not for every
  new client that comes)
   2. It might need to be internationalized.

 322 } else if (logger.warningOn()) {
 323 logger.warning("loadPasswords",
 324 "Passwords in " + passwordFile + " are in 
clear and password file is read-only. "
 325 + "Passwords cannot be hashed 
");

 326 }

The rest looks good to me!

best regards,

-- daniel



Thanks
Harsha

On Wednesday 08 November 2017 09:29 AM, mandy chung wrote:



On 11/7/17 9:04 AM, Harsha Wardhana B wrote:


Hi Mandy,

To summarize the changes,

1. The header will not contain the file modification timestamp. 
Instead when the password file is modified, a debug log will be 
printed. The log will contain the timestamp.


2. The password file is now protected from concurrent writes from 
within the JVM.


3. HashedPasswordManager.authenticate accepts char[] for password 
instead of String.




Thanks for this. That helps.
Header will be inserted. Apart from that all the comments will be 
retained.


I think this header can also be taken out.  The comment may already 
be copied from the template or deleted on purpose.


Also log a message when the file is overridden - we didn't discuss 
the format but I think it should include the pathname of the file 
and the role name of the overridden entries (should it be info 
level?). line 308-311 is debug message - is that the one?
I guess this wasn't discussed. We just output a debug log saying 
the file is overwritten. File name can be mentioned in the log.


INFO log message seems more appropriate.

Mandy

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

2017-11-10 Thread Daniel Fuchs


Hi Harsha,

On 10/11/2017 12:38, Harsha Wardhana B wrote:

Hi,

Please find the below webrev with the following changes.

1. All the reads/writes into the password file are synchronized w.r.t 
threads within the JVM and across multiple JVM processes. It is possible 
that some edits made to file while the agent is running might be lost 
and hence added a cautionary note in jmxremote.password.template.


OK - given the complexity of the alternative I believe what you
have now should be considered "good enough". We can always revisit
later if it proves to cause issues. Thanks for adding the note
to the jmxremote.password.template.

2. Added a new test-case 'testMultipleClients' that validates concurrent 
read/writes


Thanks for doing that!



3. Added an info log when the password file is over-written.

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

Please review the latest webrev.


HashPasswordManager:

 204 } catch (NoSuchAlgorithmException ex) {
 205 if (logger.warningOn()) {
 206 logger.warning("authenticate", "Unrecognized 
hash algorithm : "
 207 + 
userCredentialsMap.get(userName).hashAlgorithm

 208 + " - for user : " + userName);
 209 }
 210 return false;
 211 }
 212 } else {
 213 if (logger.warningOn()) {
 214 logger.warning("authenticate", "Unknown user : " + 
userName);

 215 }
 216 return false;
 217 }

and elsewhere in this file: these should not be warnings: debug at
the most.

 318 if (logger.infoOn()) {
 319 logger.info("loadPasswords",
 320 "Wrote hashed passwords to file : " + 
passwordFile);

 321 }

I think this should be debug as well.


The last one might probably stay as a warning but if so:

   1. it should be printed only once (and not for every
  new client that comes)
   2. It might need to be internationalized.

 322 } else if (logger.warningOn()) {
 323 logger.warning("loadPasswords",
 324 "Passwords in " + passwordFile + " are in 
clear and password file is read-only. "

 325 + "Passwords cannot be hashed ");
 326 }

The rest looks good to me!

best regards,

-- daniel



Thanks
Harsha

On Wednesday 08 November 2017 09:29 AM, mandy chung wrote:



On 11/7/17 9:04 AM, Harsha Wardhana B wrote:


Hi Mandy,

To summarize the changes,

1. The header will not contain the file modification timestamp. 
Instead when the password file is modified, a debug log will be 
printed. The log will contain the timestamp.


2. The password file is now protected from concurrent writes from 
within the JVM.


3. HashedPasswordManager.authenticate accepts char[] for password 
instead of String.




Thanks for this. That helps.
Header will be inserted. Apart from that all the comments will be 
retained.


I think this header can also be taken out.  The comment may already be 
copied from the template or deleted on purpose.


Also log a message when the file is overridden - we didn't discuss 
the format but I think it should include the pathname of the file 
and the role name of the overridden entries (should it be info 
level?).  line 308-311 is debug message - is that the one?
I guess this wasn't discussed. We just output a debug log saying the 
file is overwritten. File name can be mentioned in the log.


INFO log message seems more appropriate.

Mandy

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

2017-11-10 Thread Harsha Wardhana B


Hi,

Please find the below webrev with the following changes.

1. All the reads/writes into the password file are synchronized w.r.t 
threads within the JVM and across multiple JVM processes. It is possible 
that some edits made to file while the agent is running might be lost 
and hence added a cautionary note in jmxremote.password.template.


2. Added a new test-case 'testMultipleClients' that validates concurrent 
read/writes


3. Added an info log when the password file is over-written.

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

Please review the latest webrev.

Thanks
Harsha

On Wednesday 08 November 2017 09:29 AM, mandy chung wrote:



On 11/7/17 9:04 AM, Harsha Wardhana B wrote:


Hi Mandy,

To summarize the changes,

1. The header will not contain the file modification timestamp. 
Instead when the password file is modified, a debug log will be 
printed. The log will contain the timestamp.


2. The password file is now protected from concurrent writes from 
within the JVM.


3. HashedPasswordManager.authenticate accepts char[] for password 
instead of String.




Thanks for this. That helps.
Header will be inserted. Apart from that all the comments will be 
retained.


I think this header can also be taken out.  The comment may already be 
copied from the template or deleted on purpose.


Also log a message when the file is overridden - we didn't discuss 
the format but I think it should include the pathname of the file 
and the role name of the overridden entries (should it be info 
level?).  line 308-311 is debug message - is that the one?
I guess this wasn't discussed. We just output a debug log saying the 
file is overwritten. File name can be mentioned in the log.


INFO log message seems more appropriate.

Mandy

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

2017-11-08 Thread Harsha Wardhana B


Hi Daniel,


On Tuesday 07 November 2017 10:43 PM, Daniel Fuchs wrote:

Hi Harsha,

HashedPasswordManager.java:

I'd suggest to use java.nio.charset.StandardCharset.UTF_8 instead of
Charset.forName("UTF-8");

ok.


The reading of the password file should probably not be allowed if
some process (this one or another one) is currently writing to it,
because you could just read some garbage.
I think reading a file while it is being written to might give us 
incomplete data (depending on how often we flush the stream) but not 
garbage.


Maybe you'd need a ReadWriteLock here or something to avoid
having this become a bottleneck - but then it probably means
you'll need to keep track of which file is protected by
a FileLock too.
I think using a local lock (synchronized block or r/w lock) in 
conjunction with FileLock for both read/write operations should solve 
this problem. I am not sure why we need to keep track of FileLocks. I 
will send a modified webrev.


Also previously it was possible to update the
password file while the agent was running, as the file was
read with each login().
I'm suspecting your change will break this partly as it
looks as if reloading the file does not clear the
new userCredentialsMap.
Now also the file gets reloaded for each login. Not clearing the 
userCredentialMap might leave stale entries (which should be cleared), 
but new entries will get updated and hence it will not break existing 
functionality. The test-cases validate the above use-case.


best regards,

-- daniel



-Harsha

On 07/11/2017 16:26, Harsha Wardhana B wrote:

Hi,

Please find below the webrev addressing Daniel and Mandy's comments.

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

-Harsha

On Wednesday 01 November 2017 09:42 PM, Daniel Fuchs wrote:

On 31/10/2017 17:07, mandy chung wrote:

On 10/31/17 8:55 AM, Harsha Wardhana B wrote:


Hi Mandy,

Below is the new webrev incorporating below review comments.

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


Looks okay in general except this:

  286 // Check if header needs to be inserted
  287 if (sbuf.indexOf("# The passwords in this file are 
hashed") != 0) {

  288 String lastUpdated = "# file last updated on - "
  289 + new SimpleDateFormat("MM/dd/ 
HH:mm:ss").format(new Date()) + "\n\n";

  290 sbuf.insert(0, header + lastUpdated);
  291 }

Relying on matching the partial header string is fragile.
Also the timestamp is not updated if the file contains such
heading but the file is re-written again.

You should probably drop the header (auto-inserted), not add
it to sbuf, and always add the header when updating the
password file.


Sorry Mandy, that was my demand. The header is several lines long.
It will look strange if it's added several times to the password file
every time the user/administrator adds/changes a password.

Concerning FileLock - I'm not an expert there, but the Javadoc says:

https://docs.oracle.com/javase/9/docs/api/java/nio/channels/FileLock.html 


"File locks are held on behalf of the entire Java virtual machine.
 They are not suitable for controlling access to a file by multiple
 threads within the same virtual machine."

Which means you need to also protect against concurrent writes to
the same file from within the same JVM by some other means.

Also I wonder if HashedPasswordManager.authenticate should take
a char[] array rather than a String for the password.

best regards,

-- daniel





Mandy

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

2017-11-07 Thread mandy chung




On 11/7/17 9:04 AM, Harsha Wardhana B wrote:


Hi Mandy,

To summarize the changes,

1. The header will not contain the file modification timestamp. 
Instead when the password file is modified, a debug log will be 
printed. The log will contain the timestamp.


2. The password file is now protected from concurrent writes from 
within the JVM.


3. HashedPasswordManager.authenticate accepts char[] for password 
instead of String.




Thanks for this. That helps.
Header will be inserted. Apart from that all the comments will be 
retained.


I think this header can also be taken out.  The comment may already be 
copied from the template or deleted on purpose.


Also log a message when the file is overridden - we didn't discuss 
the format but I think it should include the pathname of the file and 
the role name of the overridden entries (should it be info level?).  
line 308-311 is debug message - is that the one?
I guess this wasn't discussed. We just output a debug log saying the 
file is overwritten. File name can be mentioned in the log.


INFO log message seems more appropriate.

Mandy

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

2017-11-07 Thread Daniel Fuchs


Hi Harsha,

HashedPasswordManager.java:

I'd suggest to use java.nio.charset.StandardCharset.UTF_8 instead of
Charset.forName("UTF-8");

The reading of the password file should probably not be allowed if
some process (this one or another one) is currently writing to it,
because you could just read some garbage.

Maybe you'd need a ReadWriteLock here or something to avoid
having this become a bottleneck - but then it probably means
you'll need to keep track of which file is protected by
a FileLock too.

Also previously it was possible to update the
password file while the agent was running, as the file was
read with each login().
I'm suspecting your change will break this partly as it
looks as if reloading the file does not clear the
new userCredentialsMap.

best regards,

-- daniel


On 07/11/2017 16:26, Harsha Wardhana B wrote:

Hi,

Please find below the webrev addressing Daniel and Mandy's comments.

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

-Harsha

On Wednesday 01 November 2017 09:42 PM, Daniel Fuchs wrote:

On 31/10/2017 17:07, mandy chung wrote:

On 10/31/17 8:55 AM, Harsha Wardhana B wrote:


Hi Mandy,

Below is the new webrev incorporating below review comments.

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


Looks okay in general except this:

  286 // Check if header needs to be inserted
  287 if (sbuf.indexOf("# The passwords in this file are 
hashed") != 0) {

  288 String lastUpdated = "# file last updated on - "
  289 + new SimpleDateFormat("MM/dd/ 
HH:mm:ss").format(new Date()) + "\n\n";

  290 sbuf.insert(0, header + lastUpdated);
  291 }

Relying on matching the partial header string is fragile.
Also the timestamp is not updated if the file contains such
heading but the file is re-written again.

You should probably drop the header (auto-inserted), not add
it to sbuf, and always add the header when updating the
password file.


Sorry Mandy, that was my demand. The header is several lines long.
It will look strange if it's added several times to the password file
every time the user/administrator adds/changes a password.

Concerning FileLock - I'm not an expert there, but the Javadoc says:

https://docs.oracle.com/javase/9/docs/api/java/nio/channels/FileLock.html
"File locks are held on behalf of the entire Java virtual machine.
 They are not suitable for controlling access to a file by multiple
 threads within the same virtual machine."

Which means you need to also protect against concurrent writes to
the same file from within the same JVM by some other means.

Also I wonder if HashedPasswordManager.authenticate should take
a char[] array rather than a String for the password.

best regards,

-- daniel





Mandy

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

2017-11-07 Thread Harsha Wardhana B


Hi Mandy,

To summarize the changes,

1. The header will not contain the file modification timestamp. Instead 
when the password file is modified, a debug log will be printed. The log 
will contain the timestamp.


2. The password file is now protected from concurrent writes from within 
the JVM.


3. HashedPasswordManager.authenticate accepts char[] for password 
instead of String.


-Harsha

On Tuesday 07 November 2017 10:24 PM, mandy chung wrote:



On 11/7/17 8:26 AM, Harsha Wardhana B wrote:

Hi,

Please find below the webrev addressing Daniel and Mandy's comments.

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


Can you summarize the change?

I thought we agree to only replace the clear passwords with the hashes 
and not to alter any other content nor inserting any header.

Header will be inserted. Apart from that all the comments will be retained.
Also log a message when the file is overridden - we didn't discuss the 
format but I think it should include the pathname of the file and the 
role name of the overridden entries (should it be info level?).  line 
308-311 is debug message - is that the one?
I guess this wasn't discussed. We just output a debug log saying the 
file is overwritten. File name can be mentioned in the log.


Mandy

Harsha

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

2017-11-07 Thread mandy chung




On 11/7/17 8:26 AM, Harsha Wardhana B wrote:

Hi,

Please find below the webrev addressing Daniel and Mandy's comments.

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


Can you summarize the change?

I thought we agree to only replace the clear passwords with the hashes 
and not to alter any other content nor inserting any header.   Also log 
a message when the file is overridden - we didn't discuss the format but 
I think it should include the pathname of the file and the role name of 
the overridden entries (should it be info level?).  line 308-311 is 
debug message - is that the one?


Mandy

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

2017-11-07 Thread Harsha Wardhana B


Hi,

Please find below the webrev addressing Daniel and Mandy's comments.

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

-Harsha

On Wednesday 01 November 2017 09:42 PM, Daniel Fuchs wrote:

On 31/10/2017 17:07, mandy chung wrote:

On 10/31/17 8:55 AM, Harsha Wardhana B wrote:


Hi Mandy,

Below is the new webrev incorporating below review comments.

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


Looks okay in general except this:

  286 // Check if header needs to be inserted
  287 if (sbuf.indexOf("# The passwords in this file are 
hashed") != 0) {

  288 String lastUpdated = "# file last updated on - "
  289 + new SimpleDateFormat("MM/dd/ 
HH:mm:ss").format(new Date()) + "\n\n";

  290 sbuf.insert(0, header + lastUpdated);
  291 }

Relying on matching the partial header string is fragile.
Also the timestamp is not updated if the file contains such
heading but the file is re-written again.

You should probably drop the header (auto-inserted), not add
it to sbuf, and always add the header when updating the
password file.


Sorry Mandy, that was my demand. The header is several lines long.
It will look strange if it's added several times to the password file
every time the user/administrator adds/changes a password.

Concerning FileLock - I'm not an expert there, but the Javadoc says:

https://docs.oracle.com/javase/9/docs/api/java/nio/channels/FileLock.html
"File locks are held on behalf of the entire Java virtual machine.
 They are not suitable for controlling access to a file by multiple
 threads within the same virtual machine."

Which means you need to also protect against concurrent writes to
the same file from within the same JVM by some other means.

Also I wonder if HashedPasswordManager.authenticate should take
a char[] array rather than a String for the password.

best regards,

-- daniel





Mandy

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

2017-11-01 Thread Daniel Fuchs


On 31/10/2017 17:07, mandy chung wrote:

On 10/31/17 8:55 AM, Harsha Wardhana B wrote:


Hi Mandy,

Below is the new webrev incorporating below review comments.

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


Looks okay in general except this:

  286 // Check if header needs to be inserted
  287 if (sbuf.indexOf("# The passwords in this file are hashed") != 0) 
{
  288 String lastUpdated = "# file last updated on - "
  289 + new SimpleDateFormat("MM/dd/ HH:mm:ss").format(new 
Date()) + "\n\n";
  290 sbuf.insert(0, header + lastUpdated);
  291 }

Relying on matching the partial header string is fragile.
Also the timestamp is not updated if the file contains such
heading but the file is re-written again.

You should probably drop the header (auto-inserted), not add
it to sbuf, and always add the header when updating the
password file.


Sorry Mandy, that was my demand. The header is several lines long.
It will look strange if it's added several times to the password file
every time the user/administrator adds/changes a password.

Concerning FileLock - I'm not an expert there, but the Javadoc says:

https://docs.oracle.com/javase/9/docs/api/java/nio/channels/FileLock.html
"File locks are held on behalf of the entire Java virtual machine.
 They are not suitable for controlling access to a file by multiple
 threads within the same virtual machine."

Which means you need to also protect against concurrent writes to
the same file from within the same JVM by some other means.

Also I wonder if HashedPasswordManager.authenticate should take
a char[] array rather than a String for the password.

best regards,

-- daniel





Mandy

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

2017-11-01 Thread Roger Riggs


Hi Harsha,

Sorry for the late editorial recommendations:

In jmxremote.password.template:

41: "Clear text" -> "A clear text"
43: 'below format" -> "format below"
53: "in clear" -> "in the clear"
63: "in clear" -> "in the clear"
77: "by ONLY the owner" -> "ONLY by the owner"
80-81: Is not consistent with the 77-78;  80-81 can be removed
82: "should" -> "must" to be consistent with 77:
82: "except for owner" ->  "ONLY by the owner"
92: should end with "." or ':"
97: "passwords will" -> "the passwords will"
98: "below entries with clear" -> "the entries below with the clear"
99: "should end with "." or ":"

management.properties:
311: sentence should end with "."

Thanks, Roger

On 10/31/2017 1:07 PM, mandy chung wrote:



On 10/31/17 8:55 AM, Harsha Wardhana B wrote:


Hi Mandy,

Below is the new webrev incorporating below review comments.

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


Looks okay in general except this:
  286 // Check if header needs to be inserted
  287 if (sbuf.indexOf("# The passwords in this file are hashed") != 0) 
{
  288 String lastUpdated = "# file last updated on - "
  289 + new SimpleDateFormat("MM/dd/ HH:mm:ss").format(new 
Date()) + "\n\n";
  290 sbuf.insert(0, header + lastUpdated);
  291 }

Relying on matching the partial header string is fragile.
Also the timestamp is not updated if the file contains such
heading but the file is re-written again.

You should probably drop the header (auto-inserted), not add
it to sbuf, and always add the header when updating the
password file.

Mandy

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

2017-10-31 Thread mandy chung




On 10/31/17 8:55 AM, Harsha Wardhana B wrote:


Hi Mandy,

Below is the new webrev incorporating below review comments.

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


Looks okay in general except this:

 286 // Check if header needs to be inserted
 287 if (sbuf.indexOf("# The passwords in this file are hashed") != 0) {
 288 String lastUpdated = "# file last updated on - "
 289 + new SimpleDateFormat("MM/dd/ HH:mm:ss").format(new Date()) 
+ "\n\n";
 290 sbuf.insert(0, header + lastUpdated);
 291 }

Relying on matching the partial header string is fragile.
Also the timestamp is not updated if the file contains such
heading but the file is re-written again.

You should probably drop the header (auto-inserted), not add
it to sbuf, and always add the header when updating the
password file.

Mandy

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

2017-10-31 Thread Harsha Wardhana B


Hi Mandy,

Below is the new webrev incorporating below review comments.

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


On Monday 30 October 2017 11:34 PM, mandy chung wrote:

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


Looks okay in general.   Daniel is closer to the FileLoginModule 
implementation that I will count on his review.


FileLoginModule.java

225 if(hashPwdMgr == null) {
226 hashPwdMgr = new HashedPasswordManager(passwordFile, hashPasswords);
227 } else { 228 hashPwdMgr.loadPasswords(); 229 } Will hashPwdMgr be 
initialized multiple threads concurrently? Does this need to be 
synchronized?
Without synchronization, it would leave an orphan instance of 
HashedPasswordManager. Fixed it.
243 ace.setStackTrace(e.getStackTrace()); I think ace.initCause(e) 
instead of replacing the stack trace would help debugging. ACE should 
have been rev'ed to take the cause parameter (a separate issue).  
jmxremote.password.template 49 # 
https://docs.oracle.com/javase/7/docs/technotes/guides/security/StandardNames.html#MessageDigest


Please refer to JDK 9 docs.
255 "jmx.remote.x.password.tohashes";
305 # com.sun.management.jmxremote.password.tohashes = true|falseSince 
"to hashes" are two words, capitalize "H" is a recommended convention.

HashedPasswordManager.java

  214 Stream lines = Files.lines(Paths.get(passwordFile));

This should be called with try-with-resource.

Done.


It would be useful to record the timestamp of when the password
file is updated with the hashed passwords.

Added it as a part of the header. The header now looks like below.

# The passwords in this file are hashed.
# In order to change password for a role, replace hashed password entry
# with clear text password or new hashed password. If new password is in 
clear,

# it will be replaced with its hash when new login attempt is made.

# file last updated on - 10/31/2017 21:23:52

-Harsha

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

2017-10-30 Thread mandy chung


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


Looks okay in general.   Daniel is closer to the FileLoginModule 
implementation that I will count on his review.


FileLoginModule.java

225 if(hashPwdMgr == null) {
226 hashPwdMgr = new HashedPasswordManager(passwordFile, hashPasswords);
227 } else { 228 hashPwdMgr.loadPasswords(); 229 } Will hashPwdMgr be 
initialized multiple threads concurrently? Does this need to be 
synchronized? 243 ace.setStackTrace(e.getStackTrace()); I think 
ace.initCause(e) instead of replacing the stack trace would help 
debugging. ACE should have been rev'ed to take the cause parameter (a 
separate issue). jmxremote.password.template 49 # 
https://docs.oracle.com/javase/7/docs/technotes/guides/security/StandardNames.html#MessageDigest


Please refer to JDK 9 docs.
255 "jmx.remote.x.password.tohashes";
305 # com.sun.management.jmxremote.password.tohashes = true|falseSince 
"to hashes" are two words, capitalize "H" is a recommended convention.


HashedPasswordManager.java

 214 Stream lines = Files.lines(Paths.get(passwordFile));

This should be called with try-with-resource.

It would be useful to record the timestamp of when the password
file is updated with the hashed passwords.

Mandy

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

2017-10-27 Thread mandy chung

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

Hi,

Below is the updated webrev incorporating review comments from Daniel,
Roger and Mandy. The password file will now be locked before writing.

What is the link to the new webrev?

Mandy,

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.
Link to the documentation is required because the exact strings as
specified in the documentation must be specified. "Java Security
Standard Algorithm Names Specification" does not actually help. So I
have not removed the link to the documentation.

At least this should point to JDK 9 docs rather than JDK 7.

Mandy

-Harsha

On Thursday 12 October 2017 09:22 PM, Harsha Wardhana B wrote:

Sure. I will send out a modified webrev soon.

-Harsha

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

On 10/12/17 8:18 AM, Harsha Wardhana B wrote:

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.

"toHashes" suffix is also good to me.

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.

The other alternative is to override it with its hash value and
output a warning that this entry is ignored. This will leave it
for the user to remove the entries.

Mandy

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

2017-10-26 Thread Harsha Wardhana B

Sorry. Missed the webrev -

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

On Friday 27 October 2017 09:27 AM, Harsha Wardhana B wrote:

Hi,

Below is the updated webrev incorporating review comments from Daniel,
Roger and Mandy. The password file will now be locked before writing.

Mandy,

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:

-Harsha

On Thursday 12 October 2017 09:22 PM, Harsha Wardhana B wrote:

Sure. I will send out a modified webrev soon.

-Harsha

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

On 10/12/17 8:18 AM, Harsha Wardhana B wrote:

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

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

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.

"toHashes" suffix is also good to me.

The other alternative is to override it with its hash value and
output a warning that this entry is ignored. This will leave it
for the user to remove the entries.

Mandy

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

2017-10-26 Thread Harsha Wardhana B

Hi,

Below is the updated webrev incorporating review comments from Daniel,
Roger and Mandy. The password file will now be locked before writing.

Mandy,

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:

-Harsha

On Thursday 12 October 2017 09:22 PM, Harsha Wardhana B wrote:

Sure. I will send out a modified webrev soon.

-Harsha

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

On 10/12/17 8:18 AM, Harsha Wardhana B wrote:

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

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

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.

"toHashes" suffix is also good to me.

The other alternative is to override it with its hash value and
output a warning that this entry is ignored. This will leave it for
the user to remove the entries.

Mandy

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

2017-10-12 Thread Harsha Wardhana B

Sure. I will send out a modified webrev soon.

-Harsha

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

On 10/12/17 8:18 AM, Harsha Wardhana B wrote:

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

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

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.

"toHashes" suffix is also good to me.

The other alternative is to override it with its hash value and output
a warning that this entry is ignored. This will leave it for the
user to remove the entries.

Mandy

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

2017-10-12 Thread mandy chung




On 10/12/17 8:18 AM, Harsha Wardhana B wrote:




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. 


"toHashes" suffix is also good to me.


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.




The other alternative is to override it with its hash value and output a 
warning that this entry is ignored.   This will leave it for the user to 
remove the entries.


Mandy

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

2017-10-12 Thread 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

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

2017-10-12 Thread 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

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

2017-10-12 Thread Harsha Wardhana B


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

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

2017-10-11 Thread Harsha Wardhana B


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

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

2017-10-11 Thread Harsha Wardhana B


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 initializ

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

2017-10-11 Thread mandy chung




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

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

2017-10-11 Thread 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 
HashedPasswordMa

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

2017-10-11 Thread 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.


Rog

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

2017-10-08 Thread 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. Commen

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

2017-10-06 Thread 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

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

2017-10-05 Thread 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? Variabl

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

2017-10-04 Thread 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  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 
fin

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

2017-10-03 Thread 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  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.





* Co

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

2017-10-03 Thread Harsha Wardhana B


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

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

2017-05-07 Thread 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  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


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

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

2017-04-26 Thread 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  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


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

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

2017-04-25 Thread Roger Riggs


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


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

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

2017-04-24 Thread 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.


Gruss
Bernd
--
http://bernd.eckenfels.net

*From:* serviceability-dev 
 on behalf of Harsha 
Wardhana B 

*Sent:* Sunday, April 23, 2017 12:20:57 PM
*To:* serviceability-dev@openjdk.java.net
*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

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

2017-04-23 Thread Bernd Eckenfels

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.

Gruss
Bernd
--
http://bernd.eckenfels.net

From: serviceability-dev  on 
behalf of Harsha Wardhana B 
Sent: Sunday, April 23, 2017 12:20:57 PM
To: serviceability-dev@openjdk.java.net
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

38 matches

Mail list logo