Thank you Valerie. Please review the updated version of it http://cr.openjdk.java.net/~tyan/8048604/webrev.02/ <http://cr.openjdk.java.net/~tyan/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
