<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Martin,</p>
<p>I imported your webrev.06 and
sun/security/pkcs11/tls/TestTLS12.java still fails, but with a
different reason (see below).<br>
</p>
<blockquote>
<pre>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</pre>
</blockquote>
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.<br>
<br>
Lastly, some nits which I forgot to include in my earlier emails<br>
1) make sure the copyright years are current, i.e. 2018.<br>
2) @author tags is past practice. We stopped this quite some years
back.<br>
<br>
Thanks, <br>
Valerie<br>
<br>
<div class="moz-cite-prefix">On 7/31/2018 4:48 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:a067bb33-4802-d12f-a4ee-ab081190a53d@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<p>I sent this email a bit prematurely. I still need to review
TestTLS12.java.</p>
<p>All else is done.</p>
<p>Valerie<br>
</p>
<br>
<div class="moz-cite-prefix">On 7/31/2018 4:26 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:7203910f-333f-89d5-ea04-ffb4b42ac542@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<p>Hi Martin,</p>
<p>Here are the review comments for the remaining files:</p>
<p><p11_convert.c><br>
</p>
- Always check non-null result for JNIEnv FindClass(...) calls,
e.g. line 530 and 802<span class="changed"></span><span
class="changed"></span><br>
<span class="changed"></span><br>
<span class="changed">- 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.</span><br>
<span class="changed"></span><br>
- line 1262, should be <span class="new">CK_TLS12_MASTER_KEY_DERIVE_PARAMS
instead of </span><span class="new">CK_TLS12_MASTER_KEY_DERIVE_PARAMS_PTR</span><br>
<span class="new"></span><span class="new"><br>
</span><span class="new"></span><br>
<span class="new">Thanks,</span><br>
<span class="new">Valerie<br>
</span>
<p><span class="changed"></span></p>
<br>
<div class="moz-cite-prefix">On 7/26/2018 9:12 AM, Martin Balao
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAKZz+gch_6QEJGy=NgAKKVyLfyc4GrtJiU=7icxruECPMMcMPA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<div dir="ltr">
<div>Hi Valerie,</div>
<div><br>
</div>
<div>Thanks for your feedback!</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Config.java changes were from a wrong version I
uploaded. This has been already fixed.</div>
<div><br>
</div>
<div>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.</div>
<div>L527: I've realized that a better place for this map is
Functions.java. I've moved it there.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>CK_ATTRIBUTE.java: SENSITIVE_TRUE removed.</div>
<div><br>
</div>
<div>Removed a dependency in TestTLS12.java to HexDumper
(internal).</div>
<div><br>
</div>
<div>Webrev 06 with all these changes here:</div>
<div><br>
</div>
<div> * <a
href="http://cr.openjdk.java.net/%7Embalao/webrevs/8029661/8029661.webrev.06/"
moz-do-not-send="true">http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.06/</a></div>
<div> * <a
href="http://cr.openjdk.java.net/%7Embalao/webrevs/8029661/8029661.webrev.06.zip"
moz-do-not-send="true">http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.06.zip</a></div>
<div><br>
</div>
<div>Regression testing on jdk/sun/security/pkcs11 passes
with samae pass rate.</div>
<div><br>
</div>
<div>Kind regards,</div>
<div>Martin.-</div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Tue, Jul 24, 2018 at 11:00 PM,
Valerie Peng <span dir="ltr"><<a
href="mailto:valerie.peng@oracle.com"
target="_blank" moz-do-not-send="true">valerie.peng@oracle.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<p>Hi Martin,</p>
<p>Sorry for the delay, I am able to resume on
reviewing this now.</p>
<p>Here are some initial comments.<br>
</p>
<p>What about CKM_TLS12_MAC? I see that it's defined
under TLS 1.2 mechanisms, but not much other
details for it.<br>
</p>
<module-info.java><br>
- Is this change still necessary? I didn't notice
any new import statement with sun.security.ssl
package in the rest of changes.<br>
<br>
<sun/security/pkcs11/Config.<wbr>java><br>
- Why removing the <span
class="m_-1525013007785267772removed">SENSITIVE_FALSE
attribute on line 829 and 854?<br>
</span><br>
<sun/security/pkcs11/<wbr>P11TlsKeyMaterialGenerator.<wbr>java><br>
- 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<br>
<br>
<sun/security/pkcs11/<wbr>P11TlsMasterSecretGenerator.<wbr>java><br>
- Please see above comments for
P11TlsKeyMaterialGenerator.<wbr>java<br>
<br>
<sun/security/pkcs11/<wbr>P11TlsPrfGenerator.java><br>
- Retrieve label outside of the new code block for
TLS 1.2, i.e. line 133- 167, as it's used by all
mechanisms.<br>
<br>
<span class="m_-1525013007785267772new"><sun/security/pkcs11/<wbr>P11TlsRsaPremasterSecretGenera<wbr>tor.java><br>
- 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<br>
<br>
</span><sun/security/pkcs11/<wbr>SunPKCS11.java><br>
- Hmm, for TLS 12 specific mechanisms, some PKCS11
libraries may not support them. Instead of
registering SunTls12MasterSecret/<wbr>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.<br>
- 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".<br>
<br>
<sun/security/pkcs11/wrapper/<wbr>Functions.java><br>
- 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) <br>
<span class="m_-1525013007785267772new"><span
class="m_-1525013007785267772new"><span
class="m_-1525013007785267772new"></span></span><br>
I still have a few files left and will send you
comments on them later.<br>
Thanks,<br>
Valerie</span></div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>