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

[Valerie Peng]

Ok, looks good.
Thanks,
Valerie

On 8/25/2015 1:10 PM, Rajan Halade wrote:
> 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/20150828/1d219ca3/attachment.htm>