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

Sat, 04 Jan 2020 15:54:21 -0800

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 <[email protected]> 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























					Reply via email to