Quantcast

RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
Everybody,

Please review:

http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.10/

These changes introduce new parameter[1] of the socket transport -
allow. Users can explicitly specify a list of hosts that allowed to
connect to jdwp server and it's the second part of JDWP hardening[2].

No restrictions are applied by default now but I'll file a separate CR
to restrict list of allowed peers to localhost by default.

Also these changes implement versioning for jdwp transport and therefor
simplify feature development of jdwp.


1. Example command line:

-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,
address=*,allow="127.0.0.0/8;192.168.0.0/24"

Possible values for allow parameter:
  *           - accept connections from everywhere.
  N.N.N.N     - accept connections from this IP address only
  N.N.N.N/nn  - accept connections from particular ip subnet



2. JDK-8052136 JDWP hardening

-Dmitry

--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Robbin Ehn
Hi Dmitry,

I took a look at this, I have two practical issues:

1:
[rehn@rehn-ws dev]$ java -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:9999,allow=6.6.6.6 -cp runs ForEver
Listening for transport dt_socket at address: 9999
ERROR: transport error 202: peer not allowed to connect: Success
JDWP exit error JVMTI_ERROR_NONE(0): could not connect, timeout or fatal error [transport.c:358]

So connecting with an unallowed client terminates the VM.

2:
[rehn@rehn-ws dev]$ java -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:9999,allow=6.BAD.6.6 -cp runs ForEver
Listening for transport dt_socket at address: 9999
ERROR: transport error 202: unable to parse list of allowed peers: Success
JDWP exit error JVMTI_ERROR_NONE(0): could not connect, timeout or fatal error [transport.c:358]

Starting with an bad allow filter terminates the VM when connecting a client.


Connecting with an unallowed ip/port should not terminate the VM and we should verify allow filter directly at startup.

Thanks

/Robbin

On 02/28/2017 10:41 AM, Dmitry Samersoff wrote:

> Everybody,
>
> Please review:
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.10/
>
> These changes introduce new parameter[1] of the socket transport -
> allow. Users can explicitly specify a list of hosts that allowed to
> connect to jdwp server and it's the second part of JDWP hardening[2].
>
> No restrictions are applied by default now but I'll file a separate CR
> to restrict list of allowed peers to localhost by default.
>
> Also these changes implement versioning for jdwp transport and therefor
> simplify feature development of jdwp.
>
>
> 1. Example command line:
>
> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,
> address=*,allow="127.0.0.0/8;192.168.0.0/24"
>
> Possible values for allow parameter:
>   *           - accept connections from everywhere.
>   N.N.N.N     - accept connections from this IP address only
>   N.N.N.N/nn  - accept connections from particular ip subnet
>
>
>
> 2. JDK-8052136 JDWP hardening
>
> -Dmitry
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
Robbin,

Agree, I'll fix it.

-Dmitry

On 2017-03-10 15:56, Robbin Ehn wrote:

> Hi Dmitry,
>
> I took a look at this, I have two practical issues:
>
> 1:
> [rehn@rehn-ws dev]$ java
> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:9999,allow=6.6.6.6
> -cp runs ForEver
> Listening for transport dt_socket at address: 9999
> ERROR: transport error 202: peer not allowed to connect: Success
> JDWP exit error JVMTI_ERROR_NONE(0): could not connect, timeout or fatal
> error [transport.c:358]
>
> So connecting with an unallowed client terminates the VM.
>
> 2:
> [rehn@rehn-ws dev]$ java
> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:9999,allow=6.BAD.6.6
> -cp runs ForEver
> Listening for transport dt_socket at address: 9999
> ERROR: transport error 202: unable to parse list of allowed peers: Success
> JDWP exit error JVMTI_ERROR_NONE(0): could not connect, timeout or fatal
> error [transport.c:358]
>
> Starting with an bad allow filter terminates the VM when connecting a
> client.
>
>
> Connecting with an unallowed ip/port should not terminate the VM and we
> should verify allow filter directly at startup.
>
> Thanks
>
> /Robbin
>
> On 02/28/2017 10:41 AM, Dmitry Samersoff wrote:
>> Everybody,
>>
>> Please review:
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.10/
>>
>> These changes introduce new parameter[1] of the socket transport -
>> allow. Users can explicitly specify a list of hosts that allowed to
>> connect to jdwp server and it's the second part of JDWP hardening[2].
>>
>> No restrictions are applied by default now but I'll file a separate CR
>> to restrict list of allowed peers to localhost by default.
>>
>> Also these changes implement versioning for jdwp transport and therefor
>> simplify feature development of jdwp.
>>
>>
>> 1. Example command line:
>>
>> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,
>> address=*,allow="127.0.0.0/8;192.168.0.0/24"
>>
>> Possible values for allow parameter:
>>   *           - accept connections from everywhere.
>>   N.N.N.N     - accept connections from this IP address only
>>   N.N.N.N/nn  - accept connections from particular ip subnet
>>
>>
>>
>> 2. JDK-8052136 JDWP hardening
>>
>> -Dmitry
>>


