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

[Sean Mullan]

On 8/8/18 5:29 PM, Xuelei Fan wrote:
> The "Default" algorithm defined in the SunJSSE provider is for TLS 
> protocols.

What if I set DTLS to be the default, though? Ex:

     SSLContext.setDefault(SSLContext.getInstance("DTLS"));

--Sean

> 
> Xuelei
> 
> On 8/8/2018 1:28 PM, Sean Mullan wrote:
>> On 8/8/18 1:58 PM, Anthony Scarpino wrote:
>>> I don't see where your example of getDefault() is any different than 
>>> the current code path.  A user has to define a SSLContext when using 
>>> getDefault() which as far as I can tell goes through the code path of 
>>> SSLContext.get[Server]SocketFactory()
>>
>> SSL{Server}SocketFactory.getDefault() says: "Otherwise, this method 
>> returns SSLContext.getDefault().get{Server}SocketFactory(). If that 
>> call fails, an inoperative factory is returned."
>>
>> So it doesn't look like we have to do anything special for this case, 
>> but as part of this fix, we should check/test that an inoperative 
>> factory is returned if a DTLS context is the default.
>>
>> --Sean
>>
>>>
>>> Tony
>>>
>>> On 08/08/2018 08:52 AM, Chris Hegarty wrote:
>>>> +1 to everything Sean said, and just to add ...
>>>>
>>>> This change will prevent an SSLContext from giving out socket
>>>> factories when it has been configured with DTLS. What about
>>>> SSL[Server]SocketFactory::getDefault when the default SSL
>>>> context is DTLS? I don’t see that getDefault can throw an UOE,
>>>> should it?    Or should / could this be resolved at the socket
>>>> factory level, when trying to create new sockets rather than at
>>>> the factory retrieval time?
>>>>
>>>> -Chris.
>>>>
>>>>> On 8 Aug 2018, at 15:46, Sean Mullan <sean.mullan at oracle.com> 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.
>>>>>
>>>>> 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.
>>>>>
>>>>> --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
>>>>>>>>>
>>>>>>>
>>>>
>>>