<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>