--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
In reply to this post by Robbin Ehn
Robbin,

Please, see:

http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.11

> 1:
> So connecting with an unallowed client terminates the VM.

Fixed.

> 2:
> Starting with an bad allow filter terminates the VM when connecting a
> client.

Moved allowed parameter (and parser call) to StartListening.

-Dmitry


On 2017-03-10 15:56, Robbin Ehn wrote:

> Hi Dmitry,
>
> I took a look at this, I have two practical issues:
>
> 1:
> [rehn@rehn-ws dev]$ java
> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:9999,allow=6.6.6.6
> -cp runs ForEver
> Listening for transport dt_socket at address: 9999
> ERROR: transport error 202: peer not allowed to connect: Success
> JDWP exit error JVMTI_ERROR_NONE(0): could not connect, timeout or fatal
> error [transport.c:358]
>
> So connecting with an unallowed client terminates the VM.
>
> 2:
> [rehn@rehn-ws dev]$ java
> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:9999,allow=6.BAD.6.6
> -cp runs ForEver
> Listening for transport dt_socket at address: 9999
> ERROR: transport error 202: unable to parse list of allowed peers: Success
> JDWP exit error JVMTI_ERROR_NONE(0): could not connect, timeout or fatal
> error [transport.c:358]
>
> Starting with an bad allow filter terminates the VM when connecting a
> client.
>
>
> Connecting with an unallowed ip/port should not terminate the VM and we
> should verify allow filter directly at startup.
>
> Thanks
>
> /Robbin
>
> On 02/28/2017 10:41 AM, Dmitry Samersoff wrote:
>> Everybody,
>>
>> Please review:
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.10/
>>
>> These changes introduce new parameter[1] of the socket transport -
>> allow. Users can explicitly specify a list of hosts that allowed to
>> connect to jdwp server and it's the second part of JDWP hardening[2].
>>
>> No restrictions are applied by default now but I'll file a separate CR
>> to restrict list of allowed peers to localhost by default.
>>
>> Also these changes implement versioning for jdwp transport and therefor
>> simplify feature development of jdwp.
>>
>>
>> 1. Example command line:
>>
>> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,
>> address=*,allow="127.0.0.0/8;192.168.0.0/24"
>>
>> Possible values for allow parameter:
>>   *           - accept connections from everywhere.
>>   N.N.N.N     - accept connections from this IP address only
>>   N.N.N.N/nn  - accept connections from particular ip subnet
>>
>>
>>
>> 2. JDK-8052136 JDWP hardening
>>
>> -Dmitry
>>


--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

serguei.spitsyn@oracle.com
In reply to this post by Dmitry Samersoff
Hi Dmitry,

We already had a big review thread back in 2014 on this.
I think, it is again going in the wrong order.

First, I think, it is better to start from a CCC (or its equivalent, CSR).
This will allow to focus on and sort out the changes in interface/behavior
first before going into the implementation details.

Second, I'd suggest to separate a couple of other things.
I still see the C .vs. C++ related change in the jdwpTransport.h.
It is better to leave it along for now as it was suggested in early
review rounds.
If you still want to fix it then it is better to file a separate bug that
should include the JNI as well (as it was discussed with Alan before).

