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 <[email protected]>; Adam Petcher
<[email protected]>; [email protected]
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 <[email protected]>; Adam Petcher
> <[email protected]>; [email protected]
> 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
>>
