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
