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/~tyan/8048604/webrev.03/> Tristan
> On Sep 22, 2015, at 4:30 PM, Valerie Peng <valerie.p...@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.p...@oracle.com >>> <mailto:valerie.p...@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/ >>>>> <http://cr.openjdk.java.net/%7Etyan/8048604/webrev.01/> >>>>> bug : https://bugs.openjdk.java.net/browse/JDK-8048604 >>>>> <https://bugs.openjdk.java.net/browse/JDK-8048604> >>>>> >>>>> Thank you very much >>>>> Tristan >>
