RFR[15] JDK-8242538: java/security/SecureRandom/ThreadSafe.java failed on windows

[sha . jiang]

Hi,
This test failed due to the expected concurrency issue didn't raise.
This patch may mitigate this test failure.

diff -r 7a61a943ce9d test/jdk/java/security/SecureRandom/ThreadSafe.java
--- a/test/jdk/java/security/SecureRandom/ThreadSafe.java    Sat Jul 04 
01:06:07 2020 -0700
+++ b/test/jdk/java/security/SecureRandom/ThreadSafe.java    Mon Jul 06 
10:15:55 2020 +0800

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -87,7 +87,7 @@
 }
 inCall = true;
 try {
-    Thread.sleep(100);
+    Thread.sleep(500);
 } catch (Exception e) {
 // OK
 }

Best regards,
John Jiang

Re: RFR 8245520: Provide a way to add CA certificate to cacerts during test run

[sha . jiang]

Move this patch to sun/security/util/module_patch
Webrev: http://cr.openjdk.java.net/~jjiang/8245520/webrev.01/

Best regards,
John Jiang

On 2020/6/11 15:55, sha.ji...@oracle.com wrote:

Hi Max,
Thanks for your info!

It looks that patch for java.base:sun.security.util.FilePaths.java is 
used
for specifying an alternative cacerts file. But my requirement is 
updating

the existing cacerts with given CAs.

Both of the functions are useful. I'll submit a new webrev after the fix
for JDK-8244148 is pushed. I also have some comments about Hai-May's
webrev.

Best regards,
John Jiang

On 2020/6/11 11:25, Weijun Wang wrote:

Hai-May has a similar function as a by-product in

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

See FilePaths.java and MyOwnCacerts.java for the detail. Her code 
change is in code review now.


Please coordinate with her on how to get this done nicely.

Thanks,
Max


On Jun 11, 2020, at 9:39 AM, sha.ji...@oracle.com wrote:

Hi,
This patch contains a patched version for class 
sun.security.util.AnchorCertificates.

It allows to add CAs to cacerts at runtime for testing purpose.

Webrev: http://cr.openjdk.java.net/~jjiang/8245520/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8245520

Best regards,
John Jiang

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

[sha . jiang]

Hi Hai-May,

On 2020/6/13 06:34, Hai-May Chao wrote:

Hi John,

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

Thanks for this updated webrev!
I have no more comment.

Best regards,
John Jiang



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/

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

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

[sha . jiang]

Hi Hai-May,

On 2020/6/8 04:01, Hai-May Chao wrote:

Updated webrev -

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?

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

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.

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

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

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


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.

Best regards,
John Jiang



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 file is 
created with the JKS storetype. We are 

Re: RFR 8245520: Provide a way to add CA certificate to cacerts during test run

[sha . jiang]

Hi Max,
Thanks for your info!

It looks that patch for java.base:sun.security.util.FilePaths.java is used
for specifying an alternative cacerts file. But my requirement is updating
the existing cacerts with given CAs.

Both of the functions are useful. I'll submit a new webrev after the fix
for JDK-8244148 is pushed. I also have some comments about Hai-May's
webrev.

Best regards,
John Jiang

On 2020/6/11 11:25, Weijun Wang wrote:

Hai-May has a similar function as a by-product in

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

See FilePaths.java and MyOwnCacerts.java for the detail. Her code change is in 
code review now.

Please coordinate with her on how to get this done nicely.

Thanks,
Max


On Jun 11, 2020, at 9:39 AM, sha.ji...@oracle.com wrote:

Hi,
This patch contains a patched version for class 
sun.security.util.AnchorCertificates.
It allows to add CAs to cacerts at runtime for testing purpose.

Webrev: http://cr.openjdk.java.net/~jjiang/8245520/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8245520

Best regards,
John Jiang

RFR 8245520: Provide a way to add CA certificate to cacerts during test run

[sha . jiang]

Hi,
This patch contains a patched version for class 
sun.security.util.AnchorCertificates.

It allows to add CAs to cacerts at runtime for testing purpose.

Webrev: http://cr.openjdk.java.net/~jjiang/8245520/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8245520

Best regards,
John Jiang

RFR[15] JDK-8246709: sun/security/tools/jarsigner/TsacertOptionTest.java compilation failed after JDK-8244683

[sha . jiang]

Hi,
This patch takes TsacertOptionTest.java to use TsaServer directly,
and removes some unnecessary constructors in TsaServer.

Webrev: http://cr.openjdk.java.net/~jjiang/8246709/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8246709

Best regards,
John Jiang

Re: RFR[15] JDK-8244683: A TSA server used by tests

[sha . jiang]

Hi Max
Please review this webrev:
http://cr.openjdk.java.net/~jjiang/8244683/webrev.05/

- TimestampCheck.java
The codes in those private methods in Interceptor are merged into
getRespParam().

- TsaParam.java
Group the fields as signing internals and TSA fields.

- TsaSigner.java
parseRequestParam() and createResponse() are private now.

Best regards,
John Jiang

On 2020/6/4 22:18, Weijun Wang wrote:

OK. Please go on. No other comment.


On Jun 4, 2020, at 12:02 AM, sha.ji...@oracle.com wrote:


Can we make them internal at the moment until someone really need to extend it?

Yes, we can.
But as a test lib, is it bad to think about a bit further?

But think about it carefully when there are new requirements. It's probably not 
a good idea to make every method overridable at the beginning and say do 
whatever you want.

Thanks,
Max

Re: RFR[15] JDK-8244683: A TSA server used by tests

[sha . jiang]

On 2020/6/3 22:57, Weijun Wang wrote:



TsaParam.java:

- Please group the fields into different area. Looks like some are fields in 
TSTInfo, and some are server internals.

One usage of this class is carrying all parameters delivered by HTTP
request. Do you mean they should be grouped in different classes?

No. Just add some comment lines inside the class and separate them into 
different groups.

This class is also used for carrying TSA request fields, such as version,
digestAlgo and hashedMessage, and especially certReq is in request only.




The overall design is good, but I have a feeling that the interface and 
implementation can be further separated. I can see that there are a lot of 
methods and classes that will not be touched by TimeStampCheck at all. Should 
these things be taken out to be another implementation which is parallel to the 
one in TimeStampCheck? Maybe TsaHandler::createSigner will be abstract.

All RespInterceptor methods are used by TimeStampCheck.
DefaultRespInterceptor is used by standalone TSA server. In this style, no
more RespInterceptor implementation is needed.

See below.


There are quite some public constructors in the classes. Are they meant to be 
called by a user? Or by a child class? Or just internal?

The public constructors in TsaServer can be called by applications for
making standalone servers.

How about the other classes?

TsaSigner has two constructors.
One is for applications without new interceptor, the other is used by
TimeStampCheck which needs to provide a custom interceptor.

Please consider this TSA server will be used as standalone style,
namely, no new interceptor is needed. It's not introduced for some
specific test, like TimeStampCheck.


Also, for those protected methods (Ex: parseRequestParam and createResponse in 
TsaSigner), do you meant to override it in the future?

Possibly.
For example, a sub parseRequestParam() may need to parse extensions from
request. A test may directly twist the byte[] returned by createResponse(),
or even an overridden createResponse() just returns an empty byte[].

Can we make them internal at the moment until someone really need to extend it?

Yes, we can.
But as a test lib, is it bad to think about a bit further?


Can you give examples on how to use this server? Will it always use 
DefaultRespInterceptor? If so, will the default impl in RespInterceptor.java 
ever be used?

I'll use this server as standalone-like service. The custom TSA fields
just be delivered by HTTP request query string. I won't provide a custom
RespInterceptor, and DefaultRespInterceptor is enough to me.

You can put DefaultRespInterceptor in the standalone server.

An application just needs to new TsaServer() with a keystore and start it,
then a standalone server is ready. No more interceptor is needed. Otherwise,
every application may need to provide an interceptor like 
DefaultRespInterceptor.



