<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
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.<br>
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?<br>
<br>
Some nit - If tests are already placed under CICO directory, their
names do not need to contain CICO.<br>
Also, maybe u can just place the tests under
com/sun/crypto/provider/CICO instead of
com/sun/crypto/provider/Cipher/CICO.<br>
<br>
<TestUtilities.java><br>
- 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".<br>
The description on line 50 could use some more words to explain what
this method does without reading through the code.<br>
<br>
<CICOChainingTest.java><br>
- after line 85, check that there are no further data available
after reading "recoveredText".<br>
<br>
<CICODESFuncTest.java><br>
- line 63, "is not exist" should be "does not exist" or "not found".<br>
- 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.<br>
- the check from line106-110 should be moved up to right after the
Cipher.getInstance() calls on line 97 and 98.<br>
<br>
<ReadModel.java><br>
- 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.<br>
- the variable "ci1" should really be named "ciIn" so that it's
clear that this argument is the cipher associated with the
CipherInputStream. <br>
<br>
<CICOSkipTest.java><br>
- line 124, "blockLen" should really be "numOfBlocks".<br>
- 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.<br>
- line 178-180 seems redundant? I think they can just be removed.<br>
- line 184, why is the key length check being done here again inside
the same method? This one for sure is useless.<br>
- 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'.<br>
- line 217 and 234, 45 should be 'save'<br>
<br>
<TextPKCS5PaddingTest.java><br>
- 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.<br>
<br>
I will send u comments on PBE ones in a separate email.<br>
Thanks,<br>
Valerie<br>
<br>
On 8/12/2015 4:06 PM, Tristan Yan wrote:
<blockquote cite="mid:55CBD193.5020504@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<font face="monospace">Please be free review these new tests for
strong crypto ciphers.<br>
<br>
webrev : <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etyan/8048604/webrev.01/">http://cr.openjdk.java.net/~tyan/8048604/webrev.01/</a><br>
bug : <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8048604">https://bugs.openjdk.java.net/browse/JDK-8048604</a><br>
<br>
Thank you very much<br>
Tristan<br>
</font> </blockquote>
</body>
</html>