491202 - [autoconfig] No fallback for secure authentication failure on secure connections during account verification

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[rsx11m]

I'm connecting to an IMAP server supporting SSL/TLS and STARTTLS for secure connections which advertises AUTH=GSSAPI in addition to AUTH=PLAIN and AUTH=LOGIN as capabilities. This domain is not listed, thus settings are guessed. Autoconfiguration determined names and ports correctly, but then combined secure connection with secure authentication settings, which leads to an error with that server and failure after "Create Account" has been clicked. Combining both options when setting up manually in the Account Manager equally fails. There is at least one report in the MZ forums where such a combination didn't work (in that case for an SMTP server after a 2.0-to-3.0 profile migration), so that issue may not be isolated to a specific type of server.

Now one could blame the server for advertising something that's not supported in all cases, but it doesn't help the user much if running into such a case. The suggestion is to fall back to no secure authentication if it fails during verification, of course only if either SSL/TLS or STARTTLS is available and successfully selected for encryption, to avoid plain-text password being sent over an unsecured connection. Alternatively, offer a checkbox to disable secure authentication if advertised, maybe grayed out if no connection encryption is available.

Comment 1

[rsx11m]

This is still the case after the redesign of the new account setup performed
in bug 506290 [Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090912 Shredder/3.0b4pre]. I don't know how wide-spread this issue may
be, but nominating for "wanted" status to keep it on the radar.

Comment 2

[rsx11m]

> There is at least one report in the MZ forums where such a combination
> didn't work (in that case for an SMTP server after a 2.0-to-3.0 profile
> migration), so that issue may not be isolated to a specific type of server.

Confirmed per bug 522633 during migration testing, so this may be related.

Comment 3

[David :Bienvenu]

This might turn out to be a somewhat significant issue in the real world. It might be too late to do anything about this in 3.0, but we should definitely fix it for 3.1

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to only fallback to insecure auth when we're on a secure connection.

It looks like we already fall back to insecure auth if secure auth fails. http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/verifyConfig.js#199

So hopefully this won't be a huge deal, but just in case, here's a patch which changes the fallback to only occur when the socketType is secure.

A future patch might want to add a better error message to the login failure, to let people know that their username/password might be correct, but we didn't test it because it was insecure.  (Then we would probably want to give them an option to check the password, even though it'll be insecure.)

Thanks,
Blake.

Comment 5

[David :Bienvenu]

I don't think we can say we simply won't support servers that don't do secure auth unless the connection is secure - is that what this proposes?

Comment 6

[Blake Winton (:bwinton) (:☕️)]

I agree we can't say that, but that's not quite what I'm proposing.

If I implemented it correctly, this patch says that we won't support falling back from checking secure auth to checking insecure auth if the connection is insecure.

The downside to this patch is that it will look like the username or password is wrong in that case, which is misleading.

To mitigate the effects of this patch, the user will always be able to create the account using the manual setup option.  Furthermore, the way the code currently works, in this case the username and password will be sent in cleartext across the internet, which might be a little surprising to the user.

So, what we're saying is more like "We won't support auto-checking your username on an insecure server that claims to do secure auth but doesn't, but you could work around it, but we won't tell you how".

Does that make more sense?

Comment 7

[David :Bienvenu]

thx, that sounds better. But, unlike the other secure authentication mechanisms, a server may legitimately offer GSSAPI, and the client machine can just as legitimately not be set up for GSSAPI.  So a client advertising GSSAPI to a client machine not configured to do so shouldn't fall into the class of "insecure servers advertising secure auth", if that make sense.

Comment 8

[Phil Ringnalda (:philor)]

Comment on attachment 407772 [details] [diff] [review]
A patch to only fallback to insecure auth when we're on a secure connection.

Totally not my department, punting.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Yeah, I agree, and that's the case that this would fail.  But if you're going to go to the trouble to set up GSSAPI, wouldn't you also set up a secure connection?

Furthermore, if you go to the trouble to set up GSSAPI, would you want your users to send their passwords in cleartext?

The other question I have in this is how many users (or possibly what percentage of users) have this setup, and will be affected by it?  It seems to me that that if it's a tiny proportion of users, most of whom are fairly technically savvy, we might be able to get away with making them jump through more hoops than a standard user.

Finally, I'ld like to note that I'm not arguing that it should be included, I just want to make sure that we understand the full implications before deciding.

Thanks,
Blake.

Comment 10

[David :Bienvenu]

