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

[Xuelei Fan]

On 8/9/2018 4:25 AM, Sean Mullan wrote:
> 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"));
> 
Good point!  Maybe, we also need to update the 
SSLSocketFactory/SSLServerSocketFactory.getDefault() to return 
inoperative factory.

Xuelei

> --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
>>>>>>>>>>
>>>>>>>>
>>>>>
>>>>