Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v4]
On Sat, 4 Dec 2021 22:03:53 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: > > Null check added and a fix in the security test group Looks good. Thanks! Valerie - Marked as reviewed by valeriep (Reviewer). 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 , 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 [v4]
> 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: Null check added and a fix in the security test group - Changes: - all: https://git.openjdk.java.net/jdk/pull/4961/files - new: https://git.openjdk.java.net/jdk/pull/4961/files/00564198..1e53b6d0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4961=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4961=02-03 Stats: 10 lines in 2 files changed: 2 ins; 0 del; 8 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
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v3]
On Fri, 3 Dec 2021 19:48:53 GMT, Valerie Peng wrote:
>> Martin Balao has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains six commits:
>>
>> - 8271566: DSA signature length value is not accurate in P11Signature
>> (Webrev.02 based)
>> - Merge branch 'master' into JDK-8271566
>> - Revert 8271566: DSA signature length value is not accurate in P11Signature
>> - Revert P11Key static inner classes refactorings.
>> - P11Key static inner classes refactorings.
>> - 8271566: DSA signature length value is not accurate in P11Signature
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java line
> 126:
>
>> 124: boolean sensitive = false;
>> 125: boolean extractable = true;
>> 126: for (CK_ATTRIBUTE attr : attrs) {
>
> Just noticed this: add a check for non-null attrs? If null, skip the for-loop.
I've noticed that when reviewing your Webrev.01 but thought that the change was
on purpose because of the P11Key constructor callers, which do not currently
pass null. Anyways, I'll add the null check again just in case.
> test/jdk/TEST.groups line 202:
>
>> 200:
>> 201: jdk_security1 = \
>> 202: sun/security/pkcs11
>
> Please discard this, this is just for quick testing, it's not meant to be
> part of this change...
Well spotted.
-
PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v3]
On Thu, 2 Dec 2021 21:31:52 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 with a new target base due to a > merge or a rebase. The pull request now contains six commits: > > - 8271566: DSA signature length value is not accurate in P11Signature > (Webrev.02 based) > - Merge branch 'master' into JDK-8271566 > - Revert 8271566: DSA signature length value is not accurate in P11Signature > - Revert P11Key static inner classes refactorings. > - P11Key static inner classes refactorings. > - 8271566: DSA signature length value is not accurate in P11Signature test/jdk/TEST.groups line 202: > 200: > 201: jdk_security1 = \ > 202: sun/security/pkcs11 Please discard this, this is just for quick testing, it's not meant to be part of this change... - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v3]
On Thu, 2 Dec 2021 21:31:52 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 with a new target base due to a
> merge or a rebase. The pull request now contains six commits:
>
> - 8271566: DSA signature length value is not accurate in P11Signature
> (Webrev.02 based)
> - Merge branch 'master' into JDK-8271566
> - Revert 8271566: DSA signature length value is not accurate in P11Signature
> - Revert P11Key static inner classes refactorings.
> - P11Key static inner classes refactorings.
> - 8271566: DSA signature length value is not accurate in P11Signature
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java line 126:
> 124: boolean sensitive = false;
> 125: boolean extractable = true;
> 126: for (CK_ATTRIBUTE attr : attrs) {
Just noticed this: add a check for non-null attrs? If null, skip the for-loop.
-
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
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature
On Fri, 6 Aug 2021 20:51:23 GMT, Martin Balao wrote: > > > Yes, I see what you mean. Contrary to P11PrivateKey::getFormat and > P11PrivateKey::getEncodedInternal where a 'null' returned value is documented > in java.security.Key, we don't have that documentation for the other > interfaces such as java.security.interfaces.DSAPrivateKey. That can lead to > NPE if the client casts the P11Key to the interface, invokes the method and > depends on a non-null value. I will give this some thought and try to come up > with something, because the information is already there and (in reality) we > need it internally only. It's clear that all these interfaces were not > designed with unextractable P11 keys in mind, because it makes sense to me > (conceptually) to have a private key from which we can retrieve some values > (public, such as the parameters) and not other ones. Right, PKCS11 and unextractable keys come a few releases later after the JCA API. So, there may be some difficulties trying to work them into existing APIs. In order to maintain backward compatibility, API changes are also limited. - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature
On Mon, 2 Aug 2021 19:31:54 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 Yes, I see what you mean. Contrary to P11PrivateKey::getFormat and P11PrivateKey::getEncodedInternal where a 'null' returned value is documented in java.security.Key, we don't have that documentation for the other interfaces such as java.security.interfaces.DSAPrivateKey. That can lead to NPE if the client casts the P11Key to the interface, invokes the method and depends on a non-null value. I will give this some thought and try to come up with something, because the information is already there and (in reality) we need it internally only. It's clear that all these interfaces were not designed with unextractable P11 keys in mind, because it makes sense to me (conceptually) to have a private key from which we can retrieve some values (public, such as the parameters) and not other ones. - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature
On Tue, 3 Aug 2021 21:05:24 GMT, Valerie Peng 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
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java line
> 386:
>
>> 384: (attributes[2].getBoolean() == false)) {
>> 385: keySensitive = true;
>> 386: }
>
> nit: this block and line 377 can simply be:
> `boolean keySensitive = (attributes[0].getBoolean() ||
> attributes[1].getBoolean() || (!attributes[2].getBoolean()));`
Sure
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java
> line 123:
>
>> 121:
>> 122: // signature length expected or 0 for unknown
>> 123: private int signatureLength;
>
> nit: use a shorter name, e.g. sigLen, so to fit in one line.
Ok
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java
> line 817:
>
>> 815: }
>> 816:
>> 817: private byte[] asn1ToDSA(byte[] sig) throws SignatureException {
>
> Have you considered keeping this as a static method but add one more int
> argument, i.e. signature length? It seems that the method asn1ToECDSA() can
> be made static too.
Ok
> test/jdk/sun/security/pkcs11/Signature/LargeDSAKey.java line 52:
>
>> 50: "Known text known text known text";
>> 51:
>> 52: public void main(Provider p) throws Exception {
>
> nit: add @Override
Sure
-
PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature
On Mon, 2 Aug 2021 19:31:54 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 Hmm, I ran more security regression tests against the proposed changes and had a second thought about this proposed P11Key change. When the underlying P11 DSA key is un-extractable and returned as a P11Key which implements DSAPrivateKey interface. With the proposed change, calling getX() upon this key object returns null which will lead to unexpected NPE. This is a serious problem. Same goes for other private keys, e.g. EC, DH. And not just the private value of these keys, but also the encodings may lead to NPE. Thus, for un-extractable keys, we will have to continue to return them as P11PrivateKey objects which does not implement any algorithm specific interface. - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature
On Mon, 2 Aug 2021 19:31:54 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 As for your changes to P11Key class, I wonder why NSS doesn't make the "extractable" attribute false for their token keys? I agree that this change makes sense for NSS given their current impl but not sure how other vendor may be impacted by this. So, how about: 1) keep this change specific to NSS. 2) now that various P11XXXPrivateKey classes can also be sensitive and un-extractable, perhaps we should just make P11PrivateKey class a general class which these P11XXXPrivateKey extends from and store the "sensitiveKey" logic inside the P11PrivateKey constructor. This way, you don't need to change the constructor signature to add this extra boolean argument. The subclasses can just access the field in P11PrivateKey class and decide. 3) for RSA private keys, instead of just use P11PrivateKey, it could still use P11RSAPrivateNonCRTKey if the CRT attributes aren't available due to being a token key. Or, perhaps availability of public exponent attribute would give an indication? - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature
On Mon, 2 Aug 2021 19:31:54 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
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java
line 123:
> 121:
> 122: // signature length expected or 0 for unknown
> 123: private int signatureLength;
nit: use a shorter name, e.g. sigLen, so to fit in one line.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java
line 817:
> 815: }
> 816:
> 817: private byte[] asn1ToDSA(byte[] sig) throws SignatureException {
Have you considered keeping this as a static method but add one more int
argument, i.e. signature length? It seems that the method asn1ToECDSA() can be
made static too.
-
PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature
On Mon, 2 Aug 2021 19:31:54 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
test/jdk/sun/security/pkcs11/Signature/LargeDSAKey.java line 52:
> 50: "Known text known text known text";
> 51:
> 52: public void main(Provider p) throws Exception {
nit: add @Override
-
PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature
On Mon, 2 Aug 2021 19:31:54 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
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java line 386:
> 384: (attributes[2].getBoolean() == false)) {
> 385: keySensitive = true;
> 386: }
nit: this block and line 377 can simply be:
`boolean keySensitive = (attributes[0].getBoolean() ||
attributes[1].getBoolean() || (!attributes[2].getBoolean()));`
-
PR: https://git.openjdk.java.net/jdk/pull/4961
