Re: RFR 8180570: Refactor sun/security/mscapi shell tests to plain java tests

[Artem Smotrakov]

Hi Max,

KeytoolChangeAlias.java:
- maybe better to run ks.deleteEntry("13579") in a finally block
- should it delete "246810" as well?

Otherwise looks good to me :)

Artem



2018-03-27 9:51 GMT+02:00 Weijun Wang :

> Ping again.
>
> > On Mar 8, 2018, at 8:37 PM, Weijun Wang  wrote:
> >
> >
> >
> >> On Mar 8, 2018, at 6:13 PM, Weijun Wang  wrote:
> >>
> >> Please take a review at
> >>
> >>  http://cr.openjdk.java.net/~weijun/8180570/webrev.00
> >
> > Updated in place.
> >
> >>
> >> Several notes:
> >>
> >> 1. The original KeyStoreCompatibilityMode.sh does not check the exit
> value of each command. I assume it should.
> >>
> >> 2. I don't really understand what SystemDrive is for in
> RSAEncryptDecrypt.sh. Maybe about running the test from \\remote\share? I
> also cannot find JDK-6449799.
> >>
> >> 3. SecurityTools::getProcessBuilder is modified a little. On my
> Windows machine without cygwin (major reason why I want to work on this
> RFE), setting -Djava.security.egd=file:/dev/./urandom causes an error.
> >>
> >> 4. nonUniqueAliases/NonUniqueAliases.sh is not touched. It has an
> @ignore.
> >
> > This one is also updated. I tried it on a Windows Server 2016 with
> certutil.exe and it succeeds.
> >
> > --Max
> >
> >>
> >> Thanks
> >> Max
> >>
> >
>
>

Re: RFR[10] JDK-8190335: Backout changeset for JDK-8176354 due to JDK-8190333

[Artem Smotrakov]

Looks good to me.

Please make sure that the tests pass after baking out 8176354.

Artem


On 10/30/2017 12:12 PM, sha.ji...@oracle.com wrote:

Hi,
It has to backout the changeset for JDK-8176354, because it changed a 
keystore and caused a lot of DTLS test failures. For more details, 
please see JDK-8190333.


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

Best regards,
John Jiang

Re: RFR 8186884: Test native KDC, Java krb5 lib, and native krb5 lib in one test

[Artem Smotrakov]

Looks good to me.

Artem


On 09/11/2017 11:07 AM, Weijun Wang wrote:

Sorry to update again. Might be because jdk10 is frozen now.

http://cr.openjdk.java.net/~weijun/8186884/webrev.03

But changes are real:

1. kdc.supported.enctypes supported by native KDCs correctly. In fact, I am now 
able to set it to aes256-sha2 (unsupported in Java yet) and test for interop 
between native libs with a native KDC.

2. A KDC::kinit method is introduced which calls native kinit when KDC is 
native. This makes the test above possible. Otherwise, Java will not be able to 
generate a aes256-sha2 ccache file.

Thanks
Max



On Sep 8, 2017, at 8:38 PM, Artem Smotrakov  wrote:

Hi Max,

Looks good to me. Below are a couple of minor comments you may want to address. 
No need a new webrev.

Thanks!

1. Proc.java, better to use braces

http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449

2. If you already updated KDC.java, it may be good to use try-with-resources 
for streams in a couple of places.

Artem


On 09/08/2017 06:19 AM, Weijun Wang wrote:

Small update on http://cr.openjdk.java.net/~weijun/8186884/webrev.02. All files 
belong to a single Proc now have the same prefix so they appear together in 
file list.

Thanks
Max


On Sep 7, 2017, at 8:39 PM, Weijun Wang  wrote:

Updated at

  http://cr.openjdk.java.net/~weijun/8186884/webrev.01/

Now the libraries can be more freely combined, so you can test interop between 
one native library and another one:

   jtreg -Dnative.krb5.libs=j=,n=,m=lib1.so,h=lib2.so BasicProc.java


More comments inline below.


On Sep 7, 2017, at 3:29 PM, Artem Smotrakov  wrote:

Hi Max,

In general, looks fine to me. Below are a couple of comments you might want to 
address.

1. BasicProc.java, it might be better to use named constants for parameters for 
once() method. That would make it easier to understand what each particular 
onse() call does

I am passing in label and library names now.


2. BasicProc.java, could you please add an exception message?

+if (!Arrays.equals(msg, msg2)) {
+throw new Exception();
+}
+break;

Fixed.


3. BasicProc.java, should the test do some cleanup then?

+Files.copy(Paths.get("ccache.base"), Paths.get("ccache." + label));

Nowadays I prefer to let jtreg do the cleanup/retain. In fact, I am able to 
find a KDC.java bug by saving the ccache, where the incoming service ticket is 
invalid and not saved into the ccache.

Thanks
Max


Artem

On 09/07/2017 03:07 AM, Weijun Wang wrote:

Please take a review at

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

BasicProc.java is enhanced to use a native JGSS provider, and KDC.java is 
enhanced to start (not use) a native KDC. For example, you would be able to 
test interop among Java JGSS, native JGSS (with MIT krb5) and Heimdal KDC with

jtreg -Dnative.krb5.lib=/usr/local/krb5/lib/libgssapi_krb5.so \
  -Dnative.kdc.path=/usr/local/heimdal \
  test/sun/security/krb5/auto/BasicProc.java

Without those 2 new system properties, it behaves like before, i.e. Java GSS on 
the embedded KDC.

Another change in Context.java. Instead of using shared states to provide 
username and password when doing a krb5 login, a callback handler is used. This 
is considered more common. An extra permission is needed to read the default 
username (though I think this can coded as optional).

Thanks
Max

Re: RFR 8186884: Test native KDC, Java krb5 lib, and native krb5 lib in one test

[Artem Smotrakov]

Hi Max,

Looks good to me. Below are a couple of minor comments you may want to 
address. No need a new webrev.


Thanks!

1. Proc.java, better to use braces

http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449

2. If you already updated KDC.java, it may be good to use 
try-with-resources for streams in a couple of places.


Artem


On 09/08/2017 06:19 AM, Weijun Wang wrote:

Small update on http://cr.openjdk.java.net/~weijun/8186884/webrev.02. All files 
belong to a single Proc now have the same prefix so they appear together in 
file list.

Thanks
Max


On Sep 7, 2017, at 8:39 PM, Weijun Wang  wrote:

Updated at

  http://cr.openjdk.java.net/~weijun/8186884/webrev.01/

Now the libraries can be more freely combined, so you can test interop between 
one native library and another one:

   jtreg -Dnative.krb5.libs=j=,n=,m=lib1.so,h=lib2.so BasicProc.java


More comments inline below.


On Sep 7, 2017, at 3:29 PM, Artem Smotrakov  wrote:

Hi Max,

In general, looks fine to me. Below are a couple of comments you might want to 
address.

1. BasicProc.java, it might be better to use named constants for parameters for 
once() method. That would make it easier to understand what each particular 
onse() call does

I am passing in label and library names now.


2. BasicProc.java, could you please add an exception message?

+if (!Arrays.equals(msg, msg2)) {
+throw new Exception();
+}
+break;

Fixed.


3. BasicProc.java, should the test do some cleanup then?

+Files.copy(Paths.get("ccache.base"), Paths.get("ccache." + label));

Nowadays I prefer to let jtreg do the cleanup/retain. In fact, I am able to 
find a KDC.java bug by saving the ccache, where the incoming service ticket is 
invalid and not saved into the ccache.

Thanks
Max


Artem

On 09/07/2017 03:07 AM, Weijun Wang wrote:

Please take a review at

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

BasicProc.java is enhanced to use a native JGSS provider, and KDC.java is 
enhanced to start (not use) a native KDC. For example, you would be able to 
test interop among Java JGSS, native JGSS (with MIT krb5) and Heimdal KDC with

jtreg -Dnative.krb5.lib=/usr/local/krb5/lib/libgssapi_krb5.so \
  -Dnative.kdc.path=/usr/local/heimdal \
  test/sun/security/krb5/auto/BasicProc.java

Without those 2 new system properties, it behaves like before, i.e. Java GSS on 
the embedded KDC.

Another change in Context.java. Instead of using shared states to provide 
username and password when doing a krb5 login, a callback handler is used. This 
is considered more common. An extra permission is needed to read the default 
username (though I think this can coded as optional).

Thanks
Max

Re: RFR[10] 8186057: TLS interoperability testing between different Java versions

[Artem Smotrakov]

In case of SNI, SSLParemeters.setServerNames() method (and others 
related to SNI) was introduced in 8. I don't think the code which use 
this method can be compiled with 6 and 7.


But if you want to test SNI with 8+, you need to call them.

https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLParameters.html#setServerNames-java.util.List-

Artem

On 09/07/2017 11:24 AM, sha.ji...@oracle.com wrote:

Hi Artem,

On 07/09/2017 16:07, Artem Smotrakov wrote:



- Please use try-with-resources if possible (files, sockets, etc)
The test uses only JDK 6-supported language features, but 
try-with-resources is introduced by JDK 7.
Here we come to another issue. I believe that it would be good to use 
write clients for all JDK versions. If you use the same code for all 
Java versions, that means that you use only JDK 6 API. Let's consider 
the following:

- we want to test 8 vs 9
- 8 and 9 may have some API (or just functionality) which is not 
supported by 6 (for example, ALPN, SNI)


If you write code which is compatible with 6, then you can use API 
from newer versions, but we still may want to test it.
In fact, the current test has already covered ALPN, though only JDK 9 
and 10 support the associated methods, like 
SSLParameters.getApplicationProtocols().
ALPN-associated case combinations are ignored if a JDK doesn't support 
this feature. Exactly, JdkUtils checks if a specific JDK build 
contains method SSLParameters.getApplicationProtocols().

I think the same approach could be applied for SNI in the future.

Best regards,
John Jiang


Artem


Best regards,
John Jiang


Artem

On 09/07/2017 08:52 AM, sha.ji...@oracle.com wrote:

Hi,
Please review this test for checking the interop compatibility on 
JSSE among different JDK releases (from 6 to 10).
It covers the cases, like handshake, data exchange, client 
authentication and APLN, on all TLS versions (if possible).
And the selected TLS cipher suites are: 
TLS_RSA_WITH_AES_128_CBC_SHA, TLS_DHE_DSS_WITH_AES_128_CBC_SHA and 
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA.

For more details, please look though the README.

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

Best regards,
John Jiang

Re: RFR[10] 8186057: TLS interoperability testing between different Java versions

[Artem Smotrakov]

Hi John,

Please see inline.


On 09/07/2017 10:52 AM, sha.ji...@oracle.com wrote:


Then you can build a Cartesian product of all parameters. Client and 
sever should take parameters, and say if they support all of the or 
not (for example, JDK 6 might not support all features that JDK 9 
does). If it does, client and server can configure themselves with 
those parameters, and start handshaking.
Frankly, what case combinations would be tested is a problem. 
Currently, I don't expand the combinations too much.
For example, I don't make combinations for data exchange and ALPN, and 
client auth always be enabled.

That's why I don't use complex data structures.
If the test requirements or case combinations could be confirmed, the 
data structures would be enriched.
That's fine to start with limited amount of parameters, but I believe we 
can design the test to be able to extend it easily in future. That's my 
main point here.


