That's correct. I'll make that change to PBEParameters.java. Thanks.
On 7 Nov 2012, at 02:51, Valerie (Yu-Ching) Peng wrote: > Vinnie, > > I noticed the following after you putback, if you agree w/ my comments, > please include them in the follow up TBD bug fixes for JEP-121. > > <PBEParameters.java> > - I believe this class is used solely for the older PBE algorithms? If yes, > its engineInit(...) should check that the PBEParameterSpec.getParameterSpec() > (line 72) is null and error out if this is not the case. No need for the > field "cipherParam" since it should be null. > > Thanks, > Valerie > > On 11/02/12 11:19, Vincent Ryan wrote: >> Thanks for all your comments so far. Here's my latest webrev: >> http://cr.openjdk.java.net/~vinnie/6383200/webrev.05/ >> >> There are 2 TBD's remaining that involve code re-factoring (in >> PKCS12PBECipherCore& PBES2Core) >> which I'd like to handle separately, later. >> >> Thanks. >> >> >> On 27 Oct 2012, at 00:18, Valerie (Yu-Ching) Peng wrote: >> >>> Vinnie, >>> >>> The last mile is the hardest...sorry for the delay. >>> >>> <PBES2Core> >>> 1. I am not sure about having the ivSpec field. It seems that this can be >>> done without since it should be same as what's returned by cipher.getIV() >>> since that's what you use for engineGetIV() call. >>> 2. in getParameters(), since we are generating default value for salt and >>> ic, perhaps we should also handle iv as well. >>> 3. Hmm, I don't quite understand why do we have to require the key must be >>> an instance of PBEKey. Well, the current types for PBE keys can be way more >>> complex than other cipher algorithms such as AES, so we may want to make >>> sure we cover as much usage scenarios as possible. For older PBE >>> algorithms, it will take any Key objects with "PBEXXX" algorithm and >>> PBEParameterSpec (or the corresponding parameters). For this newer PBE >>> algorithms, at a minimum, it needs Key object w/ "PBEXXX" algorithm and >>> (new) PBEParameterSpec (or the corresponding parameters). If no parameters >>> are supplied and the specified key object is of type PBEKey, then we may >>> use the salt and ic count from the PBEKey object as default values. >>> 4. The engineInit code starting at line171 seems quite complicated. If >>> possible, can we consolidate the validity checking on salt, ic, to one >>> place? Currently they are separated into many if-then-else blocks. Same >>> goes for the part about generating default iv for encryption/wrap mode, >>> i.e. line 207-213 + line 244-250, can be done if none are found in the >>> supplied values. >>> >>> <PBEKey> >>> 1. Well, changing this class to implementing the >>> javax.crypto.interfaces.PBEKey which contains additional salt, ic info >>> which aren't used in hashCode(). equals(..) seem confusing to me. Since PBE >>> ciphers can get salt, ic, and iv from the PBEParameterSpec (and its >>> corresponding parameters), I feel it's probably simpler to just leave it >>> unchanged. >>> >>> Thanks, >>> Valerie >>> >>> On 10/11/12 05:07, Vincent Ryan wrote: >>>> 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. >
