RFR 8048357: PKCS basic tests

[Amanda Jiang]

Hi Max,

Thanks for reviewing the webrev, below is another updated webrev which 
contains fixes for all your comments, please let me know if you have any 
other suggestions.
http://cr.openjdk.java.net/~amjiang/8048357/webrev.03/

Thanks,
Amanda

On 15/11/15 下午11:41, Wang Weijun wrote:
> Why "othervm" for all these tests?
>
> PKCS10AttrOrder.java:
>
> - Is sun.security.provider and sun.misc used anywhere? You have them in @modules.
>
> - sun.security.pkcs.PKCS9Attribute contains public constants like CONTENT_TYPE_OID, SIGNING_TIME_OID, CHALLENGE_PASSWORD_OID. You can directly use them. Same as PKCS10AttributeReader.java.
>
> - What does "order" mean in the test name? PKCS10 Attributes is a Set, therefore there is no order.
>
> - You print out "List of attributes in constructed PKCS10 request: " but I see no content.
>
> - checkAttributes() ensures every attribute in the encoded PKCS10 has the correct value. But you haven't checked if all entries in constructedMap are encoded. Maybe compare the size?
>
> PKCS10AttributeReader.java:
>
> - sun.misc in @modules?
>
> - Line 45: .asn1 -> asn.1
>
> - Please also compare the size of RequestStander and eReq.
>
> PKCS8Test.java:
>
> - sun.security.x509 in @modules?
This test uses "sun.misc.HexDumpEncoder", so that's why security.x509 is 
in @modules.
>
> - Line 206: Dump encodedKey here? Seems too late. Why not do it before the check or at least before raiseException? Same with the other 4 debug outputs after raiseException.
>
> - EXPECTED_ALG_ID_CHRS can be just a String with \n and \t inside.
>
> pkcs7/*.java:
>
> Please take a look at Bernd Eckenfels's comments on Aug 21. I fully agree with him.
>
> Thanks
> Max
>
> ----
>
>> On Nov 13, 2015, at 4:57 PM, Amanda Jiang <amanda.jiang at oracle.com> wrote:
>>
>> Hi Max,
>>
>> Please check the updated webrev which address your comments, please let me know if you have any other suggestions.
>> http://cr.openjdk.java.net/~amjiang/8048357/webrev.02/
>>
>> Thanks,
>> Amanda
>>
>> On 15/8/21 上午12:13, Weijun Wang wrote:
>>> PKCS10AttrOrder.java:
>>>
>>> - Why not inline revAttributes(), prov() and constructMap()? They are only used once. Putting the content into the main method is more clear.
>>>
>>> - You can create separate method for the while look checks. The 2 look identical.
>>>
>>> PKCS10AttributeReader.java:
>>>
>>> - Is it OK to indent the data in the comment to multiple levels?
>>>
>>> - Again, initMap() not necessary.
>>>
>>> PKCS7VerifyTest.java:
>>>
>>> - Not sure what this is for. The 1st and 2nd argument of the @run line are the same?
>> those 3 arguments are actually for two different test cases, I use two @run tags in updated webrev and add some comments in test to make it clear.
>>> SignerOrder.java:
>>>
>>> - Comment on what derString1 and derString1 are.
>>>
>>> - What do you expect verifs1.length and verifs2.length to be? Will you also check it?
>> Yes I do check verifs1.length and verifs2.length should be same.
>>   121         if (verifs1.length != verifs2.length) {
>>   122             throw new RuntimeException("Length or Original vs read-in "
>>   123                     + "should be same");
>>   124         }
>>
>>
>>
>>> PKCS8Test.java:
>>>
>>> - Typo: s/recieved/received/g
>>>
>>> Thanks
>>> Max
>>>
>>> On 08/21/2015 07:11 AM, Amanda Jiang wrote:
>>>> Hi All,
>>>>
>>>> Please be free to review new tests for conformance testing of PKCS.
>>>>
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8048357
>>>> webrev: http://cr.openjdk.java.net/~amjiang/8048357/webrev.01/
>>>>
>>>> Thanks,
>>>> Amanda
>>>>

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