RFR: JEP 249 (OCSP Stapling for TLS)

[Xuelei Fan]

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?


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.

---------------------------

- private byte setCertValidationFailure(CertificateException cexc)
+ private byte getCerticateAlert(CertificateException cexc)
Looks more like a getter method than a setter method.

---------------------------

- 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?

---------------------------

  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(
+     ...
+  }

---------------------------

 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.


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