Looks fine.
Not as part of this fix, but as a future perf. improvement we should
intern (see X509Factory.intern) the X509CertImpl objects that are
created from the contents of the OCSP response, so that they are put in
the certificate memory cache. If the same OCSP responder is used
frequently, then these X509Certificate objects won't be created each
time the OCSP response is validated.
--Sean
On 12/10/2013 12:30 PM, Vincent Ryan wrote:
Thanks for your reviews. I’ve made a minor change to include a message in
the CPVE, as suggested by Max.
% hg diff OCSPResponse.java
diff --git
a/src/share/classes/sun/security/provider/certpath/OCSPResponse.java
b/src/share/classes/sun/security/provider/certpath/OCSPResponse.java
--- a/src/share/classes/sun/security/provider/certpath/OCSPResponse.java
+++ b/src/share/classes/sun/security/provider/certpath/OCSPResponse.java
@@ -427,9 +427,14 @@ public final class OCSPResponse {
if (signerCert == null) {
// Add the Issuing CA cert and/or Trusted Responder cert
to the list
// of certs from the OCSP response
- certs.add((X509CertImpl) issuerCert);
- if (responderCert != null) {
- certs.add((X509CertImpl) responderCert);
+ try {
+ certs.add(X509CertImpl.toImpl(issuerCert));
+ if (responderCert != null) {
+ certs.add(X509CertImpl.toImpl(responderCert));
+ }
+ } catch (CertificateException ce) {
+ throw new CertPathValidatorException(
+ "Invalid issuer or trusted responder certificate", ce);
}
if (responderName != null) {
On 10 Dec 2013, at 01:44, Weijun Wang <weijun.w...@oracle.com
<mailto:weijun.w...@oracle.com>> wrote:
It looks good. Would you like to add a string message?
Thanks
Max
On 12/10/13, 9:47, Jason Uh wrote:
Hi Vinnie,
The change looks good to me.
Jason
(Not an official Reviewer)
On 12/9/13 3:25 PM, Vincent Ryan wrote:
Please review this fix to the OCSPResponse class in the internal
sun.security.provider.certpath package. Previously, when validating
an OCSP response, it expected the supplied issuer and/or trusted
responder X509 certs to already be in an internal format used by
the package. Now it accepts certs in any subclass of X509Certificate
and will convert to the internal format, if necessary.
Bug: https://bugs.openjdk.java.net/browse/JDK-8029788
Webrev: http://cr.openjdk.java.net/~vinnie/8029788/webrev.00/
This fixes a regression introduced by JDK-8015571.
Thanks.
