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

[Xuelei Fan]

src/java.base/share/classes/sun/security/ssl/ClientHandshaker.java
==================================================================
minor comment:

1603         if (!staplingActive) {
1604             fatalSE(Alerts.alert_unexpected_message,
1605                     "Unexpected CertificateStatus message");

This block may be redundant.  If staplingActive is not true, I think
certificateStatus() is unlikely get called.

-----------------
minor comment:

    checkServerCerts(deferredCerts);
    session.setPeerCertificates(deferredCerts);

The above two method are always called together.  Is it a little bit
friendly to merge them together?
    private void checkServerCerts(X509Certificate[] certs) ... {
        ...
        session.setPeerCertificates(certs);
    }

src/java.base/share/classes/sun/security/ssl/X509TrustManagerImpl.java
======================================================================
You may want to remove the block of addResponses() implementation.

src/java.base/share/classes/sun/security/validator/PKIXValidator.java
=====================================================================
minor comment:

Is it more instinctive if changing the parameter name from responseList
to ocspResponses, and the method name from addResponses() to
addOcspResponses()?

Same for SimpleValidator.java and Validator.java.

----------------
line 393-404.

For performance friendly, it is nice to construct the map if and only if
the map get used.

    if (pkixParams.isRevocationEnabled()) {
       // make the map

       try {
           ...  // use the map
       }
    }

-----------
 430   revChecker.setOcspResponses(responseMap);
User specified OCSP responses are overridden, which should be respected.
 The behavior is not what the user expected.

src/java.base/share/classes/sun/security/validator/Validator.java
=====================================================================
 207     public final String getType() {

Looks like this method does not get used. Is it redundant?


Xuelei

On 6/27/2015 11:06 PM, 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
> 
> 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
>>
>>
>