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