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

[Sean Mullan]

Hi Jamil,

On 06/27/2015 11:06 AM, Jamil Nimeh wrote:
> Hello all, I've posted an updated webrev based on comments I've received
> so far:
>
> http://cr.openjdk.java.net/~jnimeh/reviews/8046321/webrev.1

I didn't have time to review the tests yet but was able to review most 
of the code in the other files, except for StatusResponseManager which I 
believe has been reviewed already. Here are my comments ...

General comments:

- the text after an @param or @throws tag generally starts with a 
lower-case letter, ex: "the ...". I noticed you started many of these 
with a capital letter. Also some of the {@link}s to common classes like 
String and List are probably unnecessary esp. for javadoc of internal 
methods.

- I'm not sure if this is the case, but you can generally avoid cloning 
or making copies of parameters if the usage is internal and they don't 
cross the supported API boundary. There may be some unnecessary clones 
of the encoded OCSP response bytes.

- For new classes and methods, don't make them public unless they are 
needed outside of the package. Make them private or package-private, if 
called by other classes in the same package. I'm not sure if this is the 
case, but just a general comment/thing to check.

* ExtendedSSLSession

120: "Obtains" sounds like invoking this method causes a network request 
to get it. Suggest "Returns" instead.

* ServerHandshaker

59: can you add what time unit this is to the comment?

* X509TrustManagerImpl

addResponses looks like it isn't called by anything and can be removed.

--Sean

>
> Thanks,
> --Jamil
>
> On 06/18/2015 05:27 PM, Jamil Nimeh wrote:
>> Hello all,
>>
>> I have a first cut at the OCSP stapling webrev posted for your review:
>>
>> JEP: https://bugs.openjdk.java.net/browse/JDK-8046321
>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8046321/webrev.0/
>>
>> A couple items to note:
>>
>>   * I'm in the process of updating the JEP with some more details.  I
>>     should be done with these changes by tonight (PDT).
>>   * Missing are some of the TLS end-to-end tests.  These tests have
>>     been coded and run outside the jtreg framework, but for some
>>     reason things hang in jtreg.  I've included some of the supporting
>>     classes that these tests will use (CertificateBuilder.java and
>>     SimpleOCSPResponder.java) so folks could review those if they're
>>     interested.  I will update the webrev and notify the list as soon
>>     as I've got the tests working in jtreg.
>>
>> Thanks to everyone who has helped along the way.
>>
>> --Jamil
>>
>>
>