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