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

[Valerie Peng]

Thanks for the review~
Valerie
On 10/4/2019 8:30 PM, Xuelei Fan wrote:
> 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