<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 8/18/2016 8:48 PM, Valerie Peng
wrote:<br>
</div>
<blockquote
cite="mid:e584e49a-6c50-37bc-4944-c59dd59e0f84@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<p><br>
</p>
I share your view on most things. It's just that the APIs are
there before the PKCS11 provider is added.<br>
So, there are some history reason as to why things are as they are
today.<br>
Re-structuring the public classes are almost impossible
considering the compatibility impact.<br>
However, we can explore other functional fixes if necessary.<br>
</blockquote>
<br>
I went and did some document archeology and based on various
versions of RSA and PKCS11 and other APIs available around 1995-96,
I would say that the Java API for RSA objects was originally based
on the PKCS11 API rather than the original RSA documentation. The
original RSA docs all included the public exponent as part of the
Private Key. For some reason, PKCS11 did not. <br>
<br>
For RSAMultiPrimePrivateCrtKey - which appears to have been added in
1.3 - I have no idea why this wasn't subclassed from
RSAPrivateCrtKey instead of RSAPrivateKey.<br>
<br>
I think the compatibility issues can be managed. Mostly, the fields
and methods won't change except for RSAPrivateKey and
RSAPrivateKeySpec. And for those you can use:<br>
<br>
default public BigInteger getPublicExponent() {<br>
return null;<br>
}<br>
<br>
to keep from blowing up existing implementations.<br>
<br>
In any event, that's a different problem than the current one.<br>
<br>
<blockquote
cite="mid:e584e49a-6c50-37bc-4944-c59dd59e0f84@oracle.com"
type="cite"> <br>
Thanks for the review and feedback, it's very helpful,<br>
</blockquote>
<br>
You're welcome - Mike<br>
<br>
<blockquote
cite="mid:e584e49a-6c50-37bc-4944-c59dd59e0f84@oracle.com"
type="cite"> Valerie<br>
<br>
<div class="moz-cite-prefix">On 8/18/2016 2:25 PM, Michael StJohns
wrote:<br>
</div>
<blockquote
cite="mid:3419f99e-d4d4-0802-8baa-b1fe89ddf545@comcast.net"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 8/18/2016 4:49 PM, Valerie Peng
wrote:<br>
</div>
<blockquote
cite="mid:4a33b5ec-816e-5ee5-ae77-f865c6b42fb3@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi Mike,<br>
<br>
Thanks for the feedback and the detailed write up.<br>
<br>
The scenario here is complicated by the
sensitive/non-extractable keys of PKCS#11 and the fact that
java key and key specification classes assume all relevant
values being available.</blockquote>
Um... I think that's true for any PublicKey, but not for Secret
or Private keys. At worst, the Key object is a handle for the
real key that contains all those items, but they might not be
available. At best, most of those components will be
available. I say at best, because of the language in
RSAMultiPrimePrivateCRTKey for getOtherPrimeInfo() which says it
can return null.<br>
<br>
<blockquote
cite="mid:4a33b5ec-816e-5ee5-ae77-f865c6b42fb3@oracle.com"
type="cite"> Only when all relevant values are available, then
we will construct the corresponding key objects. This is
necessary as there are other providers which may receive such
keys and they can't handle keys like this.<br>
</blockquote>
<br>
Keys can't generally move across providers AIRC? You can try
and use a key factory to convert them, but that's not
guaranteed. A PKCS11 derived key isn't going to be portable to
another provider without extraction to a keyspec in any case. <br>
<br>
Hmm.. I went back and read the JDK8 p11 guide (<a
moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://docs.oracle.com/javase/8/docs/technotes/guides/security/p11guide.html">https://docs.oracle.com/javase/8/docs/technotes/guides/security/p11guide.html</a>)
and section 3.2 gives the guidance that you only use the generic
interfaces for unextractable keys. I actually think that's
wrong, given the general guidance in the JCA documentation with
respect to Opaque Keys vs transparent KeySpecs. (Hmm... I
wonder if this guidance was in the originally submitted code
package documentation).<br>
<br>
Then there's the point that even a generic Public or Private key
has a "getEncoded()" method. *bleah*<br>
<br>
<br>
<blockquote
cite="mid:4a33b5ec-816e-5ee5-ae77-f865c6b42fb3@oracle.com"
type="cite"> <br>
I am sure that the current PKCS11 provider code needs many
improvement/finer handlings. But I don't see a straightforward
way of "making CKA_PUBLIC_EXPONENT available" across various
RSA Key classes. This should be tracked in a different issue.<br>
</blockquote>
<br>
That's just one of the items. As I mentioned in another email,
I think the RSA key classes and interfaces need a bit more work
and tweaking. I wouldn't try and accomplish that quite yet.<br>
<br>
<blockquote
cite="mid:4a33b5ec-816e-5ee5-ae77-f865c6b42fb3@oracle.com"
type="cite"> <br>
Given the current release schedule, the deadline for this fix
(P4) is coming up in 10 days and I will be on vacation next
week.<br>
<br>
If you agree with the value of addressing this with the
proposed changes for JDK 9, then we can proceed.<br>
Otherwise, I will defer this bug to the update release and we
can spend more time to polish this.<br>
</blockquote>
<br>
I think you might as well go ahead with this change. The fix
you've got should work as long as someone who generates a RSA
Key pair on a PKCS11 which is both sensitive and unextractable
doesn't try to cast the keys to RSAPublic or RSAPrivate.<br>
<br>
Mike<br>
<br>
<br>
<blockquote
cite="mid:4a33b5ec-816e-5ee5-ae77-f865c6b42fb3@oracle.com"
type="cite"> Valerie<br>
<br>
<div class="moz-cite-prefix">On 8/18/2016 8:40 AM, Michael
StJohns wrote:<br>
</div>
<blockquote
cite="mid:851c0000-8341-294c-e62b-f15334d417a1@comcast.net"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 8/17/2016 11:36 PM, Valerie
Peng wrote:<br>
</div>
<blockquote
cite="mid:89d1e859-a988-627d-6b7c-c5ad6f062f18@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Regression tests are still running, but thought that I
will send the updated webrev out and see if there are more
comments.<br>
<br>
Webrev is updated at: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Evaleriep/8078661/webrev.01/">http://cr.openjdk.java.net/~valeriep/8078661/webrev.01/</a><br>
<br>
Thanks,<br>
Valerie<br>
</blockquote>
Hi Valerie - <br>
<br>
You know - re-reading this code I'm reminding of why PKCS11
annoys me so much.<br>
<br>
At line 333 (of the "new" P11Key) you grab the Token,
Sensitive and Extractable values and if the private data is
sensitive or not extractable you create a generic
P11PrivateKey and return that. However the contract for
RSAKey requires that the public modulus be returned if
available, and, since its not a sensitive attribute it
probably should be available. Also, even if the key is
sensitive - if its a sensitive CRT key, then
CKA_PUBLIC_EXPONENT should be available.<br>
<br>
That's going to be a surprise if someone tries to cast this
return to an (RSAKey) or (RSAPrivateKey). <u>This should
be changed so a key of the appropriate type is always
created.</u><br>
<br>
Also, checking for CKA_EXTRACTABLE being true, doesn't
actually get you access to the clear text information. If a
key is extractable, then it can be wrapped out under another
key. The components themselves aren't available. It's
possible to have a non-sensitive, non-extractable key where
the components are retrievable, but the key can't be wrapped
out.<br>
<br>
<br>
(Hmm... the public exponent is in RSAPublicKey and
RSAPrivateCRTKey, but should probably be in RSAKey instead).<br>
<br>
So:<br>
<br>
All RSA keys - even the sensitive private ones - should
return CKA_MODULUS.<br>
All RSA Private CRT Keys - even the sensitive ones - should
also return CKA_PUBLIC_EXPONENT.<br>
All non-sensitive RSA Private keys - should also return
CKA_PRIVATE_EXPONENT<br>
All non-sensitive RSA Private CRT Keys - should also return
CKA_PRIME_1, CKA_PRIME_2, CKA_EXPONENT_1, CKA_EXPONENT_2 and
CKA_COEFFICIENT.<br>
<br>
This is harder to do than it needs to be due to how
p11_objmgt.c::Java_sun_security_pkcs11_wrapper_PKCS11_C_1GetAttributeValue
is built. At lines 248 and 270, it does a check for an
error return and throws an exception if any error occurs.
However, for C_GetAttributeValue, there are a number of
"non-fatal" errors that indicate either buffer size errors
or sensitivity of one or more components or unavailability
of one or more components.<br>
<blockquote type="cite">Note that the error codes
CKR_ATTRIBUTE_SENSITIVE, CKR_ATTRIBUTE_TYPE_INVALID, and
CKR_BUFFER_TOO_SMALL do not denote true errors for <b>C_GetAttributeValue</b>.
If a call to <b>C_GetAttributeValue</b> returns any of
these three values, then the call must nonetheless have
processed <i>every</i> attribute in the template supplied
to <b>C_GetAttributeValue</b>. Each attribute in the
template whose value <i>can be</i> returned by the call
to <b>C_GetAttributeValue</b> <i>will be</i> returned by
the call to <b>C_GetAttributeValue</b>.</blockquote>
<br>
<br>
<br>
If you updated this slightly - maybe by adding a new method
to wrapper.PKCS11 (say GetAttributeValuesNoError) - to
return the attributes it was able to get in the call with
nulls elsewhere, then you could do all of the above in one
pass. <br>
<br>
Sorry to complicate this. Mike<br>
<br>
ps - I don't have a current build environment set up for the
JDK, otherwise I'd code it and test it myself. I'm happy to
take a swing at it and provide you unverified code you can
integrate.<br>
<br>
<blockquote
cite="mid:89d1e859-a988-627d-6b7c-c5ad6f062f18@oracle.com"
type="cite"> <br>
<div class="moz-cite-prefix">On 8/17/2016 9:55 AM, Michael
StJohns wrote:<br>
</div>
<blockquote
cite="mid:d605af02-3f3b-8fb5-4bbd-ebbd836457cc@comcast.net"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 8/16/2016 9:24 PM,
Valerie Peng wrote:<br>
</div>
<blockquote
cite="mid:f2065a91-b1bd-f55b-7e1f-055752fcdfe9@oracle.com"
type="cite"> <br>
Anyone has time to review a straightforward fix? The
current PKCS11 code assume that if public exponent is
available for RSA Private Key, then it's a RSA CRT
key. However, not all vendor implementation works this
way. Changing to a tighter check and did minor
code-refactoring to avoid re-retrieving the attribute
values. <br>
<br>
Bug: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8078661">https://bugs.openjdk.java.net/browse/JDK-8078661</a>
<br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Evaleriep/8078661/webrev.00/">http://cr.openjdk.java.net/~valeriep/8078661/webrev.00/</a>
<br>
<br>
Thanks, <br>
Valerie <br>
</blockquote>
<p>Given that there's a change to PKCS11 for 2.40 that
says that all RSA private key objects MUST also store
CKA_PUBLIC_EXPONENT, some change needed to be made.</p>
<p>Sorry - I don't think this fix will work. Or if its
working on your version of PKCS11, your version of
PKCS11 is doing it wrong. The problem is that if you
specify attributes that don't exist on the object, the
underlying PKCS11 library is supposed to return
CKR_ATTRIBUTE_TYPE_INVALID. And that should trigger a
thrown exception before you ever get anything copied
to your attributes.</p>
<p>1) Get modulus and private exponent first. That
gives you the stuff for a generic RSA private key -
and if it fails, there's no reason to continue.</p>
<p>2) Then get the rest of the stuff. If that fails,
then you already have the stuff you need for a normal
private key.<br>
</p>
<p> <br>
</p>
<p> </p>
<blockquote type="cite">
<pre> boolean crtKey;
try {
session.token.p11.C_GetAttributeValue
(session.id(), keyID, attrs2);
<span class="removed">- crtKey = (attrs2[0].pValue instanceof byte[]);</span>
<span class="new">+ crtKey = ((attrs2[1].pValue instanceof byte[]) &&</span>
<span class="new">+ (attrs2[3].pValue instanceof byte[]) &&</span>
<span class="new">+ (attrs2[4].pValue instanceof byte[]) &&</span>
<span class="new">+ (attrs2[5].pValue instanceof byte[]) &&</span>
<span class="new">+ (attrs2[6].pValue instanceof byte[]) &&</span>
<span class="new">+ (attrs2[7].pValue instanceof byte[])) ;</span>
} catch (PKCS11Exception e) {
// ignore, assume not available
crtKey = false;
}</pre>
</blockquote>
<p>// Change attrs2 so it only has the additional CRT
attributes (e.g. delete CKA_MODULUS,
CKA_PRIVATE_EXPONENT from the list<br>
</p>
<p>Replace the above with <br>
</p>
<p>CK_ATTRIBUTE[] attrs3 = new CK_ATTRIBUTE[] {<br>
new CK_ATTRIBUTE(CKA_MODULUS),<br>
new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT)<br>
};<br>
// no try block needed here - we want to throw the
error if it occurs<br>
session.token.p11.C_GetAttributeValue (session.id(),
keyID, attrs3);</p>
<p>// So far so good - we have the base attributes,
let's see if we can get the additional attributes;</p>
<p>try { <br>
session.token.p11.C_GetAttributeValue(session.id(),keyID,
attrs2);<br>
} catch (PKCS11Exception e) {<br>
// we really should check the return value for one
of the non-fatal values, but let's just assume its not
a CRT key<br>
return new P11RSAPrivateNonCRTKey (session, keyID,
algorithm, keyLength, attrs2, attrs3);<br>
}</p>
<p>// if we fall through then its a CRT key<br>
// -- we should check for byte[] ness of each of the
components, and throw an error if they arent - but
which error?</p>
<p>return new P11RSAPrivateKey (session, keyID,
algorithm, keyLength, attrs2, attrs3);<br>
</p>
<p>// there are cleanups necessary in other places. I'd
suggest rather than depending on the ordering of
attributes, you do assignment by CKA_ values just so
someone coming later doesn't mess things up by
mistake. Also, a hell of a lot more readable.<br>
</p>
<p>static CK_ATTRIBUTE getAttribute (CK_ATTRIBUTE[]
attrs, long attrType) {<br>
for (CK_ATTRIBUTE a : attrs) {<br>
if (a.type == attrType)<br>
return a;<br>
}<br>
return null; // or throw something?<br>
}<br>
</p>
<p><br>
coeff =
getAtttribute(attrs,CKA_COEFFICIENT).getBigInteger();<br>
</p>
<br>
The other possibility is to change the C code for
C_GetAttributeValues so it doesn't error out for the
non-fatal error codes and instead returns a null value
attribute when the attribute is missing.<br>
<br>
Mike<br>
<br>
</blockquote>
<br>
</blockquote>
<p><br>
</p>
</blockquote>
<br>
</blockquote>
<p><br>
</p>
</blockquote>
<br>
</blockquote>
<p><br>
</p>
</body>
</html>