398417 - Add UI for adding certificate exceptions

Status: RESOLVED FIXED

(Camino Graveyard :: Security, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

kaie stopped by this morning to point us to some changes in the certificate manager involving the handling of bad certificates (backend in bug 327181, UI in bug 387480). 

As a consequence of those changes, we need to add support for adding an exception to our certificate UI so that people can continue to access those sites (e.g., self-signed internal sites) if necessary.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

[1:51pm] ardissone|away: http://blog.mozilla.com/rob-sayre/2007/10/10/prd1-ability-to-navigate-to-web-pages/
[1:51pm] ardissone|away: http://blog.johnath.com/index.php/2007/10/11/todo-break-internet/
[1:54pm] ardissone|away: if this is really as common as you and they are saying
[1:55pm] ardissone|away: we should really push that bug back up to the front of "needs to fix" list 
[1:55pm] jeff: yea I tend to get it quite often
[1:56pm] jeff: and it makes me cry

I hate for us to keep diverting resources to the trunk, but this sounds like it could easily become a dogfood issue (and is already for jeff)....

Comment 2

[Stuart Morgan]

Are we confident that this decision is stable enough to warrant putting the time and energy into making the UI? It sounds like there's still debate about how these kind of certificates should be handled.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Changing URL, since the MS URL works now.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

All the conversation (and energy, and bugs) seems like it's solidly directed at making adding an exception more discoverable/less painful for those who know they need to do it, while keeping it hidden enough that average users do not throw away the security benefits the new stricter code provides.

See http://blog.johnath.com/index.php/2007/11/04/sleepy-happy-wtb-5-dwarves/ and the bugs referenced therein.

Comment 6

[Disorn Homchuenchom]

Still, Camino doesn't provide a way to add exception unlike an existing method in Firefox. (Click then dialog popup with an exception added.)

I don't know how to add an exception for my site.

Comment 7

[Stuart Morgan]

(In reply to comment #6)
> Still, Camino doesn't provide a way to add exception

That's why this bug is still open.

Comment 8

[Leander Koornneef]

> I don't know how to add an exception for my site.

You can do it from the command line, using "certutil" from the NSS Security Tools
(see http://www.mozilla.org/projects/security/pki/nss/tools/). These tools are also available through MacPorts (i.e.: `sudo port install nss`). 
You'll have to download the certificate that you want to import first, which can be done in more than one way: with Safari, the openssl command, or export it from your Keychain (if present there).

Then, import the certificate into the Camino certificate database with certutil like this:

For a trusted peer (server certificate):

user@host# certutil -A -n <name> -t P -d ~/Library/Application\ Support/Camino -i <path/to/certificate>

For a trusted CA that issues server certificates:

user@host# certutil -A -n <name> -t C -d ~/Library/Application\ Support/Camino -i <path/to/certificate>

(see `certutil -H` for more info/help)

You'll have to restart Camino after this for the change to take effect.

This method is not very convenient, but it's the only way I've found to circumvent this problem...

Leander

Comment 13

[Stuart Morgan]

Attachment: fix

This adds an "Add Exception..." button to the error page. I coalesced our three old dialogs into one dialog with the same basic structure (warning, then inline cert view), and pick the most serious of the problems with the cert to describe (along with some boilerplate based on Firefox's language and some of our old language). There are other approaches we could take if we prefer, such as:
- keep the text from the three separate dialogs largely as it was, meaning the text will vary much more
- follow some or all of Firefox's approach (have the cert hidden behind a button, but list each of the problems)

I assume we'll want to discuss and tweak the text that's here (once we decide for sure on an approach); this is just a first pass.

Things this doesn't do, that I'd like to cover in follow-ups since they don't block an alpha:
- Allow seeing this list and deleting things from it from the Cert prefs (I don't see any reason for us to allow adding from prefs)
- Use AutoSizingTextField for the message text, since it varies in length, and adjust the size of the sheet dynamically. That's both for polish (right now there can be extra space between the text and the cert view) and l10n (it's would be a pain for localizers to make the space in the nib match whatever they do in the .strings)
- Pretty-ify the error page's "add an exception" section if we want to (Firefox's yellow rounded rect, or something of that nature)

Comment 14

[Stuart Morgan]

Attachment: new nib

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://www.black-helicopter.org/bh/getone.html?
https://ub-bib3.ub.uni-greifswald.de/DB=1/LNG=DU/

I didn't really have time to play with this fully, but a couple of thoughts/notes:

>  <p>If you still wish to add an exception for this site, you can do so in your advanced encryption settings.</p>

From the looks of things, that line can just be deleted and bug 402459 can be zapped.

Oh my word, those things (page & sheet) are so full of text!  

The part that bothers me is that the human-readable description of the actual error is "buried" between the intro sentence and the NSS error code; can we fix something in our netError to make them all paragraphs?

cbo.ardisson.org uses an invalid security certificate.

The certificate is not trusted because it is self signed.
The certificate is only valid for box183.bluehost.com

(Error code: sec_error_untrusted_issuer)

(Oddly, above is how it appears when pasted from the page into this bug, which implies the <p>s or <br>s are there but aren't appearing properly in the HTML page.)

The sheet feels like it has a similar text-overload problem, or rather a "too much text in one chunk" problem, but I should reflect more on that when I have a fresher mind....

I want to play with this a lot more later....

Comment 16

[Stuart Morgan]

(In reply to comment #15)
> >  <p>If you still wish to add an exception for this site, you can do so in your advanced encryption settings.</p>
> 
> From the looks of things, that line can just be deleted and bug 402459 can be
> zapped.

Doh, that was just an oversight.

> can we fix something in our netError to make them all paragraphs?

Let's file that as another bug, since it's separate from the exception stuff.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #16)
> > can we fix something in our netError to make them all paragraphs?
> 
> Let's file that as another bug, since it's separate from the exception stuff.

We can probably roll that with the "Pretty-ify" error page design work you mentioned into one bug....

Also, the keyboard loop in the new nib is really bad; initial focus is on some sort of invisible view.

When we add an exception, should we make the certificate trusted for SSL when doing so?  (Note to self: see what fx does for various certificate types: self-signed, etc.)

Oh, while I'm thinking about it, bug 321825 would be good to fix as a follow-up to this (or WF if we don't want it), or at least make the initial view larger?

I promise, that's the last of the tired ramblings tonight.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 333559 [details] [diff] [review]
fix

The biggest problem I have with this patch is actually the fact you can't see and delete exceptions anywhere.  I know that's on your "doesn't block an alpha" list, but I'm not sure that I agree.  It's even worse that I can remove the added cert or CA and the exception will persist, so if I've realized I've made a mistake when adding an exception, I think I've cleaned up my mistake by removing the CA/cert I added, and I can still access the site with no warnings at all!  That's probably another bug (removing a cert/CA should remove any related exception), but the fact I can't even see what else I'd need to clean up really bothers me.  If we're blocking an alpha on getting this security UI into a workable state, I think we should block on being able to (minimally) fix any mistakes.

> old dialogs into one dialog with the same basic structure (warning, then inline
> cert view), and pick the most serious of the problems with the cert to describe
> (along with some boilerplate based on Firefox's language and some of our old
> language).

Coalescing the several almost-identical sheets into one seems like a good idea to me ;)

> - follow some or all of Firefox's approach (have the cert hidden behind a
> button, but list each of the problems)

I really think we need to list each of the problems, because otherwise the text is too vague and doesn't provide enough information, in my mind, to discourage clicking "Add".  At this point the user is unfortunately deep into "having to make a decision", and we should at least provide some useful human-readable text to help him/her out.

(I don't find anything beneficial about hiding the cert behind a button; we show nice little error icons at the top, which is a visual queue to the user, and for people who want to do more inspection of the cert, it's a saved click.)

> I assume we'll want to discuss and tweak the text that's here (once we decide
> for sure on an approach); this is just a first pass.

I'd like to split the text in the sheet into the first sentence, then perhaps the descriptive info about the cert errors, and then the last warning sentence (I like the idea of making this last sentence bold, as Firefox does, too).

As I mentioned before about the nib, the keyboard loop is awful ;)  It takes three tab-presses to end up on a button, and the initial view and the second subsequent one are some sort of hidden views :(  (The second tab press goes to the "Show Issuer Certificate" button.)

I'm not sure how to fix the thing completely (i.e., can we show a focus ring on the cert view when we head into it?), but we need the initial focus on a button.  I think the right solution for the buttons on this sheet is the same as the "Do you want to quit with downloads active?" sheet, where default button and initial focus button are both on the "Don’t Add" button, and both Return and Esc activate that button.  (I don't think we can do dual shortcuts with nibs, though, can we?)

> - Pretty-ify the error page's "add an exception" section if we want to
> (Firefox's yellow rounded rect, or something of that nature)

I think we need to do something to differentiate this, yes; I'll file a "design polish bug" on this and the error structure bit if we're all in agreement.

Just copying this from above so you have all my relevant comments in one place:

> >  <p>If you still wish to add an exception for this site, you can do so in your advanced encryption settings.</p>
> 
> From the looks of things, that line can just be deleted and bug 402459 can be
> zapped.

(In reply to comment #17)
> When we add an exception, should we make the certificate trusted for SSL when
> doing so?  (Note to self: see what fx does for various certificate types:
> self-signed, etc.)

Insofar as I can test these things (my kingdom for test servers!) and deal with the crappy Fx UI for viewing cert and CA bits, the answer to this is "No", so we're doing the right thing.

Other than those things, I think this looks pretty good.  r- at least on the nib, though.

Comment 19

[Stuart Morgan]

(In reply to comment #18)
> (From update of attachment 333559 [details] [diff] [review])
> The biggest problem I have with this patch is actually the fact you can't see
> and delete exceptions anywhere.  I know that's on your "doesn't block an alpha"
> list, but I'm not sure that I agree.

It's another bug either way; we can debate wether or not it should block alpha there.

I'll try another text layout, and fix the other issues.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

In FF3, you can see exceptions.  They show up in the Certificate Manager 
dialog's Servers tab.  I'm not sure if you can delete them or not. 
If not, that's a major bug!  If it can be confirmed, a bug should be filed
about it.  If you file such a bug, please CC me. thanks.

Comment 21

[Stuart Morgan]

(In reply to comment #20)
> In FF3, you can see exceptions.  They show up in the Certificate Manager 
> dialog's Servers tab.  I'm not sure if you can delete them or not. 
> If not, that's a major bug!  

Yes, we're all aware of the Firefox UI. Nobody claimed that it doesn't work.

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 333559 [details] [diff] [review] [details])
> > The biggest problem I have with this patch is actually the fact you can't see
> > and delete exceptions anywhere.  I know that's on your "doesn't block an alpha"
> > list, but I'm not sure that I agree.
> 
> It's another bug either way; we can debate wether or not it should block alpha
> there.

Stuart pointed out to me that the exceptions are just stored in a plain-text file (instead of inside an NSS db file), so there is a reasonably accessible way to remove the exception; given that, I don't think it needs to block.

Comment 23

[Stuart Morgan]

Re-titling, since the old title doesn't match what we are actually doing to resolve the need for this UI.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Stuart, perhaps you could write a comment in this bug summarizing what 
you're planning to do for this.  I hope you're not going back to the 
one click elimination of security errors.

Comment 26

[Stuart Morgan]

Comment 13 is the description of what we are doing. Imagine the Firefox UI but only accessible from an error page (not the prefs) and without making the user pointlessly* press a "Get Certificate" button (and without the checkbox to make the exception temporary, at least for now), and you're pretty much there.

I'm aware that you strongly objected to this model in bug 401575, but the issue is not up for re-debate from scratch here. Taking a harder line on security than everyone else at significant cost in usability is not the route we are choosing to go.


(* I realize that the argument was made in 402207 that making the user click one more button somehow added security; I don't buy that giving users a mostly-blank UI with one button other than cancel is going to cause the user to expend a great deal of thought in deciding what to do next)

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Taking away "the checkbox to make the exception temporary" will lead to a 
lot of complaints about unbypassable errors reporting duplicate issuer names
and serial numbers.  IOW, doing so will cause major usability issues for 
some users.

Comment 28

[Stuart Morgan]

We'll add it later if necessary. The purpose of this bug is to unblock our alpha, not to solve every problem related to the cert changes all at once.

Besides, the premise that having the checkbox solves the usability problem is at best highly questionable; unless everyone with an affected router somehow intuits that changing a checkbox about making exceptions temporary will prevent them from having a problem that they aren't aware of yet and won't understand when they encounter, the usability problem is still there.

Comment 29

[Stuart Morgan]

Attachment: v2

Round two:
- fixes the screwed up keyboard focus/navigation
- makes the boilerplate text bold, and moves it next to the buttons
- includes auto-resizing of the message
- makes the sheet and cert view slightly wider to fit the cert contents better

Unfortunately, what Smokey though was the generic intro text was actually the most specific text we can currently get for one of the failure modes; for some reason our cert code is getting a really vague answer from core when we ask about cert status instead of details about why it's invalid. Since that's a pre-existing problem and touches a bunch of our other cert code, we'll work on getting good error messages again in a follow-up. (To see one of the other messages we currently do have, go to https://gmail.com)

So to recap the things that we'll hit in other bugs:
- Fix the cert code to get correct, detailed errors (bug 453075; not a regression from 1.6)
- Improve the initial error page (bug 451131)
- Allow users to see and remove things from the exception list (bug 453076)
- Maybe something like bug 321825 for this sheet (not yet filed)

Comment 30

[Stuart Morgan]

Attachment: nib v2

Comment 31

[Stuart Morgan]

Attachment: v2 screenshot

A screenshot of what v2 looks like in action.

Comment 32

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 336287 [details] [diff] [review]
v2

Works as expected, and the keyboard loop is much saner now--with the exception that the cert view doesn't accept focus; r=me with a follow-up on that.

Comment 33

[Mike Pinkerton (not reading bugmail)]

+    InvalidCertOverrideDialogController* certDialogController =
+      [[[BrowserSecurityUIProvider sharedBrowserSecurityUIProvider] invalidCertOverrideDialogController] retain];

do you want to document the retain/release pattern here for the controller? just so it's more explicit?

+    if (certDialogController) {
+      [certDialogController showWithSourceURL:url parentWindow:[self window] delegate:self];
+      return;
+    }

seems odd the success case is the one returning early.

sr=pink otherwise

Comment 34

[Stuart Morgan]

(In reply to comment #33)
> do you want to document the retain/release pattern here for the controller?

Yep, I'm surprised I hadn't.

> seems odd the success case is the one returning early.

Agreed and fixed.


Landed on CVS trunk (almost a whole month before the one-year mark!). Please file new bugs for any issues not covered by existing follow-up bugs.