310446 - Add a user feedback message when certs for import are being ignored

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Kai Engert (:KaiE:)]

from bug 294660 comment 9 

The only thing we could do would be to do something about the
silent failure problem of the fix for bug 249004.  That is,
if the cert we want to import is not valid, PSM right now
silently ignores it.  It would be nice to pop up an error
dialog in that case.

Comment 1

[Wan-Teh Chang]

Note that sometimes NSS or NSS-based apps import certs
automatically.  For example, Thunderbird automatically
imports the email encryption cert of the sender.  In
those cases, if the cert is invalid, we still want to
ignore it silently.

Comment 2

[Hermann Schwab]

*** Bug 310444 has been marked as a duplicate of this bug. ***

Comment 3

[Simon Fraser [no longer active]]

Related to bug 308857, I think?

Comment 4

[Kai Engert (:KaiE:)]

I've produced a patch, that:

- implements a new UI to display a generic message box, having a message and a
"view certificate button" and an OK button

- implements the feedback message suggested in bug 298045

- enhances the "CA certificate exists" message as suggested in bug 306290

- adds user feedback as suggested in bug 308857. Actually, when trying to
install a personal certificate, in addition to a success message, it will also
display an error message, if the user does not own the corresponding private key.

- implements the user feedback as suggested in this bug 310446

However, I just realized Wan-Teh's comment 1 in this bug.

Wan-Teh, do you suggest to:
- always give user feedback when ignoring CA certs?
- never give user feedback, when importing email certs?

Or do you suggest to make the user feedback, on ignoring an incoming email
certificiate, dependent on the context, that triggered the import attempt?

Comment 5

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 6

[Wan-Teh Chang]

Kai, yes, I suggest to base this decision on whether
the import is automatic or user-initiated.

If the user has to click some button or link to import
an email cert, we should give user feedback.  On the
other hand, Thunderbird automatically imports the
email cert of the sender, and we should not give
user feedback in this case.

Comment 7

[Wan-Teh Chang]

Kai, remove the "yes" from my previous comment when
you read it.

Comment 8

[Kai Engert (:KaiE:)]

Comment on attachment 198062 [details] [diff] [review]
Patch v1

Feel free to have a first look on this patch, and give more feedback. I'm
especially interested to hear suggestions on the new wordings. Thanks.

Comment 9

[Simon Fraser [no longer active]]

Comment on attachment 198062 [details] [diff] [review]
Patch v1

> Index: security/manager/locales/en-US/chrome/pipnss/pipnss.properties
> ===================================================================
> RCS file: /cvsroot/mozilla/security/manager/locales/en-US/chrome/pipnss/pipnss.properties,v
> retrieving revision 1.1
> diff -u -u -r1.1 pipnss.properties
> --- security/manager/locales/en-US/chrome/pipnss/pipnss.properties	14 Mar 2005 10:00:54 -0000	1.1
> +++ security/manager/locales/en-US/chrome/pipnss/pipnss.properties	30 Sep 2005 22:04:18 -0000
> @@ -284,3 +284,8 @@
>  VerifyUnknown=<Unknown>
>  CertNoNickname=(no nickname)
>  CertNoEmailAddress=(no email address)
> +CaCertExists=The Certificate already exists.
> +NotACACert=The downloaded certificate is not a CA certificate

