[Update]: JEP 249 (OCSP Stapling for TLS)

[Jamil Nimeh]

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