RFR: JEP 249 (OCSP Stapling for TLS)
Jamil Nimeh
jamil.j.nimeh at oracle.com
Tue Jun 23 07:04:23 UTC 2015
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
>>
>>
More information about the security-dev
mailing list