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