202442 - add CRAM-MD5 authentication to POP3

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect)

Description

[David :Bienvenu]

Since POP3 doesn't have a nice way to tell the client that it will do cram-md5
authentication, we can either try AUTH CRAM-MD5, or we can try the CAPA
extension. My hope is that servers that support CRAM-MD5 will support CAPA.
Here's an example session.

+OK Hello there.
CAPA
+OK Here's what I can do:
SASL CRAM-MD5 CRAM-SHA1
STLS
TOP
USER
LOGIN-DELAY 10
PIPELINING
UIDL
IMPLEMENTATION Courier Mail Server
.

Comment 1

[Christian Eyrich]

No, I must let you down.
I know at least two supporting CRAM-MD5 but don't know CAPA. One supports
AUTH
+OK List of supported authentication methods follows
LOGIN
CRAM-MD5

the other not even this ...

But I gonna work on this.

Comment 2

[David :Bienvenu]

Bummer. One possibility is to add account settings ui for the pop3 server to
allow the user to specify that they want to use cram-md5. I guess I'd like to
avoid always trying CRAM-MD5 on servers that don't support it, but I'd like to
just do the right thing on servers that support AUTH or CAPA. We could just try
CRAM-MD5 the first time we connect to the pop3 server and if it fails, don't try
it again until the next time mozilla runs.

Comment 3

[Christian Eyrich]

Yes, so this could work like it currently does. Only the first time checking a
server the AUTH command is issued, the state is memorized until leaving Moz.

I don't know how much servers are out there which don't say they support CRAM
but do. It should be a small minority, but testing them one time each Mozilla
run for CRAM isn't so expensive though.

On the other hand this makes AUTH and CAPA futile and should we support those
servers?

Comment 4

[David :Bienvenu]

if you mean, should we support those commands, I'd agree that it's futile for
just the cram-md5 work and maybe futile in general. We'll still work with those
servers if we just try cram-md5, right?

Comment 5

[Christian Eyrich]

We'll work with all servers of course when testing the specific server with
cascaded ifs until one supported authentication mechanism found.
But if we want to use CRAM on all servers supporting this, we won't use CAPA for
detecting AUTH methods and can remove the AUTH question.

I'm not perfectly sure wether it's a bug in the servers not to support at least
one of these informational functions if they support cram/login a.s.o., or not.

RFC 1734 introduces the AUTH command to POP3, but only "AUTH mechanism". Do you
know of a RFC on the AUTH command without arguments (the informational version)?

Comment 6

[Christian Eyrich]

I'd implement CRAM-MD5 and APOP (because very similar to CRAM-MD5).

Then cascading through CRAM-MD5, APOP, LOGIN and USER/PASS in this order. The
first two will only be tried if the server places a Timestamp in the greeting
banner as this is necessary for both.

So it's very likely that one of the two new mechanisms is supported by the
server if we test it. This increases the chance of success of the tests.

Comment 7

[Christian Eyrich]

Attachment: proposed patch

Ok, I decided not to add APOP in this patch for several reasons.