2. With the approach above, it might be possible to avoid those 
nested loops:


  93   for (String caseType : CASE_TYPES) {
  94 for (String protocol : PROTOCOLS) {
  95 for (String cipherSuite : CIPHER_SUITES) {
  96 for (JdkInfo serverJdk : jdkInfos) {
But, how to build the above Cartesian product automatically (not by 
manual)?
"automatically" means writing some code of course, but I believe we 
don't have to define such an array of cases manually.


3. I believe we should cover all supported cipher suites. Test run is 
going to take more time, but it think it's okay. Another option is to 
introduce two modes:

- light: run the test with a couple of cipher suites
- heavy: run the test with all supported cipher suites

I hesitate to do that.
Different JDK builds and TLS versions supports different cipher suites.
That's fine. If you test JDK 6 against 9, you can use only cipher suites 
which are supported by 6.
And many cipher suites use the same algorithms on key exchange, 
signature, encryption/decryption and authentication, then is it 
necessary to check each of them?
I think it would be worth to be able to do that at least. Again, we 
never know what can go wrong.
Although we don't know "what's going to break down", I'm not sure a 
single test could check every point on TLS communication.
Right, we can't test everything, but we can to our best. If we know that 
the test can be easily enhanced in this way, it's worth to do that.

A couple of comments about the code:
- What's the reason of using disabled SHA1 and RSA with 1024-bit 
keys? You might use stronger algorithms and key sizes, and avoid 
modifying java.security file
Different JDK builds possibly support different algorithms and have 
different restrictions on such algorithms and key sizes. Weaker or 
stronger, that is relative. And this point should not be a focus of 
the test.
So, I just pick the ones that are common in history, and provides an 
alternative java.security file to avoid possible broken.

That makes sense.



- Please use try-with-resources if possible (files, sockets, etc)
The test uses only JDK 6-supported language features, but 
try-with-resources is introduced by JDK 7.
Here we come to another issue. I believe that it would be good to use 
write clients for all JDK versions. If you use the same code for all 
Java versions, that means that you use only JDK 6 API. Let's consider 
the following:

- we want to test 8 vs 9
- 8 and 9 may have some API (or just functionality) which is not 
supported by 6 (for example, ALPN, SNI)


If you write code which is compatible with 6, then you can use API from 
newer versions, but we still may want to test it.


Artem


Best regards,
John Jiang


Artem

On 09/07/2017 08:52 AM, sha.ji...@oracle.com wrote:

Hi,
Please review this test for checking the interop compatibility on 
JSSE among different JDK releases (from 6 to 10).
It covers the cases, like handshake, data exchange, client 
authentication and APLN, on all TLS versions (if possible).
And the selected TLS cipher suites are: 
TLS_RSA_WITH_AES_128_CBC_SHA, TLS_DHE_DSS_WITH_AES_128_CBC_SHA and 
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA.

For more details, please look though the README.

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

Best regards,
John Jiang

Re: RFR 8186884: Test native KDC, Java krb5 lib, and native krb5 lib in one test

[Artem Smotrakov]

Hi Max,

In general, looks fine to me. Below are a couple of comments you might 
want to address.


1. BasicProc.java, it might be better to use named constants for 
parameters for once() method. That would make it easier to understand 
what each particular onse() call does


+once(true, true, true); // pure java
+if (LIBNAME != null) {
+// save a cache for client
+Context.fromUserPass(USER, PASS, false)
+.ccache("ccache.base");
+
+once(false, false, false); // fail fast for all 
native

+once(false, true, true);
+once(false, true, false);
+once(false, false, true);
+
+once(true, true, false);
+once(true, false, true);
+once(true, false, false);

Enums may help, and might make it simper as well:

+// Just a marker for which test case is finished
+String label = (jc?"j":"n") + (js?"j":"n") + (jb?"j":"n");

2. BasicProc.java, could you please add an exception message?

+if (!Arrays.equals(msg, msg2)) {
+throw new Exception();
+}
+break;

3. BasicProc.java, should the test do some cleanup then?

+Files.copy(Paths.get("ccache.base"), Paths.get("ccache." + 
label));


Artem

On 09/07/2017 03:07 AM, Weijun Wang wrote:

Please take a review at

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

BasicProc.java is enhanced to use a native JGSS provider, and KDC.java is 
enhanced to start (not use) a native KDC. For example, you would be able to 
test interop among Java JGSS, native JGSS (with MIT krb5) and Heimdal KDC with

 jtreg -Dnative.krb5.lib=/usr/local/krb5/lib/libgssapi_krb5.so \
   -Dnative.kdc.path=/usr/local/heimdal \
   test/sun/security/krb5/auto/BasicProc.java

Without those 2 new system properties, it behaves like before, i.e. Java GSS on 
the embedded KDC.

Another change in Context.java. Instead of using shared states to provide 
username and password when doing a krb5 login, a callback handler is used. This 
is considered more common. An extra permission is needed to read the default 
username (though I think this can coded as optional).

Thanks
Max

Re: RFR[10] 8186057: TLS interoperability testing between different Java versions

[Artem Smotrakov]

Hi John,

Thanks for starting working on this. I believe this tool is going to be 
very helpful.


Let me skip some coding comments for a while, I have a couple of 
comments about the design. The main idea is that it should cover as many 
cases as it can. Even if it might look a bit redundant, I believe it is 
actually not redundant because we basically never know what's going to 
break down.


1. At the moment, you define cases in Compatibility.java

  59 private static final String[] CASE_TYPES = new String[] {
  60 Utils.CASE_HANDSHAKE,
  61 Utils.CASE_DATA_EXCHANGE,
  62 Utils.CASE_ALPN };

I believe there are much more many case which we can implement later. 
But this approach doesn't looks flexible. For example, let's consider 
that we'd like to use a couple of extensions in the same time. Let's say 
we'd like to use SNI and ALPN. It seems like we have to create a 
separate case for that, and add it manually to the array. Would it be 
possible to generate cases automatically? For example, you can define 
parameters of TLS connection, for example:

- handshake type: full, session-resumption
- use ALPN: true, false
- use SNI: true, false
- use client auth: true, false,
etc

Then you can build a Cartesian product of all parameters. Client and 
sever should take parameters, and say if they support all of the or not 
(for example, JDK 6 might not support all features that JDK 9 does). If 
it does, client and server can configure themselves with those 
parameters, and start handshaking.


2. With the approach above, it might be possible to avoid those nested 
loops:


  93   for (String caseType : CASE_TYPES) {
  94 for (String protocol : PROTOCOLS) {
  95 for (String cipherSuite : CIPHER_SUITES) {
  96 for (JdkInfo serverJdk : jdkInfos) {

3. I believe we should cover all supported cipher suites. Test run is 
going to take more time, but it think it's okay. Another option is to 
introduce two modes:

- light: run the test with a couple of cipher suites
- heavy: run the test with all supported cipher suites


A couple of comments about the code:
- What's the reason of using disabled SHA1 and RSA with 1024-bit keys? 
You might use stronger algorithms and key sizes, and avoid modifying 
java.security file

- Please use try-with-resources if possible (files, sockets, etc)

Artem

On 09/07/2017 08:52 AM, sha.ji...@oracle.com wrote:

Hi,
Please review this test for checking the interop compatibility on JSSE 
among different JDK releases (from 6 to 10).
It covers the cases, like handshake, data exchange, client 
authentication and APLN, on all TLS versions (if possible).
And the selected TLS cipher suites are: TLS_RSA_WITH_AES_128_CBC_SHA, 
TLS_DHE_DSS_WITH_AES_128_CBC_SHA and 
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA.

For more details, please look though the README.

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

Best regards,
John Jiang

[10] RFR: 8182388: Backout 8182143

[Artem Smotrakov]

This patch backs out 8182143 because of possible issues on Windows even 
if we don't have a test to reproduce it.


Checking if SunMSCAPI provider is enabled looks like a hack. I filed 
https://bugs.openjdk.java.net/browse/JDK-8182386


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

Artem

Re: [10] RFR: 8182143: SHA224-based signature algorithms are not enabled for TLSv12 on Windows

[Artem Smotrakov]

Hi Xuelei,

Please see inline.


On 06/15/2017 07:32 PM, Xuelei Fan wrote:

Hi Artem,

If the key is generated in MSCAPI, the signature algorithm implemented in other 
provider cannot be actually used.
Yes, that's what I meant by key implementation incompatibility. Here is 
an example


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

We updated the test to use the same provider for key generation and 
signatures. I'll file a bug for that.


BTW, we also need to consider the case that only MSCAPI provider is enabled in 
practice.
Agree. If a provider provides everything what's necessary for a TLS 
connection - it should work.


Artem


In general, a signature algorithm is not included in the supported list unless 
all related providers support the signature algorithm.  We're looking for 
better solution, but not yet have one in hand.

Xuelei


On Jun 15, 2017, at 6:13 PM, Artem Smotrakov  wrote:

That sounds strange to me. I assume that if an algorithm is provided by a 
provider on all platforms, then it should work on all platforms no matter what. 
I am not sure that I really understand the problem, but probably it's about 
some problems that may occur if multiple providers are used together when for a 
TLS connection. I may guess that the problem may be in incompatibility of key 
implementations for different providers. If so, this looks like an issue to me. 
Please correct me if I am wrong.

Probably there may be some specific case which fails, but 
SignatureAlgorithms.java test works fine now, and seems like SHA224 can be 
successfully used for establishing a connection.

I am okay to back out the fix, but it would be good to have a testcase which 
shows the problem why the fix should be backed out. Then, we can work on a 
solution for that.

Artem



On 06/15/2017 04:37 PM, Xuelei Fan wrote:
Hi Bernd,

Thanks for the correction.  I really missed the point that there are issues to 
enabled SHA-224 for SunMSCAPI provider.


On 6/15/2017 4:06 PM, Bernd Eckenfels wrote:
Hello,

If I recall correctly the idea of disabling those algorithms if SunMSCAPI IS(!) 
present was to avoid agreeing on a Signature algorithm which could not be 
supported by RSA offloaded keys inside CryptoAPI.

Having said that the suggested ciphers might need to be made dependent on the 
capabilities of the Signature provider for a given key type (especially if it 
is a key handle only).


Agreed.  Besides, we may check the availability of each signature and hash 
algorithms, rather than hard-coded them.  I filed a new bug for the tracking:
   https://bugs.openjdk.java.net/browse/JDK-8182318

Thanks & Regards,
Xuelei


Has this changed and the signatures are supported now by MSCapi?

Gruss
Bernd
--
http://bernd.eckenfels.net

*From:* security-dev  on behalf of Artem 
Smotrakov 
*Sent:* Thursday, June 15, 2017 10:57:00 PM
*To:* Xuelei Fan; Security Dev OpenJDK
*Subject:* [10] RFR: 8182143: SHA224-based signature algorithms are not enabled 
for TLSv12 on Windows
Hi Xuelei,

Could you please take a look at this patch?

It enables SHA224-based signature algorithms on Windows since they
should be provided not only by SunMSCAPI provider. Please see details in
the bug description.

The test works fine on all supported platforms.

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

Artem

Re: [10] RFR: 8182143: SHA224-based signature algorithms are not enabled for TLSv12 on Windows

[Artem Smotrakov]

That sounds strange to me. I assume that if an algorithm is provided by 
a provider on all platforms, then it should work on all platforms no 
matter what. I am not sure that I really understand the problem, but 
probably it's about some problems that may occur if multiple providers 
are used together when for a TLS connection. I may guess that the 
problem may be in incompatibility of key implementations for different 
providers. If so, this looks like an issue to me. Please correct me if I 
am wrong.


Probably there may be some specific case which fails, but 
SignatureAlgorithms.java test works fine now, and seems like SHA224 can 
be successfully used for establishing a connection.


I am okay to back out the fix, but it would be good to have a testcase 
which shows the problem why the fix should be backed out. Then, we can 
work on a solution for that.


Artem


On 06/15/2017 04:37 PM, Xuelei Fan wrote:

Hi Bernd,

Thanks for the correction.  I really missed the point that there are 
issues to enabled SHA-224 for SunMSCAPI provider.


On 6/15/2017 4:06 PM, Bernd Eckenfels wrote:

Hello,

If I recall correctly the idea of disabling those algorithms if 
SunMSCAPI IS(!) present was to avoid agreeing on a Signature 
algorithm which could not be supported by RSA offloaded keys inside 
CryptoAPI.


Having said that the suggested ciphers might need to be made 
dependent on the capabilities of the Signature provider for a given 
key type (especially if it is a key handle only).


Agreed.  Besides, we may check the availability of each signature and 
hash algorithms, rather than hard-coded them.  I filed a new bug for 
the tracking:

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

Thanks & Regards,
Xuelei


Has this changed and the signatures are supported now by MSCapi?

Gruss
Bernd
--
http://bernd.eckenfels.net

*From:* security-dev  on 
behalf of Artem Smotrakov 

*Sent:* Thursday, June 15, 2017 10:57:00 PM
*To:* Xuelei Fan; Security Dev OpenJDK
*Subject:* [10] RFR: 8182143: SHA224-based signature algorithms are 
not enabled for TLSv12 on Windows

Hi Xuelei,

Could you please take a look at this patch?

It enables SHA224-based signature algorithms on Windows since they
should be provided not only by SunMSCAPI provider. Please see details in
the bug description.

The test works fine on all supported platforms.

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

Artem

[10] RFR: 8182143: SHA224-based signature algorithms are not enabled for TLSv12 on Windows

[Artem Smotrakov]

Hi Xuelei,

Could you please take a look at this patch?

It enables SHA224-based signature algorithms on Windows since they 
should be provided not only by SunMSCAPI provider. Please see details in 
the bug description.


The test works fine on all supported platforms.

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

Artem

Re: RFR(XS) : 8180895 : java/security/AccessController/DoPrivAccompliceTest.java has to be improved

[Artem Smotrakov]

@summary doesn't seem to be correct, the test uses user.name system 
property.


Otherwise, looks good for me.

Artem


On 05/23/2017 10:17 PM, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8180895/webrev.00/index.html

81 lines changed: 37 ins; 23 del; 21 mod;

Hi all,

could you please review the fix which improves and refactors 
DoPrivAccompliceTest.java test?
the test had several issues:
  - it did not fail if the java spawned by JavaToolUtils::runJava exists w/ 
non-zero exit code
  - negative test case was not covered, i.e. the test did not check that if 
privileges are not granted to the right jar-file an exception is thrown
  - the test created files inside test.classes, which might interfere w/ other 
tests
  - the test uses JavaToolUtils testlibrary class to create a jar, start a jvm, 
which duplicates functionality of the common testlibrary and will be removed by 
8180898[1]

webrev: http://cr.openjdk.java.net/~iignatyev//8180895/webrev.00/index.html
jbs: https://bugs.openjdk.java.net/browse/JDK-8180895
testing: affected tests

[1] https://bugs.openjdk.java.net/browse/JDK-8180898

Thanks,
-- Igor

Re: [9] RFR: 8176183: sun/security/mscapi/SignedObjectChain.java fails on Windows

[Artem Smotrakov]

Thank you Max for the review.

This seems to be true that the actual provider may be selected when 
Signature.init() is called. At least, I see that 
Signature.engineInitSign() may call chooseProvider() method.


http://hg.openjdk.java.net/jdk9/dev/jdk/file/a54e33fc0f2d/src/java.base/share/classes/java/security/Signature.java#l1197

Artem


On 03/08/2017 05:57 PM, Weijun Wang wrote:



On 03/09/2017 09:55 AM, Artem Smotrakov wrote:

Hi Max,

I am not sure if SunMSCAPI has higher priority than SunJCE, SUN, etc on
Windows. But this test is for SunMSCAPI provider, so we explicitly set
this provider for Signature.


OK. You fix is good then.

What I was saying that even if SunMSCAPI is not the preferred 
provider, after you call Signature.getInstance("SHA256withRSA") the 
actual provider is not finally determined. When you call sig.init(key, 
SIGN), it will choose key's provider as its own provider.


At least this is how to I understand it.

Thanks
Max



Artem


On 03/08/2017 04:50 PM, Weijun Wang wrote:

I remember Signature is able to automatically switch to the correct
provider when its init(key) is called. Does this mean you don't need
to care about Signature.getInstance(alg,provider)?

Thanks
Max

On 03/09/2017 05:52 AM, Artem Smotrakov wrote:

Hello,

The test fails with "Key type not supported" error on Windows only 
with

SunMSCAPI provider. It happens because the test passes an incompatible
key object to Signature instance. Please see more details in

https://bugs.openjdk.java.net/browse/JDK-8176183?focusedCommentId=14058876&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14058876 





The following patch updates Chain.java (which is used by
SignedObjectChain.java) to use specified security provider for both 
key

generation and singing.

I ran all tests which depend on Chain.java on all generic platforms,
they worked fine.

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

Webrev: http://cr.openjdk.java.net/~asmotrak/8176183/webrev.00/

Artem

Re: [9] RFR: 8176183: sun/security/mscapi/SignedObjectChain.java fails on Windows

[Artem Smotrakov]

Hi Max,

I am not sure if SunMSCAPI has higher priority than SunJCE, SUN, etc on 
Windows. But this test is for SunMSCAPI provider, so we explicitly set 
this provider for Signature.


Artem


On 03/08/2017 04:50 PM, Weijun Wang wrote:
I remember Signature is able to automatically switch to the correct 
provider when its init(key) is called. Does this mean you don't need 
to care about Signature.getInstance(alg,provider)?


Thanks
Max

On 03/09/2017 05:52 AM, Artem Smotrakov wrote:

Hello,

The test fails with "Key type not supported" error on Windows only with
SunMSCAPI provider. It happens because the test passes an incompatible
key object to Signature instance. Please see more details in

https://bugs.openjdk.java.net/browse/JDK-8176183?focusedCommentId=14058876&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14058876 




The following patch updates Chain.java (which is used by
SignedObjectChain.java) to use specified security provider for both key
generation and singing.

I ran all tests which depend on Chain.java on all generic platforms,
they worked fine.

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

Webrev: http://cr.openjdk.java.net/~asmotrak/8176183/webrev.00/

Artem

[9] RFR: 8176183: sun/security/mscapi/SignedObjectChain.java fails on Windows

[Artem Smotrakov]

Hello,

The test fails with "Key type not supported" error on Windows only with 
SunMSCAPI provider. It happens because the test passes an incompatible 
key object to Signature instance. Please see more details in


https://bugs.openjdk.java.net/browse/JDK-8176183?focusedCommentId=14058876&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14058876

The following patch updates Chain.java (which is used by 
SignedObjectChain.java) to use specified security provider for both key 
generation and singing.


I ran all tests which depend on Chain.java on all generic platforms, 
they worked fine.


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

Webrev: http://cr.openjdk.java.net/~asmotrak/8176183/webrev.00/

Artem

Re: RFR 8172975: SecurityTools.keytool() needs to accept user input

[Artem Smotrakov]

It might be better to remove the response file in a finally block. 
Main.java needs new copyright year.


Otherwise, looks fine.

I see you removed public sign() and verify() methods in SecurityTools, 
please make sure that they are not used by other tests.


Artem


On 01/20/2017 02:44 PM, Weijun Wang wrote:

Updated

http://cr.openjdk.java.net/~weijun/8172975/root/webrev.01/
http://cr.openjdk.java.net/~weijun/8172975/webrev.01/

I'll need this in my other work.

Thanks
Max

On 01/20/2017 07:23 PM, Artem Smotrakov wrote:

It's up to you. You can change it now if you have time, or we can do it
once we need to update jarsigner tests.

Artem

Re: RFR 8172975: SecurityTools.keytool() needs to accept user input

[Artem Smotrakov]

It's up to you. You can change it now if you have time, or we can do it 
once we need to update jarsigner tests.


Artem


On 01/20/2017 12:23 PM, Weijun Wang wrote:

Also, I am feeling that the jarsigner-related calls are quite
complicated. I suggest we use the same method for signing and
verifying and ask the user to provide all arguments in their order. So
instead of calling

   SecurityTools.jarsigner("x.jar", "alias", "...");

we just call

   SecurityTools.jarsigner("... x.jar alias");

This looks better to me as well.

I am fine with current webrevs.


A little confused. Do you think the current webrev is fine or is it 
better to rewrite jarsigner()? 

Re: RFR 8172975: SecurityTools.keytool() needs to accept user input

[Artem Smotrakov]

Hi Max,

Please see inline.


On 01/19/2017 05:15 PM, Weijun Wang wrote:



On 01/19/2017 09:40 PM, Artem Smotrakov wrote:

Hi Max,

In general, looks okay.

Would it be better if it called redirectInput() only if the response
file exists? keytool() method might also delete the response file after
reading it. These two measures may prevent situations when the response
file is unnecessary used.


Yes, it's good to remove the file after reading it, so this means 
people need to set it for every call.


On the other hand, I still think creating an empty file and call 
redirectInput() on it is necessary when the response file does not 
exist. This prevents the keytool() call from hanging forever if it 
actually needs user input but the test has not provided any.
It looks like a test issue with particular test, I don't remember many 
tests which failed because of this.


Also, I am feeling that the jarsigner-related calls are quite 
complicated. I suggest we use the same method for signing and 
verifying and ask the user to provide all arguments in their order. So 
instead of calling


   SecurityTools.jarsigner("x.jar", "alias", "...");

we just call

   SecurityTools.jarsigner("... x.jar alias");

This looks better to me as well.

I am fine with current webrevs.

Artem


This is consistent with keytool(), and we can also provide 3 
overloaded forms.


What do you think? :-)

Thanks
Max



What do you think?

Artem


On 01/18/2017 05:50 PM, Weijun Wang wrote:

Please review the code changes at

   root: http://cr.openjdk.java.net/~weijun/8172975/root/webrev.00/
   jdk: http://cr.openjdk.java.net/~weijun/8172975/webrev.00/

The fix is in root repo. This is not an elegant solution because it
uses a separate method to provide the response. This means you have to
clear the response if the next keytool call does not need it. This
also means you cannot run keytool in multiple threads.

I didn't provide the response as an extra argument because there are
already many overloaded keytool() methods, and I do not want to invent
a new method name (say, keytoolWithResponse) and implement the same
number of overloaded methods.

If you are strongly against this solution, I'll think of another way.

The jdk change includes a test for this change, as well as a trivial
fix for keytool's getYesNoReply() method. Otherwise an NPE is thrown
with the following command:

   cat untrusted.cert | keytool -importcert -alias a

Thanks
Max

Re: RFR 8172975: SecurityTools.keytool() needs to accept user input

[Artem Smotrakov]

Hi Max,

In general, looks okay.

Would it be better if it called redirectInput() only if the response 
file exists? keytool() method might also delete the response file after 
reading it. These two measures may prevent situations when the response 
file is unnecessary used.


What do you think?

Artem


On 01/18/2017 05:50 PM, Weijun Wang wrote:

Please review the code changes at

   root: http://cr.openjdk.java.net/~weijun/8172975/root/webrev.00/
   jdk: http://cr.openjdk.java.net/~weijun/8172975/webrev.00/

The fix is in root repo. This is not an elegant solution because it 
uses a separate method to provide the response. This means you have to 
clear the response if the next keytool call does not need it. This 
also means you cannot run keytool in multiple threads.


I didn't provide the response as an extra argument because there are 
already many overloaded keytool() methods, and I do not want to invent 
a new method name (say, keytoolWithResponse) and implement the same 
number of overloaded methods.


If you are strongly against this solution, I'll think of another way.

The jdk change includes a test for this change, as well as a trivial 
fix for keytool's getYesNoReply() method. Otherwise an NPE is thrown 
with the following command:


   cat untrusted.cert | keytool -importcert -alias a

Thanks
Max

Re: RFR 8171353: New home for SecurityTools.java test utility

[Artem Smotrakov]

Hi Max,

I am fine with it.

Artem


On 12/16/2016 12:07 AM, Wang Weijun wrote:

Hi Artem

I hope you are OK with this change:

diff --git a/test/lib/security/SecurityTools.java 
b/test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
rename from test/lib/security/SecurityTools.java
rename to test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
--- a/test/lib/security/SecurityTools.java
+++ b/test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
@@ -20,13 +20,11 @@
   * or visit www.oracle.com if you need additional information or have any
   * questions.
   */
+package jdk.testlibrary;

  import java.util.ArrayList;
  import java.util.Collections;
  import java.util.List;
-import jdk.testlibrary.JDKToolLauncher;
-import jdk.testlibrary.OutputAnalyzer;
-import jdk.testlibrary.ProcessTools;

  public class SecurityTools {

diff --git a/test/sun/security/tools/keytool/PrintSSL.java 
b/test/sun/security/tools/keytool/PrintSSL.java
--- a/test/sun/security/tools/keytool/PrintSSL.java
+++ b/test/sun/security/tools/keytool/PrintSSL.java
@@ -25,7 +25,6 @@
   * @test
   * @bug 6480981 8160624
   * @summary keytool should be able to import certificates from remote SSL 
server
- * @library /lib/security
   * @library /lib/testlibrary
   * @run main/othervm PrintSSL
   */
@@ -37,6 +36,7 @@
  import javax.net.ssl.SSLServerSocketFactory;
  import javax.net.ssl.SSLSocket;
  import jdk.testlibrary.OutputAnalyzer;
+import jdk.testlibrary.SecurityTools;

  public class PrintSSL {

diff --git a/test/sun/security/tools/keytool/ReadJar.java 
b/test/sun/security/tools/keytool/ReadJar.java
--- a/test/sun/security/tools/keytool/ReadJar.java
+++ b/test/sun/security/tools/keytool/ReadJar.java
@@ -25,7 +25,6 @@
   * @test
   * @bug 6890872 8168882
   * @summary keytool -printcert to recognize signed jar files
- * @library /lib/security
   * @library /lib/testlibrary
   */

@@ -33,6 +32,7 @@
  import java.nio.file.Paths;
  import jdk.testlibrary.JarUtils;
  import jdk.testlibrary.OutputAnalyzer;
+import jdk.testlibrary.SecurityTools;

  public class ReadJar {

Thanks
Max

Re: [9] RFR: 8166531: sun/security/ssl/SocketCreation/SocketCreation.java fails intermittently

[Artem Smotrakov]

Hi Xuelei,

I am not sure either, but I would like to try. If it doesn't work, we 
can go and re-write the test to follow SSLSocketTemplate. Actually, I 
don't think it is really possible because SSLSocketTemplate uses 
connect() method which is not used by the test.


Artem


On 12/15/2016 02:04 PM, Xuelei Fan wrote:

I'm not sure this update would work.

As this is a special test, it might be possible to copy/past and 
update the template code (or old sample code) here as we did to use 
the old template.


Xuelei

On 12/15/2016 11:23 AM, Artem Smotrakov wrote:

Hello,

Please review the patch below for
sun/security/ssl/SocketCreation/SocketCreation.java test.

The test has been observed to fail intermittently, but I couldn't
reproduce it by running the test in a loop. The test creates sockets for
TLS connection in different ways. Even if the test is very old, I
wouldn't like to change its original behavior. It might be better to use
SSLSocketTemplate here, but it would require a significant update to
SSLSocketTemplate, and splitting the test to 5 tests. Instead of this, I
am proposing to add some simple synchronization on client side where the
test was seen to fail. If it doesn't work, we can try to use
SSLSocketTemplate then.

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

Artem

[9] RFR: 8166531: sun/security/ssl/SocketCreation/SocketCreation.java fails intermittently

[Artem Smotrakov]

Hello,

Please review the patch below for 
sun/security/ssl/SocketCreation/SocketCreation.java test.


The test has been observed to fail intermittently, but I couldn't 
reproduce it by running the test in a loop. The test creates sockets for 
TLS connection in different ways. Even if the test is very old, I 
wouldn't like to change its original behavior. It might be better to use 
SSLSocketTemplate here, but it would require a significant update to 
SSLSocketTemplate, and splitting the test to 5 tests. Instead of this, I 
am proposing to add some simple synchronization on client side where the 
test was seen to fail. If it doesn't work, we can try to use 
SSLSocketTemplate then.


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

Artem

Re: RFR 8075618: Create tests to check jarsigner work with multi-version jar

[Artem Smotrakov]

You can use 
http://hg.openjdk.java.net/jdk9/dev/jdk/file/d4d7f1f0d688/test/lib/security/SecurityTools.java 
which would simplify the code. This lib was added to be used in such tests.


Note that SecurityTools addresses a couple of known issues with running 
security tools on machines with different locales, please see 
"user.language" and "user.country" system properties


Artem


On 12/12/2016 02:43 PM, Amanda Jiang wrote:

Hi All,

Please help to review following changeset, which tests that jarsigner 
tool works with multi-release JAR files.


Webrev: http://cr.openjdk.java.net/~amjiang/8075618/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8075618


Thanks,
Amanda

Re: [9] RFR JDK-8157529 : Remove intermittent key from javax/net/ssl/DTLS/CipherSuite.java

[Artem Smotrakov]

Looks good to me.

Artem


On 11/30/2016 10:29 PM, Tim Du wrote:

Hi ,

Would you help to review the path for "8157529:Remove intermittent key 
from javax/net/ssl/DTLS/CipherSuite.java" , the intermittently failed 
issue was fixed by JDK-8167680 , '@key intermittent ' can be 
removed.Thanks.


JBS: https://bugs.openjdk.java.net/browse/JDK-8157529
webrev: http://cr.openjdk.java.net/~tidu/8157529/webrev.00/

Regards
Tim

Re: Code Review Request JDK-8170329 New SSLSocket testing template

[Artem Smotrakov]

Hi Xuelei,

Please see inline.


On 12/02/2016 09:48 PM, Xue-Lei Fan wrote:



- Exceptions are printed out in startServer/startClient methods, it
doesn't look necessary to use suppressed exceptions and initCause()
method. What was wrong with the code in runTest() method? The 
code in

runTest() method looks shorter and cleaner to me.


The main issue of runTest() is that it does not throw the original
exception.  Exception tacks have more information for debugging. I
want to keep the good side of Brad's previous hardness on this point.
It is not clear what's the original exception should be because you 
have

client and server. If you print all exceptions in
startServer/startClient (right after they occurred), then you don't 
hide

anything helpful for debugging.


The exception dump may have more information than the single exception
track, for example the nested cause stacks.

Can't it be printed out in startServer/startClient accordingly?
Throw the right exception is important even for test case.  Having the 
Java handling the exception is more nature.  Reading the debug log 
dump is not as straightforward as read the exception at the end of 
each run. Wrapping into RuntimeException is very confusing sometimes.


Comparing the following two log:
   java.net.XXException:  this is an exception message.
   cased by A
   caused by B.
and
   java.lang.RuntimeException:  
  caused by java.net.XXException:  this is an exception message.
  caused by A.
  (cause by B may be swallowed)

I don't like the later one, which introduce unnecessary exception 
stacks and take more time for the debugging.

I am not suggesting neither #2.

I am suggesting not to use initCause because it modifies the original 
exception, and not to use addSupressed() because it's not clear where 
the suppressed exception came from. Instead, I am suggesting to print 
all original exceptions on that sides where they occurred. I am not 
suggesting to wrap anything into a runtime exception. A runtime 
exception can be used just to indicate a test failure.



The original exception handling was very simple, and a lot
improvements were made so that the log is easier for debugging.


IMO, suppressed exceptions may confuse here (that's just from my
experience looking at JSSE test logs). The logic in lines 739-763
doesn't make it cleaner what exception was thrown on what side. It 
does

"local.initCause(remote)", but actually "remote" is not a cause of
"local". Finally, it does "exception.addSuppressed(startException)"
where "startException" can be thrown either on server or client side
which actually depends on test logic (an engineer needs to keep it in
mind).

Would it be better to have in logs something simple like the 
following?



This is an unexpected exception on client side:

This is an unexpected exception on server side:

Test failed.
...

Doesn't it look simpler?

The purpose of suppressed exception is fully use of the exception for
debugging.  The above suggestion still lose some exception debugging
information.

What kind of information is lost if you don't use suppressed exceptions?

Deep causes of the exception, and the nature about how an java command 
dump the exception.  We have to research what kind of information may 
be lost here.  I don't want that kind of research and using the nature 
of the exception is easier to me.
I don't think that anything is going to be lost if you just print an 
exception right away.


Artem



"startException" can be printed out right away when it occurs which I
think would be cleaner.

I dumped the exception message (line 808/818), hopefully it can help a
little bit if confusing.

I believe this is right place to dump everything out.

;-) I don't think so.

Thanks for the review.

Xuelei


It just needs to
make sure that it exceptions traces from server/client sides don't mix
up, so may be it's better to add some synchronization on System.out like
it was done in print() method.


It's good suggestion that we'd better to tell which side (the client
or server side) throws the exception.  I will think more about it in
the next webrev update.

Sure, thank you.

Artem

Re: Code Review Request JDK-8170329 New SSLSocket testing template

[Artem Smotrakov]

Please see inline


On 12/02/2016 05:25 PM, Xue-Lei Fan wrote:



- Why did you remove Peer and Application interfaces? I think those
interfaces make SSLSocketTemplate more flexible since it allows 
override

doServerSide/doClientSide logic if necessary - it doesn't seem to be
worse. If there is no strong reason to remove those interfaces, I 
would

prefer to keep them with default static/stateless
doServerSide/doClientSide versions.

I agree with you that The Peer and Application interfaces are more
flexible.  I have no strong reason to remove them.  For this update, I
focus more on simple and minimal sub-classes.  The Peer and
Application interfaces need some complicated coding (condition,
threading, etc) in sub-classes, so the design does not fall into the
scope.  I may prefer to remove them at this update and see what
happens if we moving forward with more test update. We can add them
back or create a more flexible one if we need the flexibility in the
future.

We have a lot of JSSE tests which may need to do something else in
doServerSide() and doClientSide() methods (that's what I learned from my
experience working with JSSE tests). Peer and Application interfaces
basically allow to override doServerSide() and doClientSide() methods.
Your changes make doServerSide() and doClientSide() methods private, so
that test can't add something inside it.

If you think those interfaces are helpful, I think it's better to keep
Peer and Application interfaces instead of removing/adding them back and
forth.

Another option may be providing methods which are called in
doServerSide() and doClientSide(). Those methods may be overridden by a
test if it needs to do extra stuff inside doServerSide() and
doClientSide().

I am fine with both ways, so it's up to you. But I would like to make it
clear what way we want to follow.

What do you think?
You points above are good.  When I made the update, I struggled a lot 
about the balance of flexible and simplicity.  I was inclined to 
simplicity at last as we can update the template if any new feature is 
required in the future.  I'm not sure how much this template can be used,
I suspect that all JSSE tests should be updated to use this template to 
avoid intermittent failures which we have been seeing.
exposing as less methods as possible will give us more flexible to 
make further update.  So I made most of the methods private, and 
removed a lot of methods.  Let's update the access scope if we really 
need them in the future.  A small public footprint is easier to start.
We already started a while ago :) Those methods which you removed were 
actually used by other tests which you updated. My point is that we'd 
better understand how we want the tests to be organized to avoid 
adding/removing stuff back and forth. Again, I believe that lots of 
tests should be updated to use this template. I am not suggesting to 
predict everything that the tests may need right away. If you think that 
Peer and Application interfaces are not good enough, it's fine to me, 
but we need to understand what would be a replacement for them

- Exceptions are printed out in startServer/startClient methods, it
doesn't look necessary to use suppressed exceptions and initCause()
method. What was wrong with the code in runTest() method? The code in
runTest() method looks shorter and cleaner to me.


The main issue of runTest() is that it does not throw the original
exception.  Exception tacks have more information for debugging. I
want to keep the good side of Brad's previous hardness on this point.

It is not clear what's the original exception should be because you have
client and server. If you print all exceptions in
startServer/startClient (right after they occurred), then you don't hide
anything helpful for debugging.

The exception dump may have more information than the single exception 
track, for example the nested cause stacks.

Can't it be printed out in startServer/startClient accordingly?
  The original exception handling was very simple, and a lot 
improvements were made so that the log is easier for debugging.



IMO, suppressed exceptions may confuse here (that's just from my
experience looking at JSSE test logs). The logic in lines 739-763
doesn't make it cleaner what exception was thrown on what side. It does
"local.initCause(remote)", but actually "remote" is not a cause of
"local". Finally, it does "exception.addSuppressed(startException)"
where "startException" can be thrown either on server or client side
which actually depends on test logic (an engineer needs to keep it in
mind).

Would it be better to have in logs something simple like the following?


This is an unexpected exception on client side:

This is an unexpected exception on server side:

Test failed.
...

Doesn't it look simpler?
The purpose of suppressed exception is fully use of the exception for 
debugging.  The above suggestion still lose some exception debugging 
information. 

What kind of information is l

Re: Code Review Request JDK-8170329 New SSLSocket testing template

[Artem Smotrakov]

Hi Xuelei,

Please see inline.

On 12/02/2016 03:53 PM, Xue-Lei Fan wrote:
Let's whether Sean or Weijun can have free cycle for the review of 
this part.

Yeah, that would be great.



- Why did you remove Peer and Application interfaces? I think those
interfaces make SSLSocketTemplate more flexible since it allows override
doServerSide/doClientSide logic if necessary - it doesn't seem to be
worse. If there is no strong reason to remove those interfaces, I would
prefer to keep them with default static/stateless
doServerSide/doClientSide versions.
I agree with you that The Peer and Application interfaces are more 
flexible.  I have no strong reason to remove them.  For this update, I 
focus more on simple and minimal sub-classes.  The Peer and 
Application interfaces need some complicated coding (condition, 
threading, etc) in sub-classes, so the design does not fall into the 
scope.  I may prefer to remove them at this update and see what 
happens if we moving forward with more test update. We can add them 
back or create a more flexible one if we need the flexibility in the 
future.
We have a lot of JSSE tests which may need to do something else in 
doServerSide() and doClientSide() methods (that's what I learned from my 
experience working with JSSE tests). Peer and Application interfaces 
basically allow to override doServerSide() and doClientSide() methods. 
Your changes make doServerSide() and doClientSide() methods private, so 
that test can't add something inside it.


If you think those interfaces are helpful, I think it's better to keep 
Peer and Application interfaces instead of removing/adding them back and 
forth.


Another option may be providing methods which are called in 
doServerSide() and doClientSide(). Those methods may be overridden by a 
test if it needs to do extra stuff inside doServerSide() and doClientSide().


I am fine with both ways, so it's up to you. But I would like to make it 
clear what way we want to follow.


What do you think?



It also might make sense to make
createSSLContext() a part of public API which I think would make the
template more flexible.

We have had the protected createClient/ServerSSLContext() methods.
Making createSSLContext() accessible doesn't seem to be worse to me. It 
may be final if you don't want anyone to override it for some reason.



- Exceptions are printed out in startServer/startClient methods, it
doesn't look necessary to use suppressed exceptions and initCause()
method. What was wrong with the code in runTest() method? The code in
runTest() method looks shorter and cleaner to me.

The main issue of runTest() is that it does not throw the original 
exception.  Exception tacks have more information for debugging. I 
want to keep the good side of Brad's previous hardness on this point.
It is not clear what's the original exception should be because you have 
client and server. If you print all exceptions in 
startServer/startClient (right after they occurred), then you don't hide 
anything helpful for debugging.


IMO, suppressed exceptions may confuse here (that's just from my 
experience looking at JSSE test logs). The logic in lines 739-763 
doesn't make it cleaner what exception was thrown on what side. It does 
"local.initCause(remote)", but actually "remote" is not a cause of 
"local". Finally, it does "exception.addSuppressed(startException)" 
where "startException" can be thrown either on server or client side 
which actually depends on test logic (an engineer needs to keep it in mind).


Would it be better to have in logs something simple like the following?


This is an unexpected exception on client side:

This is an unexpected exception on server side:

Test failed.
...

Doesn't it look simpler?



- lines 114-123, this code is used quite often by tests, why don't we
keep it in SSLSocketTemplate?

Good point!  I don't like it in SSLSocketTemplate as it is used for 
HTTP connections only, but it may be worthy to have a sub-template for 
HTTPS client testing.  What do you think if we address this 
enhancement in a new bug?
I am fine with it. My point is that SSLSocketTemplate may contain some 
useful static methods like this (to avoid duplicate code).


Artem

Re: Code Review Request JDK-8170329 New SSLSocket testing template

[Artem Smotrakov]

Hi Xuelei,

I am not sure how the updates in SimpleValidator relate to the template 
for JSSE tests. It might be better to separate those changes if I am not 
missing something. This update in SimpleValidator looks okay to me, but 
taking into account Sean's comments below, I'll let someone who is more 
knowledgeable review it as well. I would prefer not to mix it with 
updates for SSLSocketTemplate.


SSLSocketTemplate.java

- Fields in ContextParameters can be final
- Looks like JSSE test are supposed to extend SSLSocketTemplate. I 
suppose serverCondition/clientCondition should not be static then. Some 
JSSE test may be safely run in same JVM mode, I think it is better to 
make them non-static.
- Why did you remove Peer and Application interfaces? I think those 
interfaces make SSLSocketTemplate more flexible since it allows override 
doServerSide/doClientSide logic if necessary - it doesn't seem to be 
worse. If there is no strong reason to remove those interfaces, I would 
prefer to keep them with default static/stateless 
doServerSide/doClientSide versions.
- It may be better to pass a ContextParameters instance to 
createSSLContext() method because we may want to use more parameters 
while creating an SSL context. It also might make sense to make 
createSSLContext() a part of public API which I think would make the 
template more flexible.
- Exceptions are printed out in startServer/startClient methods, it 
doesn't look necessary to use suppressed exceptions and initCause() 
method. What was wrong with the code in runTest() method? The code in 
runTest() method looks shorter and cleaner to me.


ProxyAuthTest.java

- line 153, I believe try-with-resources doesn't make it worse.
- lines 114-123, this code is used quite often by tests, why don't we 
keep it in SSLSocketTemplate?


Artem

On 12/02/2016 11:23 AM, Xue-Lei Fan wrote:

On 11/29/2016 5:22 AM, Sean Mullan wrote:

On 11/27/16 7:43 AM, Xuelei Fan wrote:

On 11/27/2016 6:04 PM, Wang Weijun wrote:

This is not only a test update.

No, I happened to find an implementation issue with the new test, so 
fix

it altogether.  The issue is that the simple validator
(SimpleValidator.java) does not support SKID/AKID during cert path
build.  If two trusted certs has the same subject,  the simple 
validator

may not be able to find the right one.


We have had issues in the PKIX CertPathBuilder with matching on
AKID/SKID when building certpaths, so we want to be careful not to
introduce a similar issue. See this bug for more information:

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

I have not reviewed the fix enough to know if this issue applies here
but please double-check it.

The KID are used for best effort matching in this update.  If no KIDs 
get matched, the previous behavior is reserved. Should be safe, I think.


Xuelei


--Sean



Thanks,
Xuelei

On Nov 27, 2016, at 9:35 AM, Xuelei Fan  
wrote:


Hi,

Please review this test update:

  http://cr.openjdk.java.net/~xuelei/8170329/webrev.00/

The new template (SSLSocketTemplate.java) could be used to avoid the
anti-free-port issues.  By using sub-classes of it, the new one can
simplify the general SSLSocket test code significantly.

Thanks,
Xuelei

Re: [9] RFR: 8168969: Merge SSLSocketSample and SSLSocketTemplate

[Artem Smotrakov]

I don't like it, but no lambdas, no nothing in SSLSocketTemplate.java

http://cr.openjdk.java.net/~asmotrak/8168969/webrev.01/

Xuelei,

Could you please take a look?

Artem


On 11/02/2016 09:14 AM, Artem Smotrakov wrote:

Hi Xuelei,

This totally makes sense. But in my opinion we should use new features 
and APIs because:

- they are here to make code readable (okay, not to much lambdas)
- they help to avoid bugs (e.g., forgetting to close resources)
- we implicitly provide test coverage for new APIs
- we give examples how these features can be used
- finally, we encourage people to move to new Java versions

Not sure if I got correction what you mean by removing the part to 
declare the store files in each sub-classes. Do you mean getting rid 
of setting keystore/truststore in actual test files?


If so, I would prefer to do it in a separate enhancement. Most of 
SSL/TLS tests use keystores from "etc" directory. The test use 
"test.src" system property (set by jtreg) to build a path to "etc", 
and it depends on actual directory where a test stays. Also if I 
recall correctly, some tests use their own keystore. This update may 
be quite big. Would you mind if we did it separately?


8168969 is about merging helper classes to remove possible confusions 
Brad mentioned recently.


Artem

On 11/02/2016 03:57 AM, Xuelei Fan wrote:
Please use JDK 6 only features (no Lambda, try-with-resource, 
multi-catch, java.util.Objects, etc.) so as to expedite porting to 
previous releases.


It would be nice if removing the part to declare the store files in 
each sub-classes.


Xuelei

On 11/2/2016 2:48 PM, John Jiang wrote:

Hi Artem,
Thanks for making the template to be used easily.

The tests in your patch extend class SSLSocketTemplate, but
SSLSocketTemplate looks like an utility class, but not a parent class.
For example,
test/sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java 


  67 new SSLSocketTemplate()
AnonCipherWithWantClientAuth still creates a SSLSocketTemplate 
instance,

but not itself.
In fact, if replacing "print()" with "SSLSocketTemplate.print()",
AnonCipherWithWantClientAuth can work fine without extending
SSLSocketTemplate.

The original SSLSocketSample.java defines the core methods, such as
doServerSide(), runServerApplication(), doClientSide() and
runClientApplication(), as non-static. In my eyes, this style may be
better. The child class can simply override the methods if necessary.

In addition, some tests have to configure SSLServerSocket. Why not
provide one more extension point in doServerSide()? Then, it 
unnecessary

to re-write the whole doServerSide() (or, set a new server peer).
The code talks more clearly. Please take a look at my example:
http://cr.openjdk.java.net/~jjiang/8168969/example/

Best regards,
John Jiang

On 2016/11/2 8:54, Artem Smotrakov wrote:

Hello,

Please review the following patch which merges a couple of classes in
javax/net/ssl/templates.

SSLTest class contains re-usable parts of SSLSocketSample.
SSLSocketTemplate class is buggy (tests which follows it may fail
intermittently). I basically replaced SSLSocketTemplate with SSLTest,
and removed SSLSocketSample.

SSL/TLS tests should use SSLSocketTemplate class. I updated test which
use SSLTest to use SSLSocketTemplate.

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

Artem

Re: RFR 8164881: Add more tests for JDK-8139565.

[Artem Smotrakov]

Hi Mallikarjuna,

I have a couple of comments.

1. I see you extract DSA key size from "jdk.certpath.disabledAlgorithms" 
security property. I think I would be better not to rely on it, but 
expect that keys less than 1024 bits are not allowed by default. You can 
pass a boolean parameter to the test which defines what should be expected.


2. We are trying not to use lines more than 80 symbols. Could you please 
fix it?


3. Minor: DSAKeys.java line 342, you don't need to specify types for HashMap

4. It is up to you, but it would be good to update the test to use 
SSTest.java (see examples in jdk/tests) because we've been seeing 
intermittent failures of JSSE tests like this one you are updating


http://hg.openjdk.java.net/jdk9/dev/jdk/file/93fb16cbdf7f/test/javax/net/ssl/templates/SSLTest.java

Artem

On 11/13/2016 09:02 PM, Mallikarjuna Avaluri wrote:


Hi all,

Please review the fix for following issue.

JDK-8164881: Add more tests for JDK-8139565
https://bugs.openjdk.java.net/browse/JDK-8164881

*Summary:* Currently 
test/javax/net/ssl/TLSv12/DisabledShortDSAKeys.java checks only that 
DSA keys with size of 512 bits are disabled.
But we also need to check that DSA keys with sizes 1024 & 2048 are 
working fine.



*Fix: * Currently test/javax/net/ssl/TLSv12/DisabledShortDSAKeys.java 
checks only that DSA keys with size of 512 bits are disabled.
Added new tests with DSA keys with size of 960 bits disabled, 1024, 
2048, 3072 bits enabled.


*Webrev: * 
http://cr.openjdk.java.net/~bgopularam/mavaluri/JDK-8164881/webrev.00/ 




Thanks,
Mallikarjuna Avaluri 

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Artem Smotrakov]

Here is final version (I hope)

http://cr.openjdk.java.net/~asmotrak/8168882/webrev.04/

Artem


On 11/07/2016 06:50 PM, Artem Smotrakov wrote:

Hi Max,

Sure, I'll add a comment which explains why keytool resets that 
security property.


I didn't notice any strange thing happening if SSL server uses weak 
algorithms. Please see updated PrintSSL.java which now uses MD5withRSA.


Artem


On 11/07/2016 06:45 PM, Wang Weijun wrote:

Hi Artem

Change looks fine, but you can add a comment in keytool/Main on why 
you want to set that security property.


BTW, you mentioned keytool -printcert -sslserver the other time. Is 
there any strange thing happening if the SSL server is using weak 
cert/cipher?


Thanks
Max

On Nov 8, 2016, at 9:59 AM, Artem Smotrakov 
 wrote:


Sean, Max,

Please take a look at 
http://cr.openjdk.java.net/~asmotrak/8168882/webrev.03/


It doesn't print a warning anymore, and reset the security property 
only if -jarfile specified. I also updated a couple of tests to 
check if "-printcert" works fine.


Artem


On 11/03/2016 05:47 PM, Artem Smotrakov wrote:

Thank you for review Sean.

I'll remove the warning then. And I'll update it to reset the 
security property only if a jar file has been specified.


Let me also check how "-printcert -file ..." and "-printcert 
-sslserver" work.


Artem


On 11/03/2016 07:27 AM, Wang Weijun wrote:

I agree with Sean.

--Max

On Nov 3, 2016, at 10:00 PM, Sean Mullan  
wrote:


You should only unset the jdk.jar.disabledAlgorithms property if 
a jarfile has been specified.


Also, you are printing the warning message for all usages of the 
-printcert option, -ssl, etc, which is not correct.


But I don't really think the warning message is necessary. The 
docs for the -printcert option are pretty clear that it simply 
extracts the certificate and prints it. If we are going to put a 
warning in for signed JARs, then arguably we should put in a more 
general, simple warning in for all usages of this option to say 
that the certificate, etc is not verified, ex:


"WARNING: The -printcert option does not verify the certificate."

But again, I don't think this is strictly necessary.

Thanks,
Sean

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Artem Smotrakov]

Hi Max,

Sure, I'll add a comment which explains why keytool resets that security 
property.


I didn't notice any strange thing happening if SSL server uses weak 
algorithms. Please see updated PrintSSL.java which now uses MD5withRSA.


Artem


On 11/07/2016 06:45 PM, Wang Weijun wrote:

Hi Artem

Change looks fine, but you can add a comment in keytool/Main on why you want to 
set that security property.

BTW, you mentioned keytool -printcert -sslserver the other time. Is there any 
strange thing happening if the SSL server is using weak cert/cipher?

Thanks
Max


On Nov 8, 2016, at 9:59 AM, Artem Smotrakov  wrote:

Sean, Max,

Please take a look at http://cr.openjdk.java.net/~asmotrak/8168882/webrev.03/

It doesn't print a warning anymore, and reset the security property only if -jarfile 
specified. I also updated a couple of tests to check if "-printcert" works fine.

Artem


On 11/03/2016 05:47 PM, Artem Smotrakov wrote:

Thank you for review Sean.

I'll remove the warning then. And I'll update it to reset the security property 
only if a jar file has been specified.

Let me also check how "-printcert -file ..." and "-printcert -sslserver" work.

Artem


On 11/03/2016 07:27 AM, Wang Weijun wrote:

I agree with Sean.

--Max


On Nov 3, 2016, at 10:00 PM, Sean Mullan  wrote:

You should only unset the jdk.jar.disabledAlgorithms property if a jarfile has 
been specified.

Also, you are printing the warning message for all usages of the -printcert 
option, -ssl, etc, which is not correct.

But I don't really think the warning message is necessary. The docs for the 
-printcert option are pretty clear that it simply extracts the certificate and 
prints it. If we are going to put a warning in for signed JARs, then arguably 
we should put in a more general, simple warning in for all usages of this 
option to say that the certificate, etc is not verified, ex:

"WARNING: The -printcert option does not verify the certificate."

But again, I don't think this is strictly necessary.

Thanks,
Sean

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Artem Smotrakov]

Sean, Max,

Please take a look at 
http://cr.openjdk.java.net/~asmotrak/8168882/webrev.03/


It doesn't print a warning anymore, and reset the security property only 
if -jarfile specified. I also updated a couple of tests to check if 
"-printcert" works fine.


Artem


On 11/03/2016 05:47 PM, Artem Smotrakov wrote:

Thank you for review Sean.

I'll remove the warning then. And I'll update it to reset the security 
property only if a jar file has been specified.


Let me also check how "-printcert -file ..." and "-printcert 
-sslserver" work.


Artem


On 11/03/2016 07:27 AM, Wang Weijun wrote:

I agree with Sean.

--Max

On Nov 3, 2016, at 10:00 PM, Sean Mullan  
wrote:


You should only unset the jdk.jar.disabledAlgorithms property if a 
jarfile has been specified.


Also, you are printing the warning message for all usages of the 
-printcert option, -ssl, etc, which is not correct.


But I don't really think the warning message is necessary. The docs 
for the -printcert option are pretty clear that it simply extracts 
the certificate and prints it. If we are going to put a warning in 
for signed JARs, then arguably we should put in a more general, 
simple warning in for all usages of this option to say that the 
certificate, etc is not verified, ex:


"WARNING: The -printcert option does not verify the certificate."

But again, I don't think this is strictly necessary.

Thanks,
Sean

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Artem Smotrakov]

Thank you for review Sean.

I'll remove the warning then. And I'll update it to reset the security 
property only if a jar file has been specified.


Let me also check how "-printcert -file ..." and "-printcert -sslserver" 
work.


Artem


On 11/03/2016 07:27 AM, Wang Weijun wrote:

I agree with Sean.

--Max


On Nov 3, 2016, at 10:00 PM, Sean Mullan  wrote:

You should only unset the jdk.jar.disabledAlgorithms property if a jarfile has 
been specified.

Also, you are printing the warning message for all usages of the -printcert 
option, -ssl, etc, which is not correct.

But I don't really think the warning message is necessary. The docs for the 
-printcert option are pretty clear that it simply extracts the certificate and 
prints it. If we are going to put a warning in for signed JARs, then arguably 
we should put in a more general, simple warning in for all usages of this 
option to say that the certificate, etc is not verified, ex:

"WARNING: The -printcert option does not verify the certificate."

But again, I don't think this is strictly necessary.

Thanks,
Sean

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Artem Smotrakov]

My bad, I missed that.

http://cr.openjdk.java.net/~asmotrak/8168882/webrev.02/

Artem


On 11/02/2016 06:30 PM, Wang Weijun wrote:

On 11/01/2016 11:59 PM, Wang Weijun wrote:

>>Main.java:
>>
>>The warning (and the subsequent empty line) should be printed into System.err.

This one?

Re: [9] RFR: 8168969: Merge SSLSocketSample and SSLSocketTemplate

[Artem Smotrakov]

Hi John,

On 11/02/2016 05:36 PM, John Jiang wrote:
Correct, AnonCipherWithWantClientAuth can work fine without extending 
SSLSocketTemplate.


I make those classes to extend SSLSocketTemplate to make lines 
shorter (we keep lines shorter than 80 symbols).
In practice, you could divide one line to two lines if the line length 
is greater than 80.
Correct. Extending SSLSocketTemplate allows not to mention class name 
when you call a static method which makes lines shorter.




The original SSLSocketSample.java defines the core methods, such as 
doServerSide(), runServerApplication(), doClientSide() and 
runClientApplication(), as non-static. In my eyes, this style may be 
better. The child class can simply override the methods if necessary.
I think it may be better to keep them static which also means 
stateless here. It would be easier to re-use in my opinion.


In addition, some tests have to configure SSLServerSocket. Why not 
provide one more extension point in doServerSide()? Then, it 
unnecessary to re-write the whole doServerSide() (or, set a new 
server peer).
I see your point, and agree with that. I would prefer to add this 
extension when we update a test which really needs it. If you have 
such a bug, please feel free to do that.
The test AnonCipherWithWantClientAuth.java in your patch looks need 
this extension.
This update is about merging files in javax/net/ssl/template to avoid 
confusions Brad mentioned recently.


I would prefer to implement other enhancements separately. This may also 
eliminate some confusions.




We have more than one hundred SSL/TLS tests, and they may need some 
other extensions. I am not sure that it's possible to provide a 
universal SSLSocketTemplate which fits all test's needs.
I see. It's unnecessary to design too much before getting more 
concrete requirements.

Agree. Please see above.

Artem



Best regards,
John Jiang



Artem
The code talks more clearly. Please take a look at my example: 
http://cr.openjdk.java.net/~jjiang/8168969/example/


Best regards,
John Jiang

On 2016/11/2 8:54, Artem Smotrakov wrote:

Hello,

Please review the following patch which merges a couple of classes 
in javax/net/ssl/templates.


SSLTest class contains re-usable parts of SSLSocketSample. 
SSLSocketTemplate class is buggy (tests which follows it may fail 
intermittently). I basically replaced SSLSocketTemplate with 
SSLTest, and removed SSLSocketSample.


SSL/TLS tests should use SSLSocketTemplate class. I updated test 
which use SSLTest to use SSLSocketTemplate.


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

Artem

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Artem Smotrakov]

Hi Max,

Please see inline.


On 11/01/2016 11:59 PM, Wang Weijun wrote:

Main.java:

The warning (and the subsequent empty line) should be printed into System.err.

Resources.java:

"This tool accepts any algorithm" is a little confusing (sorry that I originally suggested it). 
Maybe "This tool does not attempt to verify a signed jar file, please run \"jarsigner 
-verify\" if you want to."

Sure, I'll update the message.


Also, ever since the 1st time hard coded strings are changed into dot-connected 
resource keys, newly added keys do not necessarily use the exact same string. 
Make it simple so next time if the value needs to be updated you don't need to 
change the key.
Agree, I was thinking about shorter key, but then noticed that most of 
keys look like messages. I'll make the key shorter.


Test:

- You can also add -Duser.language=en and -Duser.country=US to keytool.

Makes sense to me, will do.


- With my recent update to JarUtils.createJar(), there is no need to create the 
"test" file.
Right. I'll remove it, however creating a file would make it a bit 
clearer to me.


Thank you for review Max, please take a look at updated webrev:

http://cr.openjdk.java.net/~asmotrak/8168882/webrev.01/

By the way, I just noticed that we have another version of JarUtils.java 
which was added by Alan 7 month ago


http://hg.openjdk.java.net/jdk9/dev/jdk/log/1396fb6d0279/test/lib/testlibrary/JarUtils.java

Artem


Everything else looks fine.

Thanks
Max



On Nov 2, 2016, at 7:35 AM, Artem Smotrakov  wrote:

Hello,

Please review this small update for keytool.

"keytool -printcert -jarfile" doesn't work with jars which were signed with algorithms 
listed in "jdk.jar.disabledAlgorithms" security property.

The patch below resets "jdk.jar.disabledAlgorithms" security property before 
reading a jar file, and prints a warning.

I also re-wrote readjar.sh test, and added SecurityTools class with a couple of 
re-usable methods for jarsigner and keytool (those methods are based on methods 
from TimestampCheck.java).

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

Artem

Re: [9] RFR: 8168969: Merge SSLSocketSample and SSLSocketTemplate

[Artem Smotrakov]

Hi Xuelei,

This totally makes sense. But in my opinion we should use new features 
and APIs because:

- they are here to make code readable (okay, not to much lambdas)
- they help to avoid bugs (e.g., forgetting to close resources)
- we implicitly provide test coverage for new APIs
- we give examples how these features can be used
- finally, we encourage people to move to new Java versions

Not sure if I got correction what you mean by removing the part to 
declare the store files in each sub-classes. Do you mean getting rid of 
setting keystore/truststore in actual test files?


If so, I would prefer to do it in a separate enhancement. Most of 
SSL/TLS tests use keystores from "etc" directory. The test use 
"test.src" system property (set by jtreg) to build a path to "etc", and 
it depends on actual directory where a test stays. Also if I recall 
correctly, some tests use their own keystore. This update may be quite 
big. Would you mind if we did it separately?


8168969 is about merging helper classes to remove possible confusions 
Brad mentioned recently.


Artem

On 11/02/2016 03:57 AM, Xuelei Fan wrote:
Please use JDK 6 only features (no Lambda, try-with-resource, 
multi-catch, java.util.Objects, etc.) so as to expedite porting to 
previous releases.


It would be nice if removing the part to declare the store files in 
each sub-classes.


Xuelei

On 11/2/2016 2:48 PM, John Jiang wrote:

Hi Artem,
Thanks for making the template to be used easily.

The tests in your patch extend class SSLSocketTemplate, but
SSLSocketTemplate looks like an utility class, but not a parent class.
For example,
test/sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java
  67 new SSLSocketTemplate()
AnonCipherWithWantClientAuth still creates a SSLSocketTemplate instance,
but not itself.
In fact, if replacing "print()" with "SSLSocketTemplate.print()",
AnonCipherWithWantClientAuth can work fine without extending
SSLSocketTemplate.

The original SSLSocketSample.java defines the core methods, such as
doServerSide(), runServerApplication(), doClientSide() and
runClientApplication(), as non-static. In my eyes, this style may be
better. The child class can simply override the methods if necessary.

In addition, some tests have to configure SSLServerSocket. Why not
provide one more extension point in doServerSide()? Then, it unnecessary
to re-write the whole doServerSide() (or, set a new server peer).
The code talks more clearly. Please take a look at my example:
http://cr.openjdk.java.net/~jjiang/8168969/example/

Best regards,
John Jiang

On 2016/11/2 8:54, Artem Smotrakov wrote:

Hello,

Please review the following patch which merges a couple of classes in
javax/net/ssl/templates.

SSLTest class contains re-usable parts of SSLSocketSample.
SSLSocketTemplate class is buggy (tests which follows it may fail
intermittently). I basically replaced SSLSocketTemplate with SSLTest,
and removed SSLSocketSample.

SSL/TLS tests should use SSLSocketTemplate class. I updated test which
use SSLTest to use SSLSocketTemplate.

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

Artem

Re: [9] RFR: 8168969: Merge SSLSocketSample and SSLSocketTemplate

[Artem Smotrakov]

Hi John,

Please see inline.


On 11/01/2016 11:48 PM, John Jiang wrote:

Hi Artem,
Thanks for making the template to be used easily.

The tests in your patch extend class SSLSocketTemplate, but 
SSLSocketTemplate looks like an utility class, but not a parent class.

For example,
test/sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java
  67 new SSLSocketTemplate()
AnonCipherWithWantClientAuth still creates a SSLSocketTemplate 
instance, but not itself.
In fact, if replacing "print()" with "SSLSocketTemplate.print()", 
AnonCipherWithWantClientAuth can work fine without extending 
SSLSocketTemplate.
Correct, AnonCipherWithWantClientAuth can work fine without extending 
SSLSocketTemplate.


I make those classes to extend SSLSocketTemplate to make lines shorter 
(we keep lines shorter than 80 symbols).


The original SSLSocketSample.java defines the core methods, such as 
doServerSide(), runServerApplication(), doClientSide() and 
runClientApplication(), as non-static. In my eyes, this style may be 
better. The child class can simply override the methods if necessary.
I think it may be better to keep them static which also means stateless 
here. It would be easier to re-use in my opinion.


In addition, some tests have to configure SSLServerSocket. Why not 
provide one more extension point in doServerSide()? Then, it 
unnecessary to re-write the whole doServerSide() (or, set a new server 
peer).
I see your point, and agree with that. I would prefer to add this 
extension when we update a test which really needs it. If you have such 
a bug, please feel free to do that.


We have more than one hundred SSL/TLS tests, and they may need some 
other extensions. I am not sure that it's possible to provide a 
universal SSLSocketTemplate which fits all test's needs.


Artem
The code talks more clearly. Please take a look at my example: 
http://cr.openjdk.java.net/~jjiang/8168969/example/


Best regards,
John Jiang

On 2016/11/2 8:54, Artem Smotrakov wrote:

Hello,

Please review the following patch which merges a couple of classes in 
javax/net/ssl/templates.


SSLTest class contains re-usable parts of SSLSocketSample. 
SSLSocketTemplate class is buggy (tests which follows it may fail 
intermittently). I basically replaced SSLSocketTemplate with SSLTest, 
and removed SSLSocketSample.


SSL/TLS tests should use SSLSocketTemplate class. I updated test 
which use SSLTest to use SSLSocketTemplate.


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

Artem

[9] RFR: 8168969: Merge SSLSocketSample and SSLSocketTemplate

[Artem Smotrakov]

Hello,

Please review the following patch which merges a couple of classes in 
javax/net/ssl/templates.


SSLTest class contains re-usable parts of SSLSocketSample. 
SSLSocketTemplate class is buggy (tests which follows it may fail 
intermittently). I basically replaced SSLSocketTemplate with SSLTest, 
and removed SSLSocketSample.


SSL/TLS tests should use SSLSocketTemplate class. I updated test which 
use SSLTest to use SSLSocketTemplate.


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

Artem

[9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Artem Smotrakov]

Hello,

Please review this small update for keytool.

"keytool -printcert -jarfile" doesn't work with jars which were signed 
with algorithms listed in "jdk.jar.disabledAlgorithms" security property.


The patch below resets "jdk.jar.disabledAlgorithms" security property 
before reading a jar file, and prints a warning.


I also re-wrote readjar.sh test, and added SecurityTools class with a 
couple of re-usable methods for jarsigner and keytool (those methods are 
based on methods from TimestampCheck.java).


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

Artem

Re: Code Review Request, JDK-8167680, DTLS implementation bugs

[Artem Smotrakov]

Hi Xuelei,

Looks good to me.

Artem


On 10/27/2016 05:50 AM, Xuelei Fan wrote:

Updated to handle handle optional CertificateVerify message:

http://cr.openjdk.java.net/~xuelei/8167680/webrev.02/

Thanks,
Xuelei

On 10/21/2016 11:31 AM, Xuelei Fan wrote:

Updated webrev per Jamil's comments:

   http://cr.openjdk.java.net/~xuelei/8167680/webrev.01/

Xuelei

On 10/13/2016 10:36 PM, Xuelei Fan wrote:

Hi,

Please review the fix for JDK-8167680:
   http://cr.openjdk.java.net/~xuelei/8167680/webrev.00/

There are a few implementation bugs in JDK.

1. The sequence number is increased by 2 for GCM cipher suites.
Both GCM crypto operation and DTLS record use the sequence number.  The
current implementation may increase the sequence number for each of the
two operations.  It is not the expected behavior.

2. The implementation does not response to handshake retransmissions.
In the current implementation, receiving of retransmitted handshake
messages does not kick off a retransmission of the previous delivered
flight.  Retransmission happens on timeout.  Timeout is expensive.
Supporting response to peer retransmitted handshake messages would 
speed

up the handshaking.

3. the final CCC and finished message cannot be retransmitted.
It is an implementation bug.  Every handshake message should be able to
get retransmitted.

4. the first application data may be discarded if the last flight get
lost.
Applications may send application data immediately after the handshake
completed.  However, the peer may have not receive the handshake
complete message, and therefor discard the application data. As may
impact application logic.

For example

Client Server
   
   -- ClientKeyExchange -->
   -- ChangeCipherSpec  -->
   -- Finished  -->


   X <-- ChangeCipherSpec --
   X <-- Finished --

   <-- Application Data  --

   -...--->
   -- ClientKeyExchange -->
   -- ChangeCipherSpec  -->
   -- Finished  -->

   <-- ChangeCipherSpec --
   <-- Finished --


1. (omit the previous handshake messages) server sends ChangeCipherSpec
and Finished messages.
2. server handshake complete
3. server send application
4. client does not receive the ChangeCipherSpec or Finished messages.
5. client receives the application data.  Client cannot handle the
encrypted message, and may discard it.  Client re-transmit the previous
flight.
6. server retransmit the last flight.
7. client receives the last flight.

In this update, the last flight will be transmit twice.  As may 
mitigate

the impact of the issue.

5. resuming handshaking need no cookie exchange.
It is an implementation bug.  Cookie exchange is performed for
handshaking resuming now.  It is not the expected behavior.

6. need more debug log for DTLS handshake message fragmentation and
reassembly.


Thanks,
Xuelei

Re: Code Review Request JDK-8161106 Improve SSLSocket test template

[Artem Smotrakov]

There is SSLTest.java which follows SSLSocketSample.java and can be used 
by other tests.


Artem


On 10/26/2016 09:45 AM, Xuelei Fan wrote:

The new test case is just a test in order to make sure this approach works in 
the testing environment.  I plan to remove both of the sample and template, and 
re-org them to a class that can be inherited from.

Xuelei


On 27 Oct 2016, at 12:31 AM, Bradford Wetmore  
wrote:

Xuelei,

Sorry that I didn't have time to look at this earlier.

Why did you create a new file SSLSocketSample.java instead of just updating 
SSLSocketTemplate.java?  Why should I use one vs the other?

IMHO, unless there's a good reason to keep both, we should just copy the 
contents of SSLSocketSample.java to SSLSocketTemplate.java, and remove 
SSLSocketSample.java.

Brad




On 7/24/2016 10:22 PM, Weijun Wang wrote:



On 7/25/2016 13:14, Xuelei Fan wrote:

On 7/25/2016 12:15 PM, Weijun Wang wrote:
Is it possible to use a single new CountDownLatch(2)?


Per the spec, the countDown() release all waiting threads if the count
reaches zero, and the await() will not return until the latch has
counted down to zero, or interrupted or timeout.  It's difficult to use
one instance of CountDownLatch(2) for two conditions.

Ah, yes. I forgot about that.


Also, I think comments on lines 145-149 and 199-203 are not really
necessary, the println() lines after them are quite clear.


The comments make the logic easier to understand, I think.  Let's keep
the comments if it is not a big concern of yours.

Sure.

So everything looks fine to me.

Thanks
Max


Thanks,
Xuelei


--Max


On 7/25/2016 11:38, Xuelei Fan wrote:
Hi Weijun,

Please review this update.  Per you suggestion, I updated to use
CountDownLatch for the synchronization between client and server.
CountDownLatch is more simple than ReentrantLock in the context.

http://cr.openjdk.java.net/~xuelei/8161106/webrev.03/

Thanks,
Xuelei

Re: RFR[9] JDK-8168064: sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java failed with "Received fatal alert: handshake_failure"

[Artem Smotrakov]

Hi John,

Looks good to me, thank you for the update.

Artem


On 10/26/2016 04:45 AM, John Jiang wrote:

Hi Artem,
Please take a look at this version: 
http://cr.openjdk.java.net/~jjiang/8168064/webrev.02/

It set a new Server peer.

Best regards,
John Jiang

On 2016/10/25 1:33, Artem Smotrakov wrote:

Hi John,

I think it is too late to set parameters for server socket in 
setServerApplication() because handshaking is already done at this 
point.


You can use setServerPeer() and setClientPeer() if you need to 
configure server socket. In this case, you need to follow 
SSLSocketSample.java to implement doServerSide().


Artem


On 10/24/2016 03:24 AM, John Jiang wrote:

Hi Artem,
Thanks for your review.
Would you like take a look at the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8168064/webrev.01/
I also modified SSLTest.java a bit to expose SSLServerSocket 
instance to support the fixing.


Best regards,
John Jiang


On 2016/10/22 1:50, Artem Smotrakov wrote:

Hi John,

It may be better to use SSLTest.java to avoid duplicate code. The 
class basically contains parts of SSLSocketSample.java


http://hg.openjdk.java.net/jdk9/dev/jdk/file/0fb9ba19a63a/test/javax/net/ssl/templates/SSLTest.java 



Here is an example

http://hg.openjdk.java.net/jdk9/dev/jdk/file/0fb9ba19a63a/test/sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java 



Artem


On 10/20/2016 10:13 PM, John Jiang wrote:

Hi,
Please review this patch for fixing an intermittent issue on test 
sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java.
The fix applies the pattern from 
test/javax/net/ssl/templates/SSLSocketSample.java


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

Best regards,
John Jiang

Re: RFR[9] JDK-8168064: sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java failed with "Received fatal alert: handshake_failure"

[Artem Smotrakov]

Hi John,

I think it is too late to set parameters for server socket in 
setServerApplication() because handshaking is already done at this point.


You can use setServerPeer() and setClientPeer() if you need to configure 
server socket. In this case, you need to follow SSLSocketSample.java to 
implement doServerSide().


Artem


On 10/24/2016 03:24 AM, John Jiang wrote:

Hi Artem,
Thanks for your review.
Would you like take a look at the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8168064/webrev.01/
I also modified SSLTest.java a bit to expose SSLServerSocket instance 
to support the fixing.


Best regards,
John Jiang


On 2016/10/22 1:50, Artem Smotrakov wrote:

Hi John,

It may be better to use SSLTest.java to avoid duplicate code. The 
class basically contains parts of SSLSocketSample.java


http://hg.openjdk.java.net/jdk9/dev/jdk/file/0fb9ba19a63a/test/javax/net/ssl/templates/SSLTest.java 



Here is an example

http://hg.openjdk.java.net/jdk9/dev/jdk/file/0fb9ba19a63a/test/sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java 



Artem


On 10/20/2016 10:13 PM, John Jiang wrote:

Hi,
Please review this patch for fixing an intermittent issue on test 
sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java.
The fix applies the pattern from 
test/javax/net/ssl/templates/SSLSocketSample.java


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

Best regards,
John Jiang

Re: RFR[9] JDK-8168064: sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java failed with "Received fatal alert: handshake_failure"

[Artem Smotrakov]

Hi John,

It may be better to use SSLTest.java to avoid duplicate code. The class 
basically contains parts of SSLSocketSample.java


http://hg.openjdk.java.net/jdk9/dev/jdk/file/0fb9ba19a63a/test/javax/net/ssl/templates/SSLTest.java

Here is an example

http://hg.openjdk.java.net/jdk9/dev/jdk/file/0fb9ba19a63a/test/sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java

Artem


On 10/20/2016 10:13 PM, John Jiang wrote:

Hi,
Please review this patch for fixing an intermittent issue on test 
sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java.
The fix applies the pattern from 
test/javax/net/ssl/templates/SSLSocketSample.java


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

Best regards,
John Jiang

Re: [9] RFR: 8166530: sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java fails intermittently

[Artem Smotrakov]

Hello,

Please take a look.

Artem


On 10/07/2016 01:33 PM, Artem Smotrakov wrote:

Hello,

Please review the patch below for 
sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java test.


The test has been observed to fail intermittently, but the failure is 
not reproducible standalone. The patch updates the test to follow the 
approach from SSLSocketSample.java


http://hg.openjdk.java.net/jdk9/dev/jdk/file/1f044f413e6c/test/javax/net/ssl/templates/SSLSocketSample.java 



I merged OriginServer.java to ProxyAuthTest.java since only this test 
uses that file. I also added a couple of new common methods to 
SSLTest.java. They are not used by ProxyAuthTest.java, but can be 
useful in other tests.


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

Artem

[9] RFR: 8166530: sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java fails intermittently

[Artem Smotrakov]

Hello,

Please review the patch below for 
sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java test.


The test has been observed to fail intermittently, but the failure is 
not reproducible standalone. The patch updates the test to follow the 
approach from SSLSocketSample.java


http://hg.openjdk.java.net/jdk9/dev/jdk/file/1f044f413e6c/test/javax/net/ssl/templates/SSLSocketSample.java

I merged OriginServer.java to ProxyAuthTest.java since only this test 
uses that file. I also added a couple of new common methods to 
SSLTest.java. They are not used by ProxyAuthTest.java, but can be useful 
in other tests.


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

Artem

Re: [9] RFR: JDK-8164322: sun/security/pkcs11/PKCS11Test.java shall be updated to run on ARM platforms

[Artem Smotrakov]

Hi Xuelei,

This is not a problem with machine configuration. The actual test cases 
are not going to be run (even if there are NSS libs on a test machine) 
until "osMap" contains an entry for specific platform.


Artem


On 09/28/2016 05:52 PM, Xuelei Fan wrote:

Hi Artem,

What do you think fix the testing infrastructure (JPRT/Mach5) with 
proper configuration?  I think it is a easier than update a large 
bunch of test codes.


I understand the concerns of yours, but I don't want add additional 
effort for those who need to run the test on their own environment. 
Some people run the test locally before submit JPRT jobs (or no access 
to JPRT systems for external people).


Update the testing infrastructure may solve both of your concerns and 
my concerns.


I'm not sure the @requires tag would work or not.  It would be great 
if you can find a solution with the tag.


Thanks,
Xuelei

On 9/29/2016 4:32 AM, Artem Smotrakov wrote:

Hi Xuelei,

I understand your concerns. But I'd prefer to be aware of situations
when a test reports that it passed when it actually did nothing.

How about using @requires key? We can try to specify all expected
platforms. If I understand correctly, jtreg won't run tests if specified
requirements are not met. In this case, you need to look at the report
to make sure all tests you are interested in were run.

Artem


On 09/28/2016 08:00 AM, Xuelei Fan wrote:

On 9/28/2016 9:20 PM, Denis Kononenko wrote:

There're 60+ tests related to PKCS11. Two years they have been
"passed" on 3 unsupported platforms on hosts where usually no NSS
libraries were installed. How can we rely on these results?

;-) The words of "unsupported platforms" are very confusing in this
mail thread.

Let's think more about what if a test failed.  What one will do if a
test failed?
1. Test fail means source code problems for developers.  One cannot
submit a change-set if a test failed.  He need to pay additional
effort and analysis the failure.  It take one developer a lot effort
to know the root cause.  I would never like this unnecessary cost.
2. In order to get the test pass, he need to install the NSS libs
although NSS has nothing to do with his changeset.  It may be a very
very hard step or even impossible (for example licenses issues) step
for him.

TBH, I did not see much benefits to fail on unsupported platforms.  I
agree that skip for pass is not a good idea, but fail to warn is worse.

I think the root cause if "unsupported platforms" actually are
supported platforms, but by accident the NSS libraries are not
installed or not installed properly.

If one is not interested in NSS, the test get ignored (passed). If one
is interested in NSS, he should install the NSS libs and the test get
checked.

What do you think if fix the testing infrastructure with properly
installed NSS libs?

> The problem is the tests report they passed but actually they were
> skipped. I have no objections against skipping tests but it would
> be better to give a hint somehow how many tests were skipped and why.
Agreed.  Unfortunately, there are only two options, "pass" or "fail",
at present.  It would be nice if there is a grey area. Any idea to
make summary of skipped tests and reasons?

Xuelei

Re: [9] RFR: JDK-8164322: sun/security/pkcs11/PKCS11Test.java shall be updated to run on ARM platforms

[Artem Smotrakov]

Hi Xuelei,

I understand your concerns. But I'd prefer to be aware of situations 
when a test reports that it passed when it actually did nothing.


How about using @requires key? We can try to specify all expected 
platforms. If I understand correctly, jtreg won't run tests if specified 
requirements are not met. In this case, you need to look at the report 
to make sure all tests you are interested in were run.


Artem


On 09/28/2016 08:00 AM, Xuelei Fan wrote:

On 9/28/2016 9:20 PM, Denis Kononenko wrote:
There're 60+ tests related to PKCS11. Two years they have been 
"passed" on 3 unsupported platforms on hosts where usually no NSS 
libraries were installed. How can we rely on these results?
;-) The words of "unsupported platforms" are very confusing in this 
mail thread.


Let's think more about what if a test failed.  What one will do if a 
test failed?
1. Test fail means source code problems for developers.  One cannot 
submit a change-set if a test failed.  He need to pay additional 
effort and analysis the failure.  It take one developer a lot effort 
to know the root cause.  I would never like this unnecessary cost.
2. In order to get the test pass, he need to install the NSS libs 
although NSS has nothing to do with his changeset.  It may be a very 
very hard step or even impossible (for example licenses issues) step 
for him.


TBH, I did not see much benefits to fail on unsupported platforms.  I 
agree that skip for pass is not a good idea, but fail to warn is worse.


I think the root cause if "unsupported platforms" actually are 
supported platforms, but by accident the NSS libraries are not 
installed or not installed properly.


If one is not interested in NSS, the test get ignored (passed). If one 
is interested in NSS, he should install the NSS libs and the test get 
checked.


What do you think if fix the testing infrastructure with properly 
installed NSS libs?


> The problem is the tests report they passed but actually they were
> skipped. I have no objections against skipping tests but it would
> be better to give a hint somehow how many tests were skipped and why.
Agreed.  Unfortunately, there are only two options, "pass" or "fail", 
at present.  It would be nice if there is a grey area. Any idea to 
make summary of skipped tests and reasons?


Xuelei

Re: [9] RFR: JDK-8164322: sun/security/pkcs11/PKCS11Test.java shall be updated to run on ARM platforms

[Artem Smotrakov]

Right, thank you Max.

Currently PKCS11 tests are seen as passed on unsupported (by the tests) 
platforms, but actually nothing was tested. We'd better know about it.


Artem


On 09/27/2016 05:04 PM, Wang Weijun wrote:

On Sep 28, 2016, at 7:59 AM, Xuelei Fan  wrote:


Currently the tests silently quit which looks like they pass. This makes
people think that everything went smoothly, but actually nothing was
tested.


I did not get the idea.

I think what Artem meant is that without the platform name in osMap, the 
platform is not tested at all. If the platform has NSS libs, we won't be able 
to know whether it works.

--Max

Re: [9] RFR: JDK-8164322: sun/security/pkcs11/PKCS11Test.java shall be updated to run on ARM platforms

[Artem Smotrakov]

(cc'ing Denis who reported the bug)

I think making PKCS11 tests fail on unexpected platform would be helpful 
for people who port JDK on new platforms and run tests on them. 
Currently the tests silently quit which looks like they pass. This makes 
people think that everything went smoothly, but actually nothing was tested.


I would prefer to update PKCS11Test to report a failure in case of 
unexpected platform.


But before that it would be good to make sure that PKCS11 tests work 
fine on those generic platforms which are listed in "osMap".


Tim,

Did you run PKCS11 tests on those platforms? (it might be good to submit 
multiple runs).


One minor comment, could you please check that indentation is correct in 
lines 625-629?


Artem

On 09/27/2016 07:22 AM, Wang Weijun wrote:

Looking at the webrev, it looks we've never tested on "Linux-arm-32" and 
"Linux-aarch64-64" before and we only realized it now. This is a true problem.

On the other hand, I also agree with Xuelei's concern. If a new platform is 
added and it does not have NSS libs tests will fail.

How about this? If there is such a new platform called "mhos-arch-32", we can 
add

osMap("mhos-arch-32", new String[]{});

and, make nssLibDirs == null a failure, but nssLibDirs.length == 0 can return 
null.

Is this good?

Thanks
Max



On Sep 27, 2016, at 7:25 PM, Xuelei Fan  wrote:

I think, it is the expected behavior to ignore the test if a platform does not 
support it.  If showing failures, every testing on unsupported platform will 
fail, and additional effort MUST be paid to evaluate the root cause of the 
failure.  We should try to avoid that.

Xuelei

On 9/27/2016 6:32 PM, Tim Du wrote:

Hi All:

Would you help to review the patch for sun/security/pkcs11/PKCS11Test.java?
The test keep pass on not supported platforms, it will make nobody
notice the test was skipped,which is not our expected. Update case to
show failure, when platform not supported. And add the support for
Linux-arm-32 and Linux-arm-64 platforms. Thanks.

JBS: https://bugs.openjdk.java.net/browse/JDK-8164322
Webrev: http://cr.openjdk.java.net/~tidu/8164322/webrev.00/


Regards
Tim

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Hello,

If you don't mind, I moved some common code from SSLSocketTemplate.java 
to SSLTest.java, so that it can be re-used by other tests. It may help 
to avoid duplicate code.


http://cr.openjdk.java.net/~asmotrak/8164591/webrev.01/

Please take a look.

Artem


On 09/15/2016 11:49 AM, Artem Smotrakov wrote:

Hi Xuelei, Chris,

Thank you for looking into it. Please see inline.


On 09/15/2016 12:53 AM, Chris Hegarty wrote:

On 15 Sep 2016, at 02:55, Xuelei Fan  wrote:

On 9/15/2016 9:45 AM, Artem Smotrakov wrote:
Well, in this particular case it's not clear that it has the same 
issue
with free port (at least to me). The exception occurred on client 
side,
so it's not the case where we don't know where the handshake came 
from.


;-) Yeh, you catch the point.  But there is a free-port issue 
although the exception stack in the bug description may be not that 
case.


Let's look at a scenarios:
1. server open a server socket and listen.
2. other test case connect to the server socket.
3. this test case try to connect to server socket.
4. this test case would fail as the server only accept one connections.

I did not check it very carefully, I think for #4, the exception 
stack can be similar to the one in the bug description.

Looks like a rare, but valid case.


Anyway, as a free port is used, there are free-port issues. Please 
consider to make the enhancement in the fix. Otherwise, you cannot 
avoid the intermittent failure for this test case in the current 
testing environment.

+1.   Please remove any use of the free-port anti-pattern.
Just to be clear, this is not an issue with getting a free port with 
ServerSocket.getLocalPort() and them re-using it to create a new 
ServerSocket. It's more tricky (for example, please see the scenario 
above).


