Hi, Tristan,
The updated webrev addressed most of my previous comments except the few
that I noted below:
<Overall>
- Can u please make sure that all the Cipher objects are retrieved using
"SunJCE" provider? 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().
<CICO_PBE_SKIP_Test.java>
- both the proceedSkipTestUsingXXX() methods should check to ensure that
"SAVE" number of bytes are read.
<CICOSkipTest.java>
- line 217 is redundant
Thanks,
Valerie
On 9/21/2015 10:14 AM, Tristan Yan wrote:
Thank you Valerie. Please review the updated version of it
http://cr.openjdk.java.net/~tyan/8048604/webrev.02/
<http://cr.openjdk.java.net/%7Etyan/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/
bug : https://bugs.openjdk.java.net/browse/JDK-8048604
Thank you very much
Tristan