-There already is a bug on this (bug 43923).
-I'd be clearer to make it a extra patch
-I need time to test (haven't found a server using this) and CRAM-MD5 should it
make into 1.4b.

Implementing CRAM-MD5 itself was the simplest step (mainly copy&paste from
nsSmtpProtocol). But what I've done and why?

Some servers don't know CAPA or even AUTH to ask for a list of supported
mechanisms. So I removed the former code from SendAuth() and AuthResponse()
because we need to try and error.
I decided to step down if the mechanism results in a negative reply, not if the
server sends a negative reply to username or password.

Same as before is that once evaluated as working with this server the mechanism
is saved in m_pop3Server->SetPop3CapabilityFlags for this seasson. So the
process of starting with CRAM-MD5 and stepping down is only done once until
restart.
Not as smart as it could be, but we've to thank the shortcomings of the servers
mentioned.

The switch which mechanism will be called is in ProcessAuth(), the fallback in
AuthResponse().
CRAM-MD5 I implemented in SendUsername and SendPassword as in nsSmtpProtocol,
because it's a two step mechanism. LOGIN is a three step so the initial command
lives in a seperate function.

The evaluation of the LOGIN response I didn't move into AuthResponse() because
it would be called two times there (after initial command and after sending
username) and makes it impossible to differentiate between these situations.

Additionally I introduced SetConFlag(), ClearConFlag() and TestConFlag() to
make these flag operations simpler and clearer (thus the length of this patch).
I hope that's ok.

Comment 8

[David :Bienvenu]

great, I'm glad you have this working.

Some quick comments - The flag routines should be named Capability, not Con,
since they're not specific to connection/authentication. Also, the braces style
in this file is not K&R, but rather:

if (a)
{
...
}

Can you fix that and attach a -uw diff, so the whitespace changes are ignored?

Also, if a server supports AUTH, shouldn't we do what it says? Is it too hard to
support both AUTH and the fallback approach for servers that don't support AUTH?

Comment 9

[Christian Eyrich]

Attachment: patch v2

I named them Con because they're handling flags of m_pop3ConData. But Cap is
also good, Capability is a little bit long, no?

I changed two braces in the code I edited/added now, but not in the whole file.


Yes, we can support AUTH as before. I made a mistake by thinking the AUTH list
wouldn't be reliable since advanced mechanisms can be supported although not
AUTH supported.

I reimplemented and adjusted this in SendAuth() and AuthResponse() and renamed
my former AuthResponse() to AuthFallback().

Also added is the test for PSM before activating CRAM-MD5 support like we do in
nsSmtpProtocol.

And this patch contains a cpp I forgot as well as the Makefile.in that has
changed now (PSM test).

Comment 10

[David :Bienvenu]

Great, thx.

Did you mean to remove this export? I think we might still need it...unless the
build system has changed.
-		nsMsgLocalCID.h \

Now comes the fun part where I get picky :-)

A lot of these calls can be combined:
+        if(TestCapFlag(POP3_HAS_AUTH_LOGIN) ||
+           TestCapFlag(POP3_HAS_AUTH_USER))
into just
TestCapFlag(POP3_HAS_AUTH_LOGIN | POP3_HAS_AUTH_USER) 

that generates less code. You could change the function names to TestCapFlags if
that's more readable. I realize the original code followed the old pattern, but
now that we're making a function call, it's best to just make one.

+  if (TestCapFlag(POP3_XTND_XLST_UNDEFINED))
+    ClearCapFlag(POP3_XTND_XLST_UNDEFINED);
this should just be 
ClearCapFlag(POP3_XTND_XLST_UNDEFINED); 
it generates less code.

similarly,
+    if (TestCapFlag(POP3_UIDL_UNDEFINED))
+        ClearCapFlag(POP3_UIDL_UNDEFINED);
etc.

Did you avoid all the bugs that I didn't avoid when doing this for SMTP? e.g.,
if the user enters the wrong password, will we retry CRAM-MD5 when the user
enters a new password? It looks like it's OK, but I thought I'd ask.

Comment 11

[Christian Eyrich]

Attachment: patch v3

> Did you mean to remove this export?

Er, no. I don't know how this happened. Oh, I worked with a to old local copy,
sorry.

> [about combining flags]

Ah yes, good point. You're right. I replaced them.

> [avoiding bugs]

I think I've done, yes. As the main CRAM-MD5 code has been copied, this should
be as bugfree as in Smtp.
Reentering password has been tested although since we only fallback if the
command itself failed, that's no problem at all.
Handling failed password is other than in Smtp module anyways, I'll have a look
at this in the future.

Comment 12

[David :Bienvenu]

Comment on attachment 121361 [details] [diff] [review]
patch v3

sr=bienvenu, looks good.

Comment 13

[David :Bienvenu]

fix checked in, r=sspitzer

Comment 14

[:aceman]

(In reply to comment #2)
> ... but I'd like to just do the right thing on servers that support AUTH or CAPA.

-> bug 256294