<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    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. 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>
    <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>
    <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>
    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>
  </body>
</html>