Hi Valerie,
Thanks for comments! I have fixed all of them.
http://cr.openjdk.java.net/~rhalade/8048601/webrev.01/
- Rajan
On 8/20/15 4:19 PM, Valerie Peng wrote:
Rajan,
Here are some comments for the other half of the webrev:
TestCipherTextLength.java
1) line 82 and 85, why TEXT_LEN is needed? I don't find any usage of
it inside runAll(..) method?
removed.
2) the names here are a bit too similar, e.g. PBEWrapper,
PBECipherWrapper, AESPBEWrapper. The individual subclasses aren't that
big, why not make them inner static classes of the parent class? No
need to repeat the Wrapper and PBE in the subclass names. To match
your other wrapper class DESCipherWrapper, perhaps you can rename the
PBE counterpart to something like:
PBECipherWrapper (parent) with subclasses (defined statically
inside PBECipherWrapper) named PBKDF2, AES, and Legacy (these are
PKCS#5 v1.5 defined algorithms).
fixed, thanks!
PBKDF2Wrapper.java
1) typo on line 43 and 52 - should be T_R_ANSFORMATION
fixed
2) Instead of "PKDF2_DEFAULT_KEY_LEN" (typo? I think u meant PBKDF2),
it should really be CIPHER_KEY_SIZE since the purpose here is to
generate keys for the Cipher object.
renamed the variable.
3) the key that you generated on line 57 can be viewed as an
intermediate key. You could have moved the processing in
initCipher(...) method into the constructor and store the resulting
cipher key into 'key'. For example:
SecretKey derivedKey = keyFactory.generateSecret(pbeKeySpec);
key = new SecretKeySpec(derivedKey.getEncoded(), KEY_ALGORITHM);
key is generated in consturctor.
Thanks,
Valerie
On 8/19/2015 3:38 PM, Valerie Peng wrote:
Rajan,
TestCipher.java
1) this class is used as the base for many algorithm, modes, and
paddings. But then it hardcodes some values or parameter types which
may not work against some algorithms, e.g. IvParameterSpec with
8-byte value, padding length being 8 bytes. PAD_LEN=8 vs
PAD_BYTES=16, is this difference intentional?
Seems to me that this TestCipher class only works against Ciphers w/
8-byte blocks and uses iv. I think it's clearer to have a constant
named BLKSIZE instead of just 8.
Yes. I have changed iv to be final variable now initialized in constructor.
2) Also, u don't need to repeatedly generate random bytes for input
data, IV, etc. The input data can be generated once outside of loop.
As for the parameters, either u can just let the cipher object
generates it or grab the IV values from whatever data that u have
already, e.g. certain part of the input data.
fixed, no need for random anymore.
2) input length is always 800. Would be nice to have a comment
stating the requirement, e.g. multiple of blocks in order to work
against ciphers w/ NoPadding
3) line 54, Would be nice to clarify that the range of tested key
size is from MINIMUM_KEY_SIZE to what returned by
Cipher.getMaxAllowedKeyLength(ALGORITHM) with the increment of
KEYCUTTER.
added comments.
4) line 66, keyStrength is better phrased as variousKeySize in my
opinion.
renamed to variousKeySize.
5) MINIMUM_KEY_SIZE and KEYCUTTER should be algorithm specific. It'd
be clearer to explicitly list out the algorithms which works against
the the hardcoded values for the sake of maintenance.
added comments.
6) It looks to me that String[] modes and String[] paddings could be
moved to runAll(). This seems more consistent with the signature of
runTest() where mode and padding are again specified here.
I left it as is and changed runTest to private method. Either is ok with
me.
Thanks,
Rajan
Will send u comments regarding the TestLength ones in a separate email.
Thanks,
Valerie
On 7/14/2015 1:27 PM, Rajan Halade wrote:
May I request you to review new tests added to check Cipher
operations with different algorithms and modes.
Webrev: http://cr.openjdk.java.net/~rhalade/8048601/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8048601
Thanks,
Rajan
