<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><font face="Menlo" class="">Thank you Valerie. </font><span style="font-family: Menlo;" class="">Please review the updated version of it</span><div class=""><a href="http://cr.openjdk.java.net/~tyan/8048604/webrev.02/" class=""><font face="Menlo" class="">http://cr.openjdk.java.net/~tyan/8048604/webrev.02/</font></a></div><div class=""><font face="Menlo" class=""><br class=""></font></div><div class=""><font face="Menlo" class="">Cheers</font></div><div class=""><font face="Menlo" class="">Tristan</font></div><div class=""><font face="Menlo" class=""><br class=""></font><div class=""><blockquote type="cite" class=""><div class=""><font face="Menlo" class="">On Sep 16, 2015, at 3:24 PM, Valerie Peng <<a href="mailto:valerie.peng@oracle.com" class="">valerie.peng@oracle.com</a>> wrote:</font></div><font face="Menlo" class=""><br class="Apple-interchange-newline"></font><div class="">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type" class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<font face="Menlo" class=""><br class="">
Can u please make sure that all the Cipher objects are retrieved
using "SunJCE" provider?<br class="">
I noticed some inconsistencies here and there.<br class="">
<br class="">
<TextPKCS5PaddingTest.java><br class="">
- line 94, 'provider' object should be used here.<br class="">
- need @library tag to find the TestUtilities class, otherwise, the
compilation fails.<br class="">
<br class="">
<PbeAlgorithm.java><br class="">
- variable "model" should be named "mode".<br class="">
- nit: rename the class to PBEAlgorithm for consistency.<br class="">
<br class="">
<AbstractPBEWrapper.java><br class="">
- line 98 has a typo, I think u meant "or" instead of "of"<br class="">
<br class="">
<PBKDF2Wrapper.java><br class="">
- typo on line51: TANSFORMATION should be TRANSFORMATION<br class="">
- line 94, why not just init with mode directly as in other Wrapper
classes?<br class="">
<br class="">
<CICO_PBE_Test.java><br class="">
- line 63, variable name normally starts with lower case. Can u fix
it for better readability? Same goes for other PBE tests.<br class="">
<br class="">
<CICO_PBE_SKIP_Test.java><br class="">
- the test class description is not very readable and contains a few
typos. Can u please double check?<br class="">
- typo on lin 132 - decript<br class="">
- both the proceedSkipTestUsingXXX() methods should check to ensure
that "SAVE" number of bytes are read.<br class="">
<br class="">
<CICO_PBE_RW_Test.java><br class="">
- line 79 doesn't look right at all. The comparison should be made
against the output written to output stream instead of itself.<br class="">
</font><pre class=""><font face="Menlo" class="">if (!TestUtilities.equalsBlock(plainText, plainText, TEXT_SIZE)) {</font></pre><font face="Menlo" class="">
That's all.<br class="">
Valerie<br class="">
<br class="">
On 9/15/2015 8:15 PM, Valerie Peng wrote:
</font><blockquote cite="mid:55F8DEE4.5020404@oracle.com" type="cite" class="">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type" class="">
<font face="Menlo" class=""><br class="">
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 class="">
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 class="">
<br class="">
Some nit - If tests are already placed under CICO directory, their
names do not need to contain CICO.<br class="">
Also, maybe u can just place the tests under
com/sun/crypto/provider/CICO instead of
com/sun/crypto/provider/Cipher/CICO.<br class="">
<br class="">
<TestUtilities.java><br class="">
- 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 class="">
The description on line 50 could use some more words to explain
what this method does without reading through the code.<br class="">
<br class="">
<CICOChainingTest.java><br class="">
- after line 85, check that there are no further data available
after reading "recoveredText".<br class="">
<br class="">
<CICODESFuncTest.java><br class="">
- line 63, "is not exist" should be "does not exist" or "not
found".<br class="">
- 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 class="">
- the check from line106-110 should be moved up to right after the
Cipher.getInstance() calls on line 97 and 98.<br class="">
<br class="">
<ReadModel.java><br class="">
- 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 class="">
- the variable "ci1" should really be named "ciIn" so that it's
clear that this argument is the cipher associated with the
CipherInputStream. <br class="">
<br class="">
<CICOSkipTest.java><br class="">
- line 124, "blockLen" should really be "numOfBlocks".<br class="">
- 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 class="">
- line 178-180 seems redundant? I think they can just be removed.<br class="">
- line 184, why is the key length check being done here again
inside the same method? This one for sure is useless.<br class="">
- 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 class="">
- line 217 and 234, 45 should be 'save'<br class="">
<br class="">
<TextPKCS5PaddingTest.java><br class="">
- 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 class="">
<br class="">
I will send u comments on PBE ones in a separate email.<br class="">
Thanks,<br class="">
Valerie<br class="">
<br class="">
On 8/12/2015 4:06 PM, Tristan Yan wrote:
</font><blockquote cite="mid:55CBD193.5020504@oracle.com" type="cite" class="">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8" class="">
<font face="Menlo" class=""><font class="">Please be free review these new tests for
strong crypto ciphers.<br class="">
<br class="">
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 class="">
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 class="">
<br class="">
Thank you very much<br class="">
Tristan<br class="">
</font> </font></blockquote>
</blockquote>
</div>
</div></blockquote></div><br class=""></div></body></html>