from:"Harsha Wardhana B"




On Wednesday 21 February 2018 01:51 AM, mandy chung wrote:

The code review and CSR review can be in parallel.  For this case,
I agree with Kumar to have CSR written that would help the code
review. Please specify the behavior and its relationship with
jcmd and other relevant diagnosability tools.

ok.


On 2/20/18 6:41 AM, Kumar Srinivasan wrote:


What is the behavior when -Dcom.sun.management.jmxremote.port=1234 
--start-management-agent port=2345 
-Dcom.sun.management.jmxremote.port=3456?


What is the value of the system property 
com.sun.management.jmxremote.port at runtime?  What port number 
does the management server start with?
As said earlier, values set via new flags override values set by -D 
flags. So 2345 will be the value of 
com.sun.management.jmxremote.port. Added a test case to validate that.


VM options are the last one wins if same option specified multiple 
times.  In this case, it could cause confusion (the last -D option 
sets the value to 3456 but it's set to a different value).


Why not taking the simplest approach - when --start-management-agent 
is set, it does not accept mixing the old way (i.e. does not accept 
the management properties to be set via -D)?   This RFE is to make the 
command-line simpler and ease-of-use.  I don't see any downside to 
migrate entirely to the new form.
We cannot get rid of specifying options via -D. We have plenty of -D 
flags but very few have short-hand alternative via 
--start-management-agent. If management properties are specified by 
--start-management-agent, the options specified by -D are anyway 
overwritten if specified.


Mandy

Harsha

Re: RFR : JDK-8196028 : JMX: Not enough JDP packets received before timeout


[Correcting bug Id in the subject]

Thanks Erik.

Harsha

On Tuesday 20 February 2018 08:44 PM, Erik Gahlin wrote:

Looks OK.

Erik


Hi All,

Please find the fix below for the Jdp test-case.

issue: https://bugs.openjdk.java.net/browse/JDK-8196028
webrev : http://cr.openjdk.java.net/~hb/8196028/webrev.00/

Fix details : The test was receiving JDP packets from other VM and 
hence the multi-cast socket was not timing-out. The default timeout 
handler was causing test to fail. Added a shutdown method that passes 
the test in case of timeout.


Thanks
Harsha

Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

Ping. Could I please have reviews for below webrev.

Thanks
Harsha

On Wednesday 14 February 2018 09:59 PM, Harsha Wardhana B wrote:

Hi,

Below is the updated webrev.

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

On Wednesday 14 February 2018 01:15 AM, mandy chung wrote:

On 2/13/18 1:30 AM, Harsha Wardhana B wrote:

Hi,

Please find below the revised webrev.

http://cr.openjdk.java.net/~hb/8187498/webrev.02/

On Friday 09 February 2018 05:07 AM, mandy chung wrote:

On 2/7/18 1:19 AM, Harsha Wardhana B wrote:
> 
> --start-management-agent will not be recognized in the current format and

> hence will not default to --start-management-agent=local=true.

`--start-local-management-server` is one option as Alan suggests.
Another option is to make `--start-management-agent` to accept
an optional argument.  VM can accept `--start-management-agent`
or `--start-management-agent=port=1234,ssl=on`.  It may require
more launcher change to support it.
Yes. It requires lot more changes to launcher. Hence optional 
argument to --start-management-agent was not added in order to keep 
the launcher impact minimum.

This is just one option for you to consider.   So the current 
proposal of the new option to start a remote management server, right?
Not necessarily. --start-management-agent=local=true starts local 
server and --start-management-agent=port=1234 starts remote management 
server.

Agent.java
  If --start-management-agent is set, -Dcom.sun.management.* takes
  precedence.  Do you really want to do that?  The new VM option
  intends to simplify the command-line to type in.  I think it's
  cleaner and reasonable if --start-management-agent is specified,
  -Dcom.sun.management.* are ignored (maybe worth emit a warning if set)
We can probably document that -D options will be overridden instead 
of emitting a warning.

What is the behavior when -Dcom.sun.management.jmxremote.port=1234 
--start-management-agent port=2345 
-Dcom.sun.management.jmxremote.port=3456?

What is the value of the system property 
com.sun.management.jmxremote.port at runtime?  What port number does 
the management server start with?
As said earlier, values set via new flags override values set by -D 
flags. So 2345 will be the value of com.sun.management.jmxremote.port. 
Added a test case to validate that.

  The implementation uses VMManagement::getVmArguments and scan
  the VM options for -start-management-agent.  The VM is the one
  invoking jdk.internal.agent.Agent::startAgent.  A simpler option
  is to change Agent::startAgent to take an argument parameter
  that will be passed with the argument of --start-management-agent
  or null if not set.

  Similarly for startRemoteManagementAgent and startLocalAgent_name.
Implementing this change requires lot of changes to argument parsing 
in native code and hence it is simpler to handle this at java layer.

Can you describe how --start-management-agent option will be used for 
jcmd and any other tool that attaches to a running VM to start the 
agent?  Example command-line will be useful.
--start-management-agent cannot be used by jcmd (or dynamic attach) as 
it accepts flags only in -D format or -D flags with com.sun.management 
removed. That code to parse string passed via jcmd was a mistake and 
hence I have removed it.

Mandy

-Harsha

RFR : JDK-8196744 : JMX: Not enough JDP packets received before timeout


Hi All,

Please find the fix below for the Jdp test-case.

issue: https://bugs.openjdk.java.net/browse/JDK-8196028
webrev : http://cr.openjdk.java.net/~hb/8196028/webrev.00/

Fix details : The test was receiving JDP packets from other VM and hence 
the multi-cast socket was not timing-out. The default timeout handler 
was causing test to fail. Added a shutdown method that passes the test 
in case of timeout.


Thanks
Harsha

Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

2018-02-14 Thread Harsha Wardhana B

Hi,

Below is the updated webrev.

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

On Wednesday 14 February 2018 01:15 AM, mandy chung wrote:

On 2/13/18 1:30 AM, Harsha Wardhana B wrote:

Hi,

Please find below the revised webrev.

http://cr.openjdk.java.net/~hb/8187498/webrev.02/

On Friday 09 February 2018 05:07 AM, mandy chung wrote:

On 2/7/18 1:19 AM, Harsha Wardhana B wrote:
> 
> --start-management-agent will not be recognized in the current format and

> hence will not default to --start-management-agent=local=true.

`--start-local-management-server` is one option as Alan suggests.
Another option is to make `--start-management-agent` to accept
an optional argument.  VM can accept `--start-management-agent`
or `--start-management-agent=port=1234,ssl=on`.  It may require
more launcher change to support it.
Yes. It requires lot more changes to launcher. Hence optional 
argument to --start-management-agent was not added in order to keep 
the launcher impact minimum.

This is just one option for you to consider.   So the current proposal 
of the new option to start a remote management server, right?
Not necessarily. --start-management-agent=local=true starts local server 
and --start-management-agent=port=1234 starts remote management server.

Agent.java
  If --start-management-agent is set, -Dcom.sun.management.* takes
  precedence.  Do you really want to do that?  The new VM option
  intends to simplify the command-line to type in.  I think it's
  cleaner and reasonable if --start-management-agent is specified,
  -Dcom.sun.management.* are ignored (maybe worth emit a warning if set)
We can probably document that -D options will be overridden instead 
of emitting a warning.

What is the behavior when -Dcom.sun.management.jmxremote.port=1234 
--start-management-agent port=2345 
-Dcom.sun.management.jmxremote.port=3456?

What is the value of the system property 
com.sun.management.jmxremote.port at runtime?  What port number does 
the management server start with?
As said earlier, values set via new flags override values set by -D 
flags. So 2345 will be the value of com.sun.management.jmxremote.port. 
Added a test case to validate that.

  The implementation uses VMManagement::getVmArguments and scan
  the VM options for -start-management-agent.  The VM is the one
  invoking jdk.internal.agent.Agent::startAgent.  A simpler option
  is to change Agent::startAgent to take an argument parameter
  that will be passed with the argument of --start-management-agent
  or null if not set.

  Similarly for startRemoteManagementAgent and startLocalAgent_name.
Implementing this change requires lot of changes to argument parsing 
in native code and hence it is simpler to handle this at java layer.

Can you describe how --start-management-agent option will be used for 
jcmd and any other tool that attaches to a running VM to start the 
agent?  Example command-line will be useful.
--start-management-agent cannot be used by jcmd (or dynamic attach) as 
it accepts flags only in -D format or -D flags with com.sun.management 
removed. That code to parse string passed via jcmd was a mistake and 
hence I have removed it.

Mandy

-Harsha

Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

2018-02-13 Thread Harsha Wardhana B

Hi,

Please find below the revised webrev.

http://cr.openjdk.java.net/~hb/8187498/webrev.02/

On Friday 09 February 2018 05:07 AM, mandy chung wrote:

On 2/7/18 1:19 AM, Harsha Wardhana B wrote:
> 
> --start-management-agent will not be recognized in the current format and

> hence will not default to --start-management-agent=local=true.

`--start-local-management-server` is one option as Alan suggests.
Another option is to make `--start-management-agent` to accept
an optional argument.  VM can accept `--start-management-agent`
or `--start-management-agent=port=1234,ssl=on`.  It may require
more launcher change to support it.
Yes. It requires lot more changes to launcher. Hence optional argument 
to --start-management-agent was not added in order to keep the launcher 
impact minimum.

> Below is the webrev with above changes and corresponding tests.
>
> http://cr.openjdk.java.net/~hb/8187498/webrev.01/

arguments.cpp

3216 if (FLAG_SET_CMDLINE(bool, ManagementServer, true) != 
Flag::SUCCESS) {
3217   return JNI_EINVAL;
3218 }
3219 // management agent in module jdk.management.agent
3220 if (!create_numbered_property("jdk.module.addmods", 
"jdk.management.agent", addmods_count++)) {
3221   return JNI_ENOMEM;
3222 }

- you can consider refactor this to replace this block and line 2988-3994.

3224 jio_fprintf(defaultStream::output_stream(),
3225   "-Xmanagement is not supported in this VM.\n");

-Xmanagement not yet renamed
java.c
   To set the precedence for future VM long form options, I suggest to
rename IsManagementOption to IsVMLongFormOption and change the error
message not specific to management agent.

Incorporated above changes.

Agent.java
  If --start-management-agent is set, -Dcom.sun.management.* takes
  precedence.  Do you really want to do that?  The new VM option
  intends to simplify the command-line to type in.  I think it's
  cleaner and reasonable if --start-management-agent is specified,
  -Dcom.sun.management.* are ignored (maybe worth emit a warning if set)
We can probably document that -D options will be overridden instead of 
emitting a warning.

  The implementation uses VMManagement::getVmArguments and scan
  the VM options for -start-management-agent.  The VM is the one
  invoking jdk.internal.agent.Agent::startAgent.  A simpler option
  is to change Agent::startAgent to take an argument parameter
  that will be passed with the argument of --start-management-agent
  or null if not set.

  Similarly for startRemoteManagementAgent and startLocalAgent_name.
Implementing this change requires lot of changes to argument parsing in 
native code and hence it is simpler to handle this at java layer.

  parseXmgmtArgs should be renamed as no longer -Xmgmt. Better not
  to tightly coupled with the option name.

Ok.

  291 if(args.startsWith("--start-management-agent")) {
  292 return parseXmgmtArgs(args);
  293 }

What is this change for?

This is required when management agent is started via attach mechanism.

Mandy

-Harsha

Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

2018-02-07 Thread Harsha Wardhana B

Hi Kumar,

Thanks for your inputs.

On Wednesday 07 February 2018 09:02 PM, Kumar Srinivasan wrote:

Hi Harsha,

Changes look reasonable to me, couple of things that must be addressed:

1. Since this is a main-stream launcher change with a documented and
supported
option, a CSR is required, you have to add and document the option
in the help page
http://hg.openjdk.java.net/jdk/jdk/file/8cc67294ec56/src/java.base/share/classes/sun/launcher/resources/launcher.properties

2. You also have to create a doc bug so that the doc team will
document it in the Tools
Reference Guide, and link it to this bug. Does it need a Release note
? probably does,
in which case you will have to create Release Note subtask and follow
the RN process.

Will take care of above items.

3. Is XmanagementAgentTest.java part of tier1 test suite ? If not,
then I think it ought
to be in tier1 grouping, perhaps best to park this under
jdk/tools/launcher/management ?

Ok. I will move the test in subsequent webrev.

-Harsha

Kumar

Hi All,

After internal discussions, below format for management flags was
agreed upon.

1. --start-management-agent port=1234,ssl=on (space seperator)
or
2. --start-management-agent=port=1234,ssl=on ('=' seperator)

If option 1 is specified, it will be converted to option 2 by the
java launcher before it is passed onto VM.

With above GNU long format for management options, specifying
arguments is mandatory unlike before.

--start-management-agent will not be recognized in the current format
and hence will not default to --start-management-agent=local=true.

Below is the webrev with above changes and corresponding tests.

http://cr.openjdk.java.net/~hb/8187498/webrev.01/

Please review and comment.

Thanks
Harsha

On Monday 29 January 2018 03:14 PM, Harsha Wardhana B wrote:

Hi Alan,

I am not fully aware about Java launcher or how it passes options to
VM. Let me check with some other folks and get back to you.

Thanks
Harsha

On Monday 29 January 2018 01:55 PM, Alan Bateman wrote:

On 29/01/2018 05:20, Harsha Wardhana B wrote:

Hi Mandy,Alan,

Thanks for your inputs.
If I keep it as launcher option, it may need to know JMX agent
flags which may need to be extended in future.
I would prefer making it a VM option. I will make the required
changes and send out an updated webrev.
I think Mandy's suggestion is to just transform --management
so a form that the VM can read. The launcher will need to
replace the space anyway as the VM only accepts "=".

-Alan

Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

2018-02-07 Thread Harsha Wardhana B


Hi All,

After internal discussions, below format for management flags was agreed 
upon.


1. --start-management-agent port=1234,ssl=on        (space seperator)
or
2. --start-management-agent=port=1234,ssl=on        ('=' seperator)

If option 1 is specified, it will be converted to option 2 by the java 
launcher before it is passed onto VM.


With above GNU long format for management options, specifying arguments 
is mandatory unlike before.


--start-management-agent will not be recognized in the current format 
and hence will not default to --start-management-agent=local=true.


Below is the webrev with above changes and corresponding tests.

http://cr.openjdk.java.net/~hb/8187498/webrev.01/

Please review and comment.

Thanks
Harsha

On Monday 29 January 2018 03:14 PM, Harsha Wardhana B wrote:

Hi Alan,

I am not fully aware about Java launcher or how it passes options to 
VM. Let me check with some other folks and get back to you.


Thanks
Harsha

On Monday 29 January 2018 01:55 PM, Alan Bateman wrote:



On 29/01/2018 05:20, Harsha Wardhana B wrote:

Hi Mandy,Alan,

Thanks for your inputs.
If I keep it as launcher option, it may need to know JMX agent flags 
which may need to be extended in future.
I would prefer making it a VM option. I will make the required 
changes and send out an updated webrev.
I think Mandy's suggestion is to just transform --management 
 so a form that the VM can read. The launcher will need to 
replace the space anyway as the VM only accepts "=".


-Alan

Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

2018-01-29 Thread Harsha Wardhana B


Hi Alan,

I am not fully aware about Java launcher or how it passes options to VM. 
Let me check with some other folks and get back to you.


Thanks
Harsha

On Monday 29 January 2018 01:55 PM, Alan Bateman wrote:



On 29/01/2018 05:20, Harsha Wardhana B wrote:

Hi Mandy,Alan,

Thanks for your inputs.
If I keep it as launcher option, it may need to know JMX agent flags 
which may need to be extended in future.
I would prefer making it a VM option. I will make the required 
changes and send out an updated webrev.
I think Mandy's suggestion is to just transform --management  
so a form that the VM can read. The launcher will need to replace the 
space anyway as the VM only accepts "=".


-Alan

Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

2018-01-28 Thread Harsha Wardhana B


Hi Mandy,Alan,

Thanks for your inputs.
If I keep it as launcher option, it may need to know JMX agent flags 
which may need to be extended in future.
I would prefer making it a VM option. I will make the required changes 
and send out an updated webrev.


-Harsha

On Thursday 25 January 2018 09:31 PM, mandy chung wrote:

Hi Harsha,

JEP 293 [1] describes the guidelines for JDK command-line options.  As 
Alan points out, new options should move away from -X prefix but use 
`--` GNU-style long form option.  The guideline says:


The use of |-X| as a prefix to indicate "non-standard" options will be 
discontinued for new options, although command-line help may continue 
to draw a distinction between more commonly used options and those for 
advanced use.


You can consider `--management` as an alternative.  Should this be a 
launcher option that converts it to the corresponding 
`-Dcom.sun.management.jmxremote.` rather than a VM option?


Mandy
[1] http://openjdk.java.net/jeps/293

On 1/24/18 11:21 PM, Harsha Wardhana B wrote:


Hi Erik,

The minimal command line would be,

"-Xmanagement", that will start only the local management server.

"-Xmanagement:local=true,port=" will start the remote management 
server without SSL or authentication.



On Wednesday 24 January 2018 06:13 PM, Erik Gahlin wrote:

Hi Harsha,

Very nice to see progress on this!

Before reviewing, the minimal command line to start up the default 
management server now becomes


-Xmanagement:ssl=false,authenticate=false

No. Please refer above for minimal options.


and if you use a property that doesn't exist, or that is mandatory, 
you will get an error message stating what is wrong?
If we use property, that doesn't exist, we get invalid option error. 
As said before, no options are mandatory.

///
//./java 
-Xmanagement:ssl=true,authenticate=false,rmiregistry_ssl=true 
HelloWorld//

//Error: Invalid option specified: rmiregistry_ssl//
///


Could we reduce the command line further, so only a single property 
is needed:


-Xmanagement:secure=false

or perhaps:

-Xmanagement:unsecure

which would set ssl=false,authenticate=false, because that is what 
you want 99% of the time.


Thanks
Erik


Thanks
Harsha

Hi,

Please review the changes for above enhancement having webrev at,

http://cr.openjdk.java.net/~hb/8187498/webrev.00/

Thanks
Harsha

Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

2018-01-24 Thread Harsha Wardhana B


Hi Erik,

The minimal command line would be,

"-Xmanagement", that will start only the local management server.

"-Xmanagement:local=true,port=" will start the remote management 
server without SSL or authentication.



On Wednesday 24 January 2018 06:13 PM, Erik Gahlin wrote:

Hi Harsha,

Very nice to see progress on this!

Before reviewing, the minimal command line to start up the default 
management server now becomes


-Xmanagement:ssl=false,authenticate=false

No. Please refer above for minimal options.


and if you use a property that doesn't exist, or that is mandatory, 
you will get an error message stating what is wrong?
If we use property, that doesn't exist, we get invalid option error. As 
said before, no options are mandatory.

///
//./java -Xmanagement:ssl=true,authenticate=false,rmiregistry_ssl=true 
HelloWorld//

//Error: Invalid option specified: rmiregistry_ssl//
///


Could we reduce the command line further, so only a single property is 
needed:


-Xmanagement:secure=false

or perhaps:

-Xmanagement:unsecure

which would set ssl=false,authenticate=false, because that is what you 
want 99% of the time.


Thanks
Erik


Thanks
Harsha

Hi,

Please review the changes for above enhancement having webrev at,

http://cr.openjdk.java.net/~hb/8187498/webrev.00/

Thanks
Harsha

RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

2018-01-24 Thread Harsha Wardhana B


Hi,

Please review the changes for above enhancement having webrev at,

http://cr.openjdk.java.net/~hb/8187498/webrev.00/

Thanks
Harsha

Re: RFR : JDK-8175542 - JMX: Not enough JDP packets received

2018-01-11 Thread Harsha Wardhana B


Hi Amit,

Looks okay to me.

Thanks

Harsha


On Wednesday 03 January 2018 05:54 PM, Amit Sapre wrote:


Thanks Harsha for the inputs.

I made changes in this webrev : 
http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8175542/webrev.01/ 
<http://cr.openjdk.java.net/%7Easapre/webrev/2018/JDK-8175542/webrev.01/>


Thanks,

Amit

*From:*Harsha Wardhana B
*Sent:* Wednesday, December 13, 2017 5:32 PM
*To:* serviceability-dev@openjdk.java.net
*Subject:* Re: RFR : JDK-8175542 - JMX: Not enough JDP packets received

Hi Amit,

Increasing the timeout 'TIME_OUT_FACTOR' will increase both socket and 
test-case timeout. The test passed as can be seen from the log, but 
because of the race-condition: hasTestLivedLongEnough() checked before 
shouldContinue(), the test was declared failed because of time-out.


One possible fix would be to change line 80 to,

        if (shouldContinue() && hasTestLivedLongEnough())

Thanks

Harsha

On Wednesday 13 December 2017 11:03 AM, Amit Sapre wrote:

Hello,

Please review the timeout fix for this bug.

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8175542

Webrev :
http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8175542/webrev.00/
<http://cr.openjdk.java.net/%7Easapre/webrev/2017/JDK-8175542/webrev.00/>

Thanks,

Amit

Re: RFR : JDK-8175542 - JMX: Not enough JDP packets received

2017-12-13 Thread Harsha Wardhana B


Hi Amit,

Increasing the timeout 'TIME_OUT_FACTOR' will increase both socket and 
test-case timeout. The test passed as can be seen from the log, but 
because of the race-condition: hasTestLivedLongEnough() checked before 
shouldContinue(), the test was declared failed because of time-out.


One possible fix would be to change line 80 to,

        if (shouldContinue() && hasTestLivedLongEnough())

Thanks

Harsha


On Wednesday 13 December 2017 11:03 AM, Amit Sapre wrote:


Hello,

Please review the timeout fix for this bug.

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8175542

Webrev : 
http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8175542/webrev.00/ 



Thanks,

Amit

Re: [TestBug] RFR : JDK-8192909 - Invalid username or password in HashedPasswordFileTest.java

2017-12-05 Thread Harsha Wardhana B


Thanks Daniel and Christoph for review.

-Harsha

On Tuesday 05 December 2017 08:11 PM, Daniel Fuchs wrote:

+1

-- daniel

On 05/12/2017 12:04, Harsha Wardhana B wrote:

Hi Daniel,


On Tuesday 05 December 2017 03:42 PM, Daniel Fuchs wrote:

Hi Harsha,

Looks good.

Thanks for the review.


nit:

 366 if(random.nextBoolean()) {
 367 String[] tokens = line.split("\\s+");
 368 if ((tokens.length == 4 || tokens.length == 
3)) {


inverting the two if () (testing for the applicability of the line
first) would probably give a better chance that an existing
password is replaced, unless most lines are applicable.
All the lines will be applicable. The password file will be hashed 
before the above lines are executed. An Assert statement at line 353 
makes sure of that. Hence no point inverting the two if().


best regards,

-- daniel

Regards
Harsha


n 04/12/2017 18:27, Harsha Wardhana B wrote:

Hi All,

Please review and provide comments for fix for,

issue: https://bugs.openjdk.java.net/browse/JDK-8192909

having webrev at,

webrev : http://cr.openjdk.java.net/~hb/8192909/webrev.00/

Fix details: The test was failing intermittently because of 
duplicate entries for role in input password file. The duplicate 
entries get over-written by JMX agent, but the client was testing 
with stale entries for duplicated role. Also, the test now uses a 
single random number generator from test package 
(Utils.getRandomInstance) instead of two.


Regards

Harsha

Re: [TestBug] RFR : JDK-8192909 - Invalid username or password in HashedPasswordFileTest.java

2017-12-05 Thread Harsha Wardhana B


Hi Daniel,


On Tuesday 05 December 2017 03:42 PM, Daniel Fuchs wrote:

Hi Harsha,

Looks good.

Thanks for the review.


nit:

 366 if(random.nextBoolean()) {
 367 String[] tokens = line.split("\\s+");
 368 if ((tokens.length == 4 || tokens.length == 
3)) {


inverting the two if () (testing for the applicability of the line
first) would probably give a better chance that an existing
password is replaced, unless most lines are applicable.
All the lines will be applicable. The password file will be hashed 
before the above lines are executed. An Assert statement at line 353 
makes sure of that. Hence no point inverting the two if().


best regards,

-- daniel

Regards
Harsha


n 04/12/2017 18:27, Harsha Wardhana B wrote:

Hi All,

Please review and provide comments for fix for,

issue: https://bugs.openjdk.java.net/browse/JDK-8192909

having webrev at,

webrev : http://cr.openjdk.java.net/~hb/8192909/webrev.00/

Fix details: The test was failing intermittently because of duplicate 
entries for role in input password file. The duplicate entries get 
over-written by JMX agent, but the client was testing with stale 
entries for duplicated role. Also, the test now uses a single random 
number generator from test package (Utils.getRandomInstance) instead 
of two.


Regards

Harsha

Re: [TestBug] RFR : JDK-8192909 - Invalid username or password in HashedPasswordFileTest.java

2017-12-04 Thread Harsha Wardhana B


Thanks Christoph for the review.

Can I get one more review please?

-Harsha

On Tuesday 05 December 2017 11:29 AM, Langer, Christoph wrote:

Hi Harsha,

this looks good to me.

Small finding: Line 366, there could be a space between if and (.

Best regards
Christoph


-Original Message-
From: serviceability-dev [mailto:serviceability-dev-
boun...@openjdk.java.net] On Behalf Of Harsha Wardhana B
Sent: Montag, 4. Dezember 2017 19:28
To: serviceability-dev@openjdk.java.net
Subject: [TestBug] RFR : JDK-8192909 - Invalid username or password in
HashedPasswordFileTest.java

Hi All,

Please review and provide comments for fix for,

issue: https://bugs.openjdk.java.net/browse/JDK-8192909

having webrev at,

webrev : http://cr.openjdk.java.net/~hb/8192909/webrev.00/

Fix details: The test was failing intermittently because of duplicate
entries for role in input password file. The duplicate entries get
over-written by JMX agent, but the client was testing with stale entries
for duplicated role. Also, the test now uses a single random number
generator from test package (Utils.getRandomInstance) instead of two.

Regards

Harsha

[TestBug] RFR : JDK-8192909 - Invalid username or password in HashedPasswordFileTest.java

2017-12-04 Thread Harsha Wardhana B


Hi All,

Please review and provide comments for fix for,

issue: https://bugs.openjdk.java.net/browse/JDK-8192909

having webrev at,

webrev : http://cr.openjdk.java.net/~hb/8192909/webrev.00/

Fix details: The test was failing intermittently because of duplicate 
entries for role in input password file. The duplicate entries get 
over-written by JMX agent, but the client was testing with stale entries 
for duplicated role. Also, the test now uses a single random number 
generator from test package (Utils.getRandomInstance) instead of two.


Regards

Harsha

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

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.

-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

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

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




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


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 HashedPasswordManage

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


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 t

Re: RFR : JDK-8044122 MBean access to the PID

2017-10-10 Thread Harsha Wardhana B


Hi Ujwal,

Could you please add a test-case to validate your changes? You can spawn 
a new process and it can exchange its pid to its parent via 
System.out/in or via sockets.


Also, VMManagementImpl:145, the change from getProcessId to getVmPid 
seems unnecessary.


-Harsha


On Tuesday 10 October 2017 05:17 PM, Ujwal Vangapally wrote:

Kindly review the changes made.

https://bugs.openjdk.java.net/browse/JDK-8044122

webrev : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/


CSR : https://bugs.openjdk.java.net/browse/JDK-8189091

Thanks,

Ujwal.

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

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

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 pl

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.


* HashedPasswordFil

Re: jmx-dev JEP review : JDK-8171311 - REST APIs for JMX

2017-09-12 Thread Harsha Wardhana B


Hi Kirk,Erik,

The current JEP addresses the first use-case. Second use case can be 
realized by adding a JMXConnector that operates over REST APIs provided 
by the current JEP. But that is outside the scope of this JEP.


-Harsha


On Tuesday 12 September 2017 04:27 PM, Kirk Pepperdine wrote:


On Sep 12, 2017, at 12:44 PM, Erik Gahlin <erik.gah...@oracle.com 
<mailto:erik.gah...@oracle.com>> wrote:


I guess there are two use cases:

1) Simple interoperability with other languages.
2) A drop in replacement for RMI

Can a JMX connector be written that support both use cases? I don't 
know, but if not it could be that we need both a connector and an 
adapter.


I think if you were to extend JMXConnector to wrap the REST API you 
might be able to expose both. But I’m not sure it would be a great 
solution. I think a second JEP would be a better option.


— Kirk



Erik


Hi Kirk,

I guess the term 'connector' here is loosely applied. When I say 
connector, I mean the connector that provides implementation of the 
package below,


https://docs.oracle.com/javase/8/docs/api/javax/management/remote/package-summary.html

RMIConnector is one implementation of above connector.


On Tuesday 12 September 2017 12:56 PM, Kirk Pepperdine wrote:

Hi Harsha,

From Chapter 5 of the JMX documentation. "Many different 
implementations of connectors are possible. In particular, there 
are many possibilities for the protocol used to communicate over a 
connection between client and server.”


It goes on in the Generic Connector section under "User-Defined 
Protocols” to say; "While the RMI connector must be present in 
every implementation of the JMX Remote API, you can also implement 
a connector based on a protocol that is not defined in the JMX 
Remote API standard. A typical example of this is a connector based 
on a protocol that uses HTTP/S. Other protocols are also possible. 
The JMX specification describes how to implement a connector based 
on a user-defined protocol.”


Unless I’m missing something, this all suggests that there is 
nothing inherently wrong is using REST behind a JMXConnector.
I hope above should clarify what I refer to when I say JMXConnector. 
In that sense, REST APIs alone cannot work as connector. In fact, it 
stands parallel to connector, as in it directly wraps the 
MBeanServer and does not wrap any JMXConnector. The JEP has detailed 
information about where the REST adapter sits in the JMX architecture.


Are you suggesting that we implement a JMXConnector that works over 
REST?


As written this JEP pretty much looks like Jolokia. Jolokia is a 
great project and as such I fail to see the benefits of simply 
duplicating it. I’d also argue that the usefulness of that project 
has been some what muted because it is not a drop in replacement 
for the standard RMI connector meaning that one has to modify an 
entire tool chain just to make use of it. However, creating a REST 
based JMXConnector would be immediately useful.
As an aside, Jus last week I started on a JMXConnector that uses a 
shared memory segment for communications. Of course this 
implementation would only be available for local communications but 
it offers some advantages over using a socket based protocol, even 
if that comms is over local loopback.


Kind regards,
Kirk Pepperdine


Thanks
Harsha




On Sep 12, 2017, at 9:04 AM, Harsha Wardhana B 
<harsha.wardhan...@oracle.com> wrote:


Hi Kirk,

REST APIs work as an adapter and cannot work as a connector. To 
quote from the JEP,



The REST adapter is a part of the Distributed services level. 
Connectors mirror the interfaces of agent level services to 
remote clients, whereas adapters transform agent level services 
to different protocol. The proposed functionality will transform 
Agent level services to REST APIs, hence the name "REST adapter".
A connector *must* adhere to the JMX remoting spec. REST APIs 
cannot adhere to that because they expose APIs via HTTP and not 
Java. Hence it is called an Adapter and not a connector. It can 
never work as a 'drop-in' replacement for JMX/RMI Connector. 
Existing tools using using RMIConnector will have to be modified 
to use REST APIs.


The current JEP does not allow all of the CRUD operations on 
MBeans. In the spirit of keeping the APIs language agnostic, only 
read/write is supported. It is not possible to specify 
create/delete REST APIs for JMX without incorporating language 
specific features. I would welcome discussions around including 
create/delete APIs for MBeans.


In lieu of the above, as of now REST adapter cannot exist 
independently and will have to live along-side RMIConnector.


-Harsha


On Monday 11 September 2017 09:05 PM, Kirk Pepperdine wrote:

Hi Harsha,

The only reason I mentioned Jolokia is that it’s a project that 
usefulness is some what limited because it is *not* a compliment 
JMX connector and as such cannot be used as a straight drop-in 
replacement for th

Re: jmx-dev JEP review : JDK-8171311 - REST APIs for JMX

2017-09-12 Thread Harsha Wardhana B


Hi Kirk,

I guess the term 'connector' here is loosely applied. When I say 
connector, I mean the connector that provides implementation of the 
package below,


https://docs.oracle.com/javase/8/docs/api/javax/management/remote/package-summary.html

RMIConnector is one implementation of above connector.


On Tuesday 12 September 2017 12:56 PM, Kirk Pepperdine wrote:

Hi Harsha,

From Chapter 5 of the JMX documentation. "Many different 
implementations of connectors are possible. In particular, there are 
many possibilities for the protocol used to communicate over a 
connection between client and server.”


It goes on in the Generic Connector section under "User-Defined 
Protocols” to say; "While the RMI connector must be present in every 
implementation of the JMX Remote API, you can also implement a 
connector based on a protocol that is not defined in the JMX Remote 
API standard. A typical example of this is a connector based on a 
protocol that uses HTTP/S. Other protocols are also possible. The JMX 
specification describes how to implement a connector based on a 
user-defined protocol.”


Unless I’m missing something, this all suggests that there is 
nothing inherently wrong is using REST behind a JMXConnector.
I hope above should clarify what I refer to when I say JMXConnector. In 
that sense, REST APIs alone cannot work as connector. In fact, it stands 
parallel to connector, as in it directly wraps the MBeanServer and does 
not wrap any JMXConnector. The JEP has detailed information about where 
the REST adapter sits in the JMX architecture.


Are you suggesting that we implement a JMXConnector that works over REST?


As written this JEP pretty much looks like Jolokia. Jolokia is a great 
project and as such I fail to see the benefits of simply duplicating 
it. I’d also argue that the usefulness of that project has been some 
what muted because it is not a drop in replacement for the standard 
RMI connector meaning that one has to modify an entire tool chain just 
to make use of it. However, creating a REST based JMXConnector would 
be immediately useful.
As an aside, Jus last week I started on a JMXConnector that uses a 
shared memory segment for communications. Of course this 
implementation would only be available for local communications but it 
offers some advantages over using a socket based protocol, even if 
that comms is over local loopback.


Kind regards,
Kirk Pepperdine


Thanks
Harsha




On Sep 12, 2017, at 9:04 AM, Harsha Wardhana B 
<harsha.wardhan...@oracle.com <mailto:harsha.wardhan...@oracle.com>> 
wrote:


Hi Kirk,

REST APIs work as an adapter and cannot work as a connector. To quote 
from the JEP,



The REST adapter is a part of the Distributed services level. 
Connectors mirror the interfaces of agent level services to remote 
clients, whereas adapters transform agent level services to 
different protocol. The proposed functionality will transform Agent 
level services to REST APIs, hence the name "REST adapter".
A connector *must* adhere to the JMX remoting spec. REST APIs cannot 
adhere to that because they expose APIs via HTTP and not Java. Hence 
it is called an Adapter and not a connector. It can never work as a 
'drop-in' replacement for JMX/RMI Connector. Existing tools using 
using RMIConnector will have to be modified to use REST APIs.


The current JEP does not allow all of the CRUD operations on MBeans. 
In the spirit of keeping the APIs language agnostic, only read/write 
is supported. It is not possible to specify create/delete REST APIs 
for JMX without incorporating language specific features. I would 
welcome discussions around including create/delete APIs for MBeans.


In lieu of the above, as of now REST adapter cannot exist 
independently and will have to live along-side RMIConnector.


-Harsha


On Monday 11 September 2017 09:05 PM, Kirk Pepperdine wrote:

Hi Harsha,

The only reason I mentioned Jolokia is that it’s a project that 
usefulness is some what limited because it is *not* a compliment JMX 
connector and as such cannot be used as a straight drop-in 
replacement for the current RMI based connector. Is your plan here 
to make it a fully compliant connector so that we could configure 
tooling such as the MBean viewers in jConsole and VisualVM (or JMC 
for that matter) to use a restful connector instead of an RMI based 
connector? IMHO, doing so would represent a huge win as I know of 
quite a few projects that cannot or will not use JMX because of it’s 
reliance on RMI.


Consolidating all of the options under a single flag looks like 
another interesting win.


Kind regards,
Kirk



On Sep 11, 2017, at 4:08 PM, Harsha Wardhana B 
<harsha.wardhan...@oracle.com 
<mailto:harsha.wardhan...@oracle.com>> wrote:


Hi Erik,


On Monday 11 September 2017 07:14 PM, Erik Gahlin wrote:

Hi Harsha,

I haven't looked at Jolokia, or know what is the most reasonable 
approach in this case, but as a principle

Re: jmx-dev JEP review : JDK-8171311 - REST APIs for JMX

2017-09-12 Thread Harsha Wardhana B


Hi Kirk,

REST APIs work as an adapter and cannot work as a connector. To quote 
from the JEP,


The REST adapter is a part of the Distributed services level. 
Connectors mirror the interfaces of agent level services to remote 
clients, whereas adapters transform agent level services to different 
protocol. The proposed functionality will transform Agent level 
services to REST APIs, hence the name "REST adapter".
A connector *must* adhere to the JMX remoting spec. REST APIs cannot 
adhere to that because they expose APIs via HTTP and not Java. Hence it 
is called an Adapter and not a connector. It can never work as a 
'drop-in' replacement for JMX/RMI Connector. Existing tools using using 
RMIConnector will have to be modified to use REST APIs.


The current JEP does not allow all of the CRUD operations on MBeans. In 
the spirit of keeping the APIs language agnostic, only read/write is 
supported. It is not possible to specify create/delete REST APIs for JMX 
without incorporating language specific features. I would welcome 
discussions around including create/delete APIs for MBeans.


In lieu of the above, as of now REST adapter cannot exist independently 
and will have to live along-side RMIConnector.


-Harsha


On Monday 11 September 2017 09:05 PM, Kirk Pepperdine wrote:

Hi Harsha,

The only reason I mentioned Jolokia is that it’s a project that 
usefulness is some what limited because it is *not* a compliment JMX 
connector and as such cannot be used as a straight drop-in replacement 
for the current RMI based connector. Is your plan here to make it a 
fully compliant connector so that we could configure tooling such as 
the MBean viewers in jConsole and VisualVM (or JMC for that matter) to 
use a restful connector instead of an RMI based connector? IMHO, doing 
so would represent a huge win as I know of quite a few projects that 
cannot or will not use JMX because of it’s reliance on RMI.


Consolidating all of the options under a single flag looks like 
another interesting win.


Kind regards,
Kirk



On Sep 11, 2017, at 4:08 PM, Harsha Wardhana B 
<harsha.wardhan...@oracle.com <mailto:harsha.wardhan...@oracle.com>> 
wrote:


Hi Erik,


On Monday 11 September 2017 07:14 PM, Erik Gahlin wrote:

Hi Harsha,

I haven't looked at Jolokia, or know what is the most reasonable 
approach in this case, but as a principle, I think we should strive 
for the best possible design rather than trying to be compatible 
with third party tools.
Agreed. That will always be the first priority. That is the reason 
HTTP GET interfaces will not be changed. I am undecided if the POST 
payloads need to be changed (without compromising the REST design 
principles) to increase adoption of this feature.


How will the command line look like to start the agent with the rest 
adapter?


In the past there have been discussions about adding syntactic sugar 
for -Dcom.sun.management, i.e.


-Xmanagement:ssl=false,port=7091,authenticate=false

instead of

-Dcom.sun.management.jmxremote.ssl=false
-Dcom.sun.management.jmxremote.port=7091
-Dcom.sun.management.jmxremote.authenticate=false

which is hard to remember, cumbersome to write and error prone since 
the parameters are not validated. If we are adding support for REST, 
it could perhaps be default, i.e.


-Xmanagement:ssl=false,authenticate=false,port=80

If you want to use JMX over RMI you would specify protocol:

-Xmanagement:ssl=false,port=7091,authenticate=false,protocol=rmi
Yes. There is an enhancement request to add the -Xmanagemet:* 
syntatic sugar for -Dcom.sun.management.jmxremote.* flags. REST 
adapter will use one of the above flags though I haven't thought of 
the exact name for it yet. I will update the JEP with the details of 
the flag shortly.


Has there been any thoughts about JMX notifications?

Notifications will not be supported in this JEP.

  * MBean Notifications are not a widely used feature and will not be
supported via the REST adapter.



I know it is outside the scope of the JEP, but I think we should 
take it into consideration when doing the design, so the 
functionality could be added on later without too much difficulty.
Notifications can be added without modifying the current design too 
much. If required, it will be worked upon via an enhancement request.


Thanks
Erik


Thanks
Harsha


Hi Martin,

In my opinion, the interfaces exposed by current JEP are lot closer 
to REST style than the interfaces exposed by Jolokia.


For instance, HTTP GET by default should be used to read resources, 
but it is made part of URL in Jolokia interfaces.


/read///

I would wait on opinions from more people before considering 
changing the current interfaces.


Thanks
-Harsha

On Wednesday 06 September 2017 11:40 AM, Martin Skarsaune wrote:

Hello

Would one at least consider adopting the same URL paths and 
payloads as Jolokia? This could make life a lot easier for third 
party tools that connect to it.


Best Regards

Martin Skarsa

Re: jmx-dev JEP review : JDK-8171311 - REST APIs for JMX

2017-09-11 Thread Harsha Wardhana B


Hi Erik,


On Monday 11 September 2017 07:14 PM, Erik Gahlin wrote:

Hi Harsha,

I haven't looked at Jolokia, or know what is the most reasonable 
approach in this case, but as a principle, I think we should strive 
for the best possible design rather than trying to be compatible with 
third party tools.
Agreed. That will always be the first priority. That is the reason HTTP 
GET interfaces will not be changed. I am undecided if the POST payloads 
need to be changed (without compromising the REST design principles) to 
increase adoption of this feature.


How will the command line look like to start the agent with the rest 
adapter?


In the past there have been discussions about adding syntactic sugar 
for -Dcom.sun.management, i.e.


-Xmanagement:ssl=false,port=7091,authenticate=false

instead of

-Dcom.sun.management.jmxremote.ssl=false
-Dcom.sun.management.jmxremote.port=7091
-Dcom.sun.management.jmxremote.authenticate=false

which is hard to remember, cumbersome to write and error prone since 
the parameters are not validated. If we are adding support for REST, 
it could perhaps be default, i.e.


-Xmanagement:ssl=false,authenticate=false,port=80

If you want to use JMX over RMI you would specify protocol:

-Xmanagement:ssl=false,port=7091,authenticate=false,protocol=rmi
Yes. There is an enhancement request to add the -Xmanagemet:* syntatic 
sugar for -Dcom.sun.management.jmxremote.* flags. REST adapter will use 
one of the above flags though I haven't thought of the exact name for it 
yet. I will update the JEP with the details of the flag shortly.


Has there been any thoughts about JMX notifications?

Notifications will not be supported in this JEP.

 * MBean Notifications are not a widely used feature and will not be
   supported via the REST adapter.



I know it is outside the scope of the JEP, but I think we should take 
it into consideration when doing the design, so the functionality 
could be added on later without too much difficulty.
Notifications can be added without modifying the current design too 
much. If required, it will be worked upon via an enhancement request.


Thanks
Erik


Thanks
Harsha


Hi Martin,

In my opinion, the interfaces exposed by current JEP are lot closer 
to REST style than the interfaces exposed by Jolokia.


For instance, HTTP GET by default should be used to read resources, 
but it is made part of URL in Jolokia interfaces.


/read///

I would wait on opinions from more people before considering changing 
the current interfaces.


Thanks
-Harsha

On Wednesday 06 September 2017 11:40 AM, Martin Skarsaune wrote:

Hello

Would one at least consider adopting the same URL paths and payloads 
as Jolokia? This could make life a lot easier for third party tools 
that connect to it.


Best Regards

Martin Skarsaune

ons. 6. sep. 2017 kl. 07:04 skrev Harsha Wardhana B 
<harsha.wardhan...@oracle.com <mailto:harsha.wardhan...@oracle.com>>:


Hi Kirk,

Yes. Jolokia was considered and is listed as an alternative in
the JEP.

  * Jolokia can serve as a viable alternative but can be bulky.
We are looking for simple and lightweight solution.


-Harsha

On Wednesday 06 September 2017 10:21 AM, Kirk Pepperdine wrote:

Hi,

Have you run into this project?https://jolokia.org. Unfortunately it’s not 
exactly a drop in replacement for the standard RMI based JMX connector but it’s 
not far off.

Kind regards,
Kirk


On Sep 5, 2017, at 6:30 PM, Erik Gahlin<erik.gah...@oracle.com> 
<mailto:erik.gah...@oracle.com>  wrote:

Hi Harsha,

Looping in jmx-dev.


byte[], short[], int[], float[], double[]

Should long[] be included there as well?


The REST adapter will come with a simple and lightweight JSON parser.

Is this an internal parser or will it be exposed as an API?

If so, how does it relate to JEP 198: Light-Weight JSON API?
http://openjdk.java.net/jeps/198

Will com.sun.net.httpserver.HttpServer be used to serve the requests?

Thanks
Erik


Hi All,

Please review the JEP for REST APIs for JMX :
https://bugs.openjdk.java.net/browse/JDK-8171311

The JEP aims at providing RESTful web interfaces to MBeans.

Access to MBeans registered in a MBeanServer running inside a JVM requires 
a Java client. Language-agnostic access to MBeans will require spawning a Java 
client which can be cumbersome. The proposed JEP allows MBeans to be accessed 
in a language/platform-independent, ubiquitous and seamless manner.

Thanks
-Harsha

Re: jmx-dev JEP review : JDK-8171311 - REST APIs for JMX

2017-09-07 Thread Harsha Wardhana B


Hi Martin,

I will take a second look at POST payloads.

Thanks

Harsha


On Thursday 07 September 2017 02:16 AM, Martin Skarsaune wrote:
How about POST payloads? Since they are so similar already, it would 
be a huge advantage if they could be made interchangeable.


For instance I have done some work on hawt.io <http://hawt.io> 
plugins. A typical scenario for a plugin is to set up one or more 
chained requests for read/write/exec on an MBean, and have a callback 
process the response. If the payloads were compatible, the road to 
support this new backend might not be too difficult. That would give 
this new initiative a real boost over traditional JMX connections.
Otherwise one would have to impose an additional layer of complexity 
to bridge the difference.


I suppose you know these tools very well already. There is by the way 
a presentation on JMX, Jolokia and hawt.io <http://hawt.io> at 
JavaZone in a weeks time, and the recording is usually made available 
just a few hours later: 
https://2017.javazone.no/program/bbe08ad550174e16abd954733e018590


Best regards

Martin

ons. 6. sep. 2017 kl. 08:47 skrev Harsha Wardhana B 
<harsha.wardhan...@oracle.com <mailto:harsha.wardhan...@oracle.com>>:


Hi Martin,

In my opinion, the interfaces exposed by current JEP are lot
closer to REST style than the interfaces exposed by Jolokia.

For instance, HTTP GET by default should be used to read
resources, but it is made part of URL in Jolokia interfaces.

/read///


I would wait on opinions from more people before considering
changing the current interfaces.

Thanks

-Harsha


On Wednesday 06 September 2017 11:40 AM, Martin Skarsaune wrote:

Hello

Would one at least consider adopting the same URL paths and
payloads as Jolokia? This could make life a lot easier for third
party tools that connect to it.

Best Regards

Martin Skarsaune

ons. 6. sep. 2017 kl. 07:04 skrev Harsha Wardhana B
<harsha.wardhan...@oracle.com <mailto:harsha.wardhan...@oracle.com>>:

Hi Kirk,

Yes. Jolokia was considered and is listed as an alternative
in the JEP.

  * Jolokia can serve as a viable alternative but can be
bulky. We are looking for simple and lightweight solution.


-Harsha

On Wednesday 06 September 2017 10:21 AM, Kirk Pepperdine wrote:

Hi,

Have you run into this project?https://jolokia.org. Unfortunately it’s 
not exactly a drop in replacement for the standard RMI based JMX connector but 
it’s not far off.

Kind regards,
Kirk


On Sep 5, 2017, at 6:30 PM, Erik Gahlin<erik.gah...@oracle.com> 
<mailto:erik.gah...@oracle.com>  wrote:

Hi Harsha,

Looping in jmx-dev.


byte[], short[], int[], float[], double[]

Should long[] be included there as well?


The REST adapter will come with a simple and lightweight JSON parser.

Is this an internal parser or will it be exposed as an API?

If so, how does it relate to JEP 198: Light-Weight JSON API?
http://openjdk.java.net/jeps/198

Will com.sun.net.httpserver.HttpServer be used to serve the requests?

Thanks
Erik


Hi All,

Please review the JEP for REST APIs for JMX :
https://bugs.openjdk.java.net/browse/JDK-8171311

The JEP aims at providing RESTful web interfaces to MBeans.

Access to MBeans registered in a MBeanServer running inside a JVM 
requires a Java client. Language-agnostic access to MBeans will require 
spawning a Java client which can be cumbersome. The proposed JEP allows MBeans 
to be accessed in a language/platform-independent, ubiquitous and seamless 
manner.

Thanks
-Harsha

Re: jmx-dev JEP review : JDK-8171311 - REST APIs for JMX

2017-09-06 Thread Harsha Wardhana B


Hi Martin,

In my opinion, the interfaces exposed by current JEP are lot closer to 
REST style than the interfaces exposed by Jolokia.


For instance, HTTP GET by default should be used to read resources, but 
it is made part of URL in Jolokia interfaces.


/read///


I would wait on opinions from more people before considering changing 
the current interfaces.


Thanks
-Harsha

On Wednesday 06 September 2017 11:40 AM, Martin Skarsaune wrote:

Hello

Would one at least consider adopting the same URL paths and payloads 
as Jolokia? This could make life a lot easier for third party tools 
that connect to it.


Best Regards

Martin Skarsaune

ons. 6. sep. 2017 kl. 07:04 skrev Harsha Wardhana B 
<harsha.wardhan...@oracle.com <mailto:harsha.wardhan...@oracle.com>>:


Hi Kirk,

Yes. Jolokia was considered and is listed as an alternative in the
JEP.

  * Jolokia can serve as a viable alternative but can be bulky. We
are looking for simple and lightweight solution.


-Harsha

On Wednesday 06 September 2017 10:21 AM, Kirk Pepperdine wrote:

Hi,

Have you run into this project?https://jolokia.org. Unfortunately it’s not 
exactly a drop in replacement for the standard RMI based JMX connector but it’s 
not far off.

Kind regards,
Kirk


On Sep 5, 2017, at 6:30 PM, Erik Gahlin<erik.gah...@oracle.com> 
<mailto:erik.gah...@oracle.com>  wrote:

Hi Harsha,

Looping in jmx-dev.


byte[], short[], int[], float[], double[]

Should long[] be included there as well?


The REST adapter will come with a simple and lightweight JSON parser.

Is this an internal parser or will it be exposed as an API?

If so, how does it relate to JEP 198: Light-Weight JSON API?
http://openjdk.java.net/jeps/198

Will com.sun.net.httpserver.HttpServer be used to serve the requests?

Thanks
Erik


Hi All,

Please review the JEP for REST APIs for JMX :
https://bugs.openjdk.java.net/browse/JDK-8171311

The JEP aims at providing RESTful web interfaces to MBeans.

Access to MBeans registered in a MBeanServer running inside a JVM requires 
a Java client. Language-agnostic access to MBeans will require spawning a Java 
client which can be cumbersome. The proposed JEP allows MBeans to be accessed 
in a language/platform-independent, ubiquitous and seamless manner.

Thanks
-Harsha

Re: JEP review : JDK-8171311 - REST APIs for JMX

2017-09-05 Thread Harsha Wardhana B


Hi Kirk,

Yes. Jolokia was considered and is listed as an alternative in the JEP.

 * Jolokia can serve as a viable alternative but can be bulky. We are
   looking for simple and lightweight solution.


-Harsha

On Wednesday 06 September 2017 10:21 AM, Kirk Pepperdine wrote:

Hi,

Have you run into this project? https://jolokia.org. Unfortunately it’s not 
exactly a drop in replacement for the standard RMI based JMX connector but it’s 
not far off.

Kind regards,
Kirk


On Sep 5, 2017, at 6:30 PM, Erik Gahlin  wrote:

Hi Harsha,

Looping in jmx-dev.


byte[], short[], int[], float[], double[]

Should long[] be included there as well?


The REST adapter will come with a simple and lightweight JSON parser.

Is this an internal parser or will it be exposed as an API?

If so, how does it relate to JEP 198: Light-Weight JSON API?
http://openjdk.java.net/jeps/198

Will com.sun.net.httpserver.HttpServer be used to serve the requests?

Thanks
Erik


Hi All,

Please review the JEP for REST APIs for JMX :
https://bugs.openjdk.java.net/browse/JDK-8171311

The JEP aims at providing RESTful web interfaces to MBeans.

Access to MBeans registered in a MBeanServer running inside a JVM requires a 
Java client. Language-agnostic access to MBeans will require spawning a Java 
client which can be cumbersome. The proposed JEP allows MBeans to be accessed 
in a language/platform-independent, ubiquitous and seamless manner.

Thanks
-Harsha

Re: jmx-dev JEP review : JDK-8171311 - REST APIs for JMX

2017-09-05 Thread Harsha Wardhana B

Hi Erik,

On Tuesday 05 September 2017 10:00 PM, Erik Gahlin wrote:

Hi Harsha,

Looping in jmx-dev.

> byte[], short[], int[], float[], double[]

Should long[] be included there as well?

Yes. Thanks.

> The REST adapter will come with a simple and lightweight JSON parser.

Is this an internal parser or will it be exposed as an API?

It is an internal parser.

If so, how does it relate to JEP 198: Light-Weight JSON API?
http://openjdk.java.net/jeps/198

It is written from scratch using JavaCC and is not related to above JEP.

Will com.sun.net.httpserver.HttpServer be used to serve the requests?

Yes.

Thanks
Erik

Hi All,

Please review the JEP for REST APIs for JMX :
https://bugs.openjdk.java.net/browse/JDK-8171311

The JEP aims at providing RESTful web interfaces to MBeans.

Access to MBeans registered in a MBeanServer running inside a JVM 
requires a Java client. Language-agnostic access to MBeans will 
require spawning a Java client which can be cumbersome. The proposed 
JEP allows MBeans to be accessed in a language/platform-independent, 
ubiquitous and seamless manner.

Thanks
-Harsha

- Harsha

JEP review : JDK-8171311 - REST APIs for JMX

2017-09-04 Thread Harsha Wardhana B


Hi All,

Please review the JEP for REST APIs for JMX :
https://bugs.openjdk.java.net/browse/JDK-8171311

The JEP aims at providing RESTful web interfaces to MBeans.

Access to MBeans registered in a MBeanServer running inside a JVM 
requires a Java client. Language-agnostic access to MBeans will require 
spawning a Java client which can be cumbersome. The proposed JEP allows 
MBeans to be accessed in a language/platform-independent, ubiquitous and 
seamless manner.


Thanks
-Harsha

Re: RFR : JDK-8186224 javax/management/remote/mandatory/subjectDelegation/* fail with java.security.AccessControlException

2017-08-24 Thread Harsha Wardhana B


Hi Ujwal,

The changes look good.

Thanks

Harsha


On Tuesday 22 August 2017 02:11 PM, Ujwal Vangapally wrote:

Kindly review the changes made.

Before these changes tests fail on windows if we execute them from any 
Drive other than C.


grant codebase "file:/-" {}   is granting permissions only to code 
present in C Drive on windows.


Hence changing'grant codebase "file:/-" {} ' to  grant {} will 
solve the issue by giving permissions to all drives.



https://bugs.openjdk.java.net/browse/JDK-8186224

webrev: 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8186224/webrev.00/



Thanks,

Ujwal.

Re: RFR : 8182485 - JMX connections should have configurable ObjectInputFilter


Thanks for the review Daniel,Roger.

-Harsha


On Tuesday 20 June 2017 11:30 PM, Roger Riggs wrote:

+1

Roger


On 6/20/2017 1:24 PM, Daniel Fuchs wrote:

The fix looks good to me Harsha.

best regards,

-- daniel

On 20/06/2017 07:10, Harsha Wardhana B wrote:

Hi,

Please review the below RFE,

JDK-8182485 : JMX connections should have configurable 
ObjectInputFilter


having webrev at

http://cr.openjdk.java.net/~hb/8182485/webrev.00/

The enhancement adds ObjectInputFilter to JMX connections to 
configure filters during deserialization.


-Harsha

Re: RFR: JDK-8173180 VirtualMachine.startLocalManagementAgent() returns URI with unreliable IP address


Hi Daniel,


On Tuesday 20 June 2017 06:39 PM, Daniel Fuchs wrote:

On 20/06/2017 12:42, Ujwal Vangapally wrote:

Thanks for the Review Daniel, Harsha.

Yes with this fix JConsole running on JDK 8 won't be able to connect 
to a process running on higher version of Java containing this change.


I think even Harsha is agreeing this, his point is that the use case 
where a client running on JDK 8 trying to connect to a process 
running on latest releases is rare. As mostly for Local Management 
both client and server will be running on same machine using same JDK 
for starting both client and server .


please correct me if this is not the case.


Well - I believe it could be quite common to use VisualVM running
on JDK 8 to connect to JDK 9 processes. JMX is all about
interoperability so I'd be very reluctant to break this 'by
default'.

Agreed.



can we have this fix with interoperability issue between versions ?


Maybe you could make the fix conditional to a system property being
defined? Also are you sure what you are proposing is the correct fix
for this issue?
It's more of a workaround than a fix as we don't see any other way to 
fix this issue.


Note that - if I'm not mistaken - you can work around the original issue
by specifying -Djava.rmi.server.hostname= on the command line.
http://docs.oracle.com/javase/8/docs/technotes/guides/rmi/javarmiproperties.html 


This will instruct RMI to put the specified  inside its stubs.
The inconvenience is that this is global (for all RMI stubs, just not
only those used by JMX) - but would that be an issue for the use case
mentioned in the bug?
We had the same consideration that setting the RMI property will change 
the Interface address for all the stubs which may not be an acceptable 
fix in this case.
One possible fix (enhancement) would be to define a JMX system property 
that will control the Interface address that goes into RMI stubs. If 
that seems like too much work, we can close this issue as 'won't fix' as 
we don't have any acceptable fix.


best regards,

-- daniel


-Harsha




Thanks,

Ujwal.



On 6/20/2017 2:40 PM, Daniel Fuchs wrote:

Hi Harsha,

Maybe I'm missing something.
How is the local agent started?

If it's started when you connect jconsole to a process
by specifying the process ID - then I suspect this will
prevent e.g. jconsole or jvisualvm running on JDK 8 to
connect to your process.

Can you verify that it's not the case?

best regards,

-- daniel

On 20/06/2017 09:11, Harsha Wardhana B wrote:

Hi Daniel,

The fix is applicable only to local JMX agent and the most common 
use case would be client and server running from same JVM.
It is highly unlikely that local JMX agent will be started to cater 
for out-of-jvm clients.


I don't see how introducing this fix can cause new interoperability 
problems. Can you please elaborate?


-Harsha

On Monday 19 June 2017 10:23 PM, Daniel Fuchs wrote:

Hi,

If I'm not mistaken then this will make it impossible
for earlier release to interoperate with newer releases
as the LocalRMIClientSocketFactory class will not be
present the client tries to deserialize the stub.

best regards,

-- daniel

On 19/06/2017 11:52, Ujwal Vangapally wrote:

Hi,

Kindly review the fix for bug below

https://bugs.openjdk.java.net/browse/JDK-8173180

webrev: 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8173180/webrev.00/ 



Thanks,

Ujwal

Re: RFR: JDK-8173180 VirtualMachine.startLocalManagementAgent() returns URI with unreliable IP address


Hi Daniel,

The fix is applicable only to local JMX agent and the most common use 
case would be client and server running from same JVM.
It is highly unlikely that local JMX agent will be started to cater for 
out-of-jvm clients.


I don't see how introducing this fix can cause new interoperability 
problems. Can you please elaborate?


-Harsha

On Monday 19 June 2017 10:23 PM, Daniel Fuchs wrote:

Hi,

If I'm not mistaken then this will make it impossible
for earlier release to interoperate with newer releases
as the LocalRMIClientSocketFactory class will not be
present the client tries to deserialize the stub.

best regards,

-- daniel

On 19/06/2017 11:52, Ujwal Vangapally wrote:

Hi,

Kindly review the fix for bug below

https://bugs.openjdk.java.net/browse/JDK-8173180

webrev: 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8173180/webrev.00/


Thanks,

Ujwal

RFR : 8182485 - JMX connections should have configurable ObjectInputFilter


Hi,

Please review the below RFE,

JDK-8182485 : JMX connections should have configurable ObjectInputFilter

having webrev at

http://cr.openjdk.java.net/~hb/8182485/webrev.00/

The enhancement adds ObjectInputFilter to JMX connections to configure 
filters during deserialization.


-Harsha

Re: RFR: JDK-8178508 Co-locate remaining MM tests

2017-06-04 Thread Harsha Wardhana B


Hi Ujwal,

Looks fine.

-Harsha


On Thursday 01 June 2017 04:55 PM, Ujwal Vangapally wrote:


Gentle reminder.

Thanks,

Ujwal.


On 5/31/2017 10:32 AM, Ujwal Vangapally wrote:


Kindly review the changes made for below bug

converted tonga test to JTREG test

added an additional assert statement for verifying 
setusageThreshold() operation is successful


https://bugs.openjdk.java.net/browse/JDK-8178508

tonga test case is currently at this path :

http://sqe-hgi.us.oracle.com/hg/index.cgi/testbase/javase/functional/9/vm/file/tip/src/nsk/regression/b6653214/  



webrev :
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8178508/webrev.00/

Thanks,

Ujwal.

Re: RFR: JDK-6515161 If remote removeNotificationListener gets SecurityException, client no longer gets notifications

2017-05-08 Thread Harsha Wardhana B


Hi Daniel,

The term 'listenerid' is used in conjunction with method name/object 
field which adds context about the term 'listenerid'. Having a 
standalone method name as getClientListenerId is less ambiguous compared 
to method name : getListenerId.


I don't really have a strong opinion on this and am okay with either names.

-Harsha


On Monday 08 May 2017 03:16 PM, Daniel Fuchs wrote:

On 08/05/2017 09:45, Ujwal Vangapally wrote:

Thanks for the Review Daniel, Harsha

Please find the new webrev incorporating the review comments

webrev :
http://cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.01/


Looks good. In retrospect I wonder if renaming
getListenerIds/getListenerId into 
getClientListenerIds/getClientListenerId

was such a good idea given that 'listenerId' is an established
terminology in the specification [1] [2] (and 'listenerId' is used all
over the place in ClientNotifForwarder anyway).

Harsha, what do you think?

best regards,

-- daniel

[1] See @return in 
http://download.java.net/java/jdk9/docs/api/javax/management/remote/rmi/RMIConnection.html#addNotificationListeners-javax.management.ObjectName:A-java.rmi.MarshalledObject:A-javax.security.auth.Subject:A-

[2] see @param listenerIds in
http://download.java.net/java/jdk9/docs/api/javax/management/remote/rmi/RMIConnection.html#removeNotificationListeners-javax.management.ObjectName-java.lang.Integer:A-javax.security.auth.Subject- 






Thanks,

Ujwal


On 5/8/2017 12:03 PM, Daniel Fuchs wrote:

Hi Ujwal,

For consistency I think the new getListenerIds method should:

a) either return an array of Integer, even if it contains only 1
Integer:

  1. The name of the method implies that an array is returned
  2. You will need the array when you call
 connection.removeNotificationListeners anyway.

or b) the other possibility is to remove the 's' at the end of the
method.

Both would be acceptable.

best regards,

-- daniel

On 04/05/17 07:59, Ujwal Vangapally wrote:

corrected webrev link :
http://cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.00/


On 5/4/2017 12:14 PM, Ujwal Vangapally wrote:


Kindly review the changes made for below bug

Problem description and solution are explained in comments section

https://bugs.openjdk.java.net/browse/JDK-6515161

diff for*ClientNotifForwarder.java *might be a bit confusing as it
shows the method name

removeNotificationListener is modified to getListenerIds and new
method removeNotificationListener is introduced.

Actual change is new method getListenerIds is introduced and it is
called in removeNotificationListener method without

affecting the functionality of removeNotificationListener.

webrev :
cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.00/

Thanks,

Ujwal,

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
<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: RFR: JDK-6515161 If remote removeNotificationListener gets SecurityException, client no longer gets notifications

2017-05-07 Thread Harsha Wardhana B


Hi Ujwal,

The fix looks fine.

Some nits.

In ClientNotifForwarder.java, "getListenerIds" could be renamed to 
getClientListenrIds.


-Harsha


On Thursday 04 May 2017 12:29 PM, Ujwal Vangapally wrote:


corrected webrev link : 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.00/



On 5/4/2017 12:14 PM, Ujwal Vangapally wrote:


Kindly review the changes made for below bug

Problem description and solution are explained in comments section

https://bugs.openjdk.java.net/browse/JDK-6515161

diff for*ClientNotifForwarder.java *might be a bit confusing as it 
shows the method name


removeNotificationListener is modified to getListenerIds and new 
method removeNotificationListener is introduced.


Actual change is new method getListenerIds is introduced and it is 
called in removeNotificationListener method without


affecting the functionality of removeNotificationListener.

webrev : cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.00/

Thanks,

Ujwal,

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

2017-04-27 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
<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 
<serviceability-dev-boun...@openjdk.java.net> on behalf of Harsha 
Wardhana B <harsha.wardhan...@oracle.com>

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

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

2017-04-23 Thread Harsha Wardhana B


Hi All,

Please review this enhancement to replace plain-text password for JMX 
agent with SHA-256 hash.


Issue: https://bugs.openjdk.java.net/browse/JDK-5016517


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

Overview of implementation:

Currently, the JMX agent password file used to authenticate user, stores 
user name and password as clear text. Though system level restrictions 
are recommended for jmx password file, passwords are vulnerable since 
they are stored in clear. The current RFE proposes to store passwords as 
SHA256 hash instead of clear text.


In current implementation, if password file is writable, and if 
passwords are in clear, they will be replaced by SHA256 hash upon agent 
boot-up or when login attempt is made.


The file, 
src/jdk.management.agent/share/conf/jmxremote.password.template contains 
more details about the implementation.


- Harsha

Re: RFR - [RFE] JDK-8007141 : Introduce Dynamic MBean exposing the perf counters

2017-03-07 Thread Harsha Wardhana B


Hi Daniel,


On Tuesday 07 March 2017 08:40 PM, Daniel Fuchs wrote:

Hi Harsha,

Not a review either (at least not a complete one).

PlatformMBeanProviderImpl:

 281 @Override
 282 public Set<Class> 
mbeanInterfaces() {

 283 return Collections.emptySet();
 284 }

For future maintainers, I think it would be good to copy
the comment at line 250 and insert it before line 283 too:
 250 // DynamicMBean cannot be used to find an 
MBean by ManagementFactory



Will do.

HotSpotPerfCounterMBean:

Exception handling: I have the feeling that this part might
be a bit over-engineered. If you look at javax.management.StandardMBean
(which is a canonical implementation of DynamicMBean) and
then transitively at MBeanSupport and PerInterface where this class
eventually delegates to, you will see that:

1. You can throw NPE in setAttribute when attribute == null (no need for
   all the wrapping)
If attribute == null, isn't it better to throw IllegalArgumentException 
instead of NPE? I'll remove the wrapping.

2. You should throw AttributeNotFoundException when attempting to read
   a write-only attribute or write a read-only attribute
Isn't UnSupportedOperationEx better than AttributeNotFoundException? 
Have we committed to above in the specs?

3. You can throw NPE if AttributeList is null
If attribute == null, isn't it better to throw IllegalArgumentException 
instead of NPE? I'll remove the wrapping.


 179 String typeName = "java.lang.String";

That looks like a hack.
If attribute is null, defaulting to String would be convenient as 
getAttribute can return empty string instead of null. Do you think 
otherwise?


What if at some point the value of that attribute becomes non null,
and its class is *not* java.lang.String?
Then MBeanAttributeInfo would be lying.
If that ever happens - then it would be more consistent to coerce
the value to its declared type (String) before sending it as a
a result for getAttribute.
Since perf-counters are key-value pairs written into shared memory, it 
is reasonable to expect new counters to be added or type of existing 
counters to be updated while VM is running. I have to refresh the 
counters periodically (maybe once every 5 mins) since I will not be 
notified of any change. If any attributes are added or if existing 
attribute types are changed, then AttributeChangeNotification will be 
sent. Clients will have to refresh MBeanInfo upon receiving this 
notification. So yeah, there is a brief window where MBeanInfo will be 
lying. I don't see easy way to fix it unlesss perfcounters can notify 
MBean of any mutations. Maybe the limitation can be documented.


I will make these changes in next webrev.


Or better, find a way to figure out what should be the class name
of the value even if the value is null (isn't there some metadata
available for these perf counters somewhere?).

PerfCounterMBeanTest:

testGetAttribute should verify that class of the returned value
corresponds to what was declared in the MBeanAttributeInfo for the
corresponding attribute.

Sure. Will do.


cheers,

-- daniel

-Harsha


On 26/02/17 16:50, Harsha Wardhana B wrote:

Hi All,

Please review the below RFE,

JDK-8007141 <http://JDK-8007141> : Introduce Dynamic MBean exposing the
perf counters.

with webrev at,

http://cr.openjdk.java.net/~hb/8007141/webrev.00/

Appreciate if I can get inputs for below.

1. Location of HotSpotPerfCounterMBean. It is located at
src/jdk.management/share/classes/com/sun/management/internal. It can be
moved to src/jdk.management/share/classes/com/sun/management if 
required.


2. Javadoc comments for HotSpotPerfCounterMBean. Not sure if it is 
adequate.


3. Description for each attribute of MBean. I am using getUnits(),
getVariability(), and getFlags() for description. I am not sure if that
is the right description. Appreciate inputs from someone who knows perf
counters well.

Thanks

Harsha

Re: RFR - [RFE] JDK-8007141 : Introduce Dynamic MBean exposing the perf counters

2017-02-28 Thread Harsha Wardhana B

xFills --> 67
sun.gc.tlab.maxGcWaste --> 36674
sun.gc.tlab.maxSlowAlloc --> 0
sun.gc.tlab.maxSlowWaste --> 5028
sun.gc.tlab.slowAlloc --> 0
sun.gc.tlab.slowWaste --> 5028
sun.os.hrt.frequency --> 10
sun.os.hrt.ticks --> 6279815676
sun.property.sun.boot.library.path --> 
/home/harsha/work/jdk10_PerfCounter/build/linux-x86_64-normal-server-fastdebug/jdk/lib

sun.rt._sync_ContendedLockAttempts --> 0
sun.rt._sync_Deflations --> 3
sun.rt._sync_EmptyNotifications --> 0
sun.rt._sync_FailedSpins --> 0
sun.rt._sync_FutileWakeups --> 0
sun.rt._sync_Inflations --> 8
sun.rt._sync_MonExtant --> 4
sun.rt._sync_MonInCirculation --> 0
sun.rt._sync_MonScavenged --> 0
sun.rt._sync_Notifications --> 2
sun.rt._sync_Parks --> 2
sun.rt._sync_PrivateA --> 0
sun.rt._sync_PrivateB --> 0
sun.rt._sync_SlowEnter --> 0
sun.rt._sync_SlowExit --> 0
sun.rt._sync_SlowNotify --> 0
sun.rt._sync_SlowNotifyAll --> 0
sun.rt._sync_SuccessfulSpins --> 0
sun.rt.applicationTime --> 4337190130
sun.rt.createVmBeginTime --> 1488277242189
sun.rt.createVmEndTime --> 1488277244988
sun.rt.internalVersion --> Java HotSpot(TM) 64-Bit Server VM 
(fastdebug 10-internal+0-adhoc.harsha.jdk10PerfCounter) for linux-amd64 
JRE (10-internal+0-adhoc.harsha.jdk10PerfCounter), built on Feb 25 2017 
22:24:55 by "harsha" with gcc 5.4.0 20160609
sun.rt.javaCommand --> com.sun.javatest.regtest.agent.MainWrapper 
/home/harsha/work/jdk10_PerfCounter/build/linux-x86_64-normal-server-fastdebug/nb-jtreg/work/java/lang/management/PerfCounterMBeanTest.d/testng.0.jta 
java/lang/management/PerfCounterMBeanTest.java PerfCounterMBeanTest
sun.rt.jvmCapabilities --> 
1100

sun.rt.jvmVersion --> 167772160
sun.rt.safepointSyncTime --> 35150781
sun.rt.safepointTime --> 44606495
sun.rt.safepoints --> 9
sun.rt.vmInitDoneTime --> 1488277242348
sun.threads.vmOperationTime --> 8730462
sun.urlClassLoader.readClassBytesTime --> 0
sun.zip.zipFile.openTime --> 32909964
sun.zip.zipFiles --> 3


On Tuesday 28 February 2017 12:36 PM, David Holmes wrote:

Hi Harsha,

Not a review :)

On 27/02/2017 2:50 AM, Harsha Wardhana B wrote:

Hi All,

Please review the below RFE,

JDK-8007141 <http://JDK-8007141> : Introduce Dynamic MBean exposing the
perf counters.


What perf counters are these and where/how are they currently documented?

They may be affected by the UsePerfData VM flag (which some/most of 
the management code seems to be ignorant of).


Thanks,
David
-


with webrev at,

http://cr.openjdk.java.net/~hb/8007141/webrev.00/

Appreciate if I can get inputs for below.

1. Location of HotSpotPerfCounterMBean. It is located at
src/jdk.management/share/classes/com/sun/management/internal. It can be
moved to src/jdk.management/share/classes/com/sun/management if 
required.


2. Javadoc comments for HotSpotPerfCounterMBean. Not sure if it is 
adequate.


3. Description for each attribute of MBean. I am using getUnits(),
getVariability(), and getFlags() for description. I am not sure if that
is the right description. Appreciate inputs from someone who knows perf
counters well.

Thanks

Harsha

Re: RFR - [RFE] JDK-8007141 : Introduce Dynamic MBean exposing the perf counters

2017-02-27 Thread Harsha Wardhana B

Perfcounter are dynamic and can change. Applications must be aware of 
that and work around it.


Perf counters being dynamic must be documented and must not be a 
deterrent for consuming them (via Dymanic MBean).


Regards

Harsha


On Monday 27 February 2017 11:22 PM, Erik Gahlin wrote:
If somebody writes a tool that plots the values that the perf counters 
provide, which is bound to happen if it is exposed via JMX, but then 
the internals of the JVM is changed and the perf counter value is 
removed or it is incorrect.


Erik


Hi Erik,

I am not sure if Perf Counters could be disabled. If yes, then 
PerfCounter MBean must throw appropriate exception while getting 
MBeanInfo.


I don't know about tests for PerfCounter. The webrev contains tests 
for PerfCounter MBean.


I am not sure how we will be breaking other's code. Could you please 
elaborate?


Thanks
Harsha

On Monday 27 February 2017 07:39 PM, Erik Gahlin wrote:

Hi Harsha,

How will perf counters be supported?

Can they be removed at anytime, or only at major releases etc. Do we 
have tests for them? I think it would be good know, and perhaps 
state somewhere so we don't break people's code.


Erik


Hi All,

Please review the below RFE,

JDK-8007141  : Introduce Dynamic MBean exposing 
the perf counters.


with webrev at,

http://cr.openjdk.java.net/~hb/8007141/webrev.00/

Appreciate if I can get inputs for below.

1. Location of HotSpotPerfCounterMBean. It is located at 
src/jdk.management/share/classes/com/sun/management/internal. It 
can be moved to src/jdk.management/share/classes/com/sun/management 
if required.


2. Javadoc comments for HotSpotPerfCounterMBean. Not sure if it is 
adequate.


3. Description for each attribute of MBean. I am using getUnits(), 
getVariability(), and getFlags() for description. I am not sure if 
that is the right description. Appreciate inputs from someone who 
knows perf counters well.


Thanks

Harsha

Re: RFR - [RFE] JDK-8007141 : Introduce Dynamic MBean exposing the perf counters

2017-02-27 Thread Harsha Wardhana B


Hi Erik,

I am not sure if Perf Counters could be disabled. If yes, then 
PerfCounter MBean must throw appropriate exception while getting MBeanInfo.


I don't know about tests for PerfCounter. The webrev contains tests for 
PerfCounter MBean.


I am not sure how we will be breaking other's code. Could you please 
elaborate?


Thanks
Harsha

On Monday 27 February 2017 07:39 PM, Erik Gahlin wrote:

Hi Harsha,

How will perf counters be supported?

Can they be removed at anytime, or only at major releases etc. Do we 
have tests for them? I think it would be good know, and perhaps state 
somewhere so we don't break people's code.


Erik


Hi All,

Please review the below RFE,

JDK-8007141  : Introduce Dynamic MBean exposing 
the perf counters.


with webrev at,

http://cr.openjdk.java.net/~hb/8007141/webrev.00/

Appreciate if I can get inputs for below.

1. Location of HotSpotPerfCounterMBean. It is located at 
src/jdk.management/share/classes/com/sun/management/internal. It can 
be moved to src/jdk.management/share/classes/com/sun/management if 
required.


2. Javadoc comments for HotSpotPerfCounterMBean. Not sure if it is 
adequate.


3. Description for each attribute of MBean. I am using getUnits(), 
getVariability(), and getFlags() for description. I am not sure if 
that is the right description. Appreciate inputs from someone who 
knows perf counters well.


Thanks

Harsha

RFR - [RFE] JDK-8007141 : Introduce Dynamic MBean exposing the perf counters

2017-02-26 Thread Harsha Wardhana B


Hi All,

Please review the below RFE,

JDK-8007141  : Introduce Dynamic MBean exposing the 
perf counters.


with webrev at,

http://cr.openjdk.java.net/~hb/8007141/webrev.00/

Appreciate if I can get inputs for below.

1. Location of HotSpotPerfCounterMBean. It is located at 
src/jdk.management/share/classes/com/sun/management/internal. It can be 
moved to src/jdk.management/share/classes/com/sun/management if required.


2. Javadoc comments for HotSpotPerfCounterMBean. Not sure if it is adequate.

3. Description for each attribute of MBean. I am using getUnits(), 
getVariability(), and getFlags() for description. I am not sure if that 
is the right description. Appreciate inputs from someone who knows perf 
counters well.


Thanks

Harsha

Re: RFR : JDK-7132577 - javax/management/monitor/MultiMonitorTest.java fails in JDK8-B22

2017-02-23 Thread Harsha Wardhana B


Looks good.

-Harsha


On Thursday 23 February 2017 12:28 PM, Amit Sapre wrote:

Hello,

http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-7132577/webrev.02/ has the 
updated changes.

I ran this test on my VM and roughly takes 250-300 ms to get all the listener 
count.

Thanks,
Amit


-Original Message-
From: Amit Sapre
Sent: Thursday, February 23, 2017 11:55 AM
To: David Holmes; serviceability-dev@openjdk.java.net; Harsha Wardhana
B
Subject: RE: RFR : JDK-7132577 -
javax/management/monitor/MultiMonitorTest.java fails in JDK8-B22

Hello,

In a messy run of this test case,
the number of prints for the counter values will be 240 (120 seconds is
jtreg timeout)

Will try to optimize this. Please hold on for a new webrev.

Thanks,
Amit



-Original Message-
From: Amit Sapre
Sent: Thursday, February 23, 2017 11:48 AM
To: David Holmes; serviceability-dev@openjdk.java.net; Harsha

Wardhana

B
Subject: RE: RFR : JDK-7132577 -
javax/management/monitor/MultiMonitorTest.java fails in JDK8-B22

Hello,

Thanks David & Harsha for your inputs.
Here is the new webrev :
http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-7132577/webrev.01/

Thanks,
Amit


-Original Message-
From: David Holmes
Sent: Thursday, February 23, 2017 9:24 AM
To: serviceability-dev@openjdk.java.net; Sapre Amit
Cc: Harsha Wardhana B
Subject: Re: RFR : JDK-7132577 -
javax/management/monitor/MultiMonitorTest.java fails in JDK8-B22

Hi Amit,

On 23/02/2017 12:18 AM, Harsha Wardhana B wrote:

Hi Amit,

There is no need to wait in a loop to check we have not received

any

notifications. Without starting the monitors, the listener count
will be zero.

The first part of diff L116-L121 could be left as is.

Agreed.


By relying on Jtreg timeout for receiving notifications, we will
be unable to print the number of listeners emitted by each

Monitor.

But

I

guess there is no way to intercept a Jtreg timeout and print out

those

values.

You could print out the values every 1 second, or 5 or 10, ... at
least that way we can see what the values are when we timeout, and
also if they have been changing.

Thanks,
David


Regards

Harsha


On Wednesday 22 February 2017 03:29 PM, Amit Sapre wrote:

Hello,



Please review this test bug fix which eliminates test case's own
timeout mechanism to default jtreg timeout.



Bug ID : https://bugs.openjdk.java.net/browse/JDK-7132577

Webrev :
http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-

7132577/webrev.00/

<http://cr.openjdk.java.net/%7Easapre/webrev/2017/JDK-

7132577/webrev.

00/>



Thanks,

Amit

Re: RFR: JDK-8173130 - SubjectDelegation2Test.java and SubjectDelegation3Test.java failing on solaris


Thanks for the review Daniel.

-Harsha


On Wednesday 22 February 2017 08:21 PM, Daniel Fuchs wrote:

Hi Harsha,

OK - makes sense to me.
Maybe just wait for tomorrow to see if you get any other comments
form other reviewers...

best regards,

-- daniel

On 22/02/17 14:38, Harsha Wardhana B wrote:

Hi Daniel,

("java.lang.RuntimePermission" "accessClassInPackage.sun.security.krb5")
was missing.

I gave all the RuntimePermissions since we don't want test cases to fail
in future.

Granting java.lang.RuntimePermission "*" does not affect the test cases
in anyway as test cases rely on delegation of specific permissions.

Regards
Harsha

On Wednesday 22 February 2017 07:43 PM, Daniel Fuchs wrote:

Hi Harsha,

Looks good - but do you really need java.lang.RuntimePermission "*" ?

What permission was missing?

best regards,

-- daniel

On 22/02/17 13:35, Harsha Wardhana B wrote:

Hi All,

Please review the below test fix for

Issue: JDK-8173130 <http://JDK-8173130> - SubjectDelegation2Test.java
and SubjectDelegation3Test.java failing on solaris

webrev : http://cr.openjdk.java.net/~hb/8173130/webrev.00/

The fix does the following.

1. Removes explicit platform check

2. Fixes JMXServiceURL

3. Adds required RuntimePermissions for test to execute.

Regards

Harsha

Re: RFR: JDK-8173130 - SubjectDelegation2Test.java and SubjectDelegation3Test.java failing on solaris


Hi Daniel,

("java.lang.RuntimePermission" "accessClassInPackage.sun.security.krb5") 
was missing.


I gave all the RuntimePermissions since we don't want test cases to fail 
in future.


Granting java.lang.RuntimePermission "*" does not affect the test cases 
in anyway as test cases rely on delegation of specific permissions.


Regards
Harsha

On Wednesday 22 February 2017 07:43 PM, Daniel Fuchs wrote:

Hi Harsha,

Looks good - but do you really need java.lang.RuntimePermission "*" ?

What permission was missing?

best regards,

-- daniel

On 22/02/17 13:35, Harsha Wardhana B wrote:

Hi All,

Please review the below test fix for

Issue: JDK-8173130 <http://JDK-8173130> - SubjectDelegation2Test.java
and SubjectDelegation3Test.java failing on solaris

webrev : http://cr.openjdk.java.net/~hb/8173130/webrev.00/

The fix does the following.

1. Removes explicit platform check

2. Fixes JMXServiceURL

3. Adds required RuntimePermissions for test to execute.

Regards

Harsha

Re: RFR : JDK-7132577 - javax/management/monitor/MultiMonitorTest.java fails in JDK8-B22


Hi Amit,

There is no need to wait in a loop to check we have not received any 
notifications. Without starting the monitors, the listener count will be 
zero.


The first part of diff L116-L121 could be left as is.

By relying on Jtreg timeout for receiving notifications, we will be 
unable to print the number of listeners emitted by each Monitor. But I 
guess there is no way to intercept a Jtreg timeout and print out those 
values.


Regards

Harsha


On Wednesday 22 February 2017 03:29 PM, Amit Sapre wrote:


Hello,

Please review this test bug fix which eliminates test case’s own 
 timeout mechanism to default jtreg timeout.


Bug ID : https://bugs.openjdk.java.net/browse/JDK-7132577

Webrev : 
http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-7132577/webrev.00/ 



Thanks,

Amit

RFR: JDK-8173130 - SubjectDelegation2Test.java and SubjectDelegation3Test.java failing on solaris