RFR[15] 8242060: Add revocation checking to jarsigner

[Hai-May Chao]

> On May 4, 2020, at 6:01 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
> 
> 
> 
>> On May 5, 2020, at 3:48 AM, Hai-May Chao <hai-may.chao at oracle.com> wrote:
>> 
>> Hi Max,
>> 
>>> On May 2, 2020, at 5:25 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
>>> 
>>> In jarsigner/Main, you can just call
>>> 
>>> pkixParameters.setRevocationEnabled(revocationCheck);
>> 
>> Ok
>> 
>>> 
>>> Last time, you mentioned that "Event.clearReportListener()" cannot be called immediately after validation check because of some race conditions. Is that easily reproducible? I still find it strange.
>> 
>> It is easily reproducible. I should have better explained why the suggested changes did not work but not due to race condition. The code pkixParameters.setRevocationEnabled(revocationCheck) only sets the revocation checker enabled. It is the validateCertChain() in Main.java, which calls into RevocationChecker.check() and ultimately calls Event.report() prior to making OCSP and CRL connections. If we call clearReportListener in loadKeyStore() method (i.e finally{ }as suggested), for OCSP, by the time when OCSP.getOCSPBytes() comes in to report the OCSP event, the reporter has been cleared. And this would be same problem for CRL. So it cannot be called immediately.
> 
> Then I assume the following is OK.
> 
> try {
>   setReportListener();
>   validator.validate();
> } finally {
>   clearReportListener();
> }
> 

I’d like to know if there is an issue with the current webrev, which is being tested and it’s working as expected. I’ve also tried with your previous suggested change, and it didn’t work.

Thanks,
Hai-May


> Thanks,
> Max
> 
>> 
>> Thanks,
>> Hai-May
>> 
>>> 
>>> Thanks,
>>> Max
>>> 
>>>> On May 3, 2020, at 2:19 AM, Hai-May Chao <hai-may.chao at oracle.com> wrote:
>>>> 
>>>> Hi Sean,
>>>> 
>>>> Thanks for your review. Reply inline.
>>>> 
>>>>> On May 1, 2020, at 11:50 AM, Sean Mullan <sean.mullan at oracle.com> wrote:
>>>>> 
>>>>> * Main.java:
>>>>> 
>>>>> 2067                         Event.setReportListener(new Event.Reporter() {
>>>>> 2068                             @Override
>>>>> 2069                             public void handle(String t, Object... o) {
>>>>> 2070 System.out.println(String.format(rb.getString(t), o));
>>>>> 2071                             }
>>>>> 2072                         });
>>>>> 
>>>>> I think you could use a lambda expression above.
>>>> 
>>>> Done.
>>>> 
>>>>> 
>>>>> * Event.java:
>>>>> 
>>>>> 35     static Reporter reporter = null;
>>>>> 
>>>>> Make this private? Also, no need to explicitly initialize to null as that is the default value.
>>>> 
>>>> Done (made it private).
>>>> 
>>>>> 
>>>>> Can you add some comments at the top of the class describing the purpose of this class?
>>>> 
>>>> Done.
>>>> 
>>>>> 
>>>>> * EnableRevocation.java
>>>>> 
>>>>> - How long does this test take - does it hang for a little while trying to make a connection or timeout right away? If it takes a while, you could experiment with overriding the default timeouts for CRLs and OCSP checks to make this test finish faster. Use the system properties com.sun.security.ocsp.timeout and com.sun.security.crl.readtimeout.
>>>> 
>>>> It does not hang for OCSP as the certificate is set with localhost as OCSP responder. It hangs just a little for CRL, thus I’ve changed the certificate to local host instead of remote host. The whole test is finishing very fast now.
>>>> 
>>>>> 
>>>>> Looks good otherwise. Please add a release-note and open a follow-on issue to update the man page with the new option.
>>>> 
>>>> Done (Release note: JDK-8244285, and man page: JDK-8244274).
>>>> 
>>>> Updated webrev:
>>>> https://cr.openjdk.java.net/~hchao/8242060/webrev.02/
>>>> 
>>>> Thanks,
>>>> Hai-May
>>>> 
>>>> 
>>>>> 
>>>>> --Sean
>>>>> 
>>>>> On 5/1/20 12:02 PM, Hai-May Chao wrote:
>>>>>> Hi,
>>>>>> With small change added to ‘Usages.java' test, here is the updated webrev:
>>>>>> https://cr.openjdk.java.net/~hchao/8242060/webrev.01/
>>>>>> Thanks,
>>>>>> Hai-May
>>>>>>> On Apr 30, 2020, at 4:29 PM, Hai-May Chao <hai-may.chao at oracle.com> wrote:
>>>>>>> 
>>>>>>> Hi,
>>>>>>> 
>>>>>>> I’d like to request a review for:
>>>>>>> 
>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8242060
>>>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8244046
>>>>>>> Webrev: https://cr.openjdk.java.net/~hchao/8242060/webrev.00/
>>>>>>> 
>>>>>>> The jarsigner command currently does certificate chain validation, but does not check revocation. Users won’t be able to know if the certificates are revoked. This change is to provide an option in jarsigner to enable the revocation check, and to emit progress messages when jarsigner starts network connections to get OCSP responses and CRL.
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Hai-May
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>> 
>>> 
>> 
>