[9] RFR: 8048601: Tests for JCE crypto ciphers

[Valerie Peng]

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?
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).

PBKDF2Wrapper.java
1) typo on line 43 and 52 - should be T_R_ANSFORMATION
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.
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);

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.
> 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.
> 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.
> 4) line 66, keyStrength is better phrased as variousKeySize in my 
> opinion.
> 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.
> 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.
>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20150820/0befd827/attachment.htm>