RFR (10): 8182589 TLS SNI in new Java 9 client is not available

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

RFR (10): 8182589 TLS SNI in new Java 9 client is not available

Michael McMahon
Hi,

Could I get the following change reviewed please?

http://cr.openjdk.java.net/~michaelm/8182589/webrev.1/

This also fixes 8181422.

Thanks,
Michael
Reply | Threaded
Open this post in threaded view
|

Re: RFR (10): 8182589 TLS SNI in new Java 9 client is not available

Michael McMahon
Just to clarify, there are two issues in 8181422. This fixes one of them
but not the other. So, I will be leaving 8181422 open, and will just
edit out
the part that is fixed (the NPE from not calling GET() )

- Michael.

On 27/06/2017, 10:26, Michael McMahon wrote:

> Hi,
>
> Could I get the following change reviewed please?
>
> http://cr.openjdk.java.net/~michaelm/8182589/webrev.1/
>
> This also fixes 8181422.
>
> Thanks,
> Michael
Reply | Threaded
Open this post in threaded view
|

Re: RFR (10): 8182589 TLS SNI in new Java 9 client is not available

Daniel Fuchs
Hi Michael,

Looks good. I have imported your patch in my local
workspace and played with it a bit.

AsyncSSLDelegate.java:
  128         this.serverName = sname;
and SSLDelegate.java
  63         this.serverName = sn;
  70         serverName = sn;

Should this fail if sname/sn is null? Or is null a valid value?

HttpRequestImpl.java

  61         this.method = builder.method() == null? "GET" :
builder.method();

just a nit, but I think it might be better to use a local
variable (here and at line 81) to avoid calling method()
twice - and avoid having to wonder what will happens
if the second call returns a different value.

best regards,

-- daniel

On 27/06/2017 12:17, Michael McMahon wrote:

> Just to clarify, there are two issues in 8181422. This fixes one of them
> but not the other. So, I will be leaving 8181422 open, and will just
> edit out
> the part that is fixed (the NPE from not calling GET() )
>
> - Michael.
>
> On 27/06/2017, 10:26, Michael McMahon wrote:
>> Hi,
>>
>> Could I get the following change reviewed please?
>>
>> http://cr.openjdk.java.net/~michaelm/8182589/webrev.1/
>>
>> This also fixes 8181422.
>>
>> Thanks,
>> Michael

Reply | Threaded
Open this post in threaded view
|

Re: RFR (10): 8182589 TLS SNI in new Java 9 client is not available

Michael McMahon
Thanks Daniel. The serverName comes from the InetSocketAddress for the
destination.
It can't have a null field. Though I am wondering now if in the case of
hardcoded IP address
destinations, that we shouldn't set the SNI hostname.

- Michael

On 27/06/2017, 15:44, Daniel Fuchs wrote:

> Hi Michael,
>
> Looks good. I have imported your patch in my local
> workspace and played with it a bit.
>
> AsyncSSLDelegate.java:
>  128         this.serverName = sname;
> and SSLDelegate.java
>  63         this.serverName = sn;
>  70         serverName = sn;
>
> Should this fail if sname/sn is null? Or is null a valid value?
>
> HttpRequestImpl.java
>
>  61         this.method = builder.method() == null? "GET" :
> builder.method();
>
> just a nit, but I think it might be better to use a local
> variable (here and at line 81) to avoid calling method()
> twice - and avoid having to wonder what will happens
> if the second call returns a different value.
>
> best regards,
>
> -- daniel
>
> On 27/06/2017 12:17, Michael McMahon wrote:
>> Just to clarify, there are two issues in 8181422. This fixes one of them
>> but not the other. So, I will be leaving 8181422 open, and will just
>> edit out
>> the part that is fixed (the NPE from not calling GET() )
>>
>> - Michael.
>>
>> On 27/06/2017, 10:26, Michael McMahon wrote:
>>> Hi,
>>>
>>> Could I get the following change reviewed please?
>>>
>>> http://cr.openjdk.java.net/~michaelm/8182589/webrev.1/
>>>
>>> This also fixes 8181422.
>>>
>>> Thanks,
>>> Michael
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (10): 8182589 TLS SNI in new Java 9 client is not available

Daniel Fuchs
On 27/06/2017 17:06, Michael McMahon wrote:
> Thanks Daniel. The serverName comes from the InetSocketAddress for the
> destination.
> It can't have a null field. Though I am wondering now if in the case of
> hardcoded IP address
> destinations, that we shouldn't set the SNI hostname.

In that case I wonder whether these two statements should be
conditional to serverName != null?

  140         SNIHostName sn = new SNIHostName(serverName);
  141         sslParameters.setServerNames(List.of(sn));

(IIRC there are 2 files)

