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

[Xuelei Fan]

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