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