And if serverName can't be null - maybe a comment or an
assert or an Objects.requireNonNull could make that clear.

best regards,

-- daniel

>
> - Michael
>
> On 27/06/2017, 15:44, Daniel Fuchs wrote:
>> Hi Michael,
>>
>> Looks good. I have imported your patch in my local
>> workspace and played with it a bit.
>>
>> AsyncSSLDelegate.java:
>>  128         this.serverName = sname;
>> and SSLDelegate.java
>>  63         this.serverName = sn;
>>  70         serverName = sn;
>>
>> Should this fail if sname/sn is null? Or is null a valid value?
>>
>> HttpRequestImpl.java
>>
>>  61         this.method = builder.method() == null? "GET" :
>> builder.method();
>>
>> just a nit, but I think it might be better to use a local
>> variable (here and at line 81) to avoid calling method()
>> twice - and avoid having to wonder what will happens
>> if the second call returns a different value.
>>
>> best regards,
>>
>> -- daniel
>>
>> On 27/06/2017 12:17, Michael McMahon wrote:
>>> Just to clarify, there are two issues in 8181422. This fixes one of them
>>> but not the other. So, I will be leaving 8181422 open, and will just
>>> edit out
>>> the part that is fixed (the NPE from not calling GET() )
>>>
>>> - Michael.
>>>
>>> On 27/06/2017, 10:26, Michael McMahon wrote:
>>>> Hi,
>>>>
>>>> Could I get the following change reviewed please?
>>>>
>>>> http://cr.openjdk.java.net/~michaelm/8182589/webrev.1/
>>>>
>>>> This also fixes 8181422.
>>>>
>>>> Thanks,
>>>> Michael
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (10): 8182589 TLS SNI in new Java 9 client is not available

Michael McMahon
Yes, I agree Daniel. I will regenerate the webrev addressing that point.

Thanks,

Michael

On 27/06/2017, 17:42, Daniel Fuchs wrote:

> On 27/06/2017 17:06, Michael McMahon wrote:
>> Thanks Daniel. The serverName comes from the InetSocketAddress for
>> the destination.
>> It can't have a null field. Though I am wondering now if in the case
>> of hardcoded IP address
>> destinations, that we shouldn't set the SNI hostname.
>
> In that case I wonder whether these two statements should be
> conditional to serverName != null?
>
>  140         SNIHostName sn = new SNIHostName(serverName);
>  141         sslParameters.setServerNames(List.of(sn));
>
> (IIRC there are 2 files)
>
> And if serverName can't be null - maybe a comment or an
> assert or an Objects.requireNonNull could make that clear.
>
> best regards,
>
> -- daniel
>
>>
>> - Michael
>>
>> On 27/06/2017, 15:44, Daniel Fuchs wrote:
>>> Hi Michael,
>>>
>>> Looks good. I have imported your patch in my local
>>> workspace and played with it a bit.
>>>
>>> AsyncSSLDelegate.java:
>>>  128         this.serverName = sname;
>>> and SSLDelegate.java
>>>  63         this.serverName = sn;
>>>  70         serverName = sn;
>>>
>>> Should this fail if sname/sn is null? Or is null a valid value?
>>>
>>> HttpRequestImpl.java
>>>
>>>  61         this.method = builder.method() == null? "GET" :
>>> builder.method();
>>>
>>> just a nit, but I think it might be better to use a local
>>> variable (here and at line 81) to avoid calling method()
>>> twice - and avoid having to wonder what will happens
>>> if the second call returns a different value.
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>> On 27/06/2017 12:17, Michael McMahon wrote:
>>>> Just to clarify, there are two issues in 8181422. This fixes one of
>>>> them
>>>> but not the other. So, I will be leaving 8181422 open, and will
>>>> just edit out
>>>> the part that is fixed (the NPE from not calling GET() )
>>>>
>>>> - Michael.
>>>>
>>>> On 27/06/2017, 10:26, Michael McMahon wrote:
>>>>> Hi,
>>>>>
>>>>> Could I get the following change reviewed please?
>>>>>
>>>>> http://cr.openjdk.java.net/~michaelm/8182589/webrev.1/
>>>>>
>>>>> This also fixes 8181422.
>>>>>
>>>>> Thanks,
>>>>> Michael
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (10): 8182589 TLS SNI in new Java 9 client is not available

Michael McMahon
Webrev updated at

http://cr.openjdk.java.net/~michaelm/8182589/webrev.2/index.html

Thanks
Michael

On 27/06/2017, 18:07, Michael McMahon wrote:

