Re: RFR[15] 8242060: Add revocation checking to jarsigner
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 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 > > >
Re: RFR[15] 8242060: Add revocation checking to jarsigner
* 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.
* Event.java:
35 static Reporter reporter = null;
Make this private? Also, no need to explicitly initialize to null as
that is the default value.
Can you add some comments at the top of the class describing the purpose
of this class?
* 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.
Looks good otherwise. Please add a release-note and open a follow-on
issue to update the man page with the new option.
--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 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
Re: RFR[15] 8242060: Add revocation checking to jarsigner
> > * 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. What if we use 0.0.0.0 for both OCSP and CRLDP? I assume it will return immediately, just hope it's not an uncaught RuntimeException. --Max > > Looks good otherwise. Please add a release-note and open a follow-on issue to > update the man page with the new option. > > --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 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 >>> >>> >>>
Re: RFR[15] 8242060: Add revocation checking to jarsigner
Hi Sean,
Thanks for your review. Reply inline.
> On May 1, 2020, at 11:50 AM, Sean Mullan 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 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
>>>
>>>
>>>
Re: RFR[15] 8242060: Add revocation checking to jarsigner
Thanks Max! Using localhost would do it as well. Hai-May > On May 1, 2020, at 7:49 PM, Weijun Wang wrote: > >> >> * 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. > > What if we use 0.0.0.0 for both OCSP and CRLDP? I assume it will return > immediately, just hope it's not an uncaught RuntimeException. > > --Max > >> >> Looks good otherwise. Please add a release-note and open a follow-on issue >> to update the man page with the new option. >> >> --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 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 >
Re: RFR[15] 8242060: Add revocation checking to jarsigner
In jarsigner/Main, you can just call
pkixParameters.setRevocationEnabled(revocationCheck);
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.
Thanks,
Max
> On May 3, 2020, at 2:19 AM, Hai-May Chao wrote:
>
> Hi Sean,
>
> Thanks for your review. Reply inline.
>
>> On May 1, 2020, at 11:50 AM, Sean Mullan 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 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
>
Re: RFR[15] 8242060: Add revocation checking to jarsigner
Hi Max,
> On May 2, 2020, at 5:25 PM, Weijun Wang 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.
Thanks,
Hai-May
>
> Thanks,
> Max
>
>> On May 3, 2020, at 2:19 AM, Hai-May Chao wrote:
>>
>> Hi Sean,
>>
>> Thanks for your review. Reply inline.
>>
>>> On May 1, 2020, at 11:50 AM, Sean Mullan 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 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
>
>
>
>>
>
Re: RFR[15] 8242060: Add revocation checking to jarsigner
> On May 5, 2020, at 3:48 AM, Hai-May Chao wrote:
>
> Hi Max,
>
>> On May 2, 2020, at 5:25 PM, Weijun Wang 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();
}
Thanks,
Max
>
> Thanks,
> Hai-May
>
>>
>> Thanks,
>> Max
>>
>>> On May 3, 2020, at 2:19 AM, Hai-May Chao wrote:
>>>
>>> Hi Sean,
>>>
>>> Thanks for your review. Reply inline.
>>>
On May 1, 2020, at 11:50 AM, Sean Mullan 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
>> 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
>>
>>
>>
>>>
>>
>
Re: RFR[15] 8242060: Add revocation checking to jarsigner
> On May 4, 2020, at 6:01 PM, Weijun Wang wrote:
>
>
>
>> On May 5, 2020, at 3:48 AM, Hai-May Chao wrote:
>>
>> Hi Max,
>>
>>> On May 2, 2020, at 5:25 PM, Weijun Wang 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 wrote:
Hi Sean,
Thanks for your review. Reply inline.
> On May 1, 2020, at 11:50 AM, Sean Mullan 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
>>> 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
>>>
>>>
>>>
>>>
>>
>
Re: RFR[15] 8242060: Add revocation checking to jarsigner
> On May 5, 2020, at 12:36 PM, Hai-May Chao wrote:
>
>
>
>> On May 4, 2020, at 6:01 PM, Weijun Wang wrote:
>>
>>
>>
>>> On May 5, 2020, at 3:48 AM, Hai-May Chao wrote:
>>>
>>> Hi Max,
>>>
On May 2, 2020, at 5:25 PM, Weijun Wang 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 don't intend to clear the listener in loadKeyStore().
>>> (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.
I was thinking about clearing the listener only after validation. However, I
can see now that Validator.getInstance(...).validate(...) is called in
validateCertChain(), then called in multiple places. It's a little complicated
to set/clear multiple times.
So it's OK to clear it in run(). Do you think we can also set the listener in
this method? Say, before the "if (verify)" line?
Thanks,
Max
>>
>> 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 wrote:
>
> Hi Sean,
>
> Thanks for your review. Reply inline.
>
>> On May 1, 2020, at 11:50 AM, Sean Mullan 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
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://c
Re: RFR[15] 8242060: Add revocation checking to jarsigner
> On May 4, 2020, at 10:23 PM, Weijun Wang wrote:
>
>
>
>> On May 5, 2020, at 12:36 PM, Hai-May Chao wrote:
>>
>>
>>
>>> On May 4, 2020, at 6:01 PM, Weijun Wang wrote:
>>>
>>>
>>>
On May 5, 2020, at 3:48 AM, Hai-May Chao wrote:
Hi Max,
> On May 2, 2020, at 5:25 PM, Weijun Wang 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 don't intend to clear the listener in loadKeyStore().
>
(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.
>
> I was thinking about clearing the listener only after validation. However, I
> can see now that Validator.getInstance(...).validate(...) is called in
> validateCertChain(), then called in multiple places. It's a little
> complicated to set/clear multiple times.
>
> So it's OK to clear it in run(). Do you think we can also set the listener in
> this method? Say, before the "if (verify)" line?
The current change is based on the original flow where it sets PKIX revocation.
Hence, I’d like to keep the current change as is unless there is an issue with
it.
Thanks,
Hai-May
>
> Thanks,
> Max
>
>>>
>>> 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 wrote:
>>
>> Hi Sean,
>>
>> Thanks for your review. Reply inline.
>>
>>> On May 1, 2020, at 11:50 AM, Sean Mullan 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
Re: RFR[15] 8242060: Add revocation checking to jarsigner
On 5/2/20 2:19 PM, Hai-May Chao wrote: 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). I added a few more details to the release note. Otherwise, looks good, so you can mark it Resolved/Delivered. --Sean
Re: RFR[15] 8242060: Add revocation checking to jarsigner
> On May 5, 2020, at 6:16 AM, Sean Mullan wrote: > > On 5/2/20 2:19 PM, Hai-May Chao wrote: >>> 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). > > I added a few more details to the release note. Otherwise, looks good, so you > can mark it Resolved/Delivered. > Thanks for your review. I’ve marked it Resolved/Delivered. Hai-May
