940994 - certificate import - Files of type "Certificate Files" doesn't show .p7b files

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Bruce Robson]

I'm running Thunderbird ESR version 17.0.10 on Windows XP.

I was attempting to import a certificate for a person I wished to email
from file a called yahoo2013.p7b. I started the "Certificate Manager",
switched to the "People" tab and pressed "Import". This popped up a
dialog for selecting the file to import. I navigated to the directory
containing the file to import and the file was not shown.

The dialog was filtering on files of type "Certificate Files", when I
changed this to "All Files" the certificate file yahoo2013.p7b was
shown and could be imported.

Comment 2

[Shashank VRSN Sabniveesu]

Hi! I am ready o take up this bug. What needs to be done for this?

Comment 3

[Dana Keeler (she/her) [:keeler]]

Hi Shashank! Thank you for your interest in this. Unfortunately, I have some bad news. I initially thought this bug would be a simple change involving making the UI to show more files that are PEM-encoded X509 certificates. However, after looking at this more closely, it appears that p7b files refer to PKCS7 files, which are a different format. At the moment, we don't support importing that type of file, and it wouldn't be a good first bug to add that support.
Again, thank you for your interest, and I apologize for unintentionally being misleading about this.

Comment 4

[Dana Keeler (she/her) [:keeler]]

Whoops - spoke too soon. It looks like we can import pkcs7 files when they're DER-encoded, but not when they're PEM-encoded.

So, given that, here's what I would do:

In https://mxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/content/certManager.js, you'll see a number of lines like this:

385   fp.appendFilter(bundle.getString("file_browse_Certificate_spec"),
386                   "*.crt; *.cert; *.cer; *.pem; *.der");

Two things need to happen:
1. That string ("*.crt; *.cert; *.cer; *.pem; *.der") should be factored out into a common location, since it's always the same.
2. "*.p7b" needs to be added to it.
3. The multi-line test that starts on line 390 needs to try each one of the filetype suffixes - it would be nice to have a less cumbersome way to do that.

Let me know if you need any more information.

Comment 5

[Dana Keeler (she/her) [:keeler]]

Additional information: When you have a patch ready, please export it as a unified patch with 8 lines of context, attach it to this bug ("Add an attachment"), and flag me for review.

Comment 6

[Shashank VRSN Sabniveesu]

Attachment: BUG 940994 - Adding '.p7b' to 'known file types' list of 'Certificate Manager'

Comment 7

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8483185 [details] [diff] [review]
BUG 940994 - Adding '.p7b' to 'known file types' list of 'Certificate Manager'

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

Great! r=me with the nit addressed. If you want to address the second comment, please ask for review again.
At this point, since we don't actually have tests for this code, I think it would be best to post a before/after screenshot to illustrate that the change has been made successfully.
(Also, for future reference, requesting review is as effective as using needinfo, so you don't need to do both.)

::: security/manager/pki/resources/content/certManager.js
@@ +14,5 @@
>  const nsIPKIParamBlock    = Components.interfaces.nsIPKIParamBlock;
>  const nsPKIParamBlock    = "@mozilla.org/security/pkiparamblock;1";
>  const nsINSSCertCache = Components.interfaces.nsINSSCertCache;
>  const nsNSSCertCache = "@mozilla.org/security/nsscertcache;1";
> +const nsCertFileTypes = "*.crt; *.cert; *.cer; *.pem; *.der; *.p7b";

nit: let's not prefix this with "ns" - how about just "gCertFileTypes"? ("g" is commonly used to indicate a global)

@@ +389,5 @@
>    if (fp.show() == nsIFilePicker.returnOK) {
>      // If this is an X509 user certificate, import it as one.
>      if (fp.file.path.endsWith(".crt") || fp.file.path.endsWith(".cert") ||
>          fp.file.path.endsWith(".cer") || fp.file.path.endsWith(".pem") ||
> +        fp.file.path.endsWith(".der") || fp.file.path.endsWith(".p7b")) {

It would be nice to have this use gCertFileTypes somehow instead of listing them again, but maybe that can be a good next bug.

Comment 8

[Shashank VRSN Sabniveesu]

Attachment: Adding '.p7b' to 'known file types' list of 'Certificate Manager' r=keeler

1) All nits addressed

2) Eliminated the redundant definition of file types

(Screenshots will be attached next)

Comment 9

[Shashank VRSN Sabniveesu]

Attachment: before-patch.png

Before applying patch

Comment 10

[Shashank VRSN Sabniveesu]

Attachment: after-patch.png

After applying patch

Comment 11

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8496359 [details] [diff] [review]
Adding '.p7b' to 'known file types' list of 'Certificate Manager' r=keeler

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

This looks good. However, it's best to use true/false for boolean types. Also, I would call the indicator variable something more like 'isX509FileType'. r=me with comments addressed.

::: security/manager/pki/resources/content/certManager.js
@@ +389,5 @@
>    fp.appendFilters(nsIFilePicker.filterAll);
>    if (fp.show() == nsIFilePicker.returnOK) {
>      // If this is an X509 user certificate, import it as one.
> +
> +    var knownFileType = 0;

nit: 'var isX509FileType = false;'

@@ +391,5 @@
>      // If this is an X509 user certificate, import it as one.
> +
> +    var knownFileType = 0;
> +    var fileTypesList = gFileTypesList.split('; ');
> +    for (type in fileTypesList) {

nit: 'for (var type in ...'

@@ +393,5 @@
> +    var knownFileType = 0;
> +    var fileTypesList = gFileTypesList.split('; ');
> +    for (type in fileTypesList) {
> +      if (fp.file.path.endsWith(type)) {
> +        knownFileType = 1;

isX509FileType = true;

@@ +398,5 @@
> +  	break;
> +      }
> +    }
> +
> +    if(knownFileType) {

nit: space after 'if'

Comment 12

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8496366 [details]
after-patch.png

Looks good.

Comment 13

[Shashank VRSN Sabniveesu]

Attachment: nits addressed - Adding '.p7b' to 'known file types' list of 'Certificate Manager' r=keeler

Made the flag boolean
Renamed the flag
Made the scope of looping variable local
Added a space after 'if'

Comment 14

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8501340 [details] [diff] [review]
nits addressed - Adding '.p7b' to 'known file types' list of 'Certificate Manager' r=keeler

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

Great!
We don't have any automated tests for this, so this is about ready to be checked in: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
We should verify this by hand when it lands, though.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6c19a7af9f

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d6c19a7af9f

Comment 17

[Dana Keeler (she/her) [:keeler]]

Attachment: follow-up

Turns out, I missed a couple of issues that break importing PKCS12 user certificates. This is just a quick follow-up to fix them.

Comment 18

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8520765 [details] [diff] [review]
follow-up

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

::: security/manager/pki/resources/content/certManager.js
@@ +391,1 @@
>        if (fp.file.path.endsWith(type)) {

Hmm, I am wondering how this patch ever worked! Also I notice that gCertFileTypes has "*" in each type, but the original code checks for endsWith(".pb7") or whatnot. I think this might still fail.

I think it would be saner to just make gCertFileTypes an array and not split, btw.

Comment 19

[Dana Keeler (she/her) [:keeler]]

Attachment: follow-up v2

You're right - that patch didn't actually work either. One thing to note is this is actually the exception use case for gCertFileTypes - all other uses expect it to be of this form. Let me know what you think of this.

Comment 20

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8523195 [details] [diff] [review]
follow-up v2

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

LGTM, thanks for fixing.

Comment 21

[Dana Keeler (she/her) [:keeler]]

Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/b683d38c4c00

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b683d38c4c00