The discussion in bug 327181 culminated in the realization that it would be helpful if the Web Sites tab of the Certificate manager grew an extra button (and backing functionality) to allow users to add "exception sites." Like the current file-based import action, this would allow users to explicitly extend trust to certificates which might otherwise not be trusted (e.g. self signed certs, domain mismatches, &c) but with two notable exceptions: 1) The cert is fetched over the net immediately 2) The action is tied to the domain/port from which it was fetched This requires UI front end, as well as the necessary changes to the backend to support the cert/site/port triples. Kai's original napkin drawing can be found in the original bug here: https://bugzilla.mozilla.org/attachment.cgi?id=269919 I've also attached a first-pass XUL mockup based off of his drawing (built with the live XUL editor tool, so please ignore artifacts like the status bar and broken dialog buttons).

Suggested enhancement to the text of the blurb for self signed certs: old: If you know you can trust this certificate, new: If you have verified the certificate's contents with the person who issued it, and know that it is really the correct certificate for this site, Next suggestion: After fetching the cert, show it to the user and ask him to verify its contents. Show him the "fingerprints", and ask the question: Is this the certificate you believe is correct for this site?"

BTW, I think this is a great idea (extra button in the cert manager). But you knew that I would. :)

I had discussed with Johnathan, and in order to get this going, he needs some new backend functionality. I will add such a patch to bug 327181. My original proposal was: Let's do things in multiple steps: - land backend code to allow for overrides - Johnathan implements the UI overrides - we then switch to error pages But when I worked on the new backend code, by re-using the patch from bug 345007, and by implementing the new error pages and the additional status-on-error that Johnathan requires, I ended up having code overlaps. It would be good if you could use my patch from 328171 while you develop the UI. Ideally we would land your patch and mine at the same time. Let's try if that works out, if not, I'll have to work harder to allow us to do it in two steps.

We have decided (somewhere else) that we no longer want to import server certs and assign trust to them. Because when a user enters an override, it should be limited to the current hostname (and ideally port). However, the cert could carry hostnames that would allow it to become valid for additional hostnames. This is an unwanted side effect of "importing and trusting a cert". Therefore I propose we no longer import the cert with general trust added, because it's not necessary. The new code I'm adding in bug 327181 uses a dynamic approach. It stores {hostname,port,cert-fingerprint} together with the allowed override. At the time we open a connection, the list of overrides is checked against the cert presented by the peer server. If all hostname, port and cert-fingerprint matches with our override entry, the cert will be allowed. However, the above is not sufficient to display a full cert in cert manager's "web site tab". Maybe, in addition to the list of triples described above, should we import the server cert without any trust? Hmm, but in order to allow for finding the real cert based on the trip, we might need to store the issuer+serial-number in the override list as well.

Attaching this as a touch-point, it basically implements the dialog and functionality described, building off Kai's patch in bug 327181 (attachment 276553 [details] [diff] [review]). There are still bugs in that adding an exception doesn't dismiss the dialog, or show up in the exceptions list.

Comment on attachment 276699 [details] [diff] [review] Interim, basically-functional patch >Index: security/manager/pki/resources/jar.mn >=================================================================== >RCS file: /cvsroot/mozilla/security/manager/pki/resources/jar.mn,v >retrieving revision 1.54 >diff -u -8 -p -r1.54 jar.mn >--- security/manager/pki/resources/jar.mn 13 Apr 2007 01:26:39 -0000 1.54 >+++ security/manager/pki/resources/jar.mn 14 Aug 2007 22:10:38 -0000 >@@ -28,16 +28,19 @@ pippki.jar: > content/pippki/editemailcert.xul (content/editemailcert.xul) > content/pippki/editsslcert.xul (content/editsslcert.xul) > content/pippki/editcerts.js (content/editcerts.js) >+ content/pippki/editsslcert.xul (content/editsslcert.xul) >+ content/pippki/exceptionDialog.xul (content/exceptionDialog.xul) >+ content/pippki/exceptionDialog.js (content/exceptionDialog.js) > content/pippki/deletecert.xul (content/deletecert.xul) > content/pippki/deletecert.js (content/deletecert.js) > content/pippki/viewCertDetails.js (content/viewCertDetails.js) Found a copy-and-paste bug in your patch, you're adding editsslcert.xul a second time.

I wonder if I'm missing some code required to run this patch. When using the override dialog, besides a couple of JS warning, I get the following error message: JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x804b000a [nsIIOService.newURI]" nsresult: "0x804b000a (<unknown>)" location: "JS frame :: chrome://pippki/content/exceptionDialog.js :: getURI :: line 119" data: no]

My mistake. I entered a plain hostname, not a full URI as the dialog requires.

