RFR 8044199: tests for RSA keys and key specifications

[Valerie Peng]

Sure, can u please send me the putback message/comment that u would like 
to use?
And your OpenJDK user name if u have one.
Thanks,
Valerie

On 9/8/2015 2:44 PM, Tristan Yan wrote:
> 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/%7Etyan/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/
>>>
>>> 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
>>> />>/  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/~tyan/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/880f6135/attachment.htm>