<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Martin,</p>
<p>Please find my reply below:<br>
</p>
<div class="moz-cite-prefix">On 10/18/2018 9:26 AM, Martin Balao
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra">Hi Valerie,</div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">Thanks for your feedback.</div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">I suggest to keep only one
hierarchy of webrevs, so I'll continue in Webrev.12 using
your Webrev.02 as a base.</div>
</div>
</div>
</div>
</blockquote>
Sure, I agree and prefer to use your hierarchy for the sake of
complete history.<br>
<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra">In general, I'm happy with your
P11Key refactorings and with keeping the previous
reference inc/dec scheme on P11key clients side.</div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">I'll go file-by-file reviewing
changes between your Webrev.02 and my Webrev.11:</div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra"> * P11Key.java</div>
<div class="gmail_extra"> * Why is P11Key now public?</div>
</div>
</div>
</div>
</blockquote>
Hmm, this change may not be intentional. Please discard it.<br>
<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra"> * I have some concerns regarding
how you calculate "extractKeyInfo" value:</div>
<div class="gmail_extra"> * Why is !sensitive a
requirement?</div>
<div class="gmail_extra"> * We can extract them wrapped,
and the code already handles that.</div>
<div class="gmail_extra"> * !tokenObject should be a
requirement</div>
<div class="gmail_extra"> * I know it's checked in
NativeKeyHolder constructor but it could be part of
extractKeyInfo too for clarity</div>
<div class="gmail_extra"> * Why is !type.equals(SECRET) a
requirement?</div>
</div>
</div>
</div>
</blockquote>
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.<br>
<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra"> * In case of createNativeKey
failure, I think we should decrement the
previously-incremented reference counter</div>
</div>
</div>
</div>
</blockquote>
True, but since an exception is thrown, it may not matter much.<br>
<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra"> * 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)</div>
</div>
</div>
</div>
</blockquote>
This is done before I changed those methods to be synchronized. We
don't need to use AtomicInteger now.<br>
<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra"> * 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.</div>
</div>
</div>
</div>
</blockquote>
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.<br>
<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra"> * 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?</div>
</div>
</div>
</div>
</blockquote>
Sure, sounds good.<br>
<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra"> * P11KeyStore.java</div>
<div class="gmail_extra"> * Copyright updated</div>
<div class="gmail_extra"> * 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.</div>
<div class="gmail_extra"> * "long newKeyID" variable not
used. Fixed on some other files too.</div>
</div>
</div>
</div>
</blockquote>
Ok.<br>
<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra"> * P11Mac.java</div>
<div class="gmail_extra"> * Copyright updated</div>
<div class="gmail_extra"> * Split ensureInitialized and
initialize functions again for the reasons we've
previously discussed.</div>
</div>
</div>
</div>
</blockquote>
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().<br>
<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra"> * P11SecretKeyFactory.java</div>
<div class="gmail_extra"> * Copyright updated</div>
<div class="gmail_extra"> * The inc/dec pattern was broken
in case of "token.p11.C_CopyObject" failure. Fixed.</div>
</div>
</div>
</div>
</blockquote>
Good.<br>
<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra"> * P11TlsKeyMaterialGenerator.java</div>
<div class="gmail_extra"> * Removed dead & debug code</div>
</div>
</div>
</div>
</blockquote>
Sure.<br>
<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra"> * P11TlsPrfGenerator.java</div>
<div class="gmail_extra"> * Removed dead & debug code</div>
</div>
</div>
</div>
</blockquote>
Sure.<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra"> * p11_keymgmt.c</div>
<div class="gmail_extra"> * Removed dead code</div>
<div class="gmail_extra"> * 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.</div>
</div>
</div>
</div>
</blockquote>
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.
<br>
<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra">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.</div>
</div>
</div>
</div>
</blockquote>
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.<br>
<br>
Thanks,<br>
Valerie<br>
<blockquote type="cite"
cite="mid:CAKZz+gc0=neivy=kbmaJrJAfHHDFM5Qr8VrgqTmq__vB+-A--A@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_extra">Webrev.12 is based on your
Webrev.02, with modifications previously described:</div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra"> * <a
href="http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.12/"
moz-do-not-send="true">http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.12/</a></div>
<div class="gmail_extra"> * <a
href="http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.12.zip"
moz-do-not-send="true">http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.12.zip</a></div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">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.</div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">Kind regards,</div>
<div class="gmail_extra">Martin.-</div>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>