431819 - IMAP/POP/SMTP/LDAP with SSL client auth, Thunderbird repeatedly prompts for client certificate

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Steven Tucker]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14
Build Identifier: Thunderbird version 2.0.0.14 (20080421)

When connecting to a IMAP site via SSL w/ certificate, Thunderbird now pops up a security dialog every time the IMAP connection is opened.  The dialog reads "User Identification Request", "This site has requested that you identify yourself with a certificate".  In previous versions of Thunderbird this authentication was automatic and there was no security popup every time Thunderbird was run.  I am not sure if the popup is a bug, or if its supposed to be remembering which certificate to use after the first time and isn't.

Reproducible: Always

Steps to Reproduce:
1. Configure IMAP connection via SSL with certificate.
2. Start Thunderbird
3.
Actual Results:  
The security dialog appears every time Thunderbird is started, or the IMAP connection is restored.

Expected Results:  
Thunderbird should authenticate using the saved certificate automatically without user intervention, as all previous versions did.

Comment 1

[Ignacio Coupeau]

At my site, *all* the computers with these configuration ask for "User Identification Request" every 30 seconds plus every time sendmail smtp STARTTLS is called.

Comment 2

[Ignacio Coupeau]

Also the POP3 with SSL has the problem.
Reproducible in the same way that the IMAPS.

Comment 3

[DEXTER]

Congrats guys!

This bug is extremely annoying, and it is still in unconfirmed status?

Conrats guys again!

Comment 4

[Ignacio Coupeau]

A shortcut or maybe the solution:

1. Select the menu: "Tools|options" 
2. select the sub.menu "Advanced"
3. Select the pane "General"
4. Select the button: "Config Editor" 
5. Set the property
security.default_personal_cert "Ask every time" TO "Select automatically" 

I don't know why several clients have the "Select automatically" and other the "Ask every time" value. Ask every time, forces to the client to ask for the certificate every time, with time ~ check mail time or so, and every send with smtp secure.

Comment 5

[Steven Tucker]

The setting "security.default_personal_cert" does not effect the behavior in any way on my system with the client version reported above.

Comment 6

[Stoo]

I have the same issue under Linux Fedora Core 9 bundled RPM update with the details:

version 2.0.0.14 (20080501)

Comment 8

[Phil Ringnalda (:philor)]

Kaie, dveditz: thoughts? Despite what bug 295922 comment 7 says about having clear UI to disable it, *we* don't seem to have any UI at all, and while we could add it on the trunk, we can't on the branch. So, what's our privacy risk, anything other than RSS feeds that load the web page instead of the feed content?

Comment 9

[Eddy Nigg (StartCom)]

Hello? Somebody can confirm this one please? This bug is confirmed by me. Which blocking flags do we want here? This should go away with the next update of TB2 and TB3alpha.

Comment 10

[Eddy Nigg (StartCom)]

Sorry, it was confirmed just now.

Comment 11

[Phil Ringnalda (:philor)]

Also bug 431957, though I don't know enough about it to know if that's a duplicate.

Comment 12

[Eddy Nigg (StartCom)]

(In reply to comment #5)
> The setting "security.default_personal_cert" does not effect the behavior in
> any way on my system with the client version reported above.
> 

Try "Select Automatically", without the quotes and caSe-SensiTIve.

Comment 13

[Steven Tucker]

Thank you, Select Automatically did work, I must have misspelled it last time or used the wrong case.

Comment 14

[Stoo]

Confirmed that the above case sensitive string fixed it under Linux TB 2.0.0.14

Comment 16

[Paul Scott]

Something changed in version version 2.0.0.14 (20080421) that caused this. Immediately after upgrading I experienced the problem. Before seeing this bug report I had looked at advanced option security.default_personal_cert and it said something like "Select automatically", but it didn't. Unfortunately, I don't remember the "exact" wording, and I subsequently cleared the field to blanks. That made no difference. However, now setting it to "Select Automatically" (case sensitive) the problem disappeared. Very possible that only the case sensitivity changed from the prior release.

Comment 17

[Phil Ringnalda (:philor)]

There's no mystery about what changed: bug 295922 changed the default, because it's a privacy risk in a browser, feeling it was a safe change because Firefox and SeaMonkey and Camino all have UI to change back to automatic, and we just need to decide what to do about the fallout for us, since we don't have UI and probably don't have anywhere near the privacy problem.

Comment 18

[Eddy Nigg (StartCom)]

It should be "Select Automatically" for TB, it doesn't make sense for TB to ask every time as for FF. See first comment of bug 432367.

Comment 19

[Kai Engert (:KaiE:)]

I think it was a wrong decision made by Thunderbird developers to remove UI for important preferences (when they converted seamoneky code to thunderbird code, this is not the only one they removed). But maybe I should blame poor wording in seamonkey prefs, which suggest it's only about web sites. In fact it's for all kind of SSL connections.

I'm not sure I can agree with comment 18.

In the following scenario, the privacy issue applies as well:
- user owns an email cert from a popular ca
- spam sender sends html page
- user has enabled remote content
- html page loads some remote content over https

If the spam server is configured to do optional ssl client auth, we have the privacy issue, no?

I think the future default for thunderbird should remain at "ask every time" as well, but we really must work on a comfort feature that allows users to remember decisions based on sites.

Comment 20

[Steven Tucker]

I agree, a simple checkbox in the security dialog that said 'Always use this certificate for this site' would be an acceptable solution to me.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

This is pretty annoying behavior; setting to block 3.0a2 (and we should relnote for 3.0a1, I suspect, and I agree that something needs to be done for 2.0.0.15 as well.

On the trunk, we're going to need some UI here.  I suspect a checkbox in the dialog will be needed at minimum, maybe some other UI (in PSM prefs?) is wanted too.

On the branch it seems as though we have several choices:

1) revert the change that caused this for Tb only

Simple, but it does re-open the privacy hole that was closed here.

2) release note the change suggesting that people using the preferences workaround described in this bug.  We should almost certainly do this for 2.0.0.14 anyway.

2) accept minimal new UI on the branch

This likely requires a new string, which we'd have to get for all of our locales.  We could potentially make the code fall-back to using the English string in cases where the correct locale-string isn't available, though.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

That last choice, of course, should have been labeled with the number 3.  If folks can think of other options, please add...

Comment 23

[Eddy Nigg (StartCom)]

(In reply to comment #19)
> In the following scenario, the privacy issue applies as well:
> - user owns an email cert from a popular ca
> - spam sender sends html page
> - user has enabled remote content
> - html page loads some remote content over https
> 
> If the spam server is configured to do optional ssl client auth, we have the
> privacy issue, no?
> 
> I think the future default for thunderbird should remain at "ask every time" as
> well, but we really must work on a comfort feature that allows users to
> remember decisions based on sites.
> 

I was one of the most proponent supporters initially to get this fixed in FF. However isn't remote content blocked by default in TB? If this is NOT the case than you are right, else I suggest to consider selecting one by default AND add the select boxes back for choosing the other option.

Comment 24

[Steven Tucker]

The best solution would be to move certificate selection to the SSL section under account setup.  It seems to belong there since its part of SSL configuration for a connection.

Comment 25

[Kai Engert (:KaiE:)]

Adding Bob to cc list, so he can track this SSL client auth issue.

Comment 26

[Paul Scott]

Keep in mind that the "pop up" window mentioned in the problem title isn't just at startup. It happens every time you press the "send" button after composing an e-mail. In this case, it has nothing to do with SSL or a connection to the server, even though the message seems to indicate that. It's just asking which certificate is to be used to digitally sign the e-mail message. And the correct default setting should be "Select Automatically" so that the certificate matching the From: account is chosen by default. Maybe I should not have closed bug 431957 as a duplicate?

Comment 27

[Kai Engert (:KaiE:)]

Clarifying subject.

Comment 29

[Eddy Nigg (StartCom)]

(In reply to comment #24)
> The best solution would be to move certificate selection to the SSL section
> under account setup.  It seems to belong there since its part of SSL
> configuration for a connection.
> 

Don't think that's feasible, considering multiple accounts would be almost impossible to handle in this UI.

(In reply to comment #27)
> Clarifying subject.
> 

Please note that's not only for IMAP/POP but also SMTP.

Comment 30

[Phil Ringnalda (:philor)]

Given that nobody has (so far) proposed a privacy issue with select automatically for mail servers, only remote content, I think my choice would be

4) Default to select automatically for imap/pop/smtp(/signing? I'm still unsure about bug 431957), and to ask every time for content.

That has the advantage for branch of not needing to break the string freeze, though the disadvantage of not being something I know how to fix, or even how to evaluate the risk of the patch.

Remote content can't be handwaved away with "it's disabled by default" because that's an oversimplification: it's disabled until you click the [Load Images] button on an email, or click the link to always allow remote images from [e-mail] and from anyone pretending to be them, or set either bad-idea pref, to allow all remote content or to allow remote content from any address at a specified domain. If I wanted to match individual Danes to their email addresses, I'd start by looking for any instructions about setting mail.trusteddomains, and if that failed look for popular newsletters or other frequent mailers with remote images, and as a last resort just start sending mail with remote images that promised to be... something people might be foolish enough to want to see badly enough to risk loading remote images.

That said, if another fix doesn't materialize, I'm pretty sure I'd take a branch-only patch to just revert the pref - particularly in the misconfigured server cases like comment 1, which seem to have made all the NSS people say "oh, no, we can't just flip the pref" in bug 395399 before bug 295922 did just flip the pref, I think if I got an "update" like that to my supposedly stable mail client, I'd just switch to one that didn't throw a dialog at me every 30 seconds, rather than teach my email provider about the intricacies of SSL or look for a bug report that would tell me how to manually change a case-sensitive string pref.

Comment 31

[Bryan Clark (DevTools PM) [:clarkbw]]

Is it possible to keep the FF setting for remote content that might be viewed in an email and revert the setting for mail/pop/smtp/signing?  I don't think either is a great experience, but what we have now is a real problem.

Perhaps this is oversimplifying the problem, but where we have browser behavior and remote content privacy concerns I'd like to align with FF and it's behavior.  Where we are connecting as an email client we need to walk our own path. 

Now feel the power of my laughable pseudo code!
security.tb.default_personal_cert = "Select Automatically"
security.ff.default_personal_cert = "Ask every time"

Our 'security.tb' setting handles traffic between a mail server while the 'security.ff' setting handles any traffic coming from the message viewing pane.

Comment 32

[Daniel Veditz [:dveditz]]

(In reply to comment #31)
> Is it possible to keep the FF setting for remote content that might be viewed
> in an email and revert the setting for mail/pop/smtp/signing?

That's what Phil was saying in comment 30.

In nsNSS_SSLGetClientAuthData we know the hostname, the port, and IP address. If we also have the scheme stashed somewhere we could treat mail protocols according to a different pref. Thunderbird could probably override the UI and then decide when to show it (Kai, is there anything like nsIBadCertListener for this?), but then it's got to supply the UI itself when it _is_ appropriate to choose.

If we can't do comment 30 easily then for Thunderbird 2 we just reverse the pref if we have to.

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Sounds like we're on the right track here; removing uiwanted.

Comment 34

[Johnathan Nightingale [:johnath]]

I don't know what I can add here, but I will say that I (as well) agree that the interaction and relationship a tbird user has with their SMTP server (long-standing, significant trust, 1-1, configured in advance) is a very different beast from the relationship a ff user has with a web site.

If tbird branch just flipped the pref altogether that would not keep me up at night - though obviously we need a good understanding of the right ways to handle remote-looking mail content.  As dmose says though - there seems to be progress afoot, so I'll step back into the shadows.  :)

Comment 35

[Bryan Clark (DevTools PM) [:clarkbw]]

Excellent, I'm thinking that a good course of action might be to flip the pref back for the short term TB and file a new bug to head in the direction we've been talking about here for the longer term TB.

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

In the 2.0.0.14 post-mortem, it was agreed that we'd add release note verbiage to the 2.0.0.14 relnotes describing the problem and workaround forthwith.

So for the slight longer term, possible courses of action would seem to be:

1) spin a 2.0.0.15 with the fix described in comments 30-32

* best for end users, as everyone who got the previous update is likely to get this one and have the problem go away automagically, and they keep the privacy win
* may not have sufficient build and QA resources

2) spin a 2.0.0.15 with the pref reverted

* less risky than option 1
* stresses already constrained build and QA resources
* may not have sufficient build and QA resources

3) build an XPI that reverts the pref, and try to notify users (via blogs, etc.)
* many users probably won't be reached
* less QA & build load than previous options
* Tb XPI install experience is complex

4) notify users of the existing workaround (via blogs, etc.)
* many users probably won't be reached
* requires users to use advanced config editor

5) wait until the planned 2.0.0.15 (current projections have mentioned June)

We're still weighing these options.

Comment 37

[Kai Engert (:KaiE:)]

(In reply to comment #36)
> 1) spin a 2.0.0.15 with the fix described in comments 30-32

