On Tue, 2 Nov 2021 22:44:16 GMT, Valerie Peng <valer...@openjdk.org> 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, I appreciate that you took the time to explain your reasons. In any case, I believe that this iteration will be a significant improvement when compared to the old code, so I'm good to move forward on that. I'll check the other discussion points and your Webrev.01 proposal as soon as possible. Martin.- ------------- PR: https://git.openjdk.java.net/jdk/pull/4961
