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.)

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.)

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

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.

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 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"

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

>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?

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.

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

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]

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

Verified.

If this bug is fixed, then why do we continue to receive reports about -8101 errors??

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?

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.

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

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

*** 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.

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

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 :(

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

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.

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/

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.

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 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).

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.

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

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

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

(In reply to comment #43) > typo: Cllient Thanks, I filed bug 359280 to get this fixed in NSS.

Comment on attachment 241409 [details] [diff] [review] Patch v5 Obsoleting patch, we want some additional tweaks.

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.

there was a variable init missing, fixed

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).

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.

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?

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?

Comment on attachment 252824 [details] [diff] [review] Patch v7 - netError.dtd in dom and browser Neil, please see the previous comment.

Thanks for Stephen Donner who made me aware of a typo. Third attempt of attaching this patch :-)

Updated screenshot to correct typo.

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 ;-)

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

explanation follows

(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?

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

(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

(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.

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

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?

Comment on attachment 255040 [details] Screen Shot v9 I meant to ask for review on the patch, not the screenshot.

Carrying forward Neil's sr on the seamonkey / dom portion. Asking Mike to review for Firefox.

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.

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.

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?

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. :-)

(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?

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?

Comment on attachment 268194 [details] [diff] [review] Error page wording change, patch v10 [checked in] r=smorgan for the Camino change.

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.

Comment on attachment 268194 [details] [diff] [review] Error page wording change, patch v10 [checked in] This patch checked in to trunk.

(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

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"

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.

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.

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).