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

[Dmitry Samersoff]

Serguei,

1. New webrev with couple of issues addressed (see Robbin's e-mails):

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

2. We have to keep transport version in global variable because old
transport doesn't initialize reserved1 field and we are loosing version
information after first disconnect. i.e. if I remove this "dancing"
debugger is not able to connect second time.

I'd reorganized code a bit for better readability.

3. ccc tool doesn't have JDK10 so we can't go forward.

-Dmitry


On 2017-03-17 12:20, serguei.spitsyn at oracle.com wrote:
> 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, serguei.spitsyn at oracle.com 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, serguei.spitsyn at oracle.com 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 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.