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

[Anthony Scarpino]

On 08/14/2018 11:27 AM, Sean Mullan wrote:
> On 8/14/18 1:56 PM, Anthony Scarpino wrote:
>> On 08/13/2018 12:42 PM, Sean Mullan wrote:
>>> On 8/10/18 3:49 PM, Anthony Scarpino 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.
>>>
>>> Here's the code I would use:
>>>
>>>          SSLContext.setDefault(SSLContext.getInstance("DTLS"));
>>>          ServerSocketFactory fac = SSLServerSocketFactory.getDefault();
>>>
>>> If I read the spec correctly, fac should be an "inoperative factory".
>>>
>>> --Sean
>>
>>
>> Sure, but SSLServerSocketFactory.getDefault() typically return 
>> SSLContext.getDefault().getServerSocketFactory(). Which is covered by 
>> the planned exception change, unless I'm missing something here.
>>
>> Now there is the atypically case where the property is set, but that 
>> seems like a stretch to set the property to use SunJSSE,
>>
>> Finally, even if there is a change to be made here. I'm not sure the 
>> impact of the CSR.. It's seems easier to explain an obvious exception 
>> than a nebulous "inoperative factory"
> 
> If our implementation throws an Exception that is not specified by the 
> API, then we are non-compliant (or the API needs to be updated to throw 
> that Exception).
> 
> --Sean
> 

Fair point.. It throws nothing right now.  I guess inoperative factory 
it shall be.

I updated the Solution to include this change, I don't think it needs to 
be hi-lighted elswhere given it's a result to the main change.

Tony