<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 9/4/2018 9:59 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:acc35122-91f4-6d2c-2101-70136316cf9c@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
These sun.security.pkcs11.wrapper classes are internal and subject
to changes without notice.<br>
</blockquote>
No arguments there. But that interface has been stable since the
initial contribution and to be blunt - the PKCS11 provider only
works well if you use the keys you created through the provider.
There's a set of idiosyncratic choices for how to map keys to certs
to keys that make keys created by non-provider calls (e.g. C code or
other non-java libs) to the token difficult to use through the
provider and vice versa. That led to us using the wrapper classes
directly. <br>
<br>
Maybe its time to provide a PKCS11AttributeSpec of some sort for
key creation and for looking things up? The current model is
literally 12-15 years old AFAICT.<br>
<br>
<blockquote type="cite"
cite="mid:acc35122-91f4-6d2c-2101-70136316cf9c@oracle.com"> <br>
For the time being, maybe we can leave these method intact and add
a comment about calling the new methods which use P11Key argument
instead of the key handle argument.<br>
</blockquote>
<br>
That should work. You may want to think about deprecating those
methods and target removing them for a later release in a couple of
years. <br>
<br>
Thanks - Mike<br>
<br>
<br>
<blockquote type="cite"
cite="mid:acc35122-91f4-6d2c-2101-70136316cf9c@oracle.com"> <br>
Regards,<br>
Valerie<br>
<br>
<div class="moz-cite-prefix">On 9/1/2018 11:18 AM, Michael StJohns
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:c4d19de4-e65e-fcac-e4c2-b82d489cfa8f@comcast.net">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<div class="moz-cite-prefix">Hi Valerie - <br>
<br>
I may be speaking only for myself, but there are probably a
number of folk using sun.security.pkcs11.wrapper.* in lieu of
the PKCS11 provider for a number of reasons including an
ability to manage the key storage and wrapping efficiently.
Looking at this I'm thinking there will be a large number of
things breaking because of the way you refactored things.<br>
<br>
While I understand that none of the sun.security.* classes are
"public" - these were mostly taken directly from the IAIK
contribution way back when and the folks I worked with used
them in lieu of external libraries to have a consistent PKCS11
interface java-to-java.<br>
<br>
Perhaps instead you'd consider doing something like leaving
the Java to Native public methods intact?<br>
<br>
Mike<br>
<br>
<br>
<br>
<br>
On 8/31/2018 7:53 PM, Valerie Peng wrote:<br>
</div>
<blockquote type="cite"
cite="mid:44a3e1c4-64ca-47bf-2061-4fe55fa7b827@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<p>Hi Martin,</p>
<p>With the new model of "creating the key handle as needed",
I think we should not allow the direct access of keyID field
outside of P11Key class. This field should be made private
and accessed through methods. In addition, existing
PKCS11.C_XXX() methods with P11 keyID arguments can be made
private and we can add wrapper methods with P11Key object
argument to ensure proper usage of the P11Key object and its
keyID field. <br>
</p>
<p>I have also refactored the keyID management code into a
separate holder class. The prototype code can be found at: <a
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Evaleriep/6913047Exp/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~valeriep/6913047Exp/webrev.00/</a></p>
<p>The main changes that I made are in P11Key.java and
PKCS11.java. The other java classes are updated to call the
wrapper methods in PKCS11.java.</p>
<p>Thanks & have a great Labor day weekend,</p>
<p>Valerie<br>
</p>
<br>
<div class="moz-cite-prefix">On 8/16/2018 5:28 PM, Valerie
Peng wrote:<br>
</div>
<blockquote type="cite"
cite="mid:2a8e2986-631a-02f5-f419-e28c18232b33@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<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>
</blockquote>
<br>
</blockquote>
<p><br>
</p>
</blockquote>
<br>
</blockquote>
<p><br>
</p>
</body>
</html>