Code review request for JEP-121

[Vincent Ryan]

Thanks for this latest review. Comments below.


On 11/10/2012 04:16, Valerie (Yu-Ching) Peng wrote:
> Hi, Vinnie,
>
> Here are my comments on the latest webrev 04.
>
> <PBEParameterSpec>
> <PBEWithMD5AndDESCipher>
> <PBEWithMD5AndTripleDESCipher>
> <PBES1Core>
> <PBKDF2Core>
> <PBEKeyFactory>
> => looks fine.
>
> <PBEParameters.java>
> => Well, the fields contains the new cipherParam field needed for PBES2
> cipher, but the encoding is still for the older PBES1 cipher.
> => Perhaps it's cleaner to use a separate class for parameters for PBES2
> cipher. The ASN.1 syntax is defined in PCKS#5v2.1 Appendix A.2 and B.2

Right. I've overlooked the ASN.1 encoding issue. I'll create a new
PBES2Parameters class as you suggest.


>
> <PKCS12PBECipherCore>
> => fine, although as I previously mentioned that it'll be easier to
> maintain and understand if we can refactor the code with a
> non-CipherCore object, so that no special handling needed for RC4. Can
> we file a separate bug/rfe to keep track of this refactoring?

I'll file a bug on that.


>
> <PBMAC1Core>
> => Well, the HmacPKCS12PBESHA1 class (which you renamed to "PBMAC1Core")
> implements the PKCS#12 v1 standard and is different from the PBMAC1
> algorithms defined in PKCS#5 v2.1. So, the new comments at line 39-40
> aren't correct. The two standards, i.e. PKCS#12 and PKCS#5, aren't
> consistent and have different ways on how the keys are derived. If you
> look at PKCS#5v2.1, it explicitly specified that the key shall be
> derived using PBKDF2 and the impl inside HmacPKCS12PBESHA1 relies on the
> PKCS12PBECipherCore.derive(...) method for deriving the keys. If the
> goal is about supporting "PBMAC1" function defined in PKCS#5v2.1, then
> we need to have separate classes which use PBKDF2.
> => The HmacPKCS12PBESHA1 class is used by PKCS12 keystore class. So, we
> still need to keep it and can't shift it to the impl defined by
> PKCS#5v2.1. Currently, PKCS#12 only uses SHA1. Well, but things are
> confusing as is already...
>

I'll re-instate HmacPKCS12PBESHA1 and define a separate implementation
class for PBMAC1.


> <SunJCE>
> => Given the above on PBMAC1Core, the "// PBMAC1" comment on line 678
> isn't correct.
>
> I am still thinking about the changes on PBEKey and PBES2Core classes,
> but thought that I should send you above comments first while I sort my
> thoughts out.
>
> Thanks,
> Valerie
>
> On 10/04/12 03:50, Vincent Ryan wrote:
>> I've made further modifications including adding support to
>> PBEParameterSpec
>> for an AlgorithmParameterSpec object. This is used to convey parameters
>> (in addition to salt and iteration count) to the underlying cipher.
>>
>> For example, AES requires an initialization vector so PBE algorithms
>> that use
>> AES need a mechanism to supply an IV parameter.
>>
>> The latest webrev is at:
>>     http://cr.openjdk.java.net/~vinnie/6383200/webrev.04/
>>
>>
>>
>> On 4 Sep 2012, at 18:04, Vincent Ryan wrote:
>>
>>> Thanks Valerie.
>>>
>>> I'd addressed your comments except the first one.
>>>
>>> Since RC4 is a stream cipher and RC2 is a block cipher they are handled
>>> slightly differently. It would be possible to re-factor this code to
>>> simplify it but I'd like to leave that for later as I'm under pressure
>>> to meet the next promotion date.
>>>
>>> The latest webrev is at:
>>>   http://cr.openjdk.java.net/~vinnie/6383200/webrev.03/
>>>
>>>
>>>
>>> On 08/31/12 03:05 AM, Valerie (Yu-Ching) Peng wrote:
>>>> Vinnie,
>>>>
>>>> <PKCS12PBECipherCore.java>
>>>> 1. Is it possible to replace the CipherCore object w/ CipherSpi object
>>>> so to maximize the code re-use? The new code uses CipherSpi object for
>>>> RC4 and CipherCore for RC2. Perhaps by using CipherSpi for both RC4 and
>>>> RC2, we can have less code which would be easier to maintain...
>>>>
>>>> <PBEKeyFactory>
>>>> 1. line 57, change the initial set size to 17 from 4?
>>>>
>>>> <PBES2Core.java>
>>>> 1. the impls of the two following engineInit() methods are
>>>> inconsistent,
>>>> i.e.
>>>> engineInit(int, Key, AlgorithmParameterSpec, SecureRandom) - expects
>>>> IvParameterSpec
>>>> engineInit(int, Key, AlgorithmParameters, SecureRandom) - expects
>>>> objects created from PBEParameterSpec
>>>> 2. The impl of engineGetParameters() currently returns objects created
>>>> from PBEParameterSpec. It should return whatever is expected in the
>>>> engineInit(...) calls, I'd think.
>>>>
>>>> Will send you the rest of comments later,
>>>> Valerie
>>>>
>>>> On 08/29/12 08:20, Vincent Ryan wrote:
>>>>> On 06/ 1/12 07:18 PM, Vincent Ryan wrote:
>>>>>> Hello Valerie,
>>>>>>
>>>>>> Could you please review these changes for JEP-121:
>>>>>> http://cr.openjdk.java.net/~vinnie/6383200/webrev.00/
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>> The latest webrev is now available at:
>>>>>
>>>>> http://cr.openjdk.java.net/~vinnie/6383200/webrev.02/
>>>>>
>>>>> I've incorporated review comments and made some fixes
>>>>> to the implementation of AES-based PBE algorithms.
>>>>>
>>>>> Thanks.
>