On 8/20/19 12:14 PM, Jamil Nimeh wrote:
Thanks for the review, Sean. I've made a couple changes based on your
recommendations. I don't create a mutable ArrayList any longer in the
case where we're making a new PKIXRevocationChecker. In that one case I
just directly add it to the PKIXBuilderParameters and if one already
exists from the immutable CPC list I can just add the responses to it
and push the whole list back.
http://cr.openjdk.java.net/~jnimeh/reviews/8225436/webrev.02/
406 // Make a modifiable copy of the CertPathChecker list
This comment is not true anymore, so I would delete it.
Looks good otherwise.
--Sean
--Jamil
On 8/19/19 12:55 PM, Sean Mullan wrote:
Looks good. There is one case where an unnecessary ArrayList is
created (on line 406-7) if revocation is disabled AND we don't find a
RevocationChecker -- it would be useful if you could avoid that by
iterating over the checkers before creating the array list, since it
is probably the more common case. In fact, there is probably some
unnecessary copying even before that since the PKIXParameters has
already been cloned in engineValidate. We avoid some of that
cloning/copying in the certpath provider (see the ValidatorParams
class in sun.security.provider.certpath.PKIX) - you could look into
that to see if you could use that here - I'll also think about it
again when I have a little more time ...
--Sean
On 8/16/19 5:25 PM, Jamil Nimeh wrote:
Hello all,
This fixes a bug where stapled OCSP responses were being ignored by
the internal Validator in all cases when revocation checking is
disabled. If the TrustManagerFactory is initialized with
CertPathParameters that include a PKIXRevocationChecker, then that
should override the setRevocationEnabled flag and any stapled
responses should be taken into account during path validation.
Bug: https://bugs.openjdk.java.net/browse/JDK-8225436
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8225436/webrev.01/
Thanks,
--Jamil