Okay, let me update it to follow the approach which is implemented in 
SSLSocketSample.java


Artem


-Chris.


Xuelei

I can make this enhancement, but like I said I don't think it's 
going to

help, so I would like to keep debug output on.

Artem


On 09/14/2016 06:39 PM, Xuelei Fan wrote:

On 9/15/2016 9:23 AM, Artem Smotrakov wrote:

Hi Xuelei,

For this one, I am not sure that it would help here since the test
failed after it already had started handshaking.

It has the same issue as a free-port is used.  We don't actually know
the handshake is coming from the right client.

Xuelei


I would prefer to have it as a separate enhancement.

Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:

As you were already there, I would suggest to consider the
SSLSocketSample.java template as the comment in JDK-8163924 review
thread.

Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:
Not urgent, but I would appreciate if someone can get a chance 
to look

at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-...@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but 
it's still
not clear why it failed because there is not much info in 
logs. The
patch updates the test to enable additional debug output, so 
that we

have more info if it fails next time.

While looking at the test, I notices a couple of issues, but 
they

don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result,
only
values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

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

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Hi Xuelei, Chris,

Thank you for looking into it. Please see inline.


On 09/15/2016 12:53 AM, Chris Hegarty wrote:

On 15 Sep 2016, at 02:55, Xuelei Fan  wrote:

On 9/15/2016 9:45 AM, Artem Smotrakov wrote:

Well, in this particular case it's not clear that it has the same issue
with free port (at least to me). The exception occurred on client side,
so it's not the case where we don't know where the handshake came from.


;-) Yeh, you catch the point.  But there is a free-port issue although the 
exception stack in the bug description may be not that case.

Let's look at a scenarios:
1. server open a server socket and listen.
2. other test case connect to the server socket.
3. this test case try to connect to server socket.
4. this test case would fail as the server only accept one connections.

I did not check it very carefully, I think for #4, the exception stack can be 
similar to the one in the bug description.

Looks like a rare, but valid case.


Anyway, as a free port is used, there are free-port issues.  Please consider to 
make the enhancement in the fix.  Otherwise, you cannot avoid the intermittent 
failure for this test case in the current testing environment.

+1.   Please remove any use of the free-port anti-pattern.
Just to be clear, this is not an issue with getting a free port with 
ServerSocket.getLocalPort() and them re-using it to create a new 
ServerSocket. It's more tricky (for example, please see the scenario above).


Okay, let me update it to follow the approach which is implemented in 
SSLSocketSample.java


Artem


-Chris.


Xuelei


I can make this enhancement, but like I said I don't think it's going to
help, so I would like to keep debug output on.

