RFR JDK-8236039: JSSE Client does not accept status_request extension in CertificateRequest messages for TLS 1.3
Jamil Nimeh
jamil.j.nimeh at oracle.com
Mon Jan 6 05:01:37 UTC 2020
I don't think we need it. We're already getting a similar message as
things stand now. The existing message doesn't correlate the extension
with the message, but you can figure out which message is being
processed by looking at where it falls in the trace.
I'll push this as-is. Thanks for the review and the improvements to the
overall fix.
--Jamil
On 1/5/2020 6:45 PM, Xuelei Fan wrote:
> Hi Jamil,
>
> Thanks for the new update, which looks pretty good to me.
>
> If have something (like SSLLogger.debug()) for line 96 could mitigate
> your worries about the debug log?
>
> SSLExtensions.java:
> - 96 // debug log to ignore unknown extension for handshakeType
> + if (SSLLogger.isOn && ...) {
> + SSLLogger.debug("Ignore unknown or unsupported extension (" +
> + SSLExtension.nameOf(extId) + ") in " +
> + SSLHandshake.nameOf(handshakeType));
> + }
>
> Thanks,
> Xuelei
>
> On 1/5/2020 2:20 PM, Jamil Nimeh wrote:
>> Hi Xuelei, et al.,
>>
>> I have a new version of the webrev here:
>>
>> https://cr.openjdk.java.net/~jnimeh/reviews/8236039/webrev.02
>>
>> I thought a little more about what you said about consumers and tried
>> looking at the logging both with and without the CR consumer in
>> place. With your changes to the SSLExtensions constructor, combined
>> with the new SSLExtension.nameOf(int), I think we can not have the
>> consumer and the resulting logs look good. You still get a warning
>> when processing a CR status_request, but now an "unknown or
>> unsupported" message that identifies "status_request (5)" makes more
>> sense. The status_request extension is identified by name now but
>> unsupported in the CR message which is accurate, but no consumer
>> overhead at all, debug or not. Best of both worlds.
>>
>> When we get to doing the client-side OCSP stapling feature, then
>> we'll need server side producers and client-side consumers. But all
>> that fun is for a later date.
>>
>> Thanks,
>> --Jamil
>>
>>
>> On 1/4/20 6:41 PM, Xuelei Fan wrote:
>>> 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
>>>>>>>>
More information about the security-dev
mailing list