On 06/29/2015 10:29 PM, Xuelei Fan wrote:
src/java.base/share/classes/sun/security/ssl/ClientHandshaker.java
==================================================================
minor comment:
1603 if (!staplingActive) {
1604 fatalSE(Alerts.alert_unexpected_message,
1605 "Unexpected CertificateStatus message");
This block may be redundant. If staplingActive is not true, I think
certificateStatus() is unlikely get called.
I think you're right. I think the HandshakeStateManager will yell if
CertificateStatus is sent when status_request isn't set by both
endpoints. I'll remove this check.
-----------------
minor comment:
checkServerCerts(deferredCerts);
session.setPeerCertificates(deferredCerts);
The above two method are always called together. Is it a little bit
friendly to merge them together?
private void checkServerCerts(X509Certificate[] certs) ... {
...
session.setPeerCertificates(certs);
}
Done.
src/java.base/share/classes/sun/security/ssl/X509TrustManagerImpl.java
======================================================================
You may want to remove the block of addResponses() implementation.
Gone!
src/java.base/share/classes/sun/security/validator/PKIXValidator.java
=====================================================================
minor comment:
Is it more instinctive if changing the parameter name from responseList
to ocspResponses, and the method name from addResponses() to
addOcspResponses()?
Same for SimpleValidator.java and Validator.java.
I've tried to not use "ocsp" in the names, only because OCSP is just one
type of stapled response for certificate revocation status. Granted, it
is the only one used today. I didn't want to use a term that denoted
that the only kind of data coming through CertificateStatus is OCSP
data, since in the future it may be something different. I know there
are places where I didn't adhere to my own rule, but I really tried to
where I could.
----------------
line 393-404.
For performance friendly, it is nice to construct the map if and only if
the map get used.
if (pkixParams.isRevocationEnabled()) {
// make the map
try {
... // use the map
}
}
-----------
430 revChecker.setOcspResponses(responseMap);
User specified OCSP responses are overridden, which should be respected.
The behavior is not what the user expected.
Good catch. I'll rework the Map instantiation and fix this.
src/java.base/share/classes/sun/security/validator/Validator.java
=====================================================================
207 public final String getType() {
Looks like this method does not get used. Is it redundant?
It's gone now. I was using it when I did the ThreadLocal solution and
forgot to rip it out when we moved the logic into PKIXValidator.
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
