<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
The updated webrev looks good.<br>
Valerie<br>
<br>
On 9/23/2015 9:50 AM, Tristan Yan wrote:
<blockquote
cite="mid:B066F600-3546-4A57-944E-6618BDDE15A4@oracle.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div class=""><font class="" face="Menlo">Thanks Valerie, </font><span
style="font-family: Menlo;" class="">I fixed with new version</span><span
style="font-family: Menlo;" class="">, please review it again.</span></div>
<div class=""><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Etyan/8048604/webrev.03/"
style="font-family: Menlo;" class="">http://cr.openjdk.java.net/~tyan/8048604/webrev.03/</a></div>
<div class=""><font class="" face="Menlo">Tristan</font></div>
<div class=""><font class="" face="Menlo"><br class="">
</font>
<div>
<blockquote type="cite" class="">
<div class=""><font class="" face="Menlo">On Sep 22, 2015,
at 4:30 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"><font class="">Hi, Tristan,<br
class="">
<br class="">
The updated webrev addressed most of my previous
comments except the few that I noted below:<br
class="">
</font><font class=""><br class="">
<Overall><br class="">
- 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
class="">
<font class=""><br class="">
<CICO_PBE_SKIP_Test.java><br class="">
- both the proceedSkipTestUsingXXX() methods should
check to ensure that "SAVE" number of bytes are
read.</font><br class="">
<font class=""> </font><br class="">
<font class=""><CICOSkipTest.java><br class="">
- line 217 is redundant<br class="">
<br class="">
Thanks,<br class="">
Valerie<br class="">
<br class="">
</font>On 9/21/2015 10:14 AM, Tristan Yan wrote: </font>
<blockquote
cite="mid:10EA3908-7CB8-43A9-ACA6-F3512F39D75D@oracle.com"
type="cite" class="">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1" class="">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1" class="">
<font class="" face="Menlo"><font class="">Thank you
Valerie. </font><span class="">Please review the
updated version of it</span> </font>
<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"><font
class=""><br class="">
</font> </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"><font class=""><br
class="Apple-interchange-newline">
</font> </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"><font 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> </font>
<pre class=""><font class="" face="Menlo">if (!TestUtilities.equalsBlock(plainText, plainText, TEXT_SIZE)) {</font></pre>
<font class="" face="Menlo"><font class="">
That's all.<br class="">
Valerie<br class="">
<br class="">
On 9/15/2015 8:15 PM, Valerie Peng
wrote: </font> </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"><font 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> </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>
<font class="" face="Menlo"><br class="">
</font></div>
</blockquote>
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</blockquote>
</body>
</html>