NNTP authentication should check capabilities #54496

jelie · 2010-11-01T21:56:31Z

BPO	10287
Nosy	@pitrou, @hynek
Files	nntplib_optcapabilities.patch: Patch to optionally disable capabilities at connection. nntp-retry-capas-after-login.diff: Patch to retry CAPABILITIES after login 0cf0b06e1d31.diff: Add AUTH tests, don't fail on temporary errors in getcapabilities, get caps after login again e986dd8bb47d.diff: Same as above with review comments fixed b33bcf179df4.diff: Send MODE READER only if we're not in READER mode already. Add some tests.

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2012-02-14.22:34:12.256>
created_at = <Date 2010-11-01.21:56:31.473>
labels = ['type-bug', 'library']
title = 'NNTP authentication should check capabilities'
updated_at = <Date 2012-02-14.22:34:12.255>
user = 'https://bugs.python.org/jelie'

bugs.python.org fields:

activity = <Date 2012-02-14.22:34:12.255>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2012-02-14.22:34:12.256>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2010-11-01.21:56:31.473>
creator = 'jelie'
dependencies = []
files = ['23615', '24466', '24499', '24500', '24502']
hgrepos = ['113']
issue_num = 10287
keywords = ['patch']
message_count = 20.0
messages = ['120183', '147141', '147156', '147157', '147184', '152944', '153147', '153148', '153195', '153205', '153209', '153210', '153211', '153212', '153213', '153217', '153229', '153370', '153373', '153374']
nosy_count = 5.0
nosy_names = ['pitrou', 'jelie', 'python-dev', 'hynek', 'Nathan.Clayton']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue10287'
versions = ['Python 3.2', 'Python 3.3']

jelie · 2010-11-01T21:56:31Z

RFC 4643:

The server MAY list the AUTHINFO capability with no arguments, which
indicates that it complies with this specification and does not
permit any authentication commands in its current state. In this
case, the client MUST NOT attempt to utilize any AUTHINFO commands,
even if it contains logic that might otherwise cause it to do so
(e.g., for backward compatibility with servers that are not compliant
with this specification).

Yet, nntplib attempts to authenticate.

self.capabilities() should be sent at startup.

If "READER" is advertised, no need to send a "MODE READER" command at all...

If "MODE-READER" is advertised, then "MODE READER" (if wanted) can be sent.
Then, self.capabilities() should be sent again. Capabilities changed!

Then authentication if "AUTHINFO USER" is advertised with NNTP version >=2. If "AUTHINFO" without "USER", no authentication at all.

And after authentication, self.capabilities() should be sent again.

Please note that the readermode_afterauth variable I see in the source code should normally not be used by a client... RFC 4643 mentions:

o the MODE READER command MUST NOT be used in the
same session following successful authentication.

NathanClayton · 2011-11-06T04:18:54Z

By always sending capabilities at connection, some servers immediately throw an error. I've modified the class initialization to include an optional parameter to indicate if this should be disabled.

jelie · 2011-11-06T13:03:02Z

A drawback is that CAPABILITIES will still being unsent after an upgrade of the news server to a more up-to-date version supporting the command.

If CAPABILITIES is not understood by the news server, it is not an issue. A 500 response code is usually answered by the news server, which is RFC-compliant. nntplib should then behave properly. (getcapabilities() returns an empty dictionary when CAPABILITIES is not implemented.)

pitrou · 2011-11-06T13:33:35Z

By always sending capabilities at connection, some servers immediately
throw an error. I've modified the class initialization to include an
optional parameter to indicate if this should be disabled.

Which kind of error? As Julien says, we could simply catch it and
proceed.
By the way, when sending a patch, please send a unified diff if possible
("diff -u"; if you are using Mercurial, this should be the default).

NathanClayton · 2011-11-06T20:56:59Z

When using Easynews, it sends a 480 error (e.g. "nntplib.NNTPTemporaryError: 480 You must log in.")

hynek · 2012-02-09T12:56:26Z

I think I've fixed it to do as described. Alas, I have no Easynews account to test it (I mailed their support about that so maybe that'll change).

Would someone with an account mind to test, if it works? Nathan?

pitrou · 2012-02-12T03:02:16Z

Hynek, the patch doesn't apply properly here. Are you soon it's been generated against an up-to-date working copy?

Also, I think the logic is wrong: the capabilities should *always* be queried after auth, even if they are already known. That's because they can change after auth is successful.

See RFC 4643: “Other capabilities returned in response to a CAPABILITIES
command received after authentication MAY be different from those
returned before authentication. For example, an NNTP server may not
want to advertise support for a specific extension unless a client
has been authenticated.”

pitrou · 2012-02-12T03:02:44Z

(oh, and better if it can come with a test)

hynek · 2012-02-12T10:58:32Z

