<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p><br>
</p>
Thanks for the input.<br>
SunPKCS11 provider always generate RSA CRT keys if I recall
correctly, so it's hard to test the non-CRT scenarios. Thus, I made
the changes basing on the suggestions in the bug report.<br>
<br>
I will update the webrev accordingly.<br>
Valerie<br>
<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>
</body>
</html>