> Yes, I agree Daniel. I will regenerate the webrev addressing that point.
>
> Thanks,
>
> Michael
>
> On 27/06/2017, 17:42, Daniel Fuchs wrote:
>> On 27/06/2017 17:06, Michael McMahon wrote:
>>> Thanks Daniel. The serverName comes from the InetSocketAddress for
>>> the destination.
>>> It can't have a null field. Though I am wondering now if in the case
>>> of hardcoded IP address
>>> destinations, that we shouldn't set the SNI hostname.
>>
>> In that case I wonder whether these two statements should be
>> conditional to serverName != null?
>>
>>  140         SNIHostName sn = new SNIHostName(serverName);
>>  141         sslParameters.setServerNames(List.of(sn));
>>
>> (IIRC there are 2 files)
>>
>> And if serverName can't be null - maybe a comment or an
>> assert or an Objects.requireNonNull could make that clear.
>>
>> best regards,
>>
>> -- daniel
>>
>>>
>>> - Michael
>>>
>>> On 27/06/2017, 15:44, Daniel Fuchs wrote:
>>>> Hi Michael,
>>>>
>>>> Looks good. I have imported your patch in my local
>>>> workspace and played with it a bit.
>>>>
>>>> AsyncSSLDelegate.java:
>>>>  128         this.serverName = sname;
>>>> and SSLDelegate.java
>>>>  63         this.serverName = sn;
>>>>  70         serverName = sn;
>>>>
>>>> Should this fail if sname/sn is null? Or is null a valid value?
>>>>
>>>> HttpRequestImpl.java
>>>>
>>>>  61         this.method = builder.method() == null? "GET" :
>>>> builder.method();
>>>>
>>>> just a nit, but I think it might be better to use a local
>>>> variable (here and at line 81) to avoid calling method()
>>>> twice - and avoid having to wonder what will happens
>>>> if the second call returns a different value.
>>>>
>>>> best regards,
>>>>
>>>> -- daniel
>>>>
>>>> On 27/06/2017 12:17, Michael McMahon wrote:
>>>>> Just to clarify, there are two issues in 8181422. This fixes one
>>>>> of them
>>>>> but not the other. So, I will be leaving 8181422 open, and will
>>>>> just edit out
>>>>> the part that is fixed (the NPE from not calling GET() )
>>>>>
>>>>> - Michael.
>>>>>
>>>>> On 27/06/2017, 10:26, Michael McMahon wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Could I get the following change reviewed please?
>>>>>>
>>>>>> http://cr.openjdk.java.net/~michaelm/8182589/webrev.1/
>>>>>>
>>>>>> This also fixes 8181422.
>>>>>>
>>>>>> Thanks,
>>>>>> Michael
>>>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (10): 8182589 TLS SNI in new Java 9 client is not available

Michael McMahon
Third and hopefully final version at

http://cr.openjdk.java.net/~michaelm/8182589/webrev.3/index.html

- Michael

On 28/06/2017, 12:29, Michael McMahon wrote:

> Webrev updated at
>
> http://cr.openjdk.java.net/~michaelm/8182589/webrev.2/index.html
>
> Thanks
> Michael
>
> On 27/06/2017, 18:07, Michael McMahon wrote:
>> Yes, I agree Daniel. I will regenerate the webrev addressing that point.
>>
>> Thanks,
>>
>> Michael
>>
>> On 27/06/2017, 17:42, Daniel Fuchs wrote:
>>> On 27/06/2017 17:06, Michael McMahon wrote:
>>>> Thanks Daniel. The serverName comes from the InetSocketAddress for
>>>> the destination.
>>>> It can't have a null field. Though I am wondering now if in the
>>>> case of hardcoded IP address
>>>> destinations, that we shouldn't set the SNI hostname.
>>>
>>> In that case I wonder whether these two statements should be
>>> conditional to serverName != null?
>>>
>>>  140         SNIHostName sn = new SNIHostName(serverName);
>>>  141         sslParameters.setServerNames(List.of(sn));
>>>
>>> (IIRC there are 2 files)
>>>
>>> And if serverName can't be null - maybe a comment or an
>>> assert or an Objects.requireNonNull could make that clear.
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>>>
>>>> - Michael
>>>>
>>>> On 27/06/2017, 15:44, Daniel Fuchs wrote:
>>>>> Hi Michael,
>>>>>
>>>>> Looks good. I have imported your patch in my local
>>>>> workspace and played with it a bit.
>>>>>
>>>>> AsyncSSLDelegate.java:
>>>>>  128         this.serverName = sname;
>>>>> and SSLDelegate.java
>>>>>  63         this.serverName = sn;
>>>>>  70         serverName = sn;
>>>>>
>>>>> Should this fail if sname/sn is null? Or is null a valid value?
>>>>>
>>>>> HttpRequestImpl.java
>>>>>
>>>>>  61         this.method = builder.method() == null? "GET" :
>>>>> builder.method();
>>>>>
>>>>> just a nit, but I think it might be better to use a local
>>>>> variable (here and at line 81) to avoid calling method()
>>>>> twice - and avoid having to wonder what will happens
>>>>> if the second call returns a different value.
>>>>>
>>>>> best regards,
>>>>>
>>>>> -- daniel
>>>>>
>>>>> On 27/06/2017 12:17, Michael McMahon wrote:
>>>>>> Just to clarify, there are two issues in 8181422. This fixes one
>>>>>> of them
>>>>>> but not the other. So, I will be leaving 8181422 open, and will
>>>>>> just edit out
>>>>>> the part that is fixed (the NPE from not calling GET() )
>>>>>>
>>>>>> - Michael.
>>>>>>
>>>>>> On 27/06/2017, 10:26, Michael McMahon wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Could I get the following change reviewed please?
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~michaelm/8182589/webrev.1/
>>>>>>>
>>>>>>> This also fixes 8181422.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Michael
>>>>>
>>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (10): 8182589 TLS SNI in new Java 9 client is not available