I see comments 30-32 as two different proposals.

1a) introduce new smart logic that can distinguish between
      "SSL connections for mail protocols"
           (select automatically)
     and
      "SSL connections that attempt to load remote content from the web"
           (always ask)

     We don't have such logic yet, and I guess it will be tricky to code it.

1b) Have separate prefs for Firefox and Thunderbird 

    I don't know how this can be achieved. Maybe the default pref value
    (which is currently stored in 
        mozilla/netwerk/base/public/security-prefs.js )
    could be removed from core, and duplicated
    into the application code of each application.
    But I don't recommend this.
    It has the risk that some other applications dependent on 1.8 branch code
    will be missed.

1c) Keep the core pref.
    Introduce some logic that overrides the core pref with a thunderbird pref.


> 2) spin a 2.0.0.15 with the pref reverted

This is core code, so if you revert the pref, it will affect Firefox, too (and SeaMonkey).


> 3) build an XPI that reverts the pref, and try to notify users (via blogs,
> etc.)

> 4) notify users of the existing workaround (via blogs, etc.)


I wouldn't do 3 or 4. I think users who read blogs are able to open about:config


> 5) wait until the planned 2.0.0.15 (current projections have mentioned June)

With the plan to implement option 1a, 1b or 1c ?

Comment 38

[Eddy Nigg (StartCom)]

1c with a longer term investment for TB3 (can be 1a, but other ideas were also mentioned, worth exploring).

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #37)
> (In reply to comment #36)
> > 1) spin a 2.0.0.15 with the fix described in comments 30-32
> 
> I see comments 30-32 as two different proposals.
> 
> 1a) introduce new smart logic that can distinguish between
>       "SSL connections for mail protocols"
>            (select automatically)
>      and
>       "SSL connections that attempt to load remote content from the web"
>            (always ask)
> 
>      We don't have such logic yet, and I guess it will be tricky to code it.

This is what I was trying to express.

> 
> 1b) Have separate prefs for Firefox and Thunderbird 
> 
>     I don't know how this can be achieved. Maybe the default pref value
>     (which is currently stored in 
>         mozilla/netwerk/base/public/security-prefs.js )
>     could be removed from core, and duplicated
>     into the application code of each application.
>     But I don't recommend this.
>     It has the risk that some other applications dependent on 1.8 branch code
>     will be missed.

This is really what I meant by 2.  I've been assuming, though, that simply adding a different setting to all-thunderbird.js would override the one in security-prefs.js.  Is that correct?

> > 5) wait until the planned 2.0.0.15 (current projections have mentioned June)
> 
> With the plan to implement option 1a, 1b or 1c ?

Right.

Comment 40

[Dan Mosedale (:dmosedale, :dmose)]

Adding KaiRo to the CC, since if SeaMonkey is shipping off of branch, they're likely effected too.

Comment 41

[Kai Engert (:KaiE:)]

(In reply to comment #40)
> Adding KaiRo to the CC, since if SeaMonkey is shipping off of branch, they're
> likely effected too.

And with SeaMonkey the story is more complicated, because the single pref affects both web and mail connections.

Unless we do code changes, SeaMonkey is stuck at having the same behavior in both web and mail.

Comment 42

[Kai Engert (:KaiE:)]

(In reply to comment #39)
> > 1b) Have separate prefs for Firefox and Thunderbird 
> > 
> >     I don't know how this can be achieved. Maybe the default pref value
> >     (which is currently stored in 
> >         mozilla/netwerk/base/public/security-prefs.js )
> >     could be removed from core, and duplicated
> >     into the application code of each application.
> >     But I don't recommend this.
> >     It has the risk that some other applications dependent on 1.8 branch code
> >     will be missed.
> 
> This is really what I meant by 2.  I've been assuming, though, that simply
> adding a different setting to all-thunderbird.js would override the one in
> security-prefs.js.  Is that correct?


You propose we try 1c, yes I'd agree (although that doesn't help SeaMonkey).

I don't know if that works, let's test, can be done quickly, by adding that line, and looking at about:config

Comment 43

[Kai Engert (:KaiE:)]

(In reply to comment #32)
> Kai, is there anything like nsIBadCertListener for this?

I'm not aware of anything helpful.

PSM does not have knowledge about the application protocol that runs on top of the SSL connection.

Application code requests a socket from networking, which uses nsISocketProvider, which only provides host and port to PSM.

One could be tempted to do some guessing based on port numbers. But that feels like a hack. It will fail in configurations where user use port forwarding, or mail servers on nonstandard ports. So, our solution will help some users, maybe even the majority, but power users in special environmens will still have problems.

Comment 44

[Robert Kaiser]

SeaMonkey surely will ship from 1.8.1.x branch, not from the .14 release branch though, just FYI.

I understand the basic problem why this was introduced - but what's the problem in re-sending the same info to _the same host_ in _the same session_ again? I think the solution should be to only ask again (by default) if we send either to a different host or we have a new session (which could also mean the master password has expired due to sanitize or SeaMonkey/wallet's "Log Out" function) - the same way we AFAIK do it for passwords.

Would this be new functionality we don't have yet? If so, what problem is that for branch? Or am I completely thinking in a wrong way?

Comment 45

[Dan Mosedale (:dmosedale, :dmose)]

A comment from philor in IRC:

"unless [PSM] abandons the pref name .default_personal_cert, we have to ship another release with select automatically even if they do start ignoring the pref for mail protocols, so that all the people who change manually don't get stuck with a user value for the pref"

a) The pref code actually notices when a value changes back to the default and removes the user value?

b) Having some small set of people stuck with a user value here doesn't seem like a terrible thing to me.  It's not clear to me whether it's worth shipping another release with select automatically just to avoid that scenario, but I don't feel like I completely understand the pros and cons here.

Comment 46

[Kai Engert (:KaiE:)]

Yes, we don't have smart SSL client cert selection logic yet, neither on branch nor on trunk.

FWIW, being smart appears to require that we consider a lot of details. http://wiki.mozilla.org/PSM:CertPrompt

Comment 47

[Kai Engert (:KaiE:)]

Attachment: Patch v1: revert pref for thunderbird, only

