CryptoStore: add methods for importing and exporting EncryptedPrivateKeyInfo

RESOLVED FIXED in 4.4.1

Status

JSS
Library
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: Fraser Tweedale, Assigned: Fraser Tweedale)

Tracking

4.4.1
4.4.1

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

9 months ago
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).
(Assignee)

Comment 1

9 months ago
Created attachment 8856833 [details] [diff] [review]
jss-ftweedal-0000-Add-SEC_OID-mappings-for-AES-ECB-CBC-algorithms.patch
(Assignee)

Comment 2

9 months ago
Created attachment 8856834 [details] [diff] [review]
jss-ftweedal-0001-Add-InvalidDERException-to-jss_exceptions.h.patch
(Assignee)

Comment 3

9 months ago
Created attachment 8856835 [details] [diff] [review]
jss-ftweedal-0002-Add-methods-for-importing-and-exporting-EncryptedPri.patch

Updated

9 months ago
Assignee: glenbeasley → ftweedal

Updated

9 months ago
Attachment #8856835 - Flags: review?(cfu)

Updated

9 months ago
Attachment #8856834 - Flags: review?(cfu)

Updated

9 months ago
Attachment #8856833 - Flags: review?(cfu)

Comment 4

9 months ago
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.
Attachment #8856833 - Flags: review?(cfu) → review+

Comment 5

9 months ago
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.
Attachment #8856834 - Flags: review?(cfu) → review+

Comment 6

9 months ago
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.
(Assignee)

Comment 7

9 months ago
Created attachment 8861811 [details] [diff] [review]
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.
Attachment #8856835 - Attachment is obsolete: true
Attachment #8856835 - Flags: review?(cfu)
Attachment #8861811 - Flags: review?(cfu)
(Assignee)

Comment 8

9 months ago
Created attachment 8862263 [details] [diff] [review]
jss-ftweedal-0002-3-Add-methods-for-importing-and-exporting-EncryptedPri.patch
Attachment #8861811 - Attachment is obsolete: true
Attachment #8861811 - Flags: review?(cfu)
Attachment #8862263 - Flags: review?(emaldona)
Attachment #8862263 - Flags: review?(cfu)

Comment 9

9 months ago
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.
Attachment #8862263 - Flags: review?(emaldona) → review+

Comment 10

9 months ago
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.
Attachment #8862263 - Flags: review?(cfu) → review+

Comment 11

9 months ago
Pushed: https://hg.mozilla.org/projects/jss/rev/c8885dd6787639d74a1c9d634fd289ff17fa6f02
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED

Updated

9 months ago
Target Milestone: --- → 4.4.1
You need to log in before you can comment on or make changes to this bug.