(In reply to comment #9)
> Yeah, I agree, and that's the case that this would fail.  But if you're going
> to go to the trouble to set up GSSAPI, wouldn't you also set up a secure
> connection?

Most likely...but some servers just advertise GSSAPI without actually supporting it; you could argue those are mis-configured, but I think it's an out of the box thing, so the server admins have gone to no trouble.

> 
> Furthermore, if you go to the trouble to set up GSSAPI, would you want your
> users to send their passwords in cleartext?

There are no passwords sent with gssapi. It's like a certificate based authentication (though it involves "tickets", not certs).

> 
> The other question I have in this is how many users (or possibly what
> percentage of users) have this setup, and will be affected by it?

I have no idea - it's quite possible that it won't be many users...

> 
> Finally, I'ld like to note that I'm not arguing that it should be included, I
> just want to make sure that we understand the full implications before
> deciding.

Yeah, I'm not sure what the right thing to do is either but I'm just trying to make sure we have a common understanding of the facts. I'm a little worried about the failure mode being fairly unfriendly. It also would affect configurations that come from ispdb, right, because everyone goes though verifyLogon?

Comment 11

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #10)
> (In reply to comment #9)
> > if you're going to go to the trouble to set up GSSAPI, wouldn't you also
> > set up a secure connection?
> Most likely...but some servers just advertise GSSAPI without actually
> supporting it; you could argue those are mis-configured, but I think it's an
> out of the box thing, so the server admins have gone to no trouble.

Fair enough.

> > Furthermore, if you go to the trouble to set up GSSAPI, would you want your
> > users to send their passwords in cleartext?
> There are no passwords sent with gssapi. It's like a certificate based
> authentication (though it involves "tickets", not certs).

Normally, sure, but if the user enters a password and the GSSAPI fails, then we'll send the password in cleartext, which might be surprising.

> I'm a little worried
> about the failure mode being fairly unfriendly. It also would affect
> configurations that come from ispdb, right, because everyone goes though
> verifyLogon?

Nope.  Well, kinda nope.  Everyone does go through verifyLogon, but…
(see http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/verifyConfig.js#170 )
if the logon fails, and we've come from the ispdb, then we don't get to the tryNextLogon() call, and so we don't do the fall back in the first place.

Comment 12

[David :Bienvenu]

Comment on attachment 407772 [details] [diff] [review]
A patch to only fallback to insecure auth when we're on a secure connection.

ok, I think this is reasonable - I'm going to try to run with it tomorrow.

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Bienvenu made a comment on IRC, which made me wonder why the account was being created with secure auth if the secure auth failed.  So I looked into it a little, and it turns out there's an added wrinkle.

If we end up falling back to insecure auth, we don't send that information back to the successCallback, and so when we create the account it creates it with secure auth, even though that failed.

I could probably whip up a new version of the patch which would pass the validated config back to the success callback, for use in creating the actual account.  Would tomorrow work for you?  :)

Comment 14

[David :Bienvenu]

tomorrow works great :-)

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with code to pass back the results of the verification.

I'm not sure if we want the previous patch to be marked obsolete, in case we just want to take it for some reason.

Thanks,
Blake.

Comment 16

[rsx11m]

Blake, I've applied that patch but it doesn't change the behavior.
The initial verification fails, I go into the Manual setup and uncheck
"Use secure authentication", then everything is fine.

I tried an IMAP log, but it only captures the activities after the account
has been established, not during the probing itself.

Comment 17

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #16)
> Blake, I've applied that patch but it doesn't change the behavior.
> The initial verification fails, I go into the Manual setup and uncheck
> "Use secure authentication", then everything is fine.
> 
> I tried an IMAP log, but it only captures the activities after the account
> has been established, not during the probing itself.

If I put up a patch with a bunch of logging, would you run it, and send me the results?  Or would you be okay with emailing me the server you're running against, so that I can see what's happening for myself?

Thanks,
Blake.

Comment 18

[rsx11m]

A patch with logging output would be fine, I can try that.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with logging.

Okay, give this a try.  All the messages start with "BW - ", so you if you cared, you could add '2>&1 | egrep "^BW - "' to only get the interesting stuff.  :)

Thanks,
Blake.

Comment 20

[rsx11m]

I see, I needed browser.dom.window.dump.enabled="true" first so get anything.
The output looks promising, but it still fails in the end:

Error -> TypeError: this._treeElement is undefined
BW - Starting to run url 1.
BW -   username? true
BW -   secAuth? true
BW - Url 2 failed, but we're re-trying!
BW -   username? true
BW -   secAuth? true
BW - Retry I.
BW - Starting to run url 1.
BW -   username? false
BW -   secAuth? true
BW - Url 2 failed, but we're re-trying!
BW -   username? false
BW -   secAuth? true
BW - Retry II.
BW - Starting to run url 1.
BW -   username? false
BW -   secAuth? false
BW - Url 2 failed, but we're re-trying!
BW -   username? false
BW -   secAuth? false
BW - Retry III.

If I read your code correctly, username? should be true with secAuth? false,
but that combination is never tested. What I see in the prefs is the very
first version with both parameters being true.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Attachment: A newer patch, still with logging.

(In reply to comment #20)
> The output looks promising, but it still fails in the end:
> If I read your code correctly, username? should be true with secAuth? false,
> but that combination is never tested. What I see in the prefs is the very
> first version with both parameters being true.

Excellent!  Here's a new patch which I believe will test all cases.  If you could double-check the logging in it, that would be great.

Thanks,
Blake.

Comment 22

[rsx11m]

That's the same, except that Retry II is now III and Retry III is now IIII,
though the Romans used IV as "four" if I remember correctly ;-)

One problem is that this.mUsernameSaved apparently doesn't resolve to "true", thus it's not reverted to username? true (there is no Retry II in the debug output). There is no difference whether or not I check "Remember password" in the dialog. Also, there is no verifyLogon clause for Retry II, thus even if it were entered, no additional verification step would be done.

I added this.mServer.username to the debug output before calling verifyLogon, resulting in an output of "null" where the entered user name should have appeared. Thus, this.mUsernameSaved is somehow not correctly set in Retry I.

Hope this helps.

Comment 23

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #22)
> That's the same, except that Retry II is now III and Retry III is now IIII,
> though the Romans used IV as "four" if I remember correctly ;-)

Heh.  IIII seemed easier.

> One problem is that this.mUsernameSaved apparently doesn't resolve to "true",

Right, which I think is because we're creating a new copy of the listener every time through the loop.  I'll try to have a new patch up soon to test that.

> Also, there is no verifyLogon clause for Retry II, thus even if it
> were entered, no additional verification step would be done.

Yeah, Retry II is just there to tell me if we're re-setting the name.  It should be followed by a Retry III.

> Hope this helps.

Yup, you've been very helpful, and I think we're getting quite close to fixing it.

Thanks,
Blake.

Comment 24

[rsx11m]

(In reply to comment #23)
> Heh.  IIII seemed easier.

8-)

> Yeah, Retry II is just there to tell me if we're re-setting the name.  It
> should be followed by a Retry III.

I still think you'd need four verifyLogon passes in total to cover all possible combinations of username? true/false with secAuth? true/false, thus far I am counting three only, there should be IIIa/IIIb. Maybe I'm missing something?

Comment 25

[Blake Winton (:bwinton) (:☕️)]

Attachment: The final logging patch?

