Bug 2418 - NNTP authentication is broken
Summary: NNTP authentication is broken
Status: RESOLVED FIXED
Alias: None
Product: Claws Mail (GTK 2)
Classification: Unclassified
Component: NNTP (show other bugs)
Version: 3.7.9
Hardware: PC Linux
: P3 normal
Assignee: users
URL: http://bugs.debian.org/625732
Depends on:
Blocks:
 
Reported: 2011-05-06 12:29 UTC by Ricardo Mones
Modified: 2011-08-14 22:49 UTC (History)
0 users

See Also:


Attachments
Possible fix for this. (75 bytes, text/plain)
2011-05-09 22:59 UTC, Ricardo Mones
no flags Details
nntp: Treating unexpected errors differently when authenticating (2.20 KB, patch)
2011-08-13 14:14 UTC, Sébastien Bigaret
no flags Details | Diff
Unified diff (1.67 KB, patch)
2011-08-13 15:14 UTC, Sébastien Bigaret
no flags Details | Diff
Patch related to comments #13 and #14 (1.36 KB, patch)
2011-08-14 11:41 UTC, Sébastien Bigaret
no flags Details | Diff
Sending MODE READER unconditionnally (related to: comment #19) (3.13 KB, patch)
2011-08-14 16:35 UTC, Sébastien Bigaret
no flags Details | Diff

Description Ricardo Mones 2011-05-06 12:29:34 UTC
See bug URL for a detailed description of NNTP commands currently sent and an example of a correct command sequence.
Comment 1 Ricardo Mones 2011-05-09 22:59:27 UTC
Created attachment 980 [details]
Possible fix for this.
Comment 2 users 2011-05-12 16:53:51 UTC
Changes related to this bug have been committed.
Please check latest CVS and update the bug accordingly.
You can also get the patch from:
http://www.colino.net/claws-mail/

2011-05-12 [mones]	3.7.9cvs21

	* src/etpan/nntp-thread.c
		Fix bug #2418 "NNTP authentication is broken"
Comment 3 Sébastien Bigaret 2011-08-13 14:14:03 UTC
Created attachment 996 [details]
nntp: Treating unexpected errors differently when authenticating
Comment 4 Sébastien Bigaret 2011-08-13 14:14:36 UTC
FYI now that this has been fixed, there is a bug in the current libetpan (both 1.1 and devHEAD) that prevents claws-mail to successfully connect to news servers when 'MODE READER' gets a reply 201 (Posting prohibited)

Bug has been submitted here:
https://sourceforge.net/tracker/?func=detail&aid=3390872&group_id=41064&atid=429696

Without libetpan fixed, claws says "Error authenticating to ..." while authentification was not even tried (news.c: news_session_new_for_folder()), which is quite misleading.

While this does not avoid patching libetpan to fix the problem, I suggest to log unexpected errors differently, and also to do the MODE READER stuff before disabling logging so that the network logs shows it (and the answer).

I've attached the proposed patch.
Comment 5 Michael Rasmussen 2011-08-13 15:05:34 UTC
I can confirm this. The same situation applies to news.dotsrc.org
Comment 6 Michael Rasmussen 2011-08-13 15:09:02 UTC
Could you please provide a unified diff instead?
Comment 7 Sébastien Bigaret 2011-08-13 15:14:57 UTC
Created attachment 997 [details]
Unified diff

Sorry about the format of the previous submitted patch.
Comment 8 Michael Rasmussen 2011-08-13 16:14:13 UTC
This patch does not catch all broken states from libetpan.

[16:12:40] NNTP< 200 news.sunsite.dk NNRP Service Ready - staff@sunsite.dk (posting ok). 
[16:12:40] NNTP> MODE READER 
[16:12:40] NNTP< 480 Authentication required for command - please register at http://news.sunsite.dk/ 
[16:12:40] NNTP< Authentication required for command - please register at http://news.sunsite.dk/
*** Error authenticating to news.dotsrc.org:119 ...

If the news server response is code 480 libetpan fails as well.
Comment 9 Michael Rasmussen 2011-08-13 21:20:19 UTC
Changes related to this bug have been committed.
Please check latest CVS and update the bug accordingly.
You can also get the patch from:
http://www.colino.net/claws-mail/

2011-08-13 [mir]    3.7.9cvs46

    * src/etpan/nntp-thread.c
    Fix bug when NNTP server sends authentication warning
    as part of the connection session. Privously etpan would
    bail out for any return code other than NEWSNNTP_NO_ERROR
    but as early as the connection session a return code like
    NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_USERNAME and
    NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_PASSWORD should not
    be considered as an error.
Comment 10 Colin Leroy 2011-08-13 21:34:12 UTC
Nice, thanks! Is comment #4 valid anyway or is there no need to have an up-to-date libetpan with cvs46 ?
Comment 11 Michael Rasmussen 2011-08-13 22:14:25 UTC
I don't think so since cvs46 should catch this to. Comment #4 states that given authorization is required some servers will return posting is prohibited which IMHO libetpan will resolve to either NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_USERNAME or NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_PASSWORD in which case cvs46 will catch it. However, should the servers response be resolved to anything else than the above plus NEWSNNTP_NO_ERROR then "Huston we have a problem". In that case libetpan surely must return NEWSNNTP_ERROR_X in which case a work-around must come from a patched libetpan since beginning to interpret error codes loose is a dangerous path to choose.
Comment 12 Michael Rasmussen 2011-08-13 22:19:46 UTC
I situation could give a problem. If the NNTP server is a read-only account then my best guess is that we will see an error. As I see it libetpan seems to handle only read-write accounts with or without authorization. Am I missing something here?
Comment 13 Sébastien Bigaret 2011-08-14 11:39:18 UTC
Hi Michael and all,

If you don't mind, I'll divide my comments in two parts.

About your bug first: you're perfectly correct in that 480 (must
authenticate) should be handled by CM.  I would have foreseen it while
digging into libetpan'c code if I was not completely focused on my
problem.

