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. 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 ------------- PR: https://git.openjdk.java.net/jdk/pull/4961