If you change TsaSigner.java

   80 public TsaSigner(SignerEntry signerEntry, byte[] requestData,
   81 TsaParam param) {
   82 this(signerEntry, requestData,
   83 new DefaultRespInterceptor(param));

to use the even more "default" RespInterceptor, i.e. new RespInterceptor(){}, 
is it still useable?

The param is needed. Or, the TSA fields cannot be changed without
introducing a new interceptor.

Could I launch a standalone TSA server at first, and then run jarsigner
separately for manually checking some cases?

For example, jarsigner ... -tsa "http://host:port?version=2; ...
In the above command, the standalone TSA server will send a response
with version is 2.

Best regards,
John Jiang

Re: RFR[15] JDK-8244683: A TSA server used by tests

[sha . jiang]

Hi Max,

On 2020/6/3 17:17, Weijun Wang wrote:

TimeStampCheck.java:

- Can you please inline all those private getXyz() calls in Interceptor into 
getRespParam()? I would like to see all these customizations in one place. Just 
add a blank line in between and it will be clean enough for me.

OK



TsaParam.java:

- Please group the fields into different area. Looks like some are fields in 
TSTInfo, and some are server internals.

One usage of this class is carrying all parameters delivered by HTTP
request. Do you mean they should be grouped in different classes?



The overall design is good, but I have a feeling that the interface and 
implementation can be further separated. I can see that there are a lot of 
methods and classes that will not be touched by TimeStampCheck at all. Should 
these things be taken out to be another implementation which is parallel to the 
one in TimeStampCheck? Maybe TsaHandler::createSigner will be abstract.

All RespInterceptor methods are used by TimeStampCheck.
DefaultRespInterceptor is used by standalone TSA server. In this style, no
more RespInterceptor implementation is needed.



There are quite some public constructors in the classes. Are they meant to be 
called by a user? Or by a child class? Or just internal?

The public constructors in TsaServer can be called by applications for
making standalone servers.



Also, for those protected methods (Ex: parseRequestParam and createResponse in 
TsaSigner), do you meant to override it in the future?

Possibly.
For example, a sub parseRequestParam() may need to parse extensions from
request. A test may directly twist the byte[] returned by createResponse(),
or even an overridden createResponse() just returns an empty byte[].



Can you give examples on how to use this server? Will it always use 
DefaultRespInterceptor? If so, will the default impl in RespInterceptor.java 
ever be used?

I'll use this server as standalone-like service. The custom TSA fields
just be delivered by HTTP request query string. I won't provide a custom
RespInterceptor, and DefaultRespInterceptor is enough to me.

Best regards,
John Jiang

Re: RFR[15] JDK-8244683: A TSA server used by tests

[sha . jiang]

After discussed with Max, I just updated the patch,
http://cr.openjdk.java.net/~jjiang/8244683/webrev.04/

Interface TsaInterceptor is renamed to RespInterceptor. The methods, which
affect TSA response fields, are merged into getRespParam(reqParam).

Best regards,
John Jiang

On 2020/5/13 08:18, sha.ji...@oracle.com wrote:

Hi Max,
Thanks for your comments!
Please review the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8244683/webrev.02/

The codes are refactored significantly.

On 2020/5/11 10:51, Weijun Wang wrote:
Can you update the existing TimeStampCheck test to use this class? I 
know that test can simulate some error conditions. Maybe you can add 
one or more virtual methods in this class so  TimeStampCheck can 
override them.

This test is updated to use this TSA server.
A new introduced class, namely TsaInterceptor, defines some extension 
points for the signing.


getDefaultSigAlgo(): Please call AlgorithmId.getDefaultSigAlgForKey() 
instead. It will be enhanced to support new algorithm.

Fixed.


Param: How about making it a JDK 14 record?

In the updated webrev, this class has changed to TsaParam.
The fields are not final, and especially this class could be extended 
by tests.
With my understanding, this language feature may not be applicable for 
this scenario .


Best regards,
John Jiang

Thanks,
Max


On May 11, 2020, at 9:28 AM,sha.ji...@oracle.com  wrote:

Hi,
This patch introduces a TSA server, which can work with jarsigner.
This server will be used by the following jar signing tests.

Issue:https://bugs.openjdk.java.net/browse/JDK-8244683
Webrev:http://cr.openjdk.java.net/~jjiang/8244683/webrev.00/

Best regards,
John Jiang

RFR[15] JDK-8245691: Add EdDSA certificstes to SSLSocketTemplate and CertUtils

[sha . jiang]

Hi,
This patch just adds some EdDSA, including Ed25519 and Ed448, certificates
to javax/net/ssl/SSLSocketTemplate.java and 
jdk/test/lib/security/CertUtils.java.


Please note these new certificates are not used by default.

Issue: https://bugs.openjdk.java.net/browse/JDK-8245691
Webrev: http://cr.openjdk.java.net/~jjiang/8245691/webrev.00/

Best regards,
John Jiang

Re: RFR[15] JDK-8245134: test/lib/jdk/test/lib/security/KeyStoreUtils.java should allow to specify aliases

[sha . jiang]

Hi Valerie,
Thanks for your clarification!

Could you please review this updated webrev:
http://cr.openjdk.java.net/~jjiang/8245134/webrev.01/

Best regards,
John Jiang

On 2020/5/22 06:08, Valerie Peng wrote:



On 5/20/2020 7:38 PM, sha.ji...@oracle.com wrote:


- line 176, maybe it's better to just supply the type, clearer and 
less dependency.



Not get this point.
Could you please describe some more?


I mean to change line 176 as below (see the added text in blue):

176 return createTrustStore(DEFAULT_TYPE, certStrs, null);


With this extra aliases argument, number of createTrustStore(...) 
methods doubled from 2 to 4, number of createKeyStore(...) methods 
also doubled from 4 to 8. Isn't it a bit much to have 8 methods 
doing the same thing? Especially in the case of createKeyStore(...), 
quite a few of them have long list of arguments with the same type, 
combining this with the large number of methods, it can get 
confusing on which method is called. How often do you think the 
aliases are supplied? Maybe we only add methods which will be used 
instead of adding all possible combinations.


I'll remove 4 createKeyStore(...) methods (like the below one) that 
have long list of arguments.

createKeyStore(String type, String[] keyAlgos,
    String[] keyStrs, String[] passwords, String[] certStrs,
    String[] aliases)
It looks no test is using them. My tests also won't use them.


Sounds good.

Thanks,
Valerie


Best regards,
John Jiang


Thanks,
Valerie
On 5/15/2020 11:40 PM, sha.ji...@oracle.com wrote:

Hi,
This patch adds some new createTrustStore() and createKeyStore() 
methods to test

lib class jdk.test.lib.security.KeyStoreUtils.
These new methods allow to pass trust/key store aliases in.

Issue: https://bugs.openjdk.java.net/browse/JDK-8245134
Webrev: http://cr.openjdk.java.net/~jjiang/8245134/webrev.00/

Best regards,
John Jiang

Re: RFR[15] JDK-8245134: test/lib/jdk/test/lib/security/KeyStoreUtils.java should allow to specify aliases

[sha . jiang]

Hi Valerie,
Thanks for your comments!

On 2020/5/21 04:21, Valerie Peng wrote:


Hi John,

- line 125, typo: "aliaes" should be "aliases"


Will fix it.


- line 132: if aliases is not null, check its length is certStrs.length


Will fix it.


- line 152, use the specified type instead of DEFAULT_TYPE?


Will fix it.

- line 176, maybe it's better to just supply the type, clearer and 
less dependency.



Not get this point.
Could you please describe some more?

With this extra aliases argument, number of createTrustStore(...) 
methods doubled from 2 to 4, number of createKeyStore(...) methods 
also doubled from 4 to 8. Isn't it a bit much to have 8 methods doing 
the same thing? Especially in the case of createKeyStore(...), quite a 
few of them have long list of arguments with the same type, combining 
this with the large number of methods, it can get confusing on which 
method is called. How often do you think the aliases are supplied? 
Maybe we only add methods which will be used instead of adding all 
possible combinations.


I'll remove 4 createKeyStore(...) methods (like the below one) that have 
long list of arguments.

createKeyStore(String type, String[] keyAlgos,
    String[] keyStrs, String[] passwords, String[] certStrs,
    String[] aliases)
It looks no test is using them. My tests also won't use them.

Best regards,
John Jiang


Thanks,
Valerie
On 5/15/2020 11:40 PM, sha.ji...@oracle.com wrote:

Hi,
This patch adds some new createTrustStore() and createKeyStore() 
methods to test

lib class jdk.test.lib.security.KeyStoreUtils.
These new methods allow to pass trust/key store aliases in.

Issue: https://bugs.openjdk.java.net/browse/JDK-8245134
Webrev: http://cr.openjdk.java.net/~jjiang/8245134/webrev.00/

Best regards,
John Jiang

RFR[15] JDK-8245134: test/lib/jdk/test/lib/security/KeyStoreUtils.java should allow to specify aliases

[sha . jiang]

Hi,
This patch adds some new createTrustStore() and createKeyStore() methods 
to test

lib class jdk.test.lib.security.KeyStoreUtils.
These new methods allow to pass trust/key store aliases in.

Issue: https://bugs.openjdk.java.net/browse/JDK-8245134
Webrev: http://cr.openjdk.java.net/~jjiang/8245134/webrev.00/

Best regards,
John Jiang

RFR[15] JDK-8245005: javax/net/ssl/compatibility/BasicConnectTest.java failed with No enum constant

[sha . jiang]

Hi,
Possibly run the manual test 
javax/net/ssl/compatibility/BasicConnectTest.java
with JDK builds supporting TLS_KRB5 cipher suites. However these cipher 
suites

are not in test/jdk/javax/net/ssl/TLSCommon/ChpherSuite.java. When convert a
TLS_KRB5 cipher suite name to a ChpherSuite enum, IllegalArgumentException
will be raised. This patch just adds TLS_KRB5 cipher suites to 
ChpherSuite.java.


In addition, test/jdk/javax/net/ssl/compatibility/Compatibility.java was 
already

removed, so test/jdk/javax/net/ssl/compatibility/README should be updated
accordingly.

Issue: https://bugs.openjdk.java.net/browse/JDK-8245005
Webrev: http://cr.openjdk.java.net/~jjiang/8245005/webrev.00/

Best regards,
John Jiang

Re: RFR[15] JDK-8244683: A TSA server used by tests

[sha . jiang]

Hi Max,
Thanks for your comments!
Please review the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8244683/webrev.02/

The codes are refactored significantly.

On 2020/5/11 10:51, Weijun Wang wrote:

Can you update the existing TimeStampCheck test to use this class? I know that 
test can simulate some error conditions. Maybe you can add one or more virtual 
methods in this class so  TimeStampCheck can override them.

This test is updated to use this TSA server.
A new introduced class, namely TsaInterceptor, defines some extension 
points for the signing.



getDefaultSigAlgo(): Please call AlgorithmId.getDefaultSigAlgForKey() instead. 
It will be enhanced to support new algorithm.

Fixed.


Param: How about making it a JDK 14 record?

In the updated webrev, this class has changed to TsaParam.
The fields are not final, and especially this class could be extended by 
tests.
With my understanding, this language feature may not be applicable for 
this scenario .


Best regards,
John Jiang

Thanks,
Max


On May 11, 2020, at 9:28 AM,sha.ji...@oracle.com  wrote:

Hi,
This patch introduces a TSA server, which can work with jarsigner.
This server will be used by the following jar signing tests.

Issue:https://bugs.openjdk.java.net/browse/JDK-8244683
Webrev:http://cr.openjdk.java.net/~jjiang/8244683/webrev.00/

Best regards,
John Jiang

RFR[15] JDK-8244683: A TSA server used by tests

[sha . jiang]

Hi,
This patch introduces a TSA server, which can work with jarsigner.
This server will be used by the following jar signing tests.

Issue: https://bugs.openjdk.java.net/browse/JDK-8244683
Webrev: http://cr.openjdk.java.net/~jjiang/8244683/webrev.00/

Best regards,
John Jiang

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/

RFR[15] JDK-8243549: sun/security/ssl/CipherSuite/NamedGroupsWithCipherSuite.java failed with Unsupported signature algorithm: DSA

[sha . jiang]

Hi,
DHE_DSS cipher suites cannot work with SHA256withDSA (key size 2048) 
certificates over pre-TLSv1.2 protocols.

Please see JDK-8242928 for more details.

This fix takes the test to use a SHA1withDSA certificate (key size 1024) 
for TLSv1 and TLSv1.1.


Webrev: http://cr.openjdk.java.net/~jjiang/8243549/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8243549

Best regards,
John Jiang

RFR[15] JDK-8243029: Rewrite javax/net/ssl/compatibility/Compatibility.java with a flexible interop test framework

[sha . jiang]

Hi,
This patch introduces a flexible test framework for supporting JSSE 
interop testing.
This framework supports the SSL/TLS communication between not only JDK 
builds,

but also possibly other TLS implementations.

The original test cases in 
javax/net/ssl/compatibility/Compatibility.java are
divided into 3 separated tests, namely BasicConnectTest.java, 
SniTest.java and
AlpnTest.java, and introduces a new compatibility test on TLS 1.3 hello 
retry
request, namely HrrTest.java. These tests depend on the aforementioned 
framework.


And the negative cases are removed for making the test results clean and 
clear.
It doesn't generate result report anymore. All of failed test cases are 
highlighted

at the end of a test run.

Additionally, more compatibility tests will be added.

Webrev: http://cr.openjdk.java.net/~jjiang/8243029/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8243029

Best regards,
John Jiang

Re: RFR[15] JDK-8237977: Further update javax/net/ssl/compatibility/Compatibility.java

[sha . jiang]

Hi Rajan,
Thanks for your suggestions.
Please review the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8237977/webrev.01/


On 2020/3/25 11:08, Rajan Halade wrote:

Hi John,

Thanks for taking care of this fix. Your changes look good me.

I have couple of suggestions:

- consider changing header for last column from “Why negative case” to 
“Reason”
- for a failed test case (a testcase that succeeds when expected to 
fail or a testcase that fails when expected to succeed) code will not 
have any reason listed and we will need to click hyperlink in the 
report to find out root cause. May I suggest to pass in status to 
TestCase.negativeCaseReason() method call and use reason as “Refer to 
log at test hyperlink for details…” for such cases.

A new method TestCase::reason is introduced for the reasons.

Best regards,
John Jiang


Thanks,
Rajan

On Mar 24, 2020, at 6:50 PM, sha.ji...@oracle.com 
 wrote:


Hi,
This patch updates the manual test 
javax/net/ssl/compatibility/Compatibility.java on the following points:

1. Covers SSLv3
2. The server side doesn't limit/specify protocols and cipher suites 
anymore. Only the client side specifies these parameters.
3. Only focus on the testing JDK specified by jtreg option "-jdk", 
and not run the cases between the JDKs builds in jdkList. This would 
save much execution time.
4. The report has a new column that clarifying why a case is a 
negative case.


Issue: https://bugs.openjdk.java.net/browse/JDK-8237977
Webrev: http://cr.openjdk.java.net/~jjiang/8237977/webrev.00/

Best regards,
John Jiang

RFR[15] JDK-8237977: Further update javax/net/ssl/compatibility/Compatibility.java

[sha . jiang]

Hi,
This patch updates the manual test 
javax/net/ssl/compatibility/Compatibility.java on the following points:

1. Covers SSLv3
2. The server side doesn't limit/specify protocols and cipher suites 
anymore. Only the client side specifies these parameters.
3. Only focus on the testing JDK specified by jtreg option "-jdk", and 
not run the cases between the JDKs builds in jdkList. This would save 
much execution time.
4. The report has a new column that clarifying why a case is a negative 
case.


Issue: https://bugs.openjdk.java.net/browse/JDK-8237977
Webrev: http://cr.openjdk.java.net/~jjiang/8237977/webrev.00/

Best regards,
John Jiang

Re: RFR[15] 8238677: java/net/httpclient/ssltest/CertificateTest.java should not specify TLS version

[sha . jiang]

Hi Daniel,



Would it be possible to include a comment in Cert.java that contains
the command you used to generate the certificates?

That will be a great help to future maintainers if the certificates
ever needs to be re-generated (e.g. to update the expiry date
etc...)


I'll do that.
Please review this updated webrev: 
http://cr.openjdk.java.net/~jjiang/8238677/webrev.01/

The script, exactly gen-certs.sh, can be used to generate the certs.

Best regards,
John Jiang



Best regards,
John Jiang



best regards,

-- daniel

Disclaimer: I am not an expert in ssl/security


On 07/02/2020 10:51, sha.ji...@oracle.com wrote:

Hi,
java/net/httpclient/ssltest/CertificateTest.java shouldn't use a 
specific TLS version.

And it would be better not to use binary key store files.
Since DSA is not supported by TLSv1.3, this fix also updates the 
certificates to use RSA.


Webrev: http://cr.openjdk.java.net/~jjiang/8238677/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8238677

Best regards,
John Jiang

Re: RFR[15] 8238677: java/net/httpclient/ssltest/CertificateTest.java should not specify TLS version

[sha . jiang]

Hi Daniel,

On 2020/2/7 19:29, Daniel Fuchs wrote:

Hi John,

Looks good to me. Thanks for taking care of this!
I'm glad to see the binary files go away :-)

Thanks for your review!



Would it be possible to include a comment in Cert.java that contains
the command you used to generate the certificates?

That will be a great help to future maintainers if the certificates
ever needs to be re-generated (e.g. to update the expiry date
etc...)


I'll do that.

Best regards,
John Jiang



best regards,

-- daniel

Disclaimer: I am not an expert in ssl/security


On 07/02/2020 10:51, sha.ji...@oracle.com wrote:

Hi,
java/net/httpclient/ssltest/CertificateTest.java shouldn't use a 
specific TLS version.

And it would be better not to use binary key store files.
Since DSA is not supported by TLSv1.3, this fix also updates the 
certificates to use RSA.


Webrev: http://cr.openjdk.java.net/~jjiang/8238677/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8238677

Best regards,
John Jiang

RFR[15] 8238677: java/net/httpclient/ssltest/CertificateTest.java should not specify TLS version

[sha . jiang]

Hi,
java/net/httpclient/ssltest/CertificateTest.java shouldn't use a 
specific TLS version.

And it would be better not to use binary key store files.
Since DSA is not supported by TLSv1.3, this fix also updates the 
certificates to use RSA.


Webrev: http://cr.openjdk.java.net/~jjiang/8238677/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8238677

Best regards,
John Jiang

RFR[14] JDK-8234727: sun/security/ssl/X509TrustManagerImpl tests support TLSv1.3

[sha . jiang]

Hi,
Because the below tests use MD5withRSA certificates, so TLSv1.3 cannot 
be used.

test/jdk/sun/security/ssl/X509TrustManagerImpl/BasicConstraints.java
test/jdk/sun/security/ssl/X509TrustManagerImpl/SelfIssuedCert.java

This fix just re-generates the certificates to use SHA256withRSA.

Issue: https://bugs.openjdk.java.net/browse/JDK-8234727
Webrev: http://cr.openjdk.java.net/~jjiang/8234727/webrev.00/

Best regards,
John Jiang

Re: RFR[14] JDK-8235813: System property fullCipherSuites is not used by javax/net/ssl/compatibility/Compatibility.java

[sha . jiang]

Hi Xuelei,

On 2019/12/16 11:22, Xuelei Fan wrote:

I think the property is for test code only, is it?

Yes. It is only used by this test.



The update looks fine to me.

Thanks!

Best regards,
John Jiang


Xuelei


On Dec 15, 2019, at 6:09 PM, sha.ji...@oracle.com wrote:

Hi,
javax/net/ssl/compatibility/Compatibility.java should use property 
fullCipherSuites to indicate if testing all of specific cipher suites.
In addition, the cases on TLS_ECDH_RSA cipher suites fail due to a RSA-signed 
EC key certificate has something wrong.

Issue: https://bugs.openjdk.java.net/browse/JDK-8235813
Webrev: http://cr.openjdk.java.net/~jjiang/8235813/webrev.00/

Best regards,
John Jiang

RFR[14] JDK-8235813: System property fullCipherSuites is not used by javax/net/ssl/compatibility/Compatibility.java

[sha . jiang]

Hi,
javax/net/ssl/compatibility/Compatibility.java should use property 
fullCipherSuites to indicate if testing all of specific cipher suites.
In addition, the cases on TLS_ECDH_RSA cipher suites fail due to a 
RSA-signed EC key certificate has something wrong.


Issue: https://bugs.openjdk.java.net/browse/JDK-8235813
Webrev: http://cr.openjdk.java.net/~jjiang/8235813/webrev.00/

Best regards,
John Jiang

RFR[14] JDK-8231810: javax/net/ssl/templates/SSLSocketSSLEngineTemplate.java fails intermittently with "java.lang.Exception: Unexpected EOF"

[sha . jiang]

Hi,
When the server socket in this test tries to receive close_notify, 
possibly the client socket is (being) closed.


Without this patch, this failure was easily reproduced by running the 
test in a loop.

With this patch, I didn't reproduce it in the same way.

Issue: https://bugs.openjdk.java.net/browse/JDK-8231810
Webrev: http://cr.openjdk.java.net/~jjiang/8231810/webrev.00/

Best regards,
John Jiang

RFR[14] JDK-8235255: ProblemList javax/net/ssl/templates/SSLSocketSSLEngineTemplate.java

[sha . jiang]

Hi,
It needs to put javax/net/ssl/templates/SSLSocketSSLEngineTemplate.java 
into ProblemList due to JDK-8231810.


diff -r aa12d1f0bc66 test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt    Tue Dec 03 14:10:53 2019 +
+++ b/test/jdk/ProblemList.txt    Tue Dec 03 22:31:15 2019 +0800
@@ -681,6 +681,7 @@
 javax/net/ssl/DTLS/PacketLossRetransmission.java 8169086 macosx-x64
 javax/net/ssl/DTLS/RespondToRetransmit.java 8169086 macosx-x64
 javax/net/ssl/DTLS/CipherSuite.java 8202059 macosx-x64
+javax/net/ssl/templates/SSLSocketSSLEngineTemplate.java 8231810 generic-all

 sun/security/provider/KeyStore/DKSTest.sh 8180266 windows-all

Best regards,
John Jiang

RFR[14] JDK-8234724: javax/net/ssl/templates/SSLSocketSSLEngineTemplate.java supports TLSv1.3

[sha . jiang]

Hi,
This simple patch just takes test 
javax/net/ssl/templates/SSLSocketSSLEngineTemplate.java to cover TLSv1.3.


diff -r 88502b1cf76f 
test/jdk/javax/net/ssl/templates/SSLSocketSSLEngineTemplate.java
--- a/test/jdk/javax/net/ssl/templates/SSLSocketSSLEngineTemplate.java 
Mon Sep 09 11:43:16 2019 -0400
+++ b/test/jdk/javax/net/ssl/templates/SSLSocketSSLEngineTemplate.java 
Thu Nov 28 16:07:12 2019 +0800

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2016, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2011, 2019, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -124,7 +124,6 @@
 private static final String pathToStores = "../etc";
 private static final String keyStoreFile = "keystore";
 private static final String trustStoreFile = "truststore";
-    private static final String passwd = "passphrase";
 private static final String keyFilename =
 System.getProperty("test.src", ".") + "/" + pathToStores
 + "/" + keyStoreFile;
@@ -146,7 +145,7 @@
 }

 String [] protocols = new String [] {
-    "SSLv3", "TLSv1", "TLSv1.1", "TLSv1.2" };
+    "SSLv3", "TLSv1", "TLSv1.1", "TLSv1.2", "TLSv1.3" };

 for (String protocol : protocols) {
 log("Testing " + protocol);

Best regards,
John Jiang

Re: RFR: 8231351: Add notes for PKCS11 tests in the test doc

[sha . jiang]

Hi Jie,
IIRC, this test passed on your Ubuntu 18.04 with a new built NSS 3.35 libs.
So, I suspected your Linux or the system built-in NSS libs had something 
wrong.
Now that this test passed with NSS 3.35 on others' Linux, including 
mine, it may not make sense that this test is skipped for NSS 3.35 on 
all platforms (even Linux only).

Certainly, you are always welcomed to re-open JDK-8231338.

A bit furthermore...
Some different PKCS11 test cases have been skipped due to the bugs on 
different NSS releases or platforms.
It would be better not to make the codes more complicated to deal with 
this scenario.
The alternative NSS libs could be specified (via system property 
test.nss.lib.paths) for PKCS11 tests.

It doesn't have to depend on the system built-in NSS libs.
Different people can use different NSS versions based on their requirements.

Best regards,
John Jiang

On 2019/9/24 15:03, Jie Fu wrote:
I can't understand why JDK-8231338 was closed as "Cannot Reproduce" 
since it can always be reproduced on Ubuntu 18.04.2 LTS.


Reproduce on Ubuntu 18.04 is very simple:
-
make test TEST="jtreg:sun/security/pkcs11/Secmod/AddTrustedCert.java" 
CONF=re

-

In the fix of JDK-8180837, the NSS-3.35 was assumed to work with 
AddTrustedCert.java, but it failed on Ubuntu 18.04.
I wonder whether there is some other reasons which may lead to the 
failure.

So I filed JDK-8231338 hoping to find the root cause of the failure.

But I was disappointed since the author of JDK-8180837 just told me:
 "it's hard to say what's the problem, Linux, NSS build or others, on 
this test case."


In fact, the root cause for the failure on Ubuntu 18.04 still remains 
unknown.


Thanks a lot.
Best regards,
Jie

On 2019/9/24 下午2:01, Xuelei Fan wrote:
I may be a little bit hesitate to add such words, "highly recommended 
to use the latest NSS version ...", in the general TOP doc.  There 
are a few issues that I wary about:


It is not always expected that all PKCS 11 test should be run on 
latest NSS version.  Otherwise, there might be compatibility issues 
that we did not handle properly.  The JDK is expected to work with as 
much NSS versions as possible, not just the latest version.  It is 
good to know which version does not really work because of a specific 
bug.


The note should be added as close as to the place where the issue 
happens, for maintaining and searching.  I think the best place could 
be the test code where the failure occurs 
(test/jdk/sun/security/pkcs11/Secmod/AddTrustedCert.java?).


However, I'm still not sure if we really want this note.

JDK-8231338 is reported with NSS 3.35.  Per the comment, the test 
could pass with NSS 3.35 on Debian and Ubuntu, and the bug submitter 
could pass the test with a proper build of NSS 3.35. And then the bug 
was closed as "Cannot Reproduce".  I think we are done with the bug.  
It might not be necessary to add a note any more.


Just my $.02.

Xuelei

On 9/23/2019 9:06 PM, Jia Huang wrote:

Hi John Jiang,

Thank you for your review.

在 2019年09月23日 20:54, sha.ji...@oracle.com 写道:
In fact, PKCS11 tests have their own doc at: 
test/jdk/sun/security/pkcs11/README 
I'm afraid most people wouldn't see 
test/jdk/sun/security/pkcs11/README at all.

So it makes very little sense to add the notes in it.
I still prefer doc/testing.md.

A reference to test/jdk/sun/security/pkcs11/README had been added in 
[1].


Thanks a lot.
Best regards,
Jia

[1] http://cr.openjdk.java.net/~jiefu/8231351-huangjia/webrev.01/

Re: RFR: 8231351: Add notes for PKCS11 tests in the test doc

[sha . jiang]

Hi Jia,
I think this isn't a general testing problem.
It may not worthy of highlighting this point in the JDK testing doc.
In fact, PKCS11 tests have their own doc at: 
test/jdk/sun/security/pkcs11/README


Best regards,
John Jiang

On 2019/9/23 18:04, Jia Huang wrote:

Hi all,

JBS:    https://bugs.openjdk.java.net/browse/JDK-8231351
Webrev: http://cr.openjdk.java.net/~jiefu/8231351-huangjia/webrev.00/

sun/security/pkcs11/Secmod/AddTrustedCert.java failed on Ubuntu 18.04.
According to the comments in JDK-8231338, it was caused by the 
improper NSS libs of the system.


These failures are confusing and hard to diagnose.
It might be better to add some notes for the pkcs11 tests.

Thanks a lot.

Best regards,
Jia

RFR[14] JDK-8180837: SunPKCS11-NSS tests failing with CKR_ATTRIBUTE_READ_ONLY and CKR_MECHANISM_PARAM_INVALID

[sha . jiang]

Hi,
Tests sun/security/pkcs11/Secmod/AddTrustedCert.java and 
sun/security/pkcs11/tls/TestKeyMaterial.java failed due to some changes 
in NSS.
This fix just workarounds this problem. For more details, please see the 
comments in this JBS issue.
And it also provides a pre-built NSS libs for linux-x64 platform. Then, 
the PKCS11 tests won't depend on the system built-in NSS libs in our CI 
on this platform.


Issue: https://bugs.openjdk.java.net/browse/JDK-8180837
Webrev: http://cr.openjdk.java.net/~jjiang/8180837/webrev.00/

Best regards,
John Jiang

Re: FRF[14] JDK-8228967: Trust/Key store and SSL context utilities for tests

[sha . jiang]

Ping...

In the future, we would use these utilities to refactor some existing 
tests, which depend on binary trust/key stores.


I just run all tests in java/security, javax/security, javax/net/ssl and 
sun/security with this patch.

The results are fine.

Best regards,
John Jiang

On 2019/8/19 15:17, sha.ji...@oracle.com wrote:

Could this patch be reviewed?

Best regards,
John

On 2019/8/4 07:32, sha.ji...@oracle.com wrote:

Hi Sean,
I moved test/jdk/java/security/testlibrary/CertUtils.java  to 
test/lib/jdk/test/lib/security/CertUtils.java, and added the new 
methods to this class.

The affected existing tests are also modified.

http://cr.openjdk.java.net/~jjiang/8228967/webrev.01/

Best regards,
John Jiang

On 2019/8/2 19:26, Sean Mullan wrote:
Not a full review but there is already a utility class for creating 
certificates, etc in test/jdk/java/security/testlibrary/CertUtils.java

Could you combine/merge that with your new CertUtils class?

--Sean

On 8/2/19 4:29 AM, sha.ji...@oracle.com wrote:

Hi,
This enhancement provides a set of utilities for creating 
certificate, trust/key store and SSL context.
It provides the default trust and key stores with RSA, ECDSA, 
RSASSA-PSS and DSA certificates, and also the default TLS and DTLS 
contexts with the default trust and key stores.

Three tests are modified for demonstrating the usages.

Webrev: http://cr.openjdk.java.net/~jjiang/8228967/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8228967

Best regards,
John Jiang

RFR[14] JDK-8226221: Update PKCS11 tests to use NSS 3.46 libs

[sha . jiang]

Hi,
The below simple patch just take PKCS11 tests to use the latest NSS, 
exactly version 3.46, libs.


diff -r bf3fb5465543 test/jdk/sun/security/pkcs11/PKCS11Test.java
--- a/test/jdk/sun/security/pkcs11/PKCS11Test.java    Tue Sep 03 
17:45:02 2019 +0300
+++ b/test/jdk/sun/security/pkcs11/PKCS11Test.java    Wed Sep 04 
11:19:43 2019 +0800

@@ -906,21 +906,21 @@
 @Artifact(
 organization = "jpg.tests.jdk.nsslib",
 name = "nsslib-windows_x64",
-    revision = "3.41-VS2017",
+    revision = "3.46-VS2017",
 extension = "zip")
 private static class WINDOWS_X64 { }

 @Artifact(
 organization = "jpg.tests.jdk.nsslib",
 name = "nsslib-windows_x86",
-    revision = "3.41-VS2017",
+    revision = "3.46-VS2017",
 extension = "zip")
 private static class WINDOWS_X86 { }

 @Artifact(
 organization = "jpg.tests.jdk.nsslib",
 name = "nsslib-macosx_x64",
-    revision = "3.41",
+    revision = "3.46",
 extension = "zip")
 private static class MACOSX_X64 { }
 }

