On 07/18/2015 04:56 PM, Xuelei Fan wrote:
In the new webrev, you try the approach to throw exceptions
CertificateStatus constructor and catch it in ServerHandshaker. It is a
kind a abuse of SSLHandshakeException. I would like to make the
checking before construct CertificateStatus in ServerHandshaker. It's
really hard to understand the code. When I find a
SSLHandshakeException, firstly, I think the connection would be
terminated. But actually the handshaking can continue. It's not easy
to understand the code.
What you're saying makes sense. I can make that change. If it is a
quick change I'll get it out before the weekend is over, otherwise I'll
file the bug as you mentioned.
You can file a new bug and make the update later.
Xuelei
On 7/1/2015 10:42 AM, Xuelei Fan wrote:
On 7/1/2015 10:02 AM, Jamil Nimeh wrote:
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).
Good you can accomplish it in ServerHandshaker.
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.
It's the expected behavior to throw exception for reading issues. What
we are talking about previously is for write side constructor.
Thanks,
Xuelei
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.