However, I respectfully disagree with your comment #8 and the message of
commit cvs46: this is not (totally, see below) libetpan faults: as per
rfc 4643 [1], the response code 480 is a possible reply by the server.
I also disagree with accepting REQUEST_AUTHORIZATION_PASSWORD after mode
reader, as you'll see below.

Note: I'm not an nntp expert in any way, I'm just commenting after
      having carefully read the RFCs. I'll try to be as clear as
      possible.


I suspect more bugs/inconsistencies in libetpan now that I have read the
rfcs more carefully:

1. 480 NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_USERNAME is badly named:
   it should be an error, not a warning;

2. as far as newsnntp_mode_reader() is concerned, it should reject code
   381 (password required) and return UNEXPECTED instead of accepting it
   and returning NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_PASSWORD because
   381 is generated by AUTHINFO USER *only* [2].

(I'll submit these comments to libetpan in a moment)

As far as CM is concerned:

- Above point 2. and the corresponding RFC points means that CM should
  consider a request for password as a protocol error if it happens just
  after the MODE READER (esp. since libetpan fails to do that);

- no AUTHINFO PASS should be given if we get a response 281
  (auth. accepted) to newsnntp_authinfo_username() (translated to:
  NEWSNNTP_NO_ERROR by libetpan) --see [3].


I've attached a patch illustrating these points (patch sb prop.2).


Last, since 480 is a 4xx response code (command correct but not
performed [4]), this means that the MODE READER should normally be
re-issued after correct authentification if this is what we want.

However, I've noticed that we have a separate
nntp_threaded_mode_reader() in nntp-thread.c whichis called at other
places, so I guess it is not that important to re-send the mode-reader
if we got NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_USERNAME at the first
place.

__ S
Comment 14 Sébastien Bigaret 2011-08-14 11:39:44 UTC
Now about my initial problem.

In fact, you patch solves your pb but it does not adress mine: libetpan
is still faulty to consider that in response to a MODE READER a 201 is
unexpected: it is perfectly valid.  Also note that it also does not mean
that the server is read-only: it may be read-only for a unauthenticated
user, but accepting posts after successful authentification; here is an
example (real exchange):

  [10:01:10] NNTP> MODE READER 
  [10:01:10] NNTP< 201 news.server.dom Welcome! (No Posting) 
  [10:01:10] NNTP> AUTHINFO USER ****
  [10:01:10] NNTP< 381 PASS required 
  [10:01:10] NNTP> AUTHINFO PASS ****
  [10:01:10] NNTP< 281 OK (Posting Allowed) 

So, alas, the patch for libetpan is needed to correctly handle these
situations, and there is nothing CM can do.


Secondly, I was suggesting in my initial patch to distinguish between
real authentification failure and protocol errors... but it was kinda
naive:

- libetpan treats code 481 as UNEXPECTED, for it only 482 means
  Authentication failed (funny since it does not: 482 is 'Authentication
  commands issued out of sequence'!);

- even if libetpan had no problem, I have at least two news servers at
  hand answering 502 (command unavailable) instead of 481 upon invalid
  credentials...

Let's forget about this!


Last, the exchange in your comment #8 showing the answer 480 to MODE
READER would not have shown up with the actual claws-mail (cvs45 or
cvs46), because logging is disabled before sending it: may I suggest
again that we call newsnntp_mode_reader() before disabling logging? (it
would also help diagnose other possible errors). The attached patch
(patch sb prop.2) includes this proposal.

Thanks for your time!

__ S
Comment 15 Sébastien Bigaret 2011-08-14 11:41:18 UTC
Created attachment 998 [details]
Patch related to comments #13 and #14
Comment 16 Michael Rasmussen 2011-08-14 11:52:55 UTC
Hi s
Comment 17 Sébastien Bigaret 2011-08-14 15:20:36 UTC
Okay I took some time to submit my understanding of the RFC and the implied changes to libEtPan!, here they are:

- 381 Password required is only valid for 
Comment 18 Sébastien Bigaret 2011-08-14 15:47:57 UTC
Michael & all: if someone is looking at this, please hang on a moment, I think there is a flaw in what I said...
I'm investigating this and I will report as soon as possible.

__ S
Comment 19 Sébastien Bigaret 2011-08-14 16:33:53 UTC
In comment #13 I wrote:
  Last, since 480 is a 4xx response code (command correct but not
  performed [4]), this means that the MODE READER should normally be
  re-issued after correct authentification if this is what we want.

But I was completely wrong, and I discovered this while re-reading some
parts of the RFCs in an effort to document the patches submitted to
libEtPan!

See http://tools.ietf.org/html/rfc4643#section-2.2 e.g.:

  
Comment 20 Sébastien Bigaret 2011-08-14 16:35:33 UTC
Created attachment 999 [details]
Sending MODE READER unconditionnally (related to: comment #19)
Comment 21 Michael Rasmussen 2011-08-14 20:57:04 UTC
Your solution is correct in all respects unless the server returns code 480.
"   Response code 480
      Generic response
      Meaning: command unavailable until the client has authenticated
      itself."
Your patch will treat this as an error since libetpan returns 1 mening NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_PASSWORD, but will continue anyway. I find it problematic to treat this as an error when we know it could be a bug in libetpan returning NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_PASSWORD in stead of NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_USERNAME. I am, however not convinced that the proper return code is NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_USERNAME (or 381) since according to NNTP-AUTH (RFC 4643) a proper return code given a request for a capability which requires authentication is 480. So what we are dealing with here is that libetpan simply misses to implement code 480 and therefore returns NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_PASSWORD to indicate that authorization is required to be able to request for this capability. I will therefore state that until libetpan implements code 480 we should threat a return code from libetpan of NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_USERNAME and  NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_PASSWORD equally wrong. Your error message indicates 'mode reader' fails which is not correct in the above situation, 'mode reader' never actually started. My final patch for this bug will therefore do this.
Comment 22 Michael Rasmussen 2011-08-14 21:39:51 UTC
I have mistakenly written NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_PASSWORD a number of places in comment #21 where NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_USERNAME should have been written:-\ The argumentation of code 480 still stands. My Patch will deal with S
Comment 23 users 2011-08-14 21:41:34 UTC
Changes related to this bug have been committed.
Please check latest CVS and update the bug accordingly.
You can also get the patch from:
http://www.colino.net/claws-mail/

2011-08-14 [mir]	3.7.9cvs47

	* src/news.c
	* src/etpan/nntp-thread.c
	Fix bug 2418. Patch provided by S
Comment 24 Sébastien Bigaret 2011-08-14 22:06:38 UTC
Hi Michael,

I'm sorry, I guess there is some misunderstading here

libEtPan _does_ handle 480: see
https://github.com/dinhviethoa/libetpan/blob/master/src/low-level/nntp/newsnntp.c

and my patch do not consider this as an error: in the patch when opening
a session, both NO_ERROR and REQUEST_AUTHORIZATION_USERNAME are ok and
do not log anything.

(what I previously said is that libetpan is considering
REQUEST_AUTHORIZATION_PASSWORD as a valid answer for MODE READER, which
it is not --but the returned error code is coherent with the one it gets
from nntp, it does not return one instead of the other).

Last, mode reader cannot fail with 480, it has nothing to do with
authentification but with the set of enabled commands (which will then
be available or not depending on credentials e.g.); while I cannot be
100% sure, the fact that the RFC forbids MODE READER to happen after
authentification takes place, is another sign in this direction.

Anyway, your problem was with some servers replying with 480, and I
guess it is okay for CM not to be too picky and to transparently accept
this behaviour, and that was the spirit of my patch: not logging an
error in this case.

So I have the feeling that we say the same thing, don't we?


For now, the only remaining "problem" is that the resp. code 201
"service available, posting prohibited" will be logged as an error by CM
until libetpan is fixed.  This cannot be avoided since CM only gets from
libetpan an UNEXPECTED in this case, and we cannot avoid logging errors
for UNEXPECTED in general.  At least logging an error is not harmful (in
comparison to the previous impl. where CM was stopping auth. before it
has a chance to begin).

But you're probably right in that the error message is not clear in this
situation, and something like "MODE READER got an unexpected answer
(check the network log), continuing nevertheless" would be better.

__ S
Comment 25 Sébastien Bigaret 2011-08-14 22:15:21 UTC
Oops sorry, I was not fast enough to answer and you've committed in the meantime.

So to summarize I'm ok with your patch, with the exception that the part

    if (r == NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_USERNAME)

contains an erroneous comment wrt libEtPan: if you go for logging something specific here, I suggest this has to do with supporting newsservers answering 480 to MODE READER (I initially choose not to log anything in that case but why not).

Anyhow, thank you again for your time.

__ S
Comment 26 Michael Rasmussen 2011-08-14 22:32:53 UTC
I might be mistaken but NNTP-AUTH states that whenever a client request a capability requiring authentication it must respond with a code 480 a response of code 381 is only available for legacy support. So libetpan should have returned eg. NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_REQUIRED if the server response is 480. As I read NNTP-AUTH two athorization methods are available: Legacy following NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_USERNAME/NEWSNNTP_WARNING_REQUEST_AUTHORIZATION_PASSWORD or code 480.

I guess we both can agree on the fact that libetpan is not RFC compliant when the session is initially created - Legacy has some rough edges while NNTP-AUTH seems broken.
Comment 27 Sébastien Bigaret 2011-08-14 22:49:58 UTC
Oh ok, now I think I see what you mean!

I guess that since libEtPan does provide 2 separate methods newsnntp_authinfo_username & newsnntp_authinfo_password instead of a single one for authentification, it had to keep the two original resp. code (480 and 381), just in case a server uses a login only, and no password, for auth. (the rfc says it's possible, but does it solely exist?!)

Fine, we've come to the point and we probably should stop before others start throwing tomatoes at us for spamming the ml!-)

Oh and since I've noticed that libetpan has switched to github in the last days, I'll try to find some time to fork it and propose the patchs submitted to sf.net as pull requests.  If some of the points discussed here gets fixed, I'll be happy to keep you all informed here.

__ S
Comment 28 users 2011-08-23 22:00:33 UTC
Changes related to this bug have been committed.
Please check latest CVS and update the bug accordingly.
You can also get the patch from:
http://www.colino.net/claws-mail/

		Fix bug 2418. Patch provided by S

Note You need to log in before you can comment on or make changes to this bug.