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

[Rajan Halade]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20150825/3bfbe7fc/attachment.htm>