On 06/30/2015 06:04 PM, Xuelei Fan wrote:
On 7/1/2015 6:39 AM, Jamil Nimeh wrote:
src/java.base/share/classes/sun/security/ssl/HandshakeMessage.java
==================================================================
line 713/714, 730/731 throws SSLHandshakeException for extension
constructor in server side. That's unlikely to happen, I think. I was
wondering, if CertificateStatus cannot be constructed, the server may
not want to send the message, rather than terminate the connection
immediately.
I think you're right. While the exception is unlikely, I'd like to have
the HandshakeMessage throw the exception if something bad happens. I do
however, agree that we shouldn't make it a fatal error. I'll catch the
exception in ServerHandshaker, log it, and just not send the message as
you suggested since that is legal. OK?
I have not read the server side implementation. I would like firstly
check whether the message should be delivered, and than new the
instance. Exception catching is not performance friendly, and looks a
little bit not-straightforward. I think you may want a static method
for the validity checking in CertificateStatus class, instead.
As it is written today, the ServerHandshaker will only create a new
CertificateStatus instance if the following is true:
* Stapling has already been activated in the server (meaning that the
client requested it and the server has the feature enabled)
* A "get" operation was done from the StatusResponseManager and at
least one OCSP response was returned. In other words, if no
responses can be brought back from the SRM, then there's no point in
even asserting status_request[_v2] in the ServerHello.
I don't see the advantage of making a static method that does what is
already accomplished in two lines of code in ServerHandshaker (873-4).
It's OK to throw exception if something bad happens. For easy reading,
please have a comment that it is unlikely to happen if you keep the
throw exception blocks.
Okay, I can definitely do that. There are some cases where I think we
need to throw an exception, particularly on the constructor from a
HandshakeInStream. That's a case where we want the client to Alert if
the server asserts some weird/unsupported/illegal type or does something
like type = ocsp and a zero length response (also illegal according to
the spec). Zero length responses are OK for ocsp_multi, though.
