Hi Sean/Adam,

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

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

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.

Thanks,
Siba

-----Original Message-----
From: Sean Mullan 
Sent: Thursday, April 05, 2018 8:46 PM
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

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

Reply via email to