Closed Bug 940994 Opened 11 years ago Closed 10 years ago

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

Categories

(Core Graveyard :: Security: UI, defect)

x86
Windows XP
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: bns_robson, Assigned: shashank)

References

Details

(Whiteboard: [good first bug])

Attachments

(4 files, 3 obsolete files)

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.
Component: Untriaged → Security: UI
Product: Thunderbird → Core
Version: 17 → Trunk
Whiteboard: [good first bug]
Hi! I am ready o take up this bug. What needs to be done for this?
Flags: needinfo?(dkeeler)
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.
Flags: needinfo?(dkeeler)
Whiteboard: [good first bug]
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.
Whiteboard: [good first bug]
Assignee: nobody → shashank
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.
Attachment #8483185 - Flags: review?(dkeeler)
Flags: needinfo?(dkeeler)
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.
Attachment #8483185 - Flags: review?(dkeeler) → review+
Flags: needinfo?(dkeeler)
1) All nits addressed

2) Eliminated the redundant definition of file types

(Screenshots will be attached next)
Attachment #8483185 - Attachment is obsolete: true
Attachment #8496359 - Flags: review?(dkeeler)
Attached image before-patch.png
Before applying patch
Attachment #8496365 - Flags: review?(dkeeler)
Attached image after-patch.png
After applying patch
Attachment #8496366 - Flags: review?(dkeeler)
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'
Attachment #8496359 - Flags: review?(dkeeler) → review+
Comment on attachment 8496366 [details]
after-patch.png

Looks good.
Attachment #8496366 - Flags: review?(dkeeler)
Made the flag boolean
Renamed the flag
Made the scope of looping variable local
Added a space after 'if'
Attachment #8496359 - Attachment is obsolete: true
Attachment #8501340 - Flags: review?(dkeeler)
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.
Attachment #8501340 - Flags: review?(dkeeler) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1d6c19a7af9f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attached patch follow-up (obsolete) — Splinter Review
Turns out, I missed a couple of issues that break importing PKCS12 user certificates. This is just a quick follow-up to fix them.
Attachment #8520765 - Flags: review?(mmc)
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.
Attached patch follow-up v2Splinter Review
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.
Attachment #8520765 - Attachment is obsolete: true
Attachment #8523195 - Flags: review?(mmc)
Comment on attachment 8523195 [details] [diff] [review]
follow-up v2

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

LGTM, thanks for fixing.
Attachment #8523195 - Flags: review?(mmc) → review+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.