RFR for JDK-8048604 : Tests for strong crypto ciphers

Wed Sep 23 17:46:20 UTC 2015

The updated webrev looks good.
Valerie

On 9/23/2015 9:50 AM, Tristan Yan wrote:
> Thanks Valerie, I fixed with new version, please review it again.
> http://cr.openjdk.java.net/~tyan/8048604/webrev.03/ 
> <http://cr.openjdk.java.net/%7Etyan/8048604/webrev.03/>
> Tristan
>
>> On Sep 22, 2015, at 4:30 PM, Valerie Peng <valerie.peng at oracle.com 
>> <mailto:valerie.peng at oracle.com>> wrote:
>>
>> Hi, Tristan,
>>
>> The updated webrev addressed most of my previous comments except the 
>> few that I noted below:
>>
>> <Overall>
>> - Can u please make sure that all the Cipher objects are retrieved 
>> using "SunJCE" provider? At least the AESPBEWrapper.java, 
>> CICOSkipTest.java (there may be more as I didn't check all) didn't 
>> specify a provider when calling Cipher.getInstance().
>>
>> <CICO_PBE_SKIP_Test.java>
>> - both the proceedSkipTestUsingXXX() methods should check to ensure 
>> that "SAVE" number of bytes are read.
>>
>> <CICOSkipTest.java>
>> - line 217 is redundant
>>
>> Thanks,
>> Valerie
>>
>> On 9/21/2015 10:14 AM, Tristan Yan wrote:
>>> Thank you Valerie. Please review the updated version of it
>>> http://cr.openjdk.java.net/~tyan/8048604/webrev.02/ 
>>> <http://cr.openjdk.java.net/%7Etyan/8048604/webrev.02/>
>>>
>>> Cheers
>>> Tristan
>>>
>>>> On Sep 16, 2015, at 3:24 PM, Valerie Peng <valerie.peng at oracle.com 
>>>> <mailto:valerie.peng at oracle.com>> wrote:
>>>>
>>>>
>>>> Can u please make sure that all the Cipher objects are retrieved 
>>>> using "SunJCE" provider?
>>>> I noticed some inconsistencies here and there.
>>>>
>>>> <TextPKCS5PaddingTest.java>
>>>> - line 94, 'provider' object should be used here.
>>>> - need @library tag to find the TestUtilities class, otherwise, the 
>>>> compilation fails.
>>>>
>>>> <PbeAlgorithm.java>
>>>> - variable "model" should be named "mode".
>>>> - nit: rename the class to PBEAlgorithm for consistency.
>>>>
>>>> <AbstractPBEWrapper.java>
>>>> -  line 98 has a typo, I think u meant "or" instead of "of"
>>>>
>>>> <PBKDF2Wrapper.java>
>>>> - typo on line51: TANSFORMATION should be TRANSFORMATION
>>>> - line 94, why not just init with mode directly as in other Wrapper 
>>>> classes?
>>>>
>>>> <CICO_PBE_Test.java>
>>>> - line 63, variable name normally starts with lower case. Can u fix 
>>>> it for better readability? Same goes for other PBE tests.
>>>>
>>>> <CICO_PBE_SKIP_Test.java>
>>>> - the test class description is not very readable and contains a 
>>>> few typos. Can u please double check?
>>>> - typo on lin 132 - decript
>>>> - both the proceedSkipTestUsingXXX() methods should check to ensure 
>>>> that "SAVE" number of bytes are read.
>>>>
>>>> <CICO_PBE_RW_Test.java>
>>>> - line 79 doesn't look right at all. The comparison should be made 
>>>> against the output written to output stream instead of itself.
>>>> if (!TestUtilities.equalsBlock(plainText, plainText, TEXT_SIZE)) {
>>>> That's all.
>>>> Valerie
>>>>
>>>> On 9/15/2015 8:15 PM, Valerie Peng wrote:
>>>>>
>>>>> Most of the tests are DES, DESede, and Blowfish and they are 
>>>>> written in a way which won't work with AES due to the larger block 
>>>>> size as the block size 8 is dispersed through out the tests 
>>>>> instead of as a constant.
>>>>> At some point, I think we need to enhance these tests to cover AES 
>>>>> instead of the legacy algorithms such as DES, DESede, etc. Can u 
>>>>> define a constant for the block size and replace 8 with this constant?
>>>>>
>>>>> Some nit - If tests are already placed under CICO directory, their 
>>>>> names do not need to contain CICO.
>>>>> Also, maybe u can just place the tests under 
>>>>> com/sun/crypto/provider/CICO instead of 
>>>>> com/sun/crypto/provider/Cipher/CICO.
>>>>>
>>>>> <TestUtilities.java>
>>>>> - if my reading of the code is right, the equalsBlockSave(byte[] 
>>>>> b1, byte[]b2, int bLen, int save) method compares b1 and b2 by 
>>>>> chopping up b1 into blocks of 'bLen' and b2 into blocks of 'save', 
>>>>> and then compare the first 'save' bytes of each block to make sure 
>>>>> they are equal. line 59 looks incorrect -  the number of blocks 
>>>>> should be computed using b1.length instead of b2.length. The term 
>>>>> "save" seems confusing too. Maybe "partial" would be more 
>>>>> suitable? Or maybe changing "bLen" to "b1Len", and "save" to "b2Len".
>>>>> The description on line 50 could use some more words to explain 
>>>>> what this method does without reading through the code.
>>>>>
>>>>> <CICOChainingTest.java>
>>>>> - after line 85, check that there are no further data available 
>>>>> after reading "recoveredText".
>>>>>
>>>>> <CICODESFuncTest.java>
>>>>> - line 63, "is not exist" should be "does not exist" or "not found".
>>>>> - the comments on line 67-68 and line 77 seems contradictory to 
>>>>> each other. Essentially, NoPadding is tested for all modes vs 
>>>>> PKCS5Padding is tested for some modes.
>>>>> - the check from line106-110 should be moved up to right after the 
>>>>> Cipher.getInstance() calls on line 97 and 98.
>>>>>
>>>>> <ReadModel.java>
>>>>> - line 74, instead of byte[] plainText, I think it's clearer to 
>>>>> just have "int inputLen". The content of the input array is not 
>>>>> used in any of the enum values, but rather just the length for 
>>>>> output buffer allocation.
>>>>> - the variable "ci1" should really be named "ciIn" so that it's 
>>>>> clear that this argument is the cipher associated with the 
>>>>> CipherInputStream.
>>>>>
>>>>> <CICOSkipTest.java>
>>>>> - line 124, "blockLen" should really be "numOfBlocks".
>>>>> - Will LengthLimitException ever be thrown for this test? Given 
>>>>> that you are only using default key length, I doubt the check on 
>>>>> line 163 will be true.
>>>>> - line 178-180 seems redundant? I think they can just be removed.
>>>>> - line 184, why is the key length check being done here again 
>>>>> inside the same method? This one for sure is useless.
>>>>> - Instead of 2 constructors with comments indicating what ciphers 
>>>>> they are for, it's better to just use static factory method. The 
>>>>> implementation isn't that different, they both return a pair of 
>>>>> ciphers. U can handle the different parameter type and secret key 
>>>>> generation using a switch construct based on the specified 
>>>>> algorithm 'algo'.
>>>>> - line 217 and 234, 45 should be 'save'
>>>>>
>>>>> <TextPKCS5PaddingTest.java>
>>>>> - the testPaddingScheme() method doesn't seem too useful as it is 
>>>>> testing the PKCS5Padding inside the test class itself. How would 
>>>>> this detect any regression in SunJCE provider? I understand that 
>>>>> the test may have problem accessing the actual PKCS5Padding class 
>>>>> inside the SunJCE provider, but still, copy-n-paste the internal 
>>>>> class out into test class is meaningless. This test method and the 
>>>>> cut-n-pasted classes should be removed.
>>>>>
>>>>> I will send u comments on PBE ones in a separate email.
>>>>> Thanks,
>>>>> Valerie
>>>>>
>>>>> On 8/12/2015 4:06 PM, Tristan Yan wrote:
>>>>>> Please be free review these new tests for strong crypto ciphers.
>>>>>>
>>>>>> webrev : http://cr.openjdk.java.net/~tyan/8048604/webrev.01/
>>>>>> bug    : https://bugs.openjdk.java.net/browse/JDK-8048604
>>>>>>
>>>>>> Thank you very much
>>>>>> Tristan
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20150923/d4219410/attachment.htm>