<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body 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.java><br>
- Why removing the <span class="removed">SENSITIVE_FALSE attribute
on line 829 and 854?<br>
</span><br>
<sun/security/pkcs11/P11TlsKeyMaterialGenerator.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/P11TlsMasterSecretGenerator.java><br>
- Please see above comments for P11TlsKeyMaterialGenerator.java<br>
<br>
<sun/security/pkcs11/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="new"><sun/security/pkcs11/P11TlsRsaPremasterSecretGenerator.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/SunPKCS11.java><br>
- 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.<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/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="new"><span class="new"><span class="new"></span></span><br>
I still have a few files left and will send you comments on them
later.<br>
Thanks,<br>
Valerie<br>
<br>
<br>
<br>
</span>
<div class="moz-cite-prefix">On 7/13/2018 2:08 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:bb8d9731-3844-2fb7-4e16-4ed980227a74@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<p>Thanks for updating the webrev, I will take a look.</p>
<p>Valerie<br>
</p>
<br>
<div class="moz-cite-prefix">On 7/10/2018 10:18 AM, Martin Balao
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAKZz+geWNMxrFhJ2FOuS_8qavsTUTqLb2C9iwFaShzE4mNUUCA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<div dir="ltr">
<div>Hi,</div>
<div><br>
</div>
<div>Webrev 04 for JDK-8029661 is ready:</div>
<div><br>
</div>
<div> * <a
href="http://cr.openjdk.java.net/%7Embalao/webrevs/8029661/8029661.webrev.04.zip"
moz-do-not-send="true">http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.04.zip</a></div>
<div> * <a
href="http://cr.openjdk.java.net/%7Embalao/webrevs/8029661/8029661.webrev.04/"
moz-do-not-send="true">http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.04/</a></div>
<div><br>
</div>
<div>New:</div>
<div><br>
</div>
<div> * Rebased to latest JDK revision (after TLS 1.3 merge)</div>
<div> * Rev 1acfd2f56d72</div>
<div> * ProtocolVersion dependencies removed (raw TLS protocol
version numbers are now used)</div>
<div> * Code style improvements (tabs, trailing whitespaces,
max lines length, etc.)</div>
<div><br>
</div>
<div>Thanks,</div>
<div>Martin.-</div>
</div>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>