Artem


On 09/14/2016 06:39 PM, Xuelei Fan wrote:

On 9/15/2016 9:23 AM, Artem Smotrakov wrote:

Hi Xuelei,

For this one, I am not sure that it would help here since the test
failed after it already had started handshaking.

It has the same issue as a free-port is used.  We don't actually know
the handshake is coming from the right client.

Xuelei


I would prefer to have it as a separate enhancement.

Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:

As you were already there, I would suggest to consider the
SSLSocketSample.java template as the comment in JDK-8163924 review
thread.

Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:

Not urgent, but I would appreciate if someone can get a chance to look
at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-...@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but it's still
not clear why it failed because there is not much info in logs. The
patch updates the test to enable additional debug output, so that we
have more info if it fails next time.

While looking at the test, I notices a couple of issues, but they
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result,
only
values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

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

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Well, in this particular case it's not clear that it has the same issue 
with free port (at least to me). The exception occurred on client side, 
so it's not the case where we don't know where the handshake came from.


I can make this enhancement, but like I said I don't think it's going to 
help, so I would like to keep debug output on.


Artem


On 09/14/2016 06:39 PM, Xuelei Fan wrote:

On 9/15/2016 9:23 AM, Artem Smotrakov wrote:

Hi Xuelei,

For this one, I am not sure that it would help here since the test
failed after it already had started handshaking.
It has the same issue as a free-port is used.  We don't actually know 
the handshake is coming from the right client.


Xuelei


I would prefer to have it as a separate enhancement.

Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:

As you were already there, I would suggest to consider the
SSLSocketSample.java template as the comment in JDK-8163924 review
thread.

Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:

Not urgent, but I would appreciate if someone can get a chance to look
at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-...@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but it's still
not clear why it failed because there is not much info in logs. The
patch updates the test to enable additional debug output, so that we
have more info if it fails next time.

While looking at the test, I notices a couple of issues, but they
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result, 
only

values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

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

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Hi Xuelei,

For this one, I am not sure that it would help here since the test 
failed after it already had started handshaking. I would prefer to have 
it as a separate enhancement.


Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:
As you were already there, I would suggest to consider the 
SSLSocketSample.java template as the comment in JDK-8163924 review 
thread.


Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:

Not urgent, but I would appreciate if someone can get a chance to look
at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-...@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but it's still
not clear why it failed because there is not much info in logs. The
patch updates the test to enable additional debug output, so that we
have more info if it fails next time.

While looking at the test, I notices a couple of issues, but they
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result, only
values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

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

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Not urgent, but I would appreciate if someone can get a chance to look 
at this.


Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-...@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for 
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java


The test has been observed to fail a couple of times, but it's still 
not clear why it failed because there is not much info in logs. The 
patch updates the test to enable additional debug output, so that we 
have more info if it fails next time.


While looking at the test, I notices a couple of issues, but they 
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE 
provider reads them only once while initialization. As a result, only 
values which were set in the first iteration are actually used.

- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple 
cosmetic changes.


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

Artem

Re: [9] RFR: 8163924: SSLEngineBadBufferArrayAccess.java fails intermittently with Unrecognized SSL message

[Artem Smotrakov]

Hi Xuelei,

Sure, I am fine to use the approach which is implemented in 
SSLSocketSample.java. But please note that 
SSLEngineBadBufferArrayAccess.java test uses ServerSocket+SSEngine on 
server side, so I applied the approach from SSLSocketSample.java to the 
test:


http://cr.openjdk.java.net/~asmotrak/8163924/webrev.01/

If everything is okay, I am going to apply this approach to other test 
bugs we have.


I am wondering if we could move some common code from 
SSLSocketSample.java to a helper class, so that it could be re-used by 
other tests (not for all of them because tests may be different). It 
might reduce some duplicate code.


Artem


On 09/13/2016 06:39 PM, Xuelei Fan wrote:
I think the failure may caused by using free port by other test cases. 
As you were already there, would you mind update to use the 
SSLSocketSample.java template under the folder 
test/javax/net/ssl/templates?


Thanks,
Xuelei

On 9/14/2016 9:31 AM, Artem Smotrakov wrote:

Hello,

Not urgent, but I would appreciate if somebody can take a look at the
webrev below and webvev for 8164591. Thank you!

Artem

On 09/07/2016 02:58 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
SSLEngineBadBufferArrayAccess.java test.

The test has been observed to fail intermittently with "Unrecognized
SSL message" error. I couldn't reproduce it, and it's currently not
clear what caused the test to fail. One guess is that something else
might connect to the server which caused this error. The patch enables
additional debug output, so that we have more info if it fails next
time. JTREG truncates logs, but it's fine since we are interested only
in last handshaking.

The patch also updates the test to report unexpected exceptions, and
close files and sockets.

Any other suggestions are very welcome.

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

Artem

Re: [9] RFR: 8163924: SSLEngineBadBufferArrayAccess.java fails intermittently with Unrecognized SSL message

[Artem Smotrakov]

Hello,

Not urgent, but I would appreciate if somebody can take a look at the 
webrev below and webvev for 8164591. Thank you!


Artem

On 09/07/2016 02:58 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for 
SSLEngineBadBufferArrayAccess.java test.


The test has been observed to fail intermittently with "Unrecognized 
SSL message" error. I couldn't reproduce it, and it's currently not 
clear what caused the test to fail. One guess is that something else 
might connect to the server which caused this error. The patch enables 
additional debug output, so that we have more info if it fails next 
time. JTREG truncates logs, but it's fine since we are interested only 
in last handshaking.


The patch also updates the test to report unexpected exceptions, and 
close files and sockets.


Any other suggestions are very welcome.

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

Artem

Re: [9] RFR: 8156054: Test Task: Develop new tests for JEP C155: Remove FilePermission Pathname Canonicalization

[Artem Smotrakov]

Hi Siba,

I see that the test expects only true or false. You can just pass these 
boolean values, and check() can make sure that everything returns the 
expected boolean value without building a string. That would be clearer 
to me. Not an issue, it's up to you to change it or not.


Artem


On 09/13/2016 03:14 AM, Sibabrata Sahoo wrote:


Hi,

Please review the patch for,

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

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



Description:

It checks equals(), implies() and hashCode () of FilePermission when 
“jdk.io.permissionsUseCanonicalPath” set and un-set. Along with, it 
also verify compatibility with previous JDK version.


Thanks,

Siba

Re: RFR[9] JDK-8077138: Some PKCS11 tests fail because NSS library is not initialized

[Artem Smotrakov]

I am wondering if we really need to put nss-3.16-with-nspr-4.10.4.tar.gz 
in to the repository. Would it be better just to provide a link to this 
archive?


Artem

On 09/13/2016 02:43 AM, John Jiang wrote:

Hi,
Please review this patch for fixing JDK-8077138.
The solution is re-building NSS libraries with VS2013, and then the 
new NSS DLLs can depend on msvcr120.dll, which is already distributed 
with JDK 9.


And please note that, this patch also removes the PKCS11 tests from 
ProblemList.txt, though these tests have another issue JDK-8023434.
JDK-8023434 is related to Solaris, but the PKCS11 tests are marked 
with windows-all. So, I think it's no meaning to keep such items. 
Then, these tests will really be executed on Windows platforms.


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

Best regards,
John Jiang

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Sending to net-...@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for 
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java


The test has been observed to fail a couple of times, but it's still 
not clear why it failed because there is not much info in logs. The 
patch updates the test to enable additional debug output, so that we 
have more info if it fails next time.


While looking at the test, I notices a couple of issues, but they 
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE 
provider reads them only once while initialization. As a result, only 
values which were set in the first iteration are actually used.

- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple cosmetic 
changes.


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

Artem

[9] RFR: 8163924: SSLEngineBadBufferArrayAccess.java fails intermittently with Unrecognized SSL message

[Artem Smotrakov]

Hello,

Please review the following patch for SSLEngineBadBufferArrayAccess.java 
test.


The test has been observed to fail intermittently with "Unrecognized SSL 
message" error. I couldn't reproduce it, and it's currently not clear 
what caused the test to fail. One guess is that something else might 
connect to the server which caused this error. The patch enables 
additional debug output, so that we have more info if it fails next 
time. JTREG truncates logs, but it's fine since we are interested only 
in last handshaking.


The patch also updates the test to report unexpected exceptions, and 
close files and sockets.


Any other suggestions are very welcome.

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

Artem

[9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Hello,

Please review the following patch for 
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java


The test has been observed to fail a couple of times, but it's still not 
clear why it failed because there is not much info in logs. The patch 
updates the test to enable additional debug output, so that we have more 
info if it fails next time.


While looking at the test, I notices a couple of issues, but they don't 
seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE provider 
reads them only once while initialization. As a result, only values 
which were set in the first iteration are actually used.

- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple cosmetic 
changes.


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

Artem

Re: [jdk9] (S) RFR: 8165463: Native implementation of sunmscapi should use operator new (nothrow) for allocations

[Artem Smotrakov]

I think we are talking about the same "continue", but in different 
webrevs :) I meant this:


http://cr.openjdk.java.net/~igerasim/8165463/01/webrev/src/jdk.crypto.mscapi/windows/native/libsunmscapi/security.cpp.html

...

1410 if (::CertGetNameString(pCertContext,
1411 CERT_NAME_FRIENDLY_DISPLAY_TYPE, 0, NULL, 
pszNameString,

1412 cchNameString) == 1) {
1413
1414 continue; // not found
1415 }

...

Thanks!


Artem

On 09/06/2016 01:07 PM, Ivan Gerasimov wrote:

Hi Artem!

On 06.09.2016 20:45, Artem Smotrakov wrote:

Hi Ivan,

It's not really about your changes, but I am wondering if "delete [] 
pszNameString" should be called before "continue" in line 1414? Looks 
like a potential memory leak.



I assume you mean "continue" at the line 1430, not 1414?
Yes, that caught my eye too :)
Actually, this particular case seems to be benign.
If the first call to CertGetNameString() at 1413 returned > 1 then 
_most_likely_ the second call at 1426 should be Okay as well.
But I agree that this should be rewritten to make the deallocation 
logic more clear.


There seems to be another, more promising memleak in 
loadKeysOrCertificateChains(), which I'm investigating at the moment.

I think these two leaks can be fixed together.

With kind regards,
Ivan


Artem


On 09/06/2016 02:54 AM, Ivan Gerasimov wrote:

Thank you Christoph for looking into this!


On 05.09.2016 23:52, Langer, Christoph wrote:

Hi Ivan,

this looks like a good idea.

Maybe the pattern to do new (std::nothrow), then check for 0 and 
throw OOM is a good candidate for a Macro which would keep the code 
a bit more compact?


Yes, I agree that this code needs some refactoring.
Let's use the overloaded 'operator new':

http://cr.openjdk.java.net/~igerasim/8165463/01/webrev/

With kind regards,
Ivan


Best regards
Christoph


-Original Message-
From: security-dev [mailto:security-dev-boun...@openjdk.java.net] 
On Behalf

Of Ivan Gerasimov
Sent: Montag, 5. September 2016 21:53
To: security-dev@openjdk.java.net
Subject: [jdk9] (S) RFR: 8165463: Native implementation of 
sunmscapi should

use operator new (nothrow) for allocations

Hello!

In the native layer of sunmscapi provider, for memory allocations the
::operator new() is used.
In (a very unlikely) case of failure, it will raise a C++ 
exception of

type bad_alloc, which is bad, as we don't have handling code.

One simple way to improve the situation would be to use ::operator 
new

(std::nothrow), which will just return zero to indicate a failure
instead of throwing an exception.
Then we can (try to) throw a Java exception of type OutOfMemoryError.

Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8165463
WEBREV: http://cr.openjdk.java.net/~igerasim/8165463/00/webrev/

Comments/suggestions are very welcome.

With kind regards,
Ivan

Re: [jdk9] (S) RFR: 8165463: Native implementation of sunmscapi should use operator new (nothrow) for allocations

[Artem Smotrakov]

Hi Ivan,

It's not really about your changes, but I am wondering if "delete [] 
pszNameString" should be called before "continue" in line 1414? Looks 
like a potential memory leak.


Artem


On 09/06/2016 02:54 AM, Ivan Gerasimov wrote:

Thank you Christoph for looking into this!


On 05.09.2016 23:52, Langer, Christoph wrote:

Hi Ivan,

this looks like a good idea.

Maybe the pattern to do new (std::nothrow), then check for 0 and 
throw OOM is a good candidate for a Macro which would keep the code a 
bit more compact?


Yes, I agree that this code needs some refactoring.
Let's use the overloaded 'operator new':

http://cr.openjdk.java.net/~igerasim/8165463/01/webrev/

With kind regards,
Ivan


Best regards
Christoph


-Original Message-
From: security-dev [mailto:security-dev-boun...@openjdk.java.net] On 
Behalf

Of Ivan Gerasimov
Sent: Montag, 5. September 2016 21:53
To: security-dev@openjdk.java.net
Subject: [jdk9] (S) RFR: 8165463: Native implementation of sunmscapi 
should

use operator new (nothrow) for allocations

Hello!

In the native layer of sunmscapi provider, for memory allocations the
::operator new() is used.
In (a very unlikely) case of failure, it will raise a C++ exception of
type bad_alloc, which is bad, as we don't have handling code.

One simple way to improve the situation would be to use ::operator new
(std::nothrow), which will just return zero to indicate a failure
instead of throwing an exception.
Then we can (try to) throw a Java exception of type OutOfMemoryError.

Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8165463
WEBREV: http://cr.openjdk.java.net/~igerasim/8165463/00/webrev/

Comments/suggestions are very welcome.

With kind regards,
Ivan

Re: RFR 8157404: Unable to read certain PKCS12 keystores from SequenceInputStream

[Artem Smotrakov]

Hi Svetlana,

It looks fine, but I am not an official reviewer.

"keystorePath" in readTest() can be a static field.

I also meant that one test with SequenceInputStream seems to be enough, 
so you could just add a new test case to ReadP12Test.java. But it's fine.


I am not sure how DerIndefLenConverter works, but it looks a little 
strange to me that it needs to extend an array before passing it to 
DerIndefLenConverter. I see that convert() method also uses arraycopy() 
method. But it seems to be out of scope here.


Artem


On 08/29/2016 11:23 AM, Svetlana Nikandrova wrote:

Hi Artem,

thank you for your replay. I've updated copyright and made separate 
test for this bug.
As for Arrays.copyOfRange() unfortunately it won't simplify code in my 
case. I need to extend an array, not to get a sub-array of existing one.


http://cr.openjdk.java.net/~snikandrova/8157404/webrev.01/ 
<http://cr.openjdk.java.net/%7Esnikandrova/8157404/webrev.01/>


Thanks,
Svetlana

On 26.08.2016 23:48, Artem Smotrakov wrote:


Hi Svetlana,

DerValue class may be implicitly used in different areas (x509, 
SSL/TLS, keystores, maybe krb5, etc). Please make sure that tests 
from jdk_security pass.


I'll leave the main review to someone who is more knowledgeable in 
this area, here are a couple of comments:

- Please update copyright year
- You may want to replace new byte[] + System.arraycopy() by 
Arrays.copyOfRange()
- It may be better to add a separate test case in ReadP12Test.java 
for SequenceInputStream instead of loading a keystore twice in each 
call to readTest(). One test with SequenceInputStream seems to be 
enough, and it would make the logic of readTest() clearer.


Artem

On 08/26/2016 10:58 AM, Svetlana Nikandrova wrote:

Hello,

please review this fix. It's not possible to read PKCS12 keystore 
with big undefined length DER value in it from SequenceInputStream.  
Root cause of the problem is that sun.security.util.DerValue relays 
on InputStream.available() to get a complete 'indefinite.length' 
section length and then read it, but for SequenceInputStream this 
method returns number of available bytes only for current input 
stream, not the whole sequence. Fixed to read all available data.


JBS:
https://bugs.openjdk.java.net/browse/JDK-8157404
Webrev:
http://cr.openjdk.java.net/~snikandrova/8157404/webrev.00/ 
<http://cr.openjdk.java.net/%7Esnikandrova/8157404/webrev.00/>


Thanks,
Svetlana

Re: RFR 8157404: Unable to read certain PKCS12 keystores from SequenceInputStream

[Artem Smotrakov]

Hi Svetlana,

DerValue class may be implicitly used in different areas (x509, SSL/TLS, 
keystores, maybe krb5, etc). Please make sure that tests from 
jdk_security pass.


I'll leave the main review to someone who is more knowledgeable in this 
area, here are a couple of comments:

- Please update copyright year
- You may want to replace new byte[] + System.arraycopy() by 
Arrays.copyOfRange()
- It may be better to add a separate test case in ReadP12Test.java for 
SequenceInputStream instead of loading a keystore twice in each call to 
readTest(). One test with SequenceInputStream seems to be enough, and it 
would make the logic of readTest() clearer.


Artem

On 08/26/2016 10:58 AM, Svetlana Nikandrova wrote:

Hello,

please review this fix. It's not possible to read PKCS12 keystore with 
big undefined length DER value in it from SequenceInputStream.  Root 
cause of the problem is that sun.security.util.DerValue relays on 
InputStream.available() to get a complete 'indefinite.length' section 
length and then read it, but for SequenceInputStream this method 
returns number of available bytes only for current input stream, not 
the whole sequence. Fixed to read all available data.


JBS:
https://bugs.openjdk.java.net/browse/JDK-8157404
Webrev:
http://cr.openjdk.java.net/~snikandrova/8157404/webrev.00/ 



Thanks,
Svetlana

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Artem Smotrakov]

Sure Xuelei, I read 
https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html#InstallationAndCustomization


If I understand correctly, the most settings can be done via 
system/security properties. But there may be some other ways like 
setDefaultHostnameVerifier(), setDefaultSSLSocketFactory() methods.


My point is that it's better to run tests which don't modify the default 
JSSE configuration in agent VM. It would speed up test execution. I 
don't think that it's too hard to identify those tests which modify the 
default JSSE configuration. and run them in a separate JVM. I believe 
most of test are fine now.


I rely on your expertise, and I'll let you decide about it.

Artem

On 08/25/2016 10:45 AM, Xuelei Fan wrote:

On Aug 26, 2016, at 1:33 AM, Artem Smotrakov  wrote:

Hi Xuelei,



On 08/25/2016 10:26 AM, Xuelei Fan wrote:


On Aug 26, 2016, at 12:43 AM, Artem Smotrakov  
wrote:

Hi Xuelei,

I am not sure why you think it's too strong to follow to it. To me it's pretty 
easy to see if a test calls System.setProperty() or Security.setProperty() 
methods which modify default JSSE configuration.

Is there any other way to modify default JSSE configuration, so that it can't 
be changes then?

Yes.

Could you give an example?

I will give some tomorrow.  If you want to make a research by yourself, please 
read the jsse reference guides.



And even it can only be set via explicit call to setProperty(), run it in agent 
vm is still not safe because we don't know where the set may be used other than 
this test case.

Those tests which modify default JSSE configuration should be run in othervm 
mode. This way they won't affect any other tests.

"Should" is an assumption,but not the reality.  And we do not always make it 
right even we want that.


  It is not only about this test, the impact need to consider all JDK test 
cases in a big picture, which is a huge number and we cannot actually evaluate 
so many test cases.

I think we just need to identify all tests that modify default JSSE 
configuration, and make them run in othervm mode. I believe that currently most 
of them run in othervm mode.


To be safe, all is needed, most is not enough.  As I said, too much test case 
to make the evaluation and make all right.   Reality is reality.  I will try to 
find some out of the jsse components tomorrow. If you want to do by yourself 
you can search https or ocsp or ldaps on all JDK test, it may be able to find 
some hints.

Xuelei


Artem

I will reply more details when I have a PC tomorrow.

Xuelei


Artem



On 08/25/2016 12:43 AM, Xuelei Fan wrote:
On 8/25/2016 2:06 PM, Weijun Wang wrote:
I think Artem is correct here.

All those tests that modify static fields should run in othervm mode, so
that *each* runs in its *own* VM.  All other tests, which make no such
modifications, can share the *same* VM using agentvm.

It's too strong to follow.  And it is not easy to know there is no static 
fields update in a test case.

Xuelei


On 8/25/2016 13:31, Xuelei Fan wrote:
On 8/25/2016 12:31 PM, Artem Smotrakov wrote:
Those tests which modify default JSSE parameters should be run in
othervm mode. But other tests which don't do that can be safely run in
agent VM. This test doesn't seem to modify any default JSSE parameter.

If this test get booted, other test that need to do more customization
may not work any more.

For example, boolean variable A is a singleton static field, default is
true.  If A get initialized in default mode, it is "true".  It is not
possible to customize it to "false" any more in other test in
agentvm/samevm mode.

Shouldn't these "other" tests run in othervm mode?

Or, do you mean that even if a JSSE test does not modify a static field
inside the test, it will modify some through innocent JSSE calls, so no
JSSE test should ever run in agentvm?

I find this unnatural.

Thanks
Max


This test has no impact in agentvm/samevm mode unless no test run in
agentvm/samevm mode or all tests run in agentvm/samevm mode has the same
configuration as this one.   This assumption is too strong to follow.

Xuelei


Artem

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Artem Smotrakov]

Hi Xuelei,


On 08/25/2016 10:26 AM, Xuelei Fan wrote:



On Aug 26, 2016, at 12:43 AM, Artem Smotrakov  
wrote:

Hi Xuelei,

I am not sure why you think it's too strong to follow to it. To me it's pretty 
easy to see if a test calls System.setProperty() or Security.setProperty() 
methods which modify default JSSE configuration.

Is there any other way to modify default JSSE configuration, so that it can't 
be changes then?


Yes.

Could you give an example?

And even it can only be set via explicit call to setProperty(), run it in agent 
vm is still not safe because we don't know where the set may be used other than 
this test case.
Those tests which modify default JSSE configuration should be run in 
othervm mode. This way they won't affect any other tests.

  It is not only about this test, the impact need to consider all JDK test 
cases in a big picture, which is a huge number and we cannot actually evaluate 
so many test cases.
I think we just need to identify all tests that modify default JSSE 
configuration, and make them run in othervm mode. I believe that 
currently most of them run in othervm mode.


Artem

I will reply more details when I have a PC tomorrow.

Xuelei


Artem



On 08/25/2016 12:43 AM, Xuelei Fan wrote:

On 8/25/2016 2:06 PM, Weijun Wang wrote:
I think Artem is correct here.

All those tests that modify static fields should run in othervm mode, so
that *each* runs in its *own* VM.  All other tests, which make no such
modifications, can share the *same* VM using agentvm.

It's too strong to follow.  And it is not easy to know there is no static 
fields update in a test case.

Xuelei


On 8/25/2016 13:31, Xuelei Fan wrote:

On 8/25/2016 12:31 PM, Artem Smotrakov wrote:
Those tests which modify default JSSE parameters should be run in
othervm mode. But other tests which don't do that can be safely run in
agent VM. This test doesn't seem to modify any default JSSE parameter.

If this test get booted, other test that need to do more customization
may not work any more.

For example, boolean variable A is a singleton static field, default is
true.  If A get initialized in default mode, it is "true".  It is not
possible to customize it to "false" any more in other test in
agentvm/samevm mode.

Shouldn't these "other" tests run in othervm mode?

Or, do you mean that even if a JSSE test does not modify a static field
inside the test, it will modify some through innocent JSSE calls, so no
JSSE test should ever run in agentvm?

I find this unnatural.

Thanks
Max


This test has no impact in agentvm/samevm mode unless no test run in
agentvm/samevm mode or all tests run in agentvm/samevm mode has the same
configuration as this one.   This assumption is too strong to follow.

Xuelei


Artem

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Artem Smotrakov]

Hi Xuelei,

I am not sure why you think it's too strong to follow to it. To me it's 
pretty easy to see if a test calls System.setProperty() or 
Security.setProperty() methods which modify default JSSE configuration.


Is there any other way to modify default JSSE configuration, so that it 
can't be changes then?


Artem


On 08/25/2016 12:43 AM, Xuelei Fan wrote:

On 8/25/2016 2:06 PM, Weijun Wang wrote:

I think Artem is correct here.

All those tests that modify static fields should run in othervm mode, so
that *each* runs in its *own* VM.  All other tests, which make no such
modifications, can share the *same* VM using agentvm.

It's too strong to follow.  And it is not easy to know there is no 
static fields update in a test case.


Xuelei


On 8/25/2016 13:31, Xuelei Fan wrote:

On 8/25/2016 12:31 PM, Artem Smotrakov wrote:

Those tests which modify default JSSE parameters should be run in
othervm mode. But other tests which don't do that can be safely run in
agent VM. This test doesn't seem to modify any default JSSE parameter.


If this test get booted, other test that need to do more customization
may not work any more.

For example, boolean variable A is a singleton static field, default is
true.  If A get initialized in default mode, it is "true".  It is not
possible to customize it to "false" any more in other test in
agentvm/samevm mode.


Shouldn't these "other" tests run in othervm mode?

Or, do you mean that even if a JSSE test does not modify a static field
inside the test, it will modify some through innocent JSSE calls, so no
JSSE test should ever run in agentvm?

I find this unnatural.

Thanks
Max



This test has no impact in agentvm/samevm mode unless no test run in
agentvm/samevm mode or all tests run in agentvm/samevm mode has the 
same

configuration as this one.   This assumption is too strong to follow.

Xuelei


Artem

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Artem Smotrakov]

Those tests which modify default JSSE parameters should be run in othervm mode. 
But other tests which don't do that can be safely run in agent VM. This test 
doesn't seem to modify any default JSSE parameter.

Artem


- Original Message -
From: xuelei@oracle.com
To: artem.smotra...@oracle.com
Cc: security-dev@openjdk.java.net, svetlana.nikandr...@oracle.com
Sent: Wednesday, August 24, 2016 8:20:45 PM GMT -08:00 US/Canada Pacific
Subject: Re: RFR 8164533: [TEST_BUG] 
sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while 
cleaning up threads after test"



On 8/25/2016 11:17 AM, Artem Smotrakov wrote:
> Hi Xuelei,
>
> Yes, I know that JSSE provider initializes only once. But I suppose that 
> tests which use default JSSE configuration (like this test) can be safely run 
> in agent VM. Am I missing something?
>
The default one can be customized.  For example, the protocols, cipher 
suites, etc.  So it is not safe to run in agentvm/samevm mode.

Xuelei

> Artem
>
> - Original Message -
> From: xuelei@oracle.com
> To: artem.smotra...@oracle.com, security-dev@openjdk.java.net
> Cc: svetlana.nikandr...@oracle.com
> Sent: Wednesday, August 24, 2016 7:21:55 PM GMT -08:00 US/Canada Pacific
> Subject: Re: RFR 8164533: [TEST_BUG] 
> sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while 
> cleaning up threads after test"
>
> On 8/25/2016 9:27 AM, Artem Smotrakov wrote:
>>> BTW, please run the test in othervm mode.
>> Why do you think it's necessary here? I don't see the test modifies
>> anything that may affect other tests running in the same JVM (for
>> example, system properties). Am I missing something?
> It's not about the test code, but about the JSSE impl.  The JSSE impl
> uses a lot of singleton static fields for performance.  Once these
> fields are initialized, they may impact other test cases.
>
> In JSSE test cases, we always run in othervm mode, and put a comment
> like (See test/javax/net/ssl/templates/SSLSocketTemplate.java):
>
> // SunJSSE does not support dynamic system properties, no way to re-use
> // system properties in samevm/agentvm mode.
>
>
> Xuelei
>

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Artem Smotrakov]

Hi Xuelei,

Yes, I know that JSSE provider initializes only once. But I suppose that tests 
which use default JSSE configuration (like this test) can be safely run in 
agent VM. Am I missing something?

Artem

- Original Message -
From: xuelei@oracle.com
To: artem.smotra...@oracle.com, security-dev@openjdk.java.net
Cc: svetlana.nikandr...@oracle.com
Sent: Wednesday, August 24, 2016 7:21:55 PM GMT -08:00 US/Canada Pacific
Subject: Re: RFR 8164533: [TEST_BUG] 
sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while 
cleaning up threads after test"

On 8/25/2016 9:27 AM, Artem Smotrakov wrote:
>> BTW, please run the test in othervm mode.
> Why do you think it's necessary here? I don't see the test modifies
> anything that may affect other tests running in the same JVM (for
> example, system properties). Am I missing something?
It's not about the test code, but about the JSSE impl.  The JSSE impl 
uses a lot of singleton static fields for performance.  Once these 
fields are initialized, they may impact other test cases.

In JSSE test cases, we always run in othervm mode, and put a comment 
like (See test/javax/net/ssl/templates/SSLSocketTemplate.java):

// SunJSSE does not support dynamic system properties, no way to re-use
// system properties in samevm/agentvm mode.


Xuelei

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Artem Smotrakov]

Hi Xuelei,

Please see inline.

On 08/24/2016 06:10 PM, Xuelei Fan wrote:



On 8/25/2016 3:55 AM, Artem Smotrakov wrote:

Hi Svetlana,

Thank you for cleaning up this test. I have a couple of comments (mostly
about the original test).

1. I see that the test tries to connect to a server three times, but the
server accept only first connection, and then it stops. So test cases
#2-3 fail just because the connection was refused. The original test
behaves like this. This looks like a bug to me. What do you think?
Should the server have a loop of three iterations?


I think it's the purpose to test behavior of the close server socket.
Okay, makes sense to me. I am not sure what the author meant, but I 
thought the purpose is to check if an exception is thrown when server 
accepts all connections, but then close them right away.



2. Here is server's code:

  95 @Override
  96 public void run() {
  97 try (Socket s = serverSocket.accept()) {
  98 System.out.println("Server accepted connection");
  99 // wait a bit before closing the socket to give
 100 // the client time to send its hello message
 101 Thread.currentThread().sleep(100);
 102 s.close();
 103 System.out.println("Server closed socket, done.");
 104 } catch (Exception e) {
 105 throw new RuntimeException("Problem in test
execution", e);
 106 }
 107 }

Not sure if it is a good assumption to expect that ClientHello is
received in 100 milliseconds. It might read first data, and then close
the socket. It also doesn't seem to be necessary to call close() there.

It is not expected to perform handshaking for this test.  Get 
connection, and immediately close the socket before handshaking, and 
then see what happens in client side (connect/read/write).


I have the same concern that 100 MS may be an issue.  Please consider 
to make an improvement.


BTW, please run the test in othervm mode.
Why do you think it's necessary here? I don't see the test modifies 
anything that may affect other tests running in the same JVM (for 
example, system properties). Am I missing something?


Artem


Xuelei


Otherwise, the webrev looks good to me, but please note that I am not an
official reviewer. You may want to fix the issues above, or we can just
file a new bug.

Artem

On 08/24/2016 11:21 AM, Svetlana Nikandrova wrote:

Hello,

please review this test bug fix. Test failed because of staled threads
left after execution.
Added try-with-resources statements to make sure test closes it's
resources. Also as test is overall quite old-fashioned I've done some
refactoring (hope now it looks better).

JBS:
https://bugs.openjdk.java.net/browse/JDK-8164533
Webrev:
http://cr.openjdk.java.net/~snikandrova/8164533/webrev.00/
<http://cr.openjdk.java.net/%7Esnikandrova/8164533/webrev.00/>

Thank you,
Svetlana

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Artem Smotrakov]

Hi Svetlana,

Thank you for cleaning up this test. I have a couple of comments (mostly 
about the original test).


1. I see that the test tries to connect to a server three times, but the 
server accept only first connection, and then it stops. So test cases 
#2-3 fail just because the connection was refused. The original test 
behaves like this. This looks like a bug to me. What do you think? 
Should the server have a loop of three iterations?


2. Here is server's code:

  95 @Override
  96 public void run() {
  97 try (Socket s = serverSocket.accept()) {
  98 System.out.println("Server accepted connection");
  99 // wait a bit before closing the socket to give
 100 // the client time to send its hello message
 101 Thread.currentThread().sleep(100);
 102 s.close();
 103 System.out.println("Server closed socket, done.");
 104 } catch (Exception e) {
 105 throw new RuntimeException("Problem in test 
execution", e);

 106 }
 107 }

Not sure if it is a good assumption to expect that ClientHello is 
received in 100 milliseconds. It might read first data, and then close 
the socket. It also doesn't seem to be necessary to call close() there.


Otherwise, the webrev looks good to me, but please note that I am not an 
official reviewer. You may want to fix the issues above, or we can just 
file a new bug.


Artem

On 08/24/2016 11:21 AM, Svetlana Nikandrova wrote:

Hello,

please review this test bug fix. Test failed because of staled threads 
left after execution.
Added try-with-resources statements to make sure test closes it's 
resources. Also as test is overall quite old-fashioned I've done some 
refactoring (hope now it looks better).


JBS:
https://bugs.openjdk.java.net/browse/JDK-8164533
Webrev:
http://cr.openjdk.java.net/~snikandrova/8164533/webrev.00/ 



Thank you,
Svetlana

Re: RFR: (XS) 8162916:Test sun/security/krb5/auto/UnboundSSL.java fails

[Artem Smotrakov]

Hi Sean,

The patch below looks fine to me, but I am not an official reviewer.

Artem


On 08/18/2016 08:28 AM, Seán Coffey wrote:
Thanks for the tip Artem, Max. No need to modify the policy file then. 
Below is the new suggested patch for jdk8u-dev. JPRT results are good.


diff --git a/test/sun/security/krb5/auto/UnboundSSL.java 
b/test/sun/security/krb5/auto/UnboundSSL.java

--- a/test/sun/security/krb5/auto/UnboundSSL.java
+++ b/test/sun/security/krb5/auto/UnboundSSL.java
@@ -34,9 +34,9 @@
  * @bug 8025123
  * @summary Checks if an unbound server can handle connections
  *  only for allowed service principals
- * @run main/othervm/policy=unbound.ssl.policy UnboundSSL
+ * @run main/othervm/java.security.policy=unbound.ssl.policy UnboundSSL
  *  unbound.ssl.jaas.conf server_star
- * @run main/othervm/policy=unbound.ssl.policy UnboundSSL
+ * @run main/othervm/java.security.policy=unbound.ssl.policy UnboundSSL
  *  unbound.ssl.jaas.conf 
server_multiple_principals

  */
 public class UnboundSSL {

Regards,
Sean.

On 18/08/2016 04:11, Weijun Wang wrote:

If I recall correctly, there should be a way to specify a policy file
in @run without overriding the default one. May be it is "@run
main/othervm/java.security.policy=unbound.ssl.policy_new"


Yes, I think this should work. I've also just learned about it and 
don't know from which jtreg it is supported. Hopefully the 
minimized-required version of jtreg for jdk8u already has it.


--Max

[9] RFR: 8164100: com/sun/crypto/provider/KeyFactory/TestProviderLeak.java fails with java.util.concurrent.TimeoutException

[Artem Smotrakov]

Hello,

Please review the following patch for 
com/sun/crypto/provider/KeyFactory/TestProviderLeak.java test.


This is a request to make the test take into account a test timeout 
factor. Timeout factor can be specified with "-timeout" jtreg's command 
line option. This option is used in some test runs to make test runs 
more stable (for example, VM SQE uses it while running regression tests 
with different JVM options).


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

Artem

Re: RFR: (XS) 8162916:Test sun/security/krb5/auto/UnboundSSL.java fails

[Artem Smotrakov]

Sorry, my bad, I didn't notice '9-na' label.

I suppose that code from ext directory should have all permissions:

artem@artem-laptop:$ cat ~/jdk/jdk1.8.0_92b14/jre/lib/security/java.policy

// Standard extensions get all permissions by default

grant codeBase "file:${{java.ext.dirs}}/*" {
permission java.security.AllPermission;
};

// default permissions granted to all domains
...

I am wondering if it would be better it the test didn't override the 
default policy.


Artem

On 08/17/2016 10:12 AM, Seán Coffey wrote:

Hi Artem,

Sorry - should have said that this is for jdk8u-dev. The bug is marked 
9-na. The provider loading changes made in this area for 9 mean that 
it's not affected.


Regards,
Sean.

On 17/08/16 18:10, Artem Smotrakov wrote:

Hi Sean,

If I remember correctly, there is no ext directory in JDK 9 any more.

I don't see in jtr file that "java.ext.dirs" system property is 
passed to the test. If I understand correctly, 
"file:${{java.ext.dirs}}/*" becomes "file:/*" which seems to grand 
all permissions to all the code. It doesn't look correct for this test.


It looks like the test overrides the default policy, please see in 
jtr file


-Djava.security.policy==/export/home/gtee/scripts/Results/workDir/scratch_2/unbound.ssl.policy_new 
\\


If I recall correctly, there should be a way to specify a policy file 
in @run without overriding the default one. May be it is "@run 
main/othervm/java.security.policy=unbound.ssl.policy_new"


Artem


On 08/17/2016 09:53 AM, Seán Coffey wrote:
A recently added test case lacks sufficient permissions to read a 
conf file when running with security manager.


bug report : https://bugs.openjdk.java.net/browse/JDK-8162916

proposed patch :
 diff --git a/test/sun/security/krb5/auto/unbound.ssl.policy 
b/test/sun/security/krb5/auto/unbound.ssl.policy

--- a/test/sun/security/krb5/auto/unbound.ssl.policy
+++ b/test/sun/security/krb5/auto/unbound.ssl.policy
@@ -1,7 +1,13 @@
+// Standard extensions get all permissions by default
+
+grant codeBase "file:${{java.ext.dirs}}/*" {
+permission java.security.AllPermission;
+};
+
 grant {
 permission java.util.PropertyPermission "*", "read,write";
 permission java.net.SocketPermission "*:*", 
"listen,resolve,accept,connect";

-permission java.io.FilePermission "*", "read,write,delete";
+permission java.io.FilePermission "<>", 
"read,write,delete";

 permission java.lang.RuntimePermission "accessDeclaredMembers";
 permission java.lang.reflect.ReflectPermission 
"suppressAccessChecks";

 permission java.lang.RuntimePermission "accessClassInPackage.*";

Re: RFR: (XS) 8162916:Test sun/security/krb5/auto/UnboundSSL.java fails

[Artem Smotrakov]

Hi Sean,

If I remember correctly, there is no ext directory in JDK 9 any more.

I don't see in jtr file that "java.ext.dirs" system property is passed 
to the test. If I understand correctly, "file:${{java.ext.dirs}}/*" 
becomes "file:/*" which seems to grand all permissions to all the code. 
It doesn't look correct for this test.


It looks like the test overrides the default policy, please see in jtr file

-Djava.security.policy==/export/home/gtee/scripts/Results/workDir/scratch_2/unbound.ssl.policy_new 
\\


If I recall correctly, there should be a way to specify a policy file in 
@run without overriding the default one. May be it is "@run 
main/othervm/java.security.policy=unbound.ssl.policy_new"


Artem


On 08/17/2016 09:53 AM, Seán Coffey wrote:
A recently added test case lacks sufficient permissions to read a conf 
file when running with security manager.


bug report : https://bugs.openjdk.java.net/browse/JDK-8162916

proposed patch :
 diff --git a/test/sun/security/krb5/auto/unbound.ssl.policy 
b/test/sun/security/krb5/auto/unbound.ssl.policy

--- a/test/sun/security/krb5/auto/unbound.ssl.policy
+++ b/test/sun/security/krb5/auto/unbound.ssl.policy
@@ -1,7 +1,13 @@
+// Standard extensions get all permissions by default
+
+grant codeBase "file:${{java.ext.dirs}}/*" {
+permission java.security.AllPermission;
+};
+
 grant {
 permission java.util.PropertyPermission "*", "read,write";
 permission java.net.SocketPermission "*:*", 
"listen,resolve,accept,connect";

-permission java.io.FilePermission "*", "read,write,delete";
+permission java.io.FilePermission "<>", 
"read,write,delete";

 permission java.lang.RuntimePermission "accessDeclaredMembers";
 permission java.lang.reflect.ReflectPermission 
"suppressAccessChecks";

 permission java.lang.RuntimePermission "accessClassInPackage.*";

Re: [9] RFR: 8162484: javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails intermittently with "Address already in use" error

[Artem Smotrakov]

Hi Xuelei,

Makes sense to me, I removed those 1 second delays

http://cr.openjdk.java.net/~asmotrak/8162484/webrev.03/

Artem


On 08/12/2016 06:17 PM, Xuelei Fan wrote:

It's a nice find of the port reuse issues.

This update will turn into expected connection failure into
reading/writing interruption as the server simulate the failure by
closing the incoming connections.  It's fine for this test,  I think.

For lines like:
  288 intOcsp.rejectConnections();
  289 rootOcsp.rejectConnections();
  290 Thread.sleep(1000);

I was wondering as the server does not need to bootup again, is the
delay still needed?

Otherwise, looks fine to me.

Thanks,
Xuelei

On 8/13/2016 6:25 AM, Artem Smotrakov wrote:

Thank you for review Jamil.

Xuelei,

Could you please take a look?

Artem


On 08/12/2016 02:38 PM, Jamil Nimeh wrote:

Thank you Artem.  The fix looks good.  You just need a +1 from an
official reviewer.



--Jamil

 Original message ----
From: Artem Smotrakov 
Date: 8/12/16 1:07 PM (GMT-08:00)
To: Jamil Nimeh , Security Dev OpenJDK

Subject: Re: [9] RFR: 8162484:
javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails
intermittently with "Address already in use" error

No problem.

http://cr.openjdk.java.net/~asmotrak/8162484/webrev.02/

Artem


On 08/12/2016 12:02 PM, Jamil Nimeh wrote:

For the tests as we use them today we don't intend the server to
restart.  The intent of SimpleOCSPServer was to be of use for a
variety of testing purposes.  I don't know that we can say for all
intended uses that we'll *never* need to restart it. That's why I'd
like to keep the unbound socket/set sockopt/bind/listen behavior.  I
don't think ServerSocket(0) achieves that.

--Jamil

On 8/12/2016 11:30 AM, Artem Smotrakov wrote:

Hi Jamil,

There was no any specific reason to remove ServerSocket.bind() call.
ServerSocket(0) constructor creates a server socket, automatically
bound to a random free port. If I am not missing something, it
doesn't look necessary to set the SO_REUSEADDR socket options if the
server is not going to restart. The code is just shorter if we use
ServerSocket(0) constructor to open a server socket, but I can revert
it to use bind() with 0 port number if you think it's better.

Artem


On 08/12/2016 09:13 AM, Jamil Nimeh wrote:

Hi Artem, more comments in-line


On 8/11/2016 11:46 AM, Artem Smotrakov wrote:

Hi Jamil,

Thank you for review. Please see inline.


On 08/10/2016 04:16 PM, Jamil Nimeh wrote:

Hi Artem,

I'm not an official reviewer but the solution for making the
servers reject connections rather than stop and start looks pretty
fair to me and seems like a nice way to simulate a downed OCSP
responder instead of having to bounce it.  A couple
comments/questions:

I'm a bit surprised that you get the "Address already in use"
error though.

Well, to be honest, I was not able to reproduce this failure
locally. I was running the test in a loop for a couple of days, and
it didn't fail. But the test has been observed to fail in other
test runs (jprt, CI, etc).

I am not an expert in networking, and I would appreciate if someone
more knowledgeable gives an advise how these intermittent failures
can be avoided.


Isn't servSocket.setReuseAddress(true) on line 214 supposed to set
the SO_REUSEADDR at the system call level and prevent EADDRINUSE
when listening or binding?

If I am not missing something, the test has been observed to fails
while re-binding. I am wondering if it's possible that the port
becomes busy after the server socket was closed, but before bind()
is called again. The probability of this situation seems to be very
low which has been actually seen - the test fails very rare.

If this is the case, it seems like servSocket.setReuseAddress(true)
doesn't help because the port is taken by another process (I am not
sure that SO_REUSEADDR prevents from this). Again, this is only my
guess, and I may be wrong.

You know, I hadn't thought of that.  I've never been able to
reproduce that problem either, but I'm running on a local virtual
box VM on a laptop, and usually the tests are running sequentially.
I could definitely see the case where other processes are soaking up
the OCSP responder's port.  With those tests, I kind of need the
port to remain the same since I'm putting that server and port in
the AIA extensions of the certs for which it answers.  Given this
particular case, it seems like your solution of keeping the server
bound but just chopping connections off is the best way to go.

When you create the new ServerSocket on line 212, you're now
binding it to the port now where originally it started as an
unbound socket.  By doing so, the behavior of setReuseAddress() on
line 214 is now undefined.

This setReuseAddress() call looks unnecessary now. I'll update the
test.

While this test no longer stops/starts the

Re: [9] RFR: 8162484: javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails intermittently with "Address already in use" error

[Artem Smotrakov]

Thank you for review Jamil.

Xuelei,

Could you please take a look?

Artem


On 08/12/2016 02:38 PM, Jamil Nimeh wrote:
Thank you Artem.  The fix looks good.  You just need a +1 from an 
official reviewer.




--Jamil

 Original message 
From: Artem Smotrakov 
Date: 8/12/16 1:07 PM (GMT-08:00)
To: Jamil Nimeh , Security Dev OpenJDK 

Subject: Re: [9] RFR: 8162484: 
javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails 
intermittently with "Address already in use" error


No problem.

http://cr.openjdk.java.net/~asmotrak/8162484/webrev.02/

Artem


On 08/12/2016 12:02 PM, Jamil Nimeh wrote:
> For the tests as we use them today we don't intend the server to
> restart.  The intent of SimpleOCSPServer was to be of use for a
> variety of testing purposes.  I don't know that we can say for all
> intended uses that we'll *never* need to restart it. That's why I'd
> like to keep the unbound socket/set sockopt/bind/listen behavior.  I
> don't think ServerSocket(0) achieves that.
>
> --Jamil
>
> On 8/12/2016 11:30 AM, Artem Smotrakov wrote:
>> Hi Jamil,
>>
>> There was no any specific reason to remove ServerSocket.bind() call.
>> ServerSocket(0) constructor creates a server socket, automatically
>> bound to a random free port. If I am not missing something, it
>> doesn't look necessary to set the SO_REUSEADDR socket options if the
>> server is not going to restart. The code is just shorter if we use
>> ServerSocket(0) constructor to open a server socket, but I can revert
>> it to use bind() with 0 port number if you think it's better.
>>
>> Artem
>>
>>
>> On 08/12/2016 09:13 AM, Jamil Nimeh wrote:
>>> Hi Artem, more comments in-line
>>>
>>>
>>> On 8/11/2016 11:46 AM, Artem Smotrakov wrote:
>>>> Hi Jamil,
>>>>
>>>> Thank you for review. Please see inline.
>>>>
>>>>
>>>> On 08/10/2016 04:16 PM, Jamil Nimeh wrote:
>>>>> Hi Artem,
>>>>>
>>>>> I'm not an official reviewer but the solution for making the
>>>>> servers reject connections rather than stop and start looks pretty
>>>>> fair to me and seems like a nice way to simulate a downed OCSP
>>>>> responder instead of having to bounce it.  A couple
>>>>> comments/questions:
>>>>>
>>>>> I'm a bit surprised that you get the "Address already in use"
>>>>> error though.
>>>> Well, to be honest, I was not able to reproduce this failure
>>>> locally. I was running the test in a loop for a couple of days, and
>>>> it didn't fail. But the test has been observed to fail in other
>>>> test runs (jprt, CI, etc).
>>>>
>>>> I am not an expert in networking, and I would appreciate if someone
>>>> more knowledgeable gives an advise how these intermittent failures
>>>> can be avoided.
>>>>
>>>>> Isn't servSocket.setReuseAddress(true) on line 214 supposed to set
>>>>> the SO_REUSEADDR at the system call level and prevent EADDRINUSE
>>>>> when listening or binding?
>>>> If I am not missing something, the test has been observed to fails
>>>> while re-binding. I am wondering if it's possible that the port
>>>> becomes busy after the server socket was closed, but before bind()
>>>> is called again. The probability of this situation seems to be very
>>>> low which has been actually seen - the test fails very rare.
>>>>
>>>> If this is the case, it seems like servSocket.setReuseAddress(true)
>>>> doesn't help because the port is taken by another process (I am not
>>>> sure that SO_REUSEADDR prevents from this). Again, this is only my
>>>> guess, and I may be wrong.
>>> You know, I hadn't thought of that.  I've never been able to
>>> reproduce that problem either, but I'm running on a local virtual
>>> box VM on a laptop, and usually the tests are running sequentially.
>>> I could definitely see the case where other processes are soaking up
>>> the OCSP responder's port.  With those tests, I kind of need the
>>> port to remain the same since I'm putting that server and port in
>>> the AIA extensions of the certs for which it answers.  Given this
>>> particular case, it seems like your solution of keeping the server
>>> bound but just chopping connections off is the best way to go.
>>>>>
>>>>> When you create th

Re: [9] RFR: 8162484: javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails intermittently with "Address already in use" error

[Artem Smotrakov]

No problem.

http://cr.openjdk.java.net/~asmotrak/8162484/webrev.02/

Artem


On 08/12/2016 12:02 PM, Jamil Nimeh wrote:
For the tests as we use them today we don't intend the server to 
restart.  The intent of SimpleOCSPServer was to be of use for a 
variety of testing purposes.  I don't know that we can say for all 
intended uses that we'll *never* need to restart it. That's why I'd 
like to keep the unbound socket/set sockopt/bind/listen behavior.  I 
don't think ServerSocket(0) achieves that.


--Jamil

On 8/12/2016 11:30 AM, Artem Smotrakov wrote:

Hi Jamil,

There was no any specific reason to remove ServerSocket.bind() call. 
ServerSocket(0) constructor creates a server socket, automatically 
bound to a random free port. If I am not missing something, it 
doesn't look necessary to set the SO_REUSEADDR socket options if the 
server is not going to restart. The code is just shorter if we use 
ServerSocket(0) constructor to open a server socket, but I can revert 
it to use bind() with 0 port number if you think it's better.


Artem


On 08/12/2016 09:13 AM, Jamil Nimeh wrote:

Hi Artem, more comments in-line


On 8/11/2016 11:46 AM, Artem Smotrakov wrote:

Hi Jamil,

Thank you for review. Please see inline.


On 08/10/2016 04:16 PM, Jamil Nimeh wrote:

Hi Artem,

I'm not an official reviewer but the solution for making the 
servers reject connections rather than stop and start looks pretty 
fair to me and seems like a nice way to simulate a downed OCSP 
responder instead of having to bounce it.  A couple 
comments/questions:


I'm a bit surprised that you get the "Address already in use" 
error though. 
Well, to be honest, I was not able to reproduce this failure 
locally. I was running the test in a loop for a couple of days, and 
it didn't fail. But the test has been observed to fail in other 
test runs (jprt, CI, etc).


I am not an expert in networking, and I would appreciate if someone 
more knowledgeable gives an advise how these intermittent failures 
can be avoided.


Isn't servSocket.setReuseAddress(true) on line 214 supposed to set 
the SO_REUSEADDR at the system call level and prevent EADDRINUSE 
when listening or binding?
If I am not missing something, the test has been observed to fails 
while re-binding. I am wondering if it's possible that the port 
becomes busy after the server socket was closed, but before bind() 
is called again. The probability of this situation seems to be very 
low which has been actually seen - the test fails very rare.


If this is the case, it seems like servSocket.setReuseAddress(true) 
doesn't help because the port is taken by another process (I am not 
sure that SO_REUSEADDR prevents from this). Again, this is only my 
guess, and I may be wrong.
You know, I hadn't thought of that.  I've never been able to 
reproduce that problem either, but I'm running on a local virtual 
box VM on a laptop, and usually the tests are running sequentially. 
I could definitely see the case where other processes are soaking up 
the OCSP responder's port.  With those tests, I kind of need the 
port to remain the same since I'm putting that server and port in 
the AIA extensions of the certs for which it answers.  Given this 
particular case, it seems like your solution of keeping the server 
bound but just chopping connections off is the best way to go.


When you create the new ServerSocket on line 212, you're now 
binding it to the port now where originally it started as an 
unbound socket.  By doing so, the behavior of setReuseAddress() on 
line 214 is now undefined.
This setReuseAddress() call looks unnecessary now. I'll update the 
test.
While this test no longer stops/starts the server, other tests may 
wish to do so and their behavior may not be consistent (though 
apparently it wasn't consistent even in the old scheme where the 
socket was unbound, then setReuseAddress() was called...)

Correct. I checked other code which depend on SimpleOCSPServer

javax/net/ssl/Stapling/HttpsUrlConnClient.java
javax/net/ssl/Stapling/SSLEngineWithStapling.java
javax/net/ssl/Stapling/SSLSocketWithStapling.java
javax/net/ssl/Stapling/StapleEnableProps.java
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusReqSelection.java 

sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusResponseManagerTests.java 

artem@artem-laptop:~/ws/jdk/jdk9_dev_stapling_test/jdk/test$ kate 
javax/net/ssl/Stapling/HttpsUrlConnClient.java 
javax/net/ssl/Stapling/SSLEngineWithStapling.java 
javax/net/ssl/Stapling/SSLSocketWithStapling.java 
javax/net/ssl/Stapling/StapleEnableProps.java 
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusReqSelection.java 
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusResponseManagerTests.java 



These tests call stop() only once when actual testcases are done. 
Actually, some of them don't even 

Re: [9] RFR: 8162484: javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails intermittently with "Address already in use" error

[Artem Smotrakov]

Hi Jamil,

There was no any specific reason to remove ServerSocket.bind() call. 
ServerSocket(0) constructor creates a server socket, automatically bound 
to a random free port. If I am not missing something, it doesn't look 
necessary to set the SO_REUSEADDR socket options if the server is not 
going to restart. The code is just shorter if we use ServerSocket(0) 
constructor to open a server socket, but I can revert it to use bind() 
with 0 port number if you think it's better.


Artem


On 08/12/2016 09:13 AM, Jamil Nimeh wrote:

Hi Artem, more comments in-line


On 8/11/2016 11:46 AM, Artem Smotrakov wrote:

Hi Jamil,

Thank you for review. Please see inline.


On 08/10/2016 04:16 PM, Jamil Nimeh wrote:

Hi Artem,

I'm not an official reviewer but the solution for making the servers 
reject connections rather than stop and start looks pretty fair to 
me and seems like a nice way to simulate a downed OCSP responder 
instead of having to bounce it.  A couple comments/questions:


I'm a bit surprised that you get the "Address already in use" error 
though. 
Well, to be honest, I was not able to reproduce this failure locally. 
I was running the test in a loop for a couple of days, and it didn't 
fail. But the test has been observed to fail in other test runs 
(jprt, CI, etc).


I am not an expert in networking, and I would appreciate if someone 
more knowledgeable gives an advise how these intermittent failures 
can be avoided.


Isn't servSocket.setReuseAddress(true) on line 214 supposed to set 
the SO_REUSEADDR at the system call level and prevent EADDRINUSE 
when listening or binding?
If I am not missing something, the test has been observed to fails 
while re-binding. I am wondering if it's possible that the port 
becomes busy after the server socket was closed, but before bind() is 
called again. The probability of this situation seems to be very low 
which has been actually seen - the test fails very rare.


If this is the case, it seems like servSocket.setReuseAddress(true) 
doesn't help because the port is taken by another process (I am not 
sure that SO_REUSEADDR prevents from this). Again, this is only my 
guess, and I may be wrong.
You know, I hadn't thought of that.  I've never been able to reproduce 
that problem either, but I'm running on a local virtual box VM on a 
laptop, and usually the tests are running sequentially. I could 
definitely see the case where other processes are soaking up the OCSP 
responder's port.  With those tests, I kind of need the port to remain 
the same since I'm putting that server and port in the AIA extensions 
of the certs for which it answers.  Given this particular case, it 
seems like your solution of keeping the server bound but just chopping 
connections off is the best way to go.


