Re: RFR JDK-8219989 : Retire the com.sun.net.ssl.internal.ssl.Provider name

[Hai-May Chao]

Hi Xuelei,

Looks good to me.

Hai-May


> On Mar 12, 2020, at 10:39 AM, Xuelei Fan  wrote:
> 
> Hi,
> 
> Could I get the following update reviewed?
> 
> Bug#:   https://bugs.openjdk.java.net/browse/JDK-8219989
> Webrev: http://cr.openjdk.java.net/~xuelei/8219989/webrev.00/
> Release note task: https://bugs.openjdk.java.net/browse/JDK-8240967
> 
> This is a request to remove the legacy SunJSSE provider name, 
> "com.sun.net.ssl.internal.ssl.Provider".  The "SunJSSE" should be used 
> instead (for example, "SSLContext.getInstance("TLS", "SunJSSE")").
> 
> The use of the legacy provider name should be rare now.  But please let me 
> know if you have concerns before March 19, 2019.
> 
> Thanks,
> Xuelei

Re: RFR JDK-8227024 : Remove the deprecated javax.security.cert APIs

[Hai-May Chao]

Hi Xuelei,

Looks good to me.

Hai-May


> On Mar 12, 2020, at 10:34 AM, Xuelei Fan  wrote:
> 
> And the release note task:
>  https://bugs.openjdk.java.net/browse/JDK-8240968
> 
> Xuelei
> 
> On 3/12/2020 9:47 AM, Xuelei Fan wrote:
>> Hi,
>> Could I get the following update reviewed?
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8227395
>> Webrev: http://cr.openjdk.java.net/~xuelei/8227024/webrev.00/
>> The legacy javax.security.cert APIs and the dependent were initially 
>> deprecated in Java SE 9 and marked for removal in Java SE 13. Applications 
>> should use the java.security.cert APIs for now.  This is a request to remove 
>> the deprecated javax.security.cert APIs.
>> The use of the legacy APIs should be rare now.  But please let me know if 
>> you have concerns before March 19, 2019.
>> Thanks & Regards,
>> Xuelei

Re: RFR 8242811: AlgorithmId::getDefaultAlgorithmParameterSpec returns incompatible PSSParameterSpec for an RSASSA-PSS key

[Hai-May Chao]

Changes good to me.

Hai-May


> On Apr 17, 2020, at 3:27 PM, Valerie Peng  wrote:
> 
> 
> Changes look good~
> 
> Valerie
> 
> On 4/15/2020 3:34 AM, Weijun Wang wrote:
>> Please take a review at
>> 
>>https://cr.openjdk.java.net/~weijun/8242811/webrev.00/
>> 
>> The AlgorithmId::getDefaultAlgorithmParameterSpec method is used internally 
>> to retrieve a default AlgorithmParameterSpec to initialize a Signature, 
>> mainly be keytool.
>> 
>> The test shows a case where it can be called.
>> 
>> Thanks,
>> Max
>> 

Re: RFR 8242184: CRL generation error with RSASSA-PSS

[Hai-May Chao]

Hi Max,

Changes look good to me.

Hai-May


> On Apr 6, 2020, at 8:11 PM, Weijun Wang  wrote:
> 
> Please review the fix at
> 
>   http://cr.openjdk.java.net/~weijun/8242184/webrev.00/
> 
> The major change is inside X509CRLImpl.java to allow params setting and 
> reading.
> 
> I also take this chance to:
> 
> 1. Provide a default -sigalg for "keytool -genkeypair -keyalg rsassa-pss".
> 
> 2. Revert a former change in X509CertImpl.java, which might be a safer call.
> 
> Thanks,
> Max
> 

Re: RFR[15]: 8172404: Tools should warn if weak algorithms are used before restricting them

[Hai-May Chao]

Hi Max,

Thanks for your review.
I’ve updated webrev with your comment.

Hai-May


> On Apr 7, 2020, at 8:13 PM, Weijun Wang  wrote:
> 
> Everything looks fine, except a very tiny issue:
> 
> 1332 private String verifyWithWeak(PublicKey key) {
> 1333 if (DISABLED_CHECK.permits(SIG_PRIMITIVE_SET, key)) {
> 1334 if (LEGACY_CHECK.permits(SIG_PRIMITIVE_SET, key)) {
> 1335 int kLen = KeyUtil.getKeySize(key);
> 1336 if (kLen >= 0) {
> 1337 return String.format(rb.getString("key.bit"), kLen);
> 1338 } else {
> 1339 return rb.getString("unknown.size");
> 1340 }
> 1341 } else {
> 1342 weakPublicKey = key;
> 1343 legacyAlg |= 8;
> 1344 return String.format(rb.getString("key.bit.weak"), 
> KeyUtil.getKeySize(key));
> 1345 }
> 1346 } else {
> 1347disabledAlgFound = true;
> 1348return String.format(rb.getString("key.bit.disabled"), 
> KeyUtil.getKeySize(key));
> 1349 }
> 1350 }
> 
> You can move line 1335 before line 1334 since the size is also used in the 
> else block on lines 1342-1344.
> 
> Thanks,
> Max
> 
>> On Apr 6, 2020, at 12:51 AM, Hai-May Chao  wrote:
>> 
>> Here is the webrev:
>> 
>> http://cr.openjdk.java.net/~weijun/8172404/webrev.00/
>> 
>> Thanks,
>> Hai-May
>> 
>> 
>>> On Apr 4, 2020, at 11:41 PM, Hai-May Chao  wrote:
>>> 
>>> Hi,
>>> 
>>> I'd like to request a review for:
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8172404
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8238640
>>> 
>>> It’d be useful to start warning users that certain algorithms and key 
>>> lengths are becoming weak, so that users could begin transition away from 
>>> them before they are actually disabled. A new security property named 
>>> jdk.security.legacyAlgorithms is added to the java.security file to list 
>>> the legacy algorithms. The keytool and jarsigner tools are enhanced to 
>>> enforce the new property and to emit the warning messages when legacy 
>>> algorithms are used.
>>> 
>>> Thanks,
>>> Hai-May
>> 
> 

Re: RFR 8242260: Remove customizable ContentSigner from jarsigner

[Hai-May Chao]

Hi Max,

Changes look good to me.

Is there a man page bug being filed for this?

Thanks,
Hai-May


> On Apr 7, 2020, at 1:04 AM, Weijun Wang  wrote:
> 
> I am thinking about removing the `jarsigner -altsigner -altsignerpath` 
> options and underlying classes:
> 
>JBS : https://bugs.openjdk.java.net/browse/JDK-8242260
> 
> Please review everything at:
> 
>   Release note : https://bugs.openjdk.java.net/browse/JDK-8242261
>CSR : https://bugs.openjdk.java.net/browse/JDK-8242262
> webrev : http://cr.openjdk.java.net/~weijun/8242260/webrev.00/
> 
> The CSR "Problem" section has more info on why it's better to remove it now.
> 
> Thanks,
> Max
> 

RFR[15]: 8186143: keytool -ext option doesn’t accept wildcards for DNS subject alternatives names

[Hai-May Chao]

Hi,

I need a code review for -

Bug: https://bugs.openjdk.java.net/browse/JDK-8186143
Webrev: http://cr.openjdk.java.net/~weijun/8186143/webrev.00/

The keytool -ext option doesn’t accept wildcards for DNS subject alternatives 
names in certificates. Certificates with wildcarded domains are useful for 
allowing domain names under a common subdomain to share the same certificate.

The fix involves adding a new DNSName constructor with an additional boolean 
flag ‘allowWildcard’.

Thank you,
Hai-May

Re: RFR[15]: 8186143: keytool -ext option doesn’t accept wildcards for DNS subject alternatives names

[Hai-May Chao]

Hi Jamil,

Thanks for your review! Reply inline.

