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

[Jamil Nimeh]

Whoops...forgot to reply-all.

On 1/4/2020 10:02 AM, Jamil Nimeh wrote:
> Thanks for catching the comment typo.  I'll fix that.
>
> My original fix was just the definition in SSLExtension with all null 
> consumer/producers.  That will work, but you get a warning when debug 
> tracing is turned on that is spurrious:
>
> SSLExtensions.java:132|Ignore unknown or unsupported extension (
> "unknown extension (5)": {
>
> }
>
> I didn't really like that "unknown extension" log message when we 
> clearly know what status_request is in general.  When I went through 
> and traced it, it basically falls into that code path because there's 
> no onLoadConsumer.  So I just made a very simple onLoad that would 
> parse the extension and nothing else.  I figured it would serve as the 
> springboard for client-side OCSP stapling if we ever decide to do it.
>
> Regarding your comment about defining extensions for all messages or 
> none at all, I noticed the same thing and came to a similar conclusion 
> when I was debugging that spurious debug message above.  The golang 
> server asserts signed_certificate_timestamp which it was happy to 
> treat as an unknown extension while SCT caused the client to alert.
>
> I'll try adding your check and commenting out my consumer to see if a) 
> it allows the extension, b) it doesn't throw a bogus debug message.
>
> --Jamil
>
> On 1/4/2020 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
>>>