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)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla36
People
(Reporter: bns_robson, Assigned: shashank)
References
Details
(Whiteboard: [good first bug])
Attachments
(4 files, 3 obsolete files)
159.54 KB,
image/png
|
Details | |
159.59 KB,
image/png
|
Details | |
5.36 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
mmc
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Component: Untriaged → Security: UI
Product: Thunderbird → Core
Version: 17 → Trunk
Updated•10 years ago
|
Whiteboard: [good first bug]
Assignee | ||
Comment 2•10 years ago
|
||
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]
Updated•10 years ago
|
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.
Assignee | ||
Comment 6•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
Before applying patch
Attachment #8496365 -
Flags: review?(dkeeler)
Assignee | ||
Comment 10•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8496365 -
Flags: review?(dkeeler)
Comment on attachment 8496366 [details]
after-patch.png
Looks good.
Attachment #8496366 -
Flags: review?(dkeeler)
Assignee | ||
Comment 13•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6c19a7af9f
Keywords: checkin-needed
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1d6c19a7af9f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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 18•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8520765 -
Flags: review?(mmc)
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 20•10 years ago
|
||
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+
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•