[14] RFR JDK-8229243 "SunPKCS11-Solaris provider tests failing on Solaris 11.4"

[Xuelei Fan]

I have no more comment.

Xuelei

On 10/2/2019 5:09 PM, Valerie Peng wrote:
> During the pre-integration mach5 run, I ran into a window-specific data 
> alignment problem when the custom GCM param structure is declared inside 
> pkcs11wrapper.h in webrev.01. So, I ended up adding a separate header 
> file pkcs11gcm2.h containing only this structure and include it in the 
> platform-specific header p11_md.h so the current logic of using sizeof() 
> works as expected.
> 
> Mach5 run and Solaris 11.4 run pass as expected with webrev.02.
> 
> http://cr.openjdk.java.net/~valeriep/8229243/webrev.02/
> 
> If there are more comments, please let me know in a few days.
> 
> Thanks!
> Valerie
> On 9/24/2019 9:45 PM, Valerie Peng wrote:
>> Great, thanks for the review and feedback~
>>
>> Valerie
>>
>> On 9/24/2019 9:20 PM, Xuelei Fan wrote:
>>> I looked at the latest webrev:
>>>   http://cr.openjdk.java.net/~valeriep/8229243/webrev.01/
>>>
>>> Which is good to me.
>>>
>>> Thanks,
>>> Xuelei
>>>
>>> On 9/19/2019 5:46 PM, Valerie Peng wrote:
>>>>
>>>> I am not on the PKCS#11 committee and not sure about the plan.
>>>> As for which one is right, I am more inclined to the "spec is right" 
>>>> side which is also what NSS picked.
>>>> Comparing between spec and header, shouldn't the former get more 
>>>> eyeballs in terms of review?
>>>> The header file also has a deprecated structure CK_AES_GCM_PARAMS 
>>>> which contains both ulIvLen and ulIvBits fields.
>>>> As ulIvBits and ulIvLen are essentially same in terms of meaning 
>>>> except bytes vs bits. Having both just creates confusion and 
>>>> potential inconsistency and makes not much sense to me.
>>>>
>>>> Valerie
>>>>
>>>> ----- Original Message -----
>>>> From: xuelei.fan at oracle.com
>>>> To: valerie.peng at oracle.com, security-dev at openjdk.java.net
>>>> Sent: Thursday, September 19, 2019 7:47:43 AM GMT -08:00 US/Canada 
>>>> Pacific
>>>> Subject: Re: [14] RFR JDK-8229243 "SunPKCS11-Solaris provider tests 
>>>> failing on Solaris 11.4"
>>>>
>>>> Will the inconsistency structure be continue?  I was just wondering if
>>>> OpenHSM2/Solaris/NSS will fix the bug and use one structure in the
>>>> future, then we may not need to workaround the issue in the calling 
>>>> side.
>>>>
>>>> I had a quick look the PKCS#11 3.0 draft, there is no update of the
>>>> structure yet.
>>>>
>>>> Xuelei
>>>>
>>>> On 9/18/2019 6:01 PM, Valerie Peng wrote:
>>>>> Ping?
>>>>>
>>>>> Can someone help take a look?
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Valerie
>>>>>
>>>>> On 8/15/2019 4:49 PM, Valerie Peng wrote:
>>>>>>
>>>>>> Anyone has time to help review this fix? PKCS#11 v2.40 has
>>>>>> inconsistent definition for CK_GCM_PARAMS struct. The mechanism spec
>>>>>> (sec 2..12.3) has:
>>>>>>
>>>>>>      typedef struct CK_GCM_PARAMS {
>>>>>>          CK_BYTE_PTR       pIv;
>>>>>>          CK_ULONG          ulIvLen;
>>>>>>          CK_BYTE_PTR       pAAD;
>>>>>>          CK_ULONG          ulAADLen;
>>>>>>          CK_ULONG          ulTagBits;
>>>>>>      } CK_GCM_PARAMS;
>>>>>>
>>>>>> However, the header file pkcs11t.h has an extra "ulIvBits" field:
>>>>>>
>>>>>>      typedef struct CK_GCM_PARAMS {
>>>>>>          CK_BYTE_PTR       pIv;
>>>>>>          CK_ULONG          ulIvLen;
>>>>>>      *    CK_ULONG          ulIvBits;*
>>>>>>          CK_BYTE_PTR       pAAD;
>>>>>>          CK_ULONG          ulAADLen;
>>>>>>          CK_ULONG          ulTagBits;
>>>>>>      } CK_GCM_PARAMS;
>>>>>>
>>>>>> Some vendors such OpenHSM2 and Solaris go with the header file while
>>>>>> some vendor such as NSS go with the mech spec. Current SunPKCS11
>>>>>> provider impl works with NSS but not with other vendors whose
>>>>>> CK_GCM_PARAMS struct contains ulIvBits field. Based on testing
>>>>>> results, OpenHSM2 and Solaris error out at init time when the
>>>>>> parameter length does not have their expected value. Thus, one way to
>>>>>> workaround this inconsistency is to re-try with a different structure
>>>>>> for AES GCM when the init failed. In addition, given the parameters
>>>>>> contains Iv and AAD which some vendor may use during subsequent
>>>>>> enc/dec operations, I changed to use the same model as RSASSA-PSS to
>>>>>> defer the freeing after the enc/dec operation is finished. Verified
>>>>>> the changes on Solaris 11.4 against existing GCM regression tests.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8229243
>>>>>>
>>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8229243/webrev.00/
>>>>>>
>>>>>> Thanks,
>>>>>> Valerie