(In reply to comment #24)
> > Yeah, Retry II is just there to tell me if we're re-setting the name.  It
> > should be followed by a Retry III.
> I still think you'd need four verifyLogon passes in total to cover all
> possible combinations of username? true/false with secAuth? true/false, thus
> far I am counting three only, there should be IIIa/IIIb. Maybe I'm missing
> something?

Nope, you're counting three, because there's a bug.

The combos should be:

BW - Starting to run url 1.
BW -   username? true
BW -   secAuth? true

BW - Retry I. (== set username)
BW - Starting to run url 1.
BW -   username? false
BW -   secAuth? true

BW - Retry II. (== unset username)
BW - Retry III. (== set secAuth)
BW - Starting to run url 1.
BW -   username? true
BW -   secAuth? false

and 

BW - Retry I. (== set username)
BW - Starting to run url 1.
BW -   username? false
BW -   secAuth? false

in that order, I hope.

Thanks,
Blake.

Comment 26

[rsx11m]

Ok, the order is now:

BW - Retry I.
BW - Retry II.
BW - Retry III.
BW - Retry I.
BW - Retry II.
BW - Retry IIII.

with the first I/II/III block secAuth? true, and I/II/III secAuth? false.
I see the correct combination just after Retry III:

BW - Retry III.
BW - Starting to run url.
BW -   username? true
BW -   secAuth? false
BW - Url failed, but we're re-trying!
BW -   username? true
BW -   secAuth? false
BW -   usernameSaved? false
BW - Retry I.

but upon further inspection, this.mConfig.incoming.username is the correct user name whereas this.mServer.username is again null in that try (it was correct in the first try before Retry I).

Comment 27

[Blake Winton (:bwinton) (:☕️)]

I'm getting a little confused.  Could you post the whole log?  (By which I mean only the "BW - " parts of the log.)

Comment 28

[rsx11m]

Here we go!

> +      dump("BW - Retry III.\n");
>        this.mServer.useSecAuth = false;
> -      this.mServer.username = this.mUsernameSaved;
> + ***  this.mServer.username = this.mConfig.usernameSaved;
>        this.mServer.password = this.mConfig.incoming.password;
>        verifyLogon(this.mConfig, this.mServer, this.mAlter, this.mMsgWindow,
>                    this.mSuccessCallback, this.mErrorCallback);
> +      return;

Make *** instead:

> +      this.mServer.username = this.mConfig.incoming.username;

and it works, apparently this.mConfig.usernameSaved somehow got "lost" again.

BW - Url successful!
BW -   username? true
BW -   secAuth? false

Comment 29

[rsx11m]

(In reply to comment #27)
> I'm getting a little confused.  Could you post the whole log?

Oops, it was long thus I condensed it. In case you still need the full log:

BW - Starting to run url.
BW -   username? true
BW -   secAuth? true
BW - Url failed, but we're re-trying!
BW -   username? true
BW -   secAuth? true
BW -   usernameSaved? false
BW - Retry I.
BW - Starting to run url.
BW -   username? false
BW -   secAuth? true
BW - Url failed, but we're re-trying!
BW -   username? false
BW -   secAuth? true
BW -   usernameSaved? true
BW - Retry II.
BW - Retry III.
BW - Starting to run url.
BW -   username? true
BW -   secAuth? false
BW - Url successful!
BW -   username? true
BW -   secAuth? false

Comment 30

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #28)
> Here we go!
> 
> > +      dump("BW - Retry III.\n");
> >        this.mServer.useSecAuth = false;
> > -      this.mServer.username = this.mUsernameSaved;
> > + ***  this.mServer.username = this.mConfig.usernameSaved;
> >        this.mServer.password = this.mConfig.incoming.password;
> >        verifyLogon(this.mConfig, this.mServer, this.mAlter, this.mMsgWindow,
> >                    this.mSuccessCallback, this.mErrorCallback);
> > +      return;
> Make *** instead:
> > +      this.mServer.username = this.mConfig.incoming.username;
> and it works, apparently this.mConfig.usernameSaved somehow got "lost" again.

Better is to just remove that line, and the line after it (setting the password), because the username should be set by the Retry II block above, and the password shouldn't have changed.

Did you want to test that, while I strip the comments out of my version, and post it?

Thanks,
Blake.

Comment 31

[rsx11m]

If fails again in Retry III when I do that:

...(same as comment #29 up to here)
BW - Retry III.
BW - Starting to run url.
BW -   username? true
BW -   secAuth? false
BW - Url failed, but we're re-trying!
BW -   username? true
BW -   secAuth? false
BW -   usernameSaved? false
BW - Retry I.
BW - Starting to run url.
BW -   username? false
BW -   secAuth? false
BW - Url failed, but we're re-trying!
BW -   username? false
BW -   secAuth? false
BW -   usernameSaved? true
BW - Retry II.
BW - Retry IIII.

with

> +      dump("BW - Retry III.\n");
>        this.mServer.useSecAuth = false;
> -      this.mServer.username = this.mUsernameSaved;
> -      this.mServer.password = this.mConfig.incoming.password;
>        verifyLogon(this.mConfig, this.mServer, this.mAlter, this.mMsgWindow,
>                    this.mSuccessCallback, this.mErrorCallback);
> +      return;

Comment 32

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with cleaned up logging.

(In reply to comment #31)
> If fails again in Retry III when I do that:
> > -      this.mServer.password = this.mConfig.incoming.password;

Hmm.  I wonder if we need to re-set the password, and so removing this line was a mistake?

Here's a new diff, with the password line, and with more standard logging, and with a couple of prefs to turn logging on (defaults to logging off), as per philor's suggestion in IRC.

Phil, I got the logging working once, but haven't been able to see it since then, despite my setting of the preferences to true.  Can you take a look at the patch, and see if there's anything obvious I'm doing wrong there?

Thanks,
Blake.

Comment 33

[Phil Ringnalda (:philor)]

asuth's messing with your head, with his prefs: see http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/gloda.js#212 and http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/log4moz.js#70. Log4Moz is actually looking for a char pref that says what level you want, like All or Debug; the mailnews.database.global.logging prefs are gloda just looking for where, because it will decide for itself how much to log in each place. (Of course, I'd forgotten that too when I suggested a default pref - I guess if we don't log anything Fatal then a default of Fatal would be basically false.

Comment 34

[Blake Winton (:bwinton) (:☕️)]

Attachment: The final patch for tonight.

(In reply to comment #33)
> Log4Moz is actually looking for a char pref that says what level you want,
> like All or Debug;
> I guess if we don't log anything Fatal then a default of Fatal would be
> basically false.

Sure, but I might as well use the magic constant "None", as per http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/log4moz.js#133

And the logging helped me find a couple more bugs, which are now fixed.

Thanks,
Blake.

Comment 35

[rsx11m]

Comment on attachment 408158 [details] [diff] [review]
The final patch for tonight.

I've tried the patch, but again it works only if the "Retry III" block has
> +      this.mServer.username = this.mConfig.incoming.username;

Thus, it appears that you need this line to set username, for whatever reason.

Comment 36

[Blake Winton (:bwinton) (:☕️)]

Sure, although I suspect I'll make it:
this.mServer.username = this.mConfig.incoming.username;
to match the pair on lines 208 and 209.

(Do you mind waiting until tomorrow before getting the new patch?)

Thanks,
Blake.

Comment 37

[rsx11m]

Sure, I think that's close enough to a working solution now.

(In reply to comment #32)
> with a couple of prefs to turn logging on (defaults to logging off),

The error console logging is great! Those are the only mail prefs starting with "email" now, can you make it mail.wizard.logging.* (or mailwizard.logging.* if the dot is not tolerated) instead? This would be more consistent.

Comment 38

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to fall back to insecure auth sometimes, and pass the results of the probe back to the calling code, with logging and logging prefs.

Okay, so I lied.  But _this_ is really the final patch, and should be ready for review tomorrow, once rsx11m gives it the go-ahead.  (Unless I forgot something, cause I'm tired. :)

Thanks,
Blake.

Comment 39

[rsx11m]

Didn't try this again (I'm kind of tired too), but this looks good and contains the username line I've added in the latest test. So, consider this my go ahead.

Comment 40

[David :Bienvenu]

Comment on attachment 408164 [details] [diff] [review]
A patch to fall back to insecure auth sometimes, and pass the results of the probe back to the calling code, with logging and logging prefs.

thx, r/sr=me. Some nits:

You can use the ? operator here:
+                          // back-port it to the current config.
+                          if (successfulServer.useSecAuth)
+                            me._currentConfigFilledIn.incoming.auth = 2;
+                          else
+                            me._currentConfigFilledIn.incoming.auth = 1;


this is a little scary here - I guess inServer is still sufficiently alive after being removed to be useful? Might be worth a comment.

       accountManager.removeIncomingServer(inServer, true);
-      successCallback();
+      successCallback(inServer);

Can you tweak this comment to say "go back to trying just the username"?
+      // If we tried full email address as the username, then let's go back
+      // before trying the other cases.

Comment 41

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Nominating blocking-thunderbird3 since there's a patch available.

I was about to file a new bug on this - here's my story (hopefully it's the same issue);

I was using autoconfig wizard to test my school's IMAP server. It can work on both STARTTLS and SSL for the IMAP server, but it's preferred to use SSL.

autoconfig, upon detecting STARTTLS support, greenlights it, but I switch to SSL. It then greenlights on SSL, but when I click Create Account, it gets "stuck", the Create Account gets greyed out (makes sense since I already clicked it), and the wizard fails to create the account, it goes into some sort of attempted authenticating loop before eventually asking me to recheck my username/password (which are definitely correct)

Later I realize when I click Manual Setup, the Use Secure Authentication checkbox was ticked. No wonder it doesn't work! However, there wasn't any way for me to explicitly specify "Don't use Secure Authentication" in the autoconfig wizard.

Comment 42

[rsx11m]

Sounds like the same thing, not testing non-secure variants correctly and not writing back the successful variant. If you can, apply the patch, and if it works it was the same issue you've observed.

Comment 43

[Blake Winton (:bwinton) (:☕️)]

Attachment: [checked-in] The previous patch, with nits fixed.

(In reply to comment #40)
> (From update of attachment 408164 [details] [diff] [review])
> thx, r/sr=me. Some nits:
> You can use the ? operator here:
> +                          // back-port it to the current config.

Done.

> this is a little scary here - I guess inServer is still sufficiently alive
> after being removed to be useful? Might be worth a comment.
>        accountManager.removeIncomingServer(inServer, true);
> -      successCallback();
> +      successCallback(inServer);

Done, with auxiliary comment on what we could change it to if it turns out not to work.

> Can you tweak this comment to say "go back to trying just the username"?
> +      // If we tried full email address as the username, then let's go back
> +      // before trying the other cases.

Done.

(I'm asking for a quick r because I'm not sure about the indentation for the ?: line.)

Thanks,
Blake.

Comment 44

[David :Bienvenu]

Comment on attachment 408294 [details] [diff] [review]
[checked-in] The previous patch, with nits fixed.

looks fine, thx.

Comment 45

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 408294 [details] [diff] [review]
[checked-in] The previous patch, with nits fixed.

(We haven't marked it blocking-3.0, so I'm asking for approval before asking for checkin.)

Comment 46

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/comm-central/rev/dab073a1923f
http://hg.mozilla.org/releases/comm-1.9.1/rev/af594896a4d6

Comment 47

[Ben Bucksch (:BenB)]

+      // inServer seems to still be sufficiently alive after being removed
+      // to be useful.  If this turns out to be a problem, we could just
+      // pass back the useSecAuth attribute.
+      successCallback(inServer);

Sorry for the late comment, but can't you just return an AccountConfig object (e.g. mConfig) as successServer ? That's one reason why we have it, and most of the code there just deals with AccountConfig instead of nsIMsgIncomingServer.

Comment 48

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to use the config instead of the server.

(In reply to comment #47)
> Sorry for the late comment, but can't you just return an AccountConfig object
> (e.g. mConfig) as successServer ? That's one reason why we have it, and most
> of the code there just deals with AccountConfig instead of
> nsIMsgIncomingServer.

Something like this?

Later,
Blake.

Comment 49

[Blake Winton (:bwinton) (:☕️)]

Attachment: [checked in] The previous patch with irc-nits fixed.

The irc nits were:
1. make a comment after the 1 (int enum literal), like // [fall back to] insecure auth
and
2. maybe return a *copy* of the config object. (there's an AccountConfig.copy()). not sure if needed, so you can ignore this.

We don't seem to need to make the copy, so I didn't.  I did add the comment, though (to match the similar comment on line 95).

Thanks,
Blake.

Comment 50

[David :Bienvenu]

Comment on attachment 408415 [details] [diff] [review]
[checked in] The previous patch with irc-nits fixed.

interdiff looks reasonable.

Comment 51

[Mark Banner (:standard8)]

Comment on attachment 408415 [details] [diff] [review]
[checked in] The previous patch with irc-nits fixed.

Checked in:
http://hg.mozilla.org/comm-central/rev/ff1147beada3
http://hg.mozilla.org/releases/comm-1.9.1/rev/8f7b93ee8d9d

Comment 52

[rsx11m]

Verified fixed for today's Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5pre) Gecko/20091027 Shredder/3.0pre nightly build.

Maybe Gary can double-check that the fix resolves his comment #41 as well.

Comment 53

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #52)
> Verified fixed for today's Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
> rv:1.9.1.5pre) Gecko/20091027 Shredder/3.0pre nightly build.
> 
> Maybe Gary can double-check that the fix resolves his comment #41 as well.

Unfortunately, the fix didn't resolve that. Spun off as bug 525083.

Comment 54

[Ben Bucksch (:BenB)]

First, note all this does not affect configs fetched from our database or the ISP. Below is only about guessed configs.

I'm working on this code in bug 495412.

I notice that - during config guessing - we do not fall back to plaintext auth unless we have SSL/STARTTLS - during verification.
<http://hg.mozilla.org/comm-central/annotate/54c135ebb58c/mailnews/base/prefs/content/accountcreation/verifyConfig.js#l227>
However, we don't have any problem with using plaintext auth on insecure connections when the server never advertises any secure auth (like GSSAPI or MD5). That doesn't make sense.

In other words, a server that has no SSL and does not support encrypted passwords nor Kerberos will work, we'll set up the account (after a warning).
A server that has no SSL and does not support encrypted passwords, but advertises "GSSAPI", but GSSAPI/Kerberos fails during verification (because it's not set up on client or server), we'll fail entirely and can't set up the account, and even claim the username/password is wrong.
I think the second scenario is no less secure than the first. Therefore, I think we should allow that - if we warn(ed) the user.

Is that OK, if I change it like that?

Comment 55

[Ben Bucksch (:BenB)]

Followup bug 635628 about the last comment.