1355358 - CryptoStore: add methods for importing and exporting EncryptedPrivateKeyInfo

Status: RESOLVED FIXED

(JSS Graveyard :: Library, enhancement)

Description

[Fraser Tweedale]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170310133403

Steps to reproduce:

JSS needs more versatile methods for importing/exporting EncryptedPrivateKeyInfo.
(PBEAlgorithm is too limiting).

Comment 1

[Fraser Tweedale]

Attachment: jss-ftweedal-0000-Add-SEC_OID-mappings-for-AES-ECB-CBC-algorithms.patch

Comment 2

[Fraser Tweedale]

Attachment: jss-ftweedal-0001-Add-InvalidDERException-to-jss_exceptions.h.patch

Comment 3

[Fraser Tweedale]

Attachment: jss-ftweedal-0002-Add-methods-for-importing-and-exporting-EncryptedPri.patch

Comment 4

[Christina Fu]

Comment on attachment 8856833 [details] [diff] [review]
jss-ftweedal-0000-Add-SEC_OID-mappings-for-AES-ECB-CBC-algorithms.patch

Review of attachment 8856833 [details] [diff] [review]:
-----------------------------------------------------------------

looks good.

Comment 5

[Christina Fu]

Comment on attachment 8856834 [details] [diff] [review]
jss-ftweedal-0001-Add-InvalidDERException-to-jss_exceptions.h.patch

Review of attachment 8856834 [details] [diff] [review]:
-----------------------------------------------------------------

looks good.

Comment 6

[Christina Fu]

Comment on attachment 8856835 [details] [diff] [review]
jss-ftweedal-0002-Add-methods-for-importing-and-exporting-EncryptedPri.patch

Review of attachment 8856835 [details] [diff] [review]:
-----------------------------------------------------------------

A few comments:
* The general practice in JSS c code is to always check the values when they come in before they are referenced.  e.g.
   you want to say something like

   const char *nicknameChars = NULL;
   if (nickname) {
       nicknameChars = (*env)->GetStringUTFChars(env, nickname, NULL);
   } else {
        JSS_throw(env, NULL_POINTER_EXCEPTION);
        goto finish;
    }
Or, you could check all of them together at very beginning of the method.
I found some places where no checks are made.  Instead of naming them all, I'll let you do the check on all patchs and fix on your own.

* in Java_org_mozilla_jss_pkcs11_PK11Store_importEncryptedPrivateKeyInfo(), call to PK11_ImportEncryptedPrivateKeyInfo passes
   0 /* keyUsage */
  Does "0" represent all key usages?  Did you test to see if the PKCS12Util you modified, after import, were
  you able to use the key to sign or encrypt?  Please put a comment there so people know what "0" or any number means.. maybe refer to where they are
  defined in nss.
  It might be more flexible to allow the keyUsage can be passed in from the calling application, but default to "all usages".
* in CryptoStore.java: You changed the signature of the method getEncryptedPrivateKeyInfo() by adding throws ...
  This might cause backward compatibility issues as existing apps would likely not to handle these new exceptions introduced into the method signature.
  Although I don't find any existing calling places in Dogtag, so maybe the impact is minimal.

The rest of the patch seems good.

Comment 7

[Fraser Tweedale]

Attachment: jss-ftweedal-0002-2-Add-methods-for-importing-and-exporting-EncryptedPri.patch

Updated patch 0002.  Variable validation / initialisation fixes and
add commentary on 'keyUsage = 0'.

I have not changed the addition of exceptions to
getEncryptedPrivateKeyInfo() because I agree that the impact is
low, and it gives better insight into the kinds of errors that
can occur.

Comment 8

[Fraser Tweedale]

Attachment: jss-ftweedal-0002-3-Add-methods-for-importing-and-exporting-EncryptedPri.patch

Comment 9

[Elio Maldonado]

Comment on attachment 8862263 [details] [diff] [review]
jss-ftweedal-0002-3-Add-methods-for-importing-and-exporting-EncryptedPri.patch

Review of attachment 8862263 [details] [diff] [review]:
-----------------------------------------------------------------

r+, Addresses the concerns I had expressed regarding the location of the new function in jss.def.

::: org/mozilla/jss/pkcs11/PK11Store.java
@@ +60,5 @@
> +            PBEAlgorithm pbeAlg,
> +            Password pw,
> +            int iteration)
> +            throws CryptoManager.NotInitializedException,
> +                ObjectNotFoundException, TokenException {

This is minor and on the other of a nitpick. The function throws 3 exceptions and returns a the encrypted key which it would be nice it that were documented with @throws and a @return but this is entirely option and best deferred. If we go that way we find more and would never end. We do have tools to check for javadoc so let's wait until we run them again.

Comment 10

[Christina Fu]

Comment on attachment 8862263 [details] [diff] [review]
jss-ftweedal-0002-3-Add-methods-for-importing-and-exporting-EncryptedPri.patch

Review of attachment 8862263 [details] [diff] [review]:
-----------------------------------------------------------------

looks like my comments were addressed. ack.

Comment 11

[Elio Maldonado]

Pushed: https://hg.mozilla.org/projects/jss/rev/c8885dd6787639d74a1c9d634fd289ff17fa6f02