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

[Weijun Wang]

> On May 5, 2020, at 12:36 PM, Hai-May Chao  wrote:
> 
> 
> 
>> On May 4, 2020, at 6:01 PM, Weijun Wang  wrote:
>> 
>> 
>> 
>>> On May 5, 2020, at 3:48 AM, Hai-May Chao  wrote:
>>> 
>>> Hi Max,
>>> 
 On May 2, 2020, at 5:25 PM, Weijun Wang  wrote:
 
 In jarsigner/Main, you can just call
 
 pkixParameters.setRevocationEnabled(revocationCheck);
>>> 
>>> Ok
>>> 
 
 Last time, you mentioned that "Event.clearReportListener()" cannot be 
 called immediately after validation check because of some race conditions. 
 Is that easily reproducible? I still find it strange.
>>> 
>>> It is easily reproducible. I should have better explained why the suggested 
>>> changes did not work but not due to race condition. The code 
>>> pkixParameters.setRevocationEnabled(revocationCheck) only sets the 
>>> revocation checker enabled. It is the validateCertChain() in Main.java, 
>>> which calls into RevocationChecker.check() and ultimately calls 
>>> Event.report() prior to making OCSP and CRL connections. If we call 
>>> clearReportListener in loadKeyStore() method

I don't intend to clear the listener in loadKeyStore().

>>> (i.e finally{ }as suggested), for OCSP, by the time when 
>>> OCSP.getOCSPBytes() comes in to report the OCSP event, the reporter has 
>>> been cleared. And this would be same problem for CRL. So it cannot be 
>>> called immediately.

I was thinking about clearing the listener only after validation. However, I 
can see now that Validator.getInstance(...).validate(...) is called in 
validateCertChain(), then called in multiple places. It's a little complicated 
to set/clear multiple times.

So it's OK to clear it in run(). Do you think we can also set the listener in 
this method? Say, before the "if (verify)" line?

Thanks,
Max

>> 
>> Then I assume the following is OK.
>> 
>> try {
>>  setReportListener();
>>  validator.validate();
>> } finally {
>>  clearReportListener();
>> }
>> 
> 
> I’d like to know if there is an issue with the current webrev, which is being 
> tested and it’s working as expected. I’ve also tried with your previous 
> suggested change, and it didn’t work.
> 
> Thanks,
> Hai-May
> 
> 
>> Thanks,
>> Max
>> 
>>> 
>>> Thanks,
>>> Hai-May
>>> 
 
 Thanks,
 Max
 
> On May 3, 2020, at 2:19 AM, Hai-May Chao  wrote:
> 
> Hi Sean,
> 
> Thanks for your review. Reply inline.
> 
>> On May 1, 2020, at 11:50 AM, Sean Mullan  wrote:
>> 
>> * Main.java:
>> 
>> 2067 Event.setReportListener(new 
>> Event.Reporter() {
>> 2068 @Override
>> 2069 public void handle(String t, Object... 
>> o) {
>> 2070 System.out.println(String.format(rb.getString(t), o));
>> 2071 }
>> 2072 });
>> 
>> I think you could use a lambda expression above.
> 
> Done.
> 
>> 
>> * Event.java:
>> 
>> 35 static Reporter reporter = null;
>> 
>> Make this private? Also, no need to explicitly initialize to null as 
>> that is the default value.
> 
> Done (made it private).
> 
>> 
>> Can you add some comments at the top of the class describing the purpose 
>> of this class?
> 
> Done.
> 
>> 
>> * EnableRevocation.java
>> 
>> - How long does this test take - does it hang for a little while trying 
>> to make a connection or timeout right away? If it takes a while, you 
>> could experiment with overriding the default timeouts for CRLs and OCSP 
>> checks to make this test finish faster. Use the system properties 
>> com.sun.security.ocsp.timeout and com.sun.security.crl.readtimeout.
> 
> It does not hang for OCSP as the certificate is set with localhost as 
> OCSP responder. It hangs just a little for CRL, thus I’ve changed the 
> certificate to local host instead of remote host. The whole test is 
> finishing very fast now.
> 
>> 
>> Looks good otherwise. Please add a release-note and open a follow-on 
>> issue to update the man page with the new option.
> 
> Done (Release note: JDK-8244285, and man page: JDK-8244274).
> 
> Updated webrev:
> https://cr.openjdk.java.net/~hchao/8242060/webrev.02/
> 
> Thanks,
> Hai-May
> 
> 
>> 
>> --Sean
>> 
>> On 5/1/20 12:02 PM, Hai-May Chao wrote:
>>> Hi,
>>> With small change added to ‘Usages.java' test, here is the updated 
>>> webrev:
>>> https://cr.openjdk.java.net/~hchao/8242060/webrev.01/
>>> Thanks,
>>> Hai-May
 On Apr 30, 2020, at 4:29 PM, Hai-May Chao  
 wrote:
 
 Hi,
 
 I’d like to request a review for:
 
 JBS: https://bugs.openjdk.java.net/browse/JDK-8242060
 CSR: https://bugs.openjdk.java.net/browse/JDK-8244046
 Webrev: 