Best regards,
John Jiang

Re: FRF[14] JDK-8228967: Trust/Key store and SSL context utilities for tests

[sha . jiang]

Could this patch be reviewed?

Best regards,
John

On 2019/8/4 07:32, sha.ji...@oracle.com wrote:

Hi Sean,
I moved test/jdk/java/security/testlibrary/CertUtils.java  to 
test/lib/jdk/test/lib/security/CertUtils.java, and added the new 
methods to this class.

The affected existing tests are also modified.

http://cr.openjdk.java.net/~jjiang/8228967/webrev.01/

Best regards,
John Jiang

On 2019/8/2 19:26, Sean Mullan wrote:
Not a full review but there is already a utility class for creating 
certificates, etc in test/jdk/java/security/testlibrary/CertUtils.java

Could you combine/merge that with your new CertUtils class?

--Sean

On 8/2/19 4:29 AM, sha.ji...@oracle.com wrote:

Hi,
This enhancement provides a set of utilities for creating 
certificate, trust/key store and SSL context.
It provides the default trust and key stores with RSA, ECDSA, 
RSASSA-PSS and DSA certificates, and also the default TLS and DTLS 
contexts with the default trust and key stores.

Three tests are modified for demonstrating the usages.

Webrev: http://cr.openjdk.java.net/~jjiang/8228967/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8228967

Best regards,
John Jiang

Re: FRF[14] JDK-8228967: Trust/Key store and SSL context utilities for tests

[sha . jiang]

Hi Sean,
I moved test/jdk/java/security/testlibrary/CertUtils.java  to 
test/lib/jdk/test/lib/security/CertUtils.java, and added the new methods 
to this class.

The affected existing tests are also modified.

http://cr.openjdk.java.net/~jjiang/8228967/webrev.01/

Best regards,
John Jiang

On 2019/8/2 19:26, Sean Mullan wrote:
Not a full review but there is already a utility class for creating 
certificates, etc in test/jdk/java/security/testlibrary/CertUtils.java

Could you combine/merge that with your new CertUtils class?

--Sean

On 8/2/19 4:29 AM, sha.ji...@oracle.com wrote:

Hi,
This enhancement provides a set of utilities for creating 
certificate, trust/key store and SSL context.
It provides the default trust and key stores with RSA, ECDSA, 
RSASSA-PSS and DSA certificates, and also the default TLS and DTLS 
contexts with the default trust and key stores.

Three tests are modified for demonstrating the usages.

Webrev: http://cr.openjdk.java.net/~jjiang/8228967/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8228967

Best regards,
John Jiang

FRF[14] JDK-8228967: Trust/Key store and SSL context utilities for tests

[sha . jiang]

Hi,
This enhancement provides a set of utilities for creating certificate, 
trust/key store and SSL context.
It provides the default trust and key stores with RSA, ECDSA, RSASSA-PSS 
and DSA certificates, and also the default TLS and DTLS contexts with 
the default trust and key stores.

Three tests are modified for demonstrating the usages.

Webrev: http://cr.openjdk.java.net/~jjiang/8228967/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8228967

Best regards,
John Jiang

RFR[13] JDK-8228403: SignTwice.java failed with java.io.FileNotFoundException: File name too long

[sha . jiang]

Hi,
The Java runtime version of JDK builds may be long, then the path to the 
generated jars, which use this version as part of file name, would be 
too long.
This fix takes the jar names to use Java version, which should be much 
shorter.


Now, the report table displays the Java versions and the associated 
indices in the JDK list.
Please take a look at the report sample: 
http://cr.openjdk.java.net/~jjiang/8228403/webrev.00/report/report.html


Webrev: http://cr.openjdk.java.net/~jjiang/8228403/webrev.00/
Issue : https://bugs.openjdk.java.net/browse/JDK-8228403

Best regards,
John Jiang

Re: RFR[13]: 8227551 Session Resumption without Server-Side State off by default

[sha . jiang]

Hi Tony,
Just a minor comment.

 242 if (st.compareToIgnoreCase("true") == 0) {
 243 statelessSession = true;
 244 }
Could it simply use the below statement?
statelessSession = st.equalsIgnoreCase("true");

Best regards,
John Jiang

On 2019/7/17 11:12, Anthony Scarpino wrote:

Please review the change to set stateless session resumption off default

http://cr.openjdk.java.net/~ascarpino/8227551/webrev/

Thanks

Tony

Re: RFR 8217375: jarsigner breaks old signature with long lines in manifest

[sha . jiang]

Hi,
Sorry for this late reply!

I only focus on the changes on the tests in 
sun/security/tools/jarsigner/compatibility.
Compatibility.java has been modified significantly, I just had a quick 
look at it.


sun/security/tools/jarsigner/compatibility/SignTwice.java

With my testing, this test usually got timeout in our CI testing system.
The timeout is 480 seconds.


sun/security/tools/jarsigner/compatibility/Compatibility.java

 38 import java.io.ByteArrayOutputStream;
 44 import java.io.FilterOutputStream;
 65 import java.util.jar.Attributes;
The above import statements are unnecessary.

 82 private static JdkInfo TEST_JDK_INFO;
100 private static String[] KEY_ALGORITHMS;
111 private static String[] DIGEST_ALGORITHMS;
Now that these variables are not constant anymore, the names would be 
testJdkInfo, keyAlgorithms and digestAlgorithms.


161 System.out.println("using sigfile " + sigfileName + " for 
alias "
162 + alias + " signing " + u + ".jar to " + s + 
".jar");

Could it be better to use System.out.printf()?

409 private static String outfile() {
410 return System.getProperty("o");
411 }
Would it be better to define a constant for outfile?

