Add UI for adding certificate exceptions

RESOLVED FIXED in Camino2.0

Status

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: alqahira, Assigned: stuart.morgan+bugzilla)

Tracking

({regression})

Trunk
Camino2.0
PowerPC
Mac OS X
regression
Dependency tree / graph
Bug Flags:
camino2.0a1 +

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

v2
37.53 KB, patch
alqahira
: review+
mikepinkerton
: superreview+
Details | Diff | Splinter Review
6.95 KB, application/octet-stream
Details
70.49 KB, image/png
Details
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.
[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)....
(Assignee)

Updated

11 years ago
(Assignee)

Comment 2

11 years ago
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.
Changing URL, since the MS URL works now.
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.

Updated

11 years ago
Summary: Add the "Add Exception" action to the Websites section of the Certificates window → Add the "Add Exception" action to the Websites section of the Camino Certificates window
Duplicate of this bug: 410311

Comment 6

11 years ago
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.
(Assignee)

Comment 7

11 years ago
(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

11 years ago
> 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
(Assignee)

Updated

11 years ago
Duplicate of this bug: 414578

Updated

11 years ago
Summary: Add the "Add Exception" action to the Websites section of the Camino Certificates window → Add the "Add Exception" action to the Websites section of the Camino Certificates window (UI for self-signed certificates)
Duplicate of this bug: 423416
(Assignee)

Updated

11 years ago
Keywords: regression
(Assignee)

Updated

11 years ago
Flags: camino2.0a1+
(Assignee)

Updated

11 years ago
Blocks: 430673
(Assignee)

Updated

11 years ago
Assignee: nobody → stuart.morgan
(Assignee)

Updated

11 years ago
Duplicate of this bug: 445500

Updated

10 years ago
Duplicate of this bug: 450055
(Assignee)

Comment 13

10 years ago
Created attachment 333559 [details] [diff] [review]
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)
Attachment #333559 - Flags: superreview?(mikepinkerton)
Attachment #333559 - Flags: review?(alqahira)
(Assignee)

Comment 14

10 years ago
Created attachment 333560 [details]
new nib
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....
(Assignee)

Comment 16

10 years ago
(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.
(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 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.
Attachment #333559 - Flags: review?(alqahira) → review-
(Assignee)

Updated

10 years ago
Attachment #333559 - Flags: superreview?(mikepinkerton)
(Assignee)

Comment 19

10 years ago
(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.
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.
(Assignee)

Comment 21

10 years ago
(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.
(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.
(Assignee)

Comment 23

10 years ago
Re-titling, since the old title doesn't match what we are actually doing to resolve the need for this UI.
Status: NEW → ASSIGNED
Summary: Add the "Add Exception" action to the Websites section of the Camino Certificates window (UI for self-signed certificates) → Add UI for adding certificate exceptions
(Assignee)

Updated

10 years ago
No longer blocks: 402459
Duplicate of this bug: 402459
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.
(Assignee)

Comment 26

10 years ago
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)
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.
(Assignee)

Comment 28

10 years ago
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.
(Assignee)

Comment 29

10 years ago
Created attachment 336287 [details] [diff] [review]
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)
Attachment #333559 - Attachment is obsolete: true
Attachment #336287 - Flags: superreview?(mikepinkerton)
Attachment #336287 - Flags: review?(alqahira)
(Assignee)

Comment 30

10 years ago
Created attachment 336288 [details]
nib v2
Attachment #333560 - Attachment is obsolete: true
(Assignee)

Comment 31

10 years ago
Created attachment 336289 [details]
v2 screenshot

A screenshot of what v2 looks like in action.
Attachment #336287 - Flags: review?(alqahira) → review+
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.
+    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
Attachment #336287 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 34

10 years ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.