Re: RFR 8239385: KerberosTicket client name refers wrongly to sAMAccountName in AD

[Weijun Wang]

Yes, please.

Thanks,
Max

> On May 5, 2020, at 11:58 AM, Martin Balao  wrote:
> 
> Hi Max,
> 
> On 3/30/20 5:24 PM, Martin Balao wrote:
>> 
>> CSR requested here: https://bugs.openjdk.java.net/browse/JDK-8241871
>> 
> 
> Now that the CSR has been approved, are we good to push?
> 
> Thanks,
> Martin.-
> 

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 8239385: KerberosTicket client name refers wrongly to sAMAccountName in AD

[Martin Balao]

Hi Max,

On 3/30/20 5:24 PM, Martin Balao wrote:
> 
> CSR requested here: https://bugs.openjdk.java.net/browse/JDK-8241871
> 

Now that the CSR has been approved, are we good to push?

Thanks,
Martin.-

Re: [15] RFR JDK-8242151 Improve OID mapping and reuse among JDK security providers for aliases registration

[Weijun Wang]

Do you want to add OIDs in CurveDB into KnownOIDs as well?

Thanks,
Max

Re: [RFR] 8166597: Crypto support for the EdDSA Signature Algorithm (JEP 339)

[Bradford Wetmore]

All minor nits, can be done later if it won't be a problem to make minor 
API wording tweaks.


On 5/4/2020 10:17 AM, Anthony Scarpino wrote:

On 2/25/20 12:49 PM, Anthony Scarpino wrote:

Hi

I need a code review for the EdDSA support in JEP 339.  The code 
builds on the existing java implemented constant time classes used for 
XDH and the NIST curves.  The change also adds classes to the public 
API to support EdDSA operations.




Here is the final code review for the JEP. As the JEP is preparing to 
move to Propose-to-Target, if you have comments please state if they 
must be fixed as part of the initial putback.


https://cr.openjdk.java.net/~ascarpino/8166597/webrev.05/


Javadoc issues remain throughout java.security.*.

e.g. EdDCPoint
@param y the y-coordinate, represented using a BigInteger
->
@param y the y-coordinate, represented using a @{code BigInteger}

a boolean indicating whether the x-coordinate is odd.
->
a boolean indicating whether the x-coordinate is odd

src/java.base/share/classes/sun/security/provider/SHA3.java
--
114:  Is this comment about the 2-bit suffix still correct?

src/java.base/share/classes/sun/security/util/KeyUtil.java
--
2:  Copyright date.

110:  Do you want to go with the hardcoded values, or fix the commented 
out code?


src/java.base/share/classes/java/security/interfaces/EdECPrivateKey.java
--
38:  Very minor nit: EdEC reads a bit awkward to my eye: that term isn't 
used in the RFC.  Change or keep.  Maybe:


An EdEC private key
->
An Edwards-Curve private key ...
or
An {@code EdECPrivateKey} ...

Noticed some new typos, or you could do a minor replacement and reduce 
some of the duplicate wording.


51:
the an {@code Optional}
->
an {@code Optional}

52:
Ff
->
If

Alternate idea:  You could take out the second sentence in the method
description above and replace the @return with:

* @return the private key byte array, or an empty {@code Optional} 
if the

* key cannot be extracted (e.g. if the provider is a hardware token
* and the private key is not allowed to leave the crypto boundary).

This was based on the XEC wording.  Your call/choice.

src/java.base/share/classes/java/security/interfaces/EdECPublicKey.java
--
37:  Same comment about EdEC.  Change or keep.

src/java.base/share/classes/java/security/spec/EdDSAParameterSpec.java
--
92:  Repeat of previous comment from last week.   You replied "Ok" but 
didn't

