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

[Jamil Nimeh]

On 06/29/2015 10:29 PM, Xuelei Fan wrote:
> 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.
I think you're right.  I think the HandshakeStateManager will yell if 
CertificateStatus is sent when status_request isn't set by both 
endpoints.  I'll remove this check.
>
> -----------------
> 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);
>      }
Done.
>
> src/java.base/share/classes/sun/security/ssl/X509TrustManagerImpl.java
> ======================================================================
> You may want to remove the block of addResponses() implementation.
Gone!
>
> 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.
I've tried to not use "ocsp" in the names, only because OCSP is just one 
type of stapled response for certificate revocation status. Granted, it 
is the only one used today.  I didn't want to use a term that denoted 
that the only kind of data coming through CertificateStatus is OCSP 
data, since in the future it may be something different.  I know there 
are places where I didn't adhere to my own rule, but I really tried to 
where I could.
>
> ----------------
> 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.
Good catch.  I'll rework the Map instantiation and fix this.
>
> 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?
It's gone now.  I was using it when I did the ThreadLocal solution and 
forgot to rip it out when we moved the logic into PKIXValidator.
>
>
> 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
>>>
>>>