(In reply to comment #39)
> This is really what I meant by 2.  I've been assuming, though, that simply
> adding a different setting to all-thunderbird.js would override the one in
> security-prefs.js.  Is that correct?

Indeed, you're right.
Using config editor, I've tested the attached patch overrides the core pref (implementing option 1c from comment 37).

Comment 48

[David Ascher (:davida)]

Correct workaround (to be referenced in relnotes):

1. Select the menu: "Tools|options" 
2. select the sub.menu "Advanced"
3. Select the pane "General"
4. Select the button: "Config Editor" 
5. Set the property
security.default_personal_cert "Ask every time" TO "Select Automatically" 

(make sure you get the case of letters correctly)

Comment 49

[David Ascher (:davida)]

Relnoted for 3.0a1 as:

   If using SSL client authentication, Thunderbird will ask for confirmation too often.  See workaround in <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=431819#48">bug 431819, comment 48.

Comment 50

[Phil Ringnalda (:philor)]

(In reply to comment #45)
> a) The pref code actually notices when a value changes back to the default and
> removes the user value?

Yep, default prefs eat user prefs with the same value. If there were in fact any users who chose Always Ask on their own, that information disappeared the first time they closed 2.0.0.14.

It only becomes a problem if we get a PSM fix in 2.0.0.15 that makes security.default_personal_cert only apply to remote content, because then the unknowable percentage who switch back to automatic will get no benefit from this whole exercise. But since kaie makes it sound like we're unlikely to get any 2.x solution, the problem's moot. Right now, everyone has either a default Always or a user Select, then whenever we ship .15 with a default Select, everyone has a default Select, and if at some future time having a default Always makes sense we can move the people who stay with the default Select over without losing anyone in a position where we caused them to choose less privacy for no reason. Meantime, people with personal certs really need to keep their hands off the [Load Images] button.

Comment 51

[Dan Mosedale (:dmosedale, :dmose)]

As far as The Right Fix, I think the SeaMonkey case demonstrates that having one or two preferences isn't really sufficient here.  I suspect the desired granularity for this control is per-protocol (per port?).  This could potentially still be done with preferences, the way the "expose" preferences work in necko.  But maybe it wants a simpler way to override on a per-connection basis, I'm not sure.

Comment 52

[Phil Ringnalda (:philor)]

Attachment: Trunk: revert pref, add ui, v.1

Maybe I'm just kidding myself, but I like a comment with my pref overrides, warning off people that I don't trust to check blame. Also, for the trunk (where we can), we should add UI for the benefit of people who've learned from having it changed for them that they might want always ask, and for the hypothetical people who already had it changed, who we'll be defaulting back to select automatically. While I was there, I took out our... rather unsemantic use of a <description> to make an <hbox wrap="true"> - judging by Firefox's XUL, which dates back to 1.8.0, there used to be a problem with all four buttons being too much for one row on OS X, but that no longer seems to be a problem, on any platform.

Comment 53

[Phil Ringnalda (:philor)]

Attachment: Branch: revert pref, v.1

Just revert the pref, for 1.8

Comment 55

[dragos]

This is happening also for AddressBook LDAP access. 
When it is needed to use the SSL for authentication of course.

Comment 56

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 320415 [details] [diff] [review]
Branch: revert pref, v.1

r=dmose on the branch version.  Can you put a screenshot in the bug for the trunk patch?

Comment 57

[Phil Ringnalda (:philor)]

Attachment: UI screenshot

Your screenshot, as requested.

Comment 58

[Phil Ringnalda (:philor)]

Attachment: Trunk: revert pref, add ui, v.2

dmose reasonably pointed out that I should indent the radiobuttons, 'cf the "Open Messages In:" piece of Advanced/General' but that markup made me itch, so I made both of them like Advanced/Update, instead.

Comment 59

[Phil Ringnalda (:philor)]

(with a dash of added accessibility, since I was bitrotting one of Marco's patches in my queue anyway)

Comment 60

[Phil Ringnalda (:philor)]

Attachment: Indented UI screenshot

Comment 61

[Marco Zehe (:MarcoZ)]

Accessibility bits look fine, thanks!

Comment 62

[Eddy Nigg (StartCom)]

Did somebody of you actually read the statement above select box? Doesn't make so much sense for mail servers. This is also the mostly used scenario and not html/rss in the message pan.

Comment 63

[dragos]

guys you should add also LDAP in the subject

Comment 64

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 320645 [details]
Indented UI screenshot

I am wondering if we couldn't make the verbiage here a bit easier to grok.  Using this as an excuse to break in our new ui-r flag and get Bryan's thoughts...

Comment 65

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #62)
> Did somebody of you actually read the statement above select box? Doesn't make
> so much sense for mail servers. This is also the mostly used scenario and not
> html/rss in the message pan.
> 

Eddy, are you referring to the security dialog that comes up in the case where this is unpatched?  Or the checkbox in the preferences dialog screenshot that Phil posted?  Or something else?

Comment 66

[Daniel Veditz [:dveditz]]

If you're adding UI to let users set the pref why would you revert the default to the original setting that had the privacy problem?

For the branch I understand the l10n concerns, but given the small number of affected users could we _not_ revert the pref and add the proposed UI? If we use _exactly_ the already-translated text used in the equivalent Firefox UI MoCo should be able to manually copy those specific strings without needing to rely on being able to get the localizers to do it.

Axel: am I insane to suggest this?

Comment 67

[Axel Hecht [:Pike]]

Well, the "server" string we use in en-US would work for mail in English, but it could be translated in a browser-specific context in the firefox prefs. So I'm not sure that the strings naturally match.

Comment 68

[Eddy Nigg (StartCom)]

(In reply to comment #65)
> 
> Eddy, are you referring to the security dialog that comes up in the case where
> this is unpatched?  Or the checkbox in the preferences dialog screenshot that
> Phil posted?  Or something else?
> 

At the preferences...from the screen shot. Remote content in the message pan is usually turned off (I think the default) so HTML and RSS are much less likely. The issue we are all having here is with secured mail servers with protocols like SMTP, IMAP and POP3 over TLS.

Comment 69

[Eddy Nigg (StartCom)]

(In reply to comment #66)
> If you're adding UI to let users set the pref why would you revert the default
> to the original setting that had the privacy problem?
> 

This is a very  bad idea, because we are talking here about mail servers which were explicitly configured by the user and TLS was selected. There is no privacy issue with mail servers, there is no drive-by-snooping-cookie-like issue as with the web. 

> For the branch I understand the l10n concerns, but given the small number of
> affected users 

Do you have some specific information about your claim that it's a small number of users? Perhaps some statistics we can review somewhere?

> If we
> use _exactly_ the already-translated text used in the equivalent Firefox UI
> MoCo should be able to manually copy those specific strings without needing to
> rely on being able to get the localizers to do it.
> 

Firefox is for the web, it doesn't match the intended purpose for mail at all. TB is not a browser. I think the current string (from the screen shot) and/or applying that of FF rather confusing, certainly for less savvy users.

Comment 70

[Phil Ringnalda (:philor)]

(In reply to comment #67)
> Well, the "server" string we use in en-US would work for mail in English

Wrong branch. 1.8 Fx uses "site Web"/"Website"/"sothungelwano" (I think, from context)/"witryna" for the en-US "When a web site requires a certificate:"

Comment 71

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 320645 [details]
Indented UI screenshot

FF uses a succinct message: "When a server requests my personal certificate"

I'd like to see our messaged toned down to: "When a mail or web server requests my person certificate"

Since we don't have a mail vs. web setting right now we need to keep it short and to the very important points.  Mail, Web, and Your Personal Certificate.  What do you think?

I'm not so sure about the indent.  Can we change the radio buttons to be vertically listed on different lines? Then I'd like the indent, otherwise I think it should line up with the text.

i.e.
(o) Select one Automatically
( ) Ask me every time

Comment 72

[Bryan Clark (DevTools PM) [:clarkbw]]

arg you tiny comment box!

That should read: "When a mail or web server requests my personal certificate:"

And have indented the example:

When a mail or web server requests my personal certificate:
  (o) Select one Automatically
  ( ) Ask me every time

Comment 73

[Eddy Nigg (StartCom)]

That's a good suggestion! +1

Comment 74

[Axel Hecht [:Pike]]

re comment 72:

grammar nit.

And it's not gonna make it onto the branch due to l10n impact.

Comment 75

[Phil Ringnalda (:philor)]

(In reply to comment #66)
> If you're adding UI to let users set the pref why would you revert the default
> to the original setting that had the privacy problem?

Comment 50. I don't have any employees, much less any employees that I can order to patch core security UI, particularly without even knowing what branch will be Tb3, so I can only plan for things I can do, and hope for things I don't control to make it obsolete.

If nothing changes, then we can certainly ship a default Always Ask with UI that explains the risks of changing it back to Select Automatically (well, assuming I can persuade my reviewers that the UI matters in ways other than minimizing word count), but to be able to do that, we have to ship a default Select Automatically, to collect all the users we've gotten off the default value by relnoting them into getting away from our new Always Ask default and switching to a user value instead.

Comment 76

[Phil Ringnalda (:philor)]

(In reply to comment #68)
> Remote content in the message pan is
> usually turned off (I think the default)

You keep saying that, as though remote content was something that happens to other people. It is not.

Try this experiment for me: go to http://blogsearch.google.com/ and search for "StartCom", as hundreds of thousands of people have done with their company or their name or their blog URL, then on the results page click the feed icon in the addressbar, to load http://blogsearch.google.com/blogsearch_feeds?hl=en&q=startcom&ie=utf-8&num=10&output=rss (the RSS feed for your search). Then, in Thunderbird, create a "RSS News & Blogs" account if you don't already have one, and subscribe to that URL, accepting the default unchecked state of the "Show the summary instead of the web page" checkbox.

At this point, when you click on the first item (assuming it ends up in the same order as the feed), you'll load remote content, and when you click on the second item you'll load malicious remote content. In this case, as far as I know it will just be maliciousness aimed at search engines, rather than at you, but if I had an SSL web server set to phish for personal certs that chain up to StartCom, I'd certainly spend the five minutes it would take to drop an image into a spam blog with the company name, and the names of any employees I could find, and any customer names I could find from testimonials in advertising or weblog posts mentioning the company since they might be customers and it doesn't cost me anything to try.

Remote content is *not* something that happens to other people, or only to foolish people. It's something that happens to people who use our feed support, and it's something that happens to people who click the Load Images button in individual emails, and it's something that happens automatically to anyone who gets forged email claiming to be from someone for whom they got tired of clicking the Load Images button, and decided to always load their images, with no idea that doing so would expose all the information in their personal cert to anyone who could guess that sender's address.

Comment 77

[Phil Ringnalda (:philor)]

(In reply to comment #71)
> FF uses a succinct message: "When a server requests my personal certificate"

Fx has a very different situation: other than maybe AUS update pings, which are rather outside the user's control, all the requests it sends to servers are roughly the same level of trust, fairly little. There are times when the user trusts the server, but that isn't really conveyed down into the code. We, however, have (at least) two very strongly divided classes: we talk to mail servers that we trust a fair amount, and that are the primary (or exclusive, in Eddy's case) thing that our users think we talk to, and we also talk to other servers (web servers for remote content in mail and feeds, LDAP servers that maybe the user trusts completely, or maybe are just some random directory he thinks he's using anonymously), which we distrust a great deal.

> I'd like to see our messaged toned down to: "When a mail or web server requests
> my person certificate"

Please, can we not? There are three possible cases for this UI: someone might buy us a pony in PSM (and that's exactly how much we should plan on it), in which case it'll be obsolete and I'll laugh while I delete it; we might decide that we prefer convenience to privacy, and just ship Tb3 with Select Automatically as the default, in which case it really doesn't much matter, because the only people who will see it are the people who've read an article about how "Thunderbird by default is a slut that will tell any server on the planet all the personal details from your cert" and they'll have already decided what to do without needing our description, and will be able to (safely) see the results of their decision, as every sort of contact with a cert-requesting server prompts them to select a cert; or, where I'm headed, we might need to ship Tb3 with Always Ask set as the default even though it Always Asks every time you connect to your mail server, in which case my extra ten words are the only thing standing between our users' desire to have the stupid prompts shut up and any feed or From: forger being able to have their personal information.

> and to the very important points.  Mail, Web, and Your Personal Certificate. 

Ask Eddy whether "or web" will be enough to persuade him that he needs to pay attention to and consider the impact of Thunderbird contacting web servers with his personal cert. He may not have drawn the correct conclusion from my phrasing, but at least he noticed it. 

> I'm not so sure about the indent.  Can we change the radio buttons to be
> vertically listed on different lines?

We can indeed, that's actually the default for the widget. I was just looking at and matching Advanced - General rather than Advanced - Update. Lacking any other explanation for why we choose one way over the other, we may well choose horizontal when we're worried about how much space we have to spare on a pane (though that doesn't explain why Fx chose horizontal for their version of this same UI).

Comment 78

[Eddy Nigg (StartCom)]

(In reply to comment #76)
> 
> You keep saying that, as though remote content was something that happens to
> other people. It is not.
> 
> Try this experiment for me: go to http://blogsearch.google.com/ and search for
> "StartCom", as hundreds of thousands of people have done with their company or
> their name or their blog URL, then on the results page click the feed icon in
> the addressbar, to load
> http://blogsearch.google.com/blogsearch_feeds?hl=en&q=startcom&ie=utf-8&num=10&output=rss
> (the RSS feed for your search). Then, in Thunderbird, create a "RSS News &
> Blogs" account if you don't already have one, and subscribe to that URL

You've got a point here, however this is an action you performed explicit (and apparently you know what you are doing). This isn't really a drive-by / browse-by situation stealing your details.

Now, YOU are the power-user and I'd expect you to handle the settings accordingly. However how many casual users are using SMTP over TLS AND copy RSS news feeds into TB? Interestingly I'm making now a similar argument which was brought against me when we discussed fixing exactly this one in Firefox. However I view the situation in TB differently:

In Firefox I'm not pinged every time I want to view a page for a certificate, whereas in TB every message I send would have that. That's very annoying and a casual user might even have a problem understanding the question about selecting a certificate. Which certificate? I don't have any...

In Firefox this happens rarely after all and usually for the purpose of authentication. See for example https://www.startssl.com/?app=12 (you need to register first). Those are the legitimate situations for this use. In Thunderbird you connect to the mail server constantly (or remain connected) and every message sent, begs for a certificate...

I'm very security minded, but also take into account usability. In this case you'd make a disservice to millions of users, weighing against the security implication. Mail services over TLS are becoming the standard and we must handle this reasonable. If 99% of the users would have to revert the default preference in order to make the product usable than something is wrong. In this specific case I believe this to be the situation (for all users with protocols over TLS). At least until we have the capability to differ between mail protocols and the web services.
 

>...
> and it's something that happens to people who click the Load Images button in
> individual emails, and it's something that happens automatically to anyone who
> gets forged email claiming to be from someone for whom they got tired of
> clicking the Load Images button, and decided to always load their images, with
> no idea that doing so would expose all the information in their personal cert
> to anyone who could guess that sender's address.
> 

I think what you just said should encourage to fix this bug properly and distinguish between mail and web. We really don't want to produce a product which is unusable in its default configuration. As much as I agree with you [*] what this issue concerns for everything related to the web services, I have a problem when usability is impacted to such extend.

[*] To make my point, I blogged about this issue in relation to Firefox here: https://blog.startcom.org/?p=36
Please note that this was way before it was accepted as a bug (See bug 395399) 

Comment 79

[Robert Kaiser]

(In reply to comment #77)
> We,
> however, have (at least) two very strongly divided classes: we talk to mail
> servers that we trust a fair amount, and that are the primary (or exclusive, in
> Eddy's case) thing that our users think we talk to, and we also talk to other
> servers (web servers for remote content in mail and feeds, LDAP servers that
> maybe the user trusts completely, or maybe are just some random directory he
> thinks he's using anonymously), which we distrust a great deal.

That's why I think a solution that gives the same amount of trust to all servers is no real solution at all (not even for SeaMonkey, BTW). If we warn _once per server_ and then save the fact that we trust _this one server_ (at least for the session, maybe even permanently) it's a good idea. AFAIK, the mechanism for cert exceptions does exactly that. Why can't the mechanism we use here do this?

Comment 80

[Eddy Nigg (StartCom)]

This could be also an interesting approach, provided it's on a "per-server" basis.

Comment 81

[Kai Engert (:KaiE:)]

(In reply to comment #71)
> (From update of attachment 320645 [details])
> FF uses a succinct message: "When a server requests my personal certificate"
> 
> I'd like to see our messaged toned down to: "When a mail or web server requests
> my person certificate"


Mail and Web are not the only kind of servers.
We have LDAP servers, IRC servers, and your-companys-application-server.
So my vote would be to stick with a generic string that works with any kind of server.

Comment 82

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #77)
> > I'd like to see our messaged toned down to: "When a mail or web server requests
> > my person certificate"
[snip]
> Asks every time you connect to your mail server, in which case my extra ten
> words are the only thing standing between our users' desire to have the stupid
> prompts shut up and any feed or From: forger being able to have their personal
> information.

We cannot make the assumption that more words to explain situations means more security (10 words insecure vs. 20 words secure?).  There are countless usability/security research programs that have proven this is an incorrect assumption and the opposite is actually more likely correct.

The wording needs to stay short and to the point.  Of course my grammar and clarity are often very poor.  But if we are trying to adopt different wording to improve security we have failed and need to approach the problem differently.

Like Robert writes in comment 79 a per-server, at least per-session approach could be the kind of change in approach we need.

Comment 83

[Paul Scott]

(In reply to comment #81)
<snip>
> Mail and Web are not the only kind of servers.
> We have LDAP servers, IRC servers, and your-companys-application-server.

And each have their own *different* security issues.

> So my vote would be to stick with a generic string that works with any kind of
> server.

How helpful would that be?

I would argue that in the case of TB you should not have to identify yourself
to the mail server with a custom certificate when using SSL, and hence no
message is warranted or desired.

Comment 84

[Eddy Nigg (StartCom)]

(In reply to comment #83)
> I would argue that in the case of TB you should not have to identify yourself
> to the mail server
> 

Servers use this increasingly for authentication as well.

Comment 85

[Paul Scott]

(In reply to comment #84)
> (In reply to comment #83)
> > I would argue that in the case of TB you should not have to identify yourself
> > to the mail server
> > 
> 
> Servers use this increasingly for authentication as well.
> 

you surreptitiously truncated my statement, which said "I would argue that in the case of TB you should not have to identify yourself to the mail server *with a custom certificate* when using SSL"

By this I mean it is not necessary to provide a custom certificate every time you connect. Everything necessary to provide a secure connection was provided in the account setup parameters. There is no need for prompting, which adds no additional security, but more than a little irritation. 

Comment 86

[Eddy Nigg (StartCom)]

OK, sorry! Got it...

Comment 87

[Dan Mosedale (:dmosedale, :dmose)]

Kai: do you agree that the existing single-preference is the wrong granularity here?  What do you believe the right thing to do inside of PSM is, both regarding APIs, preferences, and UI?

Comment 88

[Kai Engert (:KaiE:)]

(In reply to comment #87)
> Kai: do you agree that the existing single-preference is the wrong granularity
> here?  

yes


> What do you believe the right thing to do inside of PSM is, both
> regarding APIs, preferences, and UI?

In this comment I want to focus on the stable branch (TB 2), where only minimal changes are possible.

Also, I think our solution must be acceptable to Firefox 2, at the same time, because the SSL client auth code happens in Core PSM. I would like to avoid having TB specific code here, in order to avoid duplicate work.

I think reverting the pref is not good, because:
- it re-introduces the attack vector, even if it seems a bit less likely
  to be abused.
- it does not help SeaMonkey users. yes, they can revert to "always ask",
  but those who use client auth certainly don't want to

IMHO, we should have a solution that prompts "remember this decision"
once per server:port combination.

Unfortunately, we can't do a permanent remember, across sessions.
Because we can not add new UI to erase such stored decisions.

I'd prefer a solution that:
- keeps our pref at "always ask"
- re-define "always ask" to mean "at least ask once per user session"
- add a checkbox to the client auth select cert prompt which says
  "remember this decision"
- (we should discuss whether it shall be on or off by default)
- the mozilla/dom directory on a branch already has a string
  "remember this decision".
  Unfortunately that string also contains the word "always", which
  is not completely true with my proposal to limit remembering to the session.
  Using that string is a compromise, as it avoids l10n changes.
- Implement a little new service, that could be quite similar to the code
  that we added on trunk for FF 3, the "cert override service".
  Have a list of mappings from host:port to (client cert) that have been 
  remembered.
  (Bob Relyea thinks, it might be sufficient to remember selections
   based on certificate, only, IIUC, but I'm afraid this would take
   away the flexibility that the "always ask" pref offers,
   too much for a stable branch, IMHO)

As a result, users who use client auth would be prompted at most once for each server:port they are connecting to, per session.

Comment 89

[Phil Ringnalda (:philor)]

Ponies it is, then!

Comment 90

[Eddy Nigg (StartCom)]

(In reply to comment #88)
> As a result, users who use client auth would be prompted at most once for each
> server:port they are connecting to, per session.
> 

Kai, that's a lot of prompts...That's at least one for the software security device (in trunk it's currently for every account), one for the smart card (if you got them), one for SMTP and one for IMAP/POP3 (maybe also for each account?). Under the best circumstances at least 3 prompts at every startup (more prompts for every account)...is that usable?

Comment 91

[Kai Engert (:KaiE:)]

(In reply to comment #90)
> Kai, that's a lot of prompts...That's at least one for the software security
> device (in trunk it's currently for every account), one for the smart card (if
> you got them), one for SMTP and one for IMAP/POP3 (maybe also for each
> account?). Under the best circumstances at least 3 prompts at every startup
> (more prompts for every account)...is that usable?

Eddy, your comment confuses me.

As I understand it:
- this bug is not about dropping the (one time) prompts for smart card
- this bug is not about dropping the (one time) prompt for your 
  software security device password

My proposal is independent of the choice to go to config editor and switch to "select automatically" (if you prefer to avoid the ssl client auth prompts and prefer to be at risk).

This bug and my proposal is about stopping the "repeated" prompts, while still keeping our settings at a default that protects unedcuated end users.

Yes, if you are using ssl client auth for both your mail-receive and your mail-send server, then my proposal would result in 2 prompts per session - unless you go to config editor and switch to "select automatically".

Comment 92

[Kai Engert (:KaiE:)]

(In reply to comment #85)
> By this I mean it is not necessary to provide a custom certificate every time
> you connect. Everything necessary to provide a secure connection was provided
> in the account setup parameters. There is no need for prompting, which adds no
> additional security, but more than a little irritation. 

We are not proposing to force the end user to deal with a prompt for every kind of server.

This is only about the limited number of servers where the operator of the server has explicitly configured it to request or require a client authentication certificate from the end user (which excludes servers who operate with password based authentication).

And as of today, the account setup configuration UI does not include a choice for selecting the preferred personal certificate to use for authentication (and we can't add that on the stable branch for Thunderbird 2).

Comment 93

[Eddy Nigg (StartCom)]

Kai, I'm using TB as a....mail client! But my personal preference isn't the debate here, rather that of a typical user. Assuming that mail protocols over TLS are and will be more common (even Google has it: http://mail.google.com/support/bin/answer.py?hl=en&answer=13287 ) I'm looking at how many prompts one can get (from the usability POV):

1. Software security device
2. Sending email, SMTP over TLS (at least once per server and session)
3. Receiving mail, POP3/IMAP over TLS (at least once per server and session)

Repeat 2 and 3 for each account.

That sums up potentially to many prompts. Supposed I'm a user switching from a different product, I'd switch back after the 10th nagging, specially since my preference wouldn't be stored but come back *every time* I start it. Not such a good idea IMO.

Analyzing the risk to TB users which use it first and foremost for mail, with all server settings to be configured manually, there isn't a situation where information can be harvested on-the-fly and the browsing habits recorded (I understand that there is a certain problem with news feeds/RSS, however Firefox handles them as well).I view the risks highly minimized compared to Firefox.

Now in Firefox, the chance to be prompted for a client certificate is minimal. Since it's for authentication purpose, instead of entering user/pass information one clicks at the right client certificate, which I think is less intrusive. Not comparable to what is supposed to happen here with TB.

Comment 94

[Eddy Nigg (StartCom)]

(In reply to comment #92)
> And as of today, the account setup configuration UI does not include a choice
> for selecting the preferred personal certificate to use for authentication (and
> we can't add that on the stable branch for Thunderbird 2).
> 

Obviously I understand the limitations for TB2, hence we should revert to what it was previously. The risks involved are IMO not comparable to that of FF and a better solution can be backed for TB3. IMO, this is what we should do in a pragmatic way.

Comment 95

[Eddy Nigg (StartCom)]

(In reply to comment #92)
> This is only about the limited number of servers where the operator of the
> server has explicitly configured it to request or require a client
> authentication certificate from the end user (which excludes servers who
> operate with password based authentication).
> 

Sorry for the multiple replies, but also here a counter-argument:

Users which don't require client auth are not affected by this and hence don't have to be taken into consideration and your argument is not relevant. We are talking about the users which ARE affected by this measure and most of them don't want to get prompted multiple times at every startup. Most likely all of them will revert to "don't ask" because it's otherwise unusable. My users would be affected and your proposal would be extremely annoying.

Comment 96

[Kai Engert (:KaiE:)]

(In reply to comment #93)
> Assuming that mail protocols over
> TLS are and will be more common (even Google has it:

Does that argument count? We are not talking about all SSL/TLS enabled mail servers, but only about a subset.


(In reply to comment #94)
> Obviously I understand the limitations for TB2, hence we should revert to what
> it was previously. The risks involved are IMO not comparable to that of FF and
> a better solution can be backed for TB3. IMO, this is what we should do in a
> pragmatic way.

But your proposal to revert the pref for TB does not work for SeaMonkey users.


(In reply to comment #95)
> We are
> talking about the users which ARE affected by this measure and most of them
> don't want to get prompted multiple times at every startup.

We can tell users who are annoyed about this in the release notes of TB 2.0.0.15 and explain how to revert the pref using config editor.

We could even add a help button to the "client auth prompt" that goes to a web page with the details about this, explain the risk and give the explanation about how to disable the prompt.


> Most likely all of
> them will revert to "don't ask" because it's otherwise unusable.

They are still able to do that with my proposal.


> My users would be affected and your proposal would be extremely annoying.

My proposal is better than what we have right now, that's all I'm trying to achieve.

Comment 97

[Eddy Nigg (StartCom)]

(In reply to comment #96)
> 
> But your proposal to revert the pref for TB does not work for SeaMonkey users.
> 

Correct. I think that for Seamonkey a different solution might be better and most likely they don't have the same limitations in relation to releases. However I'm not really into Seamonkey and prefer not to make any advice for this product.

> 
> We can tell users who are annoyed about this in the release notes of TB
> 2.0.0.15 and explain how to revert the pref using config editor.
> 

There was just recently a discussion somewhere about "telling users" and it was viewed that most users aren't reached. Don't remember where it was exactly. Certainly new users are the ones mostly unreached nor is it really easy to do that on TB for those.

> My proposal is better than what we have right now, that's all I'm trying to
> achieve.
> 

Yes of course, you have the best intentions. Still, I'm the opinion that for TB2 we should simply revert back, for TB3 find a better solution. The former would be realistic in terms of code changes, usability and risk assessment. The later is something we should discuss in the correct context (i.e. many possible options before TB3 release). Enough said, lets make a decision and get along with it.

Comment 98

[Robert Kaiser]

(In reply to comment #97)
> Enough said, lets make a decision and get along with it.

Exactly. I think What Kai proposes is reasonable for branch, and trunk can go a similar way with better UI to make this even better.
The "typical user" you pointed to doesn't need a personal certificate for even one server nowadays, but we surely have a number of users out there who might need them. I think they can live with a once-per-session-and-server prompt, which is better than what they have right now on Thunderbird 2.0.0.14 (and incidentally also SeaMonkey 1.1.9).
For trunk, we can even add UI for permanently remembering this and never re-prompting for the same server/port+cert combination. If authentication with personal certs becomes more popular in the Thunderbird 3 time frame, we'll also have this better solution for its users.
And those who want to revert to the old behavior still can with the explanation they'll find in release notes.

I think this is the best solution we can offer with providing the best security by default and not make things too annoying for the small group of users who really run into this - so please let's move forward and try to go this way!

Comment 99

[Kai Engert (:KaiE:)]

I propose by default we set the "remember this decision" checkbox to mean "yes remember".

We can introduce a new hidden pref "ssl_client_auth_prompt_remember_checkbox_checked" which we set to true by default.

Those few power users who, for some reason, really need the old behavior to "always ask" (even repeatedly during the session), and would have to always manually deselect the checkbox, can change that pref.



(In reply to comment #97)
> There was just recently a discussion somewhere about "telling users" and it was
> viewed that most users aren't reached. Don't remember where it was exactly.
> Certainly new users are the ones mostly unreached nor is it really easy to do
> that on TB for those.

While discussing possible solutions for this bug, "tell users" had been considered as an option for solving this bug.

I'm not proposing that. What I mean is: If we go with the code change, this will happen with a new release, like TB 2.0.0.15. And we can add additional information to the release notes of that new release. I think that's a good channel to let power users know about our offerings and new options.

I've started to implement my proposal.

Comment 100

[Kai Engert (:KaiE:)]

I find it unfortunate that as part of thunderbird's "UI reduction strategy" an important menu comment got removed:
The equivalent of "Tools / Password Manager / Log Out" available in seamonkey, and "Tools / Clear Private Data / Authenticated Session" in Firefox.

Would you agree to add something back to Thunderbird 3?

If we do, we can have that menu command remove the list of remembered client cert selections, too.
I want to add that action to core code, so Firefox and SeaMonkey will already benefit from this possibility.

Comment 101

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Comment 102

[Kai Engert (:KaiE:)]

Attachment: screen shot for v4: prompt with new checkbox

Comment 103

[Kai Engert (:KaiE:)]

Attachment: screen shot for v3: proposed new prefs

Comment 104

[Kai Engert (:KaiE:)]

Explanation of behavior (all apps) with patch v3:

A new checkbox is shown, as proposed earlier, see attachment 321376 [details].

I propose to use the default "checkbox is enabled", so if users simply confirm the dialog, they won't get prompted again for this session.


This has a disadvantage for power users, who (for whatever reason) indeed want the "ask every time", because they now require an additional click.

In order to provide a workaround for those power users, I've added a pref:
   "security.remember_cert_checkbox_default_setting"
Power users can set this to false to get the unchecked checkbox, equivalent to the former behavior.


We know that "canceling" the dialog means:
  "Connect without selecting a certificate."
I think this decision (no cert) should be remembered, too.

The patch will evaluate the setting of the checkbox independently of whether "confirm" or "cancel" action was used to dismiss the dialog.

In other words, if the user cancels, and the checkbox was checked, you won't get prompted again during the session.


In Firefox and SeaMonkey, you can use the logout action (as described in comment 100) to "forget" all remembered ssl client auth decisions.


I think that some power users might require the ability to select different certificates for each server and each port.

Although this might seem a rare scenario, I think we must not take away this ability on the stable branch.

But for convenience, I propose that by default, we remember the selection for all ports of a single host. (This should be fine, because by default, the remembered selection is tied to the server certificate, too. In other words, if there are two or more ports running on one server, with different server certs, our default can not cause harm. The remembered selection for the first port will be ignored when we connect to the second port and discover it uses a different cert.)

As a result of this default, when a single server runs both IMAP and SMTP, on different ports, and both use client auth, you'll only get prompted once.


Those users who need to work with individual selections per port can use the new pref
  security.remember_cert_checkbox_per_port
which is "false" by default, and switch it to "true".


The selection could become smarter in a future patch, maybe based on cert, but for now, I think this is a good first step.
Because there is no storage on disk, we can easily change our implementation to use a key different than host:port (and switch to a cert based key) later, if desired.

Comment 105

[Kai Engert (:KaiE:)]

(In reply to comment #104)
> I think that some power users might require the ability to select different
> certificates for each server and each port.

I mean for different ports on a single server


> Although this might seem a rare scenario, I think we must not take away this
> ability on the stable branch.
> 
> But for convenience, I propose that by default, we remember the selection for
> all ports of a single host. (This should be fine, because by default, the
> remembered selection is tied to the server certificate, too. In other words, if
> there are two or more ports running on one server, with different server certs,
> our default can not cause harm. The remembered selection for the first port
> will be ignored when we connect to the second port and discover it uses a
> different cert.)
> 
> As a result of this default, when a single server runs both IMAP and SMTP, on
> different ports, and both use client auth, you'll only get prompted once.


While I explained this, I realized that this thinking is broken.
Those few users who have different services with different certs on a single server are unable to benefit from this patch, because the per-host remembered setting would repeatedly get overwritten when doing alternating connections between the ports.


I come to the conclusion that using
  server:cert
is a better key.

I want to work on an updated patch, remove the "per port" pref, and switch the index key, thus supporting any combination of services on a single host (same or different certs).

Comment 106

[Kai Engert (:KaiE:)]

Comment on attachment 321378 [details]
screen shot for v3: proposed new prefs

obsolete screenshot. Will only the single new pref remember_cert_checkbox_default_setting

Comment 107

[Kai Engert (:KaiE:)]

Comment on attachment 321376 [details]
screen shot for v4: prompt with new checkbox

this screen shot is still valid for patch v4

Comment 108

[Kai Engert (:KaiE:)]

Attachment: Patch v4 for 1.8 branch (TB2 / FF2)

Changed as described.
No longer look at port numbers.
Remember selection for each combination of {hostname, certificate}

I've tested this patch with Firefox 2 branch and:
- https://kuix.de/misc/test6/
- https://kuix.de:9443/misc/test6/

(I used a cert from my test CA for testing the URL at port 9443)

Comment 109

[Kai Engert (:KaiE:)]

Hmm, sorry, maybe it hasn't been a good idea to reassign this bug to Core/Security:PSM, because in that process I lost the blocking-thunderbird flag.

But actually, I think this bug is really a core bug.
I would like to propose that I merge this patch to Mozilla 1.9 and that we land it for Firefox 3.0.1 and Thunderbird 3 (and SeaMonkey 2) as well.

Comment 110

[Eddy Nigg (StartCom)]

(In reply to comment #105)
> I come to the conclusion that using
>   server:cert
> is a better key.
> 

Yes, much better! Waiting for the patch to land...

Comment 111

[Kai Engert (:KaiE:)]

Attachment: Patch v4 for trunk (TB3 / FF3)

While attachment 321387 [details] [diff] [review] is for FF 2 / TB 2 / SM 1.1,

this new attachment is the same patch adjusted for trunk for FF 3 / TB 3.


Please feel free to apply these patches to your local builds, do some testing and give feedback.
Thanks.

Comment 112

[Kai Engert (:KaiE:)]

Comment on attachment 321387 [details] [diff] [review]
Patch v4 for 1.8 branch (TB2 / FF2)

Asking Bob for review, let's start with 1.8 branch (it's more urgent, and trunk is frozen anyway)

Comment 113

[Robert Relyea]

Comment on attachment 321387 [details] [diff] [review]
Patch v4 for 1.8 branch (TB2 / FF2)

r+ with a couple of comments.

1. XPCOM and UI are not my strong points, it might be useful to have another reviewer look at those usages.

2. You have two redundant uses of monitors, one in nsClientAuthRememberService:: Observe()
 and the other in nsClientAuthRememberService::RememberDecision

In the first case you grab the monitor, then call a function which is completely protected by that same monitor. Since the call was not conditioned on any variable referenced under the monitor, the first monitor is redundant.

The second is less clear, there is a conditional in the call, and there is code inside AddEntryToList(), but it's relatively easy to show both are stack variables can cannot be affected by other threads, so I believe this moniter is redundant as well.

Since you are using monitors and not locks, there is no bug (you are protecting a few extra instructions that you don't need to), so it's only a minor effeciency issue, thus ther r+

bob

Comment 114

[Kai Engert (:KaiE:)]

Attachment: Patch v4 for 1.8 branch (again), now with -u10 -p

Comment 115

[Nelson Bolyard (seldom reads bugmail)]

What server product has this behavior, that it requests client authentication
on every connection?

That is a server that has been misconfigured.  It's SSL/TLS session cache has
been disabled, or is broken, or has an absurdly short session timeout time.

Let's not be quick to fall on our swords over this.  Let's push on the server
vendors or server admins to fix their SSL session caches.  

Comment 116

[Eddy Nigg (StartCom)]

Nelson, even if it's a server problem, it would make TB unusable. As it happens, sometimes we have to work around (similar to the need to fetch and cache intermediate CA certificates as in bug 399324). I'm using standard sendmail, cyrus-imapd and dovecot (pop3,imap) servers over TLS and experienced the problem at different servers and implementations.

Obviously you are free to push for better implementations at server vendors, but in the meantime we need to have a workable product with these servers. Don't know about Exchange (MS), but expect the same behavior.

Comment 117

[Nelson Bolyard (seldom reads bugmail)]

Eddy, don't blow off my question.

There is a preference, and the few people who have this problem can choose it,
IF they can't get their server admins to fix their servers.  If you want to 
choose automatic cert choice every time, be my guest.  

Did you notify the sysadmins of the servers that affected you of their
misconfiguration?  Or is it merely easier to ask Mozilla to change than to 
ask those sysadmins to fix their configurations?  

I find that the Mozilla community holds itself in low respect, and is quick 
to blame itself rather than others when an interoperability issue is found.  
This attitude leads to disrespect from others. Personally, this offends me.  

It really is a huge win for server admins to fix their servers.  Their
CPU cost per connection will go WAY down, to less than 1/4 of what it is 
with the bad configuration.  

So, let's not cave in on the first complaint, and change the default for
ALL users, so that the few users who are victims of misconfigured servers
can avoid flipping a pref.  

The real UI issue here, for the mail/news clients, is that there seems to 
be no UI with which to configure the client's response to cert authentication
requests from servers.  The only choices seem to be:

a) choose automatically every time for me (which often chooses the "wrong" cert, that is, a cert that doesn't help the user), or 
b) ask me every time.

There should also be choices that say: 
"For this email account on this server, use this certificate every time.  
Do not make some automatic choice, and do no ask me, but rather, honor this 
choice being made here in this pref, every time."

That's the best solution to this issue.  But it requires TB UI work.

Comment 118

[Eddy Nigg (StartCom)]

(In reply to comment #117)
> Eddy, don't blow off my question.
> 

Nelson, you know that I love you :-)

(is that inappropriate to say on bugzilla?)

>  If you want to choose automatic cert choice every time, be my guest.  
> 

I would love to do that on TB (as I'm not browsing the web with TB and deny remote content). As a matter of fact we've been doing exactly this until we fixed this issue in FF (despite yours and Robert's initial opposition, see bug 395399) and I'm still doing it that way.

> Did you notify the sysadmins of the servers that affected you of their
> misconfiguration?  

That would be *me* in all my glory, for ten years or so...happens on all my mail servers, using AS-3 (older version) and AS-5.

> Or is it merely easier to ask Mozilla to change than to 
> ask those sysadmins to fix their configurations?  

1.) I'm not convinced yet that this is a server issue. Connection created, client certificate requested, connection closed. New connection, new client certificate request...It works that way with Apache for the web and apparently it works that way with sendmail too. IMAP is a persistent connection and hence asks only once.

2.) I'm not aware of anything I could do to influence that on the server side. I there is I'd be glad to know.


> I find that the Mozilla community holds itself in low respect, and is quick 
> to blame itself rather than others when an interoperability issue is found.  
> This attitude leads to disrespect from others. Personally, this offends me. 

I look at it rather the pragmatic way. I want a product that worksforme. I want another few million users experience the same. 

> The real UI issue here, for the mail/news clients, is that there seems to 
> be no UI with which to configure the client's response to cert authentication
> requests from servers.  The only choices seem to be:
> 
> a) choose automatically every time for me (which often chooses the "wrong"
> cert, that is, a cert that doesn't help the user), or 
> b) ask me every time.
> 
> There should also be choices that say: 
> "For this email account on this server, use this certificate every time.  
> Do not make some automatic choice, and do no ask me, but rather, honor this 
> choice being made here in this pref, every time."

I think that Kai has covered this partly in his patch (per session for now) and it was agreed to find a better solution for the longer term. I certainly agree with your suggestion from above. That's exactly what should be done.

Comment 119

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #118)

> 1.) I'm not convinced yet that this is a server issue. Connection created,
> client certificate requested, connection closed. New connection, new client
> certificate request...It works that way with Apache for the web and apparently
> it works that way with sendmail too. 

Then those servers are not using an SSL server session cache.  That's very 
poor configuration.  Maybe it's bad programming.  But SSL was designed to 
work more intelligently than that.  All those extra requests are a total
waste of CPU power and time.

> IMAP is a persistent connection and hence asks only once.

IMAP servers can have multiple connections, and I gather than one connection
per folder is typical.  There's no need for each of those connections to 
request client auth.  

The first time your client connects to the server, the server requests 
client auth.  The server and the client both establish a session.  
The server produces a session number and sends it securely to the client.  

The second time the client connects to the server, the client sends a "hello" 
message that says "Last time we connected, we established this session; let's
reuse this session".  The server makes a choice: reuse it (including using 
the authentication information previously established for the session) and 
take the fast path, or start a new session, and go through all the expensive 
RSA computations again, and request client authentication again.  

The reuse of an existing session is not vulnerable to MITM attacks, because 
in order to successfully reuse it, both sides must prove to the other that 
they each have the session key from the former session, something that only 
the previously authenticated parties would have.  

The CPU cost of establishing a new session is much greater (maybe 10x) than 
the cost of reusing an existing session.  It is this capability that gives
SSL a tremendous potential performance advantage over other encryption 
protocols (such as SSH).  But if the server refuses to ever restart existing
sessions, and always starts a new one, then the performance advantage is 
wasted, and the user is annoyed.  

> 2.) I'm not aware of anything I could do to influence that on the server side.
> I there is I'd be glad to know.

The use of the server session cache should be a simple configuration variable,
or perhaps a small set of variables.  The tunable values would include:
a) the maximum SSL session lifetime, typically in seconds or minutes, and 
b) the maximum amount of memory (or disk space) to devote to storing that 
session cache information.  

In Apache with mod_ssl, the configuration parameters are: 
SSLSessionCache and SSLSessionCacheTimeout.  
I suggest a timeout time of several hours, say 14400 or 28800 seconds. 
See http://www.modssl.org/docs/2.8/ssl_reference.html#ToC5

According to http://www.linuxmanpages.com/man5/imapd.conf.5.php :
"tls_session_timeout: The length of time (in minutes) that a TLS session 
will be cached for later reuse. The maximum value is 1440 (24 hours), the 
default. A value of 0 will disable session caching."

> I look at it rather the pragmatic way. I want a product that worksforme. 

Then I suggest that you configure your server so that you and your users get 
the most out of it.

Comment 120

[Eddy Nigg (StartCom)]

(In reply to comment #119)
> The CPU cost of establishing a new session is much greater (maybe 10x) than 
> the cost of reusing an existing session.  

Well, CPU cycles isn't my particular concern here as long as we can waste them on spamassassin, the annoyance for users is.

> In Apache with mod_ssl, the configuration parameters are: 
> SSLSessionCache and SSLSessionCacheTimeout.  
> I suggest a timeout time of several hours, say 14400 or 28800 seconds. 
> See http://www.modssl.org/docs/2.8/ssl_reference.html#ToC5

No dice and never was. I can show you around if you want (by priv. mail).

> According to http://www.linuxmanpages.com/man5/imapd.conf.5.php :
> "tls_session_timeout: The length of time (in minutes) that a TLS session 
> will be cached for later reuse. The maximum value is 1440 (24 hours), the 
> default. A value of 0 will disable session caching."

As indicated above, IMAP is not of a concern, except the annoying first request (for each account!!!). Restart TB a few times a day with some 10 IMAP accounts and you start to understand how stupid it is.

Sendmail is another story yet...not sure about other MTAs.

Comment 121

[DEXTER]

Oh god!

I have to say that you guys are crazy (not all of you of course). This bug was started 1 month ago, and it is still in NEW state?

Open source thing is good.. most of the time. But now... I think this just sucks. I'm always installing all sec updates when it arrives, but a month ago I realized that every 5 min. (yes it is set up to pull mails 5mins) I got a window saying choose a cert. What?? Every 5 min I have to click on a button? WTF? Than I had to google to find some solution. For some days I could only find this bug, but the first solution came up 3 days later.

And now Nelson... you starting it over again? Saying that it is not TB's problem it is a server problem? As Eddy sad it happens all of his mail servers for ten years or so...

Are you nuts? How much time will it take to finally close this bug, and to release another update? Monts? Years? Till than you all will just talking about server configurations?

...

Comment 122

[Tony Mechelynck [:tonymec]]

(In reply to comment #121)
> Oh god!
> 
> I have to say that you guys are crazy (not all of you of course). This bug was
> started 1 month ago, and it is still in NEW state?

So what? If you think you can fix it in a way which will satisfy the module owners, go ahead. If you can't, recriminations are not going to get it fixed; they may even discourage the devs from trying.

> 
> Open source thing is good.. most of the time. But now... I think this just
> sucks. I'm always installing all sec updates when it arrives, but a month ago I
> realized that every 5 min. (yes it is set up to pull mails 5mins) I got a
> window saying choose a cert. What?? Every 5 min I have to click on a button?
> WTF? Than I had to google to find some solution. For some days I could only
> find this bug, but the first solution came up 3 days later.
[...]

That's exceptionally quick. I've seen bugs sit unsolved for years before someone finally was found who had the time, the means, the willingness and the aptitude to fix it. What surprises me is that there are already more than 120 comments after not even a full month.

Again, if all you can do is rant, you'd better shut up.

Comment 123

[L A Walsh]

The reason I looked at this bug (it isn't affecting me that I know of, even though I do use IMAPS for my local email accounts), is that there may be a workaround, or at least a "pattern" to build a workaround (if the listed extension doesn't already work).

Is anyone familiar with the Tbird "Remember Mismatched Domains"
(https://addons.mozilla.org/en-US/thunderbird/addon/2131).

The intent was to override problematic ssl certs where the name in the cert doesn't match the name you are using.  In my case, the cert was crafted for the normal domain-name, but aliases for the various server functions were created to make user-configuration easier.  I.e. the canonical hostname might
be "myxylplyk.smallville.usa" but for mail, a user in domain smallville can
just use 'mail' (other aliases might included 'web-proxy', 'socks-proxy', etc.)

It is configurable on a per-site (or per-domain) basis -- so it is site-specific and wouldn't cause harm by disabling global protections.

It may not solve the underlying problem, but it might "just work" as a workaround as is, or, at least, provide a 'template' for writing a similar extension that would handle this new problem....

To reiterate -- I do use Secure IMAP (IMAPS), but haven't noticed any new problem cropping up with any recent updates -- but I'm also using the 'remember mismatched cert/domain' extension.
-linda w.

Comment 124

[Eddy Nigg (StartCom)]

Error reporting for non-matching domains are correct and should not be altered. That really should be fixed at the configuration (account settings) and the correct server locations should be used.
In any case this is not the cause and issue of this bug.

Comment 125

[Daniel Veditz [:dveditz]]

Thunderbird 3 should have something like a slightly more secure equivalent of the Remember Mismatched Domain addon in its account-setup flow, to deal with the fact that users can't "OK" the error dialog. But that's being tracked in a different bug.

Comment 126

[Daniel Veditz [:dveditz]]

Comment on attachment 322331 [details] [diff] [review]
Patch v4 for 1.8 branch (again), now with -u10 -p

>Index: mozilla/security/manager/pki/src/nsNSSDialogs.cpp
>===================================================================
>+  nsCOMPtr<nsIClientAuthUserDecision> extraResult = do_GetInterface(ctx);

Why GetInterface rather than QueryInterface? Seems like extra work implementing the GI chunk when QI is implemented automagically just by adding the interface to the declaration.

>+void nsClientAuthRememberService::ClearRememberedDecisions()
>+{
>+  RemoveAllFromMemory();
>+}

Why not have just one function? Why is one name public and the other private when they're equivalent?

> /* void getInterface (in nsIIDRef uuid, [iid_is (uuid), retval] out nsQIResult result); */
> NS_IMETHODIMP nsNSSSocketInfo::GetInterface(const nsIID & uuid, void * *result)
> {
>   nsresult rv;
>+  if (uuid.Equals(NS_GET_IID(nsIClientAuthUserDecision))) {
>+    rv = this->QueryInterface(uuid, result);
>+    if (NS_SUCCEEDED(rv) && *result)
>+      return rv;
>+  }

If you used QI directly you shouldn't need this (mentioned above).

r=dveditz

Comment 127

[Kai Engert (:KaiE:)]

You're right, using QueryInterface (and omitting that new code in GetInterface) works, too.

> >+void nsClientAuthRememberService::ClearRememberedDecisions()
> >+{
> >+  RemoveAllFromMemory();
> >+}
> 
> Why not have just one function? Why is one name public and the other private
> when they're equivalent?


ClearRememberedDecision is the entry point of the public API.
RemoveAllFromMemory is a helper function to do the work.
Yes, as of today these are equivalent.

I've made a small change to the code to make the intention more obvious.
I removed the lock from RemoveAllfromMemory, so it can be called from the destructor.
I added the lock in function ClearRememberedDecision.
So, RemoveAllFromMemory it the helper function to do the work without locking, and the caller has to decide whether locking is appropriate (not appropriate when in the destructor).

Comment 128

[Kai Engert (:KaiE:)]

Attachment: Patch v5, minor changes based on comments from dveditz

Comment 129

[Daniel Veditz [:dveditz]]

Comment on attachment 323558 [details] [diff] [review]
Patch v5, minor changes based on comments from dveditz

Approved for 1.8.1.15, a=dveditz for release-drivers

Comment 130

[Kai Engert (:KaiE:)]

Attachment: Bustage fix on top of v5

Patch v5 checked in to 1.8 branch.
I had introduced two bustages, one on windows, one on Mac.
The Mac compiler didn't like a ?: expression, and there was a mismatch between NS_IMETHODIMP/nsresult.

This patch is the incremental checkin I used to fix the bustage.

Comment 131

[Kai Engert (:KaiE:)]

Marking as fixed 1.8.1.15

Keeping bug open, because we must check in something for Firefox 3.0.x and for Firefox 3.1

Comment 132

[Kai Engert (:KaiE:)]

Attachment: Patch v5+ for mozilla 1.9 and mozilla-central

This is the equivalent of patch v5 plus the bustage fix.

Comment 133

[Kai Engert (:KaiE:)]

I would like to invite people who suffer from this bug to test tomorrow's nightly build of thunderbird 2.0.0.x (and firefox 2.0.0.x)

Comment 134

[Kai Engert (:KaiE:)]

As said before, I want to check this in to 1.9.0 branch for FF 3.0.x as soon as possible. I hope to see an approval1.9.0? flag soon

I also want to check this in to mozilla-central.
While this patch is not the desired future solution, I would like to have this patch as a starting point.

Comment 135

[Kai Engert (:KaiE:)]

Comment on attachment 323779 [details] [diff] [review]
Patch v5+ for mozilla 1.9 and mozilla-central

clearing approval1.9 request as it's released.

requesting 1.9.0.1 approval for firefox 3.0.1

Comment 136

[Kai Engert (:KaiE:)]

I've checked this in to trunk, as I explained in comment 134.
Marking bug fixed.

We still want this on 1.9.0 branch for FF/TB 3.0(.x)

Comment 137

[Pelle Johnsen]

I'm getting an assertion during startup when running debug build of FF (mozilla-central):

Oops!  You're asking for a weak reference to an object that doesn't support that.

This comes from the line:

proxiedObserver->AddObserver(this, "profile-before-change", PR_TRUE);

at the end of nsClientAuthRememberService::Init()
<nsClientAuthRemember.cpp:96>

I tried adding nsSupportWeakReference, but it seams that has thread issues, as this gave crashes instead :(

Comment 138

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #131)
> Marking as fixed 1.8.1.15
> 
> Keeping bug open, because we must check in something for Firefox 3.0.x and for
> Firefox 3.1
> 

Can we get someone with the right server set up to verify that this is fixed in 1.8.1.15 since this is the major TB bug for that release?

Comment 139

[Eddy Nigg (StartCom)]

Has it already been applied to 1.9.0.1 ? I can test that one (aka 3.0a2pre+).

Comment 140

[Kai Engert (:KaiE:)]

(In reply to comment #139)
> Has it already been applied to 1.9.0.1 ? I can test that one (aka 3.0a2pre+).

No

Comment 141

[Al Billings [:abillings - ex-MoCo]]

Mozilla Messaging set up a test environment in order to repro the problem and the fix. Right now, accounts use a certificate generated for test users and then IMAP account credentials to log in. The cert isn't mapped to that specific account though.

I've found using both Thunderbird 2.0.0.14 and last night's branch nightly (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.16pre) Gecko/2008062603 Thunderbird/2.0.0.16pre), that I am prompted to accept the cert once, when I first access the account. After that, I'm never prompted again. The only difference between the behavior in the two builds is the checkbox that says "Remember this Decision" in the dialog for the cert (which is titled "User Identification Request"). 

So, this is clearly not the right repro case for the bug. I expect that we need a setup where the cert is per user and is the primary means of recognizing the user (not the IMAP credentials) but this isn't an area of expertise for me (or most of us).

Comment 142

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #141)

> I am prompted to accept the cert once, when I first access the account. 

To accept the cert?  
Do you mean to choose the user's client certificate?
Or do you mean to accept an unrecognized server cert?

Comment 143

[Al Billings [:abillings - ex-MoCo]]

To choose which cert to use from the list. Of course, it is a list of exactly one.

Comment 144

[Robert Relyea]

Sigh, This is mozilla UI. Since 2.0 is out,there are restrictions to changing the UI strings. What you have now is a compromise UI.

Hopefully some time can be put to fixing the Client auth case correctly, for now I prefer to reset the pref back to 'select automatically', but some privacy advocates have deemed that too dangerous for the common user (why this default is in thunderbird, where the privacy is not an issue is beyond me... probably a seamonkey argument -- so we are stuck with the preference that browsers want for everything ....).

Anyway even with that there are corner cases that need to be solved anyway, so a newly thought out UI is in order...

bob

Comment 145

[Philippe M. Chiasson (:gozer)]

(In reply to comment #141)
> [...]
> 
> So, this is clearly not the right repro case for the bug. I expect that we need
> a setup where the cert is per user and is the primary means of recognizing the
> user (not the IMAP credentials) but this isn't an area of expertise for me (or
> most of us).

I've setup such an account for abillings and sent a new cert over by direct e-mail. That certificate allows direct access to the mailbox, without additionnal IMAP authentication.

Comment 146

[Kai Engert (:KaiE:)]

I think I've introduced a potential crasher, being worked on in bug 426555.

Comment 147

[Kai Engert (:KaiE:)]

Comment on attachment 323779 [details] [diff] [review]
Patch v5+ for mozilla 1.9 and mozilla-central

Should this get approved for 3.0.x, I'll need to land attachment 327110 [details] [diff] [review], too

Comment 148

[Kai Engert (:KaiE:)]

comment 145 is also the response for Pelle's comment 137

Comment 149

[David Ascher (:davida)]

re: comment 141 -- abillings and I now have per user certs, and I can't get Thunderbird to ask me more than once per session when checking IMAP.  I've tried going offline using the mode switch, pulling network cables, but Tb still doesn't ask more than at the first session.

Can someone provide explicit STR for Thunderbird?

Comment 150

[Kai Engert (:KaiE:)]

(In reply to comment #149)
> re: comment 141 -- abillings and I now have per user certs, and I can't get
> Thunderbird to ask me more than once per session when checking IMAP.  I've
> tried going offline using the mode switch, pulling network cables, but Tb still
> doesn't ask more than at the first session.
> 
> Can someone provide explicit STR for Thunderbird?

David, did you test using Thunderbird 2.0.0.14 or earlier (not fixed)?

Or did you test Thunderbird 2.0.0.x latest nightly (has the fix).
With latest, you'll not get prompted again if you have the checkbox checked, even if you cancel. (You probably figured that out already, just wanted to reassure).

If you can not reproduce with 2.0.0.14 or earlier, then you seem to have a smart server.

If I understand correctly, one needs to use an SSL server that isn't smart with regards to session caching and does full handshakes often.

Comment 151

[Phil Ringnalda (:philor)]

Or possibly, depending on the accuracy of various comments, one needs to see whether SMTP remembers.

Comment 152

[Philippe M. Chiasson (:gozer)]

Cyrus, I believe is indeed quite smart. However, it's also very configurable, so I can disable tls session caching on the test server we have up.

tls_session_timeout: 0

Comment 153

[David Ascher (:davida)]

Cool, I (not real QA) can verify that the IMAP aspect of the bug is fixed in 2.0.0.16pre.

In 2.0.0.14, and in 3.0a2pre, I get the dialog show up multiple times per session. Specifically, once per imap connection (there can be multiple imap connections per session, up to 5).  

In 2.0.0.16pre, I only get one dialog occurence per session.

Steps to reproduce:

1) get a stupid server (see comment #152 on how to stupidify a smart server), a per-user cert installed.  

2) setup an imap account with multiple folders.  Selecting multiple folders will (in the buggy condition) bring up the same dialog multiple times.  In the fixed condition, only once per session.

3) switch to offline mode (lightbulb in bottom left corner).  Switch back to online mode (same lightbulb, lights off).

4) selecting folders will bring up the same dialog again (in the buggy condition).

(tested only on OS X).

Comment 154

[Al Billings [:abillings - ex-MoCo]]

I tested this on XP with 2.0.0.14 and last night's branch build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.16pre) Gecko/2008062703 Thunderbird/2.0.0.16pre). Like David, I can now repro the prompts (whenever I visit a folder for the first time) in 2.0.0.14 and the fact that the "remember this" checkbox works in the nightly build.

Comment 155

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 323779 [details] [diff] [review]
Patch v5+ for mozilla 1.9 and mozilla-central

a=shaver for 1.9.0.x landing in service of TB3A2

Comment 156

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

If this is important enough to land on the 1.9.0.x branch, it's important enough to have a test.  Please, people who were so interested in this fix, provide such a test in the next 2 weeks?

Comment 157

[Kai Engert (:KaiE:)]

Attachment: Combined patch for 1.9.0

This is the combined patch I've checked in to 1.9.0 branch as a single commit.

It's simply patches from this bug and bug 426555 merged.

Comment 158

[Kai Engert (:KaiE:)]

Backed out on 1.9.0 branch (cvs head), because of regression bug 454406.
Removing fixed1.9.0.2 keyword.

Comment 159

[Nelson Bolyard (seldom reads bugmail)]

reopening, since the fix was backed out.

Comment 160

[Magnus Melin [:mkmelin]]

Hm, since thunderbird has also moved to hg (and we had no release from 1.9), this isn't actually backed out for anything that thunderbird uses.

Comment 161

[Nelson Bolyard (seldom reads bugmail)]

OK, different products have different rules for determining when a bug is
resolved, and when it isn't.  If this bug is still resolved as Thunderbird
defines that, then please mark it resolved again, with my apologies.

Comment 162

[Kai Engert (:KaiE:)]

The patch should get backed out on mozilla-central, then the REOPENED state will be correct.

Comment 163

[Kai Engert (:KaiE:)]

I'm able to reproduce the failure reported in bug 454406 and will work on an improved patch.

Comment 164

[Magnus Melin [:mkmelin]]

Yes please reopen if you back-out from mozilla-central. 
Note that we have a code freeze for tb3b1 coming up 2008-09-23. I don't think we'd want to have this bug showing up again for that.

Comment 165

[Reed Loden [:reed]]

Backed out on trunk.

Comment 166

[DEXTER]

Fixed->reop->fixed->reop.... see my commment #121

Comment 167

[Matthias Versen [:Matti]]

Dexter read https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Comment 168

[Mark Banner (:standard8)]

Kaie, we will most likely be cutting the release for Alpha 3 tomorrow. As we would rather not regress the state from Alpha 2, we are planning to add the patch back to mozilla-central for the release, and then back it out after the release - this will all be done in one push, so it won't actually affect mozilla-central, just what Thunderbird ships.

Please let me, or any of the Thunderbird drivers know asap if there are any problems with this.

Comment 169

[Nelson Bolyard (seldom reads bugmail)]

Mark, for many users, the "fix" to this bug is a serious regression.
So, I would not necessarily consider backing this bug out as introducing
a regression.

Comment 170

[Reed Loden [:reed]]

(In reply to comment #168)
> Kaie, we will most likely be cutting the release for Alpha 3 tomorrow.

Why are there already tags for Thunderbird 3.0a3 then?
http://hg.mozilla.org/mozilla-central/tags

Comment 171

[Mark Banner (:standard8)]

(In reply to comment #169)
> Mark, for many users, the "fix" to this bug is a serious regression.
> So, I would not necessarily consider backing this bug out as introducing
> a regression.

I thought according to comment 165 this bug was already backed out on mozila-central? So we'd be putting it back in for the release.

dmose knows slightly more about this than I do. However having Thunderbird repeatedly prompt for a certificate versus broken functionality in some (presumably fewer?) cases seems desirable to allow some level of use for the alpha for those who do require certs.

Comment 172

[Mark Banner (:standard8)]

(In reply to comment #170)
> (In reply to comment #168)
> > Kaie, we will most likely be cutting the release for Alpha 3 tomorrow.
> 
> Why are there already tags for Thunderbird 3.0a3 then?
> http://hg.mozilla.org/mozilla-central/tags

This why a dry-run that we had done for (when it was) beta. We'll be updating/moving/creating tags when we do the proper build - probably tomorrow.

Comment 173

[Tony Mechelynck [:tonymec]]

(In reply to comment #171)
> (In reply to comment #169)
> > Mark, for many users, the "fix" to this bug is a serious regression.
> > So, I would not necessarily consider backing this bug out as introducing
> > a regression.
> 
> I thought according to comment 165 this bug was already backed out on
> mozila-central? So we'd be putting it back in for the release.
> 
> dmose knows slightly more about this than I do. However having Thunderbird
> repeatedly prompt for a certificate versus broken functionality in some
> (presumably fewer?) cases seems desirable to allow some level of use for the
> alpha for those who do require certs.

IIUC, the "fix" for this bug was the "cause" of bug 454406 (and the fix for the latter consisted of rolling back the former). I suppose that in the current state of affairs you'll have to decide which bug you prefer to have, until you can write a patch which fixes both.

Comment 174

[Dan Mosedale (:dmosedale, :dmose)]

Tony: exactly, we need to pick our poison for this release so we can get it out the door.

(In reply to comment #169)
> Mark, for many users, the "fix" to this bug is a serious regression.
> So, I would not necessarily consider backing this bug out as introducing
> a regression.

As far I can see from reading the introduced regression bug 454406, it only was seen in the wild by one installation.  This bug, however, was seen by a number of Thunderbird deployments.

As such, I think we should temporarily re-land this patch for Thunderbird 3.0a3.

Comment 175

[Dan Mosedale (:dmosedale, :dmose)]

Given that this also seems to have exposed a crasher (bug 426555), Standard8 and I ultimately decided to punt on relanding this patch for Alpha 3 and relnoting instead.

Comment 176

[Nelson Bolyard (seldom reads bugmail)]

In a message sent to an IETF working group, Ben Laurie (Mister mod_ssl :)
wrote;

> FWIW, Apache-SSL does its own caching of client certs because (at
> least at the time) OpenSSL's cache didn't store them. I'm not sure if
> this has been fixed.

I think we may infer from that statement that server programs that 
use OpenSSL to implement SSL, and that request client authentication,
but do not implement their own client cert cache, will find themselves
wanting to do a full handshake in every connection, because although
they have the previous session cached, they do not have the client cert
that was associated with it, and so they are unable to determine the 
user identify associated with that previous connection.

I guess there are a couple of ways to look at this.  I tend to think of 
this as an OpenSSL bug, because the SSL library on which I work, NSS's 
libSSL, caches client certs along with client SSL/TLS sessions.  But I
suppose that IF someone devised an SSL server library, and declared that,
in their design, caching of certs for sessions was the application's job,
and not the SSL library's job, then in their view, the failure to remember
the client cert from a previous session would NOT be a fault of their SSL
library, but rather a fault of the application.  

I think that's the situation here.  Apache's mod_ssl has its own client
cert cache, so it doesn't request client authentication on every connection.
The IMAPS/POPS/SMTPS/LDAPS servers that are the subject of this bug 
apparently do NOT implement their own client certificate cache, and so they
find themselves asking the client to reauthenticate itself with a certificate
on every connection.  This is a server shortcoming.

What mail server product is it (again) that requests client authentication 
on every connection?

Comment 177

[Eddy Nigg (StartCom)]

Apparently that's sendmail, cyrus, dovecot and I expect a bunch more...

Comment 178

[Daniel Veditz [:dveditz]]

No longer wanted 1.9.0.x since thunderbird3 is now shipping off 1.9.1. No way to nominate this to block Thunderbird 3 so picking the branch as the next best thing.

Comment 179

[Benjamin Smedberg]

Wanted for 1.9.1. I'm not sure what we want to do about the blocking request, because the bug seems to have stalled.

Is this really fixed 1.8.1.15?

Comment 180

[Samuel Sidler (old account; do not CC)]

(In reply to comment #179)
> Is this really fixed 1.8.1.15?

Yes.

Comment 181

[Dan Mosedale (:dmosedale, :dmose)]

Kai, do you have any sort of guesstimate on when a new patch iteration could be done?

Comment 182

[Mike Beltzner [:beltzner, not reading bugmail]]

Somebody please explain: 
 - where (if anywhere) there is a fix
 - how much effort it would be to port that to 1.9.1
 - why we need to block 1.9.1 on this in order for Thunderbird to ship

Comment 183

[Samuel Sidler (old account; do not CC)]

(In reply to comment #182)
>  - where (if anywhere) there is a fix

A fix landed on 1.8.1.x in time for 1.8.1.15 (and Thunderbird 2.0.0.16 since we abandoned 2.0.0.15).

>  - how much effort it would be to port that to 1.9.1

That's for Kai to answer.

>  - why we need to block 1.9.1 on this in order for Thunderbird to ship

I'm not sure you do, but someone from Thunderbird needs to decide if the behavior mentioned in comment 153 is "okay" for Thunderbird 3 (it wasn't okay to regress for Thunderbird 2, which is why we took it).

Comment 184

[David Ascher (:davida)]

The bug makes Thunderbird painful to the point of unusability without this fix for users w/ client certs for setups which have short timeouts.  

It's hard to know how many users that affects, but it was certainly loudly noticed when the bug showed up on 2.0.x.

FWIW, the last patch on this bug is trivial to make apply on trunk, but I have no idea whether it is sufficient or not.

Comment 186

[Kai Engert (:KaiE:)]

I refreshed my memory, a new patch is needed that addresses the failure scenario seen in bug 454406.

That failure happens when there are multiple certificates available with the same nickname.

A new patch needs to be created that takes the cert usage in consideration when filtering the available certs, picking the right remembered one.

This should be simple to do. I'll merge the latest patch and produce that new patch now.

Comment 187

[Kai Engert (:KaiE:)]

Attachment: Patch v6, works for mozilla-1.9.1 and mozilla-central

This new patch is based on the latest attachment 328557 [details] [diff] [review].

It has very few changes compared to that patch.
I will add some more attachments that allow to easily view only the changes.

Comment 188

[Kai Engert (:KaiE:)]

Attachment: (subset A) changes to nsClientAuthRemember service

Mozilla 1.8 (for Thunderbird 2) contains two files
nsClientAuthRemember.h and nsClientAuthRemember.cpp

This patch shows the changes on top of those old files.

Instead of remembering a certificate nickname, we now remember the unique database key of the selected cert (equivalent to issuer name and serial number).

Comment 189

[Kai Engert (:KaiE:)]

Attachment: for comparison with (subset B)

This is a subset of old attachment 328557 [details] [diff] [review], which was the most recent patch prior to v6.

It's the only portion of that patch that will be changed.

I want to enable you to make use of bugzilla's interdiff feature.
Because the patches are against different branches, cvs vs. hg, I'm attaching this as a helper attachment.

Comment 190

[Kai Engert (:KaiE:)]

Attachment: (subset B) changes to the code that looks up the remembered cert

It should be possible to use the "diff" link on this attachment and compare it with the attachment named "for comparison with (subset B)"

Comment 191

[Kai Engert (:KaiE:)]

Comment on attachment 364241 [details] [diff] [review]
Patch v6, works for mozilla-1.9.1 and mozilla-central

Bob and/or Dan, could you please review this new patch?

While this patch looks big, most of this code has already been reviewed before.

Please see comments from 186 to 190 which should allow you to do a quick incremental review.

Note, there is one type cast in the code. We cast nsIX509Cert* to nsNSSCertificate*, we know for sure that's the only type implementing that interface for the objects we deal with.

Comment 192

[Kai Engert (:KaiE:)]

Regarding tests:

I used the test environment described in bug 454406 comment 64 ff (private comments).

I verified that we use the cert explicitly selected by the user on subsequent connections, by comparing the serial numbers.

Comment 194

[Dirk-Willem van Gulik]

Eh - while I agree 480121 and this bug are related - they may not be identical. 

480121 is about the capability - this one is (also) about Thunderbird getting into a state where it keeps re-authing - even though it does not have to; most propably due to some problem in the SSL stack with the protocol suspend.

Comment 195

[Michael Baffoni]

Bug 464931 was marked as a duplicate of this one.  I've attempted to read through the comments to see where the proposed fix(es) will prevent re-requesting certificate-based authentication on an existing connection (e.g. after an idle timeout, IMAP service restarts, etc.) where there _aren't_ multiple certificates involved, but I'm not sure if it will be handled as a subset of the issue or not.  Also, will this fix remember those connections that do NOT use a certificate at all when certificates exist, the server allows for certs, but standard SSL-encrypted simple auth is needed?  If this fix's solution is to remember cert/server pairs based on serial number, does this scenario require a dummy cert (e.g. serial #000000....) to enable it's ability to remember the connection does not use a cert?

Comment 196

[Kai Engert (:KaiE:)]

(In reply to comment #195)
> where there _aren't_ multiple certificates involved

Yes, both single or multiple certificate environments will be handled.

> Also, will this fix remember those connections
> that do NOT use a certificate at all when certificates exist, the server allows
> for certs, but standard SSL-encrypted simple auth is needed?

The patch does not introduce any new requirements or restrictions.

Maybe I don't understand your question.

Do you ask about connections, where the user owns client certs, the server asks for a cert, but the users decides to "cancel" the cert selection and thereby decides "don't use a client cert"?
Yes, we will remember that you have pressed cancel and not prompt you again.


> If this fix's
> solution is to remember cert/server pairs based on serial number, does this
> scenario require a dummy cert (e.g. serial #000000....) to enable it's ability
> to remember the connection does not use a cert?

No
If the user does not have a cert, you will never get prompted to select one.
This is the same with and without patch.

Comment 197

[Michael Baffoni]

(In reply to comment #196)
Yes, that is the situation I spoke of where the user hits cancel.  Thank you!

Comment 198

[Pierre Couderc]

Maybe I should declare a new bug, but I have this problem (despite I checked "remember this choice"), every time I open Thunderbird.
This occurs since I have added a webdav .ics requiring a user certificate in Lightning.

Comment 199

[Robert Relyea]

Kai, I have not fully reviewed the patch, but I do have one concern. Does this patch break automatic smart card authentication? (the two issues are what happens if you remember a NULL cert and what happens if the remembered cert is no longer available).

Perhaps this is ok until we have a 'clear current SSL session' java call (which the smart card threads could call, and would also clear the fact this cert is remembered).

bob

Comment 200

[Kai Engert (:KaiE:)]

(In reply to comment #199)
> Kai, I have not fully reviewed the patch, 

Note we had landed a patch very close to this one earlier, for Thunderbird 2, see attachment 321387 [details] [diff] [review] which has your r+, so you can make use of the incremental review approach, as outlined in comment 186 to 190.


> but I do have one concern. Does this
> patch break automatic smart card authentication? (the two issues are what
> happens if you remember a NULL cert and what happens if the remembered cert is
> no longer available).

If we remember a null cert? This means, the user has explicitly decided to cancel.

I guess your point is: On first connect, smartcard may be absent. What happens if the user inserts the smartcard later?

There are two different scenarios.

Scenario 1:
User does not have any (matching) software certificates.
In that case, we don't display the prompt at all.
No prompt shown means => we don't remember anything.
Should the user insert a smartcard later, the prompt will show up on next handshake.

Scenario 2:
User DOES have potentially matching software certificates (accepted by server).
BOTH the software certs and the (absent) smartcard certs are accepted by the server.
In this scenario, if the user decides to cancel, we do remember the "no cert" decision (NULL cert).
Even if the user inserts a smartcard later, we won't prompt again.
How likely is this scenario? Having both soft and hardware certs accepted by a server?


Note, this behavior is identical to what we have added to Thunderbird 2 in the past.


> Perhaps this is ok until we have a 'clear current SSL session' java call (which
> the smart card threads could call, and would also clear the fact this cert is
> remembered).

We DO have a workaround.
If the user uses "clear private data / active logins" we will forget the remembered client auth certs, too!


(I guessed which user interface string it is, because the UI string has been renamed in Firefox 3.1. In the past (Firefox 3.0) it was named "clear authenticated sessions. But I'm sure one of the codepaths triggers it.)

Comment 201

[Robert Relyea]

> Even if the user inserts a smartcard later, we won't prompt again.
> How likely is this scenario? Having both soft and hardware certs accepted by a
> server?

It likely if the user has software certs as well.

This is one of the annoying things about IE... once you've selected something, the only way to unselect is to clear all your preferences (and there is no way to do this from javascript).

> We DO have a workaround.
> If the user uses "clear private data / active logins" we will forget the
> remembered client auth certs, too!

yes, but it clears all of them (at least I think it's easier to find than windows), and I don't think you can do it programatically from the server. If the user has to start clicking 'hidden' clear buttons to get smart cards to work, it really kills their usability.

bob

Comment 202

[Kai Engert (:KaiE:)]

(In reply to comment #201)
> This is one of the annoying things about IE... 

Not sure why you add this reference to IE.


> > We DO have a workaround.
> > If the user uses "clear private data / active logins" we will forget the
> > remembered client auth certs, too!
> 
> yes, but it clears all of them (at least I think it's easier to find than
> windows), and I don't think you can do it programatically from the server. If
> the user has to start clicking 'hidden' clear buttons to get smart cards to
> work, it really kills their usability.

Is this a blocker for you, or will you accept the patch anyway?

If you see it as a blocker, do you have a better proposal?
Should we erase the list of all remembered client cert decision every time the user inserts or removes a smartcard?

Comment 203

[Robert Relyea]

> > This is one of the annoying things about IE... 
> 
> Not sure why you add this reference to IE.

Because your proposal is very similar to what IE does, and is exactly the behavior which causes so many problems with trying to use IE to authenticate using smart cards.


> Is this a blocker for you, or will you accept the patch anyway?

I would very much like a solution. This will certainly break our smart card stuff. This is the problem with partial solutions, we keep adding patch on top of patch on top of patch without the full solution. We break one set of users to feed the need of another set, then patch on top of the to break yet a third set of users.

I think this patch would be acceptable for now if we made any of the following changes:


1) Give websites has a way to 'turn it off' for them (without affecting the rest of the browser). Ideally it would be in the form of "forget cert preferences previously set for this site" call.

2) Add a 'Automatically select for this site' button and remember that decision permanmently. [this is probably non-ideal, you still get stuck if you hit cancel and don't know how to 'logout of all session'].

3) Have different defaults for different protocols (gee, haven't we talked about this before), this patch is perfectly acceptable for TB, it just has problems for websites.... of source the 'ask any' default is perfectly acceptable for TB as well...

Ideally we would sit down and *DESIGN* they way cert prompting should work (and when it can be automatic) before we create another bandaid (it might help with how to make a proper choice between the above 3 options as well).

bob

Comment 204

[Kai Engert (:KaiE:)]

The purpose of this bug is to get the bandaid applied for the close-to-ship products, in the same way as we did for Thunderbird 2 (plus one fix).

The bandaid had been accepted for Thunderbird 2, therefore I would have hoped to use it for Thunderbird 3, too.

This is in common core code. Unfortunately because of common xulrunner we can't use conditional compilation to make it product specific.

You didn't say whether you like/dislike the proposal to clear the list of remembered certs on smartcard insert/removal events.


I agree it's unfortunate we haven't yet designed and implemented the right solution. The client auth tracker bug 159274 lists several open design / implementation bugs. If we want this done, we must start this process immediately, at the beginning of the mozilla-1.9.2 development (for Firefox 3.6), but I'm afraid it's too late for the common core code shared by Firefox 3.5/Thunderbird 3 (mozilla-1.9.1).


Your proposals (1) and (2) involve new UI, and it's too late for new UI in the common core which Thunderbird 3 will be based on.

Your proposal (3) requires protocol detection logic, which we don't have access to. The SSL code is generic as of today. But we have had this requirement before, for example when we worked on error reporting. I will file a platform enhancement bug to get this ability implemented, hopefull we will have it available at a later time.


If you don't like the proposed patch (plus forget cert on smartcard events), my best idea is to implement the new user interfaces and cert remember feature as external add-ons.

Comment 205

[Kai Engert (:KaiE:)]

(In reply to comment #204)
> Your proposal (3) requires protocol detection logic, which we don't have access
> to. The SSL code is generic as of today. But we have had this requirement
> before, for example when we worked on error reporting. I will file a platform
> enhancement bug to get this ability implemented, hopefull we will have it
> available at a later time.

Filed bug 482870 for this

Comment 206

[Daniel Veditz [:dveditz]]

Comment on attachment 364241 [details] [diff] [review]
Patch v6, works for mozilla-1.9.1 and mozilla-central

sr=dveditz

Comment 207

[Dan Mosedale (:dmosedale, :dmose)]

It's getting late enough in the 1.9.1 cycle that I'm getting concerned about how difficult it's likely to be get this accepted on the 1.9.1 branch.  Bob, do you know when you'll be able to respond to Kai's most recent comments?

Comment 208

[Dan Mosedale (:dmosedale, :dmose)]

I just spent some time talking to rrelyea on phone, and he graciously consented to allow me to summarize our conversation here.  He said that since the last discussion in this bug, he and Kai have conversed out-of-band, and Bob stated that he wasn't willing to put his name on a review of that patch even as a bandaid.  

However, Bob also pointed out to me that all of this code is in PSM (of which Kai is the only module owner, but Bob is a peer) and none of it in NSS (where Bob is a module owner).  The upshot of this is that, from Bob's point of view, it's not ultimately his call to make whether or not this can go into the module as a bandaid.

(Bob, feel free to correct me if I've mis-characterized anything here).

So, given all that, I will contact Kai directly and ask him to put on his module owner hat and help me figure out what the right thing to do for Tb3 is.

Comment 209

[Mike Beltzner [:beltzner, not reading bugmail]]

My concern here is in comment 203, where rrylea implies that we'll be robbing Peter to pay Paul; specifically that we'll break SmartCard users in order to solve this issue.

Now, Kaie indicates that he can mitigate the bustage. If he can, then I think I'm fine with this for Firefox 3.5 / Gecko 1.9.1, though I agree with rrylea's concern that we're not as much designing our PSM interface as we are building a shantytown of mitigations and propped up fixes.

Comment 210

[Kai Engert (:KaiE:)]

only users who insert the smartcard in the middle of the app might be affected by the imperfection. the workaround is to restart the app.

we want to get this patch going now. we'll add the tweak "clear remembered list on smartcard events" in 1.9.1.1 for firefox 3.5.1 - agreement reached on irc

received review from honza bambas on the interdiff. he proposed to change
  if (dbkey) {
to
  if (NS_SUCCEEDED(rv) && dbkey) {


and furthermore he proposed to change
  PR_FREEIF(dbkey)
to
  PORT_Free (and move inside the if(dbkey) block)

I'll make these changes.
Honza gave r+ to the interdiffs with these fixes.

I'll declare relyea's one year old r+ (see patch v4 in this bug) plus honza's r+ for the incremental changes into a complete r+

with dveditz' sr+ we're ready to land

Comment 211

[Mike Beltzner [:beltzner, not reading bugmail]]

After a lengthy discussion on IRC, we've decided to take this for 1.9.1 if we can get the final review done in time. Please nominate for approval and feel free to land on trunk after the review is done (assuming that trunk is green) so it can bake.

Comment 212

[Kai Engert (:KaiE:)]

Attachment: Patch v7 for mozilla-1.9.1

This patch v7 is :
- patch v6
- plus addressed Honza's minor review comments
- merged to the current branch (one init block moved)

This is based on Patch v4, which has r=relyea (in this bug)
The diff on top has r=honzab (from irc today)
Carrying forward sr=dveditz

Comment 213

[Kai Engert (:KaiE:)]

Attachment: Patch v7 for trunk (mozilla-central)

merged to trunk, minor differences in context

Comment 214

[Kai Engert (:KaiE:)]

Attachment: Patch v7 for trunk (mozilla-central)

previous attachment was wrong

Comment 215

[Kai Engert (:KaiE:)]

Pushed to trunk, mozilla-central
http://hg.mozilla.org/mozilla-central/rev/3c567e89cc0b

Comment 216

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 378697 [details] [diff] [review]
Patch v7 for mozilla-1.9.1

a191=beltzner

Comment 217

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Patch V8 for mozilla 1.9.1

There was a non-meaningful merge collision when merging the previous patch to the tip; this updates to fix.  Carrying forward r and sr.  I'd say it's entirely reasonable to carry forward approval as well, but bugzilla doesn't feel the same way, so I can't.

Comment 218

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 378751 [details] [diff] [review]
Patch V8 for mozilla 1.9.1

a191=beltzner

Comment 219

[Dan Mosedale (:dmosedale, :dmose)]

Checked into the branch.  I forgot to include the a191=beltzner notation there, but we've got it here.

Comment 220

[DEXTER]

Comment#3 and Comment#121

Good work!

Comment 221

[Johnathan Nightingale [:johnath]]

(In reply to comment #219)
> Checked into the branch.  I forgot to include the a191=beltzner notation there,
> but we've got it here.

For posterity, 1.9.1 changeset is:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fc3a742c4bf2

Comment 222

[[e-mail]]

Comment on attachment 378700 [details] [diff] [review]
Patch v7 for trunk (mozilla-central)

>+  fp.Adopt(CERT_Hexify(&fpItem, 1));
I only just happened to notice this today; I'm not sure whether it warrants a patch, let alone a bug, but I just wanted to point out that other consumers, such as nsNSSCertificate.cpp, use PORT_Free to free the return value of CERT_Hexify; this effectively ends up using NS_Free instead.