315871 - In Certificate Viewer, allow to export cert or full chain

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, enhancement, P1)

Description

[Kai Engert (:KaiE:)]

In Certificate Viewer, allow to export the currently viewed cert or even the full chain

See also bug 161275, another bug that requests the ability to export certificates.

Comment 1

[Kai Engert (:KaiE:)]

Kaspar, thanks a lot for your work on your cert viewer plus extension.

I would like to provide such an export ability in our code by default, without the need for your extension, as it is really helpful.

I see that you have already put your code under the MPL/GPL/LGPL triple license, which is great, thanks a lot!

Are you ok that I reuse portions of your code and add it to PSM?

While I like the export button, I have problems with view-source. I would like the PSM code to work on Firefox, Thunderbird, and Seamonkey, too. Export works fine. View-source does not. So I would like to start without the view-cert UI.

Comment 2

[Kaspar]

(In reply to comment #1)
> Are you ok that I reuse portions of your code and add it to PSM?

Yes, please do so... glad to see you consider these features useful!

> While I like the export button, I have problems with view-source. I would like
> the PSM code to work on Firefox, Thunderbird, and Seamonkey, too. Export works
> fine. View-source does not.

I think the view-source problem with Seamonkey is relatively
easy to fix - it's just the chrome URL which needs to be adapted:
when using chrome://navigator/content/viewSource.xul (instead
of chrome://global/content/viewSource.xul, as for Firefox and
Thunderbird), view-source also works under Seamonkey.

The "Display in PEM Format" button is kind of a power-user feature,
that's true, but I'd be happy to see it added to the official
Mozilla releases... thanks.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: patch v1

patch based on Kaspar's code

I had produced this earlier, will look at view-source later, thanks

Comment 4

[Kaspar]

Attachment: proposal for patch v2

> will look at view-source later, thanks

Kai, I hope you don't mind if I'm proposing a slightly extended version of your patch, which again includes the "Display In PEM Format" button and works under both Firefox/Thunderbird and Seamonkey (by using #ifdef MOZ_SUITE). Maybe the title of this bug/RFE has to be adapted somewhat, then...

Another issue I encountered with v0.7 of Cert Viewer Plus is the fact that the access keys (shortcuts with underlined letters) only work on the second tab if they are set to the same value on both tabs ("General" and "Details"). I therefore had to add separate access keys for the second tab for both buttons.

Finally, a question relating to the formats available when exporting a cert: PKCS#7 would actually be a very useful option (it's the most common format when storing certs with their complete chain), but no appropriate method is available with XPConnect (JavaScript) so far. Is there any chance that SEC_PKCS7CreateCertsOnly() could be made available via PSM for this purpose? Something like getPKCS7(), which would return the PKCS#7 wrapped cert (raw bytes).

Comment 5

[Kaspar]

Attachment: getPKCS7() method for nsIX509Cert3 (scriptable)

> Is there any chance that SEC_PKCS7CreateCertsOnly() could be made available
> via PSM for this purpose? Something like getPKCS7(), which would return the
> PKCS#7 wrapped cert (raw bytes).

Here's a proposal for a patch that does that - it should apply to the 1.8 branch and adds getPKCS7() to nsIX509Cert3. There's also a small change to p7create.c in NSS to make sure that the selfsigned root is also included when the chain is added to the PKCS#7 data (not sure why the hardcoded default is to omit the root). If you consider this patch for inclusion in PSM, I guess I would have to file a separate NSS bug for this... but as a proof of concept, it works pretty well and allows to extend the "Save to File" formats by PKCS#7 (with or without chain). I've already created an updated version of Cert Viewer Plus with this capability - and of course I'll gladly provide it, if you're interested.

Comment 6

[Kaspar]

Attachment: getPKCS7(), second version

Well, seems like libsmime (nob libpkcs7) is the way to go in this case, as I learned in the meantime (from a relatively recent posting from Nelson to dev-tech-crypto). Here's the libsmime based version of getPKCS7(), which also has the advantage that no NSS code has to be changed. I've renamed the "omitChain" parameter to "includeChain", btw, as this is probably more intuitive for callers to use.

Comment 7

[Kaspar]

Attachment: getPKCS7(), third version

Aller guten Dinge sind drei, as the German proverb says. Upon further consideration, I think it makes sense to let the caller decide whether he wants to include the root or not. I've therefore reused the NSSCMSCertChainMode enum from libsmime, adding these constants to nsIX509Cert3.idl. The patch should apply cleanly to MOZILLA_1_8_BRANCH, and this time it also has the hunk with '#include "cms.h"' (which I omitted in v2 by mistake...).

Comment 8

[Kaspar]

Attachment: proposal for patch v3

Kai, to get one step further with this one, I've created an updated version of the patch (against trunk) which is again in sync with the code in Cert Viewer Plus 1.1. It adds both buttons and works for Firefox/Thunderbird as well as for Seamonkey (by using a MOZ_SUITE ifdef in viewCertDetails.js). If the getPKCS7() patch is also applied (see separate attachment, 230307), then exporting to PKCS#7 is supported, too.

Can you please review? (I will also be requesting review for the getPKCS7() patch in a couple of seconds). Thanks.

Comment 9

[Ajay Gautam]

This would be a great feature to have.
I am a Java dev, with a Linux dev workstation, and have to go to IE to export a certificate!

Comment 10

[Philipp Hug]

Is there anything else that needs to be done to have this patch integrated?

Comment 11

[Kaspar]

Attachment: patch v4

Kai, is there any chance of getting these patches into Firefox/Thunderbird 3? I'd really like to see my CertViewer Plus extension being obsoleted by these next releases.

To that end, I've updated the patch which has the changes to the frontend
(the one for getPKCS7() is left unchanged):

- I've added "Export" buttons to all the tabs of the cert manager (except for the "Your Certificates", which already has two "Backup" buttons for saving to PKCS#12). Cert selection (one or multiple) is handled similar to the Edit/Delete buttons, and it will allow to save to the following five formats: Base64/PEM (w/o or with chain), DER, and PKCS#7 (w/o or with chain - depends on the getPKCS7() patch, of course).

- The "Display In PEM Format" and "Save To File..." buttons no longer appear on the "General" tab of the cert viewer, but only on the "Details" tab. Bug 380775 is apparently going to overhaul the first tab, but leaves the second one to the "experts", so I think that's the right place for these two buttons. Note that I deliberately left the "Save To File..." button there, because it allows you to save a cert even if it's not permanently stored in the DB (e.g. when looking at a Web site cert with Firefox).

- A couple of diagnostic messages have been improved (also related to l10n).

- Some of the JavaScript functions are now in pippki.js (those called from both certManager.js and viewCertDetails.js).

And as with previous versions of this patch, view-source works for both Fx/Tb and Seamonkey (by using #ifdef MOZ_SUITE). I'll attach some collected screenshots to illustrate the UI changes in a few seconds.

Finally - I think getting these two patches in would also address bug 161275 (opened by you in 2002... ;-).

Please let me know if anything is missing (or if I should request reviews from other people, if you're too busy). Thanks!

Comment 12

[Kaspar]

Attachment: Collection of screenshots illustrating the UI changes

Comment 13

[Kai Engert (:KaiE:)]

Johnathan, FYI, this is a feature that we desperately need for debugging purposes, and I'd like to get it in by before the firefox 3 UI feature freeze deadline.

I really like the idea to have an export button inside of certificate viewer.

This allows us to export any kind of cert, regardless of the context one encounters the cert, whether the cert is stored in our databases or just shown because it's being used by a web server or for signing an email message.

I'm not so sure about the "display in PEM format" button.
This seems to be a power user feature. You can achieve the same thing by using "export" and opening the exported file with a text editor.

Regarding the label for the "Save to file" button, I wonder if we should call that button "export" as well, for consistency.

Comment 14

[Johnathan Nightingale [:johnath]]

To be entirely fair, the entire cert management infrastructure can be seen as a "power user feature" for many definitions of "power user."  :)  

I think the idea of supporting export makes a lot of sense.  I think 95%, maybe 99%, of Firefox users won't use it, but on the other hand, it feels like a missing piece of PSM, really, and with a patch in hand it's hard for me to see much downside.

I share your concern that adding the PEM Format button is one level MORE power user.  I also expect more users to see the cert viewer (hence bug 380775) than the cert manager, which makes me wary.  I appreciate Kaspar moving it to the details tab, but even still, I feel like filling the gap in export functionality in PSM is a definite win, the PEM format support feels more like extension territory: legitimate and valuable, but to a specific set of users sophisticated enough to be able to find it, when desired.

Comment 15

[Kaspar]

So, to conclude, would it be ok then, if I produce an updated version of the patch which

1) leaves out the "Display In PEM Format" button (and the related JS code)

2) changes the "Save To File..." button to "Export", but keeps it on the "Details" tab (btw, should it be "Export" or "Export...", what rules do apply?)