416 String[] jdkList = list("jdkList");
417 if (jdkList.length == 0) {
418 jdkList = new String[] { "TEST_JDK" };
419 }
420
421 List jdkInfoList = new ArrayList<>();
422 for (String jdkPath : jdkList) {
423 JdkInfo jdkInfo = "TEST_JDK".equalsIgnoreCase(jdkPath) ?
424 TEST_JDK_INFO : new JdkInfo(jdkPath);
Not sure understand this update.
Now that the constant TEST_JDK is the current testing JDK path, then new 
JdkInfo(TEST_JDK) should be equivalent to TEST_JDK_INFO in this update.


435 private static List keyAlgs() throws IOException {
436 if (KEY_ALGORITHMS == null) KEY_ALGORITHMS = list("keyAlgs");
437 if (KEY_ALGORITHMS.length == 0)
438 return Arrays.asList(DEFAULT_KEY_ALGORITHMS);
439 return Arrays.stream(KEY_ALGORITHMS).map(a -> a.split(";")[0])
440 .collect(Collectors.toList());
441 }
...
467 private static List digestAlgs() throws IOException {
468 if (DIGEST_ALGORITHMS == null) DIGEST_ALGORITHMS = 
list("digestAlgs");

469 if (DIGEST_ALGORITHMS.length == 0)
470 return Arrays.asList(DEFAULT_DIGEST_ALGORITHMS);
471 return Arrays.asList(DIGEST_ALGORITHMS);
472 }
It looks better that the above methods return String array rather than 
list. Then, the callings on Arrays.asList() are unnecessary.


793 System.out.println("verifyingStatus: error: exit code != 
" + expectedExitCode + ": " + outputAnalyzer.getExitValue() + " != " + 
expectedExitCode);

...
799 System.out.println("verifyingStatus: error: 
expectedSuccessMessage not found: " + expectedSuccessMessage);

...
810 System.out.println("verifyingStatus: error: 
line.matches(" + Test.ERROR + "\" ?\"): " + line);

The above lines are too long.


sun/security/tools/jarsigner/compatibility/JdkUtils.java

The year should be updated in the copyright notes.

Best regards,
John Jiang

On 2019/6/21 11:34, Weijun Wang wrote:

BTW, I haven't looked at the changes in 
test/jdk/sun/security/tools/jarsigner/compatibility yet.

John, do you have any comment in this area?

Thanks,
Max


On Jun 21, 2019, at 11:32 AM, Weijun Wang  wrote:

The tests.

- For all:

We are thinking about removing default value for -keyalg for "keytool -genkeypair" some day, so it 
will be better to add an explicit "-keyalg RSA" or "-keyalg EC" now.

There are several tests printing out the manifest in a sequence of "[[0]=83='S', ...". While it is precise, I 
find it not very easy to check. How about we just print out the string format but for each "\r" we just print 
"\r" literally and keep printing "\n" with a new line,  like this:

Signature-Version: 1.0\r
Created-By: 13-internal (Oracle Corporation)\r
SHA-256-Digest-Manifest: Pxklci7ELW1zzSh2jZ+oz7TPR0WK2uTsAIiHar1m6Eo=\r
SHA-256-Digest-Manifest-Main-Attributes: ETfvdTNx3yN/ClSZz20wR0SM9ta8H7O\r
E7U0F4uZ9JV8=\r
\r
Name: abc\r
SHA-256-Digest: uTZ8UM3iJLPtl8W7+NEC/AC2auI7LP2Gu53ajj6jLgg=\r
\r

If you find running jarsigner slow, is the JarSigner::sign API any better? The 
verify side can also usually be replaced with open the file and read everything 
inside unless you want to grab certain outputs, but those can also be done with 
JarEntry::getSigners.

- DigestDontIgnoreCase.java:

getManifestBytes(). This could reformat the manifest (I understand it's 
irrelevant in this test), you can just return

   jf.getInputStream(jf.getJarEntry(JarFile.MANIFEST_NAME)).readAllBytes()

- EmptyIndividualSectionName.java:

Are you going to do something around testNameEmptyTrusted()?

- FindHeaderEndVsManifestDigesterFindFirstSection.java:

Several TODO and FIXME.

52: 

Re: [13]RFR:8224650:Add tests to support X25519 and X448 in TLS

[sha . jiang]

Hi,
Because Siba has to be offline for some days, now I take over this task.

Please review this updated webrev: 
http://cr.openjdk.java.net/~jjiang/8224650/webrev.01/
It covers more cipher suites, and changes SSLSocketTemplate.java on 
creating SSL context.
Now, SSLSocketTemplate.java contains new ECDSA certificates on curves 
secp384r1 and secp521r1.

But these new certificates are not used by default.

Run all tests in test/jdk/javax/net/ssl and test/jdk/sun/security/ssl, 
no failure raised.


Best regards,
John Jiang

On 2019/6/21 16:22, sha.ji...@oracle.com wrote:


Hi Siba,
I have some minor comments.

Now that JDK-8225766 has been fixed, I suppose this test can cover 
some ECDHE_ECDSA cipher suites.


  48 private static volatile int index;
  ...
  56 for (String c : getCiphers(protocols[index], args[0])) {
  ...
  66 String[] ps = new String[]{protocols[index]};
Could it directly use the protocol value, but not the index in the 
protocol array?
Could these cases run concurrently? Otherwise, volatile may be 
unnecessary.
In fact, I think both of parameters cipher and index (or directly 
protocol) would not be static.
They would be the members of class NamedGroupsWithCipherSuite, and can 
be passed to the class constructor.
Then, every case run, say "new NamedGroupsWithCipherSuite(cipher, 
protocol).run()", could not concern these TLS parameters are modified 
by others.


 123 /**
 124  * Get some TLSv1.1 supported ciphers.
 125  */
 126 private static List tlsCiphers() {
 ...
 131
 132 /**
 133  * Get some TLSv1.1 supported ciphers.
 134  */
 135 private static List dheCiphers() {
The above methods would have different docs.

More spaces would be needed in the array initialization statements, 
for example,

  66 String[] ps = new String[]{protocols[index]};
  71 socket.setEnabledCipherSuites(new String[]{cipher});
Of course, this point is trivial.

Best regards,
John Jiang

On 2019/6/21 14:59, Sibabrata Sahoo wrote:


Hi Xuelei/Brad,

Please review the patch for,

JBS: https://bugs.openjdk.java.net/browse/JDK-8224650

Webrev: http://cr.openjdk.java.net/~ssahoo/8224650/webrev.00/

This is a small Test inherited from “SSLSocketTemplate” and reuse 
most part of it. The only difference is, it uses supported named 
groups along with a fixed set of ciphers supported with different TLS 
protocols. Though there are large number of supported ciphers but I 
have selected few to ensure the Test does not take much time to 
complete the execution. Please let me know if you have any suggestion 
for improvement.


Thanks,

Siba

Re: [13]RFR:8224650:Add tests to support X25519 and X448 in TLS

[sha . jiang]

Hi Siba,
I have some minor comments.

Now that JDK-8225766 has been fixed, I suppose this test can cover some 
ECDHE_ECDSA cipher suites.


  48 private static volatile int index;
  ...
  56 for (String c : getCiphers(protocols[index], args[0])) {
  ...
  66 String[] ps = new String[]{protocols[index]};
Could it directly use the protocol value, but not the index in the 
protocol array?

Could these cases run concurrently? Otherwise, volatile may be unnecessary.
In fact, I think both of parameters cipher and index (or directly 
protocol) would not be static.
They would be the members of class NamedGroupsWithCipherSuite, and can 
be passed to the class constructor.
Then, every case run, say "new NamedGroupsWithCipherSuite(cipher, 
protocol).run()", could not concern these TLS parameters are modified by 
others.


 123 /**
 124  * Get some TLSv1.1 supported ciphers.
 125  */
 126 private static List tlsCiphers() {
 ...
 131
 132 /**
 133  * Get some TLSv1.1 supported ciphers.
 134  */
 135 private static List dheCiphers() {
The above methods would have different docs.

More spaces would be needed in the array initialization statements, for 
example,

  66 String[] ps = new String[]{protocols[index]};
  71 socket.setEnabledCipherSuites(new String[]{cipher});
Of course, this point is trivial.

Best regards,
John Jiang

On 2019/6/21 14:59, Sibabrata Sahoo wrote:


Hi Xuelei/Brad,

Please review the patch for,

JBS: https://bugs.openjdk.java.net/browse/JDK-8224650

Webrev: http://cr.openjdk.java.net/~ssahoo/8224650/webrev.00/

This is a small Test inherited from “SSLSocketTemplate” and reuse most 
part of it. The only difference is, it uses supported named groups 
along with a fixed set of ciphers supported with different TLS 
protocols. Though there are large number of supported ciphers but I 
have selected few to ensure the Test does not take much time to 
complete the execution. Please let me know if you have any suggestion 
for improvement.


Thanks,

Siba

RFR JDK-8225390: ProblemList sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java due to JDK-8161536

[sha . jiang]

Hi,
Test sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java should be in 
ProblemList until JDK-8161536 is resolved.


diff -r 184b05daf50f test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt    Wed Jun 05 21:50:29 2019 -0400
+++ b/test/jdk/ProblemList.txt    Thu Jun 06 09:58:13 2019 +0800
@@ -660,6 +660,7 @@
 sun/security/pkcs11/Secmod/AddTrustedCert.java 8180837 generic-all
 sun/security/pkcs11/tls/TestKeyMaterial.java 8180837 generic-all
 sun/security/pkcs11/tls/tls12/FipsModeTLS12.java 8224954 windows-all
+sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java 8161536 generic-all

 sun/security/tools/keytool/ListKeychainStore.sh 8156889 macosx-all
 sun/security/tools/keytool/KeyToolTest.java 8224644 solaris-all

Best regards,
John Jiang

Re: RFR 8211018: Session Resumption without Server-Side State

[sha . jiang]

Hi Tony,

On 2019/6/5 00:46, Anthony Scarpino wrote:


125 if (secondSession.getCreationTime() > secondStartTime &&
126 !clientCache && !serverServerless) {
127 throw new RuntimeException("Session was not 
reused");

128 }
If the session should be resumed via session ID, beside checking the 
creation time, would it be better to compare the session IDs for 
double-checking?


the client side in stateless mode sends no session id, as the spec 
allows.  So the session id has no more value.
If either peer doesn't enable the session ticket extension, the session 
would be resumed via old cache way, but not RFC 5077.
For this case, I suppose it would be better to check the session IDs 
between two connections.
This checking indicates the session MAY not be resumed via RFC 5077 and 
the new properties should work as expected.


Best regards,
John Jiang

Re: RFR 8211018: Session Resumption without Server-Side State

[sha . jiang]

ResumeChecksServer.java

57 static boolean clientCache = false, serverServerless = false;
Should "serverServerless" be "serverStateless"?

86 if (st.compareToIgnoreCase("stateless") == 0) {
87 serverServerless = true;
88 }
89 st = System.getProperty("javax.net.ssl.sessionCacheClient", 
"cache");

90 if (st.compareToIgnoreCase("cache") == 0) {
91 clientCache = true;
92 }
Could method String::equalsIgnoreCase be used in the condition 
statements?
In addition, the client side property is 
"jdk.tls.client.enableSessionTicketExtension".
And the new introduced properties should be bool values, and they should 
not have such "stateless" and "cache" values.


Best regards,
John Jiang

Re: RFR 8211018: Session Resumption without Server-Side State

[sha . jiang]

Hi Tony,
I have some minor comments on the test.

ResumeChecksServer.java

57 static boolean clientCache = false, serverServerless = false;
Should "serverServerless" be "serverStateless"?

86 if (st.compareToIgnoreCase("stateless") == 0) {
87 serverServerless = true;
88 }
89 st = System.getProperty("javax.net.ssl.sessionCacheClient", 
"cache");

90 if (st.compareToIgnoreCase("cache") == 0) {
91 clientCache = true;
92 }
Could method String::equalsIgnoreCase be used in the condition statements?

125 if (secondSession.getCreationTime() > secondStartTime &&
126 !clientCache && !serverServerless) {
127 throw new RuntimeException("Session was not reused");
128 }
If the session should be resumed via session ID, beside checking the 
creation time, would it be better to compare the session IDs for 
double-checking?


Best regards,
John Jiang

On 2019/6/4 08:42, Anthony Scarpino wrote:

I believe I updated all comments in the latest webrev.

http://cr.openjdk.java.net/~ascarpino/stateless/webrev.02

Tony

On 5/16/19 2:30 PM, Anthony Scarpino wrote:
I'm asking for a review of this rather large change to add support 
stateless tickets in the TLS 1.3 5077 RFC.

https://bugs.openjdk.java.net/browse/JDK-8211018

Re: RFR 8211018: Session Resumption without Server-Side State

[sha . jiang]

Hi Tony,
The new introduced properties in the source and CSR are 
jdk.tls.client.sessionTicketExtension and jdk.tls.server.sessionCacheState,
but the new tests ResumeChecksClientStateless.java and 
ResumeChecksServerStateless.java use 
javax.net.ssl.sessionTicketExtension and javax.net.ssl.sessionCacheServer.


Best regards,
John Jiang

On 2019/5/22 08:35, Anthony Scarpino wrote:

Hi all,

I’ve updated in-place some recent changes due to some additional testing

Tony


On May 16, 2019, at 2:30 PM, Anthony Scarpino  
wrote:

I'm asking for a review of this rather large change to add support stateless 
tickets in the TLS 1.3 5077 RFC.
https://bugs.openjdk.java.net/browse/JDK-8211018

http://cr.openjdk.java.net/~ascarpino/stateless/webrev.00

thanks

Tony

Re: RFR[13] JDK-8204203: Many pkcs11 tests failed in Provider initialization, after compiler on Windows changed

[sha . jiang]

Hi Valeria,

On 2019/5/1 04:25, Valerie Peng wrote:

Looks fine to me.

Thanks for your review!

With further testing, sun/security/tools/keytool/NssTest.java may fail 
due to some dependencies are not found.
But this failure should not be related to this NSS libs upgrade, because 
it could also raise without this fix.

So, just loading the dependencies explicitly.
Please review the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8204203/webrev.01/




Just curious, how do we see the changes on the artifactory side?


Please search the below files in our Artifactory.
nsslib-macosx_x64-3.41.zip
nsslib-windows_x64-3.41-VS2017.zip
nsslib-windows_x86-3.41-VS2017.zip

Best regards,
John Jiang



Valerie

On 4/23/2019 7:39 PM, sha.ji...@oracle.com wrote:

Hi,
NSS 3.41 has been approved, so just build this version for windows 
(with VS2017) and macosx.


Webrev: http://cr.openjdk.java.net/~jjiang/8204203/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8204203

Best regards,
John Jiang

RFR[13] JDK-8204203: Many pkcs11 tests failed in Provider initialization, after compiler on Windows changed

[sha . jiang]

Hi,
NSS 3.41 has been approved, so just build this version for windows (with 
VS2017) and macosx.


Webrev: http://cr.openjdk.java.net/~jjiang/8204203/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8204203

Best regards,
John Jiang

RFR[13] JDK-8222391: javax/net/ssl/compatibility/Compatibility.java should be more flexible

[sha . jiang]

Hi,
This fix just makes some improvements for the manual test 
javax/net/ssl/compatibility/Compatibility.java in the following points:

1. The test cases could be customized.
2. The current testing JDK (namely, the test.jdk) always be covered.
3. It allows to exclude the test cases if the JSSE server and client are 
from a same JDK build.

4. An alternative security properties file can be specified.

These changes could be used by an auto test, which is derived from this 
manual test.


Webrev: http://cr.openjdk.java.net/~jjiang/8222391/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8222391

Best regards,
John Jiang

Re: RFR[13] JDK-8220410: sun/security/tools/jarsigner/warnings/NoTimestampTest.java failed with missing expected output

[sha . jiang]

Hi Max,

On 2019/3/18 22:04, Weijun Wang wrote:

Hi John,

The new webrev looks mostly fine, except that I don't like the new lambda in 
Test.java. But I'll leave it to you to decide if that style is good.

Have you been able to reproduce the original test failure and make sure your 
updated test passes in the same environment? Otherwise, I cannot be sure if the 
fix is at the right spot.


I had reproduced this issue. It only raised at the midnight.
The cause is what I mentioned on "-J-Duser.timezone=null".
The test passed with my fix.

Best regards,
John Jiang



Thanks,
Max


On Mar 18, 2019, at 8:28 PM, sha.ji...@oracle.com wrote:

Hi Max,
If using "-J-Duser.timezone=null", it looks jarsigner always uses timezone 
GMT-8/PST, but not the local timezone.
Currently, the testing machines should use GMT-7/PDT.
If no user.timezone is specified, jarsigner just uses local timezone.

Please review the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8220410/webrev.01/
It doesn't specify user.timezone for jarsigner if this property is null, and 
still makes sure jarsigner and date formatter use the same timezone.

Best regards,
John Jiang

On 2019/3/14 09:52, Weijun Wang wrote:

On Mar 13, 2019, at 4:58 PM, sha.ji...@oracle.com wrote:

Hi,
This fix just makes sure that specified timezone is not null,

Or maybe if it's null then shall we also not pass it to the tools? If I understand 
correctly, by doing this we also test the default timezone case, which I believe is more 
common. (At least I don't see it in "java -XshowSettings:properties" on my 
laptop).

BTW, are you able to reproduce the failure by faking the time? All failures 
happened from 2019-03-11T07:23:34Z to 2019-03-11T07:32:36Z.

--Max


and the jar verifying and date formatting use the same different timezone.

Webrev: http://cr.openjdk.java.net/~jjiang/8220410/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8220410

Best regards,
John Jiang

Re: RFR[13] JDK-8220410: sun/security/tools/jarsigner/warnings/NoTimestampTest.java failed with missing expected output

[sha . jiang]

Hi Max,
If using "-J-Duser.timezone=null", it looks jarsigner always uses 
timezone GMT-8/PST, but not the local timezone.

Currently, the testing machines should use GMT-7/PDT.
If no user.timezone is specified, jarsigner just uses local timezone.

Please review the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8220410/webrev.01/
It doesn't specify user.timezone for jarsigner if this property is null, 
and still makes sure jarsigner and date formatter use the same timezone.


Best regards,
John Jiang

On 2019/3/14 09:52, Weijun Wang wrote:



On Mar 13, 2019, at 4:58 PM, sha.ji...@oracle.com wrote:

Hi,
This fix just makes sure that specified timezone is not null,

Or maybe if it's null then shall we also not pass it to the tools? If I understand 
correctly, by doing this we also test the default timezone case, which I believe is more 
common. (At least I don't see it in "java -XshowSettings:properties" on my 
laptop).

BTW, are you able to reproduce the failure by faking the time? All failures 
happened from 2019-03-11T07:23:34Z to 2019-03-11T07:32:36Z.

--Max


and the jar verifying and date formatting use the same different timezone.

Webrev: http://cr.openjdk.java.net/~jjiang/8220410/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8220410

Best regards,
John Jiang

Re: RFR[13] JDK-8220410: sun/security/tools/jarsigner/warnings/NoTimestampTest.java failed with missing expected output

[sha . jiang]

On 2019/3/13 16:58, sha.ji...@oracle.com wrote:


Hi,
This fix just makes sure that specified timezone is not null, and the 
jar verifying and date formatting use the same different timezone.

... use the same (not different) timezone.

John



Webrev: http://cr.openjdk.java.net/~jjiang/8220410/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8220410

Best regards,
John Jiang

RFR[13] JDK-8220410: sun/security/tools/jarsigner/warnings/NoTimestampTest.java failed with missing expected output

[sha . jiang]

Hi,
This fix just makes sure that specified timezone is not null, and the 
jar verifying and date formatting use the same different timezone.


Webrev: http://cr.openjdk.java.net/~jjiang/8220410/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8220410

Best regards,
John Jiang

Re: RFR[13] JDK-8219723: javax/net/ssl/compatibility/Compatibility.java failed on some SNI cases

[sha . jiang]

Hi Jamil,
Thanks for your suggestion.

Best regards,
John Jiang

On 2019/3/1 01:54, Jamil Nimeh wrote:
This comment doesn't directly apply to this fix, but tests that depend 
on the certs from Cert.java and have configurable PKIXParameters might 
also just set setDate to something in the middle of this validity 
period so you never have to worry about regenerating the certs due to 
date issues.  That may not work for all your tests though, if  you're 
not customizing the trust managers with PKIXParameter objects.


Just something to consider as you're going forward, no changes to this 
webrev.


--Jamil

On 2/27/2019 6:20 PM, sha.ji...@oracle.com wrote:

Hi,
Test javax/net/ssl/compatibility/Compatibility.java failed due to the 
associated certificates are expired.
This fix just re-generates the certificates and set much longer 
validity period.


Issue: https://bugs.openjdk.java.net/browse/JDK-8219723
Webrev: http://cr.openjdk.java.net/~jjiang/8219723/webrev.00/

Best regards,
John Jiang

RFR[13] JDK-8219723: javax/net/ssl/compatibility/Compatibility.java failed on some SNI cases

[sha . jiang]

Hi,
Test javax/net/ssl/compatibility/Compatibility.java failed due to the 
associated certificates are expired.
This fix just re-generates the certificates and set much longer validity 
period.


Issue: https://bugs.openjdk.java.net/browse/JDK-8219723
Webrev: http://cr.openjdk.java.net/~jjiang/8219723/webrev.00/

Best regards,
John Jiang

RFR[13] JDK-8215524: Finished message validation failure should be decrypt_error alert

[sha . jiang]

Hi,
When Finished message is validated failed, illegal_parameter is raised 
currently. But per RFC 8446 section 6.2, this error should alert 
decrypt_error.
And according to the same section, if the length of verify_data in 
Finished is incorrect, it should alert decode_error rather than 
illegal_parameter.


This fix is verified by fuzzing testing, but it's hard to add a new 
regression test.


Webrev: http://cr.openjdk.java.net/~jjiang/8215524/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8215524

Best regards,
John Jiang

RFR[13] JDK-8146392: sun/security/tools/keytool/selfissued.sh failed with ar_SA locale

[sha . jiang]

Hi,
This fix just makes sure that the keytool in test 
sun/security/tools/keytool/selfissued.sh uses en_US locale,

or the test fails with ar_SA.

diff -r bc20d0376402 test/jdk/sun/security/tools/keytool/selfissued.sh
--- a/test/jdk/sun/security/tools/keytool/selfissued.sh    Mon Jan 28 
23:00:31 2019 +0100
+++ b/test/jdk/sun/security/tools/keytool/selfissued.sh    Mon Feb 11 
14:58:34 2019 +0800

@@ -1,5 +1,5 @@
 #
-# Copyright (c) 2009, 2014, Oracle and/or its affiliates. All rights 
reserved.
+# Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights 
reserved.

 # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 #
 # This code is free software; you can redistribute it and/or modify it
@@ -45,7 +45,7 @@
 esac

 KS=selfsigned.ks
-KT="$TESTJAVA${FS}bin${FS}keytool ${TESTTOOLVMOPTS} -storepass changeit 
-keypass changeit -keystore $KS -keyalg rsa"
+KT="$TESTJAVA${FS}bin${FS}keytool ${TESTTOOLVMOPTS} 
-J-Duser.language=en -J-Duser.country=US -storepass changeit -keypass 
changeit -keystore $KS -keyalg rsa"


 rm $KS


Best regards,
John Jiang

Re: RFR[12] JDK-8203687: javax/net/ssl/compatibility/Compatibility.java supports TLS 1.3

[sha . jiang]

Hi Xuelei,

On 2019/1/18 02:00, Xuelei Fan wrote:

Hi John,

Looks fine to me except a minor format comment.

Would you mind check the line length and limit to 80 characters each 
line?


For example, using "\"  join multiple lines together.
    // openssl req -x509 -new -key key.pem \
    // -subj "/CN=RSA-2048-SHA256" -sha256 -out cer.pem


I should stick to this convention.
Please review this new webrev: 
http://cr.openjdk.java.net/~jjiang/8203687/webrev.01/

Only Cert.java is updated.

Best regards,
John Jiang



Thanks,
Xuelei

On 1/17/2019 12:26 AM, sha.ji...@oracle.com wrote:

Hi,
The patch adds TLS 1.3 cases for test 
javax/net/ssl/compatibility/Compatibility.java.
Beside this enhancement, it also changes the test on the following 
points:
1. Re-generate all certificates to use key size 2048 and SHA256 
rather than 1024 and SHA1. And new RSA signed EC key certificates are 
added.
2. Each JDK build should check if a cipher suite is supported by 
itself. It's really unnecessary to maintain a predefined table for it.
3. Uses classes in javax/net/ssl/TLSCommon for defined protocols and 
cipher suites. And also adds a new common class for key algorithm.

4. Correct some statements in README.

Webrev: http://cr.openjdk.java.net/~jjiang/8203687/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8203687

Best regards,
John Jiang

RFR[12] JDK-8203687: javax/net/ssl/compatibility/Compatibility.java supports TLS 1.3

[sha . jiang]

Hi,
The patch adds TLS 1.3 cases for test 
javax/net/ssl/compatibility/Compatibility.java.

Beside this enhancement, it also changes the test on the following points:
1. Re-generate all certificates to use key size 2048 and SHA256 rather 
than 1024 and SHA1. And new RSA signed EC key certificates are added.
2. Each JDK build should check if a cipher suite is supported by itself. 
It's really unnecessary to maintain a predefined table for it.
3. Uses classes in javax/net/ssl/TLSCommon for defined protocols and 
cipher suites. And also adds a new common class for key algorithm.

4. Correct some statements in README.

Webrev: http://cr.openjdk.java.net/~jjiang/8203687/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8203687

Best regards,
John Jiang

RFR[12] JDK-8214937: sun/security/tools/jarsigner/warnings/NoTimestampTest.java failed due to unexpected expiration date

[sha . jiang]

Hi,
This test should determine the cert expiration date from the cert 
itself, but not try to calculate that date.


Issue: https://bugs.openjdk.java.net/browse/JDK-8214937
Webrev: http://cr.openjdk.java.net/~jjiang/8214937/webrev.00/

Best regards,
John Jiang

RFR[12] JDK-8214520: [TEST_BUG] sun/security/mscapi/nonUniqueAliases/NonUniqueAliases.java failed with incorrect jtreg tags order

[sha . jiang]

Hi,
When run this test on Windows, jtreg complains that '@library' must 
appear before first action tag.

So, just adjust the tag positions.

Issue: https://bugs.openjdk.java.net/browse/JDK-8214520

diff -r a6182c464b31 
test/jdk/sun/security/mscapi/nonUniqueAliases/NonUniqueAliases.java
--- 
a/test/jdk/sun/security/mscapi/nonUniqueAliases/NonUniqueAliases.java 
Wed Dec 12 10:13:11 2018 +0530
+++ 
b/test/jdk/sun/security/mscapi/nonUniqueAliases/NonUniqueAliases.java 
Wed Dec 12 15:32:06 2018 +0800

@@ -23,11 +23,11 @@

 /*
  * @test
- * @ignore Uses certutil.exe that isn't guaranteed to be installed
  * @bug 6483657 8154113
  * @requires os.family == "windows"
  * @library /test/lib
  * @summary Test "keytool -list" displays correctly same named 
certificates

+ * @ignore Uses certutil.exe that isn't guaranteed to be installed
  */

 import jdk.test.lib.process.ProcessTools;

Best regards,
John Jiang

Re: RFR[12] JDK-8214459: NSS source should be removed

[sha . jiang]

Hi Valerie,
Do you have more concern about this issue?

I re-generate this patch with hg diff option -g. The new patch should 
delete all the files, including the directory.


diff --git 
a/test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz 
b/test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz

deleted file mode 100644
index 
63fa5feb5108f92b1cfcd2fbfc990294ed4d8fed..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391

GIT binary patch
literal 0
Hc$@diff --git 
a/test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz.sha256 
b/test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz.sha256

deleted file mode 100644
--- 
a/test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz.sha256

+++ /dev/null
@@ -1,1 +0,0 @@
-9d23633683ab3cea14519a22a997bc7f5d8d9664b6342df492c194966184ce0d 
nss-3.16-with-nspr-4.10.4.tar.gz


Best regards,
John Jiang

On 2018/11/30 10:49, sha.ji...@oracle.com wrote:

Hi Valerie,

On 2018/11/30 10:28, Valerie Peng wrote:
I assume not just the two files, their containing src directory will 
be removed, right?


Yes. This directory also will be removed.

Best regards,
John Jiang



Valerie

On 11/28/2018 10:35 PM, sha.ji...@oracle.com wrote:

Hi,
The NSS 3.16 source in test/jdk/sun/security/pkcs11/nss/src is not 
needed anymore, so just remove it.


diff -r 1d520c376105 
test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz
Binary file 
test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz 
has changed
diff -r 1d520c376105 
test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz.sha256
--- 
a/test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz.sha256 
Wed Nov 28 18:16:39 2018 -0800

+++ /dev/null    Thu Jan 01 00:00:00 1970 +
@@ -1,1 +0,0 @@
-9d23633683ab3cea14519a22a997bc7f5d8d9664b6342df492c194966184ce0d 
nss-3.16-with-nspr-4.10.4.tar.gz


Best regards,
John Jiang

Re: RFR[12] JDK-8214459: NSS source should be removed

[sha . jiang]

Hi Valerie,

On 2018/11/30 10:28, Valerie Peng wrote:
I assume not just the two files, their containing src directory will 
be removed, right?


Yes. This directory also will be removed.

Best regards,
John Jiang



Valerie

On 11/28/2018 10:35 PM, sha.ji...@oracle.com wrote:

Hi,
The NSS 3.16 source in test/jdk/sun/security/pkcs11/nss/src is not 
needed anymore, so just remove it.


diff -r 1d520c376105 
test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz
Binary file 
test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz 
has changed
diff -r 1d520c376105 
test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz.sha256
--- 
a/test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz.sha256 
Wed Nov 28 18:16:39 2018 -0800

+++ /dev/null    Thu Jan 01 00:00:00 1970 +
@@ -1,1 +0,0 @@
-9d23633683ab3cea14519a22a997bc7f5d8d9664b6342df492c194966184ce0d 
nss-3.16-with-nspr-4.10.4.tar.gz


Best regards,
John Jiang

RFR[12] JDK-8214459: NSS source should be removed

[sha . jiang]

Hi,
The NSS 3.16 source in test/jdk/sun/security/pkcs11/nss/src is not 
needed anymore, so just remove it.


diff -r 1d520c376105 
test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz
Binary file 
test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz 
has changed
diff -r 1d520c376105 
test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz.sha256
--- 
a/test/jdk/sun/security/pkcs11/nss/src/nss-3.16-with-nspr-4.10.4.tar.gz.sha256 
Wed Nov 28 18:16:39 2018 -0800

+++ /dev/null    Thu Jan 01 00:00:00 1970 +
@@ -1,1 +0,0 @@
-9d23633683ab3cea14519a22a997bc7f5d8d9664b6342df492c194966184ce0d 
nss-3.16-with-nspr-4.10.4.tar.gz


Best regards,
John Jiang

RFR[12] JDK-8212562: To remove lib/security from test/jdk/TEST.groups

[sha . jiang]

Hi,
test/jdk/lib/security doesn't exist, so this directory could be removed 
from jdk_security3


diff -r f54dcfc5a5f8 test/jdk/TEST.groups
--- a/test/jdk/TEST.groups    Fri Oct 05 15:12:37 2018 -0700
+++ b/test/jdk/TEST.groups    Wed Oct 17 15:01:36 2018 +0800
@@ -218,8 +218,7 @@
 -sun/security/krb5 \
 -sun/security/jgss \
 javax/net \
-    com/sun/net/ssl \
-    lib/security
+    com/sun/net/ssl

 jdk_security4 = \
 com/sun/security/jgss \

Best regards,
John Jiang

Re: RFR[12] JDK-8210632: Add key exchange algorithm to javax/net/ssl/TLSCommon/CipherSuite.java

[sha . jiang]

Ping...

On 2018/10/10 11:25, sha.ji...@oracle.com wrote:

Hi,
It would be better that javax/net/ssl/TLSCommon/CipherSuite.java has 
an attribute on key exchange algorithm.
This attribute could be used on selecting appropriate certificates by 
some tests.


Issue: https://bugs.openjdk.java.net/browse/JDK-8210632
Webrev: http://cr.openjdk.java.net/~jjiang/8210632/webrev.00/

Best regards,
John Jiang

RFR JDK-8211971: Move security/cacerts/VerifyCACerts.java and security/CheckBlacklistedCerts.java

[sha . jiang]

Hi,
This fix moves tests VerifyCACerts.java and CheckBlacklistedCerts.java 
in lib/security to sun/security/lib.


Issue: https://bugs.openjdk.java.net/browse/JDK-8211971
Webrev: http://cr.openjdk.java.net/~jjiang/8211971/webrev.00/

Best regards,
John JIang

Re: RFR[12] JDK-8211978: move testlibrary/jdk/testlibrary/SimpleSSLContext.java and testkeys to network testlibrary

[sha . jiang]

At least, the inner SimpleSSLContext classes in AbstractSSLTubeTest.java 
and FlowTest.java could be combined.

I'm doing it.

Best regards,
John Jiang

On 2018/10/15 17:21, Daniel Fuchs wrote:

Hi Max,

These tests are whitebox tests - the tests classes are patched
into the java.net.http module, and as a consequence they can't
compile against library classes which are on the classpath.
This is the unfortunate reason for the presence of a
SimpleSSLContext clone in there.

best regards,

-- daniel

On 15/10/2018 03:53, Weijun Wang wrote:

Hi John

I see testkeys is directly referenced in AbstractSSLTubeTest.java and 
FlowTest.java by an inner class also named SimpleSSLContext. Is it 
very different from the one in SimpleSSLContext.java? Can they be 
combined?


If so, then we have a chance to ASCII-fy the testkeys file and 
directly include it inside SimpleSSLContext.java. This way we can 
delete one more binary file and do not need to care about file 
permissions. What a marvelous gain would that be!


Thanks
Max

RFR[12] JDK-8210632: Add key exchange algorithm to javax/net/ssl/TLSCommon/CipherSuite.java

[sha . jiang]

Hi,
It would be better that javax/net/ssl/TLSCommon/CipherSuite.java has an 
attribute on key exchange algorithm.
This attribute could be used on selecting appropriate certificates by 
some tests.


Issue: https://bugs.openjdk.java.net/browse/JDK-8210632
Webrev: http://cr.openjdk.java.net/~jjiang/8210632/webrev.00/

Best regards,
John Jiang

Re: RFR[12] JDK-8209546: Make sun/security/tools/keytool/autotest.sh to support macosx

[sha . jiang]

On 2018/9/27 21:20, Weijun Wang wrote:

Change looks fine.

On PKCS11Test.java:453, you can use Files::newInputStream.

Just addressed it, please take a look at this updated webrev:
http://cr.openjdk.java.net/~jjiang/8209546/webrev.03/
It also renames the test in ProblemList.txt accordingly.

Best regards,
John Jiang

Thanks
Max


On Sep 27, 2018, at 5:34 PM,sha.ji...@oracle.com  wrote:

Hi Max,
Please review this new 
webrev:http://cr.openjdk.java.net/~jjiang/8209546/webrev.02/
You previous points, except #1, are addressed.

Best regards,
John Jiang

On 2018/9/27 11:18,sha.ji...@oracle.com  wrote:

On 2018/9/27 10:34, Weijun Wang wrote:

Hi John

1. Please add @bug to all tests.

Which issue should be linked? JDK-8209546?
I suppose @bug should indicate a product issue here.
At least, JDK-8209546 looks have no much association with this test.


2. Are getLibPath() and findLib() in AutoTest.java really necessary? It looks 
like PKCS11Test::getNSSLibDir is doing something similar.

I'll modify PKCS11Test.java a bit to help this point.


3. Looks like Standard.java is not necessary now. You can just make 
KeyToolTest.java a @test and add a @run line there, something like

  @run main/othervm/timeout=600 -Dfile KeyToolTest

Will address.


4. Maybe we can rename AutoTest.java to NssTest.java. The old name says nothing.

Will address.

Best regards,
John Jiang

Thanks
Max


On Sep 27, 2018, at 9:25 AM,sha.ji...@oracle.com  wrote:

Hi Max,
Please review the updated 
webrev:http://cr.openjdk.java.net/~jjiang/8209546/webrev.01/
All your comments are addressed, though this test is moved to problem list for 
windows due to JDK-8204203.

Best regards,
John Jiang
On 2018/9/25 22:30, Weijun Wang wrote:

Some questions:

1. Do we still need the OS check on lines 47-49? As long as getLibPath() can 
return something, does it mean the test should just run? Especially, does the 
test run on Windows?

2. Is launching a separate process necessary? Can we just call 
KeyToolTest::main after setting system properties and copying the files.

3. Is it possible to include standard.sh?

Thanks
Max



On Sep 25, 2018, at 6:30 PM,sha.ji...@oracle.com
   wrote:

Hi,
JDK-8164639 removed NSS libs from repo, so 
sun/security/tools/keytool/autotest.sh has to download NSS libs from 
artifactory on macosx.
This patch also refactors this shell test to a Java test.

Webrev:
http://cr.openjdk.java.net/~jjiang/8209546/webrev.00/

Issue:
https://bugs.openjdk.java.net/browse/JDK-8209546


Best regards,
John Jiang

Re: RFR[12] JDK-8209546: Make sun/security/tools/keytool/autotest.sh to support macosx

[sha . jiang]

Hi Max,
Please review this new webrev: 
http://cr.openjdk.java.net/~jjiang/8209546/webrev.02/

You previous points, except #1, are addressed.

Best regards,
John Jiang

On 2018/9/27 11:18, sha.ji...@oracle.com wrote:

On 2018/9/27 10:34, Weijun Wang wrote:

Hi John

1. Please add @bug to all tests.

Which issue should be linked? JDK-8209546?
I suppose @bug should indicate a product issue here.
At least, JDK-8209546 looks have no much association with this test.



2. Are getLibPath() and findLib() in AutoTest.java really necessary? 
It looks like PKCS11Test::getNSSLibDir is doing something similar.

I'll modify PKCS11Test.java a bit to help this point.



3. Looks like Standard.java is not necessary now. You can just make 
KeyToolTest.java a @test and add a @run line there, something like


 @run main/othervm/timeout=600 -Dfile KeyToolTest

Will address.



4. Maybe we can rename AutoTest.java to NssTest.java. The old name 
says nothing.

Will address.

Best regards,
John Jiang


Thanks
Max


On Sep 27, 2018, at 9:25 AM, sha.ji...@oracle.com wrote:

Hi Max,
Please review the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8209546/webrev.01/
All your comments are addressed, though this test is moved to 
problem list for windows due to JDK-8204203.


Best regards,
John Jiang
On 2018/9/25 22:30, Weijun Wang wrote:

Some questions:

1. Do we still need the OS check on lines 47-49? As long as 
getLibPath() can return something, does it mean the test should 
just run? Especially, does the test run on Windows?


2. Is launching a separate process necessary? Can we just call 
KeyToolTest::main after setting system properties and copying the 
files.


3. Is it possible to include standard.sh?

Thanks
Max



On Sep 25, 2018, at 6:30 PM, sha.ji...@oracle.com
  wrote:

Hi,
JDK-8164639 removed NSS libs from repo, so 
sun/security/tools/keytool/autotest.sh has to download NSS libs 
from artifactory on macosx.

This patch also refactors this shell test to a Java test.

Webrev:
http://cr.openjdk.java.net/~jjiang/8209546/webrev.00/

Issue:
https://bugs.openjdk.java.net/browse/JDK-8209546


Best regards,
John Jiang

Re: RFR[12] JDK-8209546: Make sun/security/tools/keytool/autotest.sh to support macosx

[sha . jiang]

On 2018/9/27 11:20, Weijun Wang wrote:

On Sep 27, 2018, at 11:18 AM, sha.ji...@oracle.com wrote:

On 2018/9/27 10:34, Weijun Wang wrote:

Hi John

1. Please add @bug to all tests.

Which issue should be linked? JDK-8209546?
I suppose @bug should indicate a product issue here.
At least, JDK-8209546 looks have no much association with this test.

I meant JDK-8209546. My understanding is that even if it's a noreg-self change, 
the bug id should also be included.
In practice, we may not add a direct associated test bug id to the test 
itself;

otherwise, many tests would contain a lot of bug ids.
In fact, we can find the test bug id in the changeset message.

In addition, the below statement is copied from jtreg FAQ item 4.28 [1]
"If you're updating a test because it was affected by a bug fix,
but the test is not otherwise a regression test for the bug fix,
then you should probably not update the @bug entry."

With my understanding, in my case, this @bug should not be updated.

[1] 
http://openjdk.java.net/jtreg/faq.html#when-should-i-update-the-bug-entry-in-a-test-description


Best regards,
Best regards,

Re: RFR[12] JDK-8209546: Make sun/security/tools/keytool/autotest.sh to support macosx

[sha . jiang]

On 2018/9/27 10:34, Weijun Wang wrote:

Hi John

1. Please add @bug to all tests.

Which issue should be linked? JDK-8209546?
I suppose @bug should indicate a product issue here.
At least, JDK-8209546 looks have no much association with this test.



2. Are getLibPath() and findLib() in AutoTest.java really necessary? It looks 
like PKCS11Test::getNSSLibDir is doing something similar.

I'll modify PKCS11Test.java a bit to help this point.



3. Looks like Standard.java is not necessary now. You can just make 
KeyToolTest.java a @test and add a @run line there, something like

 @run main/othervm/timeout=600 -Dfile KeyToolTest

Will address.



4. Maybe we can rename AutoTest.java to NssTest.java. The old name says nothing.

Will address.

Best regards,
John Jiang


Thanks
Max


On Sep 27, 2018, at 9:25 AM, sha.ji...@oracle.com wrote:

Hi Max,
Please review the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8209546/webrev.01/
All your comments are addressed, though this test is moved to problem list for 
windows due to JDK-8204203.

Best regards,
John Jiang
On 2018/9/25 22:30, Weijun Wang wrote:

Some questions:

1. Do we still need the OS check on lines 47-49? As long as getLibPath() can 
return something, does it mean the test should just run? Especially, does the 
test run on Windows?

2. Is launching a separate process necessary? Can we just call 
KeyToolTest::main after setting system properties and copying the files.

3. Is it possible to include standard.sh?

Thanks
Max



On Sep 25, 2018, at 6:30 PM, sha.ji...@oracle.com
  wrote:

Hi,
JDK-8164639 removed NSS libs from repo, so 
sun/security/tools/keytool/autotest.sh has to download NSS libs from 
artifactory on macosx.
This patch also refactors this shell test to a Java test.

Webrev:
http://cr.openjdk.java.net/~jjiang/8209546/webrev.00/

Issue:
https://bugs.openjdk.java.net/browse/JDK-8209546


Best regards,
John Jiang

Re: RFR[12] JDK-8209546: Make sun/security/tools/keytool/autotest.sh to support macosx

[sha . jiang]

2. Is launching a separate process necessary? Can we just call 
KeyToolTest::main after setting system properties and copying the files.

I did think about this point.
It looks this test could be run by manual if someone want to set those 
system properties.

I supposed that's why KeyToolTest.java is not a jtreg test.

I misunderstood this point.
Will apply that.

Best regards,
John Jiang

Re: RFR[12] JDK-8209546: Make sun/security/tools/keytool/autotest.sh to support macosx

[sha . jiang]

Hi Max,

On 2018/9/25 22:30, Weijun Wang wrote:

Some questions:

1. Do we still need the OS check on lines 47-49? As long as getLibPath() can 
return something, does it mean the test should just run? Especially, does the 
test run on Windows?
The original test ignores Windows, and says "This test is only executed 
on several platforms".

So, I didn't change this test logic.
But I'll have a try without that OS checking.


2. Is launching a separate process necessary? Can we just call 
KeyToolTest::main after setting system properties and copying the files.

I did think about this point.
It looks this test could be run by manual if someone want to set those 
system properties.

I supposed that's why KeyToolTest.java is not a jtreg test.



3. Is it possible to include standard.sh?

No problem. This test looks quite similar to autotest.sh.

Best regards,
John Jiang


Thanks
Max


On Sep 25, 2018, at 6:30 PM, sha.ji...@oracle.com wrote:

Hi,
JDK-8164639 removed NSS libs from repo, so 
sun/security/tools/keytool/autotest.sh has to download NSS libs from 
artifactory on macosx.
This patch also refactors this shell test to a Java test.

Webrev: http://cr.openjdk.java.net/~jjiang/8209546/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8209546

Best regards,
John Jiang

RFR[12] JDK-8209546: Make sun/security/tools/keytool/autotest.sh to support macosx

[sha . jiang]

Hi,
JDK-8164639 removed NSS libs from repo, so 
sun/security/tools/keytool/autotest.sh has to download NSS libs from 
artifactory on macosx.

This patch also refactors this shell test to a Java test.

Webrev: http://cr.openjdk.java.net/~jjiang/8209546/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8209546

Best regards,
John Jiang

Re: RFR[12] JDK-8209362: sun/security/ssl/SSLSocketImpl/ReuseAddr.java failed due to "BindException: Address already in use (Bind failed)"

[sha . jiang]

On 2018/8/31 09:22, Xuelei Fan wrote:

On 8/30/2018 6:16 PM, sha.ji...@oracle.com wrote:

Hi Xuelei,
It still be possible that two test runs fail.


Yes.  I was wondering the possibility of the failure may go down.

I searched this test in JBS, and didn't find many failures on this test.
So, it may be unnecessary to concern it too much.

Run all javax/net/ssl and sun/security/ssl tests with this fix via 
Mach5, the result is green.


Best regards,
John Jiang


Thanks,
Xuelei


Best regards,
John Jiang

On 2018/8/30 22:02, Xuelei Fan wrote:

Maybe, run the test twice and pass if one passed?

Xuelei

On 8/29/2018 7:34 PM, sha.ji...@oracle.com wrote:

Hi,
In test sun/security/ssl/SSLSocketImpl/ReuseAddr.java,
the second server tries to reuse the first server's port after it 
stops.
But the port may already be occupied by another test before this 
rebinding.


Although I'm not sure a way to resolve this problem thoroughly,
it's a chance to refactor this test with SSLSocketTemplate.java.

Webrev: http://cr.openjdk.java.net/~jjiang/8209362/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8209362

Best regards,
John Jiang

Re: RFR[12] JDK-8209362: sun/security/ssl/SSLSocketImpl/ReuseAddr.java failed due to "BindException: Address already in use (Bind failed)"

[sha . jiang]

Hi Xuelei,
It still be possible that two test runs fail.

Best regards,
John Jiang

On 2018/8/30 22:02, Xuelei Fan wrote:

Maybe, run the test twice and pass if one passed?

Xuelei

On 8/29/2018 7:34 PM, sha.ji...@oracle.com wrote:

Hi,
In test sun/security/ssl/SSLSocketImpl/ReuseAddr.java,
the second server tries to reuse the first server's port after it stops.
But the port may already be occupied by another test before this 
rebinding.


Although I'm not sure a way to resolve this problem thoroughly,
it's a chance to refactor this test with SSLSocketTemplate.java.

Webrev: http://cr.openjdk.java.net/~jjiang/8209362/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8209362

Best regards,
John Jiang

RFR[11] JDK-8209537: Two security tests failed after JDK-8164639 due to dependency was missed

[sha . jiang]

Hi,
JDK-8164639 removed NSS libs from repo and added @Artifact in /test/lib 
to PKCS11Test.java as dependency.
Some tests outside of sun/security/pkcs11 are also affected. So they 
have to be modified accordingly.


Webrev: http://cr.openjdk.java.net/~jjiang/8209537/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8209537

Have run all tier2 tests with this fix. The result is green.

Best regards,
John Jiang

Re: RFR JDK-8164639: Configure PKCS11 tests to use user-supplied NSS libraries

[sha . jiang]

On 2018/8/15 15:14, Weijun Wang wrote:

I see.

One more question: Since the osMap content is not changed, is there a special 
reason you move the code from a static block into a static method?
If preferable NSS lib paths have been provided, it looks no need to 
create that map.


Best regards,
John Jiang



Thanks
Max


On Aug 15, 2018, at 3:07 PM, sha.ji...@oracle.com wrote:

On 2018/8/15 14:54, Weijun Wang wrote:

I notice one behavior change in PKCS11Test.java.

  693 private static String[] getNssLibPaths(String osId) {
  694 String[] preferablePaths = getPreferableNssLibPaths(osId);
  695 if (preferablePaths.length != 0) {
  696 return preferablePaths;
  697 } else {
  698 return getOsMap().get(osId);
  699 }
  700 }

If getPreferableNssLibPaths(osId) returns a non-empty array, it seems you will 
not look at osMap at all. This means if the system property is set but it does 
not contain NSS libraries you will not look into osMap as a fallback. Is this 
intended?

Yes.

I assume user have specified the full libs. If the specified libs are not 
enough, that may be a user's mistake.
If it allows the tests work with custom libs and system libs, that may be 
confused.

Best regards,
John Jiang


Thanks
Max



On Aug 15, 2018, at 2:45 PM, sha.ji...@oracle.com wrote:

Hi Max,
Thanks for your comments very much!

Please review this version: 
http://cr.openjdk.java.net/~jjiang/8164639/werbrev.03/
All of your comments are addressed.

Assume external developers have no JIB jar, the artifact resolving fails 
quickly. The tests will be skipped for this case.

Best regards,
John Jiang

On 2018/8/15 11:23, Weijun Wang wrote:

Two comments on PKCS11Test.java:

First,

  865 private static String fetchNssLib(Class clazz) {
  866 try {
  867 String path = 
ArtifactResolver.resolve(clazz).entrySet().stream()
  868 .findAny().get().getValue() + File.separator + 
"nsslib"
  869 + File.separator;
  870 return path;
  871 } catch (ArtifactResolverException e) {
  872 throw new RuntimeException("Fetch artifact failed: " + clazz
  873 + "\nPlease make sure the artifact is available "
  874 + "and JIB jar is in the classpath", e);
  875 }
  876 }

If an external developer is running this test and cannot download the artifact 
(either because there's no JIB jar or cannot access the server), will this test 
fail?

If yes, this is not a good idea. I'm OK with the test succeed with a warning 
(even if no one notice it) but failure is bad.

Second,

  666 osMap.put("SunOS-sparc-32",
  667 getNssLibPaths(new String[] { "/usr/lib/mps/" }));
  ...

We needn't call getNssLibPaths() for every platform, especially, the 
fetchNssLib() action should only run on the current platform. We can simply add 
something after these existing lines

  309 String osid = osName + "-"
  310 + props.getProperty("os.arch") + "-" + 
props.getProperty("sun.arch.data.model");
  311 String[] nssLibDirs = osMap.get(osid);

For example,

 String[] nssLibDirs = expandDirs(osMap.get(osid));

and let expandDirs() to do the test.nss.lib.paths search and artifact 
downloading.

REAME is good, although I would like to see more blank lines.

Thanks
Max


On Aug 15, 2018, at 10:13 AM, sha.ji...@oracle.com wrote:

Thanks for the comments!
Please take a look the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8164639/webrev.02/
Only README was adjusted.

Best regards,
John Jiang
On 2018/8/14 23:48, Rajan Halade wrote:

Few minor comments on README:

- Please leave an empty line after each numbered section
- I would suggest to update #2 to have general instruction on use of 
artifactory. Something like

2. Pre-built NSS libraries from artifactory server
If the value of system property test.nss.lib.paths is null then tests will 
try
to download pre-built NSS libraries from artifactory server.
Currently, test only looks for libraries for Windows and MacOSX on 
artifactory.
Please note that, JIB jar MUST be present in classpath when downloading the 
libraries.

Other changes look good to me.

Thanks,
Rajan

On 8/14/18 4:40 AM, sha.ji...@oracle.com wrote:

Hi Max,
Please review the new webrev: 
http://cr.openjdk.java.net/~jjiang/8164639/webrev.01/

The new system property has been renamed to test.nss.lib.paths, and it supports 
multiple paths.
Currently, it cannot download the artifacts outside Oracle network. This 
affects the test executions on Windows and MacOSX.
I added a block to README for clarifying something on getting NSS libraries.

Best regards,
John Jiang
On 2018/8/13 16:48, Weijun Wang wrote:

Sorry, more questions:



On Aug 13, 2018, at 3:36 PM, sha.ji...@oracle.com
  wrote:



Is there an artifact server available on the open internet?


It's transparent to me. @Artifact 

Re: RFR JDK-8164639: Configure PKCS11 tests to use user-supplied NSS libraries

[sha . jiang]

On 2018/8/15 14:54, Weijun Wang wrote:

I notice one behavior change in PKCS11Test.java.

  693 private static String[] getNssLibPaths(String osId) {
  694 String[] preferablePaths = getPreferableNssLibPaths(osId);
  695 if (preferablePaths.length != 0) {
  696 return preferablePaths;
  697 } else {
  698 return getOsMap().get(osId);
  699 }
  700 }

If getPreferableNssLibPaths(osId) returns a non-empty array, it seems you will 
not look at osMap at all. This means if the system property is set but it does 
not contain NSS libraries you will not look into osMap as a fallback. Is this 
intended?

Yes.

I assume user have specified the full libs. If the specified libs are 
not enough, that may be a user's mistake.
If it allows the tests work with custom libs and system libs, that may 
be confused.


Best regards,
John Jiang



Thanks
Max



On Aug 15, 2018, at 2:45 PM, sha.ji...@oracle.com wrote:

Hi Max,
Thanks for your comments very much!

Please review this version: 
http://cr.openjdk.java.net/~jjiang/8164639/werbrev.03/
All of your comments are addressed.

Assume external developers have no JIB jar, the artifact resolving fails 
quickly. The tests will be skipped for this case.

Best regards,
John Jiang

On 2018/8/15 11:23, Weijun Wang wrote:

Two comments on PKCS11Test.java:

First,

  865 private static String fetchNssLib(Class clazz) {
  866 try {
  867 String path = 
ArtifactResolver.resolve(clazz).entrySet().stream()
  868 .findAny().get().getValue() + File.separator + 
"nsslib"
  869 + File.separator;
  870 return path;
  871 } catch (ArtifactResolverException e) {
  872 throw new RuntimeException("Fetch artifact failed: " + clazz
  873 + "\nPlease make sure the artifact is available "
  874 + "and JIB jar is in the classpath", e);
  875 }
  876 }

If an external developer is running this test and cannot download the artifact 
(either because there's no JIB jar or cannot access the server), will this test 
fail?

If yes, this is not a good idea. I'm OK with the test succeed with a warning 
(even if no one notice it) but failure is bad.

Second,

  666 osMap.put("SunOS-sparc-32",
  667 getNssLibPaths(new String[] { "/usr/lib/mps/" }));
  ...

We needn't call getNssLibPaths() for every platform, especially, the 
fetchNssLib() action should only run on the current platform. We can simply add 
something after these existing lines

  309 String osid = osName + "-"
  310 + props.getProperty("os.arch") + "-" + 
props.getProperty("sun.arch.data.model");
  311 String[] nssLibDirs = osMap.get(osid);

For example,

 String[] nssLibDirs = expandDirs(osMap.get(osid));

and let expandDirs() to do the test.nss.lib.paths search and artifact 
downloading.

REAME is good, although I would like to see more blank lines.

Thanks
Max


On Aug 15, 2018, at 10:13 AM, sha.ji...@oracle.com wrote:

Thanks for the comments!
Please take a look the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8164639/webrev.02/
Only README was adjusted.

Best regards,
John Jiang
On 2018/8/14 23:48, Rajan Halade wrote:

Few minor comments on README:

- Please leave an empty line after each numbered section
- I would suggest to update #2 to have general instruction on use of 
artifactory. Something like

2. Pre-built NSS libraries from artifactory server
If the value of system property test.nss.lib.paths is null then tests will 
try
to download pre-built NSS libraries from artifactory server.
Currently, test only looks for libraries for Windows and MacOSX on 
artifactory.
Please note that, JIB jar MUST be present in classpath when downloading the 
libraries.

Other changes look good to me.

Thanks,
Rajan

On 8/14/18 4:40 AM, sha.ji...@oracle.com wrote:

Hi Max,
Please review the new webrev: 
http://cr.openjdk.java.net/~jjiang/8164639/webrev.01/

The new system property has been renamed to test.nss.lib.paths, and it supports 
multiple paths.
Currently, it cannot download the artifacts outside Oracle network. This 
affects the test executions on Windows and MacOSX.
I added a block to README for clarifying something on getting NSS libraries.

Best regards,
John Jiang
On 2018/8/13 16:48, Weijun Wang wrote:

Sorry, more questions:



On Aug 13, 2018, at 3:36 PM, sha.ji...@oracle.com
  wrote:



Is there an artifact server available on the open internet?


It's transparent to me. @Artifact tool delegates the downloading.


Have you tried running the test outside Oracle?

Have you tried submitting the change to Mach5 as a non-Oracle developer? (i.e. 
using submit-repo)

While I am glad to see these files removed from the repo, I hope people still 
have a chance to run the tests.

Thanks
Max

Re: RFR JDK-8164639: Configure PKCS11 tests to use user-supplied NSS libraries

[sha . jiang]

Hi Max,
Thanks for your comments very much!

Please review this version: 
http://cr.openjdk.java.net/~jjiang/8164639/werbrev.03/

All of your comments are addressed.

Assume external developers have no JIB jar, the artifact resolving fails 
quickly. The tests will be skipped for this case.


Best regards,
John Jiang

On 2018/8/15 11:23, Weijun Wang wrote:

Two comments on PKCS11Test.java:

First,

  865 private static String fetchNssLib(Class clazz) {
  866 try {
  867 String path = 
ArtifactResolver.resolve(clazz).entrySet().stream()
  868 .findAny().get().getValue() + File.separator + 
"nsslib"
  869 + File.separator;
  870 return path;
  871 } catch (ArtifactResolverException e) {
  872 throw new RuntimeException("Fetch artifact failed: " + clazz
  873 + "\nPlease make sure the artifact is available "
  874 + "and JIB jar is in the classpath", e);
  875 }
  876 }

If an external developer is running this test and cannot download the artifact 
(either because there's no JIB jar or cannot access the server), will this test 
fail?

If yes, this is not a good idea. I'm OK with the test succeed with a warning 
(even if no one notice it) but failure is bad.

Second,

  666 osMap.put("SunOS-sparc-32",
  667 getNssLibPaths(new String[] { "/usr/lib/mps/" }));
  ...

We needn't call getNssLibPaths() for every platform, especially, the 
fetchNssLib() action should only run on the current platform. We can simply add 
something after these existing lines

  309 String osid = osName + "-"
  310 + props.getProperty("os.arch") + "-" + 
props.getProperty("sun.arch.data.model");
  311 String[] nssLibDirs = osMap.get(osid);

For example,

 String[] nssLibDirs = expandDirs(osMap.get(osid));

and let expandDirs() to do the test.nss.lib.paths search and artifact 
downloading.

REAME is good, although I would like to see more blank lines.

Thanks
Max


On Aug 15, 2018, at 10:13 AM, sha.ji...@oracle.com wrote:

Thanks for the comments!
Please take a look the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8164639/webrev.02/
Only README was adjusted.

Best regards,
John Jiang
On 2018/8/14 23:48, Rajan Halade wrote:

Few minor comments on README:

- Please leave an empty line after each numbered section
- I would suggest to update #2 to have general instruction on use of 
artifactory. Something like

2. Pre-built NSS libraries from artifactory server
If the value of system property test.nss.lib.paths is null then tests will 
try
to download pre-built NSS libraries from artifactory server.
Currently, test only looks for libraries for Windows and MacOSX on 
artifactory.
Please note that, JIB jar MUST be present in classpath when downloading the 
libraries.

Other changes look good to me.

Thanks,
Rajan

On 8/14/18 4:40 AM, sha.ji...@oracle.com wrote:

Hi Max,
Please review the new webrev: 
http://cr.openjdk.java.net/~jjiang/8164639/webrev.01/

The new system property has been renamed to test.nss.lib.paths, and it supports 
multiple paths.
Currently, it cannot download the artifacts outside Oracle network. This 
affects the test executions on Windows and MacOSX.
I added a block to README for clarifying something on getting NSS libraries.

Best regards,
John Jiang
On 2018/8/13 16:48, Weijun Wang wrote:

Sorry, more questions:



On Aug 13, 2018, at 3:36 PM, sha.ji...@oracle.com
  wrote:



Is there an artifact server available on the open internet?


It's transparent to me. @Artifact tool delegates the downloading.


Have you tried running the test outside Oracle?

Have you tried submitting the change to Mach5 as a non-Oracle developer? (i.e. 
using submit-repo)

While I am glad to see these files removed from the repo, I hope people still 
have a chance to run the tests.

Thanks
Max

Re: RFR JDK-8164639: Configure PKCS11 tests to use user-supplied NSS libraries

[sha . jiang]

Thanks for the comments!
Please take a look the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8164639/webrev.02/

Only README was adjusted.

Best regards,
John Jiang

On 2018/8/14 23:48, Rajan Halade wrote:

Few minor comments on README:

- Please leave an empty line after each numbered section
- I would suggest to update #2 to have general instruction on use of 
artifactory. Something like


2. Pre-built NSS libraries from artifactory server
If the value of system property test.nss.lib.paths is null then tests will 
try
to download pre-built NSS libraries from artifactory server.
Currently, test only looks for libraries for Windows and MacOSX on 
artifactory.
Please note that, JIB jar MUST be present in classpath when downloading the 
libraries.
Other changes look good to me.

Thanks,
Rajan

On 8/14/18 4:40 AM, sha.ji...@oracle.com wrote:


Hi Max,
Please review the new webrev: 
http://cr.openjdk.java.net/~jjiang/8164639/webrev.01/


The new system property has been renamed to test.nss.lib.paths, and 
it supports multiple paths.
Currently, it cannot download the artifacts outside Oracle network. 
This affects the test executions on Windows and MacOSX.
I added a block to README for clarifying something on getting NSS 
libraries.


Best regards,
John Jiang

On 2018/8/13 16:48, Weijun Wang wrote:

Sorry, more questions:


On Aug 13, 2018, at 3:36 PM,sha.ji...@oracle.com  wrote:


Is there an artifact server available on the open internet?

It's transparent to me. @Artifact tool delegates the downloading.

Have you tried running the test outside Oracle?

Have you tried submitting the change to Mach5 as a non-Oracle developer? (i.e. 
using submit-repo)

While I am glad to see these files removed from the repo, I hope people still 
have a chance to run the tests.

Thanks
Max

Re: RFR JDK-8164639: Configure PKCS11 tests to use user-supplied NSS libraries

[sha . jiang]

Hi Max,
Please review the new webrev: 
http://cr.openjdk.java.net/~jjiang/8164639/webrev.01/


The new system property has been renamed to test.nss.lib.paths, and it 
supports multiple paths.
Currently, it cannot download the artifacts outside Oracle network. This 
affects the test executions on Windows and MacOSX.

I added a block to README for clarifying something on getting NSS libraries.

Best regards,
John Jiang

On 2018/8/13 16:48, Weijun Wang wrote:

Sorry, more questions:


On Aug 13, 2018, at 3:36 PM,sha.ji...@oracle.com  wrote:


Is there an artifact server available on the open internet?

It's transparent to me. @Artifact tool delegates the downloading.

Have you tried running the test outside Oracle?

Have you tried submitting the change to Mach5 as a non-Oracle developer? (i.e. 
using submit-repo)

While I am glad to see these files removed from the repo, I hope people still 
have a chance to run the tests.

Thanks
Max

Re: RFR JDK-8164639: Configure PKCS11 tests to use user-supplied NSS libraries

[sha . jiang]

Hi Max,

On 2018/8/13 13:58, Weijun Wang wrote:



On Aug 13, 2018, at 1:46 PM, sha.ji...@oracle.com wrote:

On 2018/8/13 11:25, Weijun Wang wrote:

Can test.nss.lib.path contain multiple paths? For example, some systems might 
have libsoftkn3.so and libnss3.so in different directories [1] and depending on 
whether secmod is used the test might load one or the other.

I assume the custom libs are in a single directory.
This property is used for manual test run. When run different tests, users can 
specify different target lib path.

But as [1] shows they can be in different directories. Therefore either you 
have to move them into a single directory or you will not be able to run all 
PKCS11 tests with a single jtreg command.
For some platforms, including linux-x64 and linux-x86, the test may 
search multiple directories for a specific NSS lib, namely softokn3 or nss3.

Please see method getOsMap() in PKCS11Test.java.

But I assume this property is used when a user-built NSS libs, but not 
system NSS libs, are needed.

The user-built NSS libs could be in a single directory.


I have the same question on the downloaded file from the artifact server. It seems for 
each platform it is a zip file. Will it extract all libraries into the same 
"nsslib" directory?

In my case, only one zip file is downloaded in a specific platform.

And each archive is a separated artifact, which has different artifactId.
The local path contains the artifactId, so different artifacts cannot be in the 
same directory.


Also, this is the 1st time I hear about @Artifact in an openjdk test and know 
nothing about it. Is there a detailed description on this feature somewhere?

I also don't get the details.

Is there an artifact server available on the open internet?

It's transparent to me. @Artifact tool delegates the downloading.


As for this test, if customNssLib is the first element in nssLibDirs and not 
set by a user, does this mean the test will always download libraries from an 
artifact server?

If customNssLib is null, the test downloads the platform-specific artifact from 
Artifactory;
otherwise, the test just uses the specified local NSS libs and no downloading 
is triggered.

But I think the most common case is no user-customized nss lib path.

Yes. So, generally the value of property test.nss.lib.path is null.
PKCS11 tests use system NSS libs on Linux and Solaris, but download 
windows and macosx NSS libs from Artifactory (previously, these libs are 
in repo).

My fix doesn't change this logic.


Would it spend too much time on downloading?

In this case, with my testing, it didn't take much time.


Is there a cache mechanism so that after the 1st PKCS11 test downloads the 
libraries the other tests can reuse them?

With my testing, if the local path for a specific artifact exists, the same 
artifact should not be re-downloaded.

What is the local path? I assume it is out of the scratch directory.

In my laptop, the path is:
/private/var/tmp/jib-sha.jiang/install/jpg/tests/jdk/nsslib/nsslib-macosx_x64/3.35/nsslib-macosx_x64-3.35.zip/nsslib


If multiple tests are running in agentvm mode, is there a risk they share the 
same local path and see incomplete download?

They should share the same local path.
If no synchronization on actions of building local directory structure, 
downloading and/or unpacking, some issue may raise.

I will consult infra team on this issue.

Best regards,
John Jiang


Thanks
Max


Are they cleaned up at some time?

The libs are NOT removed after my manual test execution.
But I don't know the details about this point in CI system.

Best regards,
John Jiang


Thanks
Max

[1] https://packages.ubuntu.com/xenial-updates/arm64/libnss3/filelist


On Aug 13, 2018, at 10:44 AM, sha.ji...@oracle.com wrote:

Hi,
This patch provides a system property, exactly test.nss.lib.path, for 
specifying the absolute path to the custom NSS lib.
And it also removes the NSS 3.16 binary libs on windows and macosx from repo.
On these two platforms, PKCS11 tests will download new built NSS 3.35 libs from 
Artifactory.

Webrev: http://cr.openjdk.java.net/~jjiang/8164639/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8164639

Best regards,
John Jiang

Re: RFR JDK-8164639: Configure PKCS11 tests to use user-supplied NSS libraries

[sha . jiang]

On 2018/8/13 11:25, Weijun Wang wrote:

Can test.nss.lib.path contain multiple paths? For example, some systems might 
have libsoftkn3.so and libnss3.so in different directories [1] and depending on 
whether secmod is used the test might load one or the other.

I assume the custom libs are in a single directory.
This property is used for manual test run. When run different tests, 
users can specify different target lib path.



I have the same question on the downloaded file from the artifact server. It seems for 
each platform it is a zip file. Will it extract all libraries into the same 
"nsslib" directory?

In my case, only one zip file is downloaded in a specific platform.

And each archive is a separated artifact, which has different artifactId.
The local path contains the artifactId, so different artifacts cannot be 
in the same directory.




Also, this is the 1st time I hear about @Artifact in an openjdk test and know 
nothing about it. Is there a detailed description on this feature somewhere?

I also don't get the details.



As for this test, if customNssLib is the first element in nssLibDirs and not 
set by a user, does this mean the test will always download libraries from an 
artifact server?
If customNssLib is null, the test downloads the platform-specific 
artifact from Artifactory;
otherwise, the test just uses the specified local NSS libs and no 
downloading is triggered.



Would it spend too much time on downloading?

In this case, with my testing, it didn't take much time.


Is there a cache mechanism so that after the 1st PKCS11 test downloads the 
libraries the other tests can reuse them?
With my testing, if the local path for a specific artifact exists, the 
same artifact should not be re-downloaded.



Are they cleaned up at some time?

The libs are NOT removed after my manual test execution.
But I don't know the details about this point in CI system.

Best regards,
John Jiang



Thanks
Max

[1] https://packages.ubuntu.com/xenial-updates/arm64/libnss3/filelist


On Aug 13, 2018, at 10:44 AM, sha.ji...@oracle.com wrote:

Hi,
This patch provides a system property, exactly test.nss.lib.path, for 
specifying the absolute path to the custom NSS lib.
And it also removes the NSS 3.16 binary libs on windows and macosx from repo.
On these two platforms, PKCS11 tests will download new built NSS 3.35 libs from 
Artifactory.

Webrev: http://cr.openjdk.java.net/~jjiang/8164639/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8164639

Best regards,
John Jiang

RFR JDK-8164639: Configure PKCS11 tests to use user-supplied NSS libraries

[sha . jiang]

Hi,
This patch provides a system property, exactly test.nss.lib.path, for 
specifying the absolute path to the custom NSS lib.
And it also removes the NSS 3.16 binary libs on windows and macosx from 
repo.
On these two platforms, PKCS11 tests will download new built NSS 3.35 
libs from Artifactory.


Webrev: http://cr.openjdk.java.net/~jjiang/8164639/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8164639

Best regards,
John Jiang

Re: [11] RFR: 8208496: New Test to verify concurrent behavior for TLS.

[sha . jiang]

Hi Siba,
Would it be better to check how many connections the server accepts?
In your case, the server must accept 50 (no more no less) connections; 
otherwise, some problem may raise.


And I suppose, when the server thread is interrupted, the server socket 
may not be closed.
The server should exit immediately and gracefully when it has accepted 
all the connections.
If the server can be closed gracefully, it may be no need to set the 
server thread as daemon.


Some minors:
-- 28 import java.net.SocketException;
This import statement looks unused.

-- 131 sslSocket.setNeedClientAuth(false);
Now that client auth is not requested by default, so it may be 
unnecessary to set false value explicitly.


Best regards,
John Jiang

On 2018/8/2 18:41, Sibabrata Sahoo wrote:


Hi Xuelei,

Please review the patch for,

JBS: https://bugs.openjdk.java.net/browse/JDK-8208496

Webrev: http://cr.openjdk.java.net/~ssahoo/8208496/webrev.00/ 



This is a new Test which test concurrent behavior of TLS. It uses 50 
client thread to access a single server port concurrently and repeat 
this process for each protocol supported.


Thanks,

Siba

Re: RFR[11] JDK-8206258: [Test Error] sun/security/pkcs11 tests fail if NSS libs not found

[sha . jiang]

Hi Valerie,
Thanks for your review!
Please take a look at this new webrev: 
http://cr.openjdk.java.net/~jjiang/8206258/webrev.02


On 2018/7/24 06:18, Valerie Peng wrote:

Hi John,

Changes look fine.

I just have one nit, perhaps add more information reporting when 
skipping tests, e.g.


PKCS11Test: line 163

Different tests may have different reasons for skipping testing.
So, it would be better to output the info in PKCS11Test's child classes.
In fact, the tests overriding the method PKCS11Test::skipTest already 
report the reasons respectively.



TestNssDbSqlite.java: line 68.

Add the below line
120 System.out.println("Cannot init security module 
database, skipping");


Best regards,
John Jiang


Thanks,
Valerie

On 7/9/2018 12:38 AM, sha.ji...@oracle.com wrote:

Hi Thomas,
Thanks for your testing.

I'm not sure that's a reasonable case.
From my view, PKCS11Test.java simply checks if the NSS library 
directory exists.

But it looks unnecessary to check every library file.
In fact, if removing libnss3 or libsoftokn3's dependencies, like 
libnssutil3, the test also fails.


However, I still re-checked my previous solution, and made a new 
webrev [1].

The constant badNSSVersion in PKCS11Test.java may not be fine.
The static field nss_library in PKCS11Test.java can be softokn3 or 
nss3 for different tests.
badNSSVersion should be checked after the target nss library is 
determined.
And this checking should happen before the real testing, especially 
before security manager is enabled.
So, a new extension method, exactly PKCS11Test::skipTest, was 
introduced, and the affected tests were modified accordingly.


[1] http://cr.openjdk.java.net/~jjiang/8206258/webrev.01/

Best regards,
John Jiang

On 2018/7/4 14:15, Thomas Stüfe wrote:

Hi,

Unfortunately this is not enough.

Running tests with your patch and NSS libs disabled (I renamed
libsoftokn3.so) yields the following errors:

sun/security/pkcs11/Secmod/AddPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/AddTrustedCert.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/Crypto.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/GetPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/JksSetPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/LoadKeystore.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/TestNssDbSqlite.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/TrustAnchors.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS


Excerpt from TestNssDbSqlite.jtr:

--messages:(3/98)--
command: build TestNssDbSqlite
reason: Named class compiled on demand
elapsed time (seconds): 0.0
result: Passed. All files up to date

#section:main
--messages:(5/721)--
command: main TestNssDbSqlite
reason: User specified action: run main/othervm/timeout=120 
TestNssDbSqlite

Mode: othervm [/othervm specified]
Additional options from @modules: --add-modules
java.base,jdk.crypto.cryptoki --add-exports
java.base/sun.security.rsa=ALL-UNNAMED --add-exports
java.base/sun.security.provider=ALL-UNNAMED --add-exports
java.base/sun.security.jca=ALL-UNNAMED --add-exports
java.base/sun.security.tools.keytool=ALL-UNNAMED --add-exports
java.base/sun.security.x509=ALL-UNNAMED --add-exports
java.base/com.sun.crypto.provider=ALL-UNNAMED --add-exports
jdk.crypto.cryptoki/sun.security.pkcs11=ALL-UNNAMED --add-opens
jdk.crypto.cryptoki/sun.security.pkcs11=ALL-UNNAMED
elapsed time (seconds): 0.445
--configuration:(11/604)--
Boot Layer
   add modules: java.base jdk.crypto.cryptoki
   add exports: java.base/com.sun.crypto.provider ALL-UNNAMED
    java.base/sun.security.jca ALL-UNNAMED
    java.base/sun.security.provider ALL-UNNAMED
    java.base/sun.security.rsa ALL-UNNAMED
    java.base/sun.security.tools.keytool ALL-UNNAMED
    java.base/sun.security.x509 ALL-UNNAMED
   

Re: RFR[11] JDK-8206258: [Test Error] sun/security/pkcs11 tests fail if NSS libs not found

[sha . jiang]

Ping...

John

On 2018/7/13 09:15, sha.ji...@oracle.com wrote:

Could the new patch be reviewed?
http://cr.openjdk.java.net/~jjiang/8206258/webrev.01/

Thanks!
John

On 2018/7/9 15:38, sha.ji...@oracle.com wrote:

Hi Thomas,
Thanks for your testing.

I'm not sure that's a reasonable case.
From my view, PKCS11Test.java simply checks if the NSS library 
directory exists.

But it looks unnecessary to check every library file.
In fact, if removing libnss3 or libsoftokn3's dependencies, like 
libnssutil3, the test also fails.


However, I still re-checked my previous solution, and made a new 
webrev [1].

The constant badNSSVersion in PKCS11Test.java may not be fine.
The static field nss_library in PKCS11Test.java can be softokn3 or 
nss3 for different tests.
badNSSVersion should be checked after the target nss library is 
determined.
And this checking should happen before the real testing, especially 
before security manager is enabled.
So, a new extension method, exactly PKCS11Test::skipTest, was 
introduced, and the affected tests were modified accordingly.


[1] http://cr.openjdk.java.net/~jjiang/8206258/webrev.01/

Best regards,
John Jiang

On 2018/7/4 14:15, Thomas Stüfe wrote:

Hi,

Unfortunately this is not enough.

Running tests with your patch and NSS libs disabled (I renamed
libsoftokn3.so) yields the following errors:

sun/security/pkcs11/Secmod/AddPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/AddTrustedCert.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/Crypto.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/GetPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/JksSetPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/LoadKeystore.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/TestNssDbSqlite.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/TrustAnchors.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS


Excerpt from TestNssDbSqlite.jtr:

--messages:(3/98)--
command: build TestNssDbSqlite
reason: Named class compiled on demand
elapsed time (seconds): 0.0
result: Passed. All files up to date

#section:main
--messages:(5/721)--
command: main TestNssDbSqlite
reason: User specified action: run main/othervm/timeout=120 
TestNssDbSqlite

Mode: othervm [/othervm specified]
Additional options from @modules: --add-modules
java.base,jdk.crypto.cryptoki --add-exports
java.base/sun.security.rsa=ALL-UNNAMED --add-exports
java.base/sun.security.provider=ALL-UNNAMED --add-exports
java.base/sun.security.jca=ALL-UNNAMED --add-exports
java.base/sun.security.tools.keytool=ALL-UNNAMED --add-exports
java.base/sun.security.x509=ALL-UNNAMED --add-exports
java.base/com.sun.crypto.provider=ALL-UNNAMED --add-exports
jdk.crypto.cryptoki/sun.security.pkcs11=ALL-UNNAMED --add-opens
jdk.crypto.cryptoki/sun.security.pkcs11=ALL-UNNAMED
elapsed time (seconds): 0.445
--configuration:(11/604)--
Boot Layer
   add modules: java.base jdk.crypto.cryptoki
   add exports: java.base/com.sun.crypto.provider ALL-UNNAMED
    java.base/sun.security.jca ALL-UNNAMED
    java.base/sun.security.provider ALL-UNNAMED
    java.base/sun.security.rsa ALL-UNNAMED
    java.base/sun.security.tools.keytool ALL-UNNAMED
    java.base/sun.security.x509 ALL-UNNAMED
    jdk.crypto.cryptoki/sun.security.pkcs11 ALL-UNNAMED
   add opens:   jdk.crypto.cryptoki/sun.security.pkcs11 ALL-UNNAMED

--System.out:(1/64)--
Warning: can't find NSS librarys on this machine, skipping test
--System.err:(25/1633)--
java.security.ProviderException: Could not initialize NSS
at 
jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11.(SunPKCS11.java:218)
at 
jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11$1.run(SunPKCS11.java:113)
at 
jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11$1.run(SunPKCS11.java:110)

at 

RFR[12] JDK-8206443: Update security libs manual test to cope with removal of javac -source/-target 6

[sha . jiang]

Hi,
JDK-8028563 has removed javac support for 6/1.6 source and target, so 
the following tests has to be updated accordingly.

javax/net/ssl/compatibility/Compatibility.java
sun/security/tools/jarsigner/compatibility/Compatibility.java

In addition, this fix also makes 
javax/net/ssl/compatibility/Compatibility.java can work with JDK 12 builds.


Webrev: http://cr.openjdk.java.net/~jjiang/8206443/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8206443

Best regards,
John Jiang

Re: RFR[11] JDK-8206258: [Test Error] sun/security/pkcs11 tests fail if NSS libs not found

[sha . jiang]

Could the new patch be reviewed?
http://cr.openjdk.java.net/~jjiang/8206258/webrev.01/

Thanks!
John

On 2018/7/9 15:38, sha.ji...@oracle.com wrote:

Hi Thomas,
Thanks for your testing.

I'm not sure that's a reasonable case.
From my view, PKCS11Test.java simply checks if the NSS library 
directory exists.

But it looks unnecessary to check every library file.
In fact, if removing libnss3 or libsoftokn3's dependencies, like 
libnssutil3, the test also fails.


However, I still re-checked my previous solution, and made a new 
webrev [1].

The constant badNSSVersion in PKCS11Test.java may not be fine.
The static field nss_library in PKCS11Test.java can be softokn3 or 
nss3 for different tests.
badNSSVersion should be checked after the target nss library is 
determined.
And this checking should happen before the real testing, especially 
before security manager is enabled.
So, a new extension method, exactly PKCS11Test::skipTest, was 
introduced, and the affected tests were modified accordingly.


[1] http://cr.openjdk.java.net/~jjiang/8206258/webrev.01/

Best regards,
John Jiang

On 2018/7/4 14:15, Thomas Stüfe wrote:

Hi,

Unfortunately this is not enough.

Running tests with your patch and NSS libs disabled (I renamed
libsoftokn3.so) yields the following errors:

sun/security/pkcs11/Secmod/AddPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/AddTrustedCert.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/Crypto.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/GetPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/JksSetPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/LoadKeystore.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/TestNssDbSqlite.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/TrustAnchors.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS


Excerpt from TestNssDbSqlite.jtr:

--messages:(3/98)--
command: build TestNssDbSqlite
reason: Named class compiled on demand
elapsed time (seconds): 0.0
result: Passed. All files up to date

#section:main
--messages:(5/721)--
command: main TestNssDbSqlite
reason: User specified action: run main/othervm/timeout=120 
TestNssDbSqlite

Mode: othervm [/othervm specified]
Additional options from @modules: --add-modules
java.base,jdk.crypto.cryptoki --add-exports
java.base/sun.security.rsa=ALL-UNNAMED --add-exports
java.base/sun.security.provider=ALL-UNNAMED --add-exports
java.base/sun.security.jca=ALL-UNNAMED --add-exports
java.base/sun.security.tools.keytool=ALL-UNNAMED --add-exports
java.base/sun.security.x509=ALL-UNNAMED --add-exports
java.base/com.sun.crypto.provider=ALL-UNNAMED --add-exports
jdk.crypto.cryptoki/sun.security.pkcs11=ALL-UNNAMED --add-opens
jdk.crypto.cryptoki/sun.security.pkcs11=ALL-UNNAMED
elapsed time (seconds): 0.445
--configuration:(11/604)--
Boot Layer
   add modules: java.base jdk.crypto.cryptoki
   add exports: java.base/com.sun.crypto.provider ALL-UNNAMED
    java.base/sun.security.jca ALL-UNNAMED
    java.base/sun.security.provider ALL-UNNAMED
    java.base/sun.security.rsa ALL-UNNAMED
    java.base/sun.security.tools.keytool ALL-UNNAMED
    java.base/sun.security.x509 ALL-UNNAMED
    jdk.crypto.cryptoki/sun.security.pkcs11 ALL-UNNAMED
   add opens:   jdk.crypto.cryptoki/sun.security.pkcs11 ALL-UNNAMED

--System.out:(1/64)--
Warning: can't find NSS librarys on this machine, skipping test
--System.err:(25/1633)--
java.security.ProviderException: Could not initialize NSS
at 
jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11.(SunPKCS11.java:218)
at 
jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11$1.run(SunPKCS11.java:113)
at 
jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11$1.run(SunPKCS11.java:110)

at java.base/java.security.AccessController.doPrivileged(Native Method)
at 

Re: RFR[11] JDK-8199645: javax/net/ssl/SSLSession/TestEnabledProtocols.java failed with Connection reset

[sha . jiang]

Hi Xuelei,
Thanks for your review!
The fix just was pushed.

I adjusted the following longer lines:
 129 System.out.println("Client got UNEXPECTED 
SSLHandshakeException:");
 134 System.out.println("Client got expected 
SSLHandshakeException:");


In addition, the parentheses, which encloses the new statement, also was 
removed.

 269 (new TestEnabledProtocols(
 270 serverProtocols,
 271 clientProtocols,
 272 exceptionExpected,
 273 selectedProtocol)).run();

Best regards,
John Jiang

On 2018/7/10 21:33, Xuelei Fan wrote:

Looks fine to me.  Please limit each line in 80 characters.

Thanks,
Xuelei

On 7/10/2018 2:27 AM, sha.ji...@oracle.com wrote:

Hi,
javax/net/ssl/SSLSession/TestEnabledProtocols.java may have some 
problem on sync between server and client.
And it would be better to refactor this test with 
SSLSocketTemplate.java.


Webrev: http://cr.openjdk.java.net/~jjiang/8199645/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8199645

Best regards,
John Jiang

RFR[11] JDK-8199645: javax/net/ssl/SSLSession/TestEnabledProtocols.java failed with Connection reset

[sha . jiang]

Hi,
javax/net/ssl/SSLSession/TestEnabledProtocols.java may have some problem 
on sync between server and client.

And it would be better to refactor this test with SSLSocketTemplate.java.

Webrev: http://cr.openjdk.java.net/~jjiang/8199645/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8199645

Best regards,
John Jiang

Re: RFR[11] JDK-8203007: Address missing block coverage for ChaCha20 and Poly1305 algorithms

[sha . jiang]

Ping...

John

On 2018/6/25 17:50, sha.ji...@oracle.com wrote:

Hi,
This patch introduces three new tests to enhance code coverage for 
ChaCha20 and Poly1305 algorithms.


Issue: https://bugs.openjdk.java.net/browse/JDK-8203007
Webrev: http://cr.openjdk.java.net/~jjiang/8203007/webrev.00/

Best regards,
John Jiang

Re: RFR[11] JDK-8206258: [Test Error] sun/security/pkcs11 tests fail if NSS libs not found

[sha . jiang]

Hi Thomas,
Thanks for your testing.

I'm not sure that's a reasonable case.
From my view, PKCS11Test.java simply checks if the NSS library 
directory exists.

But it looks unnecessary to check every library file.
In fact, if removing libnss3 or libsoftokn3's dependencies, like 
libnssutil3, the test also fails.


However, I still re-checked my previous solution, and made a new webrev [1].
The constant badNSSVersion in PKCS11Test.java may not be fine.
The static field nss_library in PKCS11Test.java can be softokn3 or nss3 
for different tests.

badNSSVersion should be checked after the target nss library is determined.
And this checking should happen before the real testing, especially 
before security manager is enabled.
So, a new extension method, exactly PKCS11Test::skipTest, was 
introduced, and the affected tests were modified accordingly.


[1] http://cr.openjdk.java.net/~jjiang/8206258/webrev.01/

Best regards,
John Jiang

On 2018/7/4 14:15, Thomas Stüfe wrote:

Hi,

Unfortunately this is not enough.

Running tests with your patch and NSS libs disabled (I renamed
libsoftokn3.so) yields the following errors:

sun/security/pkcs11/Secmod/AddPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/AddTrustedCert.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/Crypto.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/GetPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/JksSetPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/LoadKeystore.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/TestNssDbSqlite.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/TrustAnchors.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS


Excerpt from TestNssDbSqlite.jtr:

--messages:(3/98)--
command: build TestNssDbSqlite
reason: Named class compiled on demand
elapsed time (seconds): 0.0
result: Passed. All files up to date

#section:main
--messages:(5/721)--
command: main TestNssDbSqlite
reason: User specified action: run main/othervm/timeout=120 TestNssDbSqlite
Mode: othervm [/othervm specified]
Additional options from @modules: --add-modules
java.base,jdk.crypto.cryptoki --add-exports
java.base/sun.security.rsa=ALL-UNNAMED --add-exports
java.base/sun.security.provider=ALL-UNNAMED --add-exports
java.base/sun.security.jca=ALL-UNNAMED --add-exports
java.base/sun.security.tools.keytool=ALL-UNNAMED --add-exports
java.base/sun.security.x509=ALL-UNNAMED --add-exports
java.base/com.sun.crypto.provider=ALL-UNNAMED --add-exports
jdk.crypto.cryptoki/sun.security.pkcs11=ALL-UNNAMED --add-opens
jdk.crypto.cryptoki/sun.security.pkcs11=ALL-UNNAMED
elapsed time (seconds): 0.445
--configuration:(11/604)--
Boot Layer
   add modules: java.base jdk.crypto.cryptoki
   add exports: java.base/com.sun.crypto.provider   ALL-UNNAMED
java.base/sun.security.jca  ALL-UNNAMED
java.base/sun.security.provider ALL-UNNAMED
java.base/sun.security.rsa  ALL-UNNAMED
java.base/sun.security.tools.keytoolALL-UNNAMED
java.base/sun.security.x509 ALL-UNNAMED
jdk.crypto.cryptoki/sun.security.pkcs11 ALL-UNNAMED
   add opens:   jdk.crypto.cryptoki/sun.security.pkcs11 ALL-UNNAMED

--System.out:(1/64)--
Warning: can't find NSS librarys on this machine, skipping test
--System.err:(25/1633)--
java.security.ProviderException: Could not initialize NSS
at jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11.(SunPKCS11.java:218)
at jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11$1.run(SunPKCS11.java:113)
at jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11$1.run(SunPKCS11.java:110)
at java.base/java.security.AccessController.doPrivileged(Native Method)
at 
jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11.configure(SunPKCS11.java:110)
at