(In reply to comment #8) > My mistake. I entered a plain hostname, not a full URI as the dialog requires. That is something I'd rather the dialog not require, but I don't quite know how to approach it. If we just assume that when no protocol is specified, https should be prepended, then we likely fail for the non-browser situations, but since XHR expects a URI, I'm not sure what to do instead. Kai, you've had more background with making PSM work across various kinds of network client, what do you think?

http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIURI.idl#94 I think you can construct a new URI and the current implementation will convert several types of input strings into a valid URI. See NS_NewUri for how to use io-service to construct a new one. I think we should not "tell" the user that it's possible to paste a full URL here. I think that causes confusion for protocols other than https. If we agree this dialog shall be used as an universal dialog for any protocol, I'd recommend to talk about "site" and "port". The port field would not be required when combined with an URL, since the URL can contain a port specification as in https://www.mozilla.org:443/. But the port field is required for any other protocols. (If you would like to get around this dilemma, you could have a radio button that switches between "web site" and "mail or other server". When the "web site radio button" is selected, we'd have a single "URI/location" field that requires a full https://something, with or without port, with or without https. When the "mail or other server radio button" is select, we'd hide the "URI/location" field and show two separate "server and port" fields. If you go with this solution, you could even hide the "mail or other server" when compiling Firefox, but enable it for Thunderbird and SeaMonkey.) If you want a single strategy for constructing a full URI from any of the possible input types, you could do this: - construct URI from input field => myURI (will work with both simple-hostname and full-uri) - as you're going to use xml-http-request for the test (ignoring the actual protocol), explicitly set the scheme to https using an assignment myURI.scheme = "https"; - if the user explicitly entered a value into the port field, assign that port number to the URI. Otherwise leave the default the URI-parsing set. myURI.port = dialog.port.value; I'm not sure if "Certificate Location" is the best label. The word "location" seems to suggest we are downloading it from somewhere. With this wording, I'm worried that some people might try to enter an address like http://my-server.com/my-cert.crt (which won't work, as we are binding the exception to the port of the connection attempt). What would you think about using "Check Certificate" or "Test Certificate" as the label for the button? Would that clarify we are not downloading, but connecting?

I found an issue with our plan to use XMLHTTPRequest. I tried to add an exception for a mail server port. Unfortunately the http channel is calling NS_CheckPortSafety and blocks many ports. I think we need a solution for cert and status fetching that works with any given port number. For example, IMAP/SSL uses port 993, POP/SSL uses port 995, SMTP/SSL uses port 465. All of them are blocked when trying to connect with XMLHTTPRequest.

Also: NNTP/SSL port 563 , LDAPS port 636

I found an argument why we should support any port, even within Firefox. Example: The IRC client Chatzilla that can be installed as an addon to Firefox. Server irc.mozilla.org operates an SSL service on port 6697. While this port number is not blocked by the netwerk backend, I noticed it takes a long time until the dialog finally reports the certificate details. I guess this is related to our attempt to attempt an http procotol request on a server that really uses a different protocol. The server probably keeps the connection up for a while, waiting for the kind of protocol message it expects... Not all protocols might time out quickly.

Fixed the problem with us (silently) requiring a well-formatted URI, by using the fixup service to build the URI from the input. There are 2 remaining issues: 1) The added exception does not appear in the web sites list. I need Kai's help figuring out how best to accomplish this, and think it needs to happen before the patch lands. 2) The dialog does not support non-https protocols. This is something we should support, but I think we should land this (and 327181) even without that support. It is launched off the "Web Sites" tab, and adds new functionality. I agree that we should file a follow-up to add broader support, but I don't think we need to block progress on our overall changes to SSL error handling based on this. Of course, if anyone has a low touch solution for #2, that would be great, but if we can fix #1, I personally think the patch is ready for review.

