On Fri, 13 Aug 2021 17:11:45 GMT, Martin Balao <mba...@openjdk.org> 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
