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 <[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