Sure, I wanted to add tests as soon as I know that the flow is correct (which it isn't :)).

So essentially we want always

CAPABILITIES
LOGIN
CAPABILITIES

?

That's rather simple to achieve. The tests are going to be the harder part. ;)

Re the patch: I tried to generate it using SourceTree but probably did something wrong – will use hg next time again.

hynek · 2012-02-12T13:06:51Z

I've added general tests of AUTH USER/PASS as there weren't any present yet.

As I haven't alas heard back from Easynews, I can only presume they allow CAPABILITIES after a login – I've added a test case that models this behavior.

Otherwise I did as it was asked: getcapabilities() is tolerant to temporary errors and we get them again after a login now.

(I'm also using the remote repo tool now to avoid further patch fuckups – sorry for the extra mails.)

pitrou · 2012-02-12T17:26:31Z

I've made a couple of small comments (see "review" link next to the patch).

hynek · 2012-02-12T18:00:07Z

Thanks! I think I've addressed everything in the latest patch.

python-dev · 2012-02-12T18:18:13Z

New changeset d789f453387d by Antoine Pitrou in branch '3.2':
Issue bpo-10287: nntplib now queries the server's CAPABILITIES again after authenticating (since the result may change, according to RFC 4643).
http://hg.python.org/cpython/rev/d789f453387d

New changeset 98ad67529241 by Antoine Pitrou in branch 'default':
Issue bpo-10287: nntplib now queries the server's CAPABILITIES again after authenticating (since the result may change, according to RFC 4643).
http://hg.python.org/cpython/rev/98ad67529241

pitrou · 2012-02-12T18:18:48Z

Thank you :) Committed now.

pitrou · 2012-02-12T18:19:42Z

Perhaps I closed too quickly... Julien's original message had two other requests:

“If "READER" is advertised, no need to send a "MODE READER" command at all...

If "MODE-READER" is advertised, then "MODE READER" (if wanted) can be sent.
Then, self.capabilities() should be sent again. Capabilities changed!”

hynek · 2012-02-12T18:37:33Z

There's one more issue Julien has raised: nntplib attempts to authenticate when "AUTHINFO" is sent w/o USER. (haven't tested it but I presume it's still valid)

It's trivial to catch that but I'd say that it's fine to let the server handle it if the user has specifically told us to authenticate by giving credentials – no? Maybe if dealing with not-entirely-rfc-compliant servers?

I will look into the READER/MODE-READER part.

hynek · 2012-02-12T21:03:28Z

So my take on the whole issue and Antoine "tends to agree". ;)

If the user tells us (s)he _wants_ us to authenticate or send MODE READER, we do it even if capabilities don't advertise it. There's a lot of rfc-non-conformant servers out there. Permanent errors (i.e. not supported) will be gracefully swallowed.

That's already status quo.

If the server tells us it already _is_ in READER mode, we don't send it because in that case we can assume the server knows what it's doing.

The next patch checks whether we're in READER mode before trying to switch the mode.

I've also added a test for the basic case where a connection is made to a MODE-READER advertising server.

I also added a handler to the nntp2 tests that raises an exception if "mode reader" is called although the server advertises itself as "reader".

If there's a consensus of not sending "MODE READER" if not advertised, it's a matter of two lines to fix that. Personally, I like this middle ground best.

pitrou · 2012-02-14T21:04:16Z

I think the patch is fine. Will apply soon (thanks!)
.

python-dev · 2012-02-14T22:33:49Z

New changeset a7f1ffd741d6 by Antoine Pitrou in branch '3.2':
Issue bpo-10287: nntplib now queries the server's CAPABILITIES first before sending MODE READER, and only sends it if not already in READER mode.
http://hg.python.org/cpython/rev/a7f1ffd741d6

New changeset 03019bb62d83 by Antoine Pitrou in branch 'default':
Issue bpo-10287: nntplib now queries the server's CAPABILITIES first before sending MODE READER, and only sends it if not already in READER mode.
http://hg.python.org/cpython/rev/03019bb62d83

pitrou · 2012-02-14T22:34:12Z

This issue can now be closed for good.

jelie mannequin added the stdlib Python modules in the Lib dir label Nov 1, 2010

pitrou added the type-bug An unexpected behavior, bug, or error label Feb 12, 2012

pitrou closed this as completed Feb 12, 2012

pitrou reopened this Feb 14, 2012

pitrou closed this as completed Feb 14, 2012

ezio-melotti transferred this issue from another repository Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NNTP authentication should check capabilities #54496

NNTP authentication should check capabilities #54496

jelie mannequin commented Nov 1, 2010

jelie mannequin commented Nov 1, 2010

NathanClayton mannequin commented Nov 6, 2011

jelie mannequin commented Nov 6, 2011

pitrou commented Nov 6, 2011

NathanClayton mannequin commented Nov 6, 2011

hynek commented Feb 9, 2012

pitrou commented Feb 12, 2012

pitrou commented Feb 12, 2012

hynek commented Feb 12, 2012

hynek commented Feb 12, 2012

pitrou commented Feb 12, 2012

hynek commented Feb 12, 2012

python-dev mannequin commented Feb 12, 2012

pitrou commented Feb 12, 2012

pitrou commented Feb 12, 2012

hynek commented Feb 12, 2012

hynek commented Feb 12, 2012

pitrou commented Feb 14, 2012

python-dev mannequin commented Feb 14, 2012

pitrou commented Feb 14, 2012

NNTP authentication should check capabilities #54496

NNTP authentication should check capabilities #54496

Comments

jelie mannequin commented Nov 1, 2010

jelie mannequin commented Nov 1, 2010

NathanClayton mannequin commented Nov 6, 2011

jelie mannequin commented Nov 6, 2011

pitrou commented Nov 6, 2011

NathanClayton mannequin commented Nov 6, 2011

hynek commented Feb 9, 2012

pitrou commented Feb 12, 2012

pitrou commented Feb 12, 2012

hynek commented Feb 12, 2012

hynek commented Feb 12, 2012

pitrou commented Feb 12, 2012

hynek commented Feb 12, 2012

python-dev mannequin commented Feb 12, 2012

pitrou commented Feb 12, 2012

pitrou commented Feb 12, 2012

hynek commented Feb 12, 2012

hynek commented Feb 12, 2012

pitrou commented Feb 14, 2012

python-dev mannequin commented Feb 14, 2012

pitrou commented Feb 14, 2012