RFR JDK-8239595/JDK-8239594 : ssl context version is not respected/jdk.tls.client.protocols is not respected

[Michael McMahon]

Hi Xuelei,

I have some concerns about these bugs also, though not exactly the same 
as yours:

The "jdk.tls.client.protocols" system property is not part of the HTTP 
client API. So, it's not
clear to me why the HTTP client is expected to enforce it. It is equally 
possible for any code using
SSLContext and SSLParameters to select protocols that appear to 
contradict the protocols set
in the property.

Same goes for the protocol used to initialize the SSLContext itself (bug 
8239595).
If it is an error to initialize the context with TLSv1.2, but then try 
to set TLSv1.3 in the SSLParameters,
then why doesn't the TLS layer enforce that constraint? Is it even 
specified anywhere?

That said, I think there is an issue that needs to be fixed. If a user 
wants to use TLS1.2, we
certainly should not force them to use 1.3.

I am beginning to think that the HTTP client should behave in a way that 
is more clear/transparent
to its users as follows:

1) If the user specifies neither an SSLContext nor an SSLParameters, 
then the HTTP library should
     choose a TLS version that is most likely to work for HTTP (maybe 
choose highest version that is available)

2) If the user specifies an SSLContext then we use the default 
SSLParameters without adding any protocol versions

3) If the user does not specify an SSLContext but does specify an 
SSLParameters then we create a default context
    and apply the given SSLParameters (with no additional protocol 
versions added)

So, for 2) and 3) it is up to the caller to ensure that the resulting 
SSL configuration "works".  It may fail
of course with a handshake error (or runtime error presumably if you try 
to select DTLS for example).

Maybe it would make more sense to fix that behavior through another bug 
id, because in my view these
two issues are more of a spec/impl issue for the TLS layer itself.

What do you think?

Michael.

On 26/03/2020 16:58, Xuelei Fan wrote:
> With this update, the logic looks like: if TLSv1.3 is not enabled in 
> the SSLContext, use TLSv1.2 instead;  Otherwise, use TLSv1.3 and TLSv1.2.
>
> There may be a couple of issues:
> 1. TLSv1.2 may be not enabled, although TLSv1.3 is enabled.
> For example:
>    System.setProperty("jdk.tls.client.protocols", "TLSv1.3")
>    System.setProperty("jdk.tls.client.protocols", "TLSv1.1, TLSv1.0")
>
> 2. TLSv1.2 may be not supported in the SSLContext.
> For example:
>    SSLContext context = SSLContext.getInstance("DTLS");
>    HttpClient.newBuilder().sslContext(context)...
>
> 3. The application may not want to use TLS 1.2.
> For example:
>    System.setProperty("jdk.tls.client.protocols", "TLSv1.1, TLSv1.0")
>
> The System property may be shared by code other than httpclient. So 
> the setting may not consider the impact on httpclient.
>
> I may use enabled protocols only. If no TLSv1.2/TLSv1.3, I may use an 
> empty protocol array, and test to see what happens in the httpclient 
> implementation stack.
>
> Xuelei
>
> On 3/26/2020 9:28 AM, Sean Mullan wrote:
>> Cross-posting to security-dev as this involves TLS/SSL configuration.
>>
>> --Sean
>>
>> On 3/26/20 10:02 AM, rahul.r.yadav at oracle.com wrote:
>>> Hello,
>>>
>>> Request to have my fix reviewed for issues:
>>>
>>>      JDK-8239595 : ssl context version is not respected
>>>      JDK-8239594 : jdk.tls.client.protocols is not respected
>>>
>>> The fix updates 
>>> jdk.internal.net.http.HttpClientImpl.getDefaultParams(SSLContext ctx)
>>> to use ctx.getDefaultSSLParameters()instead of 
>>> ctx.getSupportedSSLParameters(),
>>> as the latter does not respect the context parameters set by the user.
>>>
>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8239595
>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8239594
>>>
>>> Webrev: 
>>> http://cr.openjdk.java.net/~jboes/rayayada/webrevs/8239595/webrev.00/
>>>
>>> -- Rahul