Also, I'm thinking if it is a good idea to separate the transport
versioning to an RFE.
It would allow to focus on this aspect as necessary.
In this case, the 8061228 will depend on the versioning.
However, it is much more simple and can be resolved faster.


Thanks,
Serguei


On 2/28/17 01:41, Dmitry Samersoff wrote:

> Everybody,
>
> Please review:
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.10/
>
> These changes introduce new parameter[1] of the socket transport -
> allow. Users can explicitly specify a list of hosts that allowed to
> connect to jdwp server and it's the second part of JDWP hardening[2].
>
> No restrictions are applied by default now but I'll file a separate CR
> to restrict list of allowed peers to localhost by default.
>
> Also these changes implement versioning for jdwp transport and therefor
> simplify feature development of jdwp.
>
>
> 1. Example command line:
>
> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,
> address=*,allow="127.0.0.0/8;192.168.0.0/24"
>
> Possible values for allow parameter:
>    *           - accept connections from everywhere.
>    N.N.N.N     - accept connections from this IP address only
>    N.N.N.N/nn  - accept connections from particular ip subnet
>
>
>
> 2. JDK-8052136 JDWP hardening
>
> -Dmitry
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
Serguei,

> I still see the C .vs. C++ related change in the jdwpTransport.h.

done.

http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/

see also inline.

On 2017-03-15 00:40, [hidden email] wrote:
> Hi Dmitry,
>
> We already had a big review thread back in 2014 on this.
> I think, it is again going in the wrong order.
>
> First, I think, it is better to start from a CCC (or its equivalent, CSR).
> This will allow to focus on and sort out the changes in interface/behavior
> first before going into the implementation details.

CCC was filed and approved. The only reason I withdraw it - the fix
didn't go to jdk9 but CCC tool doesn't have jdk10.

CSR process is also not yet implemented.

> Second, I'd suggest to separate a couple of other things.
> I still see the C .vs. C++ related change in the jdwpTransport.h.
> It is better to leave it along for now as it was suggested in early
> review rounds.
> If you still want to fix it then it is better to file a separate bug that
> should include the JNI as well (as it was discussed with Alan before).

