Hi Martin,
I imported your webrev.06 and sun/security/pkcs11/tls/TestTLS12.java
still fails, but with a different reason (see below).
jib > STDERR:
jib > java.lang.RuntimeException: unable to expand property
pkcs11test.nss.libdir
jib > at
jdk.crypto.cryptoki/sun.security.pkcs11.Config.expand(Config.java:340)
jib > at
jdk.crypto.cryptoki/sun.security.pkcs11.Config.parseLibrary(Config.java:663)
jib > at
jdk.crypto.cryptoki/sun.security.pkcs11.Config.parse(Config.java:429)
jib > at
jdk.crypto.cryptoki/sun.security.pkcs11.Config.<init>(Config.java:212)
jib > at
jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11$1.run(SunPKCS11.java:113)
jib > at
jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11$1.run(SunPKCS11.java:110)
jib > at java.base/java.security.AccessController.doPrivileged(Native
Method)
jib > at
jdk.crypto.cryptoki/sun.security.pkcs11.SunPKCS11.configure(SunPKCS11.java:110)
jib > at PKCS11Test.getSunPKCS11(PKCS11Test.java:152)
jib > at TestTLS12.initializeProvider(TestTLS12.java:278)
jib > at TestTLS12.main(TestTLS12.java:96)
jib > at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
jib > at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
jib > at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
jib > at java.base/java.lang.reflect.Method.invoke(Method.java:566)
jib > at
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:115)
jib > at java.base/java.lang.Thread.run(Thread.java:834)
jib >
jib > JavaTest Message: Test threw exception: java.lang.RuntimeException:
unable to expand property pkcs11test.nss.libdir
jib > JavaTest Message: shutting down test
jib >
jib > STATUS:Failed.`main' threw exception: java.lang.RuntimeException:
unable to expand property pkcs11test.nss.libdir
When looking at the test itself, the test extends SecmodTest instead of
PKCS11Test, but then it did not call initSecmod() as other tests which
extend SecmodTest. Otherwise, most of PKCS11 tests extends PKCS11Test,
implement the abstract main(Provider) method, and then skip tests if the
specified provider does not support the required implementation.
Lastly, some nits which I forgot to include in my earlier emails
1) make sure the copyright years are current, i.e. 2018.
2) @author tags is past practice. We stopped this quite some years back.
Thanks,
Valerie
On 7/31/2018 4:48 PM, Valerie Peng wrote:
I sent this email a bit prematurely. I still need to review
TestTLS12.java.
All else is done.
Valerie
On 7/31/2018 4:26 PM, Valerie Peng wrote:
Hi Martin,
Here are the review comments for the remaining files:
<p11_convert.c>
- Always check non-null result for JNIEnv FindClass(...) calls, e.g.
line 530 and 802
- Sometimes the check is there but is after the return value has been
used. These checks should be moved up, i.e. right after the
FindClass(..) calls, e.g. line 555, 834.
- line 1262, should be CK_TLS12_MASTER_KEY_DERIVE_PARAMS instead of
CK_TLS12_MASTER_KEY_DERIVE_PARAMS_PTR
Thanks,
Valerie
On 7/26/2018 9:12 AM, Martin Balao wrote:
Hi Valerie,
Thanks for your feedback!
CKM_TLS12_MAC looks like it's not in use. Authentication codes are
calculated through CKM_TLS12_KEY_AND_MAC_DERIVE mechanism. Do you
know of a library supporting CKM_TLS12_MAC but not
CKM_TLS12_KEY_AND_MAC_DERIVE? I've been testing this with NSS
software token which does not support CKM_TLS12_MAC.
module-info.java changes were from when we used SSL/TLS constants,
but then I replaced these constants with hardcoded numbers -after
discussing with Xuelei-. I forgot to revert changes on this file.
Thanks.
Config.java changes were from a wrong version I uploaded. This has
been already fixed.
SunPKCS11.java: I agree with you that registration must be done
separately. SunTls12MasterSecret and SunTls12KeyMaterial are now
registered separately. I've added CKM_TLS12_MASTER_KEY_DERIVE_DH
mechanism to SunTls12MasterSecret algorithm. It's still assumed that
if CKM_TLS12_MASTER_KEY_DERIVE is supported, then
CKM_TLS12_MASTER_KEY_DERIVE_DH it's and vice-versa -same as with
previous key derivation mechanisms-. Not a big deal though I think:
it's unlikely that a library implements only one of these. Using
protocol version to decide which mechanism to use should be valid in
this case because if an instance of P11TlsMasterSecretGenerator is
used and algorithm is SunTls12MasterSecret, then protocol must be
TLS 1.2 and we can assume that there is support of these mechanisms
in the underlying library.
L527: I've realized that a better place for this map is
Functions.java. I've moved it there.
P11TlsPrfGenerator.java: This code is for TLS 1.2 only. That's
because CKM_TLS_MAC mechanism is registered for SunTls12Prf
algorithm. Previous TLS versions use CKM_TLS_PRF and
CKM_NSS_TLS_PRF_GENERAL.
P11TlsRsaPremasterSecretGenerator.java: Yes, we can remove it and
call all keys "TlsRsaPremasterSecret", disregarding the mechanism
used to generate it (CKM_SSL3_PRE_MASTER_KEY_GEN or
CKM_TLS_PRE_MASTER_KEY_GEN). At the end of the day, this is just an
array of 48 bytes. I've removed the field too.
Functions.java: New entries (CKM_TLS12_MASTER_KEY_DERIVE,
CKM_TLS12_MASTER_KEY_DERIVE_DH and CKM_TLS_MAC) moved after line
721. CKM_TLS12_KEY_AND_MAC_DERIVE has been added for completeness.
CK_ATTRIBUTE.java: SENSITIVE_TRUE removed.
Removed a dependency in TestTLS12.java to HexDumper (internal).
Webrev 06 with all these changes here:
*
http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.06/
<http://cr.openjdk.java.net/%7Embalao/webrevs/8029661/8029661.webrev.06/>
*
http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.06.zip
<http://cr.openjdk.java.net/%7Embalao/webrevs/8029661/8029661.webrev.06.zip>
Regression testing on jdk/sun/security/pkcs11 passes with samae pass
rate.
Kind regards,
Martin.-
On Tue, Jul 24, 2018 at 11:00 PM, Valerie Peng
<valerie.p...@oracle.com <mailto:valerie.p...@oracle.com>> wrote:
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
