<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Martin,</p>
<br>
Since there is no regression test for this test, you will need to
add a "noreg-xxx" label to the bug record. For this issue, it'll
probably be "noreg-hard".<br>
<br>
To understand the changes better, please find my questions and some
review comments below:<br>
<br>
<src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java><br>
- line 397: It's incorrect to change initialize() to
ensureInitialized(). If the cipher object were to be initialized
twice with two different keys, you need to re-initialize again. <br>
- line 471: Why do you change it to Throwable from PKCS11Exception?
<br>
<br>
<src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java><br>
- line 99: comment is somewhat unclear. typo: "temporal" should be
"temporary".<br>
<span class="new">- line 148-149: </span><span class="new">JDK-8165996
has been fixed. This does not apply now, does it?</span><span
class="new"><br>
- You changed the </span><span class="new"><span class="new">constructors
of </span>all the P11Key-related classes to take an additional
boolean argument "tmpNativeKey". However, this boolean argument is
only used when the underlying token is "NSS". Why limiting to NSS
only? It seems that all callers except for PKCS11 KeyStore pass
true for "tmpNativeKey". When "tmpNativeKey" is true, the keyID
handle first destroyed in constructor and later created again (ref
count == 1) and destroyed (when ref count drops to 0). This
replaced the PhantomReference approach in </span>SessionKeyRef
class, right? Now both approaches seem to be used and key
destruction is taking places at different methods. I think we should
clarify the cleanup model and perhaps refactor the code so it's
easier which approach is in use. Or grouping all these
cleanup-related fields/variables into a separate class for
readability/clarity.<br>
<span class="new">- line 157-175: </span><span class="new">nativeKeyWrapperKeyID
is a static variable, shouldn't it be synchronized on a class
level object?</span><br>
<span class="new">- line 1343: the impl doesn't look right. Why do
you call both removeObject() and addObject() here with the same
check?<br>
</span><span class="new">- line 1363: the change seems incorrect,
you should still call session.removeObject(). the call
setKeyIDAndSession(0, null) does not lower the session object
count.<br>
</span><br>
Thanks,<br>
<span class="new"></span>Valerie<br>
<span class="new"></span><br>
<div class="moz-cite-prefix">On 8/7/2018 5:41 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:6b00e3d4-c795-f3e4-38ca-fa897f8680ea@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<p>Hi Martin,</p>
<p>Thanks for the update, I will resume the review on this one
with your latest webrev.</p>
<p>BTW, there is no webrev.07 for your other fix, i.e.
JDK-8029661, right? Just checking.</p>
Regards,<br>
Valerie<br>
<br>
<div class="moz-cite-prefix">On 8/3/2018 2:13 PM, Martin Balao
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAKZz+gd6YcFNcCD1_r4-r9jdzRcCXTVPmoGBJdSXaY1m7sc6Wg@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> * <a
href="http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.10/"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~<wbr>mbalao/webrevs/6913047/<wbr>6913047.webrev.10/</a></div>
<div> * <a
href="http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.10.zip"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~<wbr>mbalao/webrevs/6913047/<wbr>6913047.webrev.10.zip</a></div>
<div><br>
</div>
<div>New in webrev 10:</div>
<div><br>
</div>
<div> * Bug fix when private DSA/EC keys are sensitive</div>
<div> * I found this bug removing "attributes =
compatibility" entry in NSS configuration file so keys were
CKA_SENSITIVE.</div>
<div> * This is really an NSS bug when unwrapping ephemeral
keys (NSC_UnwrapKey function), because CKA_NETSCAPE_DB
attribute is required but not used/needed. There was a
similar bug when creating objects (NSC_CreateObject
function), fixed many years ago [1].</div>
<div> * In those cases in which the bug occurs (private keys,
of DSA/EC type, sensitive and without CKA_NETSCAPE_DB
attribute set), I store an empty CKA_NETSCAPE_DB attribute
in the buffer that will later be used for key unwrapping.
I'm not doing a C_SetAttributeValue call because keys are
read-only. We can let users set CKA_NETSCAPE_DB attribute in
NSS configuration file [2] but this would be a workaround on
users side.</div>
<div> * Changes in:</div>
<div> * p11_keymgmt.c</div>
<div> * L175-187, L212-214 and L275-278</div>
<div><br>
</div>
<div> * Bug fix when storing sensitive RSA keys in a keystore</div>
<div> * CKA_NETSCAPE_DB attribute is not needed so we return
it as null (instead of failing with an "Invalid algorithm"
message)</div>
<div> * Changes in:</div>
<div> * P11KeyStore.java</div>
<div> * L1909-1914</div>
<div><br>
</div>
<div> * Lines length was cut to improve code readability</div>
<div><br>
</div>
<div>Regression tests on jdk/sun/security/pkcs11 category
passed. I ran my internal test suite too, that covers the
following services (with SunPKCS11 provider): Cipher,
Signature, KeyAgreement, Digest, Mac, KeyGenerator,
KeyPairGenerator and Keystore.</div>
<div><br>
</div>
<div>Kind regards,</div>
<div>Martin.-</div>
<div><br>
</div>
<div>--</div>
<div>[1] - <a
href="https://bugzilla.mozilla.org/show_bug.cgi?id=309701#c6"
target="_blank" moz-do-not-send="true">https://bugzilla.mozilla.org/<wbr>show_bug.cgi?id=309701#c6</a></div>
<div>[2] - <a
href="https://alvinalexander.com/java/jwarehouse/openjdk-8/jdk/test/sun/security/pkcs11/fips/fips.cfg.shtml"
target="_blank" moz-do-not-send="true">https://alvinalexander.com/<wbr>java/jwarehouse/openjdk-8/jdk/<wbr>test/sun/security/pkcs11/fips/<wbr>fips.cfg.shtml</a></div>
<div><br>
</div>
</div>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>