Re: Code review request for JEP-121
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 =
Re: Code review request for JEP-121
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.
Re: Code review request for JEP-121
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
Re: Code review request for JEP-121
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.
Re: Code review request for JEP-121
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.
Re: Code review request for JEP-121
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.
Re: Code review request for JEP-121
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.
Re: Code review request for JEP-121
Thanks for those comments. I've refactored the classes as suggested and generated a new webrev: http://cr.openjdk.java.net/~vinnie/6383200/webrev.01/ On 06/ 4/12 09:17 PM, Valerie (Yu-Ching) Peng wrote: Vinnie, I am still reviewing the changes. Just some quick comments and questions: Your current changes have both PBES1 and PBES2 impl inside the PBECipherCore.java but yet there is another new classPBES2Core.java which seems to just delegate the calls to the internal PBECipherCore object. Have you considered splitting out the PBES2 impl from PBECipherCore into PBES2Core.java? Maybe renaming PBECipherCore to PBES1Core.java so it's clear that it's PBES1 impl only. Also, instead of encapsulating the PBEXXCore object and yet duplicating all the engineXXX methods which simply delegate to the implXXX calls, why not just extending from a common parent which extends the CipherSpi class and no need for the engineXXX methods for all children classes. Thanks, Valerie On 06/01/12 11:18, Vincent Ryan wrote: Hello Valerie, Could you please review these changes for JEP-121: http://cr.openjdk.java.net/~vinnie/6383200/webrev.00/ Thanks.
Re: Code review request for JEP-121
Vinnie, I am still reviewing the changes. Just some quick comments and questions: Your current changes have both PBES1 and PBES2 impl inside the PBECipherCore.java but yet there is another new classPBES2Core.java which seems to just delegate the calls to the internal PBECipherCore object. Have you considered splitting out the PBES2 impl from PBECipherCore into PBES2Core.java? Maybe renaming PBECipherCore to PBES1Core.java so it's clear that it's PBES1 impl only. Also, instead of encapsulating the PBEXXCore object and yet duplicating all the engineXXX methods which simply delegate to the implXXX calls, why not just extending from a common parent which extends the CipherSpi class and no need for the engineXXX methods for all children classes. Thanks, Valerie On 06/01/12 11:18, Vincent Ryan wrote: Hello Valerie, Could you please review these changes for JEP-121: http://cr.openjdk.java.net/~vinnie/6383200/webrev.00/ Thanks.
Code review request for JEP-121
Hello Valerie, Could you please review these changes for JEP-121: http://cr.openjdk.java.net/~vinnie/6383200/webrev.00/ Thanks.