(In reply to comment #14) > 1) The added exception does not appear in the web sites list. I need Kai's > help figuring out how best to accomplish this, and think it needs to happen > before the patch lands. I agree, I just added a big patch to bug 327181. I will need to improve and review it, and get review from rrelyea, but maybe you're interested to try it. > 2) The dialog does not support non-https protocols. This is something we > should support, but I think we should land this (and 327181) even without that > support. It is launched off the "Web Sites" tab, and adds new functionality. > I agree that we should file a follow-up to add broader support, but I don't > think we need to block progress on our overall changes to SSL error handling > based on this. I agree that we can add support for any ports in a separate step. > Of course, if anyone has a low touch solution for #2, that would be great, but > if we can fix #1, I personally think the patch is ready for review. I'll work on it, I probably should file a separate bug.

Johnathan, I'm proposing the following change to your patch, to the way you open the add-exception-dialog. I propose it should be modal, so when the dialog gets closed, we can update the contents of cert manager. Here is an example: +function addException() +{ + window.openDialog('chrome://pippki/content/exceptionDialog.xul', "", + 'chrome,centerscreen,modal'); + var certcache = Components.classes[nsNSSCertCache].createInstance(nsINSSCertCache); + certcache.cacheAllCerts(); + serverTreeView.loadCertsFromCache(certcache, nsIX509Cert.SERVER_CERT); + serverTreeView.selection.clearSelection(); + orphanTreeView.loadCertsFromCache(certcache, nsIX509Cert.UNKNOWN_CERT); + orphanTreeView.selection.clearSelection(); +} I'm refreshing the contents of both the web-sites tab and the extra/orphans-tab, as our changes can potentially affect both tabs. If you want to go one step further, you could check whether the dialog returned with "a change was made", and only then reload the contents.

Hey kai - testing my latest patch, with your changed addException call and the rest of your patches from bug 327181 works fine. The correct results now show up. What is your advice on getting review here? Shall I ask you to, or BobR, or other? I will attach a new patch for the appropriate reviewer, but being so intertwined with bug 327181, I think we might want to coordinate the two, and that's the larger patch by far. I wonder, does it makes sense to combine the two? This bug is actually independent functionality, but it can't exist without 327181 because of its reliance on services exposed therein. Likewise, bug 327181 would be an entirely over-strong restriction without a way to add network-fetched exceptions, in my opinion. Maybe combining them is the right approach?

(In reply to comment #17) I absolutely agree that we should coordinate the patches from bug 327181 and this bug. They really go hand in hand and should land at the same time. I'm not sure if we should merge the patches yet. Having the patches separate means we can more easily split the work during the review process. I guess you'll be working on review requests addressed to the UI patch in this bug, and I'll work on change requests for the backend code. It might be harder to track the changes if we both have to work on the same single patch at the same time. I'll review your patch and BobR can give optional review comments if he would like to. I'll provide a test build for him, too.

Requires v5 or later of Kai's patch in bug 327181

I thought I attach some screenshots of the latest state that show how the add exception dialog looks for the various scenarios. upper left: dialog when you first open it upper right: host mismatch lower left: untrusted cert lower right: expired cert

Regarding the statement: "You should only add this exception if you have already confirmed the information yourself." Most users will not understand what it is that must be confirmed. They will think "I am looking for a cert for site X (e.g. paypal.com) and this cert claims to be for paypal.com, so I confirm this information". They will not understand that it is not enough to confirm that the cert contains the intended site name or the intended company name. It is necessary to confirm that the value of the PUBLIC KEY in the in the cert is actually the public key belonging to the rightful owner/operator of the named site. In general, that can only be confirmed by ASKING the site owner/operator to confirm it, and doing so through channels that are known to lead to the rightful owner/operator. Sending email to the email address in the attacker's cert for confirmation isn't good enough. It cannot be confirmed by a guess by the browser user. IMO, if we don't spell that out, many people will erroneously "confirm" the wrong information, and be victims. Maybe we need a link to a page on "how to confirm the information in this cert".

This is my answer to Nelson's concerns from comment 21 and my other review comments to this patch. (a) "The information provided by this site hasn't been verified by a recognized authority, and may be misleading. You should only add this exception if you have already confirmed the information yourself." Nelson is unhappy with the second sentence. After hearing his concerns I tend to agree and propose to simply remove the second sentence altogether. The statement is like "There is a problem. But if think it's not a problem then go ahead." The second sentence is trying to give the user an advice what to do. But giving the right advice is difficult. At least, it requires a lot more words. But the more words we use in the dialog, the less likely is it that users will read it (my opinion). So my proposal is to: - state the problem - be short and concise and scary - have the user look elsewhere for more details and assistance (see b) In other words, I propose to remove all 3 sentences from the patch that start with "You should only". (b) Nelson proposes to give more information in order to assist the user to make the correct decision. I agree. But I recommend to keep the dialog wording concise and provide the assistance in a separate window. What's the usual way to give more assistance? I propose to add a help button to the dialog, which pops up a separate window. Nelson, would you be interested to write text for such a help page? After thinking more about the above and looking at the current screenshots, I played around with the code to come up with ideas... Johnathan, please feel free to tell me this is ugly, but maybe the following is helpful. (c) The "Add Exception" label in the dialog sounds innocent. What about "Disable Security"? (d) I really like the phrase "Most people would not do this ... (and other's) ... would not ask you to do this". I wonder if this phrase should get the focus. Right now it's being displayed at the top of the window, in plain text, next to another bold paragraph, which makes it loose attention. Should we print "You are about to..." as plain text, show "most people would not..." as bold, and move the latter close to the "disable security" button? (e) The "Certificate Location" label. As we don't have a separate certificate download location, but rather a site we connect to, I wonder if we can improve this wording. But maybe that's only because I know how it technically works :-) We want the user to enter the location of the site he wants to connect to. So, I think "Location" is good, because that's what being used in Firefox' File menu, too. Could we use "Site" or "Web Site" as a label for the group box? I hate to suggest "Web Site", because this will be used for mail as well. But as of today Certificate Manager has a "Web Sites" tab, even though it applies to any SSL site. So, until we fix that generally, I propose let's be consistent and re-use "Web Site" in this dialog. (I was tempted to say "Secure Site", but after all, that site might not be secure, so let's not add confusion.) (f) The current statement "You are about to..." talks about "this site". When you open a dialog and have not yet opened any details, could this be confusing? We are not in the context of a web site, instead, we came from cert manager, so maybe we should use a slightly different wording like this: "You are about to override Firefox and force it to treat a site as trusted even though it may not be." Finally here are comments related to the code and dialog behavior. (g) The Port number. I propose we open the dialog with the port number field being "empty". I played around with the dialog and found the 443 to be confusing. One of our goals for this dialog is: Allow the user to paste a full URL into the Location box. An URL can contain a port number as in https://www.somewhere.org:1234/. It feels inconsistent if we show 443 after the user pasted such an URL. My proposal is: - have the port field empty by default - should the user enter a hostname, but no port number, we default to 443 (without displaying that) - if the user enters/pastes a full URL, we use the port number from that URL (or default to 443) - if the user explicitly enters a port number, we use it (and override whatever got entered into the Location field) - the above also works if the user enters host:port into the Location field I experimented and think I have a code change that makes it work like this. (h) I get JavaScript strict warnings when I run the code, I think we need declarations at the top of the exceptionDialog.js file to make them go away: var gDialog; var gBundleBrand; var gPKIBundle; var gSSLStatus; var gChecking; var gBroken; All variables should get declared before being used first. (i) I think the dialog should be reset to the initial state (no cert, action button disabled) as soon as the user makes changes to the Location or Port fields.

... playing with the bugzilla "attachment diff" feature, hopefully this patch will work better...

Comment on attachment 281313 [details] [diff] [review] Updated addException code in certManager I'm giving you r- because I think (g) (h) (i) from comment 22 must be fixed. (a) to (f) are proposals only. These are my personal thoughts right now. I have also asked Bob R for comments about the current UI, we should wait for him.

Some thoughts: When I see the text box for "Port", my assumption is that I must fill it in. However people who don't understand how URLs work won't know what to put in there. Maybe they'll leave it alone, or maybe they'll try to put something in there. I see the discussion above, and I wonder if we need that field at all. Is there a *user-driven* reason to put it there? Why is "nntp://news.example.com:444" not acceptable? I want to copy/paste from some other source into this field. Regarding the title of the dialog, how about something like: Site Security Override Override Security Protection Override Security Settings Regarding the top text, how about: -- <b>This window allows you to override how Firefox evaluates the identity of a server.</b> This is a potentially dangerous operation. Legitimate banks, stores, and other public sites will not ask you to confirm their identity because Firefox already does that. Do not perform this operation unless you have confirmed the identity of the site. [[How do I do that safely?]] -- That last part can be a link to a help page. Regarding the tag "Certificate location", how about changing it to "Internet Site"? And perhaps change "Get Certificate" to "Evaluate Site Identity"? I agree that "Add Exception" is clear to us, but does not send the right message. Perhaps "Confirm Security Override"? Just some ideas to keep the conversation rolling. :-)

I like Kai's latest screen shots, and I also like Bob's ideas. Good stuff. Yes, I'm willing to help write a help page. I think it should be more than mere text. I'd say it should include screen shots showing the dialogs/windows that the user will see, showing which information to ask the server owner to confirm. It should also explain that the user must contact the server owner using information independent of the certificate, that is, using a phone number or other means that are NOT listed in the certificate itself. On further thought, maybe we need a "wizard" to lead the user through the steps of confirming a certificate.

There can be multiple problems with a cert, 3 different ones. Right now, the dialog will only report a single problem and be silent about other problems in parallel. However, it will store an override that bypasses all current errors. An untrusted cert is reported as "Unverified information". If the cert is also expired and/or has a mismatch, the dialog will not mention it. A trusted, but expired cert will be reported as "Outdated Information". If there is a mismatch, too, the dialog will not mention it. A trusted cert that is valid at the current time with a mismatch will be reported as "Wrong Site". Johnathan, did you intentionally report a single problem only?

(In reply to comment #29) > There can be multiple problems with a cert, 3 different ones. Right now, the > dialog will only report a single problem and be silent about other problems in > parallel. However, it will store an override that bypasses all current errors. Kai, what does the client display when encounters a site using a revoked certificate? Also, it would seem to me that we should show all important certificate information in that screen at the same time. I'd further suggest name changes as follows: * Unknown identity: This site is using a certificate from an unknown source. Firefox cannot validate the identity of this site. -or when appropriate- Unknown identity: This site is using a self-signed certificate. A self-signed certificate is one issued by the site itself instead of a well-known authority. * Expired certificate: This site is using a certificate that has expired and it is no longer valid for identifying this site. * Site name mismatch: This site is using a certificate for "www.example.com", but the certificate was issued for "www.company.com". Should we put in words there like "This error may indicate that someone is trying to hijack your communications with www.example.com"?

"issued by the site itself" sounds entirely legitimate. What's wrong with using a certificate for paypal.com that paypal.com itself issued? The point is that *we do NOT know* know issued it. Maybe it was the site named in the cert, or maybe it is an attacker. Expired Cert: Add: "This certificate may have been revoked, and we have no way to tell, since it is expired." or "FireFox cannot determine if expired certificates have been revoked or not." As for showing "all important certificate information": we know that if we show the cert's subject name, and that is the name the user expected, the user is very likely to falsely conclude that it is legitimate.

(In reply to comment #27) > When I see the text box for "Port", my assumption is that I must fill it in. > However people who don't understand how URLs work won't know what to put in > there. The intention for the port field is to support mail protocols, where you usually know the port number of your server. But I agree with your observation, a user might be unsure what to enter. Isn't this a good thing? Another hurdle for end users to add an exception that they shouldn't add :-) But I tend to agree, maybe we can find a better form... > Maybe they'll leave it alone, or maybe they'll try to put something in > there. I see the discussion above, and I wonder if we need that field at all. > Is there a *user-driven* reason to put it there? Why is > "nntp://news.example.com:444" not acceptable? I want to copy/paste from some > other source into this field. Sounds acceptable to ME, technically it already works as is, should we drop the Port field. However, I think most end users have never seen URLs for anything but http:// or https:// What if we drop the Port field and put a little help sentence below the Location field: Location: __________________________ Enter the full location of the site you wish to visit or use the host:port notation. Or maybe simply: Location (or host:port) _______________________ > Regarding the title of the dialog, how about something like: > Site Security Override > Override Security Protection > Override Security Settings I leave this discussion to you native speakers. I guess it should use similar wording as the button that opens the dialog and the action text that we use in the dialog. > Regarding the top text, how about: > -- > <b>This window allows you to override how Firefox evaluates the identity of a > server.</b> This is a potentially dangerous operation. Legitimate banks, > stores, and other public sites will not ask you to confirm their identity > because Firefox already does that. Do not perform this operation unless you > have confirmed the identity of the site. [[How do I do that safely?]] > -- > That last part can be a link to a help page. Works for me. But I guess it's tricky to have a link to a help page in the middle of chrome? In order to make it work with localization, we'd have to splie the link and non-link into separate strings, but this causes problems for right-to-left languages. I guess Johnathan should guide us here, both in the wording, whether my separation of paragraphs make sense, or whether we should use a link or help button. > Regarding the tag "Certificate location", how about changing it to "Internet > Site"? And perhaps change "Get Certificate" to "Evaluate Site Identity"? Fine with me. If we change "Web Site" to "Internet Site", should we be consistent and change the heading in Certificate Manager too? (Change "Web Sites" to "Internet Sites" or "Sites")

(In reply to comment #28) > On further thought, maybe we need a "wizard" to lead the user through the > steps of confirming a certificate. Using a wizard was initially suggested. The idea was rejected initially, and a all-in-one-dialog preferred. I'm fine to switch to a multi-page wizard, if Johnathan feels we should.

(In reply to comment #30) > Kai, what does the client display when encounters a site using a revoked > certificate? It's not perfect yet, but it won't allow an override. (FYI, you can find a testcase for OCSP and revoked certs here: http://kuix.de/misc/test-ocsp/ ) When I trust that test CA (!) and enter https://revoked.kuix.de:8352/ into the exception dialog, I get - an error popup saying the cert is revoked - the dialog says "No Information Available" and "This site doesn't supply any identification. There is no need to add an exception for this site". This indeed needs improvement. > Also, it would seem to me that we should show all important certificate > information in that screen at the same time. What information do you consider important? If we want to show more information, the dialog will have to become bigger. (Or we'll have to use a multi-page wizard, so we don't need to show all chrome at the same time) > I'd further suggest name changes as follows: > * Unknown identity: This site is using a certificate from an unknown source. > Firefox cannot validate the identity of this site. > -or when appropriate- > Unknown identity: This site is using a self-signed certificate. A self-signed > certificate is one issued by the site itself instead of a well-known authority. Johnathan, when you get the "untrusted" error, you should be able to distinguish "self signed" and "unknown/untrusted issuer" by comparing cert.issuer and cert.subject for equality. > * Expired certificate: This site is using a certificate that has expired and it > is no longer valid for identifying this site. > * Site name mismatch: This site is using a certificate for "www.example.com", > but the certificate was issued for "www.company.com". Sounds good to me. > Should we put in words there like "This error may indicate that someone is > trying to hijack your communications with www.example.com"? Sounds good to me. We had a phrase like this in the old dialog for untrusted certs.

(In reply to comment #31) > "issued by the site itself" sounds entirely legitimate. What's wrong with > using a certificate for paypal.com that paypal.com itself issued? > The point is that *we do NOT know* know issued it. Maybe it was the > site named in the cert, or maybe it is an attacker. Good point. I still think that a terse scary wording is better than a lot of explanation. Is there any value in telling the user the difference between self signed and unknown issuer? Imagine we use different wordings, and one of our wordings will sound a little less scary than the other, then attackers will use the kind of cert that sounds less scary :-) > Expired Cert: Add: "This certificate may have been revoked, and we have no > way to tell, since it is expired." or "FireFox cannot determine if > expired certificates have been revoked or not." What does it mean to an end user to read "certificate might be revoked or might not be revoked"? Probably confusion. I personally think it's ok to omit the technical details and say something like Johnathan's UI currently shows or Bob's proposal. To add my version to the mix of ideas :-) "This site presented an expired identity certificate. The ownership of the certificate can no longer be verified. It might be in possession of someone who is trying to attack your connection and commit a crime."

In reply to comment 34: IMO, we should not allow an override of revoked certs. In keeping with Bob's note about how the port box affects him, when I see a "location" box with nothing in it, I want to enter a city name and/or street address. I realize that FireFox now calls its URL bar a location bar. Even so, I suggest that we pre-fill the box with "http://" to clue the user (like me) as to what kind of location is being requested.

(In reply to comment #36) > In reply to comment 34: > IMO, we should not allow an override of revoked certs. I absolutely agree! Revoked certs should never be allowed!!! Not sure why you conclude that from my comment 34, but I agree we should never do that. When I said "this needs improvement", my concern is about the way the dialog is reporting this failure. IMHO the information presented in the "Certificate Status" box should state the certificate is revoked (right now it says no identity information is available, because there is a technical limitation in how the dialog obtains status information). > In keeping with Bob's note about how the port box affects him, > when I see a "location" box with nothing in it, I want to enter > a city name and/or street address. I realize that FireFox now > calls its URL bar a location bar. Even so, I suggest that we > pre-fill the box with "http://" make that https:// > to clue the user (like me) as > to what kind of location is being requested.

(In reply to comment #36) > In keeping with Bob's note about how the port box affects him, > when I see a "location" box with nothing in it, I want to enter > a city name and/or street address. I realize that FireFox now > calls its URL bar a location bar. Even so, I suggest that we > pre-fill the box with "http://" to clue the user (like me) as > to what kind of location is being requested. I've seen web sites that put something into search fields, like "Type your search here". They make the text a bit gray, and when you click on the field, that initial string vanishes. Maybe we could do the same here? Perhaps even suggest "e.g. https://www.example.com" or "https://..."?

(In reply to comment #34) > If we want to show more information, the dialog will have to become bigger. > (Or we'll have to use a multi-page wizard, so we don't need to show all chrome > at the same time) I'm thinking that if there are multiple things wrong with the cert, we should show them all. A cert might be self-signed, expired, and have the wrong CN, for example. Is it hard to generate a "worst case" mockup showing the largest number of problems? Maybe we don't need more real estate, but we'd have to see it to be sure.

Bug 327181 is closely related to this bug, both should go in at the same time. Therefore I'm requesting this bug should be a blocker, too.

We should move on with this bug. Today Mike Connor asked me to land bug 327181 very soon, but it must go in with some UI from this bug. Johnathan hasn't been able to address our last 20 comments with a new patch yet, so I'm trying to help out and trying to find a compromise. I propose I land something that works and addresses all technical concerns with the dialogs. We can produce a follow up patch that improves the wording and final visual tweaks to the dialog. - in cert manager, renamed "Websites" tab to "Internet Sites" - for consistency, I changed the groupbox-heading to "Internet Site" - I kept the "Add Exception" button label - for consistency, I changed the dialog's title to "Add Security Exception" and the button inside the dialog to "Confirm Security Exception" - I addressed my own request (i) from comment 22 - I addressed the proposal to completely drop the "Port" field - I used the proposal to pre-fill the Location field with https:// I think that makes it clear for power users that a host:port can be entered. We can add a help button later, that explains this possibility. - I removed the help button for now, as I don't know whether Johnathan will prefer that or an inline link, and we don't know have the help page yet. - I moved the "get certificate" button to the right of the location field, in order to save space - I renamed the "view certificate" button to "view" in order to save space, and moved it to the upper right of the cert status area. To me this looks nice, to have both "fetch" and "view" buttons aligned at the right and in the upper area of the dialog, so they are far away from the dialog confirmation buttons. But this is only a proposal, we can change this in a follow up patch. - now, if there are multiple problems with the cert (up to 3), the dialog will display three paragraphs and bold headings. - I reworded the strings for the status feedback. I removed everything that sounded like an suggestion. I attempted to explain the facts and risks as clearly as possible. - I enhanced the behavior when a connection to the site is not possible, we will say "unable to obtain information" in that scenario. - I added a new "Connection Failed" string, this will be helpful when we introduce the better cert-fetching-backend, we will be able to combine that heading with the detailed error message, e.g. "revoked" (instead of bringing up a popup box) - I reverted my earlier change to move the bold warning to the bottom of the dialog. This seems a bit unusual, and I'd like to stick with Johnathan's proposal as much as possible. However, I still have the bold/non-bold reverted, as I had previously proposed. I will attach a new patch and screenshots. I will ask Johnathan for a quick UI-review, in the hope he is ok with this as an initial patch. I will ask Bob for a code review.

Minor changes: - changed the heading at the top of the dialog - changed the headings in the cert status area to be bold (but no longer using a larger font) - fixed the size issue of the "view" button

This screen shot illustrates the new heading and the status area. Please refer to the earlier screen shots to see the various variations.

Comment on attachment 283117 [details] Screen Shot based on patch v8 First of all, thank you kai for picking up the patch - I was working on addressing your review comments in my own tree, but your patch here has already done so. I think we should land this quickly, and then tweak it as necessary to suit actual feedback. I do think the first sentence of the status ("...using information with an invalid statis.") is confusing. What about just: "This site attempts to identity itself with invalid information." I know technically the information could be valid, but maybe it conveys the right message, even if technically indefensible? ui-r=me with that string or something equivalent, I don't want to block this on getting it perfect. What happened to changing "Internet Site" to "Web Site" to match it's current placement within the "Web Sites" dialog? Either way works, but I agreed with the argument that we call it "Web Sites" as long as it is, and change the string when we have something to change it to.

I'll mark this patch as r=kengert because most of the code was done by Johnathan, I reviewed it and enhanced it. But because my code changes haven't been reviewed yet, I'll ask Bob for a second review.

marking ui-reviewed=johnathan, this has the change he requested

(In reply to comment #49) > I do think the first sentence of the status ("...using > information with an invalid statis.") is confusing. What about just: > "This site attempts to identity itself with invalid information." Changed as proposed. > What happened to changing "Internet Site" to "Web Site" to match it's current > placement within the "Web Sites" dialog? Either way works, but I agreed with > the argument that we call it "Web Sites" as long as it is, and change the > string when we have something to change it to. Please look at attachment 283101 [details], lower right corner. As part of this patch I'm changing the heading of the "Web Sites" tab to say "Internet Sites".

There is a typo in the first sentence. Please change "how Minefield identities" to "identifies".

"Internet Sites" is another such string that will be very hard to translate for localizers, as it's a new creation of a previously not existing phrase. I wonder how much English users even understand what it means. And we already have enough problems as-is with having our UI mix "website", "web site" and "site", meaning the same with three different strings.

(In reply to comment #55) > "Internet Sites" is another such string that will be very hard to translate for > localizers, as it's a new creation of a previously not existing phrase. I > wonder how much English users even understand what it means. And we already > have enough problems as-is with having our UI mix "website", "web site" and > "site", meaning the same with three different strings. > * Français: "sites internet" ("sites web" is more "franglais" so more people will frown at it). * Esperanto: "interretejoj", "interretaj paĝaroj" (though here "TTT-ejoj" [websites] is maybe more frequent but not necessarily "better" language). * Nederlands: I'm not sure how to translate "site" but both "Internet" and "web" should be their own Dutch equivalents IIUC. * Ich weiss nicht wie es auf Deutsch zu sagen, aber ich erunterstelle, dass Du [KaiRo] es wisst. In general, I believe that "the Internet" is more "international" (and thus easier to translate both faithfully and understandably) than "the web".

Sure that "the Internet" is more international, but then we should either use "Internet site" or "web site" or "website" or "site" constently though the whole application, and not 4 different forms of meaning (practically) the same thing. That is bad UI. (Oh, and in German it's "Website", which most normal users confuse with "Webseite" anyways, which means "web page". And I wonder how you'd translate the difference between "Internet site" and "web site" and "website" and "site" into other languages...)

(In reply to comment #57) > (Oh, and in German it's "Website", which most normal users confuse with > "Webseite" anyways, which means "web page". And I wonder how you'd translate > the difference between "Internet site" and "web site" and "website" and "site" > into other languages...) The term "Internet Site" is supposed to include things like "mail servers", "ftp servers", "ldap servers", any sort of server that potentially uses SSL. I think "Website" is restricted to http:// and https:// Robert, do you have a proposal?

Kai, I share your concern that we want this dialog to be usable for more than web servers. On the other hand, it's in a tab named "web sites" (according to this bug's summary, at least) so maybe we need to change the tab name as well.

(In reply to comment #59) > Kai, I share your concern that we want this dialog to be usable for > more than web servers. On the other hand, it's in a tab named > "web sites" (according to this bug's summary, at least) so maybe we > need to change the tab name as well. I did that already in the patch! :-) See the patch, and my first item in comment 41 and my last sentence in comment 52.

Kai, I know what the distinction is supposed to mean, but I'm pretty sure that 90% of our users don't even know about that distinction, and it's even harder for localizers to get it across in their language. And when you tell them that a "site" or "Internet site" includes any type of server, while a "web site" includes only web-dedicated types/areas of servers, they probably tell you "And why don't you call it 'server'? That's a word I at least read everywhere else!" :)

Comment on attachment 283174 [details] [diff] [review] Patch v9 with typo fixed [checked in] r+ I would like o see the following changes in a future patch: 1) use arrays for long/short description sin updateCertStatus. It will make adding new statuses a bit easier without greatly expanding the number of local variables. 2) gCert appears to not be declared with the other globals. What does that mean for the javascript scope of the variable (obviously it's at least global within this .js context or the dialogs would not work at all). bob

(In reply to comment #62) > (From update of attachment 283174 [details] [diff] [review]) > r+ Thanks > I would like o see the following changes in a future patch: > > 1) use arrays for long/short description sin updateCertStatus. It will make > adding new statuses a bit easier without greatly expanding the number of local > variables. I'd like delay this optimization to a future patch. > 2) gCert appears to not be declared with the other globals. Good catch. I will add this before I check the patch in. > What does that mean > for the javascript scope of the variable (obviously it's at least global within > this .js context or the dialogs would not work at all). Variables are automatically available when they are first used in JS, but it produces warnings and for clean code all variables should be declared. I'll add that.

(In reply to comment #61) > why don't you call it 'server'? I'm personally fine to drop "Websites" and drop "Internet Sites" and use "Server / Servers" everywhere (including the tab in cert manager). But what do other's think?

Server and Servers sound good to me.

Needed for another blocking bug, so blocking on this one, too.

I suspect that, and I guess I can only speak for english speakers, "web site" is less jargon-rich than "Servers," but not critically so. Let's go with that then, and if a compelling case comes up for a string change, that's a future bug. :)

This has landed on the trunk, seeting target milestone to M9. Not marking fixed yet. I think we want to continue with tweaking.

Comment on attachment 283174 [details] [diff] [review] Patch v9 with typo fixed [checked in] This is the patch that was checked, with var gCert; added

This patch renames the "Internet Sites" tab to "Servers" It renames the groupbox heading in the add exception dialog from "Internet Site" to Server"

Comment on attachment 283376 [details] [diff] [review] Patch: Rename "Internet Site(s)" to "Server(s)" [checked in] r+ I'm fine with Servers if ui and translation is. bob

Comment on attachment 283376 [details] [diff] [review] Patch: Rename "Internet Site(s)" to "Server(s)" [checked in] actually, Johnathan already said he's ok with using "server(s)" in comment 67

As I said earlier, the patch checked in today only works for https and some random ports. It does not yet work for mail server ports, although the solution was intended to be generic across protocols. Currently we use xmlhttprequest for cert-from-ssl-connection fetching, but this implementation blocks many ports, including all mail server ports. It was already clear that we'd need a generic service for fetching the server cert from a ssl connection, that would work with any port. But when I started to work on that today, I realized, it's more complicated! In addition to connection of the style: cleartext-protocol-completely-wrapped-in-ssl there is also the mechanism known as STARTTLS, which works like: plaintext-phase-then-switch-to-ssl The trouble is, the initial plaintext-phase isn't universal, for example, IMAP-STARTTLS and SMTP-STARTTLS use a different plaintext-phase. This means, we can't use a single generic implementation for fetching the server ssl certificate from a protocol specific connection. We'd have to bring the server configuration details, and we'd have to make use of the protocol specific application code for starting the connection. Maybe we will have to do this. But it seems complicated, might require tricky UI and might non-trivial code to be written. After scratching my head, I came up with a workaround approach. It's not perfect, but I think it's a reasonable workaround. I already implemented it. It's attached as "Non-Web Patch v2". Let me explain what it does. Whenever we encounter a "bad cert" (expired, untrusted, mismatch), PSM will remember it. Actually, PSM will remember a couple of recently-seen-bad-certs, and will remember to which hostname and port it was associated, as well as the verification result. These certs will not be stored on disk. Not imported into the NSS database. But they will be available to be retrived using a new service: nsIRecentBadCertsService This list of recently-seen-bad-certs will not shown to the user, only available internally. However, we can make use of this list while executing the add-exception dialog. In a typical session the user might configure a new smtp mail server that requires STARTTLS The connection will fail and an error dialog will be shown (e.g. can't connect, mail.somewhere.org:25 uses expired cert). PSM will remember that cert. Now the educated user will have to know where to go. He will open cert manager and find the add-exception button. He will enter mail.somewhere.org:25 into the location field. Now the add-exception-dialog will query the nsIRecentBadCertsService with the key mail.somewhere.org:25 The cert will get returned with the expiration status. The dialog can display the cert status and offer to add the override. Whenever the add-exception dialog gets a matching cert+status from nsIRecentBadCertsService, it can avoid to call into xmlhttprequest (which is blocked and therefore nonworking for port number 25). This means: Before an override can be added (for mail ports), the user must try to connect to the server once. Only afterwards will an attempt to add an exception work. I think this is a reasonable and simple intermediate solution, and I propose to check it in until we have something better. The thunderbird trunk testers will be thankful (because right now, with the landing of bug 327181, the use of servers with bad certs is completely impossible).

Just to make sure: your detailed example is about "plaintext-phase-then-switch-to-ssl"; but "cleartext-protocol-completely-wrapped-in-ssl" (as in bug 398534) (will) behave the same way ?

(In reply to comment #75) > Just to make sure: > your detailed example is about "plaintext-phase-then-switch-to-ssl"; > but "cleartext-protocol-completely-wrapped-in-ssl" (as in bug 398534) (will) > behave the same way ? Yes, it will work, too. Mozilla's PSM layer will remember any such bad cert, regardless of the specific application protocol variation that triggered the connection. This includes any other variations like IRC/SSL or LDAP/SSL.

(In reply to comment #74) > As I said earlier, the patch checked in today only works for https and some > random ports. > > It does not yet work for mail server ports, although the solution was intended > to be generic across protocols. > ... > After scratching my head, I came up with a workaround approach. It's not > perfect, but I think it's a reasonable workaround. > > I already implemented it. It's attached as "Non-Web Patch v2". Let me explain > what it does. You know better than I how to navigate cert management, so while the idea sounds clever in principle, I won't comment on its technical aspects. I would suggest moving it to its own bug though, since it seems like the kind of thing that might cause weird regressions, and need to be backed out/relanded independent of this bug.

Comment on attachment 283376 [details] [diff] [review] Patch: Rename "Internet Site(s)" to "Server(s)" [checked in] In this patch I renamed "Internet Sites" (formerly "web sites") to "Servers". I just realize, I didn't update the sentence below it, it still says "You have certificates on file that identify these web sites:" I will change this to "... that identify these servers:", I will add this change to the patch in bug 398549 (where we'll be doing more renaming in cert manager anyway)

(In reply to comment #77) > I would suggest moving it to its own bug though That's a reasonable suggestion. I've filed bug 399043. Moving "blocks 398534) over to new bug.

Comment on attachment 283448 [details] [diff] [review] Non-Web Patch v2 [moved to bug 399043] I've moved this patch over to bug 399043. This bug blocks 1.9 Bug 399043 does not yet block 1.9, but I think it really should.

Warning: Unknown property 'whitespace'. Declaration dropped. Source File: chrome://pippki/content/exceptionDialog.xul It's "white-space".

Kai, I don't even know if this needs review but thanks to Steffen in comment 81 for catching it. Changes whitespace css rules to the correct "white-space"

I know I've seen somewhere that typo fixes, spelling errors, etc. don't require review (and can be committed assuming the tree's not in some sort of lockdown mode), but I can't find it at the moment.

Comment on attachment 284957 [details] [diff] [review] Fix whitespace->white-space bug [checked in] r=kengert

What's left to be done for this bug? I guess we check in the white-space fix, then close it?

I´m sorry but I don´t have time to see if this was already suggested. I´m also sorry that I cross posted this to Johnathan´s blog, but i felt it would be meaningful. :) Why don’t use a IE like page that when clicking “Continue to this website (not recommended)” will led the user to a page that reads in big read letters “By entering this site some attacker can take the data you enter on the website”. And make the user wait like 3 seconds for the first 3 times he tries to enter the site. Later just the warning appears. Seems a good trade off between making users aware/angry…

Comment on attachment 284957 [details] [diff] [review] Fix whitespace->white-space bug [checked in] Email from beltzner suggests that this fix needs approval before check-in pre-beta.

Comment on attachment 284957 [details] [diff] [review] Fix whitespace->white-space bug [checked in] a=beltzner

all patches checked in, marking fixed.

Not sure if the plugin Remember Mismatched Domains located at https://addons.mozilla.org/en-US/firefox/addon/2131 is related to this.

(In reply to comment #91) > Not sure if the plugin Remember Mismatched Domains located at > https://addons.mozilla.org/en-US/firefox/addon/2131 is related to this. Well that add-on will be obsolete for Firefox 3 (and I'm guessing Thunderbird 3), but it will of course still work with Firefox 2 and Thunderbird 2 :)

Is this bug fixed for Thunderbird 3? It's not fixed in the currently-available release of 2.0.0.9. The status of "RESOLVED FIXED" is not clear as to whether it's been fixed in a nightly build of a future release or what. I just switched hosts and now have a domain mismatch (the domain is mine, but the certificate belongs to HostGator), and had to download the "Remember Mismatched Domains" extension to fix it.

The bug status is always about the "trunk", i.e. the bleeding-edge in-development code, unless obviously indicated otherwise - in this case that's the code leading up to Gecko 1.9, i.e. Firefox/Thunderbird 3, SeaMonkey 2 and others.

(In reply to comment #93) > It's not fixed in the currently-available > release of 2.0.0.9. The add exception dialog is not contained in thunderbird 2, so this bug is not applicable to tb 2.