Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Tue, 30 Nov 2021 19:48:19 GMT, Valerie Peng wrote: >> Hmm, thinking more about "internal"/"opaque", given this is naming for the >> parent, maybe "internal" is more correct. The non-sensitive keys are >> encapsulated by the children classes and is still an instance of the parent. >> If you name the parent class w/ "opaque" suffix, it looks >> misleading/confusing. The opaqueness is implied by the implementation >> instead of the properties of the objects. > >> Hi @valeriepeng , >> >> Yes, I just verified how serialization works for P11Keys and your >> 'transient' change makes sense to me now. >> >> I see your point about Internal/Opaque. Keep 'Internal' then if you prefer. >> The whole inheritance relationship between these classes sounds a bit weird >> to me, beyond the names we call them. I wouldn't suggest any additional >> changes there now, though. >> >> It looks to me that the only 2 changes expected for Webrev.02 are: 1) >> P11RSAPrivateNonCRTKey to use the parent's protected 'n'; and 2) removal of >> 'long errorCode = e.getErrorCode();'. >> >> Now that we almost have the changeset ready, I'm not sure of how to proceed >> with the push. Do you want me to commit Webrev.02 in my own branch and use >> the 'Co-authored-by:' header? If we do that, do we still need an additional >> Reviewer? Do you prefer to have your own branch? Please let me know of how >> to move forward. >> >> Martin.- > > You can just incorporate the changes on your branch and proceeds with me > being the reviewer. The webrev that I sent u is just an easier way to express > the comments/feedbacks. > > Thanks, > Valerie Hi @valeriepeng , Please take a look at the latest change based on your comments. No regressions found in 'jdk/sun/security/pkcs11'. Thanks, Martin.- - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Tue, 30 Nov 2021 19:48:19 GMT, Valerie Peng wrote: >> Hmm, thinking more about "internal"/"opaque", given this is naming for the >> parent, maybe "internal" is more correct. The non-sensitive keys are >> encapsulated by the children classes and is still an instance of the parent. >> If you name the parent class w/ "opaque" suffix, it looks >> misleading/confusing. The opaqueness is implied by the implementation >> instead of the properties of the objects. > >> Hi @valeriepeng , >> >> Yes, I just verified how serialization works for P11Keys and your >> 'transient' change makes sense to me now. >> >> I see your point about Internal/Opaque. Keep 'Internal' then if you prefer. >> The whole inheritance relationship between these classes sounds a bit weird >> to me, beyond the names we call them. I wouldn't suggest any additional >> changes there now, though. >> >> It looks to me that the only 2 changes expected for Webrev.02 are: 1) >> P11RSAPrivateNonCRTKey to use the parent's protected 'n'; and 2) removal of >> 'long errorCode = e.getErrorCode();'. >> >> Now that we almost have the changeset ready, I'm not sure of how to proceed >> with the push. Do you want me to commit Webrev.02 in my own branch and use >> the 'Co-authored-by:' header? If we do that, do we still need an additional >> Reviewer? Do you prefer to have your own branch? Please let me know of how >> to move forward. >> >> Martin.- > > You can just incorporate the changes on your branch and proceeds with me > being the reviewer. The webrev that I sent u is just an easier way to express > the comments/feedbacks. > > Thanks, > Valerie Hi @valeriepeng I've reverted the initial 2 changesets on this branch, rebased to the latest commit on 'master' (git merge), applied Webrev.02 as discussed and added the test case from the initial commit (with the change from the 2nd commit applied on top). I've noticed no regressions in jdk/sun/security/pkcs11. Does it look good to you? Thanks, Martin.- - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Fri, 19 Nov 2021 19:50:33 GMT, Valerie Peng wrote: >> Martin Balao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> P11Key static inner classes refactorings. > > Hmm, thinking more about "internal"/"opaque", given this is naming for the > parent, maybe "internal" is more correct. The non-sensitive keys are > encapsulated by the children classes and is still an instance of the parent. > If you name the parent class w/ "opaque" suffix, it looks > misleading/confusing. The opaqueness is implied by the implementation instead > of the properties of the objects. > Hi @valeriepeng , > > Yes, I just verified how serialization works for P11Keys and your 'transient' > change makes sense to me now. > > I see your point about Internal/Opaque. Keep 'Internal' then if you prefer. > The whole inheritance relationship between these classes sounds a bit weird > to me, beyond the names we call them. I wouldn't suggest any additional > changes there now, though. > > It looks to me that the only 2 changes expected for Webrev.02 are: 1) > P11RSAPrivateNonCRTKey to use the parent's protected 'n'; and 2) removal of > 'long errorCode = e.getErrorCode();'. > > Now that we almost have the changeset ready, I'm not sure of how to proceed > with the push. Do you want me to commit Webrev.02 in my own branch and use > the 'Co-authored-by:' header? If we do that, do we still need an additional > Reviewer? Do you prefer to have your own branch? Please let me know of how to > move forward. > > Martin.- You can just incorporate the changes on your branch and proceeds with me being the reviewer. The webrev that I sent u is just an easier way to express the comments/feedbacks. Thanks, Valerie - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Fri, 19 Nov 2021 19:50:33 GMT, Valerie Peng wrote: >> Martin Balao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> P11Key static inner classes refactorings. > > Hmm, thinking more about "internal"/"opaque", given this is naming for the > parent, maybe "internal" is more correct. The non-sensitive keys are > encapsulated by the children classes and is still an instance of the parent. > If you name the parent class w/ "opaque" suffix, it looks > misleading/confusing. The opaqueness is implied by the implementation instead > of the properties of the objects. Hi @valeriepeng , Yes, I just verified how serialization works for P11Keys and your 'transient' change makes sense to me now. I see your point about Internal/Opaque. Keep 'Internal' then if you prefer. The whole inheritance relationship between these classes sounds a bit weird to me, beyond the names we call them. I wouldn't suggest any additional changes there now, though. It looks to me that the only 2 changes expected for Webrev.02 are: 1) P11RSAPrivateNonCRTKey to use the parent's protected 'n'; and 2) removal of 'long errorCode = e.getErrorCode();'. Now that we almost have the changeset ready, I'm not sure of how to proceed with the push. Do you want me to commit Webrev.02 in my own branch and use the 'Co-authored-by:' header? If we do that, do we still need an additional Reviewer? Do you prefer to have your own branch? Please let me know of how to move forward. Martin.- - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Fri, 13 Aug 2021 17:11:45 GMT, Martin Balao wrote: >> As described in JDK-8271566 [1], this patch proposal is intended to fix a >> problem that arises when using DSA keys that have a 256-bits (or larger) G >> parameter for signatures (either signing or verifying). There were some >> incorrect assumptions and hard-coded length values in the code before. >> Please note that, for example, the tuple (2048, 256) for DSA is valid >> according to FIPS PUB 186-4. >> >> Beyond the specific issues in signatures, I decided to provide a broader >> solution and enable key parameter retrieval for other key types (EC, DH) >> when possible. This is, when the key is not sensitive. One thing that I >> should note here is that token keys (those that have the CKA_TOKEN attribute >> equal to 'true') are considered sensitive in this regard, at least by the >> NSS Software Token implementation. I don't have access to other vendor >> implementations but if there is any concern, we can adjust the constraint to >> NSS-only. However, I'm not sure which use-case would require to get private >> keys out of a real token, weakening its security. I'd be more conservative >> here and not query the values if not sure that it will succeed. >> >> No regressions found in jdk/sun/security/pkcs11. A new test added: >> LargerDSAKey. >> >> -- >> [1] - https://bugs.openjdk.java.net/browse/JDK-8271566 > > Martin Balao has updated the pull request incrementally with one additional > commit since the last revision: > > P11Key static inner classes refactorings. Hmm, thinking more about "internal"/"opaque", given this is naming for the parent, maybe "internal" is more correct. The non-sensitive keys are encapsulated by the children classes and is still an instance of the parent. If you name the parent class w/ "opaque" suffix, it looks misleading/confusing. The opaqueness is implied by the implementation instead of the properties of the objects. - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Thu, 18 Nov 2021 19:27:30 GMT, Martin Balao wrote: > > > Hi @valeriepeng , > > Some comments and questions regarding Webrev.01: > > * P11Key.java > > * Would you consider replacing the 'Internal' suffix with 'Opaque'? I > believe the term 'opaque' better reflects what these keys are: you cannot see > their values -thus, opaque- but you can use them as-is. 'Internal' gives me > the impression of something not supposed to be exposed; and this is not > exactly the case. > > * Why are P11RSAPrivateKeyInternal::n, P11RSAPrivateKey:: qe, coeff>, etc. now transient? > > * Now that P11RSAPrivateKey::n private field was removed, the extra > PKCS#11 lib call I mentioned in Webrev.00 is not possible anymore. The > override of ::getModulus looks good to me, though, for the reasons you said. > > * Can P11RSAPrivateNonCRTKey use the protected 'n' field from its parent > instead of declaring a private one? > > * P11Signature.java > > * 'long errorCode = e.getErrorCode();' -> Looks to me that this change > was included into the Webrev by mistake, and is part of JDK-8236512. > > > Thanks, Martin.- Either internal or opaque suffix works for me. Fields aren't really being written out into the serialized bytes are "transient". IIRC, the serialized bytes are the "encoding" instead of the fields, we should mark these fields transient. As for the P11RSAPrivateNonCRTKey, yes, it should use the 'n' from parent instead of its own. I missed it when working on webrev.01. As for 'long errorCode = e.getErrorCode();' in P11Signature.java, yes, it should have been removed. I was manually merging the changes when refreshing the source tree and missed to remove this line. Thanks, Valerie - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Thu, 18 Nov 2021 18:37:38 GMT, Valerie Peng wrote: >>> > ``` >>> > * By eliminating P11RSAPrivateKey::getModulus, looks to me that >>> > P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now >>> > called, leading to an unnecessary call to the native library as the >>> > modulus was already received on P11RSAPrivateKey constructor. This >>> > happens to P11RSAPrivateNonCRTKey as well. >>> > ``` >>> >>> There shouldn't be another call to the native library as it is only made >>> when the modulus n is null. However, since n is already available, >>> overriding the getModulus() method makes sense as there is no need to call >>> fetchValues() which should return upon a non-null n value, but still an >>> overhead. >>> >> >> In my view (Webrev.00 based comment), the variable 'n' holding the modulus >> value is private in P11RSAPrivateKey. This means that when >> P11PrivateKeyRSA::getModulus is called, P11PrivateKeyRSA::n (which is >> protected) has a 'null' value and the PKCS#11 lib call is done again. > >> >> >> > > ``` >> > > * By eliminating P11RSAPrivateKey::getModulus, looks to me that >> > > P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now >> > > called, leading to an unnecessary call to the native library as the >> > > modulus was already received on P11RSAPrivateKey constructor. This >> > > happens to P11RSAPrivateNonCRTKey as well. >> > > ``` >> > >> > >> > There shouldn't be another call to the native library as it is only made >> > when the modulus n is null. However, since n is already available, >> > overriding the getModulus() method makes sense as there is no need to call >> > fetchValues() which should return upon a non-null n value, but still an >> > overhead. >> >> In my view (Webrev.00 based comment), the variable 'n' holding the modulus >> value is private in P11RSAPrivateKey. This means that when >> P11PrivateKeyRSA::getModulus is called, P11PrivateKeyRSA::n (which is >> protected) has a 'null' value and the PKCS#11 lib call is done again. > > Hmm, this is a bug and unintended. The 'n' in the child class should be > removed as the 'n' in the parent class has scope protected and should be used > instead. > > I checked webrev.01 and this has been caught and fixed already. Good to have > a different pair of eyes and more likely to spot problems. Thanks! > > Regards, > Valerie Hi @valeriepeng , Some comments and questions regarding Webrev.01: * P11Key.java * Would you consider replacing the 'Internal' suffix with 'Opaque'? I believe the term 'opaque' better reflects what these keys are: you cannot see their values -thus, opaque- but you can use them as-is. 'Internal' gives me the impression of something not supposed to be exposed; and this is not exactly the case. * Why are P11RSAPrivateKeyInternal::n, P11RSAPrivateKey::, etc. now transient? * Now that P11RSAPrivateKey::n private field was removed, the extra PKCS#11 lib call I mentioned in Webrev.00 is not possible anymore. The override of ::getModulus looks good to me, though, for the reasons you said. * Can P11RSAPrivateNonCRTKey use the protected 'n' field from its parent instead of declaring a private one? * P11Signature.java * 'long errorCode = e.getErrorCode();' -> Looks to me that this change was included into the Webrev by mistake, and is part of JDK-8236512. Thanks, Martin.- - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Wed, 17 Nov 2021 21:25:33 GMT, Martin Balao wrote: > > > > > ``` > > > * By eliminating P11RSAPrivateKey::getModulus, looks to me that > > > P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now > > > called, leading to an unnecessary call to the native library as the > > > modulus was already received on P11RSAPrivateKey constructor. This > > > happens to P11RSAPrivateNonCRTKey as well. > > > ``` > > > > > > There shouldn't be another call to the native library as it is only made > > when the modulus n is null. However, since n is already available, > > overriding the getModulus() method makes sense as there is no need to call > > fetchValues() which should return upon a non-null n value, but still an > > overhead. > > In my view (Webrev.00 based comment), the variable 'n' holding the modulus > value is private in P11RSAPrivateKey. This means that when > P11PrivateKeyRSA::getModulus is called, P11PrivateKeyRSA::n (which is > protected) has a 'null' value and the PKCS#11 lib call is done again. Hmm, this is a bug and unintended. The 'n' in the child class should be removed as the 'n' in the parent class has scope protected and should be used instead. Regards, Valerie - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Tue, 2 Nov 2021 22:44:16 GMT, Valerie Peng wrote: > > ``` > > * By eliminating P11RSAPrivateKey::getModulus, looks to me that > > P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now > > called, leading to an unnecessary call to the native library as the modulus > > was already received on P11RSAPrivateKey constructor. This happens to > > P11RSAPrivateNonCRTKey as well. > > ``` > > There shouldn't be another call to the native library as it is only made when > the modulus n is null. However, since n is already available, overriding the > getModulus() method makes sense as there is no need to call fetchValues() > which should return upon a non-null n value, but still an overhead. > In my view (Webrev.00 based comment), the variable 'n' holding the modulus value is private in P11RSAPrivateKey. This means that when P11PrivateKeyRSA::getModulus is called, P11PrivateKeyRSA::n (which is protected) has a 'null' value and the PKCS#11 lib call is done again. - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Tue, 2 Nov 2021 22:44:16 GMT, Valerie Peng wrote: >> Martin Balao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> P11Key static inner classes refactorings. > > Hi Martin, > > Please find my comments in line below. > >> * Changing P11PrivateKey::getFormat to return 'PKCS#8' and not >> overriding this method on opaque/sensitive private key classes will be an >> observable change, and it does not match what's specified in Key::getFormat: >> 'Returns the name of the primary encoding format of this key, or null if >> this key does not support encoding.'. My thinking here is that an opaque key >> does not support encoding because its value is not even accessible. > > Hmm, good point. I agree that returning null as default format and override > it whenever an encoding is returned is the right thing to do. > >> * P11PrivateKeyRSA and P11RSAPrivateKey class names look confusing to me >> (where 'RSA' is located does not say anything about the class to me at >> least). The 'Opaque' suffix for the one that is package-private might be a >> better choice. I used the suffice 'Internal' before but now I like 'Opaque' >> more. > > Sure, I didn't spend much time on this. Either Opaque or Internal suffix is > fine. Generally I prefer shorter names. > >> * P11PrivateKeyRSA::of duplicates the attributes retrieval logic >> (session.token.p11::C_GetAttributeValue call), which is there and in >> P11Key::fetchAttributes. Sensitive RSA keys get the attributes from one >> place and non-sensitive from the other. This does not look to me an >> advantage when compared to the Fetcher, which does the same for RSA and uses >> P11Key::fetchAttributes for the others. However, having all the RSA-related >> logic in P11RSAAttributesFetcher::doFetchValues makes it a bit easier for me >> to reason about the 4 different scenarios to get the data: CRT + sensitive, >> non-CRT + sensitive, CRT + non-sensitive and non-CRT + non-sensitive. > > This is where our view differs. We both have same class hierarchy but where > the attributes are managed are different, you put all of them in the very > bottom, i.e. fetcher, but I prefer them to be at top. Using RSA as an > example, there are P11RSAPrivateKeyInternal - sensitive private key > P11RSAPrivateKey - non-sensitive CRT private key > P11RSAPrivateNonCRTKey - non-sensitive non-CRT private key > P11RSAPublicKey - public key > They all use the same P11RSAAttributesFetcher class but with different > keySensitive, isPrivate flags to decide which attributes to query and which > fields to populate. Whether the fields are null or not, the upper classes > have no idea. The logic are all inside the fetcher. The keySensitive, > isPrivate flags also have to be passed all around which seems strange as the > classes themselves already implied these values and should not need to take > arguments, i.e. P11RSAPrivateKey (keySensitive== false AND isPrivate == > true). Overall, I find it hard to read. Comparing the fetcher model vs the > non-fetcher model, the non-fetcher model has less code (at least 100 lines > less) and it's very clear which attributes each class requires. > >> * By eliminating P11RSAPrivateKey::getModulus, looks to me that >> P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now >> called, leading to an unnecessary call to the native library as the modulus >> was already received on P11RSAPrivateKey constructor. This happens to >> P11RSAPrivateNonCRTKey as well. > > There shouldn't be another call to the native library as it is only made when > the modulus n is null. However, since n is already available, overriding the > getModulus() method makes sense as there is no need to call fetchValues() > which should return upon a non-null n value, but still an overhead. > >> * P11PrivateKeyRSA does not make the CRT/non-CRT distinction for >> sensitive keys, so the public exponent is not obtained when it could be. >> This is a bit less functionality than what the Fetcher provides, and would >> require a few changes to fit it into the design. > > The proposed P11RSAPrivateKeyInternal class has no method for returning the > public exponent either. If needed, it should be doable by adding extra > attribute to the list of attributes in P11PrivateKeyRSA class. Should be > trivial. > > I updated the class naming in my prototype to align with yours and updated > the prototype changes with your feedback. You can check/compare it at: > http://cr.openjdk.java.net/~valeriep/8271566/webrev.01/ > > We have different views on this I guess. I prefer to let each class manage > their list of attributes instead of a separate fetcher with their own logic. > Former also has less code and follows the same model of existing code. > > Thanks, > Valerie Hi @valeriepeng , Thanks for having a look at this. While we might have slightly different views on the fetcher,
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Fri, 13 Aug 2021 17:11:45 GMT, Martin Balao wrote: >> As described in JDK-8271566 [1], this patch proposal is intended to fix a >> problem that arises when using DSA keys that have a 256-bits (or larger) G >> parameter for signatures (either signing or verifying). There were some >> incorrect assumptions and hard-coded length values in the code before. >> Please note that, for example, the tuple (2048, 256) for DSA is valid >> according to FIPS PUB 186-4. >> >> Beyond the specific issues in signatures, I decided to provide a broader >> solution and enable key parameter retrieval for other key types (EC, DH) >> when possible. This is, when the key is not sensitive. One thing that I >> should note here is that token keys (those that have the CKA_TOKEN attribute >> equal to 'true') are considered sensitive in this regard, at least by the >> NSS Software Token implementation. I don't have access to other vendor >> implementations but if there is any concern, we can adjust the constraint to >> NSS-only. However, I'm not sure which use-case would require to get private >> keys out of a real token, weakening its security. I'd be more conservative >> here and not query the values if not sure that it will succeed. >> >> No regressions found in jdk/sun/security/pkcs11. A new test added: >> LargerDSAKey. >> >> -- >> [1] - https://bugs.openjdk.java.net/browse/JDK-8271566 > > Martin Balao has updated the pull request incrementally with one additional > commit since the last revision: > > P11Key static inner classes refactorings. Hi Martin, Please find my comments in line below. > * Changing P11PrivateKey::getFormat to return 'PKCS#8' and not overriding > this method on opaque/sensitive private key classes will be an observable > change, and it does not match what's specified in Key::getFormat: 'Returns > the name of the primary encoding format of this key, or null if this key does > not support encoding.'. My thinking here is that an opaque key does not > support encoding because its value is not even accessible. Hmm, good point. I agree that returning null as default format and override it whenever an encoding is returned is the right thing to do. > * P11PrivateKeyRSA and P11RSAPrivateKey class names look confusing to me > (where 'RSA' is located does not say anything about the class to me at > least). The 'Opaque' suffix for the one that is package-private might be a > better choice. I used the suffice 'Internal' before but now I like 'Opaque' > more. Sure, I didn't spend much time on this. Either Opaque or Internal suffix is fine. Generally I prefer shorter names. > * P11PrivateKeyRSA::of duplicates the attributes retrieval logic > (session.token.p11::C_GetAttributeValue call), which is there and in > P11Key::fetchAttributes. Sensitive RSA keys get the attributes from one place > and non-sensitive from the other. This does not look to me an advantage when > compared to the Fetcher, which does the same for RSA and uses > P11Key::fetchAttributes for the others. However, having all the RSA-related > logic in P11RSAAttributesFetcher::doFetchValues makes it a bit easier for me > to reason about the 4 different scenarios to get the data: CRT + sensitive, > non-CRT + sensitive, CRT + non-sensitive and non-CRT + non-sensitive. This is where our view differs. We both have same class hierarchy but where the attributes are managed are different, you put all of them in the very bottom, i.e. fetcher, but I prefer them to be at top. Using RSA as an example, there are P11RSAPrivateKeyInternal - sensitive private key P11RSAPrivateKey - non-sensitive CRT private key P11RSAPrivateNonCRTKey - non-sensitive non-CRT private key P11RSAPublicKey - public key They all use the same P11RSAAttributesFetcher class but with different keySensitive, isPrivate flags to decide which attributes to query and which fields to populate. Whether the fields are null or not, the upper classes have no idea. The logic are all inside the fetcher. The keySensitive, isPrivate flags also have to be passed all around which seems strange as the classes themselves already implied these values and should not need to take arguments, i.e. P11RSAPrivateKey (keySensitive== false AND isPrivate == true). Overall, I find it hard to read. Comparing the fetcher model vs the non-fetcher model, the non-fetcher model has less code (at least 100 lines less) and it's very clear which attributes each class requires. > * By eliminating P11RSAPrivateKey::getModulus, looks to me that > P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now > called, leading to an unnecessary call to the native library as the modulus > was already received on P11RSAPrivateKey constructor. This happens to > P11RSAPrivateNonCRTKey as well. There shouldn't be another call to the native library as it is only made when the modulus n is null. However, since n is already available, overriding the
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Fri, 13 Aug 2021 17:11:45 GMT, Martin Balao wrote: >> As described in JDK-8271566 [1], this patch proposal is intended to fix a >> problem that arises when using DSA keys that have a 256-bits (or larger) G >> parameter for signatures (either signing or verifying). There were some >> incorrect assumptions and hard-coded length values in the code before. >> Please note that, for example, the tuple (2048, 256) for DSA is valid >> according to FIPS PUB 186-4. >> >> Beyond the specific issues in signatures, I decided to provide a broader >> solution and enable key parameter retrieval for other key types (EC, DH) >> when possible. This is, when the key is not sensitive. One thing that I >> should note here is that token keys (those that have the CKA_TOKEN attribute >> equal to 'true') are considered sensitive in this regard, at least by the >> NSS Software Token implementation. I don't have access to other vendor >> implementations but if there is any concern, we can adjust the constraint to >> NSS-only. However, I'm not sure which use-case would require to get private >> keys out of a real token, weakening its security. I'd be more conservative >> here and not query the values if not sure that it will succeed. >> >> No regressions found in jdk/sun/security/pkcs11. A new test added: >> LargerDSAKey. >> >> -- >> [1] - https://bugs.openjdk.java.net/browse/JDK-8271566 > > Martin Balao has updated the pull request incrementally with one additional > commit since the last revision: > > P11Key static inner classes refactorings. Hmm, ok, I was focus on something else and missed your update to this PR. it's been a while, I will take another look to refresh my memory and get back to you... Thanks~ - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Fri, 13 Aug 2021 17:11:45 GMT, Martin Balao wrote: >> As described in JDK-8271566 [1], this patch proposal is intended to fix a >> problem that arises when using DSA keys that have a 256-bits (or larger) G >> parameter for signatures (either signing or verifying). There were some >> incorrect assumptions and hard-coded length values in the code before. >> Please note that, for example, the tuple (2048, 256) for DSA is valid >> according to FIPS PUB 186-4. >> >> Beyond the specific issues in signatures, I decided to provide a broader >> solution and enable key parameter retrieval for other key types (EC, DH) >> when possible. This is, when the key is not sensitive. One thing that I >> should note here is that token keys (those that have the CKA_TOKEN attribute >> equal to 'true') are considered sensitive in this regard, at least by the >> NSS Software Token implementation. I don't have access to other vendor >> implementations but if there is any concern, we can adjust the constraint to >> NSS-only. However, I'm not sure which use-case would require to get private >> keys out of a real token, weakening its security. I'd be more conservative >> here and not query the values if not sure that it will succeed. >> >> No regressions found in jdk/sun/security/pkcs11. A new test added: >> LargerDSAKey. >> >> -- >> [1] - https://bugs.openjdk.java.net/browse/JDK-8271566 > > Martin Balao has updated the pull request incrementally with one additional > commit since the last revision: > > P11Key static inner classes refactorings. Hi Valerie, Apologies for the delay; I've intention to resume this work if you can. I had a look at your Webrev.00 but I'm not really convinced, at least when comparing it to the Fetcher approach. I'll share my comments below: * Changing P11PrivateKey::getFormat to return 'PKCS#8' and not overriding this method on opaque/sensitive private key classes will be an observable change, and it does not match what's specified in Key::getFormat: 'Returns the name of the primary encoding format of this key, or null if this key does not support encoding.'. My thinking here is that an opaque key does not support encoding because its value is not even accessible. * P11PrivateKeyRSA and P11RSAPrivateKey class names look confusing to me (where 'RSA' is located does not say anything about the class to me at least). The 'Opaque' suffix for the one that is package-private might be a better choice. I used the suffice 'Internal' before but now I like 'Opaque' more. * P11PrivateKeyRSA::of duplicates the attributes retrieval logic (session.token.p11::C_GetAttributeValue call), which is there and in P11Key::fetchAttributes. Sensitive RSA keys get the attributes from one place and non-sensitive from the other. This does not look to me an advantage when compared to the Fetcher, which does the same for RSA and uses P11Key::fetchAttributes for the others. However, having all the RSA-related logic in P11RSAAttributesFetcher::doFetchValues makes it a bit easier for me to reason about the 4 different scenarios to get the data: CRT + sensitive, non-CRT + sensitive, CRT + non-sensitive and non-CRT + non-sensitive. * By eliminating P11RSAPrivateKey::getModulus, looks to me that P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now called, leading to an unnecessary call to the native library as the modulus was already received on P11RSAPrivateKey constructor. This happens to P11RSAPrivateNonCRTKey as well. * P11PrivateKeyRSA does not make the CRT/non-CRT distinction for sensitive keys, so the public exponent is not obtained when it could be. This is a bit less functionality than what the Fetcher provides, and would require a few changes to fit it into the design. >From comment #Aug 17: "(...) and you just cover it up with different wrapper classes which seems algorithm specific but yet do not contain any algorithm specific fields and the logic is all inside the corresponding Fetcher" * In my view, it's true that the Fetcher has the logic to get and hold the data. But the classes that use it are not just wrapper classes covering it up: they implement different interfaces and override/implement methods differently. In example, P11RSAPrivateNonCRTKey implements RSAPrivateKey and decides not to provide an implementation of methods such as getPublicExponent, getPrimeP, getPrimeQ, etc. On the other hand, having the logic for the different RSA data-availability scenarios in a single place (CRT sensitive, non-CRT sensitive, CRT non-sensitive and non-CRT non-sensitive) makes it a bit easier to reason about. Otherwise, this is scattered all over the place. If you still want to take the non-fetcher approach and address the issues mentioned above, I can have a look. Kind regards, Martin.- - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Fri, 13 Aug 2021 17:11:45 GMT, Martin Balao wrote: >> As described in JDK-8271566 [1], this patch proposal is intended to fix a >> problem that arises when using DSA keys that have a 256-bits (or larger) G >> parameter for signatures (either signing or verifying). There were some >> incorrect assumptions and hard-coded length values in the code before. >> Please note that, for example, the tuple (2048, 256) for DSA is valid >> according to FIPS PUB 186-4. >> >> Beyond the specific issues in signatures, I decided to provide a broader >> solution and enable key parameter retrieval for other key types (EC, DH) >> when possible. This is, when the key is not sensitive. One thing that I >> should note here is that token keys (those that have the CKA_TOKEN attribute >> equal to 'true') are considered sensitive in this regard, at least by the >> NSS Software Token implementation. I don't have access to other vendor >> implementations but if there is any concern, we can adjust the constraint to >> NSS-only. However, I'm not sure which use-case would require to get private >> keys out of a real token, weakening its security. I'd be more conservative >> here and not query the values if not sure that it will succeed. >> >> No regressions found in jdk/sun/security/pkcs11. A new test added: >> LargerDSAKey. >> >> -- >> [1] - https://bugs.openjdk.java.net/browse/JDK-8271566 > > Martin Balao has updated the pull request incrementally with one additional > commit since the last revision: > > P11Key static inner classes refactorings. I tried a few things out and prefer just minor refactoring over this new Fetcher mechanism. I have shared my prototype changes with you over email. Please check it out. Thanks. Valerie - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Fri, 13 Aug 2021 17:11:45 GMT, Martin Balao wrote: >> As described in JDK-8271566 [1], this patch proposal is intended to fix a >> problem that arises when using DSA keys that have a 256-bits (or larger) G >> parameter for signatures (either signing or verifying). There were some >> incorrect assumptions and hard-coded length values in the code before. >> Please note that, for example, the tuple (2048, 256) for DSA is valid >> according to FIPS PUB 186-4. >> >> Beyond the specific issues in signatures, I decided to provide a broader >> solution and enable key parameter retrieval for other key types (EC, DH) >> when possible. This is, when the key is not sensitive. One thing that I >> should note here is that token keys (those that have the CKA_TOKEN attribute >> equal to 'true') are considered sensitive in this regard, at least by the >> NSS Software Token implementation. I don't have access to other vendor >> implementations but if there is any concern, we can adjust the constraint to >> NSS-only. However, I'm not sure which use-case would require to get private >> keys out of a real token, weakening its security. I'd be more conservative >> here and not query the values if not sure that it will succeed. >> >> No regressions found in jdk/sun/security/pkcs11. A new test added: >> LargerDSAKey. >> >> -- >> [1] - https://bugs.openjdk.java.net/browse/JDK-8271566 > > Martin Balao has updated the pull request incrementally with one additional > commit since the last revision: > > P11Key static inner classes refactorings. I agree with the new class hierarchy and see the merits. But need to think more about this new Fetcher way of doing things. It seems that the Fetcher is the real key class where there is the common logic (sensitive, isPrivate, etc.) and you just cover it up with different wrapper classes which seems algorithm specific but yet do not contain any algorithm specific fields and the logic is all inside the corresponding Fetcher. In addition, there seems to be some duplicated code in retrieving the attributes in RSA*Fetcher vs P11Key.fetchAttributes(). Will try a few things before sharing my review comments. Thanks. Valerie - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Fri, 13 Aug 2021 17:11:45 GMT, Martin Balao wrote: >> As described in JDK-8271566 [1], this patch proposal is intended to fix a >> problem that arises when using DSA keys that have a 256-bits (or larger) G >> parameter for signatures (either signing or verifying). There were some >> incorrect assumptions and hard-coded length values in the code before. >> Please note that, for example, the tuple (2048, 256) for DSA is valid >> according to FIPS PUB 186-4. >> >> Beyond the specific issues in signatures, I decided to provide a broader >> solution and enable key parameter retrieval for other key types (EC, DH) >> when possible. This is, when the key is not sensitive. One thing that I >> should note here is that token keys (those that have the CKA_TOKEN attribute >> equal to 'true') are considered sensitive in this regard, at least by the >> NSS Software Token implementation. I don't have access to other vendor >> implementations but if there is any concern, we can adjust the constraint to >> NSS-only. However, I'm not sure which use-case would require to get private >> keys out of a real token, weakening its security. I'd be more conservative >> here and not query the values if not sure that it will succeed. >> >> No regressions found in jdk/sun/security/pkcs11. A new test added: >> LargerDSAKey. >> >> -- >> [1] - https://bugs.openjdk.java.net/browse/JDK-8271566 > > Martin Balao has updated the pull request incrementally with one additional > commit since the last revision: > > P11Key static inner classes refactorings. I made some class refactoring in P11Key to achieve the following goals: * Make the classes hierarchy more clear. It was a bit odd to see P11PrivateKey and P11(RSA,DSA,DH,EC)PrivateKey unrelated in terms of "the latter is a more specific instance of the former" * We now have P11(RSA,DSA,DH,EC)PrivateKey inheriting from P11PrivateKey * This allowed to promote the 'encoded' field which is shared across all of them * Keep the API as before * For an external-package client, a non-sensitive private key is seen as a (RSA,DSA,DH,EC)PrivateKey as before. Thus, the result of calling methods that return key values or parameters should be non-null as expected * A sensitive private key will be of the PrivateKey visible type for an external-package client. Thus, it's not possible to get key values or parameters from there. Note that calling the methods that return the encoded key or format will keep returning null in these cases as before. * The difference now is that a package client (sun.security.pkcs11 internal class) can access key parameters for sensitive keys because the visible type for them is (RSA,DSA,DH,EC)PrivateKeyInternal * The key parameter information is now available and may be useful to several P11 classes in the future, in addition to P11Signature which is using it now to have an exact estimate of a DSA signature length * There was boiler-plate code with P11(RSA,DSA,DH,EC)PublicKey classes because there is an overlap with key attributes fetched for private keys, and the fetching machinery was duplicated. We now have a single way of getting key attributes. * In the P11RSAPrivateKey case (RSA CRT), we save one call to the native PKCS#11 library because all attributes are obtained at once, instead of delaying the fetch of CKA_MODULUS and CKA_PRIVATE_EXPONENT to a later point. We do not add additional PKCS#11 calls in any case. * Several improvements specific to RSA key classes. Despite being a bit different than DSA, DH and EC because of the existence of CRT and non-CRT keys; they are now better aligned and duplicated code was removed. The refactorings implied removing fields from private but serializable classes. In example, P11DSAPublicKey does not longer have the fields 'y' and 'params'. My understanding is that this a serial compatibility breaker and a CSR would be needed. I can address that but wish you could have a look at the proposal before, so we come to something stable first. No test regressions observed in jdk/sun/security/pkcs11 category. - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
> As described in JDK-8271566 [1], this patch proposal is intended to fix a > problem that arises when using DSA keys that have a 256-bits (or larger) G > parameter for signatures (either signing or verifying). There were some > incorrect assumptions and hard-coded length values in the code before. Please > note that, for example, the tuple (2048, 256) for DSA is valid according to > FIPS PUB 186-4. > > Beyond the specific issues in signatures, I decided to provide a broader > solution and enable key parameter retrieval for other key types (EC, DH) when > possible. This is, when the key is not sensitive. One thing that I should > note here is that token keys (those that have the CKA_TOKEN attribute equal > to 'true') are considered sensitive in this regard, at least by the NSS > Software Token implementation. I don't have access to other vendor > implementations but if there is any concern, we can adjust the constraint to > NSS-only. However, I'm not sure which use-case would require to get private > keys out of a real token, weakening its security. I'd be more conservative > here and not query the values if not sure that it will succeed. > > No regressions found in jdk/sun/security/pkcs11. A new test added: > LargerDSAKey. > > -- > [1] - https://bugs.openjdk.java.net/browse/JDK-8271566 Martin Balao has updated the pull request incrementally with one additional commit since the last revision: P11Key static inner classes refactorings. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4961/files - new: https://git.openjdk.java.net/jdk/pull/4961/files/b02826b1..ce7c79ed Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4961=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4961=00-01 Stats: 802 lines in 3 files changed: 428 ins; 188 del; 186 mod Patch: https://git.openjdk.java.net/jdk/pull/4961.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4961/head:pull/4961 PR: https://git.openjdk.java.net/jdk/pull/4961
