On 9/4/2018 9:59 PM, Valerie Peng wrote:
These sun.security.pkcs11.wrapper classes are internal and subject to
changes without notice.
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.
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.
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.
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.
Thanks - Mike
Regards,
Valerie
On 9/1/2018 11:18 AM, Michael StJohns wrote:
Hi Valerie -
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.
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.
Perhaps instead you'd consider doing something like leaving the Java
to Native public methods intact?
Mike
On 8/31/2018 7:53 PM, Valerie Peng wrote:
Hi Martin,
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.
I have also refactored the keyID management code into a separate
holder class. The prototype code can be found at:
http://cr.openjdk.java.net/~valeriep/6913047Exp/webrev.00/
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.
Thanks & have a great Labor day weekend,
Valerie
On 8/16/2018 5:28 PM, Valerie Peng wrote:
Hi Martin,
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".
To understand the changes better, please find my questions and some
review comments below:
<src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java>
- 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.
- line 471: Why do you change it to Throwable from PKCS11Exception?
<src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java>
- line 99: comment is somewhat unclear. typo: "temporal" should be
"temporary".
- line 148-149: JDK-8165996 has been fixed. This does not apply
now, does it?
- You changed the constructors of 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 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.
- line 157-175: nativeKeyWrapperKeyID is a static variable,
shouldn't it be synchronized on a class level object?
- line 1343: the impl doesn't look right. Why do you call both
removeObject() and addObject() here with the same check?
- line 1363: the change seems incorrect, you should still call
session.removeObject(). the call setKeyIDAndSession(0, null) does
not lower the session object count.
Thanks,
Valerie
On 8/7/2018 5:41 PM, Valerie Peng wrote:
Hi Martin,
Thanks for the update, I will resume the review on this one with
your latest webrev.
BTW, there is no webrev.07 for your other fix, i.e. JDK-8029661,
right? Just checking.
Regards,
Valerie
On 8/3/2018 2:13 PM, Martin Balao wrote:
Hi Valerie,
*
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.10/
<http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.10/>
*
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.10.zip
<http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.10.zip>
New in webrev 10:
* Bug fix when private DSA/EC keys are sensitive
* I found this bug removing "attributes = compatibility" entry
in NSS configuration file so keys were CKA_SENSITIVE.
* 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].
* 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.
* Changes in:
* p11_keymgmt.c
* L175-187, L212-214 and L275-278
* Bug fix when storing sensitive RSA keys in a keystore
* CKA_NETSCAPE_DB attribute is not needed so we return it as
null (instead of failing with an "Invalid algorithm" message)
* Changes in:
* P11KeyStore.java
* L1909-1914
* Lines length was cut to improve code readability
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.
Kind regards,
Martin.-
--
[1] - https://bugzilla.mozilla.org/show_bug.cgi?id=309701#c6
<https://bugzilla.mozilla.org/show_bug.cgi?id=309701#c6>
[2] -
https://alvinalexander.com/java/jwarehouse/openjdk-8/jdk/test/sun/security/pkcs11/fips/fips.cfg.shtml
<https://alvinalexander.com/java/jwarehouse/openjdk-8/jdk/test/sun/security/pkcs11/fips/fips.cfg.shtml>
