<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<font class="" face="Menlo">Hi, Tristan,<br>
<br>
The updated webrev addressed most of my previous comments except
the few that I noted below:<br>
</font><font class="" face="Menlo"><br>
<Overall><br>
- Can u please make sure that all the Cipher objects are retrieved
using "SunJCE" provider?</font> 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().<br>
<font class="" face="Menlo"><br>
<CICO_PBE_SKIP_Test.java><br class="">
- both the proceedSkipTestUsingXXX() methods should check to
ensure that "SAVE" number of bytes are read.</font><br>
<font class="" face="Menlo"> </font><br>
<font class="" face="Menlo"><CICOSkipTest.java><br>
- line 217 is redundant<br>
<br>
Thanks,<br>
Valerie<br>
<br>
</font>On 9/21/2015 10:14 AM, Tristan Yan wrote:
<blockquote
cite="mid:10EA3908-7CB8-43A9-ACA6-F3512F39D75D@oracle.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<font class="" face="Menlo">Thank you Valerie. </font><span
style="font-family: Menlo;" class="">Please review the updated
version of it</span>
<div class=""><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Etyan/8048604/webrev.02/"
class=""><font class="" face="Menlo">http://cr.openjdk.java.net/~tyan/8048604/webrev.02/</font></a></div>
<div class=""><font class="" face="Menlo"><br class="">
</font></div>
<div class=""><font class="" face="Menlo">Cheers</font></div>
<div class=""><font class="" face="Menlo">Tristan</font></div>
<div class=""><font class="" face="Menlo"><br class="">
</font>
<div class="">
<blockquote type="cite" class="">
<div class=""><font class="" face="Menlo">On Sep 16, 2015,
at 3:24 PM, Valerie Peng <<a moz-do-not-send="true"
href="mailto:valerie.peng@oracle.com" class="">valerie.peng@oracle.com</a>>
wrote:</font></div>
<font class="" face="Menlo"><br
class="Apple-interchange-newline">
</font>
<div class="">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type" class="">
<div bgcolor="#FFFFFF" text="#000000" class=""> <font
class="" face="Menlo"><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 class="" face="Menlo">if (!TestUtilities.equalsBlock(plainText, plainText, TEXT_SIZE)) {</font></pre>
<font class="" face="Menlo"> 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=ISO-8859-1"
http-equiv="Content-Type" class="">
<font class="" face="Menlo"><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=ISO-8859-1" class="">
<font class="" face="Menlo"><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>
</blockquote>
</body>
</html>