Re: RFR[11] JDK-8146293 "Add Support for RSA-PSS Signature Algorithm as in PKCS#1 v2.2"
Please find comments in line. On 5/18/2018 4:29 PM, Bradford Wetmore wrote: Mostly minor, but a couple substantive comments. I skimmed the tests this time, but didn't hit them as hard. On 5/14/2018 1:20 PM, Valerie Peng wrote: Hi Brad, The latest webrev is at http://cr.openjdk.java.net/~valeriep/8146293/webrev.04/ The only difference between webrev.03 and webrev.04 is the review comments from the CSR. SignedObject.java - Now that SignedObject is no longer in the scope of the CSR, we talked about updating the class javadoc with an example about setting the parameters before passing the Signature object in. I didn't see that, so did you want to at least give an example of it here? I don't expect that would require a separate CSR. Well, I debated about this and feel that it's probably better to leave this for later once we are set about the recommended usage for SignedObject. Signature.java -- Copyright year update. Fixed. PSSParameterSpec.java - Nit: add vertical space between description and params. 98/99, 104/105. 125: Here you do need a period, since it's the end of a sentence and you need to separate the two sentence. Periods after both sentences. 162/164: If you're going to fix some of the other spots in this file, you might also add spaces here. Ok. RSAPrivateCrtKeySpec.java - Out of curiosity, here and in a couple of other places, what prompted you to completely reword these constructor sentences? It's always bothered me, and I noticed it was done between webrev.01 and .02. It was approved in the CSR, so yay! You commented about it. So I made the changes. RSAPublicKeySpec.java RSAPrivateKeySpec.java -- 95: Shouldn't you move the null statement to the @return line instead of the method summary? e.g. * Returns the parameters associated with this key. * * @return the parameters associated with this key, or null if not present? Existing javadoc seems to be using either. I prefer to keep it as is. javax/crypto/spec/package-info.java --- Copyright year update. Fixed. SignerInfo.java PKCS10.java --- This is what we talked about earlier. In X509CRL.java, X509CRLImpl.java, X509Certificate.java, X509CertImpl.java, you are setting the parameters after the init calls, but here you are doing them before. Probably should be consistent. Correct. Good catch. RSAUtil.java 44: Extra "," Fixed. RSAPadding.java --- 106/109: Make final? Ok. RSASignature.java - 281. Indent 4 more spaces. Fixed. MGF1.java - 31. If you wouldn't mind adding RFC here? Ok. RSAPSSSignature.java 191: Would you mind inserting a comment that you "skip the JCA overhead"? Ok. 264: -> PSSParameterSpec.TRAILER_FIELD_BC instead of hardcoding 1? 418: Thanks for adding all of the "stepX" comments. That really helps readability! I'm assuming no material changes were made here? I looked at the impl code heavily on the last review, but not as much this time. No, only comments are added. TestOAEPWithParams.java --- 50: Should we also add SHA-384, SHA-512 here? I am not so sure as the key size is only 768. We can bump the size up and add SHA-384, SHA512, but since other tests in the same directory covers SHA-382 and SHA-512, I only added SHA-512/224 and SHA-512/256 to this test. Offsets.java 43: Should we also add all of the missing RSA variants as well? SHA{1,224,256...}withRSA Ok, I added more but left SHA1 out as it's sunsetting and existing coverage. Will re-run mach5 again. Webrev updated at: http://cr.openjdk.java.net/~valeriep/8146293/webrev.05/ Thanks, Valerie Thanks, Brad
Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations
Hi John, Yes, the second call must throw IllegalStateException. See the class description in javax.crypto.Cipher where it talks about AEAD modes and AAD processing. It states that all AAD has to be supplied before update and/or doFinal methods are invoked. This constraint is also talked about in Cipher.updateAAD's javadoc. Thanks for the catch on the double-space. --Jamil On 05/18/2018 04:06 PM, sha.ji...@oracle.com wrote: Hi Jamil, -- ChaCha20Cipher.java 430 } else if (aadDone) { 431 // No AAD updates allowed after the PT/CT update method is called 432 throw new IllegalStateException("Attempted to update AAD on " + 433 "Cipher after plaintext/ciphertext update"); Please consider the below case, cipher.updateAAD(); cipher.update(); cipher.updateAAD(); Should the second call on updateAAD() throw IllegalStateException? Minor: Two spaces between "method" and "is" on line 431. Best regards, John Jiang On 17/05/2018 03:05, Jamil Nimeh wrote: Round 6. This brings ChaCha20/ChaCha20-Poly1305 into conformance with Cipher's specification when forms of init that take AlgorithmParameters or AlgorithmParameterSpec are used. Previously, a non-null AP or APS object was required. In order to better conform to the specification, if a null AP or APS is provided for these ciphers, a random nonce will be generated and the counter will be set to 1, just as is currently done with valid forms of init that don't take an AP or APS object (e.g. Cipher.init(int, Key, SecureRandom) ). Per the spec in Cipher, this is only true for ENCRYPT_MODE and will throw InvalidKeyException when done in DECRYPT_MODE. I also added a few test cases that exercise these code paths in the ChaCha20Poly1305Parameters.java test case. http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/ Thanks, --Jamil On 05/04/2018 07:06 AM, Jamil Nimeh wrote: Round 5. This adds Sean's comments. Sean, I was never able to execute a case on init where a half-baked object would fail. In most cases it would fail in checks in javax.crypto.Cipher before it ever made it down to my code. I'm pretty confident the init sequence is OK. I did move the setting of a few data members toward the end of the init sequence but setting the key and nonce is necessary before creating the initial state, which is then used for generating an authentication key for AEAD mode and generating keystream. http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05 Also the CSR has been finalized and can be found here: https://bugs.openjdk.java.net/browse/JDK-8198925 --Jamil On 04/27/2018 02:21 PM, Jamil Nimeh wrote: Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff mostly: * Added words in the description of javax.crypto.Cipher recommending callers reinitialize the Cipher to use different nonces after each complete encryption or decryption (similar language to what exists already for AES-GCM encryption). * Added an additional test case for ChaCha20NoReuse * Made accessor methods for ChaCha20ParameterSpec final and cleaned up the code a bit based on comments from the field. http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/ Thanks! --Jamil On 04/13/2018 11:59 AM, Jamil Nimeh wrote: Round 3 of updates for ChaCha20 and ChaCha20-Poly1305: * Removed the key field in ChaCha20 and Poly1305 implementations and only retain the key bytes as an object field (thanks Thomas for catching this) * Added additional protections against key/nonce reuse. This is a behavioral change to ChaCha20 and ChaCha20-Poly1305. Instances of these ciphers will no longer allow you to do subsequent doUpdate/doFinal calls after the first doFinal without re-initializing the cipher with either a new key or nonce. Attempting to reuse the cipher without a new initialization will throw an IllegalStateException. This is similar to the behavior of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done for both encrypt and decrypt. http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/ Thanks, --Jamil On 04/10/2018 03:34 PM, Jamil Nimeh wrote: Hello everyone, This is a quick update to the previous webrev: * When using the form of engineInit that does only takes op, key and random, the nonce will always be random even if the random parameter is null. A default instance of SecureRandom will be used to create the nonce in this case, instead of all zeroes. * Unused debug code was removed from the ChaCha20Cipher.java file * ChaCha20Parameters.engineToString no longer obtains the line separator from a System property directly. It calls System.lineSeparator() similar to how other AlgorithmParameter classes in com.sun.crypto.provider do it. http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/ Thanks, --Jamil On 03/26/2018 12:08 PM, Jamil Nimeh wrote: Hello all, This is a request for review for the
Re: RFR[11] JDK-8146293 "Add Support for RSA-PSS Signature Algorithm as in PKCS#1 v2.2"
Mostly minor, but a couple substantive comments. I skimmed the tests this time, but didn't hit them as hard. On 5/14/2018 1:20 PM, Valerie Peng wrote: Hi Brad, The latest webrev is at http://cr.openjdk.java.net/~valeriep/8146293/webrev.04/ The only difference between webrev.03 and webrev.04 is the review comments from the CSR. SignedObject.java - Now that SignedObject is no longer in the scope of the CSR, we talked about updating the class javadoc with an example about setting the parameters before passing the Signature object in. I didn't see that, so did you want to at least give an example of it here? I don't expect that would require a separate CSR. Signature.java -- Copyright year update. PSSParameterSpec.java - Nit: add vertical space between description and params. 98/99, 104/105. 125: Here you do need a period, since it's the end of a sentence and you need to separate the two sentence. Periods after both sentences. 162/164: If you're going to fix some of the other spots in this file, you might also add spaces here. RSAPrivateCrtKeySpec.java - Out of curiosity, here and in a couple of other places, what prompted you to completely reword these constructor sentences? It's always bothered me, and I noticed it was done between webrev.01 and .02. It was approved in the CSR, so yay! RSAPublicKeySpec.java RSAPrivateKeySpec.java -- 95: Shouldn't you move the null statement to the @return line instead of the method summary? e.g. * Returns the parameters associated with this key. * * @return the parameters associated with this key, or null if not present? javax/crypto/spec/package-info.java --- Copyright year update. SignerInfo.java PKCS10.java --- This is what we talked about earlier. In X509CRL.java, X509CRLImpl.java, X509Certificate.java, X509CertImpl.java, you are setting the parameters after the init calls, but here you are doing them before. Probably should be consistent. RSAUtil.java 44: Extra "," RSAPadding.java --- 106/109: Make final? RSASignature.java - 281. Indent 4 more spaces. MGF1.java - 31. If you wouldn't mind adding RFC here? RSAPSSSignature.java 191: Would you mind inserting a comment that you "skip the JCA overhead"? 264: -> PSSParameterSpec.TRAILER_FIELD_BC instead of hardcoding 1? 418: Thanks for adding all of the "stepX" comments. That really helps readability! I'm assuming no material changes were made here? I looked at the impl code heavily on the last review, but not as much this time. TestOAEPWithParams.java --- 50: Should we also add SHA-384, SHA-512 here? Offsets.java 43: Should we also add all of the missing RSA variants as well? SHA{1,224,256...}withRSA Thanks, Brad
Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations
Hi Jamil, -- ChaCha20Cipher.java 430 } else if (aadDone) { 431 // No AAD updates allowed after the PT/CT update method is called 432 throw new IllegalStateException("Attempted to update AAD on " + 433 "Cipher after plaintext/ciphertext update"); Please consider the below case, cipher.updateAAD(); cipher.update(); cipher.updateAAD(); Should the second call on updateAAD() throw IllegalStateException? Minor: Two spaces between "method" and "is" on line 431. Best regards, John Jiang On 17/05/2018 03:05, Jamil Nimeh wrote: Round 6. This brings ChaCha20/ChaCha20-Poly1305 into conformance with Cipher's specification when forms of init that take AlgorithmParameters or AlgorithmParameterSpec are used. Previously, a non-null AP or APS object was required. In order to better conform to the specification, if a null AP or APS is provided for these ciphers, a random nonce will be generated and the counter will be set to 1, just as is currently done with valid forms of init that don't take an AP or APS object (e.g. Cipher.init(int, Key, SecureRandom) ). Per the spec in Cipher, this is only true for ENCRYPT_MODE and will throw InvalidKeyException when done in DECRYPT_MODE. I also added a few test cases that exercise these code paths in the ChaCha20Poly1305Parameters.java test case. http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/ Thanks, --Jamil On 05/04/2018 07:06 AM, Jamil Nimeh wrote: Round 5. This adds Sean's comments. Sean, I was never able to execute a case on init where a half-baked object would fail. In most cases it would fail in checks in javax.crypto.Cipher before it ever made it down to my code. I'm pretty confident the init sequence is OK. I did move the setting of a few data members toward the end of the init sequence but setting the key and nonce is necessary before creating the initial state, which is then used for generating an authentication key for AEAD mode and generating keystream. http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05 Also the CSR has been finalized and can be found here: https://bugs.openjdk.java.net/browse/JDK-8198925 --Jamil On 04/27/2018 02:21 PM, Jamil Nimeh wrote: Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff mostly: * Added words in the description of javax.crypto.Cipher recommending callers reinitialize the Cipher to use different nonces after each complete encryption or decryption (similar language to what exists already for AES-GCM encryption). * Added an additional test case for ChaCha20NoReuse * Made accessor methods for ChaCha20ParameterSpec final and cleaned up the code a bit based on comments from the field. http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/ Thanks! --Jamil On 04/13/2018 11:59 AM, Jamil Nimeh wrote: Round 3 of updates for ChaCha20 and ChaCha20-Poly1305: * Removed the key field in ChaCha20 and Poly1305 implementations and only retain the key bytes as an object field (thanks Thomas for catching this) * Added additional protections against key/nonce reuse. This is a behavioral change to ChaCha20 and ChaCha20-Poly1305. Instances of these ciphers will no longer allow you to do subsequent doUpdate/doFinal calls after the first doFinal without re-initializing the cipher with either a new key or nonce. Attempting to reuse the cipher without a new initialization will throw an IllegalStateException. This is similar to the behavior of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done for both encrypt and decrypt. http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/ Thanks, --Jamil On 04/10/2018 03:34 PM, Jamil Nimeh wrote: Hello everyone, This is a quick update to the previous webrev: * When using the form of engineInit that does only takes op, key and random, the nonce will always be random even if the random parameter is null. A default instance of SecureRandom will be used to create the nonce in this case, instead of all zeroes. * Unused debug code was removed from the ChaCha20Cipher.java file * ChaCha20Parameters.engineToString no longer obtains the line separator from a System property directly. It calls System.lineSeparator() similar to how other AlgorithmParameter classes in com.sun.crypto.provider do it. http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/ Thanks, --Jamil On 03/26/2018 12:08 PM, Jamil Nimeh wrote: Hello all, This is a request for review for the ChaCha20 and ChaCha20-Poly1305 cipher implementations. Links to the webrev and the JEP which outlines the characteristics and behavior of the ciphers are listed below. http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/ http://openjdk.java.net/jeps/329 Thanks, --Jamil
Re: Signature from User-specified URIDereferencers NodeSetData objects is wrong
On 5/17/18 1:54 AM, Shubham Rajput wrote: Any lead why the signature is forming for the node element name only and not for the whole node? I can't remember for sure now, but it probably has something to do with the way you are returning the nodes from your URIDereferencer. You are probably better off defining an XPathFilter2Transform with an XPathFilter2ParameterSpec [1] for your signature and letting that do the filtering for you automatically. HTH, Sean [1] https://docs.oracle.com/javase/10/docs/api/javax/xml/crypto/dsig/spec/XPathFilter2ParameterSpec.html