see a change yet.

"...bound to the signature" sounds premature.  This is the context bound
to the EdDSAParameteerSpec, which hasn't been yet bound to the signature.

src/java.base/share/classes/java/security/spec/EdECPoint.java
--
38:  Same comment about EdEC.  Change or keep.

test/jdk/sun/security/ec/ed/TestEdDSA.java
--
Nits:

211-212:  Indention problem

159/257/258/301:  Extra whitespace.

313:  The context value was set correctly from a previous test, but
wouldn't hurt to reiterate it here.

Brad

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

[Weijun Wang]

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

Then I assume the following is OK.

try {
   setReportListener();
   validator.validate();
} finally {
   clearReportListener();
}

Thanks,
Max

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

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (security)

[sha . jiang]

Hi,
Generally, this patch doesn't take care of the solaris/sunos/ucrypto-related
words in test summaries, code comments, configs and READMEs.
E.g.
test/jdk/javax/net/ssl/templates/SSLEngineTemplate.java
test/jdk/sun/security/provider/MessageDigest/TestSHAClone.java
test/jdk/sun/security/util/Resources/Format.config
test/jdk/sun/security/pkcs11/KeyStore/BasicData/README
Would we need some follow-up issues to double-check this point?

test/jdk/sun/security/tools/keytool/KeyToolTest.java
39   * Testing Solaris Cryptography Framework PKCS11 keystores:
40   *   # make sure you've already run pktool and set test12 as pin
41   *   echo | java -Dsolaris KeyToolTest
...
1863  if (System.getProperty("solaris") != null) {
1864  // For Solaris Cryptography Framework
1865  t.srcP11Arg = SUN_SRC_P11_ARG;
1866  t.p11Arg = SUN_P11_ARG;
1867  t.testPKCS11();
1868  t.testPKCS11ImportKeyStore();
1869  t.i18nPKCS11Test();
1870  }
It may be necessary to remove the above lines.

test/jdk/sun/security/provider/PolicyParser/PrincipalExpansionError.java
test/jdk/sun/security/provider/PolicyParser/PrincipalExpansionError.policy
It looks this manual test and the associated policy are solaris-related 
only.

Could these files be removed as well?
In fact, the solaris-related com.sun.security.auth classes, including
SolarisPrincipal, are already removed.

test/jdk/sun/security/pkcs11/tls/TestPRF.java
114  if (secret == null) {
115  // This fails on Solaris, but since we 
never call this
116  // API for this case in JSSE, ignore 
the failure.
117  // (SunJSSE uses the 
CKM_TLS_KEY_AND_MAC_DERIVE

118  // mechanism)
119  System.out.print("X");
120  continue;
121  }
Could remove this block?

Best regards,
John Jiang

On 2020/5/4 13:12, Mikael Vidstedt wrote:

Please review this change which implements part of JEP 381:

JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/security/open/webrev/
JEP: https://bugs.openjdk.java.net/browse/JDK-8241787


Note: When reviewing this, please be aware that this exercise was *extremely* 
mind-numbing, so I appreciate your help reviewing all the individual changes 
carefully. You may want to get that coffee cup filled up (or whatever keeps you 
awake)!


Background:

Because of the size of the total patch and wide range of areas touched, this 
patch is one out of in total six partial patches which together make up the 
necessary changes to remove the Solaris and SPARC ports. The other patches are 
being sent out for review to mailing lists appropriate for the respective areas 
the touch. An email will be sent to jdk-dev summarizing all the 
patches/reviews. To be clear: this patch is *not* in itself complete and 
stand-alone - all of the (six) patches are needed to form a complete patch. 
Some changes in this patch may look wrong or incomplete unless also looking at 
the corresponding changes in other areas.

For convenience, I’m including a link below[1] to the full webrev, but in case 
you have comments on changes in other areas, outside of the files included in 
this thread, please provide those comments directly in the thread on the 
appropriate mailing list for that area if possible.

In case it helps, the changes were effectively produced by searching for and 
updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
More information about the areas impacted can be found in the JEP itself.


Testing:

A slightly earlier version of this change successfully passed tier1-8, as well 
as client tier1-2. Additional testing will be done after the first round of 
reviews has been completed.

Cheers,
Mikael

[1] 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/

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] 8166597: Crypto support for the EdDSA Signature Algorithm (JEP 339)

[Anthony Scarpino]