3) but can rely on the getPKCS7() method being available with nsIX509Cert3.idl (i.e. the patch to nsNSSCertificate.cpp being incorporated)?

If any other things should be changed with an updated version of the patch, please let me know.

(In reply to comment #13)
> I'm not so sure about the "display in PEM format" button.
> This seems to be a power user feature. You can achieve the same thing by using
> "export" and opening the exported file with a text editor.

Note that when writing a cert to a file in Base64 format, I do not add subject and issuer names in plain text (this is only shown in PEM view), so strictly speaking it isn't the same thing. But anyway, if people think the PEM view should stay in an extension, then I'm ok do to so.

Comment 16

[Johnathan Nightingale [:johnath]]

(In reply to comment #15)
> So, to conclude, would it be ok then, if I produce an updated version of the
> patch which
> 
> 1) leaves out the "Display In PEM Format" button (and the related JS code)
> 
> 2) changes the "Save To File..." button to "Export", but keeps it on the
> "Details" tab (btw, should it be "Export" or "Export...", what rules do apply?)

As for the ellipsis, the typical rule is that a textual UI element (button, menu item, etc) should have a '...' if it launches a secondary dialog or otherwise requires supplmental information, and no '...' if it performs its action silently.  Since you do require supplemental information (file name, location, format) the ... is appropriate.

> 3) but can rely on the getPKCS7() method being available with nsIX509Cert3.idl
> (i.e. the patch to nsNSSCertificate.cpp being incorporated)?
> 
> If any other things should be changed with an updated version of the patch,
> please let me know.

I won't comment on the technical dependencies on NSS/PSM, but other than that, it looks good to me.

Comment 18

[Thomas Schaefer]

I'm sure for users using signed or encrypted mails, this function is very important - at least for me (a existing import-function could be much more usefull if one has a export-function to create a importable PKCS#7-file too). 

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 18, the easiest way *by far* to help someone else 
to import your EMAIL certificate is to send him a signed email.  
The signed email will contain the necessary certs, and his email 
client will typically import and remember the certs IF they are valid.
So, why is a PKCS#7 file needed for EMAIL certs?  

Comment 20

[Richard]

This is a standard feature in IE.  It should also be a standard feature in Firefox.  I recently had a customer who needed to obtain a certificate in PEM format and rather than explain the intricacies of openssl, I explained them how to obtain a PEM formatted certificate from their browser.  They may not have been a basic user, but neither would I describe them as a power user; they were simply someone with a vanity domain.