When you create the new ServerSocket on line 212, you're now binding 
it to the port now where originally it started as an unbound 
socket.  By doing so, the behavior of setReuseAddress() on line 214 
is now undefined.

This setReuseAddress() call looks unnecessary now. I'll update the test.
While this test no longer stops/starts the server, other tests may 
wish to do so and their behavior may not be consistent (though 
apparently it wasn't consistent even in the old scheme where the 
socket was unbound, then setReuseAddress() was called...)

Correct. I checked other code which depend on SimpleOCSPServer

javax/net/ssl/Stapling/HttpsUrlConnClient.java
javax/net/ssl/Stapling/SSLEngineWithStapling.java
javax/net/ssl/Stapling/SSLSocketWithStapling.java
javax/net/ssl/Stapling/StapleEnableProps.java
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusReqSelection.java 

sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusResponseManagerTests.java 

artem@artem-laptop:~/ws/jdk/jdk9_dev_stapling_test/jdk/test$ kate 
javax/net/ssl/Stapling/HttpsUrlConnClient.java 
javax/net/ssl/Stapling/SSLEngineWithStapling.java 
javax/net/ssl/Stapling/SSLSocketWithStapling.java 
javax/net/ssl/Stapling/StapleEnableProps.java 
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusReqSelection.java 
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusResponseManagerTests.java 



These tests call stop() only once when actual testcases are done. 
Actually, some of them don't even call stop(), but it seems to work 
fine. As an enhancement, I would add stop() calls to finally blocks, 
but it seems to work fine without it anyway.
I liked your solution with the stop() calls in finally blocks and I 
agree that they should have them.  I think we get away with it because 
in most if not all of those cases they are running as othervm tests 
(because we have properties that we set specific to the tests).  So 
when the JVM exits resources like sockets are closed by the OS.  
Still, it's better to have the try/finally guards and explicitly and 
gracefully shut

Re: [9] RFR: 8162484: javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails intermittently with "Address already in use" error

[Artem Smotrakov]

Hi Jamil,

Thank you for review. Please see inline.


On 08/10/2016 04:16 PM, Jamil Nimeh wrote:

Hi Artem,

I'm not an official reviewer but the solution for making the servers 
reject connections rather than stop and start looks pretty fair to me 
and seems like a nice way to simulate a downed OCSP responder instead 
of having to bounce it.  A couple comments/questions:


I'm a bit surprised that you get the "Address already in use" error 
though. 
Well, to be honest, I was not able to reproduce this failure locally. I 
was running the test in a loop for a couple of days, and it didn't fail. 
But the test has been observed to fail in other test runs (jprt, CI, etc).


I am not an expert in networking, and I would appreciate if someone more 
knowledgeable gives an advise how these intermittent failures can be 
avoided.


Isn't servSocket.setReuseAddress(true) on line 214 supposed to set the 
SO_REUSEADDR at the system call level and prevent EADDRINUSE when 
listening or binding?
If I am not missing something, the test has been observed to fails while 
re-binding. I am wondering if it's possible that the port becomes busy 
after the server socket was closed, but before bind() is called again. 
The probability of this situation seems to be very low which has been 
actually seen - the test fails very rare.


If this is the case, it seems like servSocket.setReuseAddress(true) 
doesn't help because the port is taken by another process (I am not sure 
that SO_REUSEADDR prevents from this). Again, this is only my guess, and 
I may be wrong.


When you create the new ServerSocket on line 212, you're now binding 
it to the port now where originally it started as an unbound socket.  
By doing so, the behavior of setReuseAddress() on line 214 is now 
undefined.

This setReuseAddress() call looks unnecessary now. I'll update the test.
While this test no longer stops/starts the server, other tests may 
wish to do so and their behavior may not be consistent (though 
apparently it wasn't consistent even in the old scheme where the 
socket was unbound, then setReuseAddress() was called...)

Correct. I checked other code which depend on SimpleOCSPServer

javax/net/ssl/Stapling/HttpsUrlConnClient.java
javax/net/ssl/Stapling/SSLEngineWithStapling.java
javax/net/ssl/Stapling/SSLSocketWithStapling.java
javax/net/ssl/Stapling/StapleEnableProps.java
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusReqSelection.java
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusResponseManagerTests.java
artem@artem-laptop:~/ws/jdk/jdk9_dev_stapling_test/jdk/test$ kate 
javax/net/ssl/Stapling/HttpsUrlConnClient.java 
javax/net/ssl/Stapling/SSLEngineWithStapling.java 
javax/net/ssl/Stapling/SSLSocketWithStapling.java 
javax/net/ssl/Stapling/StapleEnableProps.java 
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusReqSelection.java 
sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusResponseManagerTests.java 



These tests call stop() only once when actual testcases are done. 
Actually, some of them don't even call stop(), but it seems to work 
fine. As an enhancement, I would add stop() calls to finally blocks, but 
it seems to work fine without it anyway.


Here is an updated webrev:

http://cr.openjdk.java.net/~asmotrak/8162484/webrev.01/

Artem




--Jamil


On 08/10/2016 03:44 PM, Artem Smotrakov wrote:

Hello,

Please review this update for OCSP stapling tests.

The tests use test/java/security/testlibrary/SimpleOCSPServer.java 
which try to re-use a server port if the server restarted. Looks like 
sometimes it may cause "Address already in use" error.


The patch updates OCSP stapling tests with the following:
- updated SSLSocketWithStapling.java test not to restart OCSP responders
- updated SimpleOCSPServer to be able to reject incoming connections
- updated SimpleOCSPServer to be able to reproduce a delay without 
restarting


Jamil,

Could you please take a look at this update, and confirm if this 
update doesn't break the original test scenarios?


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

Artem

[9] RFR: 8162484: javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails intermittently with "Address already in use" error

[Artem Smotrakov]

Hello,

Please review this update for OCSP stapling tests.

The tests use test/java/security/testlibrary/SimpleOCSPServer.java which 
try to re-use a server port if the server restarted. Looks like 
sometimes it may cause "Address already in use" error.


The patch updates OCSP stapling tests with the following:
- updated SSLSocketWithStapling.java test not to restart OCSP responders
- updated SimpleOCSPServer to be able to reject incoming connections
- updated SimpleOCSPServer to be able to reproduce a delay without 
restarting


Jamil,

Could you please take a look at this update, and confirm if this update 
doesn't break the original test scenarios?


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

Artem

Re: RFR 8133910: Some sun/security/tools tests failed.

[Artem Smotrakov]

Hi Max,

The update looks good to me.

Artem


On 08/09/2016 08:39 AM, Weijun Wang wrote:

I was wrong. The test were written by Artem.

--Max

On 8/9/2016 15:37, Wang Weijun wrote:

Please review the fix at

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

Basically, "-J-Duser.language=en -J-Duser.country=US" is added to 
keytool and jarsigner calls wherever output needs to be compared to 
some English text.


*Siba*: I modified quite some tests by you. Please confirm it's OK.

Thanks
Max

Re: RFR 8163489: Avoid using Utils.getFreePort() in TsacertOptionTest.java test

[Artem Smotrakov]

+1

Minor: no need a semicolon in "try" block, I originally added it by 
mistake. You also may want to update a copyright year.


Artem


On 08/09/2016 08:46 AM, Chris Hegarty wrote:

On 9 Aug 2016, at 16:37, Weijun Wang  wrote:

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

Thanks Max, this looks good( one less use of the get free port anti-pattern! ).

-Chris.

Re: [9] RFR: 8159416: javax/net/ssl/DTLS/CipherSuite.java failed on timeout

[Artem Smotrakov]

Please see an updated webrev

http://cr.openjdk.java.net/~asmotrak/8159416/webrev.04/

Artem


On 08/02/2016 02:42 PM, Artem Smotrakov wrote:

Hi Xuelei,

Thank you for review. Please see inline. I'll send an updated webrev 
soon.


On 08/01/2016 09:35 PM, Xuelei Fan wrote:

DTLSOutputRecord

  182   int len = destination.remaining();
  235   int len = destination.remaining();

This variable is only use by debug block.  I may try to avoid to define
it out side of the debug block.
encrypt() changes "destination" buffer, that's why the "len" variable 
is defined before it. Probably the length may be calculated some other 
way. I don't like it much either, but this way is just the most simple 
and straightforward to me.

I know you want to dump the length in
the "record" debug option.  But I think the epoch/sequence# belong to
the "packet" debug option.  The "record" option does not include the
header part, so may be, it is not suitable to dump the epoch/seq# here.

I may suggest to remove the update as the "packet" option can help if
users want to know the epoch/sequence numbers.  Please see the comment
for DTLSOverDatagram.java, too.
If -Djavax.net.debug=packet is set, then it prints something like the 
following:


[Raw write]: length = 135
: 16 FE FD 00 00 00 00 00   00 00 00 00 7A 01 00 00 z...
0010: 6E 00 00 00 00 00 00 00   6E FE FD 18 E3 9C 82 DF n...n...
0020: 02 6C 42 FF AB EE 0C 9A   82 1B 78 6A 31 93 DC 6A .lB...xj1..j
0030: DC 70 9F 5A 91 DB D5 04   E1 99 65 00 00 00 02 00 .p.Z..e.
0040: 3C 01 00 00 42 00 0D 00   1C 00 1A 06 03 06 01 05 <...B...
0050: 03 05 01 04 03 04 01 04   02 03 03 03 01 03 02 02 
0060: 03 02 01 02 02 00 11 00   10 00 0E 02 00 04 00 00 
0070: 00 00 01 00 04 00 00 00   00 00 05 00 05 01 00 00 
0080: 00 00 FF 01 00 01 00   ...

Well ... I am sure that such a dump contains everything we need, but 
... it doesn't look really friendly :) If someone is not an expert in 
TLS/DTLS like me, than it might be helpful to have some thing more 
human-readable in logs :) That's why I wanted to print sequence and 
epoch numbers (it helped me while looking into 8161086).


466-470
This is a nice update to get the user friendly handshake type
description.  There are still a lot similar debug log that need to get
similar update.  Would you mind to file a new RFE, and make the update
together with other places (both TLS and DTLS) all together?

Sure, I filed https://bugs.openjdk.java.net/browse/JDK-8162996

I'll revert all changes related to debut output. They may be 
integrated in the RFE above.



DTLSInputRecord
---
  159long onlySeqNum  =
 Authenticator.getDTLSSequenceNumber(recordEnS);

Using getDTLSSequenceNumber(long) may be more effective.

161-165, if you consider to remove the debug log update in
DTLSOutputRecord, you may also want to remove the debug log update here
for similar reasons.

HandshakeMessage

See comment for DTLSOutputRecord.  I may prefer to file a new RFE, and
make the update all together.


Authenticator
-
May not need the update any more if you consider my suggestions above.
Otherwise, I think may be only

getDTLSSequenceNumber(long recordEnS)

is actually used.  Other two new methods can be removed.


InvalidRecords
--
I like this find!


DTLSOverDatagram


50 System.setProperty("javax.net.debug", "ssl,record");

I may suggest to use "ssl,record,packet" for debugging if you won't dump
the epoch/seq# in "record" debug option.  If the test passed, I may
prefer to turn the debug log off.
The tests pass now. I don't mind removing this debug output, but it 
happens often that we don't have enough info if a test fails 
intermittently. Then, we may update the test to use -Djavax.net.debug 
to get more info when it fails next time. I think it might be good to 
enable debug output for tests by default, so that we have more info to 
analyze failures. As far as I know, jtreg may be configured to keep 
jtr files only for failed tests, so additional output shouldn't take 
much space if someone runs these test often. Is there any other concerns?

doServerSide()
I did not get too much about the update:
1. Is it necessary to combine handshake and reading client application
data together?
Final handshake messages to a client may be lost. Client still will be 
expecting them, and server needs to re-send them. When server is done 
with handshaking, it tries to receive first application data. But if 
client didn't receive final handshake messages (because they were 
lost), it keeps re-sending handshake messages to server. In this 
situation, server needs to re-send final handshake messages to cli

Re: [9] RFR: 8159416: javax/net/ssl/DTLS/CipherSuite.java failed on timeout

[Artem Smotrakov]

e. handshake() 
method may return final handshake packets to the caller, so that it can 
re-send them if necessary. This is just another way, I don't think any 
of them is better that another.

I think the receiveAppData can be reused.
I don't think it can be re-used "as is" because it needs to re-send 
final handshake messages on server side. Okay, I'll update the method, 
and re-use it.


2. Is it necessary to re-send application data?
I think if application data is not allowed to drop, may not need to
the re-send code.

You're right. I'll remove those while loops.


Same for the doClientSide() implementation.

disablePacketDrop() is a static method, you may not want to call it
exception the initialization stage.  The following line need to be very
carefully used:

  162   CustomDatagramSocketImplFactory.disablePacketDrop();
  334   CustomDatagramSocketImplFactory.disablePacketDrop();
disablePacketDrop() asks CustomDatagramSocketImpl to stop dropping 
packets. This method should be called before we start sending 
application data. It seems like one of these disablePacketDrop() calls 
is redundant, I'll move it to sendAppData() method.


324-375, may be not necessary, or can be improved to use
receiveAppData() method instead.

Sure, will do, please see above.

   I think the wrap() and unwrap() impl
should take care of the retransmission of handshaking messages, but not
the application.  Resending the previous packet does not comply to DTLS
retransmission specification.  If you run into problems, would you mind
show me the debug log?
I am not sure what logs you are asking about. You can find some logs 
attached to https://bugs.openjdk.java.net/browse/JDK-8159416


I am also not sure what you mean by retransmission here. As far as I 
understand, wrap() and unwrap() methods produce/consume (D)TLS records, 
and client code should take care of sending them over the network to the 
remote peer. While expecting first application data, the test re-sends 
the same packets which were generated by last wrap() call on server 
side. This means that DTLS records with the same sequence numbers are 
used again.


Do you think that server should generate records with new sequence numbers?

(I have not tried to call wrap() again to produce final handshake 
message after server's SSLEngine has finished handshaking, not sure if 
it works)





- Updated client and server to use simple threads instead of thread
pools. This way, it's possible to specify meaningful names for
threads

A more light update may be use Thread.setName(String name) in the
ServerCallable.call().  I did not test.  If you like, you can have a try.
setName() method is not static, so it should be called on an instance of 
Thread, but I don't know how to access it in ServerCallable.call() method.