On 2/25/20 12:49 PM, Anthony Scarpino wrote:

Hi

I need a code review for the EdDSA support in JEP 339.  The code builds 
on the existing java implemented constant time classes used for XDH and 
the NIST curves.  The change also adds classes to the public API to 
support EdDSA operations.




Here is the final code review for the JEP. As the JEP is preparing to 
move to Propose-to-Target, if you have comments please state if they 
must be fixed as part of the initial putback.


https://cr.openjdk.java.net/~ascarpino/8166597/webrev.05/

thanks

Tony

Re: [15] RFR: 8242335: Additional Tests for RSASSA-PSS

[Valerie Peng]

Updated webrev looks fine.

Thanks,

Valerie

On 5/3/2020 11:27 PM, sibabrata.sa...@oracle.com wrote:

Hi Valerie,

Here is the updated web rev addressing the following comments. There 
are 7 lines of additional change added [Line: 126-133] to re-verify 
the signature with another Signature instance.


Webrev: http://cr.openjdk.java.net/~ssahoo/8242335/webrev.01/

Thanks,
Siba



On 01-May-2020, at 4:58 AM, Valerie Peng > wrote:


Hi, Siba,

Here are my comments:



- Typos: Initialisation -> Initialization.

- the 2nd algorithm specific initialization using reflection doesn't 
seem too useful. The code path should be exactly the same as the 
non-reflection one. Maybe just remove it.


Rest looks fine.

Thanks,
Valerie
On 4/28/2020 12:38 AM, sibabrata.sa...@oracle.com wrote:

Hi Valerie,

Please review the patch for,
JBS: https://bugs.openjdk.java.net/browse/JDK-8242335
Webrev: http://cr.openjdk.java.net/~ssahoo/8242335/webrev.00/

These are additional Tests developed to support RSASSA-PSS 
algorithm. One is key compatibility with OpenSSL generated while the 
other one is the API Tests for serialized keys.


Thanks,
Siba

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (security)

[Weijun Wang]

I've gone through all files. Many of them are PKCS11-related, it will be nice 
if Valerie can confirm my findings.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java:

   Maybe we should not support $ISA at all?

test/jdk/com/sun/security/auth/login/ConfigFile/InconsistentError.java:

   Please also remove the whole else block from line 61.

test/jdk/java/security/KeyPairGenerator/SolarisShortDSA.java:

   It's not worth keeping this test. Error has never occurred on other 
platforms.

test/jdk/java/security/KeyStore/TestKeyStoreBasic.java:

   128-135: There is no need to use a try-catch block.

test/jdk/java/security/KeyStore/TestKeyStoreEntry.java:

   54-60: No need
   81-97: No need. Just a single run() is OK. 
   101: Remove the ", p" argument

test/jdk/java/security/MessageDigest/TestDigestIOStream.java:
test/jdk/java/security/MessageDigest/TestSameLength.java:
test/jdk/java/security/MessageDigest/TestSameValue.java:

   No need for isSHA3Supported(). The SUN provider should always be there.
   Inside the test where NoSuchAlgorithmException is caught, the catch block 
can be removed and no need to try

test/jdk/java/security/MessageDigest/UnsupportedProvider.java:

   Not worth keeping this test.

test/jdk/sun/security/jca/PreferredProviderTest.java:

   53: remove "for other platform"

test/jdk/sun/security/pkcs/pkcs8/PKCS8Test.java

   Remove the comments on lines 37-40

test/jdk/sun/security/pkcs11/Config/ReadConfInUTF16Env.java:

   Not worth keeping the tests in this directory.

test/jdk/sun/security/pkcs11/Mac/MacSameTest.java:

   No need for try

test/jdk/sun/security/pkcs11/Provider/ConfigShortPath.java:

   Will this test ever run? There is no out-of-box SunPKCS11 provider. Even if 
one is configured manually, the name is likely to be SunPKCS11-XYZ,

test/jdk/sun/security/pkcs11/tls/TestKeyMaterial.java:

   No need for this catch block

test/jdk/sun/security/tools/keytool/fakegen/PSS.java:

   Please also remove the comment on lines 35-37.

test/jdk/sun/security/tools/keytool/fakegen/DefaultSignatureAlgorithm.java:

   Please also remove the comment on lines 36-38.

Thanks,
Max



