CSR Review: 8208641: SSLSocket should throw an exception when configuring DTLS

[Anthony Scarpino]

On 08/08/2018 07:46 AM, Sean Mullan wrote:
> On 8/7/18 8:05 PM, Xuelei Fan wrote:
>> Hi Tony,
>>
>> The Specification section looks more like the implementation details. 
>> We may change the implementation details again in the future.  It may 
>> be more suitable to move it to the Solution section, or just remove it. 
> 
> I agree, I would omit the diffs and put N/A for the Specification section.
> 
>> In the Specification section, I may just say something like, "No APIs 
>> changes.  The SunJSSE provider is updated to throw 
>> UnsupportedOperationException if  SSLContext.SSLServerSocketFactory() 
>> or SSLContext.SSLSocketFactory() get called for DTLS algorithms 
>> SSLContext".
> 
> I think the CSR should also say that the SunJSSE implementation of the 
> engineGetSocketFactory and engineGetServerSocketFactory methods of 
> SSLContextSpi have been changed to throw UnsupportedOperationException. 
> That is the specific API behavior change.
> 
> A few other comments on the CSR:
> 
> "SSLContext.getSSLSocketFactory() and 
> SSLContext.getSSLServerSocketFactory()"
> 
> Typo, there is no "SSL" in the method names.
> 
> The Compatibility Risk field has several typos ("there" -> "their", "how 
> -> now", ...) and is a bit hard to understand. Wasn't TLS being used 
> before instead of DTLS in certain scenarios? Because the API silently 
> passed in certain cases, and now we will be throwing an Exception, maybe 
> it might be better to say the risk is low.

I fixed the typos and redid the compatibility part that hopefully is 
more to the point.

> 
> In the Summary section, it says "..., but it is not documented." I 
> suggest opening a docs bug to improve the DTLS documentation so that it 
> is more clear this scenario is not supported.
> 
> I think the Interface Kind should be Java API since we are changing the 
> behavior of an implementation of a standard API. I asked Joe Darcy this 
> question yesterday, and he agreed.

I thought about API, but since it was a behavior change, API didn't 
sound completely correct.  But that's fine if that's what he wants.
> 
> --Sean
> 
>>
>> Thanks,
>> Xuelei
>>
>> On 8/7/2018 4:14 PM, Anthony Scarpino wrote:
>>> Hi Xuelei,
>>>
>>> I have updated the csr and I believe I have addressed your comments.
>>>
>>> thanks
>>>
>>> Tony
>>>
>>> On 08/07/2018 01:43 PM, Xuelei Fan wrote:
>>>> Hi Tony,
>>>>
>>>> Would you mind make it clear that this impact the JDK JSSE provider 
>>>> only?  Third party's provider may be able to support DTLS with 
>>>> SSLSocket.
>>>>
>>>> I think there may be no specification change.  The 
>>>> SSLContext.getServerSocketFactory() and 
>>>> SSLContext.getSocketFactory() defines the spec if the algorithm is 
>>>> not supported by the underlying provider, 
>>>> "UnsupportedOperationException - if the underlying provider does not 
>>>> implement the operation.".  I may prefer to make it clear that this 
>>>> is just a behavior change of the JDK JSSE provider (SunJSSE).  The 
>>>> SunJSSE provider now throws UnsupportedOperationException for 
>>>> creating SSL(Server)SocketFactory with DTLS SSLContext, because it 
>>>> does not actually support DTLS SSLSocket.
>>>>
>>>> In Solution section, "Throwing a UnsupportedOperationException when 
>>>> getting a socket from the SSLServerSocketFactory or SSLSocketFactory 
>>>> for DTLS."   I guess you meant, throwing a UOE when calling 
>>>> SSLContext.getServerSocketFactory() and SSLContext.getSocketFactory()?
>>>>
>>>> Thanks,
>>>> Xuelei
>>>>
>>>> On 8/7/2018 12:17 PM, Anthony Scarpino wrote:
>>>>> I need a review of a CSR for SSLSocket should throw an exception 
>>>>> when configuring DTLS.  We are targeting this for 12 right now.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8209031
>>>>>
>>>>> thanks
>>>>>
>>>>> Tony
>>>>>
>>>