[RFR] JDK-8225436 : Stapled OCSPResponses should be added to PKIXRevocationChecker irrespective of revocationEnabled flag

[Sean Mullan]

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
>>>