RFR JDK-8236039: JSSE Client does not accept status_request extension in CertificateRequest messages for TLS 1.3

Sun Jan 5 02:41:42 UTC 2020

It's a good idea to me to have a SSLExtension.nameOf(int) method.

Thanks,
Xuelei

On 1/4/2020 3:53 PM, Jamil Nimeh wrote:
> Let me throw out another idea.  I think the actual parsing only happens 
> in the consumer if you call the spec constructor.  You could have a 
> consumer registered that simply returns null.  That avoids the extra 
> unknown/unsupported log message that we don't need and you don't have 
> the cost of actually parsing the extension.  It should just skip over 
> it.  The only performance hit you take is the actual call into one 
> additional method.  That shouldn't have a major performance impact.
> 
> Just to see how it would work, I created SSLExtension.nameOf(int) which 
> takes the extension ID and returns a name or "unknown extension".  I did 
> that because I didn't want to mess with valueOf(SSLHandshake, int) since 
> there's other logic depending on that returning null for extensions we 
> don't support.
> 
> The result was that the unsupported-but-known extensions have their 
> names presented, which is a nice thing for those looking at the log.
> 
> --Jamil
> 
> On 1/4/20 3:35 PM, Xuelei Fan wrote:
>> We may not want to parse the extension if the debug is not enabled.   
>> It may not worthy to define consume for debug only.  How about replace 
>> “unknown” with “unsupported”? And parse the extension id to literal 
>> name if the name is known?
>>
>> Xuelei
>>
>>> On Jan 4, 2020, at 3:19 PM, Jamil Nimeh <jamil.j.nimeh at oracle.com> 
>>> wrote:
>>>
>>> Hi Xuelei, I backed out my change and went solely with the approach 
>>> you suggested below.  It works in that it allows the handshake to 
>>> proceed.  I notice these debug log messages though during consumption 
>>> of the CR message from the server:
>>>
>>> javax.net.ssl|DEBUG|01|main|2020-01-04 15:02:26.636 
>>> PST|SSLExtensions.java:135|Ignore unknown or unsupported extension (
>>> "unknown extension (5)": {
>>>
>>> }
>>> ...
>>> javax.net.ssl|DEBUG|01|main|2020-01-04 15:02:26.656 
>>> PST|CertificateRequest.java:926|Consuming CertificateRequest 
>>> handshake message (
>>> "CertificateRequest": {
>>>    "certificate_request_context": "",
>>>    "extensions": [
>>>      "unknown extension (5)": {
>>>
>>>      },
>>>      "unknown extension (18)": {
>>>
>>>      },
>>>      "signature_algorithms (13)": {
>>>        "signature schemes": [rsa_pss_rsae_sha256, 
>>> ecdsa_secp256r1_sha256, ed25519, rsa_pss_rsae_sha384, 
>>> rsa_pss_rsae_sha512, rsa_pkcs1_sha256, rsa_pkcs1_sha384, 
>>> rsa_pkcs1_sha512, ecdsa_secp384r1_sha384, ecdsa_secp521r1_sha512, 
>>> rsa_pkcs1_sha1, ecdsa_sha1]
>>>      },
>>>      "unknown extension (47)": {
>>>        0000: 00 29 00 27 30 25 31 10   30 0E 06 03 55 04 0A 0C  
>>> .).'0%1.0...U...
>>>        0010: 07 54 65 73 74 50 4B 49   31 11 30 0F 06 03 55 04  
>>> .TestPKI1.0...U.
>>>        0020: 03 0C 08 54 65 73 74 52   6F 6F 74                 
>>> ...TestRoot
>>>      }
>>>    ]
>>> }
>>> )
>>>
>>> In the debug logs, it seems like we shouldn't call a status_request 
>>> extension an "unknown extension" because we know what it is and can 
>>> parse it.  Having the entry in SSLExtension for CR_STATUS_REQUEST and 
>>> a simple do-nothing consumer gets rid of the status_request/unknown 
>>> messages and has it render in the printing of the CR message properly.
>>>
>>> While we're talking about the debug logs we have other extensions 
>>> that we know about and have entries in SSLExtension, but they have no 
>>> per-message registration.  You can see in the CR above that 18 
>>> (signed_certificate_timestamp) and 47 (certificate_authorities) are 
>>> extensions that we were aware of enough to put basic entries in 
>>> SSLExtension for, but just didn't put full consumer/producer support 
>>> in there.
>>>
>>> It might be nice to use the display approach that we have for an 
>>> unsupported extension for those extensions we at least know about, 
>>> but rather than say "unknown extension" at the start of the printing 
>>> at least give the name.  Let me see if I can make that work without 
>>> it being too invasive.
>>>
>>> --Jamil
>>>
>>>> On 1/4/20 9:27 AM, Xuelei Fan wrote:
>>>> CertStatusExtension.java
>>>> ------------------------
>>>> 1239             // The consuming happens in server side only.
>>>> typo? server -> client
>>>>
>>>> It wold be nice to add a debug log that this extension get ignored.  
>>>> But may not need to define this consumer as it is not supported.
>>>>
>>>> SSLExtension.java
>>>> -----------------
>>>> As this fix does not implement this feature yet, is it possible to 
>>>> just define it without the on-load consumer?
>>>>
>>>> Otherwise, it looks fine to me.
>>>>
>>>> BTW, this issue reminders me a common problem: if an extension is 
>>>> supported in a handshake message, we need to know all other 
>>>> handshake messages that could use the extension, and define an 
>>>> SSLExtension enum for them.  Otherwise, a similar issue could 
>>>> happen.  I think it would be challenge to know that in practice.
>>>>
>>>> So I was just wondering, could we just release the check in the 
>>>> SSLExtensions.java implementation (from line 71 to 95)?  If the 
>>>> extension for the specific handshake type is not defined, just 
>>>> ignore it, except for ServerHello?  I think the behavior complies to 
>>>> the TLS 1.3 protocol.
>>>>
>>>> if (SSLExtension.isConsumable(extId) &&
>>>>      SSLExtension.valueOf(handshakeType, extId) == null) {
>>>>      ...
>>>> -   } else {
>>>> +   } else if (handshakeType == SSLHandshake.SERVER_HELLO) {
>>>>          throw hm.handshakeContext.conContext.fatal(
>>>>                Alert.UNSUPPORTED_EXTENSION,
>>>>                "extension (" + extId +
>>>>                ") should not be presented in " + handshakeType.name);
>>>> +   } else {
>>>> +       isSupported = false;
>>>> +       // debug log to ignore unknown extension for handshakeType
>>>>      }
>>>> }
>>>>
>>>> Xuelei
>>>>
>>>>> On 1/3/2020 10:06 AM, Jamil Nimeh wrote:
>>>>> Hi All, the golang folks have been running into an issue where our 
>>>>> JSSE client treats the status_request extension in a 
>>>>> CertificateRequest message from a golang server as an unknown 
>>>>> extension and alerts.  This quick fix will allow the client to read 
>>>>> and accept the extension and proceed.  I believe you need golang 
>>>>> 1.13.x to see this take place.
>>>>>
>>>>> This fix does not implement client-side OCSP stapling.  That will 
>>>>> be an RFE for another day.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8236039
>>>>>
>>>>> Webrev: https://cr.openjdk.java.net/~jnimeh/reviews/8236039/webrev.01/
>>>>>
>>>>> --Jamil
>>>>>