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