Daniel Fuchs
Hi Michael,

On 28/06/2017 17:48, Michael McMahon wrote:
> Third and hopefully final version at
>
> http://cr.openjdk.java.net/~michaelm/8182589/webrev.3/index.html

This one looks good to me! :-)

best regards,

-- daniel

>
> - Michael
>
> On 28/06/2017, 12:29, Michael McMahon wrote:
>> Webrev updated at
>>
>> http://cr.openjdk.java.net/~michaelm/8182589/webrev.2/index.html
>>
>> Thanks
>> Michael
>>
>> On 27/06/2017, 18:07, Michael McMahon wrote:
>>> Yes, I agree Daniel. I will regenerate the webrev addressing that point.
>>>
>>> Thanks,
>>>
>>> Michael
>>>
>>> On 27/06/2017, 17:42, Daniel Fuchs wrote:
>>>> On 27/06/2017 17:06, Michael McMahon wrote:
>>>>> Thanks Daniel. The serverName comes from the InetSocketAddress for
>>>>> the destination.
>>>>> It can't have a null field. Though I am wondering now if in the
>>>>> case of hardcoded IP address
>>>>> destinations, that we shouldn't set the SNI hostname.
>>>>
>>>> In that case I wonder whether these two statements should be
>>>> conditional to serverName != null?
>>>>
>>>>  140         SNIHostName sn = new SNIHostName(serverName);
>>>>  141         sslParameters.setServerNames(List.of(sn));
>>>>
>>>> (IIRC there are 2 files)
>>>>
>>>> And if serverName can't be null - maybe a comment or an
>>>> assert or an Objects.requireNonNull could make that clear.
>>>>
>>>> best regards,
>>>>
>>>> -- daniel
>>>>
>>>>>
>>>>> - Michael
>>>>>
>>>>> On 27/06/2017, 15:44, Daniel Fuchs wrote:
>>>>>> Hi Michael,
>>>>>>
>>>>>> Looks good. I have imported your patch in my local
>>>>>> workspace and played with it a bit.
>>>>>>
>>>>>> AsyncSSLDelegate.java:
>>>>>>  128         this.serverName = sname;
>>>>>> and SSLDelegate.java
>>>>>>  63         this.serverName = sn;
>>>>>>  70         serverName = sn;
>>>>>>
>>>>>> Should this fail if sname/sn is null? Or is null a valid value?
>>>>>>
>>>>>> HttpRequestImpl.java
>>>>>>
>>>>>>  61         this.method = builder.method() == null? "GET" :
>>>>>> builder.method();
>>>>>>
>>>>>> just a nit, but I think it might be better to use a local
>>>>>> variable (here and at line 81) to avoid calling method()
>>>>>> twice - and avoid having to wonder what will happens
>>>>>> if the second call returns a different value.
>>>>>>
>>>>>> best regards,
>>>>>>
>>>>>> -- daniel
>>>>>>
>>>>>> On 27/06/2017 12:17, Michael McMahon wrote:
>>>>>>> Just to clarify, there are two issues in 8181422. This fixes one
>>>>>>> of them
>>>>>>> but not the other. So, I will be leaving 8181422 open, and will
>>>>>>> just edit out
>>>>>>> the part that is fixed (the NPE from not calling GET() )
>>>>>>>
>>>>>>> - Michael.
>>>>>>>
>>>>>>> On 27/06/2017, 10:26, Michael McMahon wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Could I get the following change reviewed please?
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~michaelm/8182589/webrev.1/
>>>>>>>>
>>>>>>>> This also fixes 8181422.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Michael
>>>>>>
>>>>