RFR for JDK-8048604 : Tests for strong crypto ciphers

[Valerie Peng]

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/20150916/7680c93c/attachment.htm>