RE: RFR: 8200219: Develop new tests for using new elliptic curves: curve25519 and curve448

[Sibabrata Sahoo]

Hi Sean/Adam,

Please review the updated patch,
Webrev: http://cr.openjdk.java.net/~ssahoo/8184359/webrev.02/

Now there is only 1 Test file in the deleted list which has duplicate code. Due 
to that I have pointed the older JBS bug ID JDK-4936763 in 
KeyAgreementTest.java and linked the same to JDK-8184359 too.

Reverted the duplicate code merge in KeySizeTest.java due to performance issue 
observed for higher key sizes. That ensured SupportedDHKeys.java & 
UnsupportedDHKeys.java will not be removed as it was provided in previous patch.

Thanks,
Siba

-Original Message-
From: Sean Mullan 
Sent: Wednesday, April 11, 2018 1:42 AM
To: Sibabrata Sahoo <sibabrata.sa...@oracle.com>; Adam Petcher 
<adam.petc...@oracle.com>; security-dev@openjdk.java.net
Subject: Re: RFR: 8200219: Develop new tests for using new elliptic curves: 
curve25519 and curve448

On 4/9/18 10:13 AM, Sibabrata Sahoo wrote:
> Here is the new patch addressing all comments.
> Webrev:http://cr.openjdk.java.net/~ssahoo/8184359/webrev.01/
> 
> Changes includes:
> 1)
> KeyAgreementTest.java
> KeySizeTest.java
>I have ensured no duplicate Test cases exist for the functionality 
> provided in these 2 files. Due to that I have remove 3 files, 
> DHGenSecretKey.java, SupportedDHKeys.java, UnsupportedDHKeys.java from 
> "open/test/jdk/com/sun/crypto/provider/KeyAgreement" folder. 

For DHGenSecretKey, can you add the bugid of 4936763 to the test file in this 
webrev where the changes were merged and also link the bugs together in JBS? 
That way we know that these tests are now covering the fixes for that bug. For 
the others, see below.

> Statements which affected due to merging the missing code from these deleted 
> files are KeyAgreementTest.java[29, 141], KeySizeTest.java[91-100, 112-162]. 
> Also I have observed that KeySizeTest.java Test takes around 2 minute to 
> complete in SOME PLATFORM to complete all 14 @run and approximately 20 
> seconds for higher keys > 4096 for "DiffieHellman" only after merge. Please 
> suggest if removing the higher Keys from the Test cases will help or 20-30 
> seconds for these higher keys is accepted here?

It's a bit concerning. One way to make the tests run faster is to split them up 
into separate tests so that they can be run concurrently by jtreg. So we might 
be making things worse by merging them :(

So I would suggest just merging DHGenSecretKey and leaving the other tests 
as-is.

> 2) I have updated the comment in KeyAgreementTest.java[129] to indicate 
> provider search order.

Ok.

--Sean

> As per Adam's comments the following changes,
> 3) KeySizeTest.java & NegativeTest.java, which need utility methods from 
> Convert.java are using the library instead of defining it's own.
> 4) MultiThreadTest.java generates two key pairs and do two key agreement 
> operations.

Re: RFR: 8200219: Develop new tests for using new elliptic curves: curve25519 and curve448

[Sean Mullan]

On 4/9/18 10:13 AM, Sibabrata Sahoo wrote:

Here is the new patch addressing all comments.
Webrev:http://cr.openjdk.java.net/~ssahoo/8184359/webrev.01/

Changes includes:
1)
KeyAgreementTest.java
KeySizeTest.java
   I have ensured no duplicate Test cases exist for the functionality provided in these 2 files. Due to that I have remove 3 files, DHGenSecretKey.java, SupportedDHKeys.java, UnsupportedDHKeys.java from "open/test/jdk/com/sun/crypto/provider/KeyAgreement" folder. 


For DHGenSecretKey, can you add the bugid of 4936763 to the test file in 
this webrev where the changes were merged and also link the bugs 
together in JBS? That way we know that these tests are now covering the 
fixes for that bug. For the others, see below.



Statements which affected due to merging the missing code from these deleted files are 
KeyAgreementTest.java[29, 141], KeySizeTest.java[91-100, 112-162]. Also I have observed that 
KeySizeTest.java Test takes around 2 minute to complete in SOME PLATFORM to complete all 14 
@run and approximately 20 seconds for higher keys > 4096 for "DiffieHellman" 
only after merge. Please suggest if removing the higher Keys from the Test cases will help 
or 20-30 seconds for these higher keys is accepted here?


