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