Comment 21

[Rupert Kolb]

I got a certificate from "Thawte Personal Freemail CA".
It is now "in" my seamonkey 1.1.4. After "backup" are both(!) in one(!) file.
How can I extract either the public or the private part (for instance for usage in other programs or for distribution (of the public one) to others.)  

Rupert

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Rupert, 
Bug reports are not the place to ask for help with using mozilla software.
You should ask in the mozilla newsgroups or mailing lists.  

Comment 23

[Rupert Kolb]

Sorry, I didn't illustrate it enough:
This is a feature request! In the meantime this missing feature might be considered to be a bug!
The request is getting a bit long in the tooth: See the request / bug report of Lance Haverkamp:
https://bugzilla.mozilla.org/show_bug.cgi?id=161275#c5

I came across this as I tried to certify OpenOffice.org documents. OpenOffice.org is using the seamonkey cerificates. And my seamonkey certificates are at a very uncommon place. The same is with the other users here.
Now I have to deal with the certificates and need some tools. As the certificates are "in the seamonkey/mozilla" I address the mozilla suite to provide the appropriate tools. 

Maybe there should be a better conception for handling this kind of certificates within linux. But that's an other topic. 

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Rupert, 
So, your complaint is that OpenOffice doesn't provide it's own tools
for managing its cert database.  Don't ask mozilla to be the tool for 
OpenOffice.  Ask OpenOffice.  

NSS (the products whose crypto libraries are used in Mozilla) comes
with command-line based tools that will probably work for you.
But moilla bug reports are NOT the place to inquire about them.  

Comment 25

[Kaspar]

Attachment: patch v5

Updated patch (which I had produced in July already). Changes compared to v4:

- omit the "Display In PEM Format" button (and related JS code)
- changes text of "Save To File..." to "Export..."
- use nsIPrompt alert() for better title bar texts
  (avoid "[JavaScript Application]")

As far as the ellipsis is concerned, there are a couple of other buttons in Cert Manager which should be changed, too, I guess (from certManager.dtd: certmgr.delete.label, certmgr.backup.label, certmgr.backupall.label, certmgr.restore.label, maybe also certmgr.view.label?). Should we have a separate bug for this? Otherwise, I can also integrate them into this patch, of course.

Kai, any chance that you can do a review of the two patches (v5 and the getPKCS7 one) in the next few days? I would be glad if I could finish working on this relatively soon. Thanks.

Comment 26

[Kai Engert (:KaiE:)]

Comment on attachment 277293 [details] [diff] [review]
patch v5

Kaspar,

Sorry for being so late with this review, I have been swamped with so many other important issues :-/

I really appreciate your work on this, it works and is a real improvement.
However, I am requesting a lot of changes.
Because it took me so long to get this done, and because of the amount of changes,
I went ahead and coded the changes I'd like to see.

Please have a look and let me know what you think.

Thanks for removing the "view as pem" module.
I understand that you added subject/issuer lines in the view mode.
I think we can still do that when saving as a PEM file,
the file format allows plain text lines outside the blocks that
are marked by the separation lines.

I took some of your code from your earlier patch and added it to the getPemString function.
I don't understand why you were trying to reverse things, the string looks fine to me.
When I export a chain of one of my certs, I get this:
  Subject: E=[e-mail],CN=Kai Wolfgang Engert,givenName=Kai Wolfgang,SN=Engert
  Issuer: CN=Thawte Personal Freemail Issuing CA,O=Thawte Consulting (Pty) Ltd.,C=ZA


What follows are comments that explain to your patch.
Things which are NOT yet done are marked with ###


+CertFormatBase64=X.509 Certificate (Base64/PEM)
+CertFormatBase64Chain=X.509 Certificate with chain (Base64/PEM)

I suggest to omit the wording Base64 and only say PEM,
unless you know a good reason to keep it.


+CertExportSuccess=Certificate successfully saved to %S\n[format: %S].

Do we really need a "filed saved ok" success message?
That's very uncommon, I think we only should show a message on failures.
I removed it.


+noCurrentCertTitle=Error
+noCurrentCert=Failed to determine which certificate is currently selected.

I think we don't need this message.
The export button should be clickable only when a cert is selected.
There are no such messages for the delete, view etc. buttons either.
Should the button be enabled although no cert is selected, that would be a bug, and I think it's fine to be silent.
I removed it.


+writeFileFailure=File Error
+writeFileFailed=Can't write to file %S:\n%S.
+writeFileAccessDenied=Access denied
+writeFileIsLocked=File is locked
+writeFileNoDeviceSpace=No space left on device
+writeFileUnknownError=Unknown error

I wonder if there is general error code somewhere in the tree to report this kind of failure on file save.
But for now let's keep it.


+  var enableExportButton=document.getElementById('websites_exportButton');

Just a note to myself, small merge conflict with the patch in bug 327181.