Do you mean?

  39 #ifdef __cplusplus
  40 extern "C" {
  41 #endif

I'll add it back to avoid any confusion.

> Also, I'm thinking if it is a good idea to separate the transport
> versioning to an RFE.
> It would allow to focus on this aspect as necessary.
> In this case, the 8061228 will depend on the versioning.
> However, it is much more simple and can be resolved faster.

It's hard to test versioning code without implementation of
new, VERSION_1_1 transport.

Network part of 8061228 is simple and clear separated from versioning,
so I would prefer to keep it together in one CR/one push.

Restriction turned off by default (I'll file separate CR to enable it
later), so we have enough time to fix any issues that might come.

-Dmitry

>
>
> Thanks,
> Serguei
>
>
> On 2/28/17 01:41, Dmitry Samersoff wrote:
>> Everybody,
>>
>> Please review:
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.10/
>>
>> These changes introduce new parameter[1] of the socket transport -
>> allow. Users can explicitly specify a list of hosts that allowed to
>> connect to jdwp server and it's the second part of JDWP hardening[2].
>>
>> No restrictions are applied by default now but I'll file a separate CR
>> to restrict list of allowed peers to localhost by default.
>>
>> Also these changes implement versioning for jdwp transport and therefor
>> simplify feature development of jdwp.
>>
>>
>> 1. Example command line:
>>
>> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,
>> address=*,allow="127.0.0.0/8;192.168.0.0/24"
>>
>> Possible values for allow parameter:
>>    *           - accept connections from everywhere.
>>    N.N.N.N     - accept connections from this IP address only
>>    N.N.N.N/nn  - accept connections from particular ip subnet
>>
>>
>>
>> 2. JDK-8052136 JDWP hardening
>>
>> -Dmitry
>>
>


--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Robbin Ehn
In reply to this post by Dmitry Samersoff
Hi Dmitry, thanks for the update.

One follow-up issue is that if you start suspended and than connect with an unallowed client the JVM starts and executes the program.
Simple program prints "Hello".

[rehn@rehn-ws vanilla-hs]$ java -agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:9999,allow=1.2.3.0/32 -cp . H
Listening for transport dt_socket at address: 9999
ERROR: Peer not allowed to connect
Listening for transport dt_socket at address: 9999
Hello

I think it would be good if the VM stayed suspended when an unallowed client connects.

Thanks, Robbin

On 03/13/2017 08:07 PM, Dmitry Samersoff wrote:

> Robbin,
>
> Please, see:
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.11
>
>> 1:
>> So connecting with an unallowed client terminates the VM.
>
> Fixed.
>
>> 2:
>> Starting with an bad allow filter terminates the VM when connecting a
>> client.
>
> Moved allowed parameter (and parser call) to StartListening.
>
> -Dmitry
>
>
> On 2017-03-10 15:56, Robbin Ehn wrote:
>> Hi Dmitry,
>>
>> I took a look at this, I have two practical issues:
>>
>> 1:
>> [rehn@rehn-ws dev]$ java
>> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:9999,allow=6.6.6.6
>> -cp runs ForEver
>> Listening for transport dt_socket at address: 9999
>> ERROR: transport error 202: peer not allowed to connect: Success
>> JDWP exit error JVMTI_ERROR_NONE(0): could not connect, timeout or fatal
>> error [transport.c:358]
>>
>> So connecting with an unallowed client terminates the VM.
>>
>> 2:
>> [rehn@rehn-ws dev]$ java
>> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:9999,allow=6.BAD.6.6
>> -cp runs ForEver
>> Listening for transport dt_socket at address: 9999
>> ERROR: transport error 202: unable to parse list of allowed peers: Success
>> JDWP exit error JVMTI_ERROR_NONE(0): could not connect, timeout or fatal
>> error [transport.c:358]
>>
>> Starting with an bad allow filter terminates the VM when connecting a
>> client.
>>
>>
>> Connecting with an unallowed ip/port should not terminate the VM and we
>> should verify allow filter directly at startup.
>>
>> Thanks
>>
>> /Robbin
>>
>> On 02/28/2017 10:41 AM, Dmitry Samersoff wrote:
>>> Everybody,
>>>
>>> Please review:
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.10/
>>>
>>> These changes introduce new parameter[1] of the socket transport -
>>> allow. Users can explicitly specify a list of hosts that allowed to
>>> connect to jdwp server and it's the second part of JDWP hardening[2].
>>>
>>> No restrictions are applied by default now but I'll file a separate CR
>>> to restrict list of allowed peers to localhost by default.
>>>
>>> Also these changes implement versioning for jdwp transport and therefor
>>> simplify feature development of jdwp.
>>>
>>>
>>> 1. Example command line:
>>>
>>> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,
>>> address=*,allow="127.0.0.0/8;192.168.0.0/24"
>>>
>>> Possible values for allow parameter:
>>>   *           - accept connections from everywhere.
>>>   N.N.N.N     - accept connections from this IP address only
>>>   N.N.N.N/nn  - accept connections from particular ip subnet
>>>
>>>
>>>
>>> 2. JDK-8052136 JDWP hardening
>>>
>>> -Dmitry
>>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

serguei.spitsyn@oracle.com
In reply to this post by Dmitry Samersoff
Hi Dmitry,



On 3/15/17 00:56, Dmitry Samersoff wrote:
> Serguei,
>
>> I still see the C .vs. C++ related change in the jdwpTransport.h.
> done.
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/

Good.
Thank you for the update!


>
> see also inline.
>
> On 2017-03-15 00:40, [hidden email] wrote:
>> Hi Dmitry,
>>
>> We already had a big review thread back in 2014 on this.
>> I think, it is again going in the wrong order.
>>
>> First, I think, it is better to start from a CCC (or its equivalent, CSR).
>> This will allow to focus on and sort out the changes in interface/behavior
>> first before going into the implementation details.
> CCC was filed and approved. The only reason I withdraw it - the fix
> didn't go to jdk9 but CCC tool doesn't have jdk10.
>
> CSR process is also not yet implemented.
The CSR preview message from Joe is attached.
My understanding is that we can continue to use CCC.
At some points the CCC process will be moved to the CSR.

The CCC is out of date now as it does not match the webrev 12.
Also, I do not like the addition of new function StartListening11() next
to StartListening().

Does it mean, that for transport version 1.2 we might have more function
clones with 12 suffix?
Perhaps, we need something smarter here but I'm unsure yet what it has
to be.


>> Second, I'd suggest to separate a couple of other things.
>> I still see the C .vs. C++ related change in the jdwpTransport.h.
>> It is better to leave it along for now as it was suggested in early
>> review rounds.
>> If you still want to fix it then it is better to file a separate bug that
>> should include the JNI as well (as it was discussed with Alan before).
> Do you mean?
>
>    39 #ifdef __cplusplus
>    40 extern "C" {
>    41 #endif
>
> I'll add it back to avoid any confusion.
Yes. I see you added it back in new version of webrev.


>
>> Also, I'm thinking if it is a good idea to separate the transport
>> versioning to an RFE.
>> It would allow to focus on this aspect as necessary.
>> In this case, the 8061228 will depend on the versioning.
>> However, it is much more simple and can be resolved faster.
> It's hard to test versioning code without implementation of
> new, VERSION_1_1 transport.
>
> Network part of 8061228 is simple and clear separated from versioning,
> so I would prefer to keep it together in one CR/one push.
No pressure.
I've got your point above.


Thanks,
Serguei

>
> Restriction turned off by default (I'll file separate CR to enable it
> later), so we have enough time to fix any issues that might come.
>
> -Dmitry
>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 2/28/17 01:41, Dmitry Samersoff wrote:
>>> Everybody,
>>>
>>> Please review:
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.10/
>>>
>>> These changes introduce new parameter[1] of the socket transport -
>>> allow. Users can explicitly specify a list of hosts that allowed to
>>> connect to jdwp server and it's the second part of JDWP hardening[2].
>>>
>>> No restrictions are applied by default now but I'll file a separate CR
>>> to restrict list of allowed peers to localhost by default.
>>>
>>> Also these changes implement versioning for jdwp transport and therefor
>>> simplify feature development of jdwp.
>>>
>>>
>>> 1. Example command line:
>>>
>>> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,
>>> address=*,allow="127.0.0.0/8;192.168.0.0/24"
>>>
>>> Possible values for allow parameter:
>>>     *           - accept connections from everywhere.
>>>     N.N.N.N     - accept connections from this IP address only
>>>     N.N.N.N/nn  - accept connections from particular ip subnet
>>>
>>>
>>>
>>> 2. JDK-8052136 JDWP hardening
>>>
>>> -Dmitry
>>>
>

Hello,

The JDK organization has long been served by the ccc webapp at
http://ccc.us.oracle.com. With the migration of bugs to JBS and the
availability of a configurable JIRA system, other workflows can be
supported by JBS as well, as seen in JEPs. As a continuation of this
trend and to allow better integration across processes, the ccc process,
with some adaptations, will soon be moved to JBS. After being moved to
JBS, the process will also be externalized, usable and visible to JBS
users who are not Oracle employees subject to the usual restrictions for
confidential issues.

Initially ccc reviews for post-9 releases will be done in the new
system. To avoid changing JCK and JCP-documentation processes
mid-release, JDK 9 will continue to be run on the existing ccc system.
After JDK 9 GA, there will be a high-fidelity bulk import of the old
requests into the new system.

One of the adaptations of the ccc process is a new name "CSR" standing
for "Compatibility and Specification Review." This new name is more
evocative of the role the process plays and is easier to explain to new
users.

The new "CSR" issue type is available for early preview on the JBS
staging system:

     https://bugs-stage.openjdk.java.net

To create a CSR for an issue, under the "More" menu select "Create CSR".
This is similar to how a backport can be created manually. [1]

Thanks to Tony Squire implementing the JBS support for CSR.

More information about the migration will be available next year,
including expected OpenJDK discussions about the process. Until the CSR
work is publicly disclosed, please keep discussions of this project
internal to Oracle.

The remainder of this email, including the attachment, includes more
detailed information about how the CSR process will differ from the ccc.

Please send initial feedback on the CSR implementation to me; there will
be more opportunities to provide comments before the process goes live.

Cheers,

-Joe

[1] Conceptually, a CSR is a subtask of its parent issue. However, we
cannot implement it that way in JIRA since we want to be able to allow a
CSR issue to have a different security setting than its parent bug. In
particular, we need to allow the import of all legacy ccc issues from
the current system as "Confidential" CSRs even if the parent issue is
publicly visible to avoid having to review and scrub all the legacy ccc
requests.


  Differences between ccc and CSR
-----------------------------

At a high-level, the envisioned CSR process is very similar to the ccc:
engineers present proposals for either a one-phase or two-phase review
by the review body. The body conducts the review, possibly providing
comments, and engineers and the review body update the state of the
request accordingly until it is approved or withdrawn.

Leveraging JIRA, rather than conducting the majority of the discussions
about a request on a separate mailing list, JIRA comments will instead
be used as the primary communication channel. This provides easier
notification to interested parties (e.g. watchers on the issue) and
better archiving of the discussion.

Looking over the sections of a ccc request, some were dropped as not
providing sufficient value for their cost including:

     Requestors
     Interface summary

The sections

     Problem
     Specification
     Solution

remain and the Description field of a CSR is pre-populated with a
markdown version of those three sections. The compatibility risk and
compatibility sections are retained as separate fields.

A CSR request has the new Scope field copied from JEPs with "SE", "JDK",
and "Implementation" choices

The attached picture shows the workflow of a CSR request. It is very
similar to the existing ccc workflow. Some difference and adaptions to JIRA:

* The "Accepted" state is renamed "Provisional." This is to put more
Hamming distance between "Accepted" and "Approved" to avoid confusion
for new and occasional ccc/csr users.

* To model JCK voting without building a full voting mechanism, the new
states "JCK Initial Review" and "JCK Final Review" have been introduced.
The JCK team can transition proposed and finalized requests,
respectively, to the new states.

* The chair can transition requests to the "Provisional" and "Closed /
Approved" states.

* Any CSR member can transition a request to "Pended".

* The assignee and reporter can transition a request to "Draft", "Closed
/ Will-not-fix" (the new "Withdrawn"), "Proposed", and "Finalized".

A few open issues about how the new process will work:

* Per general JBS guidelines, technical information about security
vulnerabilities should *not* be stored in JBS. This should be extended
to include CSR requests. As security vulnerability can and do need
ccc/CSR review, some alternative approach will need to be found.
Discussions on on the existing  ccc alias may be used as a fallback,
possibly with pointers into bugdb.

* Multi-release tracking: as currently implemented, if the same API
change is being made in multiple releases, say, 7uX, 8uX, and 9, rather
than filing three requests, one per release train, a single ccc request
against the oldest release can be filed with the understanding it also
applies by default to newer releases. The most direct way to support a
similar capability in CSR would be to use multiple values in the Fix
Version/s field. However, we have a strong convention for all other JBS
but to only use a single value of Fix Version/s and to instead use
backports for multi-release tracking.


csr-workflow.jpg (31K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

serguei.spitsyn@oracle.com
Dmitry,


Some quick comments on the webrev.12.


The style of comments must be /* */, not //.


http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/src/jdk.jdwp.agent/share/native/libjdwp/transport.c.frames.html

205 // Pass MAXIMAL supported version, expect actual transport version in

    Would it better to replace 'MAXIMAL' with 'the latest' ?


516 // interface version 1.0 doesn't support versioning, so we have to
517 // a use global variable and set the version artifically.
518 // Use (*t)->extra_data->version directly when 1.0 is gone

   516: interface => Interface
   517: Typo: a use => use
   518: Dot is missed at the end.


33 static unsigned transportVersion = JDWPTRANSPORT_VERSION_1_0; ... 207
ver = (*onLoad)(jvm, &callback, JDWPTRANSPORT_VERSION_1_1, &t);
208 if (ver == JNI_EVERSION) {
  209             ver = (*onLoad)(jvm, &callback, JDWPTRANSPORT_VERSION_1_0, &t);
210 // Special handling for versionless transports
211 info->transportVersion = JDWPTRANSPORT_VERSION_1_0;
212 }
213 else {
214 info->transportVersion = (*t)->extra_data->version;
215 } ...263 transportVersion = pTransportVersion; ... 459
info->transportVersion = transportVersion; It is not clear why do you
need the static variable transportVersion and this dance with it. It
seems, the transport version is always enforced by the TransportOnLoad
function anyway. At the line 459, you could just have:
459 info->transportVersion = JDWPTRANSPORT_VERSION_1_0; Thanks, Serguei

On 3/17/17 00:03, [hidden email] wrote:

> Hi Dmitry, On 3/15/17 00:56, Dmitry Samersoff wrote:
>> Serguei,
>>> I still see the C .vs. C++ related change in the jdwpTransport.h.
>> done. http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/ 
> Good. Thank you for the update!
>> see also inline. On 2017-03-15 00:40, [hidden email] wrote:
>>> Hi Dmitry, We already had a big review thread back in 2014 on this.
>>> I think, it is again going in the wrong order. First, I think, it is
>>> better to start from a CCC (or its equivalent, CSR). This will allow
>>> to focus on and sort out the changes in interface/behavior first
>>> before going into the implementation details.
>> CCC was filed and approved. The only reason I withdraw it - the fix
>> didn't go to jdk9 but CCC tool doesn't have jdk10. CSR process is
>> also not yet implemented.
> The CSR preview message from Joe is attached. My understanding is that
> we can continue to use CCC. At some points the CCC process will be
> moved to the CSR. The CCC is out of date now as it does not match the
> webrev 12. Also, I do not like the addition of new function
> StartListening11() next to StartListening(). Does it mean, that for
> transport version 1.2 we might have more function clones with 12
> suffix? Perhaps, we need something smarter here but I'm unsure yet
> what it has to be.
>>> Second, I'd suggest to separate a couple of other things. I still
>>> see the C .vs. C++ related change in the jdwpTransport.h. It is
>>> better to leave it along for now as it was suggested in early review
>>> rounds. If you still want to fix it then it is better to file a
>>> separate bug that should include the JNI as well (as it was
>>> discussed with Alan before).
>> Do you mean?    39 #ifdef __cplusplus    40 extern "C" {    41 #endif
>> I'll add it back to avoid any confusion.
> Yes. I see you added it back in new version of webrev.
>>> Also, I'm thinking if it is a good idea to separate the transport
>>> versioning to an RFE. It would allow to focus on this aspect as
>>> necessary. In this case, the 8061228 will depend on the versioning.
>>> However, it is much more simple and can be resolved faster.
>> It's hard to test versioning code without implementation of new,
>> VERSION_1_1 transport. Network part of 8061228 is simple and clear
>> separated from versioning, so I would prefer to keep it together in
>> one CR/one push.
> No pressure. I've got your point above. Thanks, Serguei
>> Restriction turned off by default (I'll file separate CR to enable it
>> later), so we have enough time to fix any issues that might come.
>> -Dmitry
>>> Thanks, Serguei On 2/28/17 01:41, Dmitry Samersoff wrote:
>>>> Everybody, Please review:
>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.10/ These
>>>> changes introduce new parameter[1] of the socket transport - allow.
>>>> Users can explicitly specify a list of hosts that allowed to
>>>> connect to jdwp server and it's the second part of JDWP
>>>> hardening[2]. No restrictions are applied by default now but I'll
>>>> file a separate CR to restrict list of allowed peers to localhost
>>>> by default. Also these changes implement versioning for jdwp
>>>> transport and therefor simplify feature development of jdwp. 1.
>>>> Example command line:
>>>> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,
>>>> address=*,allow="127.0.0.0/8;192.168.0.0/24" Possible values for
>>>> allow parameter:     *           - accept connections from
>>>> everywhere.     N.N.N.N     - accept connections from this IP
>>>> address only     N.N.N.N/nn  - accept connections from particular
>>>> ip subnet 2. JDK-8052136 JDWP hardening -Dmitry
Loading...