PSM error messages leave a lot to be desired

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
18 years ago
3 years ago

People

(Reporter: nelson, Unassigned)

Tracking

(Depends on 7 bugs, Blocks 1 bug, {ue})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [kerh-coa][psm-feedback])

Attachments

(10 attachments, 22 obsolete attachments)

22.14 KB, patch
blizzard
: superreview+
Details | Diff | Splinter Review
99.81 KB, image/png
Details
1003 bytes, patch
stuart.morgan+bugzilla
: review+
Details | Diff | Splinter Review
19.30 KB, patch
kaie
: review+
Details | Diff | Splinter Review
100.40 KB, patch
kaie
: review+
Details | Diff | Splinter Review
56.91 KB, image/png
Details
1.87 KB, patch
mconnor
: review+
Details | Diff | Splinter Review
3.20 KB, patch
neil
: review+
stuart.morgan+bugzilla
: review+
mconnor
: superreview+
Details | Diff | Splinter Review
5.25 KB, patch
Details | Diff | Splinter Review
1.27 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
joop@fokus.gmd.de (Robert Joop) wrote that he gets the following error 
messages:

> with mozilla 0.9.5 i get "an unknown SSL error (-8101)",

> i get
> "You cannot connect to <FQDN> because of an unknown SSL error (-12269)"
> when i use an expired client certificate.

> i get
> "You cannot connect to <FQDN> because of an unknown SSL error (-12227)"
> when i press cancel when asked for a client certificate.

Since error strings are defined for ALL of SSL, NSS, and NSPR errors, 
these dialogs ought to report error strings.  

(Yes, I know, it is additional localization burden, but we should still 
do it, IMO.)

Comment 1

18 years ago
Agreed in principle.
Rangan, we may want to store the strings in the resource file with the name of
the string the error code (or some basic variant of that.)
Assignee: ssaux → rangansen
Priority: -- → P2
Target Milestone: --- → 2.2

Comment 2

18 years ago
*** Bug 108626 has been marked as a duplicate of this bug. ***

Comment 3

18 years ago
adding useful comments from the dupe 108626

In nsNSSIOLayer::nsHandleSSLError() we only handle a few of the errors defined
in sslerr.h

Some of the errors we should clearly handle are:
 74 SSL_ERROR_REVOKED_CERT_ALERT            = (SSL_ERROR_BASE + 18),
 75 SSL_ERROR_EXPIRED_CERT_ALERT            = (SSL_ERROR_BASE + 19),

So that we can do correct validation when doing client auth.
See NSS bug 108250.



Updated

18 years ago
Status: NEW → ASSIGNED

Comment 4

18 years ago
Posted patch patch (obsolete) — Splinter Review
This patch should make error handling during ssl connection finer grained.
The idea is to handle all the error codes that might occur here [switch - case]
but try to optimize the localization overhead by grouping some of the cases
[where possible, without losing relevance] to use the same error string [for
example errors like malformed response/bad response/unknown response
type/status from ocsp server are combined into a single "unknown or malformed
response from ocsp server" kind of message. But for 'unknown cert' error from
ocsp server, we should have an unique error message]. Also, whenever more that
one error cases map into one message, the error code is provided with the
message to ensure no loss of info.
The default: case is still there, but ideally, we should never run into it.

Comment 5

18 years ago
Comment on attachment 64028 [details] [diff] [review]
patch

r=javi for the code.  Someone else should review the text and make sure it's
appropriate and "user-friendly"
Attachment #64028 - Flags: review+

Comment 6

18 years ago
cc-ing Sean...
Sean, Could you pl review the text ...

Comment 7

18 years ago
>Could not establish a secure connection.

In general, we've been trying to avoid "secure" in the interface, in favor of
words like "encrypted" or "authenticated". So I would prefer to see this phrase
replaced with one of the following, depending what's actually going on:

- Could not establish an encrypted connection. (Authentication is implied in
this case. Most users won't know that, but I think just "encrypted" is enough here.)
- Could not establish an authenticated connection. (when encryption is not involved)
- Could not establish an encrypted and/or authenticated connection. (if there
are cases where you don't know exactly)


>There is a problem with your database [Error Code: %S]. Contact your system
admistrator.

Should we specify "certificate database" if that is the case here? Might help
the system admin to know.


>%S has received an incorrect or enexpected message

Change to this:

%S has received an incorrect or unexpected message.


In general, I think we should include the abbreviation "CRL" in error messages
about CRLs, since that's how they're often referred to in the interface and
elsewhere:

>Certificate Revocation List from the CA certifying %S is past its next update
date. Please update it.

Change to this, for clarity:

Certificate Revocation List (CRL) from the CA certifying %S is past its Next
Update date. Please update the CRL.


>Certificate Revocation List from the CA certifying %S is yet to be valid.
Please check in system clock.

Change to this, for clarity:

>Certificate Revocation List (CRL) from the CA certifying %S is not yet valid.
Please check your system clock.


>Certificate Revocation List from the CA certifying %S has an invalid signature.

Change to this:

Certificate Revocation List (CRL) from the CA certifying %S has an invalid
digital signature.


>Certificate Revocation List from the CA certifying %S is not valid.

Change to this:

Certificate Revocation List (CRL) from the CA certifying %S is not valid.


>Error trying to validate Certificate from %S using OSCP

For all errors that include this phrase, lowercase "certificate". Also for
these, "Unauthorized" should be one word without a hyphen. Also, it's not clear
to me what Future Reponse and Old Response mean. Is there some better way of
describing these?

Comment 8

18 years ago
Posted patch updated patch (obsolete) — Splinter Review
New patch - addresses cotters comments.
Several comments on the patch of 01/14.

1. The patch contains two separate cases for case SEC_ERROR_EXPIRED_CERTIFICATE.
The first such case label immediately follows case SSL_ERROR_BASE_CERT_DOMAIN.
Just delete that particular case label.

2. SSL_ERROR_CLOSE_NOTIFY_ALERT is not a "connection reset".  The message 
"The connection was reset by <peer>" is misleading in this case.  It would
be better to say "<peer> has closed the connection."

3. All the "Contact your system administrator." strings might as well say 
"Say yer prayers, varmint."  Home users (who are their own system admins)
find that unhelpful, and most corporate sysadmins aren't much help with 
bad mac or corrupt DB errors.  For DB problems, it might be better to direct 
the user to a URL on netscape.com that explains the problem and suggests 
solutions.  For bad MAC errors, it might be best to contact the _servers_
administrator.  

Comment 10

18 years ago
Updated with Nelson's comments.
For Comment#3, in case of db problems, removed the "contact your System
Administrator" string for now - till I can find which url to direct the user to
in such cases.

Does anyone have a suggestion on this url should be?
+  char buf[80];

80 seems a bit excessive to me but it probably won't harm anything.

Other than that, looks fine to me.  sr=blizzard
Comment on attachment 64916 [details] [diff] [review]
update [checked in]

sr=blizzard
Attachment #64916 - Flags: superreview+

Comment 13

18 years ago
Patch checked in. Note that this patch handles only ssl/security errors [not
nspr errors - any nspr errors would still go to the default handler]
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

Comment 14

18 years ago
*** Bug 117412 has been marked as a duplicate of this bug. ***

Comment 15

18 years ago
Verified.
Status: RESOLVED → VERIFIED
If this bug is fixed, then why do we continue to receive reports about 
-8101 errors??

Comment 17

17 years ago
This patch handles the error cases during a ssl connection, and -8101 has been 
handled. It would be pretty wiered if this still shows up - does is it happen 
during a ssl connection setup or somewhere else?

Comment 18

17 years ago
You can still see -8101 when visiting this site, which is probably missing part 
of the Verisign chain. https://www.cu-webssl.net
Users report that N 7.0 displays the error message
'Could not establish an encrypted connection because certificate presented by 
www.cu-webssl.net is invalid or corrupted. Error Code:-8101'.
-8101 is SEC_ERROR_INADEQUATE_CERT_TYPE and the error string for it is 
"Certificate type not approved for application."
I am reopening this bug.  The changes made previously for this bug probably
improved things by producing better errors in some limited situations and
for some error codes, but did not really eliminate the problem.

PSM's error reporting STILL leaves a lot to be desired.  There are still 
way too many messages that boil down to "it didn't work: -<number>"  
and too many that say "it failed because of an unknown error", even
though the error code that triggered them was distinctly meaningful.

One has merely to search bugzilla for bugs about error messages with the 
strings -81 or -122 to see how common this is.  
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Depends on: 202283
Depends on: 139196
Depends on: 183371
*** Bug 202283 has been marked as a duplicate of this bug. ***
*** Bug 224307 has been marked as a duplicate of this bug. ***
Assignee: rangansen → kaie
Status: REOPENED → NEW
QA Contact: junruh → bmartin

Updated

16 years ago
Depends on: 172051
*** Bug 271834 has been marked as a duplicate of this bug. ***
Just to be clear about something: This bug isn't *BLOCKED* by anything.
It's a simple matter of having better error messsages for the existing 
error codes.  No changes to any other code are necessary for this to 
be fixed.  The error codes are documented in various pages, including
http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslerr.html
This bug just needs someone to work on PSM.
Summary: PSM error messages leave a little to be desired → PSM error messages leave a lot to be desired
*** Bug 278499 has been marked as a duplicate of this bug. ***

Comment 26

15 years ago
Another one, it is particularly cumbersome if firebird doesn't work with the
main SSL cert issuer of the german part of the world.

May well be that they are at fault, but if firebird neither helps them nor me
with a useful error message, I am forced to open MSIE and might stick to that  
:(

Updated

15 years ago
Product: PSM → Core
*** Bug 301006 has been marked as a duplicate of this bug. ***
OS: Linux → All
Hardware: PC → All

Updated

14 years ago
Whiteboard: [kerh-coa]
I want to start by looking at error messages that are produced when there is a failure during network activity.

For whatever reasons, the original authors of error reporting did not make an attempt to handle all existing error messages. They chose a subset. In addition, they did not simply use the error messages mentioned by Nelson, but used other messages, allowed reviewers to change the error message. Different error codes have been grouped to show the same error message. Some error messages are plain text, other's dynamically add the peer's host name, many add the numerical error message.

There is no consistency, and it is a mess.

I don't feel like looking at each and every error code and discuss whether the error message is appropriate.

I really would prefer to replace the existing code with something new, for all error codes, consistently showing the error message as defined in NSS documentation, adding both hostname and error number.

Such a change will be simple to do and I can easily produce such new code from the documentation.

But this has the disadvantage of losing any error message improvements that might have been done for some particular error codes.
Posted patch Patch v4 (obsolete) — Splinter Review
This patch illustrates a new approach that I'm proposing in bug 329017.

It has an error message for each NSPR/NSS error code.
Please read bug 329017 for the background explanation.

This patch replaces existing error message with those contained in the NSS library. We might want to merge the messages, removed by this patch, into the NSS files contained in directory mozilla/security/nss/cmd/lib/

Updated

14 years ago
Attachment #64028 - Attachment is obsolete: true

Updated

14 years ago
Attachment #64885 - Attachment is obsolete: true

Updated

14 years ago
Attachment #64916 - Attachment description: update → update [checked in]
Is improving the wording of the messages this bug, or another bug?
(In reply to comment #31)
> Is improving the wording of the messages this bug, or another bug? 

I propose we start by ensuring that we have at least some appropriate error message for each error code in PSM.

I propose we automatically include the wordings located at some master location. I hope the discussion in bug 329017 will bring us a decision, where that master location will be.

Once we see clear on that, I propose we use a separate bug to improve the wordings.
5 year old PSM bug.  Festering.
Depends on: unknownreason
Depends on: 155908
Depends on: 183990
Depends on: 217305
Depends on: 217721
Depends on: 265873
Depends on: 289988
Depends on: 292840
Depends on: 300071
Depends on: 328030
Depends on: 335873
Depends on: 335874

Updated

13 years ago
No longer depends on: 335873

Updated

13 years ago
Priority: P2 → P1
Target Milestone: psm2.2 → mozilla1.9alpha

Updated

13 years ago
Depends on: 329017
Depends on: 335859
Depends on: 333807
Depends on: 255140

Comment 34

13 years ago
Comment on attachment 213633 [details] [diff] [review]
Patch v4

Are there common fields?

could we define %1$s to mean <server> and %2$s to mean <brand> and so on?

i.e. each replacement string would have a fixed position, for any value we don't have we fill it in with "**localization error**" all numbers are passed as strings.

that would enable the messages to be slightly flexible.

otherwise, let's just get this into cvs and improve it later (in fact my suggestion above could be done later).

Comment 35

13 years ago
Comment on attachment 213633 [details] [diff] [review]
Patch v4

Are there common fields?

could we define %1$s to mean <server> and %2$s to mean <brand> and so on?

i.e. each replacement string would have a fixed position, for any value we don't have we fill it in with "**localization error**" all numbers are passed as strings.

that would enable the messages to be slightly flexible.

otherwise, let's just get this into cvs and improve it later (in fact my suggestion above could be done later).
Comment on attachment 213633 [details] [diff] [review]
Patch v4

>+SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION_8151=Certificate contains unknown critical extension.

I think this patch is on the right track.  I think it would be the start of 
a big improvement.

I do NOT think that PSM should delay committing this change merely because 
this new table of error messages cannot be automatically (re)generated from 
NSS sources.  

Let automated (re)generation be phase 2, the second step, the NEXT 
improvement.  But get the new error codes in now.  Please!

I would also suggest NOT putting the error numbers into the name strings.  
e.g. SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION_8151 might better be just
     SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION or even 
     SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION_STRING (to avoid duplication).
Alias: desired
Depends on: 309585
Depends on: 238142
Depends on: 261003
Depends on: 353937
One reason why I delayed this patch: I wasn't sure whether we really want to overwrite (and remove) existing application defined error strings.

In addition to the error message table automatically generated from NSS code, I believe we should allow the application to override these strings - in case the NSS error message is too geeky for the application owners.

I propose PSM does two string lookups. It should first try whether an application defined string is available. If that fails, it will try to use the NSS error message.

I want to go through my patch and look at the existing error message this patch removes. I will try to keep better error strings.

I want to keep the error reporting as general as possible. I want to avoid host names and brand strings in the middle of those strings, because I want to avoid manual tweaking of strings. This is the motivation for displaying them separately.

There may be error codes that are not related to a hostname. I want to check that at runtime, and only display the hostname section if one is related.

Will work on that now, I really hope I can work on this til completion now.

Updated

13 years ago
Attachment #213633 - Attachment is obsolete: true

Updated

13 years ago
Attachment #213635 - Attachment is obsolete: true
Posted patch Patch v5 (obsolete) — Splinter Review
New patch.

It makes the error strings defined by NSS available to the application. Currently we use it during SSL connections only - but in the future can leverage them for error messages at other situations.

It keeps all current strings explicitly defined by PSM. Only if no string is defined by PSM, it will use the NSS default error string. (In a second phase we should verify for each override which string is better)

Whenever an SSL error occurs, we will always display:
- the host name the problem occurred with
- the numeric error code
- either the PSM string or the NSS string
Posted image v5 screenshot, sample 1, OLD style (obsolete) —
Posted image v5 screenshot, sample 1, NEW style (obsolete) —
Posted image v5 screenshot, sample 2, OLD style (obsolete) —
Posted image v5 screenshot, sample 2, NEW style (obsolete) —
Depends on: 355643
Comment on attachment 241409 [details] [diff] [review]
Patch v5

+SSL_ERROR_RX_UNEXPECTED_CLIENT_KEY_EXCH=SSL received an unexpected Cllient Key Exchange handshake message.

typo: Cllient

Comment 44

13 years ago
*** Bug 357711 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Attachment #241409 - Flags: review?(rrelyea)
(In reply to comment #43)
> typo: Cllient

Thanks, I filed bug 359280 to get this fixed in NSS.
Depends on: 359280
Comment on attachment 241409 [details] [diff] [review]
Patch v5

Obsoleting patch, we want some additional tweaks.
Attachment #241409 - Attachment is obsolete: true
Attachment #241409 - Flags: review?(rrelyea)
Posted patch Patch v6 (obsolete) — Splinter Review
Attachment #241412 - Attachment is obsolete: true
Attachment #241413 - Attachment is obsolete: true
Attachment #241414 - Attachment is obsolete: true
Attachment #241415 - Attachment is obsolete: true
Comment on attachment 247181 [details] [diff] [review]
Patch v6

With this Patch v6, fatal SSL errors that occur in the context of a browser will now be shown as an error page. I'll attach a screen shot next.
In all other contexts, where no browser window is available, we'll continue to show the error as a dialog alert.
Attachment #247181 - Attachment description: Patch v6 - show SSL error pages in browser window - show dialogs for all other contexts like mail → Patch v6
Posted image error page screenshot for patch v6 (obsolete) —
Posted patch Patch v6 fixed (obsolete) — Splinter Review
there was a variable init missing, fixed
Attachment #247181 - Attachment is obsolete: true
Depends on: 347644
Depends on: 350430
Depends on: 363152
Depends on: 363161
Blocks: 360052
Depends on: 364428
Blocks: 367066

Updated

13 years ago
Depends on: 281287
Comment on attachment 247182 [details]
error page screenshot for patch v6

This is obsolete, because it uses a negative error code, that the UI owners don't like. This was also an argument for postponing this patch, because I didn't have a better solution.

But now I believe I have an idea that is acceptable to everybody. Instead of appending the numeric code, let's add the error symbol used in the source code (e.g. SSL_ERROR_BAD_CERT_ALERT).
Attachment #247182 - Attachment is obsolete: true
Posted image Screen Shot for patch v7 (obsolete) —
Posted patch Patch v7 (obsolete) — Splinter Review
Attachment #247677 - Attachment is obsolete: true
Because the patch requires changes in multiple modules, I'm separating patch v7 into several attachments, so I can independently request reviews.

This portion contains the changes to the security module.
Attachment #252819 - Attachment is obsolete: true
Attachment #252821 - Flags: review?(rrelyea)
In order to integrate the new class of errors into the existing error page code, some changes are required in docshell and netwerk.

I hope these changes are ok. I would probably be good to avoid using -0x2000 and -0x3000 directly, but I haven't found a good solution yet to avoid them, because the numbers are defined in the optional NSS source code. The discussion in bug 362488 has not yet come to a solution.

Christian, could you please have a look?
Attachment #252822 - Flags: superreview?
Attachment #252822 - Flags: review?(cbiesinger)
Mike, would you be ok with this wording? And the screenshot attached in this bug? You probably remember our discussion about avoiding numeric error codes in the UI. I hope I have found a salomonic compromise, which uses the symbolic name of the error - this is ensure we will be able to make use the of error message, even if the main wording is in a foreign language.

Neil, the same wording will also be required in the dom directory, which will be used for SeaMonkey. Are you ok with adding that new string, and I propose we keep the dom and browser wordings in synch?
Attachment #252824 - Flags: superreview?
Attachment #252824 - Flags: review?(mconnor)
Comment on attachment 252824 [details] [diff] [review]
Patch v7 - netError.dtd in dom and browser

Neil, please see the previous comment.
Attachment #252824 - Flags: superreview? → superreview?(neil)

Updated

13 years ago
Attachment #252824 - Attachment is obsolete: true
Attachment #252824 - Flags: superreview?(neil)
Attachment #252824 - Flags: review?(mconnor)
Attachment #252825 - Flags: superreview?(neil)
Attachment #252825 - Flags: review?(mconnor)
Thanks for Stephen Donner who made me aware of a typo. Third attempt of attaching this patch :-)
Attachment #252825 - Attachment is obsolete: true
Attachment #252829 - Flags: superreview?(neil)
Attachment #252829 - Flags: review?(mconnor)
Attachment #252825 - Flags: superreview?(neil)
Attachment #252825 - Flags: review?(mconnor)
Posted image Screen Shot for patch v7 (obsolete) —
Updated screenshot to correct typo.
Attachment #252818 - Attachment is obsolete: true

Updated

13 years ago
Attachment #252830 - Flags: review?(sfraser_bugs) → review?(smorgan)

Updated

13 years ago
Attachment #252830 - Flags: review?(smorgan) → review?(stuart.morgan)
Comment on attachment 252821 [details] [diff] [review]
Patch v7 - mozilla/security/manager portion

>+static const char *
>+getDefaultErrorStringName(PRInt32 err)
>+{
>+  const char *id_str = nsnull;
>+
>+  switch (err)
>+  {
>+    case SSL_ERROR_EXPORT_ONLY_SERVER: id_str = "SSL_ERROR_EXPORT_ONLY_SERVER"; break;
Alternatively could you list the error numbers in the .properties file? (nsIStringBundle has a formatStringFromID but it's just a wrapper that converts the numeric ID to a string).

>+      nsString defMsg;
>+      rv = component->GetPIPNSSBundleString(id_str, defMsg);
>+      if (NS_SUCCEEDED(rv))
>+      {
>+        returnedMessage.Append(defMsg);
>+        returnedMessage.Append(NS_LITERAL_STRING(" "));
>+      }
>+
>+      returnedMessage.Append(NS_LITERAL_STRING("\n("));
>+      returnedMessage.AppendASCII(nss_error_id_str);
>+      returnedMessage.Append(NS_LITERAL_STRING(")\n"));
Is this i18n-safe? Also are you allowed to use AppendLiteral(")\n"); etc?
Forget that, I just twigged you use the id string in the error message.
Although from an i18n point of view it might be better to embed the string in the message in which case you could format it by id anyway ;-)

Updated

13 years ago
Attachment #252829 - Flags: superreview?(neil) → superreview+

Updated

13 years ago
Attachment #252830 - Flags: review?(stuart.morgan) → review+

Updated

13 years ago
Attachment #252822 - Flags: superreview? → superreview?(bzbarsky)
Comment on attachment 252822 [details] [diff] [review]
Patch v7 - docshell and netwerk portions

Please add -p8 to your diff options?  It'll make your diffs a lot easier to review....

>Index: mozilla/docshell/base/nsDocShell.cpp

>+    else if (NS_ERROR_NSS_FAILURE == aError) {
>+        nsCOMPtr<nsISupports> securityInfo;
>+        aFailedChannel->GetSecurityInfo(getter_AddRefs(securityInfo));

You sure aFailedChannel can't be null here?

>+        if (securityInfo)
>+        {
>+            nsCOMPtr<nsITransportSecurityInfo> tsi(do_QueryInterface(securityInfo));
>+            if (tsi)
>+            {
>+                error.AssignLiteral("nssFailure");
>+                tsi->GetErrorMessage(getter_Copies(messageStr));
>+            }
>+        }

And if not, then what?  Should there be an error message that you use if you get no nsITransportSecurityInfo?

>Index: mozilla/netwerk/base/public/nsNSSError.h
>+ * Failure at NSS library level.
>+ */
>+#define NS_ERROR_NSS_FAILURE \
>+    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_SECURITY, 2)

We really can't subdivide this further to give useful error messages?

>Index: mozilla/netwerk/base/src/nsSocketTransport2.cpp
>+        if ((errorCode >= (-0x2000) && errorCode < ((-0x2000) + 1000))

I really dislike the magic numbers.  Is there anything we can do about that?

Rest looks ok; in general I think biesi can r+sr this stuff.
explanation follows
Attachment #252822 - Attachment is obsolete: true
Attachment #252822 - Flags: superreview?(bzbarsky)
Attachment #252822 - Flags: review?(cbiesinger)
explanation follows
Attachment #252821 - Attachment is obsolete: true
Attachment #252821 - Flags: review?(rrelyea)
(In reply to comment #65)
> (From update of attachment 252822 [details] [diff] [review])
> Please add -p8 to your diff options?  It'll make your diffs a lot easier to
> review....

ok


> >Index: mozilla/netwerk/base/public/nsNSSError.h
> >+ * Failure at NSS library level.
> >+ */
> >+#define NS_ERROR_NSS_FAILURE \
> >+    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_SECURITY, 2)
> 
> We really can't subdivide this further to give useful error messages?


Well, in the initial implementation it wasn't absolutely necessary, because I had intended to use other means to obtain the exact message.

But because of your other inspiring comments, I'm now using a different approach.

I removed the above error code and file.

We'll now use
  NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_SECURITY, <specific NSS error code>)


> >Index: mozilla/netwerk/base/src/nsSocketTransport2.cpp
> >+        if ((errorCode >= (-0x2000) && errorCode < ((-0x2000) + 1000))
> 
> I really dislike the magic numbers.  Is there anything we can do about that?


Ok, I solved it differently. I introduced a new service, provided by PSM, that can be used to answer the above question.

In addition I implemented a mechanism to query the (localized) error message for a given code.


> >Index: mozilla/docshell/base/nsDocShell.cpp
> 
> >+    else if (NS_ERROR_NSS_FAILURE == aError) {
> >+        nsCOMPtr<nsISupports> securityInfo;
> >+        aFailedChannel->GetSecurityInfo(getter_AddRefs(securityInfo));
> 
> You sure aFailedChannel can't be null here?


I suspect it is very likely we'll have the failed channel available - because those security errors can only get produced if we have actually started a connection.

However, I agree we should play safe and test for null. Changed in the patch.

This introduces the likelihood we need to obtain the error message from somewhere else - instead of the channel. Which is why I added the other function to the service, as explained above.


> >+        if (securityInfo)
> >+        {
> >+            nsCOMPtr<nsITransportSecurityInfo> tsi(do_QueryInterface(securityInfo));
> >+            if (tsi)
> >+            {
> >+                error.AssignLiteral("nssFailure");
> >+                tsi->GetErrorMessage(getter_Copies(messageStr));
> >+            }
> >+        }
> 
> And if not, then what?  Should there be an error message that you use if you
> get no nsITransportSecurityInfo?


Right. 

The new implementation is:
If channel is available, use prepared error message, that will contain the hostname the error is related to.
Else use the error message without host name.
Comment on attachment 253867 [details] [diff] [review]
Patch v8 - docshell and netwerk portions

(In reply to comment #65)
> Rest looks ok; in general I think biesi can r+sr this stuff.

Christian, could you please have a look?
Attachment #253867 - Flags: superreview?(cbiesinger)
Attachment #253867 - Flags: review?(cbiesinger)

Updated

13 years ago
Attachment #253868 - Flags: review?(rrelyea)

Comment 70

13 years ago
Comment on attachment 253868 [details] [diff] [review]
Patch v8 - mozilla/security/manager portion

r+

Some minor comments:

Your code that changes SSL error numbers to SSL Error names could be done with a table (see the sample code in mozilla/security/nss/cmd/lib in the Error code stuff. This table could be automatically generated from sslerr.h secerr.h.

At some point we should add the SEC errors as well (the current patch just has NSPR and SSL errors).

neither of these are serious enough to hold up the existing patch.

bob
Attachment #253868 - Flags: review?(rrelyea) → review+
(In reply to comment #70)
> Your code that changes SSL error numbers to SSL Error names could be done with
> a table (see the sample code in mozilla/security/nss/cmd/lib in the Error code
> stuff. This table could be automatically generated from sslerr.h secerr.h.

In fact, the code I'm adding is already automatically generated. The code I am using is in bug 329017.

However, the intention is to allow the strings to be localized. So it is necessary to put the error strings into a properties file.


> At some point we should add the SEC errors as well (the current patch just has
> NSPR and SSL errors).

Hmm, I think you overlooked them. They are there already.


> neither of these are serious enough to hold up the existing patch.

Great, thanks!
Comment on attachment 253867 [details] [diff] [review]
Patch v8 - docshell and netwerk portions

+++ mozilla/docshell/base/nsDocShell.cpp	3 Feb 2007 04:15:07 -0000
+        if (securityInfo)
+            tsi = do_QueryInterface(securityInfo);

do_QI is nullsafe... you could avoid this if

Have you considered using nsIErrorService? I.e. using the appropriate format for the .properties file and just calling registerErrorBundle in PSM. Docshell could then use nsIStringBundleService::FormatStatusMessage to get the message.

You'd still need GetXPCOMFromNSSError though...

+++ mozilla/netwerk/base/public/nsINSSErrorsService.idl	3 Feb 2007 04:15:07 -0000
+    void getXPCOMFromNSSError(in PRInt32 aNSPRCode,
+                              out nsresult aXPCOMErrorCode);

Please make this an actual return value, i.e.:
  nsresult getXPCOMFromNSSError(in PRInt32 aNSPRCode);

(and @return in the comment)

Similar for getErrorMessage, and that should also use AString instead of wstring

+#define NS_NSS_ERRORS_SERVICE_CONTRACTID "@mozilla.org/nss_errors_service;1"

contract IDs shouldn't live in interfaces generally, move this to nsNetCID.h.

+++ mozilla/netwerk/base/src/nsSocketTransport2.cpp	3 Feb 2007 04:15:07 -0000

+                nsresult nss_xpcom_code;
+                nsresult conversion_status =

hm, this file/module tends to use the nssXPCOMCode / conversionStatus style for variable names...

+                  nsserr->GetXPCOMFromNSSError(errorCode, &nss_xpcom_code);
+
+                if (NS_SUCCEEDED(conversion_status))
+                  rv = nss_xpcom_code;

please use 4-space indentation for consistency with the rest of the file

r+sr=me, though I'd like to see the new patch too if you change to nsIErrorService
Attachment #253867 - Flags: superreview?(cbiesinger)
Attachment #253867 - Flags: superreview+
Attachment #253867 - Flags: review?(cbiesinger)
Attachment #253867 - Flags: review+
(In reply to comment #72)

Christian, thanks a lot for your review.
I addressed all your required change requests.


> do_QI is nullsafe... you could avoid this if

"if" removed


> Please make this an actual return value, i.e.:
>   nsresult getXPCOMFromNSSError(in PRInt32 aNSPRCode);
> (and @return in the comment)

done


> Similar for getErrorMessage, and that should also use AString instead of
> wstring

done


> +#define NS_NSS_ERRORS_SERVICE_CONTRACTID "@mozilla.org/nss_errors_service;1"
> contract IDs shouldn't live in interfaces generally, move this to nsNetCID.h.

moved, and added a comment about the required interface


> hm, this file/module tends to use the nssXPCOMCode / conversionStatus style for
> variable names...

renamed both variables


> please use 4-space indentation for consistency with the rest of the file

done


> Have you considered using nsIErrorService? I.e. using the appropriate format
> for the .properties file and just calling registerErrorBundle in PSM. Docshell
> could then use nsIStringBundleService::FormatStatusMessage to get the message.
> You'd still need GetXPCOMFromNSSError though...

I did not (yet) address your optional suggestion to convert to nsIErrorService, because it would require another review cycle of the security code as well. Thanks for making this proposal optional at this time. I will look at it after getting this first cycle done.
v9 = Patch v8 after addressing Christian's minor change requests.
Attachment #253867 - Attachment is obsolete: true
Attachment #255036 - Flags: superreview+
Attachment #255036 - Flags: review+

Updated

13 years ago
Attachment #255036 - Attachment description: Patch v9 → Patch v9 - docshell and netwerk portions
Christian's request to change the string type in the interface required a minor syntax change in the implementation. No other changes in this new patch, I'm carrying forward r=rrelyea
Attachment #255037 - Flags: review+
Posted image Screen Shot v9
Here is an updated Firefox screenshot. This reflects the latest patches. The differences from v7 are:
- the error ID is now in lowercase, in order to be less intrusive
- added a message for Firefox only that points end users to the "report broken web site" feature available in the help menu.


Let me explain this screen shot in full detail, which can look differently depending on the actual failure occurred at the SSL level.


"Secure Connection Failed"

  general heading, always same


"An error occurred during a connection to %hostname%"

  general introduction, always same


"Received an incorrect or unexpected message."

  This is the exact explanation of the experienced error,
    as dynamically provided by the security library.
  Will be a localized string, i.e. when foreign end users
    send a screenshot, admins might be unable to translate.


(ssl_error_handshake_failure_alert)

  This is the technical error identifier.
    as dynamically provided by the security library.
  Will always be displayed in its original english form,
    which allows admins to understand the error reason 
    without having to translate.
  In the past this used to be the negative error number.
  As there has been controversy about using error numbers in the UI,
    this proposal it to add a human readable error identifier.


"The page you are trying to view can not be shown because the web site uses an invalid or unsupported security protocol.
Please contact the web site owners to inform them of this problem."

  This is a general wording that explains what is going on.
  Always the same


"Alternatively, use the command found in the help menu to report this broken site."

  This string is proposed for Firefox, only.
  It implements a recommendation that Mike Connor provided during a meeting.
  It points end users to the "Help / report broken web site"
    feature to report broken sites back to Mozilla.org


Mike, could you please review / approve this initial implementation?
Attachment #252831 - Attachment is obsolete: true
Attachment #255040 - Flags: review?(mconnor)
Comment on attachment 255040 [details]
Screen Shot v9

I meant to ask for review on the patch, not the screenshot.
Attachment #255040 - Flags: review?(mconnor)
Carrying forward Neil's sr on the seamonkey / dom portion.

Asking Mike to review for Firefox.
Attachment #252829 - Attachment is obsolete: true
Attachment #255041 - Flags: superreview+
Attachment #255041 - Flags: review?(mconnor)
Attachment #252829 - Flags: review?(mconnor)

Updated

13 years ago
Attachment #253868 - Attachment is obsolete: true

Updated

13 years ago
Attachment #255037 - Attachment is patch: true
Attachment #255037 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 255037 [details] [diff] [review]
Patch v9 - mozilla/security/manager portion [checked in]

99% of this patch is improving our error messages. This has been waiting to get landed very long. I think we should not delay this longer.

As explained earlier, this patch is able to report the error in the classic style, using a dialog (as we still require it for e.g. Thunderbird), and in the new style, as an error page.

I would like to land this patch now and hook up error page reporting later, once we get code review on the error page wording patch from the Firefox owners.
Comment on attachment 255036 [details] [diff] [review]
Patch v9 - docshell and netwerk portions [checked in]

This patch to docshell/webshell/netwerk can be safely divded into two separate portions.

Portion 1 is adding new interfaces that the crypto module implements. I would like to land this portion now, as it is required to land the error message improvements in the crypto code.

Portion 2 is adding and enabling the new error page, which is not yet approved, so I'm going to delay this portion.
Comment on attachment 255041 [details] [diff] [review]
Patch v9 - netError.dtd in dom and browser [checked in]

So, this is somewhat better, but I'm pretty sure the wording is not consistent with other error messages.

That said, its an improvement, and we're going to do a general string review/cleanup on trunk in a little while.

r=me on that basis.
Attachment #255041 - Flags: review?(mconnor) → review+

Updated

13 years ago
Blocks: 281287
No longer depends on: 281287

Updated

13 years ago
Attachment #255036 - Attachment description: Patch v9 - docshell and netwerk portions → Patch v9 - docshell and netwerk portions [checked in]

Updated

13 years ago
Attachment #255037 - Attachment description: Patch v9 - mozilla/security/manager portion → Patch v9 - mozilla/security/manager portion [checked in]

Updated

13 years ago
Attachment #252830 - Attachment description: Patch v7 - netError.dtd in camino → Patch v7 - netError.dtd in camino [checked in]
Comment on attachment 255041 [details] [diff] [review]
Patch v9 - netError.dtd in dom and browser [checked in]

Thanks for the review Mike. By now I've checked in all patches to the trunk.


The now completed work has been an important step on our journey to improve the error messages.

We now have strings available for all the errors that can happen.


The code we added enabled those new error wordings for SSL connections.


But there remains work to do. There are many other places in PSM where an error can occur. And these also should be changed to make use of the new wordings.

Because of that I'm going to leave this bug open.

I'm thinking we should convert this bug into a tracker bug, and discuss further improvement patches in separate bugs.
Attachment #255041 - Attachment description: Patch v9 - netError.dtd in dom and browser → Patch v9 - netError.dtd in dom and browser [checked in]

Comment 83

13 years ago
Comment on attachment 255037 [details] [diff] [review]
Patch v9 - mozilla/security/manager portion [checked in]

I think I found a couple of typos in the new error strings:

>Index: mozilla/security/manager/locales/en-US/chrome/pipnss/nsserrors.properties
>===================================================================

>+SSL_ERROR_RX_UNEXPECTED_CLIENT_KEY_EXCH=SSL received an unexpected Cllient Key Exchange handshake message.

Client Key Exchange

>+SSL_ERROR_DECOMPRESSION_FAILURE_ALERT=SSL peer was unable to succesfully decompress an SSL record it received.

successfully 

>+SEC_ERROR_UNRECOGNIZED_OID=Unrecognized Object IDentifier.

Is this capitalization correct?

Comment 84

13 years ago
I found another one :

>+SEC_ERROR_CRL_EXPIRED=The CRL for the certificate's issuer has expired.  Update it or check your system data and time.

date and time
Thanks for catchings these string errors. I filed bug 371024.
Typos fixed in both NSS and PSM on trunk.
A lot of people seem to think that the PSM error string work is already 
done, on the trunk.  They think that once the current trunk becomes a 
released version of firefox, all the error string troubles will have gone
away, and NSS's error strings will finally be displayed for all security
errors.  

But it ain't so.

I'm running a recent trunk build.  Today, in response to a request for 
help from a web site administrator, I visited a server with a cert
with a bad Key Usage extension.  NSS returned the error code sec_error_inadequate_key_usage, for which NSS's error string is 
"Certificate key usage inadequate for attempted operation."

But PSM displayed this:

> Secure Connection Failed
>
> An error occurred during a connection to 131.107.193.14.
> Could not establish an encrypted connection because the site uses a
> certificate that is invalid or corrupted. (sec_error_inadequate_key_usage)
>
> The page you are trying to view can not be shown because the web site uses an
> invalid or unsupported security protocol.
>
>     * Please contact the web site owners to inform them of this problem.

The error code sec_error_inadequate_key_usage is not correctly explained by
either :
- "invalid or corrupted"
or
- "web site uses an invalid or unsupported security protocol."

The difference between what I saw (with the trunk) and what he saw (with FF2)
was the addition of these parts:

> (sec_error_inadequate_key_usage)
>
> The page you are trying to view can not be shown because the web site uses an
> invalid or unsupported security protocol.
>
>     * Please contact the web site owners to inform them of this problem.

Sadly, "invalid or unsupported security protocol" is misleading (a key 
usage extension is not a protocol) and doesn't help the web site admin to 
determine what was wrong with his cert.
"(sec_error_inadequate_key_usage)" seems pretty specific, adding it seems like a huge improvement in pointing towards the problem.
Nelson, the reason why we displayed the string
  "Could not establish an encrypted connection because the site 
   uses a certificate that is invalid or corrupted"
is bug 355643. 
Are you able to help out with that?

The above should be done to fix it and display the correct error explanation.


The exact error description is usually technical. So in order to explain what happened in end-user-speak, we have another section at the end. This section is currently "not" individualized. It is currently the same for all NSS error codes.

The wording
  "The page you are trying to view can not be shown because the web site 
   uses an invalid or unsupported security protocol.
  * Please contact the web site owners to inform them of this problem."
was my initial attempt to provide it.

Proposals on how to improve this are welcome.
Kai: We MUST get rid of this invariant phrase: 
   "the web site uses an invalid or unsupported security protocol."
It is misleading and wrong for the vast majority of SSL errors.  

In current mozilla projects (FF,TB,SM, others) the only "valid supported 
security protocols" are SSL v3.0 or v3.1 (TLS 1.0).  The phrase "invalid or 
unsupported security protocol" means "some protocol other than SSL v3.0 and 
SSL v3.1".  Since the vast majority of SSL errors do NOT indicate the use 
of some other security protocol, we shouldn't say that all SSL errors are
due to some other security protocol.

If you're looking for a phrase that means "it didn't work well enough to
say it's secure", let me suggest something like these:

"the server sent data that was unacceptable or whose authenticity could 
not be verified" 
or "the server did not send data whose authenticity could be verified" 
or "data with verifiable authenticity was not received from the server"  
or "verifiable authentic data was not received from the server" 
or "the communication could not be verified as authentic"     (I like this one)
or "the authenticity of the received data could not be verified" 
or "it didn't work well enough to call it secure"
or "it didn't work".

Are these messages ever displayed in contexts where they might not be talking
about web servers?  Could this message be caused by attempting to read a 
signed email?  Might the "peer" be "your email correspondent", rather than
"the web site" or "the server"?  NSS uses the phrase "the peer" in cases 
where the message's peer might be any of these.

The phrase "Please contact the web site owners to inform them of this problem."
is FINE with me.  :-)
QA Contact: bmartin → ui
Depends on: 379298
Depends on: 240431
Depends on: 242139
Depends on: 384051
(In reply to comment #90)
> Kai: We MUST get rid of this invariant phrase: 
>    "the web site uses an invalid or unsupported security protocol."
> It is misleading and wrong for the vast majority of SSL errors.  

Ok, accepted.


> If you're looking for a phrase that means "it didn't work well enough to
> say it's secure", let me suggest something like these:
> 
> or "the communication could not be verified as authentic"     (I like this one)

Hmm. The communication seems a bit abstract.


> or "the authenticity of the received data could not be verified" 

I like this one best! :-)

Let me attach a patch that changes the wording.


> Are these messages ever displayed in contexts where they might not be talking
> about web servers?

No. This additional phrase is restricted to times when we render error messages as an error page in the browser. This is why it's also safe to say "the page" and "web site" in the phrase.

Nelson proposes a wording change and I agree.

old:
  the web site uses an invalid or unsupported security protocol

new:
  the authenticity of the received data could not be verified

Mike, could you please review the change in mozilla/browser for Firefox?

Neil, could you please review the change in mozilla/dom for SeaMonkey?
Attachment #268194 - Flags: superreview?(mconnor)
Attachment #268194 - Flags: review?(neil)
Comment on attachment 268194 [details] [diff] [review]
Error page wording change, patch v10 [checked in]

Stuart, please see previous comment. Could you please review this wording change in mozilla/camino?
Attachment #268194 - Flags: review?(stuart.morgan)
Depends on: 384205
Depends on: 380605

Updated

12 years ago
Attachment #268194 - Flags: review?(neil) → review+

Comment 94

12 years ago
Comment on attachment 268194 [details] [diff] [review]
Error page wording change, patch v10 [checked in]

r=smorgan for the Camino change.
Attachment #268194 - Flags: review?(stuart.morgan) → review+
Comment on attachment 268194 [details] [diff] [review]
Error page wording change, patch v10 [checked in]

I'm not sure this is user-understandable enough, but its more accurate in the end.
Attachment #268194 - Flags: superreview?(mconnor) → superreview+
Comment on attachment 268194 [details] [diff] [review]
Error page wording change, patch v10 [checked in]

This patch checked in to trunk.
Attachment #268194 - Attachment description: Error page wording change, patch v10 → Error page wording change, patch v10 [checked in]
(In reply to comment #92)
> old:
>   the web site uses an invalid or unsupported security protocol
> 
> new:
>   the authenticity of the received data could not be verified

This is a semantic change in the error message, that should be notified to localizers by changing the entity names. CC'ing Pike.
(In reply to comment #98)
> > old:
> >   the web site uses an invalid or unsupported security protocol
> > 
> > new:
> >   the authenticity of the received data could not be verified
> 
> This is a semantic change in the error message, that should be notified to
> localizers by changing the entity names. CC'ing Pike.

Good point. Fine with me. Attaching patch.
Who can do a rubberstamp review?
Comment on attachment 269071 [details] [diff] [review]
Patch: Rename string ID nssFailure to nssFailure2 [checked in]

rubberstamp=me
Comment on attachment 269071 [details] [diff] [review]
Patch: Rename string ID nssFailure to nssFailure2 [checked in]

Thanks, patch checked in
Attachment #269071 - Attachment description: Patch: Rename string ID nssFailure to nssFailure2 → Patch: Rename string ID nssFailure to nssFailure2 [checked in]
Either bug 370875 needs to be fixed or the patch from this bug that caused it needs to be backed out.
(In reply to comment #102)
> Either bug 370875 needs to be fixed or the patch from this bug that caused it
> needs to be backed out.

Hopefully fixed now.
This change is required on top of the earlier renaming of string IDs.
While neterror.xhtml appears to use other string IDs for the <div> identifiers, it actually uses JS to dynamically concatenate that ID.

Without this patch, the current error pages say "Oops... Detailed error message not available"

Updated

12 years ago
Attachment #269744 - Flags: review?(benjamin)
Attachment #269744 - Flags: review?(axel)

Updated

12 years ago
Depends on: 385904

Updated

12 years ago
Attachment #269744 - Flags: review?(benjamin) → review+

Updated

12 years ago
Attachment #269744 - Flags: review?(axel) → review?

Updated

12 years ago
Attachment #269744 - Flags: review?

Updated

12 years ago
Attachment #269744 - Attachment description: Patch: one more place where nssFailure needs to be renamed → Patch: one more place where nssFailure needs to be renamed [checked in]

Updated

12 years ago
Duplicate of this bug: 387370

Updated

12 years ago
Depends on: 400608
Depends on: 398718

Comment 106

12 years ago
rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
On an expired client cert the error message is gnomic for most of the sentient breathing population of the planet.

Quote:
Secure Connection Failed
An error occurred during a connection to some.host.com.

SSL peer was unable to negotiate an acceptable set of security parameters.

(Error code: ssl_error_handshake_failure_alert)
The page you are trying to view can not be shown because the authenticity of the received data could not be verified.

    * Please contact the web site owners to inform them of this problem.

End Quote,

The error is even misleading since it implies that its the server side which is broken rather than the client cert.



        
        
      


      
      

Comment 107

12 years ago
See bug 419069 for more explanation - the fault doe not necessarily lie with the server, but the server is the only one that can explain *why* it has send back ssl_error_handshake_failure_alert.
Simon, I think there is no need to copy information from other bugs into this bug.
If you'd like to broaden the audience, you can add people to the cc list.
I propose we discuss all details of that issue in bug 419069.

I think the error message from comment 105 correctly describes the situation, as explained in bug 419069. The server has a bug. The server does not care to send detailed failure information in the handshake. The client displays exactly what it has been told by the server. I don't see a client bug.
Duplicate of this bug: 364428
Blocks: 435025
Depends on: 443435
No longer depends on: 309585

Updated

11 years ago
Version: psm2.1 → 1.0 Branch
Depends on: 336599

Updated

9 years ago
Assignee: kaie → nobody
Whiteboard: [kerh-coa] → [kerh-coa][psm-feedback]
Keywords: ue
Target Milestone: mozilla1.9alpha1 → ---
Version: 1.0 Branch → Trunk

Updated

8 years ago
Depends on: 660559

Updated

4 years ago
Depends on: 445098
It's unclear what the state of this bug is. There appear to be checked in patches (and there don't appear to be non-obsolete non-checked-in patches), so I'm going to assume this is fixed. Any further work can be done in follow-up bugs. If this bug was intended to become a tracking bug, a new bug should be opened to track the remaining work (a bug that has checked-in patches also acting as a tracking bug is difficult to manage given our workflow).
Status: NEW → RESOLVED
Closed: 18 years ago3 years ago
Resolution: --- → FIXED
Alias: desired
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.