43923 - Support for APOP

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect, P3)

Description

[karl lillevold]

Mayby not so commonly used, but at least from where I am standing, I cannot use
a mail system without support for the more secure than POP protocal, called
APOP. Technically, should not be too hard to add, and it would provide (another)
competitive advantage over IE and other browsers.

Comment 1

[lchiang]

add to helpwanted list.

Comment 2

[Tobias Burnus]

> it would provide (another) competitive advantage over IE and other browsers.
you mean Mailing clients. Eudora supports APOP as far as I know.

For the reference see page 11 at
ftp://ftp.isi.edu/in-notes/rfc1460.txt
or better:
ftp://ftp.isi.edu/in-notes/rfc1725.txt

Comment 3

[John G. Myers]

Taking this.

Comment 4

[Jacek Piskozub]

Well, PSM2 has landed...

Comment 5

[chris hofmann]

what are the chances of this landing in the next week or so.
if not good lets re-eval the target milestone...

Comment 6

[John G. Myers]

I'd say chances are fifty-fifty.  Development is being held up by lack of time.

Comment 7

[Jacek Piskozub]

what are the chances of this landing in the next month or so.
if not good lets re-eval the target milestone...

[Chris Chofmann: Sorry for plagiarism]

Comment 8

[(Don't use, old account) - Kai Engert]

I thought it would be less work to do, when I started working on this last
weekend. However, I was unable to finish it and have no time to continue working
on it for now. I'm attaching what I have done so far.

Please note that this is a totally unfinished patch, it doesn't even compile.

However, you might get some ideas from it, especially how the interaction with
the security component (for MD5) could be done.

Comment 9

[(Don't use, old account) - Kai Engert]

Attachment: Some ideas

Comment 10

[Andrew Taylor]

Attachment: Working APOP patch

Here is a working patch that implements APOP.  It is similar to Kai's patch,
except it doesn't provide a GUI pref (APOP should always be used if available,
IMHO -- the user shouldn't need to worry about such details).  It uses
nsISignatureVerifier for the MD5 hash, as per [e-mail]'s comment in
bug 41594.

Please take a look -- feedback is welcome!

Comment 11

[Fuzzy Gorilla]

Does the patch in attachment 70185 [details] [diff] [review] need any updates for the current tree?  If
not, can we add the keyword "patch" and remove "helpwanted" to this bug?  This
seems a low risk patch.

Comment 12

[lchiang]

Before checking this in, need to locate QA resources from the Mozilla community
to test this patch thoroughly.   Netscape QA does not have resources to allocate
to testing this feature.  Just a fyi to drivers or any other folks as
consideration before allowing this patch to be checked in.

Comment 13

[Fuzzy Gorilla]

> need to locate QA resources

Might I suggest then that someone produce a test build with the patch so that
people in the community can test it?  There are 11 voters and 10 non-netscape
employees in the cc: list.  I am sure that at least one of them can test one or
both protocols (POP and APOP) before checkin.

Comment 14

[Dimi Shahbaz]

Here's a linux build using Andrew's patch ("Working APOP patch"), if anyone's
interested in testing:

http://www.dimator.org/~dimator/mozilla/

Comment 15

[Dimi Shahbaz]

One thing I noticed: just because a server is identified as APOP-capable does
not mean that the user is set up to use APOP authentication there.  That was the
case at my ISP (as well as the Qpopper pop3 server I tested locally).

Perhaps it would be good to check if the APOP command returned an -ERR, and if
so, go ahead with regular USER/PASS authentication.  (Right now when an APOP
fails, an error dialog is shown and the mail attempt is cancelled/disconnected.)

Comment 16

[John G. Myers]

Fallback from APOP to USER/PASS should not automatically happen, as this would
expose what the user typed in as their password in case they made a small typo.

If there were a [TRANSITION-NEEDED] response code defined for POP3, this would
be sufficient information to trigger fallback to USER/PASS.  Unfortunately, this
response code has not yet been officialy defined and registered with IANA.

Comment 17

[Andrew Taylor]

Attachment: Working APOP patch v2

This is an update of my original patch.  This time, it is against the 0.9.9
release.  It makes one small change -- it allows trailing spaces after the
server timestamp.

Any chance this patch might get in the trunk after 1.0 branches?

Comment 18

[Andrew Taylor]

Attachment: Working APOP patch v3

This fixes a bug in v2.  Note that mozilla must be configured with the
--enable-crypto option for APOP to be used.

Comment 19

[Andrew Hagen]

I don't think this will make 1.0, unfortunately. Though there is a patch, there
aren't enough resources to test it thoroughly for the next release. If there is
a bug, it could be a security hole. Thus, thorough testing is needed. Nominating
for Mozilla1.0.1 in case it doesn't make 1.0.

Comment 20

[Fuzzy Gorilla]

> thorough testing is needed

In that case, now that 1.0.0 has been branched, check this into the trunk.
It can then be tested by users for problems or regressions and if none are
found, it can be checked into the 1.0.0 branch for inclusion in 1.0.0 or
1.0.1.  Has anyone asked [e-mail] for a review?  Or contacted
[e-mail] to find someone else to review it?

Comment 21

[John G. Myers]

I don't have an APOP-configured server right now, so I can't test it easily. 
Attachment 77550 [details] [diff] looks OK from code inspection.

Comment 22

[Andrew Taylor]

Qpopper (http://www.eudora.com/qpopper/) can easily be set up as a APOP enabled
POP3 server.  I run it on Linux configured with "--enable-apop=/etc/pop.auth
--enable-standalone".  Note that it necessary to add a user called "pop" and add
users to the APOP authorization database with popauth (like CRAM-MD5, APOP needs
the plaintext password).

Comment 23

[Kai Engert (:KaiE:)]

I think pop.gmx.net is an APOP capable server. GMX is a very popular freemail
provider in Germany, I have an account.

I agree that it is a reasonable first step to enable APOP automatically if the
server supports it.

However, I think in a future step, the Mozilla client should also offer a
preference in the UI, to require the use of APOP. A user might chose that
transfering the password in the plain is never allowed, but APOP is sufficient.

Comment 24

[Max Alekseyev]

Let me disturb a silence here with a stupid question:
when could we expect working APOP in Mozilla?

Comment 25

[John G. Myers]

Won't have time to do this anytime soon.  Giving up ownership.

Comment 26

[Kai Engert (:KaiE:)]

I'd like to help. Taking ownership.

Comment 27

[Kai Engert (:KaiE:)]

After a chat with Andrew, the following seems reasonable to me:


I think we should not take the patch without an UI:
- either you care that your password will be transfered in secure mode, or you don't
- the existing patch only helps people who don't care, because sometimes APOP
will be used. You can not enforce it. A hacker can manipulate the server, filter
out the APOP support indication, and Mozilla will send the password without
protection
- if we support this authentication mode, I should be able to configure "I allow
a generally plain connection (not using encryption/SSL), but only if at least my
password will be protected".


Andrew suggested we could do the following, and I like that idea.
We could extend the edit/mail settings/server settings with an additional checkbox.

     [ ] Use secure connection (SSL)
new: [ ] Require secure authentication (APOP)
     [ ] Check at startup...

I refer to the options as SSL or APOP for simplicity.

If the user selects SSL, we could automatically "gray out" (disable) the APOP
checkbox, since SSL implies that security will be in effect.

If the user did not enable SSL, then APOP will be enabled an selectable.

If SSL is enabled, we will enforce it as we are doing now.

If SSL is disabled, but APOP is enabled, we will never transmit the password as
cleartext. If the server should not support APOP, we will show a warning message
to the user, that "secure authentication is required by current account
configuration, but not supported by the server".


Just for completeness, I would like to mention:

There are more mechanisms to do "secure authentication" than just APOP. There is
also CRAM-MD5. We support neither yet, and the attached patch also adds APOP.
That is fine. But if we should add support from CRAM-MD5 in the future, we
should not be required to change the controls in the UI again. We could simply
extend the wording in the prefs to say (APOP, CRAM-MD5). I suggest, at that
later time when multiple mechanisms are available, Mozilla should just require
any secure mechanism to be available, and only refuse to authenticate when none
is supported by the server. But we don't have to decide that now.

In addition, I wonder whether there is any reason, why a user would want to
require both APOP and SSL at the same time. Maybe, but if we should ever decide
in the future that it indeed makes sense to allow to require both at the same
time, we can still remove the "automatic gray out of APOP, if the user selects
SSL" later. For now I guess, automatically graying out one is clearer to
understand for the user.


If you agree to all of the above, I think we require the following:

- a patch to add the APOP checkbox pref

- a proofread version of the string "Require secure authentication (APOP)" wording.

- a proofread version of the error message "secure authentication is required by
current account configuration, but not supported by the server" wording.

- a patch to the mail news authentication code, that checks the new pref, and
disallows plain authentication if the prefs require APOP to be used.

However, if we do the above, and the user did not require that APOP must be
used, but the server supports it, I think it's still a good idea to
automatically use it (like the current patch is doing).

Comment 28

[Kai Engert (:KaiE:)]

More things to consider:
Is APOP only supported for POP3? The patch only cares for POP3.
What about IMAP and NNTP? Should the pref be hidden for those mail server types,
or do you want to add support for those protocols, too?

Comment 29

[Kai Engert (:KaiE:)]

Attachment: Patch to add secure authentication to the POP3 prefs UI

Thinking about my previous comment more, I guess APOP is only for POP, since it
has POP in its name ;-)

This patch adds a secure authentication pref for POP3 only.

Comment 30

[lchiang]

cc: jglick for any UI items and robinf for documentation, if this were to go in.
 Also, need to locate QA resources for this feature to sign up for testing this
protocol.

Comment 31

[Mario Medina]

what version of mozilla comes with apop?

i have 1.1b and it doesn't seems to use apop.

Comment 32

[Michael Gabriel]

none yet, thats what this here is about ;)

Comment 33

[Colby Kraybill]

What is the status of the attached patches?  I tried to apply them to the 1.2a
release but they fail to properly patch.  

Comment 34

[jsberg]

With the UI patch, the check box doesn't remain checked after an exit/restart. 
Luckily, it still uses APOP (which is working fine for me).

Comment 35

[Jeremy M. Dolan]

Are there ISPs that support APOP but not POP over SSL? I assume mailnews already
supports that, so I think it might be better to request your mail provider
provide SSL service, rather than bloat mozilla's code (and worse, UI) with APOP,
and then RPOP, and then KPOP, and then CRAM-MD5 support.

Comment 36

[Jacek Piskozub]

Companies which bough the technology from Qualcomm use it. UCSD (the University
from which I write this message) is an example. 

The only way to read my UCSD email with Mozilla is to forward it to another ISP :-(

Comment 37

[Jun MUTO]

Many major Japanese ISPs are not support POP over SSL.
They offer APOP for (minimal) security.

Comment 38

[Max Alekseyev]

I don't understand the problem. A working patch for APOP support has been
ALREADY proposed. Why not just incorporate it into Mozilla trunk? Almost nothing
has to be done.
So many people for so long time is awaiting for that feature. Each half a year
we observe some activity here but no results. 
Please join all the efforts and close this blocking/annoying bug.
Thank you in advance.

Comment 39

[Martin Kutschker]

Jeremy (comm. 35), APOP is extremly simple, DIGEST-MD5 and CRAM-MD5 are little
more complicated. There is no bloat. 

And I don't see any reason to add a GUI for it. Just use the mechanisms when
available.

Comment 40

[timeless]

Comment on attachment 77550 [details] [diff] [review]
Working APOP patch v3

>@@ -502,6 +511,7 @@
please change both of these PR_FREEIFs to if (x) <some free function, e.g.:
PR_Free>(x)
> 	PR_FREEIF(m_pop3ConData->only_uidl);
>+	PR_FREEIF(m_pop3ConData->apop_time_stamp);
the reason is that the next instruction free's the data but PR_FREEIF results
in PR_DELETE which nulls out the data (wasted op)
> 	PR_Free(m_pop3ConData);

>@@ -788,7 +798,24 @@

>+        char *time_stamp_end = &(line[PL_strlen(line) - 1]);
Generally we've replaced PL_strlen with strlen. The simplest cleanup that came
to mind was: line+strlen(line)-1; 

but this is better:
/* TIMELESS_ASSERTS(*line); */
char *time_stamp_end = line;
while (*++time_stamp_end);

and this:
>+        while (*time_stamp_end == ' ' && time_stamp_end > line)
>+            time_stamp_end--;
can be written as:
while (*--time_stamp_end == ' ' && time_stamp_end > line);

>+        if (*time_stamp_end == '>' &&
>+            (time_stamp_begin = strrchr(line, '<')) != NULL) {
use nsnull or just let the compiler automatically cast to boolean.

>+            m_pop3ConData->apop_time_stamp = PL_strndup(time_stamp_begin, time_stamp_end - time_stamp_begin + 1);
the pair to PL_strdup and friends is PL_strfree, not PR_Free. So please be
consistent, my guess is that it would be easy for you to use PL_strfree earlier
since you don't want to use PR_DELETE anyway

>+            #ifdef DEBUG
DEBUG_<yournamehere> or DEBUG_APOP
>+            printf("apop timestamp is '%s'\n", m_pop3ConData->apop_time_stamp);
>+            #endif

>@@ -1043,6 +1072,46 @@
>+        if (NS_SUCCEEDED(rv) && !okayValue) {
>+            // user has canceled the password prompt
drop 'has'
>+            m_pop3ConData->next_state = POP3_ERROR_DONE;
>+            return NS_ERROR_ABORT;
>+        }
drop the else here v because of the return above. ("else after return")
>+        else if (NS_FAILED(rv) || !password) {
>+            return Error(POP3_PASSWORD_UNDEFINED);
>+        }

this:
>+        cmd = "APOP ";
>+        cmd += m_username;
>+        cmd += " ";
>+        cmd += hashHex;
is probably more expensive than something like this:
cmd = NS_LITERAL_CSTRING("APOP ") +
      nsDependentCString(m_username) + 
      NS_LITERAL_CSTRING(" ") +
      nsDependentCString(hashHex);

>@@ -1050,7 +1119,14 @@
since you don't have cvs access you should provide a patch that isn't -w/-b
otherwise the person who checks it in will cause things like this:
>+    if (sendPassword)
>     m_pop3ConData->next_state_after_response = POP3_SEND_PASSWORD;
>+    else {

>@@ -3093,3 +3170,25 @@
>+nsresult nsPop3Protocol::ExpandToHex(const unsigned char* digest, char* result) {
we don't already have a function to do this?
i don't think this needs to be a member function.

Comment 41

[Andrew Hagen]

We should have a pref. I agree that there is no need for UI, though.

Some users will want to make sure that Mozilla never sends their password in
plaintext. They should have the option of telling Mozilla never to check mail
without APOP (or SSL).

Some other users may be troubleshooting a problem. The mail server could be
malfunctioning. They will want the ability to force Mozilla to not use APOP. 

So the pref needs three states: (1) default, which is to use APOP if available,
(2) disable APOP, and (3) don't check mail without APOP.

Comment 42

[Andrew Taylor]

Attachment: Working APOP patch v4 (no UI)

Timeless: thanks for the comments.  Here is a new patch that with the changes
you recommended.

Comment 43

[Andrew Taylor]

Attachment: APOP patch v5 - no UI, with pref to disallow plaintext passwords

Here is an updated version of the last patch.  The sole difference is it allows
the user to set a preference, "mailnews.pop3.password.disallow_plaintext",
which forces an error if a plaintext password would be sent.

Comment 44

[Henrik Gemal]

would it be possible to include the mailserver name in the error?

So instead of:
Mail server doesn't support encrypted passwords.

write:
Mail server pop3.mail.com doesn't support encrypted passwords.

?

Comment 45

[Kai Engert (:KaiE:)]

One reason why this did not go in yet is because there has not been sufficient
testing, as it has been required in comment 30 by lchiang.

If the patch with the UI has a problem, we should find the problem and fix it,
rather than giving up on the idea of having a UI. I personally think prefs
without UI are bad.

In addition, the UI-less patch uses a global pref only. That's against the idea
of the mail account manager which allows individual prefs for each mail account.
I think we should fix the UI patch, which would provide a separate pref for each
mail account.

(And I don't understand why you changed PR_FREEIF(m_pop3ConData->only_uidl);
into a "if, free" sequence, both are doing the same thing.)


But I'm only giving my $0.02 here. The only way to land this patch is to
convince the owners of the mail component.

Comment 46

[Kai Engert (:KaiE:)]

David, Jean-Francois, Scott, Seth, what's your opinion on this bug?

This suggests to enhance security of mail password by supporting a protocol
which uses a hash-based authentication approach, with the required lower level
crypto functionality already being available.

Are you ok with getting this in?

Comment 47

[Daniel Bratell]

PR_FREEIF have been deemed evil after causing some bugs. It evaluates its
argument twice contrary to PR_FREE, NS_RELEASE and NS_RELEASEIF so there can be
reason avoiding it.

Comment 48

[Christian :Biesinger (don't email me, ping me on IRC)]

FREEIF also sets the argument to NULL, afaik

Comment 49

[Max Alekseyev]

Well... 
Yet another dead season, isn't it?

Comment 50

[(not reading, please use [e-mail] instead)]

I'm not against APOP support, but I don't have cycles to work on it.

Question, is this for people who don't have (or want) POP over SSL, but don't
want to send the password in the clear?

I assume you can do APOP over SSL, right?

I'm ok with adding some UI (a checkbox) to the "Server Settings" panel, but for
now, we can make it a per server, hidden pref.  (adding UI is easy)

kai, how does the patch look to you? 

Comment 51

[Andrew Taylor]

Attachment: v6 - updated to patch 1.3a cleanly

Here is an updated patch.  No major changes, just patches cleanly against 1.3a

Henrik - I agree the server name should be part of the error messages; it would
be nice to do this globally though (there is already a bug for this: bug
66860).

Comment 52

[Martin Kutschker]

Seth, yes, you can do APOP over SSL. APOP is just a simple digest mechanism
(server sends a one time key in its initial greeting, which the client -
together with the password - uses to compute a hash value).

Comment 53

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 111834 [details] [diff] [review]
v6 - updated to patch 1.3a cleanly

Looking good in general.  Some minor nitpicking and one bigger issue are
mentioned below:

>Index: mailnews/local/src/nsPop3Protocol.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/local/src/nsPop3Protocol.cpp,v
>retrieving revision 1.184
>diff -u -6 -r1.184 nsPop3Protocol.cpp
>--- mailnews/local/src/nsPop3Protocol.cpp	25 Nov 2002 23:28:01 -0000	1.184
>+++ mailnews/local/src/nsPop3Protocol.cpp	7 Jan 2003 21:10:01 -0000

>@@ -54,16 +58,20 @@
> #include "nsIPref.h" 
> #include "nsIMsgWindow.h"
> #include "nsIMsgFolder.h" // TO include biffState enum. Change to bool later...
> #include "nsIDocShell.h"
> #include "nsEscape.h"
> 
>+#define DIGEST_LENGTH 16
>+#define EXPANDED_DIGEST_LENGTH 32
>+

How about making these |static const| rather than #defines?  It'll make for
easier debugging.  Also, comments about what each of these constants are for
would be good.

>@@ -491,12 +499,15 @@
> 
>   m_lineStreamBuffer = new nsMsgLineStreamBuffer(OUTPUT_BUFFER_SIZE, PR_TRUE);
>   if(!m_lineStreamBuffer)
> 	  return NS_ERROR_OUT_OF_MEMORY;
> 
>   mStringService = do_GetService(NS_MSG_POPSTRINGSERVICE_CONTRACTID);
>+
>+  mHashGenerator = do_GetService(SIGNATURE_VERIFIER_CONTRACTID);
>+

How about using the two-argument form of do_GetService here and checking the
return val?

>@@ -504,12 +515,13 @@
> 
> 	UpdateProgressPercent(0, 0);
> 
> 	FreeMsgInfo();
> 	PR_Free(m_pop3ConData->only_uidl);
> 	PR_Free(m_pop3ConData);
>+    if (m_pop3ConData->apop_time_stamp) PL_strfree(m_pop3ConData->apop_time_stamp);

Can you split the if and then clauses across 2 lines?  Otherwise it's
impossible to set a breakpoint on the then clause in most debuggers.

>@@ -781,12 +793,27 @@
>       m_commandResponse = line+4;
>     else
>       m_commandResponse = line;
>     
>     m_pop3ConData->next_state = m_pop3ConData->next_state_after_response;
>     m_pop3ConData->pause_for_read = PR_FALSE; /* don't pause */
>+ 
>+    /* look for a server timestamp that can be used for APOP authentication */
>+    char *time_stamp_begin;
>+    char *time_stamp_end = line + strlen(line) - 1;

No need to do a strlen() here, I don't think, as line_length should already be
set for you.

>@@ -1035,20 +1064,70 @@
> 	{
>         char * str =
>             PL_Base64Encode(m_username.get(), m_username.Length(), nsnull);
>         cmd = str;
>         PR_Free(str);
>     }
>+    else if (mHashGenerator != nsnull && m_pop3ConData->apop_time_stamp) {

Just use |mHashGenerator| rather than |mHashGenerator != nsnull|.

>+PRBool nsPop3Protocol::AllowPlaintextPassword() {
>+    nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID));
>+    if (!prefs)
>+        return PR_TRUE;
>+
>+    PRBool disallowPlaintext = PR_FALSE;
>+    nsresult rv = prefs->GetBoolPref("mailnews.pop3.password.disallow_plaintext",
>+                                     &disallowPlaintext);
>+
>+    return (NS_FAILED(rv) || !disallowPlaintext);
>+}

If I'm understanding things correctly, the way this patch is currently
structured, this patch will break POP entirely for the class of users described
in comment 15.	I don't think that's acceptible.

>Index: mailnews/local/resources/locale/en-US/localMsgs.properties
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/local/resources/locale/en-US/localMsgs.properties,v
>retrieving revision 1.23
>diff -u -6 -r1.23 localMsgs.properties
>--- mailnews/local/resources/locale/en-US/localMsgs.properties	17 Sep 2002 19:25:24 -0000	1.23
>+++ mailnews/local/resources/locale/en-US/localMsgs.properties	7 Jan 2003 21:10:02 -0000
>@@ -188,6 +188,11 @@
> ## @loc None
> 4027=Copying %S of %S messages to %S
> 
> ## @name MOVING_MSGS_STATUS
> ## @loc None
> 4028=Moving %S of %S messages to %S
>+
>+# user disallowed plaintext passwords, but can't be honored
>+## @name CANNOT_SEND_ENCRYPTED_PASSWORD
>+## @loc None
>+4030=Mail server doesn't support encrypted passwords.

I think the diagnostic would be slightly more useful if it included the word
APOP:

+4030=Mail server doesn't support encrypted passwords (APOP).

Comment 54

[lchiang]

We need to keep this a hidden pref unless we have someone signed up to write
testcases and perform the testcases for this feature, especially if this feature
gets exposed in the UI.

Comment 55

[(not reading, please use [e-mail] instead)]

dmose, do you have cycles (and interest) to drive this along with kaie?

Comment 56

[(not reading, please use [e-mail] instead)]

Comment on attachment 111834 [details] [diff] [review]
v6 - updated to patch 1.3a cleanly

since rejected by dmose, removing the sr request.

please seek sr after you have a review.

Comment 57

[Kai Engert (:KaiE:)]

Attachment: Patch v7

Comment 15 suggests that APOP might fail, even though the server announces it
supports it.

At this time, I think we should keep with a simple patch. I don't want to work
on code that tries multiple authentication mechanisms if one failed. As a
consequence I'm suggesting a new patch that only tries APOP if the user
explicitly requested it.

The new patch has a checkbox in the UI.
I'm simplifying again. I don't want a dependency. The new UI allows the user to
combine secure authentication with SSL connection. I don't see a reason why we
must forbid this combination.

In the previous patch there was a single checkbox for all POP accounts
altogether. I think we really should have this configuration option
individually for accounts, and the new patch allows that.

During testing I found problems with the existing patch. The problems might
have had to do with the fact that I originally checked mail on an account
without APOP enabled, but then enabled it. For some reason, the code decided to
send "AUTH LOGIN" and then "APOP ...". This pretty much confused the server and
it disconnected. I have added more code that makes sure, we do not send "AUTH
LOGIN" if we are going to send "APOP".

I also rewrote the APOP timestamp detecion code, I think it looks now clearer.
I did not like the statement that looked like "if (--i && i)", i.e. same
variable being accessed twice in the same expression, once with --.

Also, I have decided to not use APOP in the prefs file and in the base
interface for prefs lookup. APOP is for POP3 and uses MD5. There is another
auth mechanism that uses MD5 for IMAP, and that one is called CRAM-MD5. I guess
we will implement CRAM-MD5 at a later time. I've chosen a general name for the
pref, based on the string "MD5", not "APOP".

I have also addressed most of Dan's comments:

> #define => const
actually I have rewritten the code that does HEX encoding. I disliked to have a
function that uses a hardcoded string size defined elsewhere.


> How about using the two-argument form of do_GetService here and checking the
> return val?
But what should we do if we find an error?
We won't stop doing the other code anyway, since we are in a constructor.
I think absence of the signature verifier should not be fatal,
to allow for basic pop functionality.
All access to this variable is protected by non-null checks in the rest of the
file,
so we are not at risk.


> split into separate lines for debugging
done


> no need for "!= nsnull"
removed


> include APOP in error message
done

Comment 58

[Martin Kutschker]

> Created an attachment (id=115286)
> Patch v7

Great.

> At this time, I think we should keep with a simple patch. I don't want to work
> on code that tries multiple authentication mechanisms if one failed. As a
> consequence I'm suggesting a new patch that only tries APOP if the user
> explicitly requested it.

Hm, can't say I like that very much, but better have it working than nothing (a
step-by-step implementation is fine).

> Also, I have decided to not use APOP in the prefs file and in the base
> interface for prefs lookup. APOP is for POP3 and uses MD5. There is another
> auth mechanism that uses MD5 for IMAP, and that one is called CRAM-MD5. I guess
> we will implement CRAM-MD5 at a later time. I've chosen a general name for the
> pref, based on the string "MD5", not "APOP".

Actually there are two MD5-based mechanisms DIGEST-MD5 and CRAM-MD5. The former
performs only a single the latter a double hashing. Note that POP may also use
this mechanisms (IIRC see RFC 1734, 2195). So you may rethink the pref name.

Additionally it's an odd naming choice since Mozilla should in the long run try
the best mechanism (CRAM-MD5, DIGEST-MD5, APOP - in that order). Perhaps name it
"DIGEST".

Comment 59

[Andrew Taylor]

> Created an attachment (id=115286)
> Patch v7

Looks good.

>@@ -784,6 +825,24 @@
>     
>     m_pop3ConData->next_state = m_pop3ConData->next_state_after_response;
>     m_pop3ConData->pause_for_read = PR_FALSE; /* don't pause */
>+ 
>+    /* look for a server timestamp that can be used for APOP authentication */
>+    char *time_stamp_begin;
>+    char *time_stamp_end = line + strlen(line); // points to zero termination byte

I think dmoses comment (No need to do a strlen() here, I don't think, as
line_length should already be set for you) still applies.

Comment 60

[Christian Eyrich]

After speaking here of CRAM-MD5 and DIGEST-MD5 several times, it might be
interesting for some of you that the former has been implemented yesterday.
See bug 202442 for how and why.

I also had APOP in this patch but cancelled it for time and testing reasons.
I'll work on a patch against the current sources and attach it here in the next
days so we can talk about.

Comment 61

[Christian Eyrich]

Attachment: new proposed patch

This patch shows my implementation of APOP.

Remark:
The line
static PRBool useSecureAuthentication = PR_FALSE;
in ProcessAuth() is only a dummy for a user pref to only allow secure
authentications (CRAM-MD5 and APOP). I left this out for now to concentrate on
the main function.


I tested it with three servers, two of them don't support APOP, one (Qpopper)
does. Qpopper closes connection if the password is wrong - that might be
acceptable. But even the servers which don't support APOP close connection
after sending the APOP line (also tested with Eudora and KMail, so no fault of
my code).

So while CRAM-MD5 can also be tried if the !useSecureAuthentication, APOP
should only be used if user switched this on. And this only because it's the
last mechanism tried in this case.
Short, it's not suitable just for trial&error since it causes close of
connection if pw wrong or APOP not supported.

But also if we take care, it seems impossible to me, to provide the right error
message if it fails - was it the command itself or the username/password?

Comment 62

[WADA:World Anti-bad-Duping Agency]

Were some patches checked in to current nightly builds?
I experienced problem reported to bug 203219 with 20030423 and 20030424
build(Win-Me) for POP3 account.
20030421 build has no problem for POP3 account. 
 

Comment 63

[Martin Kutschker]

Christian (#61), I see that your code tests for the APOP timestamp in the server
greeting. Do I understand you correctly that some servers which don't support
APOP still send the timestamp?

Generally I think the unfortunate behaviour of disconneting could occur with any
server/method. I don't think that all pop servers disconnect on a wrong
digest/password, but it seems that the code must handle a disconnect during
authentication.

BTW, the servers that disconnect on APOP. How do they react to a CAPA or an AUTH
command?

Comment 64

[Christian Eyrich]

Martin, yes, the timestamp is a prerequisite for APOP. But it looks to me as not
all servers with a timestamp support APOP.

Sure, disconnect can occur at any time and we should handle this case.
nsPop3Protocol::OnStopRequest is called if the connection is dropped. But it
doesn't seem to do the right thing in this case. I often saw the server
disconnecting but Mozilla keep on waiting for answer.

I hope not all servers disconnect on a wrong digest/password. And at least for
USER/PASS, LOGIN or CRAM-MD5 I didn't notice such a behaviour. But for APOP the
half of the servers I know do in one of both cases: APOP mechanism not known or
APOP user/password wrong.
Of course I don't know that much servers.

> BTW, the servers that disconnect on APOP. How do they react to a CAPA or an AUTH
command?

Some know both, some only one and some neither CAPA nor AUTH.

Comment 65

[Christian Eyrich]

Attachment: new patch v2

In the former version of this patch the tests for useSecureAuthentication were
by setting the flags. I thought this is would be nice as there are out of the
way in the latter code.
But this prevented the user from switching from and to secure authentication
because only the mechanisms from the accounts first mail get were active. They
are cached in m_pop3ConData->capability_flags for the session.

Comment 66

[Michael Newton]

>Martin, yes, the timestamp is a prerequisite for APOP. But it looks to me as
>not all servers with a timestamp support APOP.

We shouldn't need to check for timestamps or anything.  If the user checks the
box in the UI, then APOP should be used.  If it's not supported, the server will
say "-ERR Not supported," and the client can offer the user a chance to use
plaintext authentication.

>I hope not all servers disconnect on a wrong digest/password. And at least for
>USER/PASS, LOGIN or CRAM-MD5 I didn't notice such a behaviour. But for APOP the
>half of the servers I know do in one of both cases: APOP mechanism not known or
>APOP user/password wrong.

According to RFC 1725, if authorization fails, "a negative response is issued
and the POP3 session remains in the AUTHORIZATION state."  This would give the
client a chance to receive an "-ERR Authorization for "username" failed."
message, and offer the user a chance to fix their password.

If a server disconnects on a wrong digest, it is configured incorrectly, or just
ignoring the standard.

Comment 67

[Christian Eyrich]

Michael, we have to look at the timestamp anyways as this is used in coding our
answer to the server when we're submitting the password.

I know the RFC but some servers don't know. Ok, they don't hang up directly,
they issue -ERR response and hang up then - but that no really big difference.

And as there are servers which disconnect not only when un/pw but when failing
APOP, we should try APOP only if we really have to -> if the timestamp is there
and the user want us to to secure authentication.


If we agree on my implementation, it should be ok. APOP is only issued if the
user checks "Require secure authentication" and is the last in the chain (after
CRAM-MD5). So it would not this bad if it fails.
My only "problem" is what we should tell the user when our APOP command gets a
-ERR response. "Password wrong", "APOP not supported" or "Maybe APOP not
supported, maybe Password wrong"?

Comment 68

[(not reading, please use [e-mail] instead)]

let's move this off.  we'd like apop support, but this seems too risky for 1.4

I think 1.5 alpha would be more appropriate, but it's up to [e-mail] (who
is working on this)

Comment 69

[Christian Eyrich]

That's ok for me, this has been waiting for so long now. And since we have
CRAM-MD5 now, there's at least one secure mechanism available.

Nevertheless I'll post a new version of the patch shortly.

Comment 70

[Christian Eyrich]

Attachment: new patch v3

This is the latest and full working version for enabling APOP with UI.

As written in my comment #67, I added a error message "Mail server does not
support secure MD5 authentication or password wrong." to be displayed if our
APOP response has been answered with -ERR.

Sadly I've only one server to test with APOP, but with this it works for weeks
now. Anyone welcome to compile a own Mailnews with this patch and report
experiences.

This and the following questions are the reason to post it now although the
path will not go in before 1.5.

1. Are these messages (local/resources/locale/en-US/localMsgs.properties) ok,
or which should be used?
2. Is "Require secure authentication" for the checkbox text ok?
3. Should CRAM-MD5 only be used if this option is set like APOP or always when
advertised by the server?

Comment 71

[Martin Kutschker]

> "Mail server does not support secure MD5 authentication or password wrong."

I suggest using the real name (ie APOP) here. The user might call his/her
sysadmin and the MD5 might be confusing here. The admin will think the user
doesn't know whether there is some error with APOP or with CRAM-MD5.

> Sadly I've only one server to test with APOP, but with this it works for weeks
> now.

You're welcome to use our server. Contact me via email.

I suggest using the best authentication that is advertized.

Comment 72

[Rafael Ebron (:rebron)]

adt:nsbeta1-

Comment 73

[Christian Eyrich]

Comment on attachment 122847 [details] [diff] [review]
new patch v3

This patch is outdated and not ready for review.

The UI and some of the main code become obsolete through patches for bug 205003
and bug 205571.

I plan to submit a new patch after 1.4 is out.

Comment 74

[Christian Eyrich]

Attachment: new patch v4

Here's another try. The most prefs and UI stuff has been done in bugs 205003
and 205571, so that's almost pure APOP stuff now.

Like CRAM-MD5, APOP will only be used is the new option "Use secure
authentication" is checked.

Also my previous remarks regarding the message when APOP fails apply too.

Comment 75

[Kai]

For what it's worth, stability enhanced Linux, 
Mac OSX and Windows builds based on Mozilla 1.3.1, 
that also come with patch v7 
from this bug, are available at http://wamcom.org

Comment 76

[David :Bienvenu]

Comment on attachment 124929 [details] [diff] [review]
new patch v4

looks prety good. I think this error message text needs to change a little, to
something like

+4031=Mail server does not support secure authentication or you entered an
incorrect password.

But I can make that change myself before checking in (along with changing the 
else
if ()

constructions to
else if ()

Comment 77

[Christian Eyrich]

Thanks David.

But I just noticed, that the changes in localMsgs.properties (new No. 4031) will
colide with http://bugzilla.mozilla.org/attachment.cgi?id=126774.
Should I do a new version of the patch, or will you do this by yourself too?

Comment 78

[David :Bienvenu]

I'll take care of that too.

Comment 79

[David :Bienvenu]

fix checked in.

Comment 80

[Christian Eyrich]

I just tested the latest nightlies with various servers and it works well.
Thanks David for review and adjusting the patch.

Comment 81

[Calum Mackay]

*** Bug 228822 has been marked as a duplicate of this bug. ***