[13] RFR JDK-8080462: Update SunPKCS11 provider with PKCS11 v2.40 support

Wed May 29 20:12:36 UTC 2019

I agree with your comments below and the current CSR looks good so I've 
added my name as Reviewer. One main comment is that I don't think you 
should check the "Source" compatibility box. That would mean this would 
be a source-incompatible change [1], which I don't think is right. I 
don't think you should check any of the Compatibility boxes as according 
to the "Compatibility Risk Description" box, you said there is no 
compatibility risk.

--Sean

[1] https://wiki.openjdk.java.net/display/csr/Kinds+of+Compatibility

On 5/29/19 3:00 PM, Valerie Peng wrote:
> Hi Sean,
> 
> Much thanks for the feedbacks. They are very helpful... I have updated 
> the CSR per your feedbacks.
> 
> https://bugs.openjdk.java.net/browse/JDK-8221442
> 
> Please find more replies inline below.
> 
> On 5/28/2019 1:51 PM, Sean Mullan wrote:
> 
>> Hi Valerie,
>>
>> On 5/24/19 6:37 PM, Valerie Peng wrote:
>>> Hi Sean,
>>>
>>> Thanks much for the suggestion. I have added the info on newly 
>>> supported algorithms to both the CSR and the bug record. Please let 
>>> me know if you have more comments.
>>
>> - In the Summary section, add a hyperlink to the PKCS#11 v2.40 
>> standard and the errata
> 
> Done.
> 
>> - In general, I would put more information in the Specification 
>> section. I think attaching a patch of all the implementation changes 
>> is a bit too raw and not that useful as it is hard to discern what is 
>> specification and what is not (also the patch is not currently 
>> attached and pointing to a webrev is not acceptable per CSR rules 
>> since it may go away). Instead, I would avoid attaching a patch and 
>> instead include descriptions of the new attributes and algorithms in 
>> the Specification section in a format similar to that what is 
>> documented in 
>> https://docs.oracle.com/en/java/javase/12/security/pkcs11-reference-guide1.html.
>>
>> Basically, I think this CSR should include the information that is 
>> exposed or configurable to users outside of the implementation, which 
>> I think can be described in 2 types of use cases:
> 
> Yes, I think this is better. I was focusing on header file updates in 
> original CSR. Agree that it'd be useful to list changes exposed by this 
> RFE.
> 
>> 1. configuring a PKCS#11 provider (need to know what attributes are 
>> supported and their defaults)
> 
> Well, PKCS11 reference guide uses the term "attribute" to refer to 
> provider configuration options. This RFE did not add new provider 
> configuration options. However, there is also attributes defined in 
> PKCS#11 whose name starts with CKA_xxx. This RFE enhances SunPKCS11 
> provider to recognize new PKCS#11 attributes and won't error out with 
> exceptions when specified inside the configuration file entries as 
> attribute values, e.g.
> 
>     |attributes(*,CKO_PRIVATE_KEY,CKK_RSA) = { *CKA_DECRYPT* = true }|
> 
> These newly recognized PKCS#11 attributes (CKA_xxx), mechanisms 
> (CKM_xxx), key types (CKK_xxx), etc., are defined in 
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11Constants.java.
> 
>> 2. using it as a provider in an application (need to know what 
>> algorithms are supported and what is disabled/enabled by default)
> 
> Yes, these are the list of newly supported algorithms which I have added 
> to the CSR to address your earlier comment. None of them are disabled by 
> default. However, only when the underlying PKCS11 library also support 
> these, they will show up as available.
> 
>> - Are there new attributes that are now supported than what are 
>> currently listed in Table 5.1 of the PKCS#11 Reference Guide?: 
>> https://docs.oracle.com/en/java/javase/12/security/pkcs11-reference-guide1.html#GUID-C4ABFACB-B2C9-4E71-A313-79F881488BB9
> 
> If you are referring to attributes per the convention of PKCS#11 
> reference guide, no new provider configuration attributes added. 
> However, new PKCS#11 attributes are recognized. We don't list recognized 
> PKCS#11 attributes in the reference guide as they are quite extensive. 
> PKCS11 header files define a long list of constants and depending on 
> what they are for, it may or may not lead to an error. For example, 
> unrecognized mechanisms and error codes will be accepted and their long 
> values are used instead. However, unrecognized PKCS#11 attributes 
> specified in the PKCS#11 provider configuration file will lead to exception.
> 
>> If so, I think we should list them in the Specification section with 
>> the same details as in the Reference Guide.
> 
> Given the above, I suppose that we don't need to list out all newly 
> supported PKCS#11 attributes? They do impact the user in some way, but 
> not sure if it's too much details. Perhaps we can just state that 
> whatever supported in the PKCS#11 v2.40 headers are now recognized by 
> SunPKCS11 provider.
> 
>> - For the new algorithms, I would include those in the Specification 
>> section, in a format like table 5.3 in the PKCS#11 Reference Guide: 
>> https://docs.oracle.com/en/java/javase/12/security/pkcs11-reference-guide1.html#GUID-D3EF9023-7DDC-435D-9186-D2FD05674777
> 
> Sure, will do.
> 
>>
>> - I would include any new or changed defaults for attributes, etc.
> 
> No new defaults or new attributes for PKCS11 provider configurations.
> 
> Regards,
> Valerie
>>
>> --Sean
>>
>>>
>>> All,
>>>
>>> RFEs need to be integrated by 6/13. Can someone help reviewing this 
>>> soon? Mach5 run is clean. I up'ed the version of webrev to webrev.01 
>>> due to the additional support for RSASSA-PSS signatures.
>>>
>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8080462
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8221442
>>> Webrev: http://cr.openjdk.java.net/~valeriep/8080462/webrev.01/
>>>
>>> Thanks,
>>> Valerie
>>>
>>> On 5/22/2019 7:55 AM, Sean Mullan wrote:
>>>> On 5/21/19 7:19 PM, Valerie Peng wrote:
>>>>>
>>>>> I thought we always file CSR when updating the version of external 
>>>>> standard, e.g. documenting the import aspect of JDK.
>>>>
>>>> Good point though I think that was primarily based on whether the 
>>>> external standard was referenced in the javadocs of the standard 
>>>> APIs or influenced the behavior of existing APIs in some way. I 
>>>> don't think PKCS#11 is referenced from any of our standard APIs, but 
>>>> since this new version does add support for additional crypto 
>>>> algorithms via the standard APIs that weren't previously available, 
>>>> that sounds like a good enough reason for filing the CSR.
>>>>
>>>> I would recommend adding some additional details to the CSR to list 
>>>> what new features/algorithms PKCS#11 v2.40 provides and which 
>>>> standard APIs those features are applicable to. It would also be 
>>>> helpful to add similar details to the main issue and the release 
>>>> note as there aren't many details about what features are in the new 
>>>> version.
>>>>
>>>> Thanks,
>>>> Sean
>>>>
>>>>>
>>>>> I'd love to close/withdraw the CSR if it's not needed.
>>>>>
>>>>> Thanks,
>>>>> Valerie
>>>>> On 5/20/2019 12:11 PM, Sean Mullan wrote:
>>>>>> On 5/17/19 3:56 PM, Valerie Peng wrote:
>>>>>>>
>>>>>>> Thanks Martin for helping me troubleshoot NSS side, I added PSS 
>>>>>>> support into PKCS11 provider and added PSS-specific regression 
>>>>>>> tests. Please find webrev updated as below:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~valeriep/8080462/webrev.01/
>>>>>>>
>>>>>>> Can someone help review the CSR first as the approval may take a 
>>>>>>> week or so.
>>>>>>
>>>>>> I am curious why a CSR is needed? This seems to be strictly an 
>>>>>> implementation change with no compatibility effects.
>>>>>>
>>>>>> --Sean
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Valerie
>>>>>>> On 4/12/2019 5:05 PM, Valerie Peng wrote:
>>>>>>>>
>>>>>>>> Anyone has time to review this? Besides the header files update, 
>>>>>>>> I added support for AES/GCM/NoPadding support. Ran into some 
>>>>>>>> strange NSS error with RSASSA-PSS signature mechanism, so I have 
>>>>>>>> not included the PSS signature impl here.
>>>>>>>>
>>>>>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8080462
>>>>>>>>
>>>>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8080462/webrev.00/
>>>>>>>>
>>>>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8221442
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Valerie
>>>>>>>>
>>>>>>>>