> On Mar 13, 2020, at 12:24 PM, Jamil Nimeh  wrote:
> 
> Hello Hai-May,
> 
> The fix overall looks good.  One or two comments about the test:
> 
> 103: I think the comment might be more clear saying something like "partial 
> wildcard disallowed" since it's not the "*" in and of itself that's the 
> issue, it's that the next character following it isn't a domain separator 
> (".”).

Agree. Will update the comment.

> A similar badSanNames test case (I think) that walks a different code path 
> would be something like "a*.com".  Although the test on line 95 might walk 
> the same codepath...If so then no need to add anything else.

I’ll add “a*.com” to badSanNames test case as it will detect the error for 
‘DNSName components must consist of letters, digits, and hyphens’.
Line 95 test case will give us a different error from “a*.com”. That is, 
‘DNSName with blank components is not permitted’.
The existing badNames test case does not have “a*.com”, and I will add it too. 

Thanks,
Hai-May

> --Jamil
> 
> On 3/13/2020 9:25 AM, Hai-May Chao wrote:
>> Hi,
>> 
>> I need a code review for -
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8186143 
>> <https://bugs.openjdk.java.net/browse/JDK-8186143>
>> Webrev: http://cr.openjdk.java.net/~weijun/8186143/webrev.00/ 
>> <http://cr.openjdk.java.net/~weijun/8186143/webrev.00/>
>> 
>> The keytool -ext option doesn’t accept wildcards for DNS subject 
>> alternatives names in certificates. Certificates with wildcarded domains are 
>> useful for allowing domain names under a common subdomain to share the same 
>> certificate.
>> 
>> The fix involves adding a new DNSName constructor with an additional boolean 
>> flag ‘allowWildcard’.
>> 
>> Thank you,
>> Hai-May
>> 

RFR[15]: 8172404: Tools should warn if weak algorithms are used before restricting them

[Hai-May Chao]

Hi,

I'd like to request a review for:

Bug: https://bugs.openjdk.java.net/browse/JDK-8172404 

CSR: https://bugs.openjdk.java.net/browse/JDK-8238640 


It’d be useful to start warning users that certain algorithms and key lengths 
are becoming weak, so that users could begin transition away from them before 
they are actually disabled. A new security property named 
jdk.security.legacyAlgorithms is added to the java.security file to list the 
legacy algorithms. The keytool and jarsigner tools are enhanced to enforce the 
new property and to emit the warning messages when legacy algorithms are used.

Thanks,
Hai-May

Re: RFR[15]: 8172404: Tools should warn if weak algorithms are used before restricting them

[Hai-May Chao]

Here is the webrev:

http://cr.openjdk.java.net/~weijun/8172404/webrev.00/

Thanks,
Hai-May


> On Apr 4, 2020, at 11:41 PM, Hai-May Chao  wrote:
> 
> Hi,
> 
> I'd like to request a review for:
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8172404 
> <https://bugs.openjdk.java.net/browse/JDK-8172404>
> CSR: https://bugs.openjdk.java.net/browse/JDK-8238640 
> <https://bugs.openjdk.java.net/browse/JDK-8238640>
> 
> It’d be useful to start warning users that certain algorithms and key lengths 
> are becoming weak, so that users could begin transition away from them before 
> they are actually disabled. A new security property named 
> jdk.security.legacyAlgorithms is added to the java.security file to list the 
> legacy algorithms. The keytool and jarsigner tools are enhanced to enforce 
> the new property and to emit the warning messages when legacy algorithms are 
> used.
> 
> Thanks,
> Hai-May

RFR[15] 8242060: Add revocation checking to jarsigner

[Hai-May Chao]

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

[Hai-May Chao]

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

[Hai-May Chao]

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

[Hai-May Chao]

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

[Hai-May Chao]

> 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

[Hai-May Chao]

> 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 

Re: RFR[15] 8242060: Add revocation checking to jarsigner

[Hai-May Chao]

> 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

RFR[15] 8245151: jarsigner should not raise duplicate warnings on verification

[Hai-May Chao]

Hi,

I’d like to request a review for -

JBS: https://bugs.openjdk.java.net/browse/JDK-8245151
Webrev: https://cr.openjdk.java.net/~hchao/8245151/webrev.00/

The change is to provide a distinct warning for jarsigner -verify command when 
it detects weak timestamp digest algorithms are used (by -tsadigestalg when 
signing).

Thanks,
Hai-May

RFR[15] 8245665: Test WeakAlg.java should only make sure no warning for weak signature algorithms by keytool on root CA

[Hai-May Chao]

Hi,

I’d like to request q review for -

JBS: https://bugs.openjdk.java.net/browse/JDK-8245665
Webrev: https://cr.openjdk.java.net/~hchao/8245665/webrev.00/

Keytool only emits warnings for the root CA in cacerts using the weak key, but 
not for using the weak algorithm. So test case WeakAlg.java should do the 
checking on warnings accordingly.

Thanks,
Hai-May

Re: RFR[15] 8245665: Test WeakAlg.java should only make sure no warning for weak signature algorithms by keytool on root CA

[Hai-May Chao]

I did testing with a MD5RSA root CA. Updated the webrev.

Thanks,
Hai-May


> On May 22, 2020, at 8:50 PM, Weijun Wang  wrote:
> 
> You probably should use shouldNotMatch() because here it's a pattern match 
> instead of simply contain.
> 
> Try add a MD5withRSA cert there to see how it works.
> 
> Thanks,
> Max
> 
>> On May 23, 2020, at 11:01 AM, Hai-May Chao  wrote:
>> 
>> Hi,
>> 
>> I’d like to request q review for -
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245665
>> Webrev: https://cr.openjdk.java.net/~hchao/8245665/webrev.00/
>> 
>> Keytool only emits warnings for the root CA in cacerts using the weak key, 
>> but not for using the weak algorithm. So test case WeakAlg.java should do 
>> the checking on warnings accordingly.
>> 
>> Thanks,
>> Hai-May
>> 
> 

Re: RFR[15] 8242060: Add revocation checking to jarsigner

[Hai-May Chao]

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: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms

[Hai-May Chao]

On Wed, 7 Oct 2020 22:08:19 GMT, Hai-May Chao  wrote:

>> Default algorithms are bumped to be based on PBES2 with AES-256 and SHA-256. 
>> Please also review the CSR at
>> https://bugs.openjdk.java.net/browse/JDK-8228481.
>
> Looks good. Only minor comments.

CSR looks good. In "Sepcification" section: a typo in 'Thr iteration counts 
used by'. At the end, it describes the new
system property will override the security properties and use the older and 
weaker algorithms, so suggest we could also
add text about setting the iteration counts to the default legacy values.

-

PR: https://git.openjdk.java.net/jdk/pull/473

Re: RFR: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms

[Hai-May Chao]

On Thu, 1 Oct 2020 20:02:34 GMT, Weijun Wang  wrote:

> Default algorithms are bumped to be based on PBES2 with AES-256 and SHA-256. 
> Please also review the CSR at
> https://bugs.openjdk.java.net/browse/JDK-8228481.

Looks good. Only minor comments.

src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 103:

> 101: = "PBEWithHmacSHA256AndAES_256";
> 102: private static final String DEFAULT_MAC_ALGORITHM = "HmacPBESHA256";
> 103: private static final int DEFAULT_PBE_ITERATION_COUNT = 5;

As we have keystore.pkcs12.certPbeIterationCount and 
keystore.pkcs12.keyPbeIterationCount, I would like to suggest that
we can define DEFAULT_CERT_PBE_ITERATION_COUNT and 
DEFAULT_KEY_PBE_ITERATION_COUNT, specifying each of the values for
finer granularity. Same for LEGACY_PBE_ITERATION_COUNT.

test/jdk/sun/security/mscapi/VeryLongAlias.java line 48:

> 46:
> 47: static String alias = String.format("%0512d", new 
> Random().nextInt(10));
> 48:

Add bug number to @bug.

-

PR: https://git.openjdk.java.net/jdk/pull/473

Re: RFR: 8238157: Remove intermittent key from AmazonCA.java

[Hai-May Chao]

Looks good.

Thanks,
Hai-May


> On Aug 26, 2020, at 10:13 AM, Rajan Halade  wrote:
> 
> Please review this update to remove key intermittent from AmazonCA test. This 
> test no longer fails intermittently.
> 
> @@ -24,7 +24,6 @@
>  /*
>   * @test
>   * @bug 8233223
> - * @key intermittent
>   * @summary Interoperability tests with Amazon's CA1, CA2, CA3, and CA4
>   * @build ValidatePathWithParams
>   * @run main/othervm -Djava.security.debug=certpath AmazonCA OCSP
> 
> Thanks,
> Rajan
> 

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Hai-May Chao]

JarSigner.java #953: The output debug message can be removed from the code.
JavaUtilZipFileAccess.java #44: Change posixPerms to extraAttrs.
ZipFile.java #661: Suggest to keep the comment and update it with the 
additional 4 bits for symlink.

The rest of code changes and CSR look good.

Thanks,
Hai-May


> On Aug 28, 2020, at 7:17 AM, Seán Coffey  wrote:
> 
> I've been poking around the zip internals and am now able to locate the 16 
> bits of interest. The position of these actual bits does appear to move 
> around from one test run to another. For now, I guess it's sufficient to look 
> for the pattern of interest in the signed zip file. New testcase added.
> 
> http://cr.openjdk.java.net/~coffeys/webrev.8250968.v4/webrev/ 
> 
> regards,
> Sean.
> 
> On 27/08/2020 15:58, Weijun Wang wrote:
>>> Looks like it was a conscious design decision to only allow recording of 
>>> POSIX permission bits for this field (& 0xFFF). I don't see anything about 
>>> symlink support in zipfs docs.
>> As long as that *byte* is there and it’s not difficult to locate, we can 
>> manually add the *bit* for symlink and see if jarsigner can keep it.
>> 
>> —Max
>> 

Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v2]

[Hai-May Chao]

> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the 
> parameters field instead of encoding a
> Null tag.

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  Updated test case to use DerUtils

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/312/files
  - new: https://git.openjdk.java.net/jdk/pull/312/files/f52268a7..edc71d03

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=312=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=312=00-01

  Stats: 189 lines in 1 file changed: 35 ins; 132 del; 22 mod
  Patch: https://git.openjdk.java.net/jdk/pull/312.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/312/head:pull/312

PR: https://git.openjdk.java.net/jdk/pull/312

Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v2]

[Hai-May Chao]

On Fri, 25 Sep 2020 00:45:09 GMT, Weijun Wang  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated test case to use DerUtils
>
> src/java.base/share/classes/sun/security/x509/AlgorithmId.java line 214:
> 
>> 212: || algid.equals(SHA512withECDSA_oid)) {
>> 213: // RFC 4055 3.3: when an RSASSA-PSS key does not require
>> 214: // parameter validation, field is absent.
> 
> Would you like to add more comments on other RFCs here?

Added the comments for RFC reference.

-

PR: https://git.openjdk.java.net/jdk/pull/312

Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v3]

[Hai-May Chao]

> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the 
> parameters field instead of encoding a
> Null tag.

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  Added comment for RFC

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/312/files
  - new: https://git.openjdk.java.net/jdk/pull/312/files/edc71d03..4f4e3688

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=312=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=312=01-02

  Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/312.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/312/head:pull/312

PR: https://git.openjdk.java.net/jdk/pull/312

Integrated: 8252377: Incorrect encoding for EC AlgorithmIdentifier

[Hai-May Chao]

On Tue, 22 Sep 2020 22:21:20 GMT, Hai-May Chao  wrote:

> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the 
> parameters field instead of encoding a
> Null tag.

This pull request has now been integrated.

Changeset: 0e855fe5
Author:    Hai-May Chao 
Committer: Weijun Wang 
URL:   https://git.openjdk.java.net/jdk/commit/0e855fe5
Stats: 125 lines in 2 files changed: 124 ins; 0 del; 1 mod

8252377: Incorrect encoding for EC AlgorithmIdentifier

Reviewed-by: weijun

-

PR: https://git.openjdk.java.net/jdk/pull/312

RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier

[Hai-May Chao]

This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the 
parameters field instead of encoding a
Null tag.

-

Commit messages:
 - 8252377: Incorrect encoding for EC AlgorithmIdentifier

Changes: https://git.openjdk.java.net/jdk/pull/312/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=312=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252377
  Stats: 217 lines in 2 files changed: 216 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/312.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/312/head:pull/312

PR: https://git.openjdk.java.net/jdk/pull/312

Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier

[Hai-May Chao]

On Wed, 23 Sep 2020 02:49:29 GMT, Weijun Wang  wrote:

>> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the 
>> parameters field instead of encoding a
>> Null tag.
>
> I don't quite understand what the test is for. The bug is about encoding but 
> the test seems to be decoding the
> certificates. Does the test fail before this fix and succeed after it?

This is because the encoding of Algorithm Identifier incorrectly adds two NULL 
tags to the parameters field in the
canned certificate. (There are two Algorithm Identifiers in the cert, with each 
NULL tag containing two bytes:  tag +
length.) I use the length of an encoded certificate 
(x509Cert.getEncoded().length) to verify that the certificate
contains an extra 4 bytes to hold the two NULL tags. Therefore, the certificate 
without the fix should be 4 bytes (5
bytes if one byte alignment is applied) longer in length than the certificate 
with the fix.

-

PR: https://git.openjdk.java.net/jdk/pull/312

RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Hai-May Chao]

Hi,

I’d like to request a review for:

JBS: https://bugs.openjdk.java.net/browse/JDK-8244148
CSR: https://bugs.openjdk.java.net/browse/JDK-8246269
Webrev: http://cr.openjdk.java.net/~hchao/8244148/webrev.00/

The change is to add the support of -trustcacerts and -keystore options to 
-printcert and -princrl command for keytool. This enables keytool to use the 
trusted certificates when verifying untrusted artifacts that are signed by CAs. 
It also incorporates Max’s change that consolidates the code to get the default 
location of cacerts keystore.

Thanks,
Hai-May

Re: RFR: 8007632: DES/3DES keys support in PKCS12 keystore [v3]

[Hai-May Chao]

On Tue, 27 Oct 2020 17:59:38 GMT, Weijun Wang  wrote:

>> Alexey Bakhtin has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix order of OIDs
>
> Marked as reviewed by weijun (Reviewer).

Change looks good.

-

PR: https://git.openjdk.java.net/jdk/pull/877

Re: RFR 8247960: jarsigner says "signer errors" for some normal warnings when -strict is set

[Hai-May Chao]

Bugid added.

Thanks,
Hai-May


> On Jul 15, 2020, at 12:06 PM, Sean Mullan  wrote:
> 
> I'll defer to Max on the code changes, but I noticed one thing on the test - 
> you should add the bugid to the @bug line of the test.
> 
> --Sean
> 
> On 7/14/20 4:09 PM, Hai-May Chao wrote:
>> Hi,
>> I’d like to request a review for:
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8247960
>> Webrev: https://cr.openjdk.java.net/~hchao/8247960/webrev.00/
>> Jarsigner is changed to emit “with signer errors” only when there are errors 
>> detected during sign and verify with -strict specified.
>> Thanks,
>> Hai-May

Re: RFR [16] [JDK-8248745] Add jarsigner and keytool tests for restricted algorithms

[Hai-May Chao]

Hi Muneer,

Updated webrev looks good.

Thanks,
Hai-May


> On Aug 4, 2020, at 7:26 PM, abdul.kolarku...@oracle.com wrote:
> 
> Thanks Hai-May for review.
> 
> Updated the webrev with your comment 
> -http://cr.openjdk.java.net/~akolarkunnu/8248745/webrev.01/
> 
> -Muneer
> 
> On 04/08/20 11:58 pm, Hai-May Chao wrote:
>> Hi Muneer,
>> 
>> Looks good with one minor comment.
>> 
>> #58: suggest that the SECURITY_WARNING will also include “and is disabled” 
>> at the end to make it clear.
>> 
>> Thanks,
>> Hai-May
>> 
>>> On Jul 27, 2020, at 9:15 AM, abdul.kolarku...@oracle.com wrote:
>>> 
>>> Hi All,
>>> 
>>> This is a new test int the area of jarsigner and keytool for the 
>>> restricted/disabled algorithms.
>>> 
>>> Bug Id - https://bugs.openjdk.java.net/browse/JDK-8248745
>>> 
>>> Webrev - http://cr.openjdk.java.net/~akolarkunnu/8248745/webrev.00/
>>> 
>>> Description:
>>> 
>>> Adding a test for key generation, jar signing and verification with all 
>>> disabled algorithms and key sizes which are in the property 
>>> jdk.jar.disabledAlgorithms.
>>> Covered the scenario of with and without these disabled entries in 
>>> jdk.jar.disabledAlgorithms.
>>> 
>>> Whenever the entries are in the property jdk.jar.disabledAlgorithms, 
>>> corresponding warning or error message should shown, otherwise everything 
>>> should work fine without any related error or warning.
>>> 
>>> This test covers all entries listed in "jdk.jar.disabledAlgorithms=MD2, 
>>> MD5, RSA keySize < 1024, DSA keySize < 1024, include 
>>> jdk.disabled.namedCurves". In case of disabled curves, this test covers 
>>> only one curve secp112r1.
>>> 
>>> Tested in Linux, Windows and Mac Osx platforms and all are working fine.
>>> 
>>> -Muneer
>>> 

Re: RFR [16] [JDK-8248745] Add jarsigner and keytool tests for restricted algorithms

[Hai-May Chao]

Hi Muneer,

Looks good with one minor comment.

#58: suggest that the SECURITY_WARNING will also include “and is disabled” at 
the end to make it clear.

Thanks,
Hai-May

> On Jul 27, 2020, at 9:15 AM, abdul.kolarku...@oracle.com wrote:
> 
> Hi All,
> 
> This is a new test int the area of jarsigner and keytool for the 
> restricted/disabled algorithms.
> 
> Bug Id - https://bugs.openjdk.java.net/browse/JDK-8248745
> 
> Webrev - http://cr.openjdk.java.net/~akolarkunnu/8248745/webrev.00/
> 
> Description:
> 
> Adding a test for key generation, jar signing and verification with all 
> disabled algorithms and key sizes which are in the property 
> jdk.jar.disabledAlgorithms.
> Covered the scenario of with and without these disabled entries in 
> jdk.jar.disabledAlgorithms.
> 
> Whenever the entries are in the property jdk.jar.disabledAlgorithms, 
> corresponding warning or error message should shown, otherwise everything 
> should work fine without any related error or warning.
> 
> This test covers all entries listed in "jdk.jar.disabledAlgorithms=MD2, MD5, 
> RSA keySize < 1024, DSA keySize < 1024, include jdk.disabled.namedCurves". In 
> case of disabled curves, this test covers only one curve secp112r1.
> 
> Tested in Linux, Windows and Mac Osx platforms and all are working fine.
> 
> -Muneer
> 

Re: [RFR] 8246806: Incorrect copyright header in KeyAgreementTest.java, GroupName.java

[Hai-May Chao]

Hi Tony,

Looks good.

Hai-May


> On Jul 7, 2020, at 5:01 PM, Anthony Scarpino  
> wrote:
> 
> Hi,
> 
> I need a code review to fix some copyright headers.  The diffs are below
> 
> thanks
> 
> Tony
> 
> --
> 
> +++ b/test/jdk/java/security/KeyAgreement/KeyAgreementTest.java
> - * Copyright (c) 2018, 2020 Oracle and/or its affiliates. All rights 
> reserved.
> + * Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights 
> reserved.
> 
> 
> +++ b/test/jdk/sun/security/tools/keytool/GroupName.java
> - * Copyright (c) 2018, 2020,  Oracle and/or its affiliates. All rights 
> reserved.
> + * Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights 
> reserved.
> 

RFR 8247960: jarsigner says "signer errors" for some normal warnings when -strict is set

[Hai-May Chao]

Hi,

I’d like to request a review for:

JBS: https://bugs.openjdk.java.net/browse/JDK-8247960
Webrev: https://cr.openjdk.java.net/~hchao/8247960/webrev.00/

Jarsigner is changed to emit “with signer errors” only when there are errors 
detected during sign and verify with -strict specified.

Thanks,
Hai-May

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Hai-May Chao]

Updated webrev -

https://cr.openjdk.java.net/~hchao/8244148/webrev.02/ 
<https://cr.openjdk.java.net/~hchao/8244148/webrev.02/>

Thanks,
Hai-May


> On Jun 5, 2020, at 11:04 PM, Weijun Wang  wrote:
> 
> I still think duplicated commands in TrustedCert.java are useless. Line 104 
> and line 133 are exactly the same, line 109 and line 138 are exactly the 
> same, and you haven't made any change to these 2 files in between.
> 
> Same for line 80 and line 96 of TrustedCRL.java.
> 
> Everything else is fine.
> 
> Thanks,
> Max
> 
> 
>> On Jun 6, 2020, at 2:25 AM, Hai-May Chao  wrote:
>> 
>> Updated webrev - 
>> 
>> https://cr.openjdk.java.net/~hchao/8244148/webrev.01/
>> 
>> Added one command line -importcert in TrustCert.java.
>> Added createCacerts() in test/lib SecurityTools.java.
>> 
>> Thanks,
>> Hai-May
>> 
>> 
>> 
>>> On Jun 4, 2020, at 5:57 AM, Weijun Wang  wrote:
>>> 
>>> 
>>> 
>>>> On Jun 4, 2020, at 7:29 PM, Hai-May Chao  wrote:
>>>> 
>>>> Hi Max,
>>>> 
>>>>> On Jun 3, 2020, at 12:59 AM, Weijun Wang  wrote:
>>>>> 
>>>>> The source change looks fine to me.
>>>>> 
>>>>> In TrustedCert.java:
>>>>> 
>>>>> - You can use FileOutputStream and Files.copy(Path,OutputStream) in cat().
>>>> 
>>>> This cat() is taken from WealAlg.java.
>>>> 
>>>>> 
>>>>> - There is no need to recreate root.jks and root.pem.
>>>> 
>>>> The sequences of the commands used in this test scenario allows me to test 
>>>> -printcert for the -trustcacerts and -keytsore options. We had discussion 
>>>> offline about it. The test uses trusted certificates and checks no 
>>>> warnings on the weak algorithms to address the requirement described in 
>>>> the bug. I believe it does serve that purpose, and looks legitimate to me. 
>>>> There could be different ways of testing a functionality, and please let 
>>>> me know if there is a problem with the current approach.
>>> 
>>> I just meant that the keytool commands generating root.jks and root.pem are 
>>> exactly the same and there is no need to recreate it.
>>> 
>>>> 
>>>> Please also elaborate your comment about no need to recreate root.jks and 
>>>> root.pem.
>>>> 
>>>>> 
>>>>> - Why not use -trustcacerts below?
>>>>> 
>>>>> 160 kt("-importcert -file server.pem -noprompt", "server.jks”);
>>>> 
>>>> 
>>>> Because here is to import the server (end-entity) cert, and it will not 
>>>> make a difference for the test result whether to use the -trustcacerts or 
>>>> not. It’s the ca (intermediate) cert needs to have it in this test 
>>>> scenario. I intended to leave it out in #160 to distinguish between server 
>>>> and ca certs.
>>> 
>>> OK.
>>> 
>>> Then how about we add a new command before line 155?
>>> 
>>>   kt("-importcert -file ca.pem", "ca.jks").shouldNotHaveExitValue(0);
>>> 
>>> This would prove the "-trustcacerts" on line 155 is really useful.
>>> 
>>>> 
>>>>> 
>>>>> - It's probably better to add a " " between cmd and options in 
>>>>> patchcmd(). Same in TrustedCRL.java.
>>>> 
>>>> Ok, will change it.
>>>> 
>>>>> 
>>>>> In TrustedCRL.java:
>>>>> 
>>>>> - No need to recreate ks and ca.crl. Just call "-printcrl" with different 
>>>>> options.
>>>> 
>>>> Same reply as above.
>>> 
>>> Same question as above.
>>> 
>>>> 
>>>>> 
>>>>> - Why create using MD5withRSA? Do you meant to warn about the weak 
>>>>> algorithm?
>>>> 
>>>> Yes, exactly, and it differentiates from the weak algorithm SHA1withRSA 
>>>> used in root CA where no warning will be emitted. There is another -gencrl 
>>>> in #119 without using MD5withRSA so I’d have two test cases.
>>>> 
>>>>> 
>>>>> Also I would suggest you create a dedicate method (maybe in 
>>>>> SecurityTools.java) to create your own cacerts. There is no need to copy 
>>>>> over the system cacerts, just make sure the

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Hai-May Chao]

> On Jun 9, 2020, at 5:58 PM, Weijun Wang  wrote:
> 
> Good to see both -keystore and -trustcacerts mentioned. Some comments:
> 
> 1. I think there is no need to say "from -file cert_file". Or, do you mean 
> the new function does not apply to those from -sslserver and -jarfile? If so, 
> that might be a problem.

You’re right. Removed it.

> 
> 2. While you said "attempts to construct a chain of trust", do you also want 
> to describe what happens if it succeeds or fails?

Updated manpage.

> 
> 3. It will be nice if you can include the exact diff of the man page files 
> either inside the CSR itself or as a comment.
> 

Included the diff of the manpage in the CSR.

Thanks,
Hai-May


> Thanks,
> Max
> 
>> On Jun 9, 2020, at 10:51 PM, Hai-May Chao  wrote:
>> 
>> 
>> 
>>> On Jun 7, 2020, at 6:08 PM, Weijun Wang  wrote:
>>> 
>>> Looks fine to me.
>>> 
>>> For CSR, since there is already a "Note" there for these 2 options, you can 
>>> add a few words about what -keystore and -trustcacerts can do.
>> 
>> Updated CSR as suggested.
>> 
>> Thanks,
>> Hai-May
>> 
>> 
>>> 
>>> Thanks,
>>> Max
>>> 
>>>> On Jun 8, 2020, at 4:01 AM, Hai-May Chao  wrote:
>>>> 
>>>> Updated webrev -
>>>> 
>>>> https://cr.openjdk.java.net/~hchao/8244148/webrev.02/
>>>> 
>>>> Thanks,
>>>> Hai-May
>>>> 
>>>> 
>>>>> On Jun 5, 2020, at 11:04 PM, Weijun Wang  wrote:
>>>>> 
>>>>> I still think duplicated commands in TrustedCert.java are useless. Line 
>>>>> 104 and line 133 are exactly the same, line 109 and line 138 are exactly 
>>>>> the same, and you haven't made any change to these 2 files in between.
>>>>> 
>>>>> Same for line 80 and line 96 of TrustedCRL.java.
>>>>> 
>>>>> Everything else is fine.
>>>>> 
>>>>> Thanks,
>>>>> Max
>>>>> 
>>>>> 
>>>>>> On Jun 6, 2020, at 2:25 AM, Hai-May Chao  wrote:
>>>>>> 
>>>>>> Updated webrev - 
>>>>>> 
>>>>>> https://cr.openjdk.java.net/~hchao/8244148/webrev.01/
>>>>>> 
>>>>>> Added one command line -importcert in TrustCert.java.
>>>>>> Added createCacerts() in test/lib SecurityTools.java.
>>>>>> 
>>>>>> Thanks,
>>>>>> Hai-May
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>> On Jun 4, 2020, at 5:57 AM, Weijun Wang  wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>>> On Jun 4, 2020, at 7:29 PM, Hai-May Chao  
>>>>>>>> wrote:
>>>>>>>> 
>>>>>>>> Hi Max,
>>>>>>>> 
>>>>>>>>> On Jun 3, 2020, at 12:59 AM, Weijun Wang  
>>>>>>>>> wrote:
>>>>>>>>> 
>>>>>>>>> The source change looks fine to me.
>>>>>>>>> 
>>>>>>>>> In TrustedCert.java:
>>>>>>>>> 
>>>>>>>>> - You can use FileOutputStream and Files.copy(Path,OutputStream) in 
>>>>>>>>> cat().
>>>>>>>> 
>>>>>>>> This cat() is taken from WealAlg.java.
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> - There is no need to recreate root.jks and root.pem.
>>>>>>>> 
>>>>>>>> The sequences of the commands used in this test scenario allows me to 
>>>>>>>> test -printcert for the -trustcacerts and -keytsore options. We had 
>>>>>>>> discussion offline about it. The test uses trusted certificates and 
>>>>>>>> checks no warnings on the weak algorithms to address the requirement 
>>>>>>>> described in the bug. I believe it does serve that purpose, and looks 
>>>>>>>> legitimate to me. There could be different ways of testing a 
>>>>>>>> functionality, and please let me know if there is a problem with the 
>>>>>>>> current approach.
>>>>>>> 
>>>>>>> I just meant that the keytool commands generat

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Hai-May Chao]

Updated webrev - 

https://cr.openjdk.java.net/~hchao/8244148/webrev.01/

Added one command line -importcert in TrustCert.java.
Added createCacerts() in test/lib SecurityTools.java.

Thanks,
Hai-May



> On Jun 4, 2020, at 5:57 AM, Weijun Wang  wrote:
> 
> 
> 
>> On Jun 4, 2020, at 7:29 PM, Hai-May Chao  wrote:
>> 
>> Hi Max,
>> 
>>> On Jun 3, 2020, at 12:59 AM, Weijun Wang  wrote:
>>> 
>>> The source change looks fine to me.
>>> 
>>> In TrustedCert.java:
>>> 
>>> - You can use FileOutputStream and Files.copy(Path,OutputStream) in cat().
>> 
>> This cat() is taken from WealAlg.java.
>> 
>>> 
>>> - There is no need to recreate root.jks and root.pem.
>> 
>> The sequences of the commands used in this test scenario allows me to test 
>> -printcert for the -trustcacerts and -keytsore options. We had discussion 
>> offline about it. The test uses trusted certificates and checks no warnings 
>> on the weak algorithms to address the requirement described in the bug. I 
>> believe it does serve that purpose, and looks legitimate to me. There could 
>> be different ways of testing a functionality, and please let me know if 
>> there is a problem with the current approach.
> 
> I just meant that the keytool commands generating root.jks and root.pem are 
> exactly the same and there is no need to recreate it.
> 
>> 
>> Please also elaborate your comment about no need to recreate root.jks and 
>> root.pem.
>> 
>>> 
>>> - Why not use -trustcacerts below?
>>> 
>>> 160 kt("-importcert -file server.pem -noprompt", "server.jks”);
>> 
>> 
>> Because here is to import the server (end-entity) cert, and it will not make 
>> a difference for the test result whether to use the -trustcacerts or not. 
>> It’s the ca (intermediate) cert needs to have it in this test scenario. I 
>> intended to leave it out in #160 to distinguish between server and ca certs.
> 
> OK.
> 
> Then how about we add a new command before line 155?
> 
>kt("-importcert -file ca.pem", "ca.jks").shouldNotHaveExitValue(0);
> 
> This would prove the "-trustcacerts" on line 155 is really useful.
> 
>> 
>>> 
>>> - It's probably better to add a " " between cmd and options in patchcmd(). 
>>> Same in TrustedCRL.java.
>> 
>> Ok, will change it.
>> 
>>> 
>>> In TrustedCRL.java:
>>> 
>>> - No need to recreate ks and ca.crl. Just call "-printcrl" with different 
>>> options.
>> 
>> Same reply as above.
> 
> Same question as above.
> 
>> 
>>> 
>>> - Why create using MD5withRSA? Do you meant to warn about the weak 
>>> algorithm?
>> 
>> Yes, exactly, and it differentiates from the weak algorithm SHA1withRSA used 
>> in root CA where no warning will be emitted. There is another -gencrl in 
>> #119 without using MD5withRSA so I’d have two test cases.
>> 
>>> 
>>> Also I would suggest you create a dedicate method (maybe in 
>>> SecurityTools.java) to create your own cacerts. There is no need to copy 
>>> over the system cacerts, just make sure the file is created with the JKS 
>>> storetype. We are thinking of upgrading the storetype of cacerts and it's 
>>> nice to do this at a single place so we can modify it easily later.
>> 
>> I created a method in SecurityTools.java to create the own cacerts. With 
>> this keystore, the subsequent importing a certificate reply would not work. 
>> It turns out that its caks.size() is zero detected at establishCertChain() 
>> in keytool/Main.java after root cert has been imported to that cacerts. At 
>> this point I’d like to suggest a separate bug be filed to cover the cacerts 
>> enhancement that you suggested.
> 
> I meant creating the cacerts in one method, something like
> 
>   void createCacerts(String ks, String... crt);
> 
> and you can call createCacerts("mycacerts", "root.crt") to create it. The 
> method can call KeyStore APIs and not keytool commands.
> 
> BTW, what does caks.size() == 0 matter here?
> 
> Thanks,
> Max
> 
> 
>> 
>> Thanks,
>> Hai-May
>> 
>> 
>>> Thanks,
>>> Max
>>> 
>>> 
>>>> On Jun 2, 2020, at 2:37 AM, Hai-May Chao  wrote:
>>>> 
>>>> Hi,
>>>> 
>>>> I’d like to request a review for:
>>>> 
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244148
>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8246269
>>>> Webrev: http://cr.openjdk.java.net/~hchao/8244148/webrev.00/
>>>> 
>>>> The change is to add the support of -trustcacerts and -keystore options to 
>>>> -printcert and -princrl command for keytool. This enables keytool to use 
>>>> the trusted certificates when verifying untrusted artifacts that are 
>>>> signed by CAs. It also incorporates Max’s change that consolidates the 
>>>> code to get the default location of cacerts keystore.
>>>> 
>>>> Thanks,
>>>> Hai-May

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Hai-May Chao]

> On Jun 7, 2020, at 6:08 PM, Weijun Wang  wrote:
> 
> Looks fine to me.
> 
> For CSR, since there is already a "Note" there for these 2 options, you can 
> add a few words about what -keystore and -trustcacerts can do.

Updated CSR as suggested.

Thanks,
Hai-May


> 
> Thanks,
> Max
> 
>> On Jun 8, 2020, at 4:01 AM, Hai-May Chao  wrote:
>> 
>> Updated webrev -
>> 
>> https://cr.openjdk.java.net/~hchao/8244148/webrev.02/
>> 
>> Thanks,
>> Hai-May
>> 
>> 
>>> On Jun 5, 2020, at 11:04 PM, Weijun Wang  wrote:
>>> 
>>> I still think duplicated commands in TrustedCert.java are useless. Line 104 
>>> and line 133 are exactly the same, line 109 and line 138 are exactly the 
>>> same, and you haven't made any change to these 2 files in between.
>>> 
>>> Same for line 80 and line 96 of TrustedCRL.java.
>>> 
>>> Everything else is fine.
>>> 
>>> Thanks,
>>> Max
>>> 
>>> 
>>>> On Jun 6, 2020, at 2:25 AM, Hai-May Chao  wrote:
>>>> 
>>>> Updated webrev - 
>>>> 
>>>> https://cr.openjdk.java.net/~hchao/8244148/webrev.01/
>>>> 
>>>> Added one command line -importcert in TrustCert.java.
>>>> Added createCacerts() in test/lib SecurityTools.java.
>>>> 
>>>> Thanks,
>>>> Hai-May
>>>> 
>>>> 
>>>> 
>>>>> On Jun 4, 2020, at 5:57 AM, Weijun Wang  wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>>> On Jun 4, 2020, at 7:29 PM, Hai-May Chao  wrote:
>>>>>> 
>>>>>> Hi Max,
>>>>>> 
>>>>>>> On Jun 3, 2020, at 12:59 AM, Weijun Wang  wrote:
>>>>>>> 
>>>>>>> The source change looks fine to me.
>>>>>>> 
>>>>>>> In TrustedCert.java:
>>>>>>> 
>>>>>>> - You can use FileOutputStream and Files.copy(Path,OutputStream) in 
>>>>>>> cat().
>>>>>> 
>>>>>> This cat() is taken from WealAlg.java.
>>>>>> 
>>>>>>> 
>>>>>>> - There is no need to recreate root.jks and root.pem.
>>>>>> 
>>>>>> The sequences of the commands used in this test scenario allows me to 
>>>>>> test -printcert for the -trustcacerts and -keytsore options. We had 
>>>>>> discussion offline about it. The test uses trusted certificates and 
>>>>>> checks no warnings on the weak algorithms to address the requirement 
>>>>>> described in the bug. I believe it does serve that purpose, and looks 
>>>>>> legitimate to me. There could be different ways of testing a 
>>>>>> functionality, and please let me know if there is a problem with the 
>>>>>> current approach.
>>>>> 
>>>>> I just meant that the keytool commands generating root.jks and root.pem 
>>>>> are exactly the same and there is no need to recreate it.
>>>>> 
>>>>>> 
>>>>>> Please also elaborate your comment about no need to recreate root.jks 
>>>>>> and root.pem.
>>>>>> 
>>>>>>> 
>>>>>>> - Why not use -trustcacerts below?
>>>>>>> 
>>>>>>> 160 kt("-importcert -file server.pem -noprompt", "server.jks”);
>>>>>> 
>>>>>> 
>>>>>> Because here is to import the server (end-entity) cert, and it will not 
>>>>>> make a difference for the test result whether to use the -trustcacerts 
>>>>>> or not. It’s the ca (intermediate) cert needs to have it in this test 
>>>>>> scenario. I intended to leave it out in #160 to distinguish between 
>>>>>> server and ca certs.
>>>>> 
>>>>> OK.
>>>>> 
>>>>> Then how about we add a new command before line 155?
>>>>> 
>>>>>  kt("-importcert -file ca.pem", "ca.jks").shouldNotHaveExitValue(0);
>>>>> 
>>>>> This would prove the "-trustcacerts" on line 155 is really useful.
>>>>> 
>>>>>> 
>>>>>>> 
>>>>>>> - It's probably better to add a " " between cmd and options in 
>>>>>>> p

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Hai-May Chao]

> On Jun 12, 2020, at 5:37 AM, Weijun Wang  wrote:
> 
> I re-read the CSR.
> 
> The precise meaning of the 2 options for -printcert is: "If the cert is a 
> trusted certificate in either keystore or cacerts, we will not warn if it 
> uses a weak signature algorithm". This is so subtle and I wonder it's worth 
> describing it. Or we just say "This command does not check for the weakness 
> of a certificate's signature algorithm if it's a trusted certificate in the 
> user keystore (specified by -keystore) or the `cacerts` keystore (if 
> -trustcacerts is specified)”.

Updated CSR with the second mentioned.

> 
> For -printcrl, the "issuer CA" is a little confusing. I would say "This 
> commands attempts to verify the CRL using a certificate from the user 
> keystore (specified by -keystore) or the `cacerts` keystore (if -trustcacerts 
> is specified), and print out a warning if it cannot be verified”.

CRL issuer is usually the CA, and the implementation of -printcrl tries to get 
its issuer from the user or cacerts keystore. Updated CSR as suggested as if 
there may be confusing.

Thanks,
Hai-May

> 
> Thanks,
> Max
> 
> p.s. A new kind of check comes to my mind. Suppose you are printing a 
> certificate whose own key is 2048-bit RSA, signature is using SHA256withRSA, 
> but the signer key is only 512 bits (we can notice this from the size of the 
> signature). Shall we print out a warning? 

Your thought?

> 
>> On Jun 11, 2020, at 11:21 AM, Weijun Wang  wrote:
>> 
>> "to a self-signed certificate in the keystore". This is not correct. What we 
>> look for is a TrustedCertificateEntry.
>> 
>> "It emits warning when a certificate is not trusted and uses weak 
>> algorithms". Precisely, it's "uses a weak signature algorithm".
>> 
>> --Max
>> 
>> 
>>> On Jun 10, 2020, at 5:31 PM, Hai-May Chao  wrote:
>>> 
>>> 
>>> 
>>>> On Jun 9, 2020, at 5:58 PM, Weijun Wang  wrote:
>>>> 
>>>> Good to see both -keystore and -trustcacerts mentioned. Some comments:
>>>> 
>>>> 1. I think there is no need to say "from -file cert_file". Or, do you mean 
>>>> the new function does not apply to those from -sslserver and -jarfile? If 
>>>> so, that might be a problem.
>>> 
>>> You’re right. Removed it.
>>> 
>>>> 
>>>> 2. While you said "attempts to construct a chain of trust", do you also 
>>>> want to describe what happens if it succeeds or fails?
>>> 
>>> Updated manpage.
>>> 
>>>> 
>>>> 3. It will be nice if you can include the exact diff of the man page files 
>>>> either inside the CSR itself or as a comment.
>>>> 
>>> 
>>> Included the diff of the manpage in the CSR.
>>> 
>>> Thanks,
>>> Hai-May
>>> 
>>> 
>>>> Thanks,
>>>> Max
>>>> 
>>>>> On Jun 9, 2020, at 10:51 PM, Hai-May Chao  wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>>> On Jun 7, 2020, at 6:08 PM, Weijun Wang  wrote:
>>>>>> 
>>>>>> Looks fine to me.
>>>>>> 
>>>>>> For CSR, since there is already a "Note" there for these 2 options, you 
>>>>>> can add a few words about what -keystore and -trustcacerts can do.
>>>>> 
>>>>> Updated CSR as suggested.
>>>>> 
>>>>> Thanks,
>>>>> Hai-May
>>>>> 
>>>>> 
>>>>>> 
>>>>>> Thanks,
>>>>>> Max
>>>>>> 
>>>>>>> On Jun 8, 2020, at 4:01 AM, Hai-May Chao  
>>>>>>> wrote:
>>>>>>> 
>>>>>>> Updated webrev -
>>>>>>> 
>>>>>>> https://cr.openjdk.java.net/~hchao/8244148/webrev.02/
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Hai-May
>>>>>>> 
>>>>>>> 
>>>>>>>> On Jun 5, 2020, at 11:04 PM, Weijun Wang  
>>>>>>>> wrote:
>>>>>>>> 
>>>>>>>> I still think duplicated commands in TrustedCert.java are useless. 
>>>>>>>> Line 104 and line 133 are exactly the same, line 109 and line 138 are 
>>>>>>>> exactly the same, and you haven't made any change to these 2 files in 
>>>

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Hai-May Chao]

Hi John,

Updated Webrev -
https://cr.openjdk.java.net/~hchao/8244148/webrev.03/

> On Jun 11, 2020, at 1:45 AM, sha.ji...@oracle.com wrote:
> 
> Hi Hai-May,
> 
> On 2020/6/8 04:01, Hai-May Chao wrote:
>> Updated webrev -
>> 
>> https://cr.openjdk.java.net/~hchao/8244148/webrev.02/ 
>> <https://cr.openjdk.java.net/~hchao/8244148/webrev.02/>-- 
>> src/java.base/share/classes/sun/security/util/FilePaths.java
> Would it be better to use String.join() or even java.nio.file.Path to
> build the file path?
> 

The current code is based on the original code such as in KeyStoreUtil.java and 
is consistent with others.

> -- src/java.base/share/classes/sun/security/util/AnchorCertificates.java
> 55 File f = new File(FilePaths.cacerts());
> ...
> 59 try (FileInputStream fis = new FileInputStream(f)) {
> f is used only once, so it may be unnecessary.

This is the existing code struct which is working fine.

> 
> 56 KeyStore cacerts;
> Though it's not in your change, would you mind to declare this variable
> in the try block? I just want to narrow the variable scope.

Done.

> 
> -- test/lib/jdk/test/lib/SecurityTools.java
> Could move method createCacerts() to 
> test/lib/jdk/test/lib/security/KeyStoreUtils.java?
> This class contains a set of methods for creating trust/key stores.
> And getCertFromFile() in test/lib/jdk/test/lib/security/CertUtils.java
> can load certificate from a file.

Moved the method createCacerts() to the suggested location under test/.
Tried to use getCertFromFile() and it did not work as it read from test.src. So 
we need to read the cert directly.

> 
> 289 int pos = 0;
> ...
> 294 for (String crt : crts) {
> If not using for-each style, pos can be declared in the for statement.

Ok, changed to not using for-each style.

> 
> --  
> test/jdk/sun/security/tools/keytool/fakecacerts/java.base/sun/security/util/FilePaths.java
> This patch can be used by other tests, so could you please move it to a
> common path? Like 
> test/jdk/sun/security/util/module_patch/java.base/sun/security/util/FilePaths.java

Moved the patch to the suggested location under test/.

> 
> 32 public static String cacerts() {
> 33 return "mycacerts";
> 34 }
> Could it be flexible for returning an alternative path?
> For example, accept a system property, say test.cacerts, for specifying
> an alternative path. mycacerts is the default value of this property.

Done.

Thanks,
Hai-May


> 
> Best regards,
> John Jiang
> 
>> 
>> Thanks,
>> Hai-May
>> 
>> 
>>> On Jun 5, 2020, at 11:04 PM, Weijun Wang >> <mailto:weijun.w...@oracle.com>> wrote:
>>> 
>>> I still think duplicated commands in TrustedCert.java are useless. Line 104 
>>> and line 133 are exactly the same, line 109 and line 138 are exactly the 
>>> same, and you haven't made any change to these 2 files in between.
>>> 
>>> Same for line 80 and line 96 of TrustedCRL.java.
>>> 
>>> Everything else is fine.
>>> 
>>> Thanks,
>>> Max
>>> 
>>> 
>>>> On Jun 6, 2020, at 2:25 AM, Hai-May Chao >>> <mailto:hai-may.c...@oracle.com>> wrote:
>>>> 
>>>> Updated webrev - 
>>>> 
>>>> https://cr.openjdk.java.net/~hchao/8244148/webrev.01/ 
>>>> <https://cr.openjdk.java.net/~hchao/8244148/webrev.01/>
>>>> 
>>>> Added one command line -importcert in TrustCert.java.
>>>> Added createCacerts() in test/lib SecurityTools.java.
>>>> 
>>>> Thanks,
>>>> Hai-May
>>>> 
>>>> 
>>>> 
>>>>> On Jun 4, 2020, at 5:57 AM, Weijun Wang  
>>>>> <mailto:weijun.w...@oracle.com> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>>> On Jun 4, 2020, at 7:29 PM, Hai-May Chao  
>>>>>> <mailto:hai-may.c...@oracle.com> wrote:
>>>>>> 
>>>>>> Hi Max,
>>>>>> 
>>>>>>> On Jun 3, 2020, at 12:59 AM, Weijun Wang  
>>>>>>> <mailto:weijun.w...@oracle.com> wrote:
>>>>>>> 
>>>>>>> The source change looks fine to me.
>>>>>>> 
>>>>>>> In TrustedCert.java:
>>>>>>> 
>>>>>>> - You can use FileOutputStream and Files.copy(Path,OutputStream) in 
>>>>>>> cat().
>>>>>> 
>>>>>> This cat() is taken from WealAlg.java

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Hai-May Chao]

Hi Max,

> On Jun 3, 2020, at 12:59 AM, Weijun Wang  wrote:
> 
> The source change looks fine to me.
> 
> In TrustedCert.java:
> 
> - You can use FileOutputStream and Files.copy(Path,OutputStream) in cat().

This cat() is taken from WealAlg.java.

> 
> - There is no need to recreate root.jks and root.pem.

The sequences of the commands used in this test scenario allows me to test 
-printcert for the -trustcacerts and -keytsore options. We had discussion 
offline about it. The test uses trusted certificates and checks no warnings on 
the weak algorithms to address the requirement described in the bug. I believe 
it does serve that purpose, and looks legitimate to me. There could be 
different ways of testing a functionality, and please let me know if there is a 
problem with the current approach.

Please also elaborate your comment about no need to recreate root.jks and 
root.pem.

> 
> - Why not use -trustcacerts below?
> 
> 160 kt("-importcert -file server.pem -noprompt", "server.jks”);


Because here is to import the server (end-entity) cert, and it will not make a 
difference for the test result whether to use the -trustcacerts or not. It’s 
the ca (intermediate) cert needs to have it in this test scenario. I intended 
to leave it out in #160 to distinguish between server and ca certs.

> 
> - It's probably better to add a " " between cmd and options in patchcmd(). 
> Same in TrustedCRL.java.

Ok, will change it.

> 
> In TrustedCRL.java:
> 
> - No need to recreate ks and ca.crl. Just call "-printcrl" with different 
> options.

Same reply as above.

> 
> - Why create using MD5withRSA? Do you meant to warn about the weak algorithm?

Yes, exactly, and it differentiates from the weak algorithm SHA1withRSA used in 
root CA where no warning will be emitted. There is another -gencrl in #119 
without using MD5withRSA so I’d have two test cases.

> 
> Also I would suggest you create a dedicate method (maybe in 
> SecurityTools.java) to create your own cacerts. There is no need to copy over 
> the system cacerts, just make sure the file is created with the JKS 
> storetype. We are thinking of upgrading the storetype of cacerts and it's 
> nice to do this at a single place so we can modify it easily later.

I created a method in SecurityTools.java to create the own cacerts. With this 
keystore, the subsequent importing a certificate reply would not work. It turns 
out that its caks.size() is zero detected at establishCertChain() in 
keytool/Main.java after root cert has been imported to that cacerts. At this 
point I’d like to suggest a separate bug be filed to cover the cacerts 
enhancement that you suggested.

Thanks,
Hai-May


> Thanks,
> Max
> 
> 
>> On Jun 2, 2020, at 2:37 AM, Hai-May Chao  wrote:
>> 
>> Hi,
>> 
>> I’d like to request a review for:
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244148
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8246269
>> Webrev: http://cr.openjdk.java.net/~hchao/8244148/webrev.00/
>> 
>> The change is to add the support of -trustcacerts and -keystore options to 
>> -printcert and -princrl command for keytool. This enables keytool to use the 
>> trusted certificates when verifying untrusted artifacts that are signed by 
>> CAs. It also incorporates Max’s change that consolidates the code to get the 
>> default location of cacerts keystore.
>> 
>> Thanks,
>> Hai-May
>> 
> 

Re: RFR 8247960: jarsigner says "signer errors" for some normal warnings when -strict is set

[Hai-May Chao]

> On Jul 24, 2020, at 6:54 AM, Weijun Wang  wrote:
> 
> I'd suggest changing
> 
> +result = isSigning ? rb.getString("jar.signed.") : 
> rb.getString("jar.verified.");
> +if ((strict) && (!errors.isEmpty())) {
> +result = isSigning ? 
> rb.getString("jar.signed.with.signer.errors.")
> + : rb.getString("jar.verified.with.signer.errors.");
> +}
> 
> to
> 
> +if ((strict) && (!errors.isEmpty())) {
> +result = isSigning ? 
> rb.getString("jar.signed.with.signer.errors.")
> + : rb.getString("jar.verified.with.signer.errors.");
> +} else {
> +result = isSigning ? rb.getString("jar.signed.") : 
> rb.getString("jar.verified.");
> +}
> 

Webrev updated as suggested.

> Everything else looks fine.
> 
> Also, I remember you meant to fix 2 bugs with a single changeset. What should 
> the full commit message be?

Fix in a single changeset, so use this bug as the commit message please.

Thanks,
Hai-May

> 
> Thanks,
> Max
> 
>> On Jul 24, 2020, at 4:25 PM, Hai-May Chao  wrote:
>> 
>> 
>> 
>>> On Jul 15, 2020, at 12:16 AM, Weijun Wang  wrote:
>>> 
>>> The following lines are useless now:
>>> 
>> 
>> Ok, this is a separate problem from the original bug to be addressed. 
>> Cleanup/refactoring of the checking on flags is also included in this 
>> changeset.
>> 
>>> 1053 if (badKeyUsage || badExtendedKeyUsage || badNetscapeCertType 
>>> ||
>>> 1054 notYetValidCert || chainNotValidated || hasExpiredCert 
>>> ||
>>> 1055 hasUnsignedEntry || signerSelfSigned || (legacyAlg != 
>>> 0) ||
>>> 1056 (disabledAlg != 0) || aliasNotInStore || 
>>> notSignedByAlias ||
>>> 1057 tsaChainNotValidated ||
>>> 1058 (hasExpiredTsaCert && !signerNotExpired)) {
>>> 
>>> 1198 }
>>> 
>>> 1205 if (hasExpiringCert ||
>>> 1206 (hasExpiringTsaCert  && expireDate != null) ||
>>> 1207 (noTimestamp && expireDate != null) ||
>>> 1208 (hasExpiredTsaCert && signerNotExpired)) {
>>> 
>>> 1245     }
>>> 
>>> I would even suggest you remove the "result" variable and move the 
>>> "System.out.println(result)" line into branches of the if-else block on 
>>> lines 1254-1272.
>> 
>> Current change has the checking for sign and verify. Keep it as-is that you 
>> agreed.
>> 
>> https://cr.openjdk.java.net/~hchao/8247960/webrev.01/
>> 
>> Thanks,
>> Hai-May
>> 
>> 
>>> 
>>> No other comments.
>>> 
>>> Thanks,
>>> Max
>>> 
>>> 
>>> 
>>>> On Jul 15, 2020, at 4:09 AM, Hai-May Chao  wrote:
>>>> 
>>>> Hi,
>>>> 
>>>> I’d like to request a review for:
>>>> 
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8247960
>>>> Webrev: https://cr.openjdk.java.net/~hchao/8247960/webrev.00/
>>>> 
>>>> Jarsigner is changed to emit “with signer errors” only when there are 
>>>> errors detected during sign and verify with -strict specified.
>>>> 
>>>> Thanks,
>>>> Hai-May
> 

Re: RFR 8247960: jarsigner says "signer errors" for some normal warnings when -strict is set

[Hai-May Chao]

> On Jul 15, 2020, at 12:16 AM, Weijun Wang  wrote:
> 
> The following lines are useless now:
> 

Ok, this is a separate problem from the original bug to be addressed. 
Cleanup/refactoring of the checking on flags is also included in this changeset.

> 1053 if (badKeyUsage || badExtendedKeyUsage || badNetscapeCertType ||
> 1054 notYetValidCert || chainNotValidated || hasExpiredCert ||
> 1055 hasUnsignedEntry || signerSelfSigned || (legacyAlg != 0) 
> ||
> 1056 (disabledAlg != 0) || aliasNotInStore || 
> notSignedByAlias ||
> 1057 tsaChainNotValidated ||
> 1058 (hasExpiredTsaCert && !signerNotExpired)) {
> 
> 1198 }
> 
> 1205 if (hasExpiringCert ||
> 1206 (hasExpiringTsaCert  && expireDate != null) ||
> 1207 (noTimestamp && expireDate != null) ||
> 1208 (hasExpiredTsaCert && signerNotExpired)) {
> 
> 1245 }
> 
> I would even suggest you remove the "result" variable and move the 
> "System.out.println(result)" line into branches of the if-else block on lines 
> 1254-1272.

Current change has the checking for sign and verify. Keep it as-is that you 
agreed.

https://cr.openjdk.java.net/~hchao/8247960/webrev.01/

Thanks,
Hai-May


> 
> No other comments.
> 
> Thanks,
> Max
> 
> 
> 
>> On Jul 15, 2020, at 4:09 AM, Hai-May Chao  wrote:
>> 
>> Hi,
>> 
>> I’d like to request a review for:
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8247960
>> Webrev: https://cr.openjdk.java.net/~hchao/8247960/webrev.00/
>> 
>> Jarsigner is changed to emit “with signer errors” only when there are errors 
>> detected during sign and verify with -strict specified.
>> 
>> Thanks,
>> Hai-May
>> 
> 

RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol…

[Hai-May Chao]

This enhancement adds support for the nonce extension in OCSP request 
extensions by system property jdk.security.certpath.ocspNonce.

Please review the CSR at:
https://bugs.openjdk.java.net/browse/JDK-8257766

-

Commit messages:
 - 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) 
Nonce Extension

Changes: https://git.openjdk.java.net/jdk/pull/2039/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2039=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256895
  Stats: 108 lines in 6 files changed: 89 ins; 1 del; 18 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2039.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2039/head:pull/2039

PR: https://git.openjdk.java.net/jdk/pull/2039

RFR: 8259401: Add checking to jarsigner to warn weak algorithms used in si…

[Hai-May Chao]

The jarsigner tool currently provides warning associated with the signer’s cert 
when it uses weak algorithms, but not for the CA certs. This change is to 
process the signer’s cert chain to warn if CA certs use weak algorithms.

-

Commit messages:
 - 8259401: Add checking to jarsigner to warn weak algorithms used in signer’s 
cert chain

Changes: https://git.openjdk.java.net/jdk/pull/2042/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2042=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8259401
  Stats: 125 lines in 2 files changed: 124 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2042.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2042/head:pull/2042

PR: https://git.openjdk.java.net/jdk/pull/2042

Re: RFR: 8257788: Class fields could be local in the SunJSSE provider

[Hai-May Chao]

On Fri, 4 Dec 2020 22:58:07 GMT, Xue-Lei Andrew Fan  wrote:

> In the SunJSSE provider implementation, there are a few class fields are not 
> used other than the constructors. Those fields could be removed and replaced 
> with local variables.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8257788

Looks good to me.

-

PR: https://git.openjdk.java.net/jdk/pull/1634

RFR: 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch t…

[Hai-May Chao]

This is a spec change with noreg-doc label.

-

Commit messages:
 - 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its 
behavior

Changes: https://git.openjdk.java.net/jdk/pull/1701/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1701=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8246005
  Stats: 2 lines in 2 files changed: 2 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1701.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1701/head:pull/1701

PR: https://git.openjdk.java.net/jdk/pull/1701

Re: RFR: 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch t… [v2]

[Hai-May Chao]

On Tue, 8 Dec 2020 18:08:38 GMT, Xue-Lei Andrew Fan  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated with implSpec tag
>
> src/java.base/share/classes/java/security/KeyStoreSpi.java line 319:
> 
>> 317:  * @throwsCertificateException if any of the certificates 
>> included in
>> 318:  *  the keystore data could not be stored
>> 319:  * @throwsUnsupportedOperationException if this method is not 
>> supported
> 
> We might be able to get it more clear about the behavior.  For example, 
> adding an implSpec tag, like:
>  * @implSpec The default implementation throws
>  *   an {@link UnsupportedOperationException}.
>  ...
>  * @throws UnsupportedOperationException if the implementation
>  * has not overridden this method

tag implSpec added. Thanks for the review.

-

PR: https://git.openjdk.java.net/jdk/pull/1701

Re: RFR: 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch t… [v2]

[Hai-May Chao]

> This is a spec change with noreg-doc label.

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  Updated with implSpec tag

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1701/files
  - new: https://git.openjdk.java.net/jdk/pull/1701/files/496360a7..a153f4f3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1701=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1701=00-01

  Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1701.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1701/head:pull/1701

PR: https://git.openjdk.java.net/jdk/pull/1701

Re: RFR: 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior [v2]

[Hai-May Chao]

On Tue, 8 Dec 2020 20:16:35 GMT, Sean Mullan  wrote:

>> src/java.base/share/classes/java/security/KeyStoreSpi.java line 322:
>> 
>>> 320:  * @throwsCertificateException if any of the certificates 
>>> included in
>>> 321:  *  the keystore data could not be stored
>>> 322:  * @throws UnsupportedOperationException if the implementation
>> 
>> Please add an extra white space after the "throws" tag so that this line 
>> align with the lines above.
>
> On line 310 and 315, can you also fix the typos as part of this change: 
> s/KeyStore.LoadStoreParmeter/KeyStore.LoadStoreParameter/

Added the white space. Sure, I can include those typos fix in this PR. Also, 
included another typo fix for line 305.

-

PR: https://git.openjdk.java.net/jdk/pull/1701

Re: RFR: 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior [v2]

[Hai-May Chao]

On Tue, 8 Dec 2020 20:24:51 GMT, Sean Mullan  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated with implSpec tag
>
> src/java.base/share/classes/java/security/KeyStoreSpi.java line 323:
> 
>> 321:  *  the keystore data could not be stored
>> 322:  * @throws UnsupportedOperationException if the implementation
>> 323:  *  has not overridden this method
> 
> I think we should keep this more general and just say "If the implementation 
> does not support this operation". There may be legitimate cases in which an 
> implementation may override this method but throw 
> UnsupportedOperationException if it doesn't support particular aspects of the 
> LoadStoreParameter.

Good point. Fixed as suggested.

> src/java.base/share/classes/java/security/KeyStore.java line 1424:
> 
>> 1422:  * @throwsCertificateException if any of the certificates 
>> included in
>> 1423:  *  the keystore data could not be stored
>> 1424:  * @throwsUnsupportedOperationException if this method is not 
>> supported
> 
> I would change "method" to "operation" as it is a bit more consistent with 
> the exception.

Fixed as suggested. Thanks for the review.

-

PR: https://git.openjdk.java.net/jdk/pull/1701

Re: RFR: 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior [v3]

[Hai-May Chao]

> This is a spec change with noreg-doc label.

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  white space after throw tag added

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1701/files
  - new: https://git.openjdk.java.net/jdk/pull/1701/files/a153f4f3..265b7e22

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1701=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1701=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1701.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1701/head:pull/1701

PR: https://git.openjdk.java.net/jdk/pull/1701

Re: RFR: 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior [v4]

[Hai-May Chao]

> This is a spec change with noreg-doc label.

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  updated spec text and included typo fix

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1701/files
  - new: https://git.openjdk.java.net/jdk/pull/1701/files/265b7e22..12db7941

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1701=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1701=02-03

  Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1701.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1701/head:pull/1701

PR: https://git.openjdk.java.net/jdk/pull/1701

Integrated: 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior

[Hai-May Chao]

On Tue, 8 Dec 2020 17:52:34 GMT, Hai-May Chao  wrote:

> This is a spec change with noreg-doc label.

This pull request has now been integrated.

Changeset: b0b70df4
Author:    Hai-May Chao 
Committer: Xue-Lei Andrew Fan 
URL:   https://git.openjdk.java.net/jdk/commit/b0b70df4
Stats: 9 lines in 2 files changed: 6 ins; 0 del; 3 mod

8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its 
behavior

Reviewed-by: xuelei

-

PR: https://git.openjdk.java.net/jdk/pull/1701

Re: RFR: 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior [v2]

[Hai-May Chao]

On Tue, 8 Dec 2020 18:52:05 GMT, Sean Mullan  wrote:

>> Marked as reviewed by xuelei (Reviewer).
>
> This will also require a CSR since you are making some specification changes. 
> I'm not sure if you were trying to get this into 16, but it is probably too 
> late to make JDK 16 since RDP is a couple of days away. You can push to 
> 17/latest once 16 forks and CSR is approved.

Please review the CSR (JDK-8258114) at:
https://bugs.openjdk.java.net/browse/JDK-8258114

-

PR: https://git.openjdk.java.net/jdk/pull/1701

Re: RFR: 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior [v2]

[Hai-May Chao]

On Fri, 11 Dec 2020 18:35:27 GMT, Hai-May Chao  wrote:

>> This will also require a CSR since you are making some specification 
>> changes. I'm not sure if you were trying to get this into 16, but it is 
>> probably too late to make JDK 16 since RDP is a couple of days away. You can 
>> push to 17/latest once 16 forks and CSR is approved.
>
> Please review the CSR (JDK-8258114) at:
> https://bugs.openjdk.java.net/browse/JDK-8258114

Thanks for the review. Updated CSR to backtick the 
UnsupportedOperationException text. I thought about placing the diff in 
Specification section, but decided to keep the document change in this format 
which is same as our javadoc API specification. I thought it'd be more clear. 
I'm open to make a change if diff format is required.

-

PR: https://git.openjdk.java.net/jdk/pull/1701

Re: RFR: 8253299: Manifest bytes are read twice when verifying a signed JAR

[Hai-May Chao]

On Thu, 19 Nov 2020 17:13:00 GMT, Lance Andersen  wrote:

>> Small change to retrieve the raw bytes of manifest during verifying signed 
>> JAR.
>
> Marked as reviewed by lancea (Reviewer).

Thank you all for the review. I added the noreg-trivial label to the bug.

-

PR: https://git.openjdk.java.net/jdk/pull/1299

Re: RFR: 8253299: Manifest bytes are read twice when verifying a signed JAR

[Hai-May Chao]

On Thu, 19 Nov 2020 17:20:58 GMT, Hai-May Chao  wrote:

>> Marked as reviewed by lancea (Reviewer).
>
> Thank you all for the review. I added the noreg-trivial label to the bug.

Lance, I've entered /integrate. Thank you for sponsoring this!

-

PR: https://git.openjdk.java.net/jdk/pull/1299

Integrated: 8253299: Manifest bytes are read twice when verifying a signed JAR

[Hai-May Chao]

On Wed, 18 Nov 2020 21:59:01 GMT, Hai-May Chao  wrote:

> Small change to retrieve the raw bytes of manifest during verifying signed 
> JAR.

This pull request has now been integrated.

Changeset: 9bb82232
Author:    Hai-May Chao 
Committer: Lance Andersen 
URL:   https://git.openjdk.java.net/jdk/commit/9bb82232
Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod

8253299: Manifest bytes are read twice when verifying a signed JAR

Reviewed-by: redestad, lancea, alanb

-

PR: https://git.openjdk.java.net/jdk/pull/1299

RFR: 8253299: Manifest bytes are read twice when verifying a signed JAR

[Hai-May Chao]

Small change to retrieve the raw bytes of manifest during verifying signed JAR.

-

Commit messages:
 - 8253299: Manifest bytes are read twice when verifying a signed JAR

Changes: https://git.openjdk.java.net/jdk/pull/1299/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1299=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8253299
  Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1299.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1299/head:pull/1299

PR: https://git.openjdk.java.net/jdk/pull/1299

Re: RFR: 8244154: Update SunPKCS11 provider with PKCS11 v3.0 header files

[Hai-May Chao]

On Wed, 28 Oct 2020 21:35:25 GMT, Valerie Peng  wrote:

> Could someone please help review this PKCS#11 v3.0 header files update?
> 
> Changes are straight-forward as below:
> 1) Updated pkcs11.h, pkcs11f.h, pkcs11t.h to v3.0
> 2) Updated java side w/ the new constants definitions and name/error code 
> mappings.
> 
> For the native headers, it's a direct copy of the official v3.0 headers 
> except that I have to remove the tab space, and trailing white spaces due to 
> JDK code requirement. I verified the result using 'diff -w'. As for the java 
> side, the edit is based on the diff of native headers. I also commented out 
> some of the unused native identifiers at java side.
> 
> I am adding the SHA-3 digests, signatures, and macs in a separate RFE and 
> would need this one to be reviewed/integrated first.
> 
> Thanks,
> Valerie

Changes look good. Only minor comments.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/Functions.java
 line 793:

> 791: addMech(CKM_SHA3_512_RSA_PKCS_PSS,  
> "CKM_SHA3_512_RSA_PKCS_PSS");
> 792: addMech(CKM_SHA3_224_RSA_PKCS,  "CKM_SHA3_224_RSA_PKCS");
> 793: addMech(CKM_SHA3_224_RSA_PKCS_PSS,  
> "CKM_SHA3_224_RSA_PKCS_PSS");

It appears that you're arranging the addMech(with CKM_xxx) based on the 
mechanism values. How about the code from #773 to #793, move it up?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/Functions.java
 line 1095:

> 1093: addMech(CKM_SP800_108_FEEDBACK_KDF, 
> "CKM_SP800_108_FEEDBACK_KDF");
> 1094: addMech(CKM_SP800_108_DOUBLE_PIPELINE_KDF,
> 1095:  
> "CKM_SP800_108_DOUBLE_PIPELINE_KDF");

same comment as above.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11Constants.java
 line 987:

> 985: public static final long  CKM_SP800_108_FEEDBACK_KDF = 
> 0x03adL;
> 986: public static final long  CKM_SP800_108_DOUBLE_PIPELINE_KDF = 
> 0x03aeL;
> 987: 

Same comment.

-

PR: https://git.openjdk.java.net/jdk/pull/917

Re: RFR: 8255494: PKCS7 should use digest algorithm to verify the signature

[Hai-May Chao]

On Wed, 28 Oct 2020 21:01:44 GMT, Weijun Wang  wrote:

> This is a regression made by 
> [JDK-8242068](https://bugs.openjdk.java.net/browse/JDK-8242068). When the 
> digest algorithm is not the same as the hash part of the signature algorithm, 
> we used to combine the digest algorithm with the key part of the signature 
> algorithm into a new signature algorithm and use it when generating a 
> signature. The previous code change uses the signature algorithm in the 
> SignerInfo directly. This bugfix will revert to the old behavior.

Looks good!

-

PR: https://git.openjdk.java.net/jdk/pull/916

Re: RFR: 8244154: Update SunPKCS11 provider with PKCS11 v3.0 header files

[Hai-May Chao]

On Fri, 30 Oct 2020 21:39:42 GMT, Valerie Peng  wrote:

>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/Functions.java
>>  line 1095:
>> 
>>> 1093: addMech(CKM_SP800_108_FEEDBACK_KDF, 
>>> "CKM_SP800_108_FEEDBACK_KDF");
>>> 1094: addMech(CKM_SP800_108_DOUBLE_PIPELINE_KDF,
>>> 1095:  
>>> "CKM_SP800_108_DOUBLE_PIPELINE_KDF");
>> 
>> same comment as above.
>
> Well, per the ordering in PKCS11Constants, these three lines are at the right 
> place. Note that the ordering of CKM_ECDSA_SHA3_224 to CKM_EDDSA in pkcs11t.h 
> is different from PKCS11Constants class, so I will add the comment about the 
> general ordering following PKCS11Constants class and keep them here.

Sounds good that a comment will be added.

-

PR: https://git.openjdk.java.net/jdk/pull/917

Re: RFR: 8244154: Update SunPKCS11 provider with PKCS11 v3.0 header files

[Hai-May Chao]

On Fri, 30 Oct 2020 21:44:00 GMT, Valerie Peng  wrote:

>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11Constants.java
>>  line 987:
>> 
>>> 985: public static final long  CKM_SP800_108_FEEDBACK_KDF = 
>>> 0x03adL;
>>> 986: public static final long  CKM_SP800_108_DOUBLE_PIPELINE_KDF = 
>>> 0x03aeL;
>>> 987: 
>> 
>> Same comment.
>
> These three are just by themselves, so unless you feel strongly about, I 
> prefer just leave them here which matches the ordering of pkcs11t.h, i.e. 
> right before the CKM_VENDOR_DEFINED line.

Just thought they could be moved like CKM_ECDSA_SHA3_224 to CKM_EDDSA (which 
are not matching to the ordering of pkcs11t.h), so we keep the ordering in 
PKCS11Constants. It's fine if you'd prefer as is.

-

PR: https://git.openjdk.java.net/jdk/pull/917

Re: RFR: 8244154: Update SunPKCS11 provider with PKCS11 v3.0 header files [v3]

[Hai-May Chao]

On Wed, 4 Nov 2020 21:06:35 GMT, Weijun Wang  wrote:

>> Valerie Peng has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated the javadoc comments of PKCS11Constants class with additional
>>   typedef info
>>   Updated the legal file to new PKCS11 version
>
> Marked as reviewed by weijun (Reviewer).

Incr webrev looks good.

-

PR: https://git.openjdk.java.net/jdk/pull/917

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension [v4]

[Hai-May Chao]

> This enhancement adds support for the nonce extension in OCSP request 
> extensions by system property jdk.security.certpath.ocspNonce.
> 
> Please review the CSR at:
> https://bugs.openjdk.java.net/browse/JDK-8257766

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  Nonce creation is done in checkOCSP method

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2039/files
  - new: https://git.openjdk.java.net/jdk/pull/2039/files/949ee2be..da6e5bab

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2039=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2039=02-03

  Stats: 68 lines in 1 file changed: 36 ins; 31 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2039.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2039/head:pull/2039

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension [v3]

[Hai-May Chao]

> This enhancement adds support for the nonce extension in OCSP request 
> extensions by system property jdk.security.certpath.ocspNonce.
> 
> Please review the CSR at:
> https://bugs.openjdk.java.net/browse/JDK-8257766

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  Revert changes to UnitTest.java

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2039/files
  - new: https://git.openjdk.java.net/jdk/pull/2039/files/26254d59..949ee2be

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2039=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2039=01-02

  Stats: 43 lines in 1 file changed: 0 ins; 41 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2039.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2039/head:pull/2039

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension [v7]

[Hai-May Chao]

> This enhancement adds support for the nonce extension in OCSP request 
> extensions by system property jdk.security.certpath.ocspNonce.
> 
> Please review the CSR at:
> https://bugs.openjdk.java.net/browse/JDK-8257766

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  Set tmpExtensions to null instead of using emptyList()

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2039/files
  - new: https://git.openjdk.java.net/jdk/pull/2039/files/b4b3a485..b334a5d0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2039=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2039=05-06

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2039.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2039/head:pull/2039

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension [v6]

[Hai-May Chao]

On Wed, 20 Jan 2021 21:40:12 GMT, Sean Mullan  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Change to save memory by List.of
>
> src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>  line 749:
> 
>> 747: }
>> 748: 
>> 749: List tmpExtensions = 
>> Collections.emptyList();
> 
> One other comment - I think you can just set this to null.

Fixed.

-

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension [v5]

[Hai-May Chao]

On Wed, 20 Jan 2021 20:57:49 GMT, Sean Mullan  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Add nonce to the list of extensions
>
> src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>  line 762:
> 
>> 760: }
>> 761: 
>> 762: tmpExtensions.add(nonceExt);
> 
> If you only need the nonce, you could use List.of and save a little bit of 
> memory, ex:
> 
> if (ocspExtensions.size() > 0) {
> tmpExtensions = new 
> ArrayList(ocspExtensions);
> tmpExtensions.add(nonceExt);
> } else {
> tmpExtensions = List.of(nonceExt);
> }

Thanks for the review. Updated as suggested.

-

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension [v4]

[Hai-May Chao]

On Wed, 20 Jan 2021 13:46:58 GMT, Sean Mullan  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Nonce creation is done in checkOCSP method
>
> src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>  line 755:
> 
>> 753: // create the 16-byte nonce by default
>> 754: Extension nonceExt = new 
>> OCSPNonceExtension(DEFAULT_NONCE_BYTES);
>> 755: tmpExtensions.add(nonceExt);
> 
> I think you should add the OCSPNonce extension to the list of extensions that 
> the application passed in, as there may be other extensions that have been 
> specified and should be sent in the OCSP response, ex:
> 
> `ocspExtensions.add(new OCSPNonceExtension(DEFAULT_NONCE_BYTES));`
> 
> This means you don't need the `tmpExtensions` variable.

During testing, I got the 
"java.base/java.util.Collections$UnmodifiableCollection.add(Collections.java:1062)
 exception with this line of change. I've changed to use a tmpExtensions 
variable when setting the OCSP nonce to the extension sets instead of modifying 
the ocspExtensions.

> src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>  line 779:
> 
>> 777: response = OCSP.check(Collections.singletonList(certId),
>> 778: responderURI, issuerInfo, responderCert, null,
>> 779: rp.ocspNonce ? tmpExtensions : ocspExtensions, 
>> params.variant());
> 
> Here you can just pass in `ocspExtensions` since it will contain the nonce if 
> the property has been set.

No change as tmpExtensions is needed.

-

PR: https://git.openjdk.java.net/jdk/pull/2039

Integrated: 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension

[Hai-May Chao]

On Mon, 11 Jan 2021 21:41:56 GMT, Hai-May Chao  wrote:

> This enhancement adds support for the nonce extension in OCSP request 
> extensions by system property jdk.security.certpath.ocspNonce.
> 
> Please review the CSR at:
> https://bugs.openjdk.java.net/browse/JDK-8257766

This pull request has now been integrated.

Changeset: 8b95d954
Author:    Hai-May Chao 
Committer: Sean Mullan 
URL:   https://git.openjdk.java.net/jdk/commit/8b95d954
Stats: 83 lines in 5 files changed: 59 ins; 6 del; 18 mod

8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) 
Nonce Extension

Reviewed-by: jnimeh, mullan

-

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension [v4]

[Hai-May Chao]

On Wed, 20 Jan 2021 13:41:04 GMT, Sean Mullan  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Nonce creation is done in checkOCSP method
>
> src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>  line 762:
> 
>> 760: } catch (IOException e) {
>> 761: throw new 
>> CertPathValidatorException("Failed to create the default nonce " +
>> 762: "in OCSP entensions");
> 
> Typo: s/entensions/extensions/
> 
> Also, use the `CertPathValidatorException(String, Throwable)` ctor instead 
> and pass the `IOException` as the 2nd parameter.

Fixed.

-

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension [v5]

[Hai-May Chao]

> This enhancement adds support for the nonce extension in OCSP request 
> extensions by system property jdk.security.certpath.ocspNonce.
> 
> Please review the CSR at:
> https://bugs.openjdk.java.net/browse/JDK-8257766

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  Add nonce to the list of extensions

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2039/files
  - new: https://git.openjdk.java.net/jdk/pull/2039/files/da6e5bab..a14a2bdb

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2039=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2039=03-04

  Stats: 9 lines in 1 file changed: 7 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2039.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2039/head:pull/2039

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension [v6]

[Hai-May Chao]

> This enhancement adds support for the nonce extension in OCSP request 
> extensions by system property jdk.security.certpath.ocspNonce.
> 
> Please review the CSR at:
> https://bugs.openjdk.java.net/browse/JDK-8257766

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  Change to save memory by List.of

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2039/files
  - new: https://git.openjdk.java.net/jdk/pull/2039/files/a14a2bdb..b4b3a485

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2039=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2039=04-05

  Stats: 4 lines in 1 file changed: 1 ins; 2 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2039.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2039/head:pull/2039

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension [v2]

[Hai-May Chao]

On Thu, 14 Jan 2021 14:35:25 GMT, Sean Mullan  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   update to use List.of() and typo changes
>
> src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>  line 88:
> 
>> 86: boolean ocspNonce;
>> 87: }
>> 88: private RevocationProperties rp;
> 
> I think this field could be `final`.

No change made due to getting an error: cannot assign a value to final variable 
rp.

> src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>  line 107:
> 
>> 105: 
>> 106: private void setDefaultNonce() {
>> 107: byte[] nonce = null;
> 
> This variable looks like it is not used and can be removed.

The setDefaultNonce() method is removed.

> src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>  line 109:
> 
>> 107: byte[] nonce = null;
>> 108: 
>> 109: // Set the nonce by default in OCSP request extension when the 
>> sytem property
> 
> Typo: s/sytem/system/

The setDefaultNonce() method is removed as creating nonce is done in 
checkOCSP() method now.

> src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>  line 113:
> 
>> 111: if (rp.ocspNonce) {
>> 112: try {
>> 113: setOcspExtensions(List.of(new 
>> OCSPNonceExtension(DEFAULT_NONCE_BYTES)));
> 
> I don't think we should use the `PKIXRevocationChecker.setOcspExtensions()` 
> API to add an OCSP Nonce extension. That API is intended to be called by 
> applications. I think the Nonce extension should be set by the implementation 
> only and not exposed via the standard API. Also, a nonce should be unique for 
> each OCSP request, but setting it here means that it could re-use the same 
> nonce for different OCSP requests.
> 
> I think a better place to create/add the OCSPExtension is in the `checkOCSP` 
> method, and the extension should be created each time that method is called 
> (if the system property is enabled), so a new nonce is created for each OCSP 
> request.

Creating the nonce is moved to checkOCSP() method.

-

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8260286: Manual Test "ws/open/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.java" fails

[Hai-May Chao]

On Mon, 25 Jan 2021 17:08:45 GMT, Fernando Guallini  
wrote:

> Fixing manual Test 
> "ws/open/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.java".
>  It was not handling "weak algorithm" warning during jarsigner output 
> verification

Change copyright year to 2021

-

PR: https://git.openjdk.java.net/jdk/pull/2224

Re: RFR: 8260286: Manual Test "ws/open/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.java" fails

[Hai-May Chao]

On Mon, 25 Jan 2021 22:45:31 GMT, Hai-May Chao  wrote:

>> Marked as reviewed by rhalade (Reviewer).
>
> Looks good.

One comment: Add bug id to the changed test. Thanks.

-

PR: https://git.openjdk.java.net/jdk/pull/2224

Re: RFR: 8260286: Manual Test "ws/open/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.java" fails

[Hai-May Chao]

On Mon, 25 Jan 2021 21:51:19 GMT, Rajan Halade  wrote:

>> Fixing manual Test 
>> "ws/open/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.java".
>>  It was not handling "weak algorithm" warning during jarsigner output 
>> verification
>
> Marked as reviewed by rhalade (Reviewer).

Looks good.

-

PR: https://git.openjdk.java.net/jdk/pull/2224

Re: RFR: 8259401: Add checking to jarsigner to warn weak algorithms used in si… [v2]

[Hai-May Chao]

On Wed, 13 Jan 2021 20:25:53 GMT, Sean Mullan  wrote:

>> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 
>> 1484:
>> 
>>> 1482: // If the cert is trusted, only check its key size, 
>>> but not its
>>> 1483: // signature algorithm. This is because warning 
>>> should not be
>>> 1484: // generated for SHA-1 roots which are not an issue.
>> 
>> SHA-1 is just a glitch in the long history at this very moment, and thus I 
>> think it's inappropriate to mention it in the source code. In my opinion, 
>> the general reason we don't check the signature is that we trust its origin 
>> anyway and we don't verify the signature at all (do we?). On the other hand, 
>> since its key is used to sign other certs, we need to make sure the key size 
>> is big enough so that no one else is able to recover the key and use it to 
>> sign other certs.
>
> Yes, I would remove the 2nd sentence that starts with "This is ...". There 
> are plenty of references on the Internet which explain this, so no need to 
> add much detail.

Removed.

-

PR: https://git.openjdk.java.net/jdk/pull/2042

Re: RFR: 8259401: Add checking to jarsigner to warn weak algorithms used in signer’s cert chain [v2]

[Hai-May Chao]

On Wed, 13 Jan 2021 20:26:17 GMT, Sean Mullan  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   No warning for trusted cert's SHA1, and added debug output to test
>
> Marked as reviewed by mullan (Reviewer).

Thanks for the review, Max. The comment about trusted cert's sigalg checking 
has been removed.

-

PR: https://git.openjdk.java.net/jdk/pull/2042

Re: RFR: 8259401: Add checking to jarsigner to warn weak algorithms used in signer’s cert chain [v2]

[Hai-May Chao]

On Wed, 13 Jan 2021 15:17:22 GMT, Weijun Wang  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   No warning for trusted cert's SHA1, and added debug output to test
>
> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 
> 1491:
> 
>> 1489: .append(checkWeakKey(key));
>> 1490: 
>> 1491: certStr.append("\n").append(tab).append("[");
> 
> It's a little strange to leave the other half of the bracket 
> (`certStr.append("]");` on line 1568) outside the if-else block. Can you 
> please move it inside? Of course you will have to duplicate them.

I'd prefer to keep it as is instead of duplicating the code in several places 
inside if block and else block.

-

PR: https://git.openjdk.java.net/jdk/pull/2042

Integrated: 8259401: Add checking to jarsigner to warn weak algorithms used in signer’s cert chain

[Hai-May Chao]

On Tue, 12 Jan 2021 03:34:00 GMT, Hai-May Chao  wrote:

> The jarsigner tool currently provides warning associated with the signer’s 
> cert when it uses weak algorithms, but not for the CA certs. This change is 
> to process the signer’s cert chain to warn if CA certs use weak algorithms.

This pull request has now been integrated.

Changeset: c7e2174b
Author:    Hai-May Chao 
Committer: Weijun Wang 
URL:   https://git.openjdk.java.net/jdk/commit/c7e2174b
Stats: 143 lines in 2 files changed: 140 ins; 0 del; 3 mod

8259401: Add checking to jarsigner to warn weak algorithms used in signer’s 
cert chain

Reviewed-by: mullan, weijun, rhalade

-

PR: https://git.openjdk.java.net/jdk/pull/2042

Re: RFR: 8259401: Add checking to jarsigner to warn weak algorithms used in si… [v3]

[Hai-May Chao]

> The jarsigner tool currently provides warning associated with the signer’s 
> cert when it uses weak algorithms, but not for the CA certs. This change is 
> to process the signer’s cert chain to warn if CA certs use weak algorithms.

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  remove comment about checking sigalg for trusted cert

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2042/files
  - new: https://git.openjdk.java.net/jdk/pull/2042/files/5a3e184a..88d94dfd

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2042=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2042=01-02

  Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2042.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2042/head:pull/2042

PR: https://git.openjdk.java.net/jdk/pull/2042

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol… [v2]

[Hai-May Chao]

> This enhancement adds support for the nonce extension in OCSP request 
> extensions by system property jdk.security.certpath.ocspNonce.
> 
> Please review the CSR at:
> https://bugs.openjdk.java.net/browse/JDK-8257766

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  update to use List.of() and typo changes

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2039/files
  - new: https://git.openjdk.java.net/jdk/pull/2039/files/5ade73d4..26254d59

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2039=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2039=00-01

  Stats: 10 lines in 2 files changed: 0 ins; 5 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2039.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2039/head:pull/2039

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol… [v2]

[Hai-May Chao]

On Tue, 12 Jan 2021 20:10:34 GMT, Rajan Halade  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   update to use List.of() and typo changes
>
> test/jdk/security/infra/java/security/cert/CertPathValidator/certification/SSLCA.java
>  line 50:
> 
>> 48: public static void main(String[] args) throws Exception {
>> 49: 
>> 50: System.setProperty("jdk.security.certpath.ocspNonce", "true");
> 
> Trying to understand this change - why do we need this change and only in 
> couple of tests? Did these test fail with your change?

No, these test did not fail with my changes. These two tests are being changed 
to set the system property jdk.security.certpath.ocspNonce=true in order to 
send the nonce as the responders in these tests will take the nonce and return 
it their responses, so we could test the benefit of having nonce extension set.

-

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol… [v2]

[Hai-May Chao]

On Tue, 12 Jan 2021 16:26:11 GMT, Jamil Nimeh  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   update to use List.of() and typo changes
>
> In general it looks pretty good.  Just a couple small comments.

Thanks for the review, Jamil. I've updated with all of your comments.

> src/java.base/share/classes/sun/security/provider/certpath/OCSPNonceExtension.java
>  line 126:
> 
>> 124:  *  is set for this extension
>> 125:  * @param incomingNonce The nonce data to be set for the extension. 
>>  This
>> 126:  *  must be a non-null array of at least one byte long and can 
>> be upto
> 
> typo: "upto" -> "up to"

fixed.

> src/java.base/share/classes/sun/security/provider/certpath/OCSPNonceExtension.java
>  line 143:
> 
>> 141: // RFC 8954, section 2.1: the length of the nonce MUST be at 
>> least 1 octet
>> 142: // and can be up to 32 octets.
>> 143: if (incomingNonce.length > 0 && incomingNonce.length <=32) {
> 
> nit: space after the <= to be consistent with style elsewhere

fixed.

> src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>  line 118:
> 
>> 116: tmpExtensions = new ArrayList();
>> 117: tmpExtensions.add(nonceExt);
>> 118: setOcspExtensions(tmpExtensions);
> 
> It seems like you could collapse 113 - 118 into something like:
> setOcspExtensions(List.of(new OCSPNonceExtension(DEFAULT_NONCE_BYTES)));
> 
> At the very least, it looks like you could do away with 113, since you 
> immediately change the value of the tmpExtensions reference on 116.

collapsing done.

-

PR: https://git.openjdk.java.net/jdk/pull/2039

Re: RFR: 8259401: Add checking to jarsigner to warn weak algorithms used in si… [v2]

[Hai-May Chao]

> The jarsigner tool currently provides warning associated with the signer’s 
> cert when it uses weak algorithms, but not for the CA certs. This change is 
> to process the signer’s cert chain to warn if CA certs use weak algorithms.

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  No warning for trusted cert's SHA1, and added debug output to test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2042/files
  - new: https://git.openjdk.java.net/jdk/pull/2042/files/eb4f93eb..5a3e184a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2042=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2042=00-01

  Stats: 39 lines in 2 files changed: 25 ins; 8 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2042.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2042/head:pull/2042

PR: https://git.openjdk.java.net/jdk/pull/2042

Re: RFR: 8259401: Add checking to jarsigner to warn weak algorithms used in si… [v2]

[Hai-May Chao]

On Tue, 12 Jan 2021 20:57:41 GMT, Sean Mullan  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   No warning for trusted cert's SHA1, and added debug output to test
>
> Changes requested by mullan (Reviewer).

Thanks for your review, Sean and Rajan. I've updated the webrev with your 
comments.

> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 
> 1404:
> 
>> 1402: }
>> 1403: 
>> 1404: private String checkWeakKey(PublicKey key) {
> 
> Can this method be static?

static added.

> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 
> 1421:
> 
>> 1419: }
>> 1420: 
>> 1421: private String checkWeakAlg(String alg) {
> 
> Can this method be static?

static added.

> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 
> 1483:
> 
>> 1481: certStr.append("\n").append(tab)
>> 1482: .append("Signature algorithm: ")
>> 1483: .append(checkWeakAlg(sigalg))
> 
> If the cert is trusted, I don't think we should print a warning if the 
> signature algorithm is weak. Otherwise this will generate false warnings for 
> SHA-1 roots which are not an issue.  You should check the key size though. 
> And you can still print the signature algorithm. You may need to move line 
> 1489-1490 before this to first determine if the cert is trusted.

Fixed to not check the signature algorithm for a trusted cert, and updated the 
test accordingly.

-

PR: https://git.openjdk.java.net/jdk/pull/2042

Re: RFR: 8259401: Add checking to jarsigner to warn weak algorithms used in si… [v2]

[Hai-May Chao]

On Tue, 12 Jan 2021 22:22:55 GMT, Rajan Halade  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   No warning for trusted cert's SHA1, and added debug output to test
>
> test/jdk/sun/security/tools/jarsigner/CheckSignerCertChain.java line 52:
> 
>> 50: public static void main(String[] args) throws Exception {
>> 51: 
>> 52: // root certificate using SHA1withRSA and 1024-bit key
> 
> It will be helpful to have these comments logged as debug messages with 
> System.out

comment messages added.

-

PR: https://git.openjdk.java.net/jdk/pull/2042

Re: RFR: 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior [v2]

[Hai-May Chao]

On Fri, 11 Dec 2020 19:59:17 GMT, Xue-Lei Andrew Fan  wrote:

>> Thanks for the review. Updated CSR to backtick the 
>> UnsupportedOperationException text. I thought about placing the diff in 
>> Specification section, but decided to keep the document change in this 
>> format which is same as our javadoc API specification. I thought it'd be 
>> more clear. I'm open to make a change if diff format is required.
>
> It may improve the readability to have the context specification of the 
> update.  For example, have the full specification of the method, and mark the 
> update of the spec, and the signature of the method.  A diff is often used as 
> it is easier to have the context in the CSR request.

Thanks for the CSR review. Updated CSR to have diff on the full spec of the 
methods.

-

PR: https://git.openjdk.java.net/jdk/pull/1701

Re: RFR: 8256421: Add 2 HARICA roots to Oracle Root CA Program

[Hai-May Chao]

On Thu, 28 Jan 2021 01:22:52 GMT, Rajan Halade  wrote:

> Following two roots are added to cacerts store -
> 
> CN=Hellenic Academic and Research Institutions RootCA 2015, O=Hellenic 
> Academic and Research Institutions Cert. Authority, L=Athens, C=GR
> 
> CN=Hellenic Academic and Research Institutions ECC RootCA 2015, O=Hellenic 
> Academic and Research Institutions Cert. Authority, L=Athens, C=GR
> 
> Release Note is at - https://bugs.openjdk.java.net/browse/JDK-8260597

Marked as reviewed by hchao (Author).

-

PR: https://git.openjdk.java.net/jdk/pull/2272

Re: RFR: 8256421: Add 2 HARICA roots to Oracle Root CA Program

[Hai-May Chao]

On Thu, 28 Jan 2021 01:22:52 GMT, Rajan Halade  wrote:

> Following two roots are added to cacerts store -
> 
> CN=Hellenic Academic and Research Institutions RootCA 2015, O=Hellenic 
> Academic and Research Institutions Cert. Authority, L=Athens, C=GR
> 
> CN=Hellenic Academic and Research Institutions ECC RootCA 2015, O=Hellenic 
> Academic and Research Institutions Cert. Authority, L=Athens, C=GR
> 
> Release Note is at - https://bugs.openjdk.java.net/browse/JDK-8260597

Looks good.

-

PR: https://git.openjdk.java.net/jdk/pull/2272

Re: RFR: 8266182: Create a manual test for jdk/sun/security/pkcs12/ParamsTest.java [v3]

[Hai-May Chao]

On Fri, 18 Jun 2021 13:24:17 GMT, Abdul Kolarkunnu  
wrote:

>> ParamsTest is an interop test between keytool <-> openssl. There are some 
>> manual steps listed in jdk/sun/security/pkcs12/params/README to perform 
>> after the execution of jtreg execution. So this test is to perform that 
>> manual steps.
>
> Abdul Kolarkunnu has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8266182: Create a manual test for jdk/sun/security/pkcs12/ParamsTest.java

Marked as reviewed by hchao (Committer).

-

PR: https://git.openjdk.java.net/jdk/pull/4413

Re: [jdk17] RFR: 8267100: [BACKOUT] JDK-8196415 Disable SHA-1 Signed JARs

[Hai-May Chao]

On Mon, 21 Jun 2021 22:43:58 GMT, Weijun Wang  wrote:

> This is a copy of https://github.com/openjdk/jdk17/pull/100 so that I can 
> integrate the fix for @seanjmullan.

Marked as reviewed by hchao (Committer).

-

PR: https://git.openjdk.java.net/jdk17/pull/113

RFR: 8266225: jarsigner is using incorrect security property to show weakness of certs

[Hai-May Chao]

Please review the change to jarsigner so it uses certpath security property in 
order to properly display the weakness of the certificate algorithms.

-

Commit messages:
 - 8266225:jarsigner is using incorrect security property to show weakness of 
certs

Changes: https://git.openjdk.java.net/jdk/pull/3905/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3905=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8266225
  Stats: 39 lines in 2 files changed: 31 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3905.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3905/head:pull/3905

PR: https://git.openjdk.java.net/jdk/pull/3905