Most users will have no idea what this means. Say what the user did that caused
them to get into this state, and what the result will be:
"The certificate that you just downloaded is not a "Certificate Authority" (CA)
certificate, so it will not be installed." or something. Maybe say that the
implications are of it not being a CA cert ("... so it cannot be used to
verify other certificates...").

> +NotImportingUnverifiedCert=A downloaded certificate will not be imported, but ignored, because it can not be verified.

Can you tell they why it could not be verified, and what they might do about
it?

> +UserCertIgnoredNoPrivateKey=A downloaded user certificate will not be imported, because you do not own its private key.

What's a private key (to the average user)? Maybe better as something like

"The certificate that you downloaded will not be imported because [Firefox]
cannot verify that this certificate was created by you (you do not own its
private key).

> +UserCertImported=You successfully installed a new personal certificate. Suggestion: It is a good idea to backup your new certificate now

In general, the Mac OS X-style 2-string dialogs give much more room for
explanations about why something went wrong, and what to do to fix it
(see below).

> Index: security/manager/pki/resources/content/alertWithCert.xul
> ===================================================================

> +<dialog id="alertWithCert" title="&alertWithCert.title;"
> +  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"      
> +  style="width: 32em;"
> +  spacerflex="1"
> +#expand buttons="accept,extra2"
> +  buttonlabelextra2="&examineCert.label;"
> +  buttonaccesskeyextra2="&examineCert.accesskey;"
> +  ondialogaccept="return doOK();"
> +  ondialogextra2="viewCert();"
> +  onload="onLoad();">
> +
> +<script type="application/x-javascript" src="chrome://global/content/strres.js"/>
> +<script type="application/x-javascript" src="chrome://pippki/content/pippki.js"/>
> +<script type="application/x-javascript" src="chrome://pippki/content/alertWithCert.js"/>
> +
> +  <description id="message1"/>
> +
> +</dialog>

I'd love to see some custom view view that we could use to display
certificates inline. Look at Camino, or Keychain in Mac OS X for examples.
This dialog could then default to having the cert hidden, but with a button
to reveal the cert. Fewer modal dialogs is better.


> Index: security/manager/ssl/public/nsICertificateDialogs2.idl
> ===================================================================

> +/**
> + * Functions that implement user interface dialogs to manage certificates.
> + */
> +[scriptable, uuid(7ef8b110-d414-4308-a962-e11214c38079)]
> +interface nsICertificateDialogs2 : nsISupports
> +{
> +  /**
> +   *  Generic UI to display an alert message
> +   *  and optionally allow the user to view more details about the given cert
> +   *
> +   *  @param ctx A user interface context.
> +   */
> +  void showAlertWithCertificate(in nsIInterfaceRequestor ctx,
> +                               in wstring message,
> +                               in nsIX509Cert cert);
> +};

Please use AString for the param.
Should this API be extended to allow for Confirm (OK/Cancel) dialogs,
with a return value? Also, what about supplying the button text?

As an embedder, I'd also prefer to be able to supply my own strings.
Alerts on Mac OS X usually have 2 strings: a simple message string,
then a longer explanation string that allows a much more user-friendly
message. I'd like to be able to do this, so either need 2 strings in
this method, or just an enum value that tells me what kind of error this is.

Comment 10

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #9)
> (From update of attachment 198062 [details] [diff] [review] [edit])
> As an embedder, I'd also prefer to be able to supply my own strings.
> Alerts on Mac OS X usually have 2 strings: a simple message string,
> then a longer explanation string that allows a much more user-friendly
> message. I'd like to be able to do this, so either need 2 strings in
> this method, or just an enum value that tells me what kind of error this is.

Speaking as another embedder, the same applies for GNOME; and since I want to
supply my own strings no matter what strings mozilla uses (for UI consistency
and i18n reasons), I'd vastly prefer to have an enum value in lieu of strings.

Comment 11

[Kai Engert (:KaiE:)]

*** Bug 338615 has been marked as a duplicate of this bug. ***

Comment 12

[Kai Engert (:KaiE:)]

Simon, this is a reply to your comments from 8 months ago.

(In reply to comment #9)
> > +NotACACert=The downloaded certificate is not a CA certificate
> 
> Most users will have no idea what this means. Say what the user did that caused
> them to get into this state, and what the result will be:
> "The certificate that you just downloaded is not a "Certificate Authority" (CA)
> certificate, so it will not be installed." or something. Maybe say that the
> implications are of it not being a CA cert ("... so it cannot be used to
> verify other certificates...").

I changed that to:
NotACACert=You are trying to install a certificate that is not a Certificate Authority (CA) certificate. It can not be used to verify other certificates. It will not be installed.


> > +NotImportingUnverifiedCert=A downloaded certificate will not be imported, but ignored, because it can not be verified.
> 
> Can you tell they why it could not be verified, and what they might do about
> it?

I would like to get started and at least provide some feedback, even if it's not yet precise enough.
New proposal:

NotImportingUnverifiedCert=You are trying to install a certificate that can not be verified for the desired purpose. The issuer might be unknown, the issuer might not be trusted by you, the certificate issuer might not have approved it for the desired purpose, the certificate might have expired or it might have been revoked, The certificate will not be installed.

> > +UserCertIgnoredNoPrivateKey=A downloaded user certificate will not be imported, because you do not own its private key.
> 
> What's a private key (to the average user)? Maybe better as something like
> 
> "The certificate that you downloaded will not be imported because [Firefox]
> cannot verify that this certificate was created by you (you do not own its
> private key).

Good idea in general, but we should say "requested".
And I'd like to avoid to mention the application name.
New proposal:

UserCertIgnoredNoPrivateKey=You are trying to install a personal certificate. It can not be installed, because you do not own the corresponding private key. Please use the computer that you used when you requested this certificate. 

> > +UserCertImported=You successfully installed a new personal certificate. Suggestion: It is a good idea to backup your new certificate now

I tweaked this to say:
UserCertImported=You successfully installed a new personal certificate. Suggestion: It is a good idea to backup your certificate.


> > Index: security/manager/pki/resources/content/alertWithCert.xul
> > ===================================================================
> 
> I'd love to see some custom view view that we could use to display
> certificates inline. Look at Camino, or Keychain in Mac OS X for examples.
> This dialog could then default to having the cert hidden, but with a button
> to reveal the cert. Fewer modal dialogs is better.

If you are willing to invest the additional time to get this going, patches are mostly welcome. But I'd like to start by giving at least some feedback.


> > Index: security/manager/ssl/public/nsICertificateDialogs2.idl
> > ===================================================================
> 
> > +  void showAlertWithCertificate(in nsIInterfaceRequestor ctx,
> > +                               in wstring message,
> > +                               in nsIX509Cert cert);
> 
> Should this API be extended to allow for Confirm (OK/Cancel) dialogs,
> with a return value? Also, what about supplying the button text?

If we need it, we can add more functions to that interface later.
The function is named "alert", we can add "query" functions at a later time.


> As an embedder, I'd also prefer to be able to supply my own strings.
> Alerts on Mac OS X usually have 2 strings: a simple message string,
> then a longer explanation string that allows a much more user-friendly
> message. I'd like to be able to do this, so either need 2 strings in
> this method, or just an enum value that tells me what kind of error this is.

I'm ok to switch to the enum proposal, will do so in the next patch.

Comment 13

[Simon Fraser [no longer active]]

(In reply to comment #12)
> Simon, this is a reply to your comments from 8 months ago.

thanks for getting to this!

> UserCertIgnoredNoPrivateKey=You are trying to install a personal certificate.
> It can not be installed, because you do not own the corresponding private key.
> Please use the computer that you used when you requested this certificate. 

I think assuming that they don't have the private key because they are on a
different computer is going to be wrong in many cases. Maybe they reformatted
their hard drive, or are using a different profile etc etc. Or maybe this cert
doesn't even belong to them.

Comment 14

[Kai Engert (:KaiE:)]

*** Bug 338424 has been marked as a duplicate of this bug. ***

Comment 15

[Kai Engert (:KaiE:)]

In order to satisfy your request, allowing an embeddor to override the warnings, we'll need a new IDL API, and I'm fine to use enum values.

However, we are too late in the game for adding that to FF 2 / TB 2, because adding IDL is not allowed after alphas.

On the other hand, I need to work on some feedback as a blocker, bug 176507.

Therefore I'd like to start things by hardcoding the feedback message within the non-overridable code for FF 2.

Because of the trunk-bake-rule-prior-to-branch, I want to start on the trunk doing the same.

We can convert to the overridable API after things have landed on the branch.

Comment 16

[Kai Engert (:KaiE:)]

*** Bug 331336 has been marked as a duplicate of this bug. ***

Comment 17

[Kai Engert (:KaiE:)]

*** Bug 298876 has been marked as a duplicate of this bug. ***

Comment 18

[Kai Engert (:KaiE:)]

(In reply to comment #13)
> > UserCertIgnoredNoPrivateKey=You are trying to install a personal certificate.
> > It can not be installed, because you do not own the corresponding private key.
> > Please use the computer that you used when you requested this certificate. 
> 
> I think assuming that they don't have the private key because they are on a
> different computer is going to be wrong in many cases. Maybe they reformatted
> their hard drive, or are using a different profile etc etc. Or maybe this cert
> doesn't even belong to them.

Good point, i changed it to:

UserCertIgnoredNoPrivateKey=You are trying to install a personal certificate. It can not be installed, because you do not own the corresponding private key. It got created when you or somebody else requested this certificate.

Comment 19

[Kai Engert (:KaiE:)]

Attachment: patch v2

Comment 20

[Kai Engert (:KaiE:)]

As suggested earlier, this patch should go to 1.8 branch, to fix blocker bug 176507. 

Once it has landed, I'll attach the remainder of the proposed changes (cert button + strings by enum, trunk only)

Comment 21

[Robert Relyea]

Comment on attachment 225921 [details] [diff] [review]
patch v2

r+

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 225921 [details] [diff] [review]
patch v2

Kai requested a string review, here's some comments from your friendly UI guy, hopefully not making too many assumptions about the purpose of these error messages.

(note: we should be giving users the information they need without trying to sound too formal, restrictive or technical; the software is their servant and friend, not their master)

>+CaCertExists=You are trying to install a certificate authority certificate, but it is already installed.

+CaCertExists=This certificate is already installed as a certificate authority.

>+NotACACert=You are trying to install a certificate that is not a Certificate Authority (CA) certificate. It can not be used to verify other certificates. It will not be installed.

+NotACACert=This is not a certificate authority certificate, so it can't be imported into the certificate authority list.

>+NotImportingUnverifiedCert=You are trying to install a certificate that can not be verified for the desired purpose. The issuer might be unknown, the issuer might not be trusted by you, the certificate issuer might not have approved it for the desired purpose, the certificate might have expired or it might have been revoked. The certificate will not be installed.

+NotImportingUnverifiedCert=This certificate can't be verified and will not be imported. The certificate issuer might be unknown or untrusted, the certificate might have expired or been revoked, or the certificate might not not have been approved.

>+UserCertIgnoredNoPrivateKey=You are trying to install a personal certificate. It can not be installed, because you do not own the corresponding private key. It got created when you or somebody else requested this certificate.

+UserCertIgnoredNoPrivateKey=This personal certificate can't be installed because you do not own the corresponding private key which was created when the certificate was requested.

>+UserCertImported=You successfully installed a new personal certificate. Suggestion: It is a good idea to backup your certificate.

+UserCertImported=Your personal certificate has been installed. You should keep a backup copy of this certificate copy.

(it would be really nice if we had a button that would do this for the user!)

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #22)
> +UserCertImported=Your personal certificate has been installed. You should keep
> a backup copy of this certificate copy.

typo:

+UserCertImported=Your personal certificate has been installed. You should keep a backup copy of this certificate.

Comment 24

[Simon Fraser [no longer active]]

(In reply to comment #22)

> +NotACACert=This is not a certificate authority certificate, so it can't be
> imported into the certificate authority list.

Can we word this without the repetition, or maybe capitalize Certificate Authority? Perhaps: "This certificate does not represent a Certificate Authority...".


> +NotImportingUnverifiedCert=This certificate can't be verified and will not be
> imported. The certificate issuer might be unknown or untrusted, the certificate
> might have expired or been revoked, or the certificate might not not have been
> approved.

We should strive to tell them which of these problems it actually is (code changes probably required).

Comment 25

[Kai Engert (:KaiE:)]

(In reply to comment #24)
> 
> > +NotACACert=This is not a certificate authority certificate, so it can't be
> > imported into the certificate authority list.
> 
> Can we word this without the repetition, or maybe capitalize Certificate
> Authority? Perhaps: "This certificate does not represent a Certificate
> Authority...".

I like the proposed wording, it makes it very clear why the cert can't be imported, because it might be ok to import it in a different context.
Our code already contains a mix of all-lowercase and capitalized. So I would like to go with all lowercase, as Mike used in both strings.

Comment 26

[Kai Engert (:KaiE:)]

Attachment: Patch v3

New patch, having Mike's wording, and one double-"not"-type removed.

Carrying forward r=rrelyea

Comment 27

[Kai Engert (:KaiE:)]

Patch checked in to trunk. I filed bug 342761 for the remaining work.

Comment 28

[Kai Engert (:KaiE:)]

Comment on attachment 227102 [details] [diff] [review]
Patch v3

This patch fixes 1.8.1 blocker bug 176507.

Comment 29

[Darin Fisher]

Comment on attachment 227102 [details] [diff] [review]
Patch v3

a=darin on behalf of drivers

Comment 30

[Kai Engert (:KaiE:)]

Attachment: Patch v3 as checked in to 1.8 branch

Adjusted version of patch v3 that applies cleanly to the 1.8 branch.

Comment 31

[Kai Engert (:KaiE:)]

There was a mistake in Patch v3. It caused regression bug 360528.

Comment 32

[Magnus Melin [:mkmelin]]

*** Bug 361656 has been marked as a duplicate of this bug. ***

Comment 33

[Ralf Hauser]

Another one where no error message/user feedback is given:
As anticipated in bug 216123 comment 1 , bouncy castle as of v1.34 now uses UTF8String when generating Issuer names. So, if you have an older Root certificate created with Subject DN as PrintableString and a new leaf cert generated by BC, TB is not going to import it because it does not find the issuer.