CustomDatagramSocketImpl.addPacket()
I may suggest to check application data here.

  927 private void addPacket(byte[] data) {
  928 // check if a packet should be dropped
-929 if (CustomDatagramSocketImplFactory.losePacket()) {
+if (ustomDatagramSocketImplFactory.losePacket() &&
+(data[0] != (byte)0x17)) {
  930 return;
  931 }

"data[0] != (byte)0x17" means that it is not application data.  As would
simplify the code a lot.  See comments above about the application data
re-sending issue.
It wouldn't simplify the code to me because "0x17" looks like a magic 
number. Of course, we can define it as a constant with some meaningful 
name, but to me it is clearer to call a method which just stops dropping 
packets. It doesn't make the code too complicated to me. If you don't 
mind, and there is no other issues with it, I would prefer to keep it.



PacketLossXXX.java
--
It should not be the common case to split test cases separated with the
test code.  I would suggest merge them into the PacketLoss.java.
The problem here is that those test take some time for execution. That's 
why I splitted them to different files.


As far as I remember, there was a concern from people who run those 
tests in CI systems that some tests take too long. Their point was that 
tests execution may be faster if we run tests in concurrent mode, and 
don't have tests which take too long. And if I recall correctly, there 
was an agreement that we split such tests this way.


I am cc'ing Rajan, he may remember more about this.

Artem

   If you
are worrying about the debug log length, please turn off the debug log.
Actually, we should turn off the debug log unless we need more detailed
debug log for problematic test.  Or alternatively, we can ask the JTREG
to consider the behavior more about the debug length restrictions.

Thanks,
Xuelei

On 8/2/2016 9:25 AM, Artem Smotrakov wrote:

Here is an updated webrev which includes a fix for 8161086 (thanks
Xuelei f

Re: [9] RFR: 8159416: javax/net/ssl/DTLS/CipherSuite.java failed on timeout

[Artem Smotrakov]

Here is an updated webrev which includes a fix for 8161086 (thanks 
Xuelei for help):

- No updates to the problem list
- CustomDatagramSocketImplFactory drops 5% of packets by default
- Updated DTLSInputRecord and DTLSOutputRecord to print sequence numbers 
if -Djavax.net.debug=ssl option is specified
- Updated InvalidRecords.java tests not to drop packets by default 
because handshaking unexpectedly succeeds if an invalid packet was dropped


http://cr.openjdk.java.net/~asmotrak/8159416/webrev.03/

Please take a look.

Artem

On 07/28/2016 04:36 PM, Artem Smotrakov wrote:

Hi Xuelei,

I updated CustomDatagramSocketImpl class to be able to drop some 
packets randomly or by specified numbers.


While updating the test, I found 
https://bugs.openjdk.java.net/browse/JDK-8161086 which I think is the 
root cause of these intermittent failures.


I added a couple of PacketLoss* tests to reproduce and verify 8161086. 
These tests drop different packets while handshaking. They use 
different cipher suites and modes, so that they take a while. I didn't 
notice they fail with timeout, but I suspect that may happen on slower 
machines. That's why I increased timeout value for them. I added the 
tests to the problem list since 8161086 is not fixed yet.


There are a couple of other updates to DTLSOverDatagram.java:
- Server tries to receive first application data when it finishes 
handshaking. Server re-sends final handshaking messages if timeout 
reached while waiting for application data from client.
- Updated client and server to use simple threads instead of thread 
pools. This way, it's possible to specify meaningful names for threads 
which makes logs more readable if "-Djavax.net.debug=ssl" specified

- Added more logging
- Made some fields final

At the moment, CustomDatagramSocketImplFactory doesn't drop packets by 
default because it may cause intermittent failures of tests which are 
based on DTLSOverDatagram class:


...
static class CustomDatagramSocketImplFactory
implements DatagramSocketImplFactory {
...
// if true, it's going to drop some packets
private static boolean dropPackets = false;
...

When 8161086 is fixed, it may be set to true by default. Currently the 
packet loss rate is 5% which I think is much more than if we use UDP 
sockets on localhost, so that we test DTLS impl in worse conditions.


Could you please take a look at updated webrev?

http://cr.openjdk.java.net/~asmotrak/8159416/webrev.02/

Artem

On 06/27/2016 06:25 PM, Xuelei Fan wrote:

On 6/28/2016 9:12 AM, Artem Smotrakov wrote:

Hello,

Please review this patch for javax/net/ssl/DTLS tests.

A couple of DTLS tests fail intermittently on Mac with timeout or "Too
many handshake loops ..." error. The tests use UDP to transfer DTLS
records. It happens sometimes that server cannot receive UDP packets
with DatagramSocket.receive() method even if client tries to re-send
them multiple times. Please see logs in the bug.

At the moment, it is not clear why UDP packets can't be delivered to
server. If someone has seen something similar before, or has some ideas
what might be the root cause, please let me know.


UDP is not reliable.  It could happen that the packets get lost.


I think that the tests and DTLS implementation are fine here. Since the
tests are for DTLS, we can workaround this issue by avoiding real UDP
connections. To avoid changing test logic much, we can use a custom
DatagramSocketImpl and DatagramSocketImplFactory implementations to
replace system UDP implementation.

The patch below updates the tests with the following:
- added custom DatagramSocketImpl and DatagramSocketImplFactory
implementations to avoid real UDP connections

Tests for real connections in practice are needed.  If we update this
test this way, we need to add other tests to test real application
usage.  I don't think this is the right direction to avoid real UDP
connections.


- added more test output, so that we can have more info it the tests
fail again


I agree with this point.

Xuelei


I have run javax/net/ssl/DTLS/CipherSuite.java test in a loop on Mac,
and I didn't see it failed. I also have run javax/net/ssl/DTLS tests on
all supported generic platforms, and they worked fine.

Bug: https://bugs.openjdk.java.net/browse/JDK-8159416
http://cr.openjdk.java.net/~asmotrak/8159416/webrev.00/

Artem

Re: [9] RFR: 8159416: javax/net/ssl/DTLS/CipherSuite.java failed on timeout

[Artem Smotrakov]

Hi Xuelei,

I updated CustomDatagramSocketImpl class to be able to drop some packets 
randomly or by specified numbers.


While updating the test, I found 
https://bugs.openjdk.java.net/browse/JDK-8161086 which I think is the 
root cause of these intermittent failures.


I added a couple of PacketLoss* tests to reproduce and verify 8161086. 
These tests drop different packets while handshaking. They use different 
cipher suites and modes, so that they take a while. I didn't notice they 
fail with timeout, but I suspect that may happen on slower machines. 
That's why I increased timeout value for them. I added the tests to the 
problem list since 8161086 is not fixed yet.


There are a couple of other updates to DTLSOverDatagram.java:
- Server tries to receive first application data when it finishes 
handshaking. Server re-sends final handshaking messages if timeout 
reached while waiting for application data from client.
- Updated client and server to use simple threads instead of thread 
pools. This way, it's possible to specify meaningful names for threads 
which makes logs more readable if "-Djavax.net.debug=ssl" specified

- Added more logging
- Made some fields final

At the moment, CustomDatagramSocketImplFactory doesn't drop packets by 
default because it may cause intermittent failures of tests which are 
based on DTLSOverDatagram class:


...
static class CustomDatagramSocketImplFactory
implements DatagramSocketImplFactory {
...
// if true, it's going to drop some packets
private static boolean dropPackets = false;
...

When 8161086 is fixed, it may be set to true by default. Currently the 
packet loss rate is 5% which I think is much more than if we use UDP 
sockets on localhost, so that we test DTLS impl in worse conditions.


Could you please take a look at updated webrev?

http://cr.openjdk.java.net/~asmotrak/8159416/webrev.02/

Artem

On 06/27/2016 06:25 PM, Xuelei Fan wrote:

On 6/28/2016 9:12 AM, Artem Smotrakov wrote:

Hello,

Please review this patch for javax/net/ssl/DTLS tests.

A couple of DTLS tests fail intermittently on Mac with timeout or "Too
many handshake loops ..." error. The tests use UDP to transfer DTLS
records. It happens sometimes that server cannot receive UDP packets
with DatagramSocket.receive() method even if client tries to re-send
them multiple times. Please see logs in the bug.

At the moment, it is not clear why UDP packets can't be delivered to
server. If someone has seen something similar before, or has some ideas
what might be the root cause, please let me know.


UDP is not reliable.  It could happen that the packets get lost.


I think that the tests and DTLS implementation are fine here. Since the
tests are for DTLS, we can workaround this issue by avoiding real UDP
connections. To avoid changing test logic much, we can use a custom
DatagramSocketImpl and DatagramSocketImplFactory implementations to
replace system UDP implementation.

The patch below updates the tests with the following:
- added custom DatagramSocketImpl and DatagramSocketImplFactory
implementations to avoid real UDP connections

Tests for real connections in practice are needed.  If we update this
test this way, we need to add other tests to test real application
usage.  I don't think this is the right direction to avoid real UDP
connections.


- added more test output, so that we can have more info it the tests
fail again


I agree with this point.

Xuelei


I have run javax/net/ssl/DTLS/CipherSuite.java test in a loop on Mac,
and I didn't see it failed. I also have run javax/net/ssl/DTLS tests on
all supported generic platforms, and they worked fine.

Bug: https://bugs.openjdk.java.net/browse/JDK-8159416
http://cr.openjdk.java.net/~asmotrak/8159416/webrev.00/

Artem

Re: RFR 8054536: sun.security.x509.Extension object may throw NPE for hashCode and equals method

[Artem Smotrakov]

Hi Svetlana,

The webrev looks fine to me. Please see one more comment inline.

On 07/15/2016 11:12 AM, Svetlana Nikandrova wrote:

Hi Artem,

ok, lets get rid of possible NPE in other methods too:
http://cr.openjdk.java.net/~snikandrova/8054536/webrev.01/ 
<http://cr.openjdk.java.net/%7Esnikandrova/8054536/webrev.01/>
I also noticed that encode(DerOutputStream) checks extensionId and 
extensionValue for null, but encode(OutputStream) doesn't.


encode(OutputStream) might just call encode(DerOutputStream), so that it 
always checks those fields for null. It also would remove some duplicate 
code.


As for unnecessary try-catch in test I'd prefer to have it to emphasis 
that we are checking for NPE.

Okay.
I'm not sure about "iff" but let it be, it seems like a right place to 
use it.

Sure.

Artem


Thank you,
Svetlana

On 14.07.2016 19:35, Artem Smotrakov wrote:

Hi Svetlana,

I'll leave the main review to an official reviewer, but I have a 
couple of comments.


There are a couple of other places in Extension.java where NPE may 
occur:
- line 255: I see that "extensionId" is checked for null in other 
methods, but not in getId()
- line 200: I see that "extensionValue" is checked for null in other 
methids, but not in getValue()


Minor: try-catch is not really necessary.

I am not sure, but "iff" might mean "if and only if", so it may be 
not a typo.


You may want to add @Override to a couple of methods since you update 
Extension.java


Artem

On 07/14/2016 08:19 AM, Svetlana Nikandrova wrote:

Hello,

please review the fix for:
https://bugs.openjdk.java.net/browse/JDK-8054536
Webrev:
http://cr.openjdk.java.net/~snikandrova/8054536/webrev.00/ 
<http://cr.openjdk.java.net/%7Esnikandrova/8054536/webrev.00/>


Description:
Equals and hasCode methods of Extension use extensionId without 
prior check for "null" (+ 1 mistype in equals javadoc).


Thank you,
Svetlana

Re: RFR 8054536: sun.security.x509.Extension object may throw NPE for hashCode and equals method

[Artem Smotrakov]

Hi Svetlana,

I'll leave the main review to an official reviewer, but I have a couple 
of comments.


There are a couple of other places in Extension.java where NPE may occur:
- line 255: I see that "extensionId" is checked for null in other 
methods, but not in getId()
- line 200: I see that "extensionValue" is checked for null in other 
methids, but not in getValue()


Minor: try-catch is not really necessary.

I am not sure, but "iff" might mean "if and only if", so it may be not a 
typo.


You may want to add @Override to a couple of methods since you update 
Extension.java


Artem

On 07/14/2016 08:19 AM, Svetlana Nikandrova wrote:

Hello,

please review the fix for:
https://bugs.openjdk.java.net/browse/JDK-8054536
Webrev:
http://cr.openjdk.java.net/~snikandrova/8054536/webrev.00/ 



Description:
Equals and hasCode methods of Extension use extensionId without prior 
check for "null" (+ 1 mistype in equals javadoc).


Thank you,
Svetlana

Re: Code Review Request JDK-8161106 Improve SSLSocket test template

[Artem Smotrakov]

Hi Xuelei,

The webrev looks good to me. Please see inline.

On 07/12/2016 10:36 PM, Xuelei Fan wrote:

Thanks for the feedback, Artem.  Here is the updated webrev per your
suggestions:

 http://cr.openjdk.java.net/~xuelei/8161106/webrev.02/

On 7/13/2016 1:03 AM, Artem Smotrakov wrote:

Hi Xuelei,

I am not an official reviewer, but I have a couple of comments.

1. line 149: would it be better to check this condition in a loop?


clientCondition.await() will wait for 30 seconds.  Need no loop any
more.  If adding a loop, it may become a potential deadloop and cause
timeout issue.

Right, I didn't get it when I read it first time.

2. Using try-with-resources blocks might simplify doServerSide() a
little bit (no need to call close() on sockets, and a couple of "try"
blocks might be probably removed)


I considered to use try-with-resources.  But as the lock is introduced.
and we want to support socket accept timeout, it more convenient to me
to use the traditional try-finally clauses.
Agree. Using try-with resources would remove one try-finally block, but 
probably it would introduce nested "try" blocks.

3. Minor: it might be better to define timeout values as constants with
meaningful names.


Maybe.  Normally, if a number is used only once, I may not define a
field for it.

Sure.



4. Minor: lines 268-273 seem to work, but I am wondering if it would be
better to use File.separator instead of "/" (someone may run the test on
Windows, but without Cygwin).


I think JDK file I/O would take care of this file separator
interoperability.  It's good to use File.separator, but as the path may
contains a few separator in test case following this template, for example:

 pathToStores = "../../../../../javax/net/ssl";

Using File.separator would not make nice line.
Okay. I am not an expert in Java I/O, so I am not sure. Agree, 
File.separator wouldn't make it nicer.

5. SSLSocketSample() constructor, startServer() and startClient() methods:

Recently we've had a couple of test issue where no exception was printed
on client/server sides. It happened because exceptions were caught and
stored, but then tests waited infinitely for another thread to finish.
Then, jtreg just killed the tests. As a result, no exception was printed
out. It might be better to simplify this code to print out an exception
right after it's caught, and then throw a runtime exception if
serverException or clientException are not null. Probably there is a
side effect that logs may be a little messy sometimes because of lack of
synchronization, but maybe something like synchronized(System.out) may
help.


This update should be able to avoid the infinitely waiting.  Updated to
print the exception message right after it is caught.
Thanks. It might be better to print whole stack trace in 
srartServer/startClient

6. Minor: it might be better to make class constants uppercase

http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-135099.html#367


Good to use uppercase.  I plan to modify this template to a overridable
one so that test case can extend this template.  By then, those fields
will not be declared as constants any more.  Let's use the current names
for a while.

Sure.

Artem


Thanks,
Xuelei


Artem

On 07/10/2016 10:16 PM, Xuelei Fan wrote:

There is a nice catch of the timeout miss-match during the handling of
serverIsReady in doClientSide().   Here is the updated webrev:

  http://cr.openjdk.java.net/~xuelei/8161106/webrev.01/

Thanks,
Xuelei

On 7/11/2016 11:44 AM, Xuelei Fan wrote:

Hi,

Please review this enhancement of SSLSocket test template:
  http://cr.openjdk.java.net/~xuelei/8161106/webrev.00/

There are some known issues with the current SSLSocket test template
(test/javax/net/ssl/templates/SSLSocketTemplate.java) in the automation
testing environment:
1. the client or server can be blocked if the peer run into problems, as
may result in intermittent timeout failure.
2. the server accepted connection may be not linked to the expected
client. For example, some other test case may try to use and connect to
a free server socket port, unfortunately this port can be actually
opened by the server of SSLSocketTemplate because there is no
synchronization about the free socket port between test cases.  It's OK
to run the test singly, but may result in weird intermittent failure in
the automation testing environment.

The new test template in this update considers the noises above.

Thanks,
Xuelei

Re: Code Review Request JDK-8161106 Improve SSLSocket test template

[Artem Smotrakov]

Hi Xuelei,

I am not an official reviewer, but I have a couple of comments.

1. line 149: would it be better to check this condition in a loop?

2. Using try-with-resources blocks might simplify doServerSide() a 
little bit (no need to call close() on sockets, and a couple of "try" 
blocks might be probably removed)


3. Minor: it might be better to define timeout values as constants with 
meaningful names.


4. Minor: lines 268-273 seem to work, but I am wondering if it would be 
better to use File.separator instead of "/" (someone may run the test on 
Windows, but without Cygwin).


5. SSLSocketSample() constructor, startServer() and startClient() methods:

Recently we've had a couple of test issue where no exception was printed 
on client/server sides. It happened because exceptions were caught and 
stored, but then tests waited infinitely for another thread to finish. 
Then, jtreg just killed the tests. As a result, no exception was printed 
out. It might be better to simplify this code to print out an exception 
right after it's caught, and then throw a runtime exception if 
serverException or clientException are not null. Probably there is a 
side effect that logs may be a little messy sometimes because of lack of 
synchronization, but maybe something like synchronized(System.out) may help.


6. Minor: it might be better to make class constants uppercase

http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-135099.html#367

Artem

On 07/10/2016 10:16 PM, Xuelei Fan wrote:

There is a nice catch of the timeout miss-match during the handling of
serverIsReady in doClientSide().   Here is the updated webrev:

 http://cr.openjdk.java.net/~xuelei/8161106/webrev.01/

Thanks,
Xuelei

On 7/11/2016 11:44 AM, Xuelei Fan wrote:

Hi,

Please review this enhancement of SSLSocket test template:
 http://cr.openjdk.java.net/~xuelei/8161106/webrev.00/

There are some known issues with the current SSLSocket test template
(test/javax/net/ssl/templates/SSLSocketTemplate.java) in the automation
testing environment:
1. the client or server can be blocked if the peer run into problems, as
may result in intermittent timeout failure.
2. the server accepted connection may be not linked to the expected
client. For example, some other test case may try to use and connect to
a free server socket port, unfortunately this port can be actually
opened by the server of SSLSocketTemplate because there is no
synchronization about the free socket port between test cases.  It's OK
to run the test singly, but may result in weird intermittent failure in
the automation testing environment.

The new test template in this update considers the noises above.

Thanks,
Xuelei

Re: [9] RFR: 8159416: javax/net/ssl/DTLS/CipherSuite.java failed on timeout

[Artem Smotrakov]

Hi Mike,

On 06/28/2016 10:36 AM, Michael StJohns wrote:

On 6/28/2016 1:02 PM, Artem Smotrakov wrote:

Hi Xuelei,

Even if the tests keep using UDP to transfer data, they may not be 
realistic since they run on local host. UDP implementation is based 
on functions provided by OS. If I remember correctly, Solaris may do 
some optimizations for localhost connections, so that no real 
networking stack is used at all.
Generically, the loopback address (127.0.0.1) is treated only 
marginally differently from using the normal interface IP address.
I'm not sure what Solaris does, but for the most part you should be 
fine if you use one of the real interface addresses.
Agree. Solaris is just an example that the loopback address may not use 
real network stack.


If we want to test DTLS with real connections, I believe we should do 
the following:

1. Use two different machines
2. Make tests expect some issues with network and other environment


Instead take a look at 
https://sandilands.info/sgordon/dropping-packets-in-ubuntu-linux-using-tc-and-iptables 
- depending on the availability of these commands and functions, you 
should be able to do the testing on a single machine over the loopback 
address and get pseudo-real network behavior.
Thank you for this link! That's really useful for me, recently I was 
looking for similar tools like "tc".


Artem


Mike



#1 requires starting server and client on different hosts which 
doesn't seem to be possible with jtreg. Another option is to set up a 
remote server.


#2 may require ignoring errors, and re-trying connections. I am not 
sure that we want to update existing tests with this.


I think it is better to focus on DTLS testing, and make tests more 
stable. That's why I am proposing to use custom UDP sockets. If we 
want to test real connections, then it should be different tests 
which take into account #1 and #2 above.


Artem

On 06/27/2016 06:25 PM, Xuelei Fan wrote:

On 6/28/2016 9:12 AM, Artem Smotrakov wrote:

Hello,

Please review this patch for javax/net/ssl/DTLS tests.

A couple of DTLS tests fail intermittently on Mac with timeout or "Too
many handshake loops ..." error. The tests use UDP to transfer DTLS
records. It happens sometimes that server cannot receive UDP packets
with DatagramSocket.receive() method even if client tries to re-send
them multiple times. Please see logs in the bug.

At the moment, it is not clear why UDP packets can't be delivered to
server. If someone has seen something similar before, or has some 
ideas

what might be the root cause, please let me know.


UDP is not reliable.  It could happen that the packets get lost.

I think that the tests and DTLS implementation are fine here. Since 
the

tests are for DTLS, we can workaround this issue by avoiding real UDP
connections. To avoid changing test logic much, we can use a custom
DatagramSocketImpl and DatagramSocketImplFactory implementations to
replace system UDP implementation.

The patch below updates the tests with the following:
- added custom DatagramSocketImpl and DatagramSocketImplFactory
implementations to avoid real UDP connections

Tests for real connections in practice are needed.  If we update this
test this way, we need to add other tests to test real application
usage.  I don't think this is the right direction to avoid real UDP
connections.


- added more test output, so that we can have more info it the tests
fail again


I agree with this point.

Xuelei


I have run javax/net/ssl/DTLS/CipherSuite.java test in a loop on Mac,
and I didn't see it failed. I also have run javax/net/ssl/DTLS 
tests on

all supported generic platforms, and they worked fine.

Bug: https://bugs.openjdk.java.net/browse/JDK-8159416
http://cr.openjdk.java.net/~asmotrak/8159416/webrev.00/

Artem

Re: [9] RFR: 8159416: javax/net/ssl/DTLS/CipherSuite.java failed on timeout

[Artem Smotrakov]

Hi Xuelei,

Even if the tests keep using UDP to transfer data, they may not be 
realistic since they run on local host. UDP implementation is based on 
functions provided by OS. If I remember correctly, Solaris may do some 
optimizations for localhost connections, so that no real networking 
stack is used at all.


If we want to test DTLS with real connections, I believe we should do 
the following:

1. Use two different machines
2. Make tests expect some issues with network and other environment

#1 requires starting server and client on different hosts which doesn't 
seem to be possible with jtreg. Another option is to set up a remote server.


#2 may require ignoring errors, and re-trying connections. I am not sure 
that we want to update existing tests with this.


I think it is better to focus on DTLS testing, and make tests more 
stable. That's why I am proposing to use custom UDP sockets. If we want 
to test real connections, then it should be different tests which take 
into account #1 and #2 above.


Artem

On 06/27/2016 06:25 PM, Xuelei Fan wrote:

On 6/28/2016 9:12 AM, Artem Smotrakov wrote:

Hello,

Please review this patch for javax/net/ssl/DTLS tests.

A couple of DTLS tests fail intermittently on Mac with timeout or "Too
many handshake loops ..." error. The tests use UDP to transfer DTLS
records. It happens sometimes that server cannot receive UDP packets
with DatagramSocket.receive() method even if client tries to re-send
them multiple times. Please see logs in the bug.

At the moment, it is not clear why UDP packets can't be delivered to
server. If someone has seen something similar before, or has some ideas
what might be the root cause, please let me know.


UDP is not reliable.  It could happen that the packets get lost.


I think that the tests and DTLS implementation are fine here. Since the
tests are for DTLS, we can workaround this issue by avoiding real UDP
connections. To avoid changing test logic much, we can use a custom
DatagramSocketImpl and DatagramSocketImplFactory implementations to
replace system UDP implementation.

The patch below updates the tests with the following:
- added custom DatagramSocketImpl and DatagramSocketImplFactory
implementations to avoid real UDP connections

Tests for real connections in practice are needed.  If we update this
test this way, we need to add other tests to test real application
usage.  I don't think this is the right direction to avoid real UDP
connections.


- added more test output, so that we can have more info it the tests
fail again


I agree with this point.

Xuelei


I have run javax/net/ssl/DTLS/CipherSuite.java test in a loop on Mac,
and I didn't see it failed. I also have run javax/net/ssl/DTLS tests on
all supported generic platforms, and they worked fine.

Bug: https://bugs.openjdk.java.net/browse/JDK-8159416
http://cr.openjdk.java.net/~asmotrak/8159416/webrev.00/

Artem

[9] RFR: 8159416: javax/net/ssl/DTLS/CipherSuite.java failed on timeout

[Artem Smotrakov]

Hello,

Please review this patch for javax/net/ssl/DTLS tests.

A couple of DTLS tests fail intermittently on Mac with timeout or "Too 
many handshake loops ..." error. The tests use UDP to transfer DTLS 
records. It happens sometimes that server cannot receive UDP packets 
with DatagramSocket.receive() method even if client tries to re-send 
them multiple times. Please see logs in the bug.


At the moment, it is not clear why UDP packets can't be delivered to 
server. If someone has seen something similar before, or has some ideas 
what might be the root cause, please let me know.


I think that the tests and DTLS implementation are fine here. Since the 
tests are for DTLS, we can workaround this issue by avoiding real UDP 
connections. To avoid changing test logic much, we can use a custom 
DatagramSocketImpl and DatagramSocketImplFactory implementations to 
replace system UDP implementation.


The patch below updates the tests with the following:
- added custom DatagramSocketImpl and DatagramSocketImplFactory 
implementations to avoid real UDP connections
- added more test output, so that we can have more info it the tests 
fail again


I have run javax/net/ssl/DTLS/CipherSuite.java test in a loop on Mac, 
and I didn't see it failed. I also have run javax/net/ssl/DTLS tests on 
all supported generic platforms, and they worked fine.


Bug: https://bugs.openjdk.java.net/browse/JDK-8159416
http://cr.openjdk.java.net/~asmotrak/8159416/webrev.00/

Artem

Re: [9] RFR: 8152745: javax/net/ssl/TLS/TestJSSE.java fails intermittently: Unsupported or unrecognized SSL message

[Artem Smotrakov]

Hi Xuelei,

Thank you for review. Please see inline.

On 06/21/2016 05:57 PM, Xuelei Fan wrote:

Looks more like a issue of "Unsupported or unrecognized SSL message".
This issue might be impacted by other test cases that running in the
same time that using free socket port.  It's good to dump the SSL/TLS
record for further evaluation.

Minor comments:

test/javax/net/ssl/TLS/TestJSSE.java
   36 System.setProperty("javax.net.debug", "ssl");
What do you think if dump the TLS record, too?
System.setProperty("javax.net.debug", "ssl,record");
Sure, "ssl,record" is much better. I checked that it doesn't result to 
"overflow output".


test/javax/net/ssl/TLS/CipherTestUtils.java
line 431-433:  Looks like you missed the print the string array.

Sure, thank you!


line 379, 498, 509: may exceed 80 characters each line.  Please also
look at other updated that may exceed 80 characters each line.
Right, a couple of lines are about 81-85 characters. I am not a fan of 
long lines, but I think they may exceed 80 characters if it makes them 
more readable. I know that some people use vi, so in some environment 
they may have problems with lines more than 80 characters.


If I recall correctly, there was a discussion about new Java coding 
guidelines, but I am not sure it has been done. I know about the 
following draft, but not sure if it is official


http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html

It has a recommendation about line lengths, but it allows lines more 
than 80 characters if it makes it more readable.


Anyway, I avoided lines more than 80 characters (mostly I renamed 
variables).


Please take a look at updated webrev:

http://cr.openjdk.java.net/~asmotrak/8152745/webrev.01/

Artem


On 6/22/2016 1:20 AM, Artem Smotrakov wrote:

Hello,

Please review the patch below for javax/net/ssl/TLS/TestJSSE.java test.

The test has been observed to fail intermittently with "Unsupported or
unrecognized SSL" error. But I couldn't reproduce it manually while
running the test in  a loop for a couple of days on Linux x64. For now
it's not clear why it failed.

The patch updates the test with the following:
- enabled debug output, so that we can have more info if the test fails
again
- splitted the test to several files to avoid jtreg's "output overflow"

Hm, a sort of hack code as these files are not actually 'java' code.
But it should work, I think.

Thanks,
Xuelei


- updated the test to close sockets
- some refactoring to make the code/logs more readable (although I
believe the test needs even more refactoring)

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

Artem