[14] RFR JDK-8176837 "SunPKCS11 provider needs to check more details on PKCS11 Mechanism"

[Xuelei Fan]

Looks good to me.

Thanks,
Xuelei

On 9/19/2019 5:57 PM, Valerie Peng wrote:
> Hi Xuelei,
> 
> I have added the debugging output for the "unknown" case as you suggested.
> 
> Webrev updated in case you feel like taking another look.
> http://cr.openjdk.java.net/~valeriep/8176837/webrev.01/
> 
> Thanks for the review~
> Valerie
> 
> ----- Original Message -----
> From: xuelei.fan at oracle.com
> To: valerie.peng at oracle.com, security-dev at openjdk.java.net
> Sent: Wednesday, September 18, 2019 6:47:31 PM GMT -08:00 US/Canada Pacific
> Subject: Re: [14] RFR JDK-8176837 "SunPKCS11 provider needs to check more details on PKCS11 Mechanism"
> 
> Just a very minor nit.
> 
> In the initToken() implementation, there is a check for the upper 32
> bits set of a mech and may continue to next mech.  The continue may
> ignore the showinfo dump for some cases in the patch.
> 
> Maybe, it might be more straightforward if we don't want to merge the
> log in one place:
> 1. always show the mech info log for every supported mechanism.
> 2. if not enabled, log with "DISABLED ...".
> 3. if upper 32 bits set, log with "UNKNOWN ...".
> 4. if legacy, log with "Legacy ...".
> 
>      for each mech {
>          if (showInfo) {
>              System.out.println("Mechanism " +
>                  Functions.getMechanismName(longMech) + ": " +
>                  p11.C_GetMechanismInfo(slotID, longMech);
>          }
> 
>          if (!config.isEnabled(longMech)) {
>              System.out.println("DISABLED in configuration");
> 
>              continue;
>          }
> 
>          if (longMech >>> 32 != 0) {
>              System.out.println("Unknown mechanism in configuration");
> 
>              continue;
>          }
> 
>          int mech = (int)longMech;
>          if (isLegacy(token, mech)) {
>              System.out.println("Legacy mechanism in configuration");
> 
>              continue;
>          }
> 
>          ...
>      }
> 
> Otherwise, looks good to me.  Just a nit, no more review cycle is needed
> for me.
> 
> Thanks,
> Xuelei
> 
> On 9/18/2019 5:56 PM, Valerie Peng wrote:
>>
>> Can someone help reviewing this fix? AFAIK, only S11.4 have such
>> mechanisms, e.g. RC4 which only does decryption but not encryption.
>> But the checks seems reasonable to apply to all PKCS11 libraries when
>> querying mechanisms.
>> No new regression test needed as this is already caught by running
>> existing regression tests on S11.4.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8176837
>> Webrev: http://cr.openjdk.java.net/~valeriep/8176837/webrev.00/
>>
>> Thanks,
>> Valerie