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

[Xuelei Fan]

sun/security/ssl/ServerHandshaker.java
======================================
OCSP stapling only used for certificate-based server authentication at
present.  I was wondering,  may be better to make a check before wrap
the ServerHello OCSP extension and CertificateStatus message that
Certificate message would be actually used.

-------------------
 860    for (CertStatusReqItemV2 csriv2 :
 861        statReqExtV2.getRequestItems()) {
 862        // Favor OCSP_MULTI over OCSP
 863        statReqType = csriv2.getType();
 864        statReqData = csriv2.getRequest();
 865        if (statReqType == StatusRequestType.OCSP_MULTI) {
 866            break;
 867        }
 868     }

Client's preference (favorite choice first) should be respected. For
type OCSP, line 863-864 prefer the last item rather than the client
preference.  And line 863-864 does not consider whether the
OCSPStatusRequest is suitable for this server or not, just select the
last one.  As may result in ocsp stapling failure if the selected item
is not for the right responder.

Line 865-867 does not consider whether the OCSPStatusRequest is suitable
for this server or not, either.

In general, client knows nothing about server certificates.  So client
would offer responders it trusted.  The responders is not necessary
suitable for the target server.  The server side should select a
suitable one.  It is not necessary the first one or the last one.  See
also section 2.2, RFC 6961:

  "Servers that support a client's selection of responders using the
   ResponderID field could implement this selection by matching the
   responder ID values from the client's list with the ResponderIDs of
   known OCSP responders, either by using a binary compare of the values
   or a hash calculation and compare method."

If the update of your implementation is pretty significant or it is
unclean how we should move forward at present, I'm OK if you want to
remove the support of client specified responders.

sun/security/ssl/StatusResponseManager.java
===========================================
line 207-213:

// It is assumed that the caller has ordered the certs in the chain
// from end-entity to trust anchor.  In order to create the CertId
// necessary for OCSP requests the subject and issuer certs must
// be present.
- if (chain.length < 2) {
+ if (chain.length == 0) {
    return Collections.emptyMap();
}

This assumption is not actually correct in general.  The trust anchor is
not necessarily be included in the Certificate message.  See page 48,
section 7.4.2, RFC 5246.

----------------------------
 436         List<ResponderId> responderIds;

You don't actually use the client specified responders, do you?  It's OK
to me if we don't support none-empty OCSPStatusRequest.  If we do not
want to do that, I would suggest to remove the related implementation
code for that.  As would simplify the job, I think.

-----------------------------
 252    // Set a bunch of threads to go do the fetching
 253    List<Future<StatusInfo>> resultList =
 254        threadMgr.invokeAll(requestList, delay, unit);

The current implementation of OCSP response getter, looks like for every
cert in the chain, a fetch thread would be created to get the
corresponding OCSP response.  In the fetch thread, if the response is
cached, use it; otherwise, connect to OCSP server and fetch it.

Performance would be a very big concern.  A connection may kick start
many threads.  Fetch threads may be a bottleneck of the server. In
general, a server may be expected to serve 10K+ connections, but may
only be able to hit around 10K- threads.  It's likely to face DoS
challenges if every ClientHello trigger 1+ fetch threads.

Another performance concern is that when there is no cache, the client
would have to wait for the server to fetch the response from OCSP
servers.  It's easy to run into client timeout issue.

I may suggest to use a single thread to get responses from OCSP servers.
 And any TLS connection would only get responses from cache.  The
scenarios may looks like:
1. Per every SSLContext instance, create a scheduled single thread to
fetch the response from OCSP servers, and cache them for each server
certificate chains.  The server certificates can be got from key manager.
2. For every client request, look for the response from the cache.
3. if responder is not cached and if we want to support none-empty
OCSPStatusRequest, fetch the response from OCSP servers in the same
thread, and cache the result.  We may also want to add the task to the
scheduled thread in #1.  Not create new thread for every connection is
important for SSLEngine in some circumstances.

#3 is also not performance friendly.  I think none-empty
OCSPStatusRequest is not common in practice.


I will stop here for the first round code review.  Looking forward for
the next round webrev.

Thanks,
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
>>
>>
>