Hi Martin,
Please find my reply below:
On 10/18/2018 9:26 AM, Martin Balao wrote:
Hi Valerie,
Thanks for your feedback.
I suggest to keep only one hierarchy of webrevs, so I'll continue in
Webrev.12 using your Webrev.02 as a base.
Sure, I agree and prefer to use your hierarchy for the sake of complete
history.
In general, I'm happy with your P11Key refactorings and with keeping
the previous reference inc/dec scheme on P11key clients side.
I'll go file-by-file reviewing changes between your Webrev.02 and my
Webrev.11:
* P11Key.java
* Why is P11Key now public?
Hmm, this change may not be intentional. Please discard it.
* I have some concerns regarding how you calculate "extractKeyInfo"
value:
* Why is !sensitive a requirement?
* We can extract them wrapped, and the code already handles that.
* !tokenObject should be a requirement
* I know it's checked in NativeKeyHolder constructor but it could
be part of extractKeyInfo too for clarity
* Why is !type.equals(SECRET) a requirement?
We can change the setting of extractKeyInfo to include !tokenObject and
remove the !senstive and !type.equals(SECRET) I suppose. I used a
stricter setting in my webrev.02 to avoid triggering the key wrapping
functionality for ease of observation and verification. I didn't get to
testing the prototype with key wrapping functionality before leaving for
vacation, so I leave the stricter setting of extractKeyInfo in webrev.02
in place.
* In case of createNativeKey failure, I think we should decrement
the previously-incremented reference counter
True, but since an exception is thrown, it may not matter much.
* Why do we need refCount to be of AtomicInteger type? All the
modifications to this instance variable are done inside instance
synchronized methods (getKeyID and releaseKeyID)
This is done before I changed those methods to be synchronized. We don't
need to use AtomicInteger now.
* The benefit of the previous incNativeKeyRef/decNativeKeyRef
methods is that they don't pay a sync cost for all cases in which the
feature is disabled. I believe we can keep that performance
improvement here too.
Sure, it's good to not pay sync cost when feature is disabled. I didn't
get to polish my webrev.02 due to the time crunch. Just want to show you
the model that I have in mind.
* nativeKeyInfo variable is written at creation time and all other
usages are read-only, so we can have multiple readers at the same time
and synchronize on it if not null (instead of synchronizing the whole
method). What do you think?
Sure, sounds good.
* P11KeyStore.java
* Copyright updated
* The reason why a call to "makeNativeKeyPersistent" was made in
P11KeyStore.updateP11Pkey method was because "P11Key key" could have
been extracted and the change not persisted in the keystore. This call
was disabling the feature for such keys. However, we now decided -for
a good reason- that keys from keystores (token objects) will not be
extracted. Thus, this call is no longer needed. I'm okay with this change.
* "long newKeyID" variable not used. Fixed on some other files too.
Ok.
* P11Mac.java
* Copyright updated
* Split ensureInitialized and initialize functions again for the
reasons we've previously discussed.
Line 195: For engineInit(...) calls, we should always call initialize()
instead of ensureInitialized(). Although ensureInitialized() here will
always call initialize() due to the reset(true) call on line 192, but
conceptually, it should be initialize().
* P11SecretKeyFactory.java
* Copyright updated
* The inc/dec pattern was broken in case of "token.p11.C_CopyObject"
failure. Fixed.
Good.
* P11TlsKeyMaterialGenerator.java
* Removed dead & debug code
Sure.
* P11TlsPrfGenerator.java
* Removed dead & debug code
Sure.
* p11_keymgmt.c
* Removed dead code
* I've seen that you replaced some variable assignments with memcpy
and memset calls because of compiler errors. I wonder what these
errors are, as we should be able to cast and do assignments.
Some are due to compiler errors, some are due to SIGBUS errors when
testing with your changes. Casting also makes long lines which may
hinder readability. This may due to my machine being solaris sparc.
I agree with having a property to completely disable this feature.
I've not seen, so far, unfixed cases in which extraction succeeds but
re-creation fails.
Right, I think the chance of extraction succeeds but re-creation fails
may be slim. The worse problem is that the re-creation succeeds but the
re-created key is different from the original one. As this change does
depend on the list of attributes, I wonder if the current list of
attributes defined in the hardcoded template may become incomplete in
the future (as newer algorithms are added or due to some other
enhancements) and whether this may lead to the re-created keys being
different. There could be other possible problems which we have not
thought of now. Having a property to disable this may come into handy if
these were to happen and helps troubleshoot if a problem is related to
this or not.
Thanks,
Valerie
Webrev.12 is based on your Webrev.02, with modifications previously
described:
*
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.12/
<http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.12/>
*
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.12.zip
<http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.12.zip>
NOTE: Webrev.12 does not address neither the P11Key.java issues above
nor the property to completely disable this feature. Those 2 things
are the only pending on my side for now.
Kind regards,
Martin.-
