RFR 8044199: tests for RSA keys and key specifications

[Tristan Yan]

Hi Valerie

Thank you for reviewing this, I fixed 2 points in latest webrev. Do you mind sponsor this for me if you’re OK with this version.
http://cr.openjdk.java.net/~tyan/8044199/webrev.03/ <http://cr.openjdk.java.net/~tyan/8044199/webrev.03/>
Cheers.
Tristan

> On Aug 28, 2015, at 2:03 PM, Valerie Peng <valerie.peng at oracle.com <mailto:valerie.peng at oracle.com>> wrote:
> 
> Hi Tristan,
> 
> Mostly fine, just 2 more nits:
> 1) KeySizeTest: the @test and @bug are still on the same line.
> 2) SpecTest.java: the @run command from line 40 - 45 still have the old name, e.g. KeyModulusTest? I think u need to match them.
> 
> Thanks,
> Valerie
> On 8/20/2015 9:31 AM, Tristan Yan wrote:
>> 
>> Thanks Valerie
>> 
>> Please review the fixed version.
>> 
>> http://cr.openjdk.java.net/~tyan/8044199/webrev.02/ <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.02/>
>> 
>> Tristan
>> 
>> 
>> > Tristan,
>> > 
>> > - Given that the tests are against SunRsaSign provider, I think 
>> > sun/security/rsa subdirectory is a better choice than java/security/rsa.
>> > - Put @test and @bug on separate lines
>> > - Elaborate more on what is tested and what is the expected result in 
>> > the bug record. The current content isn't very clear. 
>> > - Can u have more unique names for these tests? Currently, they are too 
>> > similarly named and it's somewhat confusing. The directory already 
>> > contains RSA, I don't feel that u need to have RSA again in the names of 
>> > the test especially when SunRsaSign provider doesn't support non-RSA 
>> > algorithms.
>> > 
>> > For KeyTest.java
>> > 1) typo on line 69, rsaPrivateKey2 should be rsaPrivateKey
>> > 2) This test is based on the> 
>>  provider specific behavior that CRT key is 
>> > generated by default for RSA. It'd be much clearer if you can add an 
>> > instanceof check on the generated RSA private key objects before the 
>> > KeyFactory + RSAPrivateKeySpec code. In addition, add additional code to 
>> > check for equality with the matching KeySpec (e.g. RSAPrivateCrtKeySpec 
>> > for RSAPrivateCrtKey) is used.
>> > 3) line 58, add a space between '//" and text.
>> > 
>> > For RSAKeySizeTest.java
>> > 1) specify provider "SunRSASign" for all getInstance() calls.
>> > 2) line 55 can be moved up to before the for-loop.
>> > 3) instead of using toString(int), probably bitLength() would be a 
>> > better choice.
>> > 4) sizeTest() doesn't really check size but rather it compares the 
>> > modulus values being equal. This check can be moved to RSAKeySizeTest.java.
>> > 
>> > For RSATest.java
>> > 1) specify provider "SunRSASign" for all getInstance() calls.
>> > 
>> > Thanks,
>> > Valerie
>> > 
>> > On 8/3/2015 10:27 AM, Tristan Yan wrote:
>> >> Hi
>> >>
>> >> Please review new tests for RSA keys and key specifications
>> >>
>> >> Bug:https://bugs.openjdk.java.net/browse/JDK-8044199 <https://bugs.openjdk.java.net/browse/JDK-8044199>
>> >> Webrev:http://cr.openjdk.java.net/~tyan/8044199/webrev.01/ <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.01/>  <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.01/ <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.01/>>  <http://cr.openjdk.java.net/~tyan/8044199/webrev.01/ <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.01/>  <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.01/ <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.01/>>>
>> >>
>> >> Thanks
>> >> Tristan
>> >>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20150908/2fe54443/attachment.htm>