+function getPEMString(cert)
+{
+  return '-----BEGIN CERTIFICATE-----\r\n'
+         + btoa(getDERString(cert)).replace(/(\S{64}(?!$))/g, "$1\r\n")

### Can you please explain what this regular expression does?
(as a comment that we can add to the code)


+function exportToFile(parent, cert)
+{
+  var bundle = srGetStrBundle("chrome://pippki/locale/pippki.properties");
+  if (!cert) {
+    var ps = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].
+             getService(Components.interfaces.nsIPromptService);
+    ps.alert(window, bundle.GetStringFromName("noCurrentCertTitle"),
+             bundle.GetStringFromName("noCurrentCert"));

We should avoid having this code twice in the same function.
Let's use a function.


+  fp.defaultString = cert.commonName.replace(/\s*/g,'') || cert.windowTitle;

### Can you please explain this regular expression as a comment?


+  fp.defaultExtension = "crt";
+  fp.appendFilter(bundle.GetStringFromName("CertFormatBase64"), "*.crt; *.pem");
+  fp.appendFilter(bundle.GetStringFromName("CertFormatBase64Chain"), "*.crt; *.pem");
+  fp.appendFilter(bundle.GetStringFromName("CertFormatDER"), "*.der");
+  fp.appendFilter(bundle.GetStringFromName("CertFormatPKCS7"), "*.p7c");
+  fp.appendFilter(bundle.GetStringFromName("CertFormatPKCS7Chain"), "*.p7c");
+  fp.appendFilters(nsIFilePicker.filterAll);
+  var res = fp.show();
+  if (res == nsIFilePicker.returnOK || res == nsIFilePicker.returnReplace) {

You don't do anything in the "else" case, we should invert the check and return.


+    switch (fp.filterIndex) {
+      case 0:
+        content = getPEMString(cert);
+        format = bundle.GetStringFromName("CertFormatBase64");
+        break;
...
+      default:
+        content = getPEMString(cert);
+        format = bundle.GetStringFromName("CertFormatBase64");
+        break;

instead of duplicating this case, "case 0:" and "default:" should be next to each other.


+    var msg = bundle.GetStringFromName("writeFileUnknownError");

Delay getting this string until we really need it.
Declare and check for "not yet set".


+    try {
+      var file = Components.classes["@mozilla.org/file/local;1"].
+                 createInstance(Components.interfaces.nsILocalFile);
+      file.initWithPath(fp.file.path);
+      var fos = Components.classes["@mozilla.org/network/file-output-stream;1"].
+                createInstance(Components.interfaces.nsIFileOutputStream);
+      // flags: PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE
+      fos.init(file, 0x02 | 0x08 | 0x20, 00644, 0);
+      res = fos.write(content, content.length);

You are re-using var res, and will check its value further below.
But an exception could have been thrown before the assignment.
Let's use a different variable.


+   *  Constants for specifying the chain mode when retrieving a certificate
+   */
+  const unsigned long CMS_CHAIN_MODE_None = 0;
+  const unsigned long CMS_CHAIN_MODE_CertOnly = 1;

You use _None when calling into the export function,
but you never use _CertOnly.

Inside the export function, you never check for _None and never for _CertOnly.
The function should exit for the value that it does not support.

I think you should pass _CertOnly. We don't need the _None constant.


+  const unsigned long CMS_CHAIN_MODE_CertChain = 2;
+  const unsigned long CMS_CHAIN_MODE_CertChainWithRoot = 3;

You call the function "pkcs7" but the constants are name "cms".
We should be consistent.
As you're using only CMS functions, let's call the IDL function CMS, too

I saw you require that these numerical constants match the 
numerical values that are defined inside NSS.
This is dangerous, as the constant in NSS might change.
I would have proposed to explicitly translate the IDL constants to the NSS
constants, and return a failure on unexpected values.
But then I noticed you never pass any of these constants to a
NSS functions, so let's avoid using the NSS constants at all.


+NS_IMETHODIMP
+nsNSSCertificate::GetPKCS7(PRUint32 chainMode,
+                           PRUint32 *aLength, PRUint8 **aArray)

I propose we rename to ExportAsCMS.
Let's ensure we got non-null pointers.


+{
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown())
+    return NS_ERROR_NOT_AVAILABLE;
+
+  if (mCert) {

let's return a failure if there is no cert and return early,
this avoids the nesting.

This function has several scenarios where you leak memory.
Let's avoid goto for new PSM code and make use of the C++
automatic cleanup helpers.

You often use if ((var = something()) != compare_value)
This makes debugging and setting breapoints difficult,
and it's also easy to misread.
I would prefer to have assignments and if on separate lines.

I'm learning that you took this code mostly from nsCMS.cpp,
so I understand where this came from, but I still prefer
we do this better for new code.


### One functional comment about your patch.
The destination file name does not have an extension,
I think after the user hits ok, we should automatically add
the extension like ".pem", no?

Comment 27

[Kai Engert (:KaiE:)]

Attachment: Patch v6

Comment 28

[Kaspar]

Comment on attachment 282141 [details] [diff] [review]
Patch v6

(In reply to comment #26)

Thanks for your detailed review comments and for improving my code, Kai!

> Because it took me so long to get this done, and because of the amount of
> changes,
> I went ahead and coded the changes I'd like to see.
> 
> Please have a look and let me know what you think.

I'm fine with them in general, but nevertheless I'm giving an overall r- for this version of the patch (somewhat special actually to do this for what is mostly my own code ;-).

Please consider the following comments/proposed changes:

> I understand that you added subject/issuer lines in the view mode.
> I think we can still do that when saving as a PEM file,
> the file format allows plain text lines outside the blocks that
> are marked by the separation lines.

This is true for some implementations reading PEM stuff (NSS and OpenSSL, notably). However, there's at least one case where this doesn't work... - Sun's Java keytool (try -printcert or -importcert). For maximum compatibility, I would advise not to add any subject/issuer lines to the PEM files.


> I don't understand why you were trying to reverse things, the string looks fine
> to me.

I wanted to have the RDNs displayed in the order they appear in the ASN.1 SEQUENCE, not in the "LDAP-style" order which CERT_NameToAscii() unconditionally uses. But the regex I used for this fails on a couple of corner cases anyway, so I decided to drop it completely.


> +CertExportSuccess=Certificate successfully saved to %S\n[format: %S].
> 
> Do we really need a "filed saved ok" success message?
> That's very uncommon, I think we only should show a message on failures.
> I removed it.

Please consider adding it again, for these two reasons:

a) The PKCS#12 export/import function has similar messages, too ("Successfully backed up your security certificate(s) and private key(s)" etc.)

b) When saving a file, the user might not remember the exact folder location he chose to save the file to, and therefore find it helpful if the dialog tells him where (and under what name) the file has been saved (otherwise, he has to reopen and cancel the export dialog, just to find out the directory the file was saved to).


> +noCurrentCertTitle=Error
> +noCurrentCert=Failed to determine which certificate is currently selected.
> 
> I think we don't need this message.
> The export button should be clickable only when a cert is selected.
> There are no such messages for the delete, view etc. buttons either.
> Should the button be enabled although no cert is selected, that would be a bug,
> and I think it's fine to be silent.
> I removed it.

Ok, but you should also remove these strings from pippki.properties, then.


> +writeFileFailure=File Error
> +writeFileFailed=Can't write to file %S:\n%S.
> +writeFileAccessDenied=Access denied
> +writeFileIsLocked=File is locked
> +writeFileNoDeviceSpace=No space left on device
> +writeFileUnknownError=Unknown error
> 
> I wonder if there is general error code somewhere in the tree to report this
> kind of failure on file save.

This is what I was assuming, too, when I wrote that code - but as this search e.g.

http://mxr.mozilla.org/seamonkey/search?string=NS_ERROR_FILE_ACCESS_DENIED&find=.js%24&findi=&filter=&tree=seamonkey

will demonstrate, there's apparently only one other place with JS code where NS_ERROR_FILE_* is handled (and this one has "addressbook" in its strings, cf. http://mxr.mozilla.org/seamonkey/source/suite/locales/en-US/chrome/mailnews/addressbook/addressBook.properties#148).


> +function getPEMString(cert)
> +{
> +  return '-----BEGIN CERTIFICATE-----\r\n'
> +         + btoa(getDERString(cert)).replace(/(\S{64}(?!$))/g, "$1\r\n")
> 
> ### Can you please explain what this regular expression does?
> (as a comment that we can add to the code)

This wraps the Base64 string into lines of 64 characters each, with CRLF line breaks (as specified in RFC 1421).


> +  fp.defaultString = cert.commonName.replace(/\s*/g,'') || cert.windowTitle;
> 
> ### Can you please explain this regular expression as a comment?

Removes whitespace from the commonName RDN, to make it more suitable for use as a file name. Should better be changed to

+  fp.defaultString = (cert.commonName || cert.windowTitle).replace(/\s*/g,'');

actually.


> +      res = fos.write(content, content.length);
> 
> You are re-using var res, and will check its value further below.
> But an exception could have been thrown before the assignment.
> Let's use a different variable.

> +  var written_bytes = 0;

I'd prefer "bytesWritten" (or maybe just "written").


> +    if (!msg.length) {
> +      msg = bundle.GetStringFromName("writeFileUnknownError");
> +    }

No need for brackets here.


> +NS_IMETHODIMP
> +nsNSSCertificate::GetPKCS7(PRUint32 chainMode,
> +                           PRUint32 *aLength, PRUint8 **aArray)
> 
> I propose we rename to ExportAsCMS.

I'm fine with your changes to nsNSSCertificate.cpp - thanks for all the improvements! "CMS" is probably less well known than PKCS7 (and might primarily be associated with things like "Content Management System"), but I have no strong feelings about changing GetPKCS7 to ExportAsCMS.

nsIX509Cert3 needs a new uuid, I guess (since we're changing the interface).


> ### One functional comment about your patch.
> The destination file name does not have an extension,
> I think after the user hits ok, we should automatically add
> the extension like ".pem", no?
> 

Yes - actually, 

> fp.defaultExtension = "crt";

was meant to achieve that, but it apparently doesn't have the same effect on all platforms (I prefer ".crt" over ".pem" because it better indicates what the file contains - ".pem" could also refer to a private key e.g.).

Unfortunately, appending the extension automatically (also for the DER and PKCS#7 formats) doesn't seem to work reliably on Linux and OS X, e.g. We could "fix up" the filename before saving, with code like this:

+  var content = '';
+  var format = '';
+  var filename = fp.file.path;

[...]

+    case 0:
+    default:
+      content = getPEMString(cert);
+      format = bundle.GetStringFromName("CertFormatBase64");
+      if (filename.substr(-4).toLowerCase() != ".crt")
+        filename += ".crt";
+      break;

[etc., same for the other cases...]

+    file.initWithPath(filename);

... but then we have the problem that we might unconditionally truncate a file (since the filepicker possibly only checked for a file without the extension). Do you see a better solution to this?


Finally, a last point: as we now use

> +<!ENTITY certmgr.export.label                 "Export...">

you should also add the ellipsis to certmgr.delete.label, certmgr.backup.label, certmgr.backupall.label and 
certmgr.restore.label (maybe also certmgr.view.label) - see comment #16 and comment #25 above.

And again, thanks for your help in getting this feature into the tree!

Comment 29

[Kaspar]

Comment on attachment 230307 [details] [diff] [review]
getPKCS7(), third version

marking obsolete, cancelling review request

Comment 30

[Michal 'hramrach' Suchanek]

(In reply to comment #28)
> (From update of attachment 282141 [details] [diff] [review])
> (In reply to comment #26)
> 
>

> > +CertExportSuccess=Certificate successfully saved to %S\n[format: %S].
> > 
> > Do we really need a "filed saved ok" success message?
> > That's very uncommon, I think we only should show a message on failures.
> > I removed it.
> 
> Please consider adding it again, for these two reasons:
> 
> a) The PKCS#12 export/import function has similar messages, too ("Successfully
> backed up your security certificate(s) and private key(s)" etc.)
> 
> b) When saving a file, the user might not remember the exact folder location he
> chose to save the file to, and therefore find it helpful if the dialog tells
> him where (and under what name) the file has been saved (otherwise, he has to
> reopen and cancel the export dialog, just to find out the directory the file
> was saved to).

I am strongly against "Saved OK" dialogs unless they have a "don't show this again" checkbox. They make using the interface even more tedious than the average GUI.

> > ### One functional comment about your patch.
> > The destination file name does not have an extension,
> > I think after the user hits ok, we should automatically add
> > the extension like ".pem", no?
> > 
> 
> Yes - actually, 
> 
> > fp.defaultExtension = "crt";
> 
> was meant to achieve that, but it apparently doesn't have the same effect on
> all platforms (I prefer ".crt" over ".pem" because it better indicates what the
> file contains - ".pem" could also refer to a private key e.g.).
> 
> Unfortunately, appending the extension automatically (also for the DER and
> PKCS#7 formats) doesn't seem to work reliably on Linux and OS X, e.g. We could
> "fix up" the filename before saving, with code like this:

Fixing up the name selected in the picker is a Bad Thing, makes the GUI very hard to use because it becomes unpredictable. It would be better if the preselected name contained the correct extension. But if it does not so be it.

It might be nice to implement/fix a feature in filepicker that would allow automatically adding an extension (like the add extension checkbox in GIMP, or a button that appends it to the filename immediately). But that's a filepicker problem, not problem of this code.

Thanks

Michal

Comment 31

[Kai Engert (:KaiE:)]

(In reply to comment #28)
> For maximum compatibility, I
> would advise not to add any subject/issuer lines to the PEM files.

Convincing, removed.


> > +         + btoa(getDERString(cert)).replace(/(\S{64}(?!$))/g, "$1\r\n")
> > 
> This wraps the Base64 string into lines of 64 characters each, with CRLF line
> breaks (as specified in RFC 1421).

Comment added. I split this into separate lines, I find it easier to read.


> b) When saving a file, the user might not remember the exact folder location he
> chose to save the file to, and therefore find it helpful if the dialog tells
> him where.

But it's like that in all applications where you save.


> > Do we really need a "filed saved ok" success message?
> 
> a) The PKCS#12 export/import function has similar messages, too ("Successfully
> backed up your security certificate(s) and private key(s)" etc.)

It's still uncommon.
I'd rather remove the other message eventually.


> > +noCurrentCertTitle=Error
> > +noCurrentCert=Failed to determine which certificate is currently selected.
> > 
> > I think we don't need this message.
> 
> Ok, but you should also remove these strings from pippki.properties, then.

removed now


> > +  fp.defaultString = cert.commonName.replace(/\s*/g,'') || cert.windowTitle;
> > ### Can you please explain this regular expression as a comment?
> 
> Removes whitespace from the commonName RDN, to make it more suitable for use as
> a file name.

comment added


> Should better be changed to
> +  fp.defaultString = (cert.commonName || cert.windowTitle).replace(/\s*/g,'');
> actually.

reformatted into separate lines to make it easier to understand.


> > +  var written_bytes = 0;
> I'd prefer "bytesWritten" (or maybe just "written").

changed to "written"


> > +    if (!msg.length) {
> > +      msg = bundle.GetStringFromName("writeFileUnknownError");
> > +    }
> No need for brackets here.

removed


> nsIX509Cert3 needs a new uuid, I guess (since we're changing the interface).

changed, although I will do that again in bug 327181 :-)


> > ### One functional comment about your patch.
> > The destination file name does not have an extension,
> > I think after the user hits ok, we should automatically add
> > the extension like ".pem", no?

After reading comments from Kaspar and Michal I'm convinced this should be
seen as a file picker bug.

We can't set the correct extension right from the beginning, 
because the user can change the output format.
I guess, all we could do is to use a standard extension like .cert


> you should also add the ellipsis to certmgr.delete.label, certmgr.backup.label,
> certmgr.backupall.label and 
> certmgr.restore.label (maybe also certmgr.view.label)

I was told when adding the "..." we should:
- change the string IDs, so localizers will notice
- use the special hellip … char (not three separate dots)

Because of that, the patch must convert all code to use the new ID, too.

done

Comment 32

[Kai Engert (:KaiE:)]

Attachment: Patch v7

Comment 33

[Kai Engert (:KaiE:)]

(In reply to comment #21)
> I got a certificate from "Thawte Personal Freemail CA".
> It is now "in" my seamonkey 1.1.4. After "backup" are both(!) in one(!) file.
> How can I extract either the public or the private part (for instance for usage
> in other programs or for distribution (of the public one) to others.)  

After we land this patch, you will be able to export the public portion of your certificate by using "view" then "details tab" then "export", so this part of the feature request will be satisfied.

I don't think we'll add a feature for exporting the private key only.

Comment 34

[Kaspar]

Comment on attachment 282243 [details] [diff] [review]
Patch v7

r+, with one change requested: since we're dropping the CertExportSuccess dialog (alas), there's no need for the "format" variable in pippki.js. Please remove the declaration and the six assignments.

Comment 35

[Kaspar]

Comment on attachment 267569 [details]
Collection of screenshots illustrating the UI changes

marking obsolete - most of the screenshots do no longer reflect what the patch (v7) now implements

Comment 36

[Kai Engert (:KaiE:)]

Attachment: Patch v8

Thanks Kaspar, removed.

I'd like to invite Bob to have a final look.

Comment 37

[Kai Engert (:KaiE:)]

Comment on attachment 267569 [details]
Collection of screenshots illustrating the UI changes

The changes after these screenshots are:

- most buttons now end with ...
- first two export formats have "base64" removed, they say (PEM)
- lower left screenshot no longer used
- lower right screenshot: single button only, "Export..."

Comment 38

[Robert Relyea]

Comment on attachment 282317 [details] [diff] [review]
Patch v8

r+ assuming an explanation for the following:

Why are we fetching one cert in the chain manually (CertFindIssuer()) then calling the chain construction? I'm worried that we may get 'off in the boonies' once we start running in bridged environments where fetching the issuer is not as simple as a GetCertBySubject and sorting by date.

For now the code is OK.

bob

Comment 39

[Kaspar]

Comment on attachment 282317 [details] [diff] [review]
Patch v8

(In reply to comment #38)

Thanks for the review, Bob.

> r+ assuming an explanation for the following:
> 
> Why are we fetching one cert in the chain manually (CertFindIssuer()) then
> calling the chain construction?

Basically because NSS_CMSSignedData_CreateCertsOnly() will always omit the root certificate (it calls NSS_CMSSignedData_AddCertChain(), which in turn calls CERT_CertChainFromCert(), but always with includeRoot = PR_FALSE).

If we used CERT_CertChainFromCert() for mCert itself, then we would end up with duplicate certs in the CMS SignedData, so that's why we're calling CERT_FindCertIssuer() first, and then get the chain starting at that issuer cert.

(Another option might be to add an includeRoot parameter to NSS_CMSSignedData_CreateCertsOnly(), too, but this would mean a change to a [public] libsmime function. Would that be doable? If so, I could file a separate bug about this.)

Requesting approval1.9, since I assume that this is needed for checkin (please correct me if I'm wrong).

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Kaspar, I'm sorry, but the explanation in the preceding comment doesn't 
make sense to me.  

The first paragraph says the problem is that NSS_CMSSignedData_CreateCertsOnly will omit the root.  So I gather you're trying to ensure that the root is 
included and not omitted.  But calling CERT_FindIssuer to get the lowest 
level issuer, and then calling CERT_CertChainFromCert() to find certs above 
that issuer STILL will not get the root.  So, that sequence of calls will 
either 
a) not fix the problem it is intended to solve, or 
b) omission of the root is not the problem.

I thought perhaps you were going to write that the problem is that the leaf
cert is duplicated, because the cert chain returned by 
CERT_CertChainFromCert() would include the leaf cert from which it starts,
but the leaf cert is already included, and so you're starting to look from
the issuer of the leaf in order to avoid duplicating it.  But that doesn't
seem to be what you wrote. :(

Comment 41

[Kaspar]

(In reply to comment #40)
> But calling CERT_FindIssuer to get the lowest 
> level issuer, and then calling CERT_CertChainFromCert() to find certs above 
> that issuer STILL will not get the root.

This statement is not true, IMHO - let's look at the function definition:

 extern CERTCertificateList *
 CERT_CertChainFromCert(CERTCertificate *cert, SECCertUsage usage,
                        PRBool includeRoot);

Calling CERT_CertChainFromCert() and setting includeRoot to true DOES get the root (provided that there is one available in the cert DB, of course).

I've successfully used this to export certs in PKCS#7 format, and they do include the root (my JS code only uses the _CertChainWithRoot option, but _CertChain works as well). If it doesn't work for you, then I guess this must be some bug with CERT_CertChainFromCert()...

Comment 42

[Nelson Bolyard (seldom reads bugmail)]

It is true that changing the includeRoot argument to true will cause the 
returned chain to include the root.  That will happen whether or not you
call CERT_FindIssuer before calling it.  The issue and question is:
why call CERT_FindIssuer before calling CERT_CertChainFromCert? 
The present answer -- to get the root -- doesn't explain the necessity 
for CERT_FindIssuer.

Comment 43

[Kaspar]

(In reply to comment #42)
> The issue and question is:
> why call CERT_FindIssuer before calling CERT_CertChainFromCert? 
> The present answer -- to get the root -- doesn't explain the necessity 
> for CERT_FindIssuer.

I thought I had explained this already - in comment #39 above and in a source comment in nsNSSCertificate.cpp, actually:

  /*
   * Calling NSS_CMSSignedData_CreateCertsOnly() will not allow us
   * to specify the inclusion of the root, but CERT_CertChainFromCert() does.
   * Since CERT_CertChainFromCert() also includes the certificate itself,
   * we have to start at the issuing cert (to avoid duplicate certs
   * in the SignedData).
   */

So, to conclude this discussion (hopefully), here's another attempt at explaining the necessity for CERT_FindIssuer(): first, these are the two limitations we have to cope with:

a) We want to include the root certificate, but NSS_CMSSignedData_CreateCertsOnly() never does so, even if include_chain is set to true. Furthermore, this function takes a CERTCertificate as its argument, not a CERTCertificateList - cf. its definition:

  /*
   * NSS_CMSSignedData_CreateCertsOnly - create a certs-only SignedData.
   *
   * cert          - base certificates that will be included
   * include_chain - if true, include the complete cert chain for cert
   *
   * More certs and chains can be added via AddCertificate and AddCertChain.
   *
   * An error results in a return value of NULL and an error set.
   */
  extern NSSCMSSignedData *
  NSS_CMSSignedData_CreateCertsOnly(NSSCMSMessage *cmsg, CERTCertificate *cert, PRBool include_chain);

b) CERT_CertChainFromCert() returns a CERTCertificateList which always includes the certificate itself, too.


We therefore go through these steps:

1. Call NSS_CMSSignedData_CreateCertsOnly() with include_chain = false. This adds the certificate itself, only.

2. Find the issuer with CERT_FindIssuer()

3. Get the chain of the issuer cert from step 2 using CERT_CertChainFromCert()

4. Add the cert list we retrieved in step 3 using NSS_CMSSignedData_AddCertList()


I don't see a solution to work around limitations a) and b) which doesn't require CERT_FindIssuer()... if anybody does, then please raise your voice.

Comment 44

[Robert Relyea]

So in short, you need to use CERT_FindIssuer to prevent duplicate certificates (as Nelson surmised).

Another way to do that is to use NSS_CMSSignedData_CreateCertsOnly(), except it does not have an option to include the root cert.

I think that suffices for the explanation I wanted. Please file an enhancement request for NSS_CMSSignedData_CreateCertsOnly() against NSS. We can't change an existing signature, but we can create a new function that accepts new parameters.

Thanks,

bob

Comment 45

[Kaspar]

(In reply to comment #44)
> Please file an enhancement request for NSS_CMSSignedData_CreateCertsOnly()
> against NSS. We can't change an existing signature, but we can create a new
> function that accepts new parameters.

Ok, did that - bug 397990.

Kai, can you please check in attachment 282317 [details] [diff] [review] (now that everything has been settled, and we also got approval1.9+ in the meantime)? Thanks.

Comment 46

[Kai Engert (:KaiE:)]

Patch checked in, marking fixed.

Thanks Kaspar!

Comment 47

[Kai Engert (:KaiE:)]

While trying to translate the new wordings, KaiRo made me aware, the text "Certificate with chain" seems unclear.

We are proposing to change this to "Certificate including Issuers".

This is for a "file type string" shown in the "file save as dialog", so we should try to keep it short.

Opinions?

Comment 48

[Kaspar]

(In reply to comment #47)
> While trying to translate the new wordings, KaiRo made me aware, the text
> "Certificate with chain" seems unclear.
> 
> We are proposing to change this to "Certificate including Issuers".

Hmm, "Issuers" sounds a bit vague to me, and issuer != issuer cert, see e.g. (from nsserrors.properties):

SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE=The certificate issuer's certificate has expired.  Check your system date and time.

Where exactly is the problem with "chain" for l18n? Would changing to "... with trust chain" help?

Comment 49

[Robert Kaiser]

Kaspar:
It's "L10n", the other thing is "i18n", but you mixed them up. :)

The problem is that neither a normal user in English user nor a localizer has a real clue what a "certificate with chain" actually is. Additionally, abstract words like "chain" are always a bit hard to get right in localization, as this easily can sound like "a certificate with a band of metal rings" or something similar when localized. Still, anything explaining what kind of chain is surely already better than just "chain".

Additionally, when you're at it, could you also fix the spacing on the strings you changed? due to adding a character, the strings don't line up in the file any more (not important to how it works, just a style nit).

Comment 50

[Kaspar]

(In reply to comment #49)
> The problem is that neither a normal user in English user nor a localizer has
> a real clue what a "certificate with chain" actually is.

True, but the normal user will probably not stumble over this file dialog anyway (nor will he fiddle with cert manager in general)...

> Additionally, abstract
> words like "chain" are always a bit hard to get right in localization, as this
> easily can sound like "a certificate with a band of metal rings" or something
> similar when localized.

I see this point (linguistically speaking, it's a question of disambiguation, which is a fundamental difficulty when translating - but I guess you're aware of that). I hope that localizers do not just blindly translate strings from Mozilla source files ;-)

> Still, anything explaining what kind of chain is surely
> already better than just "chain".

So, can we agree on changing it to "trust chain"? Note that "certificate chain" is actually included in the glossary -

http://lxr.mozilla.org/seamonkey/source/suite/locales/en-US/chrome/common/help/glossary.xhtml#92

so I think it isn't that bad to use the word "chain" in this dialog.

> Additionally, when you're at it, could you also fix the spacing on the strings
> you changed? due to adding a character, the strings don't line up in the file
> any more (not important to how it works, just a style nit).

You mean those in certManager.dtd? They are from Kai, actually ;-) But yes, no problem to fix them if we agree on the change(s) to pippki.properties.

Comment 51

[Nelson Bolyard (seldom reads bugmail)]

The RFC standards use different terminology for certs than the terminology
to which the Mozilla community has become accustomed.  Using and localizing 
standards-based terms might result in the translated phrase being one that
the user might have seen elsewhere.  But I doubt it does anything to help 
the translator who has no knowledge of the concepts.
    old mozilla term     new standard term
    -----------------    ------------------
    certificate chain -> certification path
    leaf              -> end entity
    root              -> trust anchor

I'd prefer either "certificate chain" or "certification path" to 
"Certificate including Issuers"

Comment 52

[Kai Engert (:KaiE:)]

So currently we say "Certificate with chain".

"Certificate with certificate chain" would be clear, but it seems long.

Can we simply say "Certificate chain"? 

Or maybe "Complete certificate chain"?
Or maybe "Full Certificate chain"?

Comment 53

[Kaspar]

(In reply to comment #52)
> Can we simply say "Certificate chain"? 
> 
> Or maybe "Complete certificate chain"?
> Or maybe "Full Certificate chain"?

In my understanding of "chain", all of these terms do not include the end-entity cert itself (which will be included in the file, of course), so my answer to these questions is no.

Robert, is there really no possibility to help l8s (guess what this stands for...?) in translating "certificate with chain" by means of additional out-of-band information (such as a comment in .properties or so)? I would prefer this solution over trying to tweak the original wording, just to make it "better suitable" for the translators.