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
