Hi,
> 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. I just though I'd second this, albeit late. We're seing the current PKCS#11 Provider model break down with some new HSMs out there. I.e. on AWS CloudHSM/Cavium, it is only possible to set CKA_ID at time of key creation, whereas the P11 Provider sets it afterwards as a setAttribute call. This effectively makes the old Sun P11 Provider not usable at all wiht CloudHSM/Cavium. With todays PKCS#11 landscape I think we need more control, or the provider will essentially be defunct. Regards, Tomas On 2018-09-05 22:49, Michael StJohns wrote: > 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> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
