RFR JDK-8029661: JDK-Support TLS v1.2 algorithm in SunPKCS11 provider

[Valerie Peng]

Hi Martin,

Sorry for the delay, I am able to resume on reviewing this now.

Here are some initial comments.

What about CKM_TLS12_MAC? I see that it's defined under TLS 1.2 
mechanisms, but not much other details for it.

<module-info.java>
- Is this change still necessary? I didn't notice any new import 
statement with sun.security.ssl package in the rest of changes.

<sun/security/pkcs11/Config.java>
- Why removing the SENSITIVE_FALSE attribute on line 829 and 854?

<sun/security/pkcs11/P11TlsKeyMaterialGenerator.java>
- As my comments for the SunPKCS11.java (see below), I think we should 
not assume the support for CKM_TLS12_KEY_AND_MAC_DERIVE. The existing 
impl and how it's registered in SunPKCS11 class is already somewhat 
problematic when CKM_TLS_KEY_AND_MAC_DERIVE is not supported. We should 
avoid assuming CKM_TLS12_KEY_AND_MAC_DERIVE is supported which may 
create even more problems. Overriding this.mechanism based on the TLS 
version specified in the parameters from the engineInit(...) call will 
lead to failure when the mechanism is not supported by underlying PKCS11 
library

<sun/security/pkcs11/P11TlsMasterSecretGenerator.java>
- Please see above comments for P11TlsKeyMaterialGenerator.java

<sun/security/pkcs11/P11TlsPrfGenerator.java>
- Retrieve label outside of the new code block for TLS 1.2, i.e. line 
133- 167, as it's used by all mechanisms.

<sun/security/pkcs11/P11TlsRsaPremasterSecretGenerator.java>
- Line 131, Is it really necessary to use SunTls12RsaPremasterSecret? 
SunJCE provider uses SunTlsRsaPremasterSecret for all cases. If 
different algorithm is not needed, then no need to save tlsVersion as a 
field

<sun/security/pkcs11/SunPKCS11.java>
- Hmm, for TLS 12 specific mechanisms, some PKCS11 libraries may not 
support them. Instead of registering 
SunTls12MasterSecret/SunTls12KeyMaterial as aliases, they should be 
registered separately so that if the native TLS 12 mechanisms were not 
supported, the new entry will be skipped. The corresponding 
implementation classes such as P11TlsMasterSecretGenerator and 
P11TlsKeyMaterialGenerator will have to check the specified parameter 
spec in their engineInit(..) calls to make sure things line up, e.g. 
error out if the TLS version in the spec does not match the native 
mechanism.
- Lines 528 - 532, the mapping is stored without checking for support. 
May become an issue when the underlying PKCS11 library does not support 
all of these hash mechanisms. Probably not a big deal as these are 
fairly common hash algorithms, but I think it'd be good to add a comment 
on line 527 with something like "// assuming all hash mechanisms below 
are supported".

<sun/security/pkcs11/wrapper/Functions.java>
- Miss the mapping for CKM_TLS12_KEY_AND_MAC_DERIVE? Move these new 
entries to be after the existing SSL3/TLS entries (from line 711-721)

I still have a few files left and will send you comments on them later.
Thanks,
Valerie



On 7/13/2018 2:08 PM, Valerie Peng wrote:
>
> Thanks for updating the webrev, I will take a look.
>
> Valerie
>
>
> On 7/10/2018 10:18 AM, Martin Balao wrote:
>> Hi,
>>
>> Webrev 04 for JDK-8029661 is ready:
>>
>>  * 
>> http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.04.zip 
>> <http://cr.openjdk.java.net/%7Embalao/webrevs/8029661/8029661.webrev.04.zip>
>>  * 
>> http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.04/ 
>> <http://cr.openjdk.java.net/%7Embalao/webrevs/8029661/8029661.webrev.04/>
>>
>> New:
>>
>>  * Rebased to latest JDK revision (after TLS 1.3 merge)
>>   * Rev 1acfd2f56d72
>>  * ProtocolVersion dependencies removed (raw TLS protocol version 
>> numbers are now used)
>>  * Code style improvements (tabs, trailing whitespaces, max lines 
>> length, etc.)
>>
>> Thanks,
>> Martin.-
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20180724/79ef6cf7/attachment.htm>