RFR: JEP 249 (OCSP Stapling for TLS)

[Jamil Nimeh]

Hi Xuelei, thanks for the comments.  Keep 'em coming!

On 06/22/2015 08:26 PM, Xuelei Fan wrote:
> src/java.base/share/classes/sun/security/ssl/HandshakeStateManager.java
> =======================================================================
> Thanks for the correction of typos, etc.
>
> line 777-797.
> Mayber, we can use the "default" block at line 857, and may not need the
> block from line 777 to 797.  What do you think?
I think that would work.  I'll remove the cert status case and see what 
happens.
>
> src/java.base/share/classes/sun/security/ssl/ClientHandshaker.java
> ==================================================================
> -  private final boolean enableStatusRequestExtension =
> +  private final static boolean enableStatusRequestExtension =
> May not want to support dynamic system property.
Actually I did want it to take that value of the property at 
instantiation time so people could selectively turn it on and off before 
creating sockets/engines.  What concerns do you have about it being dynamic?
>
> ---------------------------
>
> - private byte setCertValidationFailure(CertificateException cexc)
> + private byte getCerticateAlert(CertificateException cexc)
> Looks more like a getter method than a setter method.
I guess it's a little of both.  But I have no problem with changing the 
name.
>
> ---------------------------
>
> - private void checkServerCerts(X509Certificate[] certs)
> -        throws CertificateException {
> + private void checkServerCerts(X509Certificate[] certs)
> +        throws IOException {
>
> The caller is always handle the thrown exception with the same code:
>
>     try {
>         checkServerCerts(deferredCerts);
>     } catch (CertificateException e) {
>         // This will throw an exception, so include the
>         // original error.
>         fatalSE(setCertValidationFailure(e), e);
>     }
>
> What do you think if moving the code into checkServerCerts() and
> throwing IOException?
That would be fine.  At the time I wrote it, I wasn't sure if I'd be 
doing the same thing as a consequence of checkServerCerts throwing a 
CPVE.  But it turned out to be the same in all cases and I never went 
back to clean that up.  Good catch.  I'll get this fixed up.
>
> ---------------------------
>
>    if (staplingActive && ignoredOptStates.contains(
>        ...
>    }
>
> The same code block is used in several places in processMessage().  I
> understand the point.  However, this code block need to be inserted into
> every message of the server hello series messages, or need careful
> analysis of the message sequence.  As may not quite friendly for
> maintaining.
>
> What do you think if moving the block to the header of processMessage()?
>
> void processMessage(byte type, int messageLen) throws IOException {
>     // check the handshake state
>     List<Byte> ignoredOptStates = handshakeState.check(type);
> +  if (staplingActive && ignoredOptStates.contains(
> +     ...
> +  }
I know.  I didn't like the duplication either, but I wasn't sure if I 
could just call it at the top safely so I instead moved it into each 
state transition from Server Certificate.  If we can do it once at the 
top like I did in the pre-DTLS code, then that's by far the better way 
to go.  I'll try that out.
> ---------------------------
>
>   753  // Only enable the stapling feature if the client asserted
>   754  // these extensions.
>   755  if (enableStatusRequestExtension) {
>   756      staplingActive = true;
> +     } else {
> +         fatalSE(Alerts.alert_unexpected_message, "...");
>   757  }
>
> Maybe, better to terminate the connection with a fatal
> unexpected_message if the extension is not requested.
Yes, I think you're right.  I'll fix this up too.
>
> Xuelei
>
>
> On 6/19/2015 8:27 AM, 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
>>
>>