RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]

[Martin Balao]

On Thu, 18 Nov 2021 18:37:38 GMT, Valerie Peng <valeriep at openjdk.org> 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::<e, d, p, q, pe, 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.-

-------------

PR: https://git.openjdk.java.net/jdk/pull/4961