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

[Anthony Scarpino]

On 08/09/2018 06:49 AM, Xuelei Fan wrote:
> 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.

I'm not sure the code path you're looking as the oneI see seems pretty 
obscure.

Are you two talking about where SSL[Server]SocketFactory.getDefault() 
uses a ssl.SocketFactory.provider property set to SunJSSE?  If so, can 
see that as a code review comment, but it seems very obscure for the CSR.



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