It's a bit concerning. One way to make the tests run faster is to split 
them up into separate tests so that they can be run concurrently by 
jtreg. So we might be making things worse by merging them :(


So I would suggest just merging DHGenSecretKey and leaving the other 
tests as-is.



2) I have updated the comment in KeyAgreementTest.java[129] to indicate 
provider search order.


Ok.

--Sean


As per Adam's comments the following changes,
3) KeySizeTest.java & NegativeTest.java, which need utility methods from 
Convert.java are using the library instead of defining it's own.
4) MultiThreadTest.java generates two key pairs and do two key agreement 
operations.

Re: RFR: 8200219: Develop new tests for using new elliptic curves: curve25519 and curve448

[Sean Mullan]

Comments below ...

On 4/2/18 4:07 AM, Sibabrata Sahoo wrote:

Hi Sean,

My comments In-lined..

Thanks,
Siba

-Original Message-
From: Sean Mullan
Sent: Saturday, March 31, 2018 12:13 AM
To: Sibabrata Sahoo <sibabrata.sa...@oracle.com>; Adam Petcher 
<adam.petc...@oracle.com>; security-dev@openjdk.java.net
Subject: Re: RFR: 8200219: Develop new tests for using new elliptic curves: 
curve25519 and curve448

A few comments so far; have not finished my review yet.

General comment:

Many of these tests test more than XDH. That is fine and good for increasing 
coverage, but have you looked through existing tests to see if you are 
duplicating anything we are already testing and maybe those tests could be 
removed or you could share the same code. One of the things we should be 
looking at is to figure out how to reduce the overall time the security tests 
take.

There are few Lines of code related to " DiffieHellman " are duplicate in KeyAgreementTest.java and KeySizeTest.java 
which are available in 2 existing Test folders. i.e. "open\test\jdk\sun\security\pkcs11\KeyAgreement" and 
"open\test\jdk\com\sun\crypto\provider\KeyAgreement". While there is no equivalent Tests for the same for 
"ECDH" and "XDH". The remaining files available in the webrev are mostly new. Our initial thinking was to 
have Test files covering all similar algorithms in one place. In that case I may remove 2-3 existing Test files inside these 
folders with next patch.


Ok.


* KeyAgreementTest.java

128 // Uses platform supported provider to test interoperability.

What do you mean by "platform supported provider"? Isn't this based on the 
provider search order? So in some cases, you might be testing against the same provider 
and not really doing interop testing?

Yes my thinking here is Provider search order. I may need to change the comment 
appropriately. Here the intention is not really interop based Test unless a 
different provider found other than SunJCE.


Ok.



* KeySizeTest.java

You are generating some large keys - any issues with test timeouts? Do we need 
to test the generation of the keypairs? Could we use cached keypairs instead?

Yes here my intention is to test generation of keypairs with different key 
sizes too. I have ran these Test files several times. I have not seen these 
test files taking more times than few couple of seconds.


Ok.

I looked at the other tests and have no further comments.

Thanks,
Sean


On 3/26/18 12:38 PM, Sibabrata Sahoo wrote:

Hi,

Please review the patch for,

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

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

All the Test files uses KeyAgreement, KeyPairGenerator, Several
KeySpecs from SunJCE library to Test DiffieHellman, ECDH and XDH with
curve25519 and curve448 algorithms. Each Test files try to address
several cases and the purpose of each has been commented in their own files.

Thanks,

Siba

RE: RFR: 8200219: Develop new tests for using new elliptic curves: curve25519 and curve448

[Sibabrata Sahoo]

Hi Sean,

My comments In-lined..

Thanks,
Siba

-Original Message-
From: Sean Mullan 
Sent: Saturday, March 31, 2018 12:13 AM
To: Sibabrata Sahoo <sibabrata.sa...@oracle.com>; Adam Petcher 
<adam.petc...@oracle.com>; security-dev@openjdk.java.net
Subject: Re: RFR: 8200219: Develop new tests for using new elliptic curves: 
curve25519 and curve448

A few comments so far; have not finished my review yet.

General comment:

Many of these tests test more than XDH. That is fine and good for increasing 
coverage, but have you looked through existing tests to see if you are 
duplicating anything we are already testing and maybe those tests could be 
removed or you could share the same code. One of the things we should be 
looking at is to figure out how to reduce the overall time the security tests 
take.

There are few Lines of code related to " DiffieHellman " are duplicate in 
KeyAgreementTest.java and KeySizeTest.java which are available in 2 existing 
Test folders. i.e. "open\test\jdk\sun\security\pkcs11\KeyAgreement" and 
"open\test\jdk\com\sun\crypto\provider\KeyAgreement". While there is no 
equivalent Tests for the same for "ECDH" and "XDH". The remaining files 
available in the webrev are mostly new. Our initial thinking was to have Test 
files covering all similar algorithms in one place. In that case I may remove 
2-3 existing Test files inside these folders with next patch.

* KeyAgreementTest.java

128 // Uses platform supported provider to test interoperability.

What do you mean by "platform supported provider"? Isn't this based on the 
provider search order? So in some cases, you might be testing against the same 
provider and not really doing interop testing?

Yes my thinking here is Provider search order. I may need to change the comment 
appropriately. Here the intention is not really interop based Test unless a 
different provider found other than SunJCE.

* KeySizeTest.java

You are generating some large keys - any issues with test timeouts? Do we need 
to test the generation of the keypairs? Could we use cached keypairs instead?

Yes here my intention is to test generation of keypairs with different key 
sizes too. I have ran these Test files several times. I have not seen these 
test files taking more times than few couple of seconds. 

--Sean


On 3/26/18 12:38 PM, Sibabrata Sahoo wrote:
> Hi,
> 
> Please review the patch for,
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8184359
> 
> Webrev: http://cr.openjdk.java.net/~ssahoo/8184359/webrev.00/
> 
> All the Test files uses KeyAgreement, KeyPairGenerator, Several 
> KeySpecs from SunJCE library to Test DiffieHellman, ECDH and XDH with 
> curve25519 and curve448 algorithms. Each Test files try to address 
> several cases and the purpose of each has been commented in their own files.
> 
> Thanks,
> 
> Siba
> 

Re: RFR: 8200219: Develop new tests for using new elliptic curves: curve25519 and curve448

[Sean Mullan]

A few comments so far; have not finished my review yet.

General comment:

Many of these tests test more than XDH. That is fine and good for 
increasing coverage, but have you looked through existing tests to see 
if you are duplicating anything we are already testing and maybe those 
tests could be removed or you could share the same code. One of the 
things we should be looking at is to figure out how to reduce the 
overall time the security tests take.


* KeyAgreementTest.java

128 // Uses platform supported provider to test interoperability.

What do you mean by "platform supported provider"? Isn't this based on 
the provider search order? So in some cases, you might be testing 
against the same provider and not really doing interop testing?


* KeySizeTest.java

You are generating some large keys - any issues with test timeouts? Do 
we need to test the generation of the keypairs? Could we use cached 
keypairs instead?


--Sean


On 3/26/18 12:38 PM, Sibabrata Sahoo wrote:

Hi,

Please review the patch for,

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

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

All the Test files uses KeyAgreement, KeyPairGenerator, Several KeySpecs 
from SunJCE library to Test DiffieHellman, ECDH and XDH with curve25519 
and curve448 algorithms. Each Test files try to address several cases 
and the purpose of each has been commented in their own files.


Thanks,

Siba

Re: RFR: 8200219: Develop new tests for using new elliptic curves: curve25519 and curve448

[Adam Petcher]

I have a couple of minor comments. I am not a Reviewer, so someone else 
will still need to look at this.


KeySizeTest: You can use the byteArrayToHexString that is in Convert in 
the test lib. See TestXDH.java for an example of how this method is 
imported and used.
MultiThreadTest: In testKeyAgreement, you may want to generate two key 
pairs, do two key agreement operations, and then compare the results. 
Then this test could catch arithmetic errors caused by caching and 
precomputation.



On 3/26/2018 12:38 PM, Sibabrata Sahoo wrote:


Hi,

Please review the patch for,

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

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



All the Test files uses KeyAgreement, KeyPairGenerator, Several 
KeySpecs from SunJCE library to Test DiffieHellman, ECDH and XDH with 
curve25519 and curve448 algorithms. Each Test files try to address 
several cases and the purpose of each has been commented in their own 
files.


Thanks,

Siba