> On May 4, 2020, at 1:12 PM, Mikael Vidstedt  
> wrote:
> 
> 
> Please review this change which implements part of JEP 381:
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
> webrev: 
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/security/open/webrev/
> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
> 
> 
> Note: When reviewing this, please be aware that this exercise was *extremely* 
> mind-numbing, so I appreciate your help reviewing all the individual changes 
> carefully. You may want to get that coffee cup filled up (or whatever keeps 
> you awake)!
> 
> 
> Background:
> 
> Because of the size of the total patch and wide range of areas touched, this 
> patch is one out of in total six partial patches which together make up the 
> necessary changes to remove the Solaris and SPARC ports. The other patches 
> are being sent out for review to mailing lists appropriate for the respective 
> areas the touch. An email will be sent to jdk-dev summarizing all the 
> patches/reviews. To be clear: this patch is *not* in itself complete and 
> stand-alone - all of the (six) patches are needed to form a complete patch. 
> Some changes in this patch may look wrong or incomplete unless also looking 
> at the corresponding changes in other areas.
> 
> For convenience, I’m including a link below[1] to the full webrev, but in 
> case you have comments on changes in other areas, outside of the files 
> included in this thread, please provide those comments directly in the thread 
> on the appropriate mailing list for that area if possible.
> 
> In case it helps, the changes were effectively produced by searching for and 
> updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
> More information about the areas impacted can be found in the JEP itself.
> 
> 
> Testing:
> 
> A slightly earlier version of this change successfully passed tier1-8, as 
> well as client tier1-2. Additional testing will be done after the first round 
> of reviews has been completed.
> 
> Cheers,
> Mikael
> 
> [1] 
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/

RE: [PATCH] 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap tests

[Thejasvi Voniadka]

Adding jmx-dev and security-dev.

-Original Message-
From: Thejasvi Voniadka 
Sent: Monday, May 4, 2020 12:56 PM
To: core-libs-...@openjdk.java.net
Subject: [PATCH] 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap 
tests

Hi,

May I please find a sponsor for this patch?

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

Description:
The test 
"test/jdk/sun/management/jmxremote/bootstrap/RmiSslBootstrapTest.sh" fails 
intermittently on a lower release. While investigating the cause for the 
failure, I felt the diagnostic messaging built into the test was not adequate. 
Some of the issues observed:
1. If the test throws a RuntimeException, the full stack trace is not 
displayed.
2. The test has several logging statements. However, the logging 
environment is not properly initialized to allow levels such as FINE when 
needed.
3. The logging sequence could be improved: some messages end up at 
stdout, and others at stderr, which makes it harder to follow the output.

The patch is to improve the test in these lines.

Webrev: http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.full.00

I have also used this opportunity to clean-up the test code as a whole, in 
lines of removal of redundant logic, formatting, coding guidelines, etc If 
the above patch looks too confusing and is hard to follow, here is a simpler 
version that shows only the core logic changes (I intend to submit only the 
full patch to be committed though): 
http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.simple.00

The patch has been tested on mach5, and all jmxremote tests passed.

Re: [15] RFR: 8242335: Additional Tests for RSASSA-PSS

[sibabrata.sa...@oracle.com]

Hi Valerie,

Here is the updated web rev addressing the following comments. There are 7 
lines of additional change added [Line: 126-133] to re-verify the signature 
with another Signature instance.

Webrev: http://cr.openjdk.java.net/~ssahoo/8242335/webrev.01/ 


Thanks,
Siba



> On 01-May-2020, at 4:58 AM, Valerie Peng  wrote:
> 
> Hi, Siba,
> 
> Here are my comments:
> 
> 
> 
> - Typos: Initialisation -> Initialization.
> 
> - the 2nd algorithm specific initialization using reflection doesn't seem too 
> useful. The code path should be exactly the same as the non-reflection one. 
> Maybe just remove it.
> 
> Rest looks fine.
> 
> Thanks,
> Valerie
> On 4/28/2020 12:38 AM, sibabrata.sa...@oracle.com 
>  wrote:
>> Hi Valerie,
>> 
>> Please review the patch for,
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8242335 
>> 
>> Webrev: http://cr.openjdk.java.net/~ssahoo/8242335/webrev.00/ 
>> 
>> 
>> These are additional Tests developed to support RSASSA-PSS algorithm. One is 
>> key compatibility with OpenSSL generated while the other one is the API 
>> Tests for serialized keys.
>> 
>> Thanks,
>> Siba
>> 
>> 
>>