NNTPS support in nntplib

[chasonr]

BPO	1926
Nosy	@pitrou, @giampaolo, @tiran
Dependencies	bpo-9360: nntplib cleanup
Files	python-nntps-patch-1.txt: Patch for NNTPS support in nntplib python-nntps-patch-2.txt python_nntp_ssl_patch1.diff: New nntps/starttls patch, for 3.2. nntps_only_patch.diff python_nntp_ssl_patch2.diff

^{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 2010-11-09.18:59:48.406>
created_at = <Date 2008-01-24.18:41:34.765>
labels = ['type-feature', 'library']
title = 'NNTPS support in nntplib'
updated_at = <Date 2010-11-09.20:10:41.213>
user = 'https://bugs.python.org/chasonr'

bugs.python.org fields:

activity = <Date 2010-11-09.20:10:41.213>
actor = 'ajvant'
assignee = 'none'
closed = True
closed_date = <Date 2010-11-09.18:59:48.406>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2008-01-24.18:41:34.765>
creator = 'chasonr'
dependencies = ['9360']
files = ['9280', '9286', '19463', '19485', '19519']
hgrepos = []
issue_num = 1926
keywords = ['patch']
message_count = 35.0
messages = ['61648', '61654', '61668', '61684', '61685', '61689', '61733', '61764', '61772', '98511', '115774', '115793', '116382', '120166', '120209', '120210', '120239', '120242', '120285', '120289', '120295', '120334', '120368', '120428', '120463', '120525', '120526', '120606', '120608', '120610', '120612', '120641', '120645', '120889', '120896']
nosy_count = 8.0
nosy_names = ['janssen', 'pitrou', 'giampaolo.rodola', 'christian.heimes', 'chasonr', 'ajvant', 'jelie', 'StevenJ']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue1926'
versions = ['Python 3.2']

[chasonr]

This patch adds SSL support to nntplib. It is a followup to issue
bpo-1535659 and addresses the objections raised in the comments to that
issue; it also changes the default port to 563 (nntps) rather than 119
if SSL is requested.

[tiran]

I assign it to janssens. He is our SSL expert. I also set the version to
2.6. New features get into the next major release.

[janssen]

Unfortunately, it uses the deprecated socket.ssl calls. Re-worked to
use the new SSL module, it would be OK.

[chasonr]

OK, I got a copy of the Subversion sources and the new SSL library looks
like just what is needed here. Examining the other protocol modules
that already support SSL, I find these things:

httplib has:
HTTPConnection(host[, port[, strict[, timeout]]])
HTTPSConnection(host[, port[, key_file[, cert_file[, strict[, timeout]]]]])

poplib has:
POP3(host[, port[, timeout]])
POP3_SSL(host[, port[, keyfile[, certfile]]])

imaplib has:
IMAP4([host[, port]])
IMAP4_SSL([host[, port[, keyfile[, certfile]]]])

smtplib has:
SMTP([host[, port[, local_hostname[, timeout]]]])
SMTP_SSL([host[, port[, local_hostname[, keyfile[, certfile[, timeout]]]]]])

Now nntplib as it stands has no SSL-using variant. The existing factory
method is:
NNTP(host[, port[, user[, password[, readermode[, usenetrc]]]]])

What seems most consistent with the other modules is to define:
NNTP_SSL(host[, port[, keyfile[, certfile[, user[, password[,
readermode[, usenetrc]]]]]]])

[janssen]

Sounds good. If you want to develop this with 2.5.1, you can get an
API-compliant version of the SSL module for 2.5.1 from
http://pypi.python.org/pypi/ssl/.

[chasonr]

Here's take 2.

The pre-patch NNTP class has a long and complicated constructor. Rather
than duplicate this constructor in NNTP_SSL, the patch converts most of
the NNTP class to a new class, NNTPBase, which takes an
already-connected socket as a parameter. NNTP and NNTP_SSL both inherit
NNTPBase and create that socket in their own respective ways.

[janssen]

Great, Ray.

I don't see any test cases for the nntp library in the Lib/test/ directory.
How can we make sure it works on the buildbots?

Bill

On Jan 25, 2008 12:49 PM, Ray Chason <report@bugs.python.org> wrote:

Ray Chason added the comment:

Here's take 2.

The pre-patch NNTP class has a long and complicated constructor. Rather
than duplicate this constructor in NNTP_SSL, the patch converts most of
the NNTP class to a new class, NNTPBase, which takes an
already-connected socket as a parameter. NNTP and NNTP_SSL both inherit
NNTPBase and create that socket in their own respective ways.

Added file: http://bugs.python.org/file9286/python-nntps-patch-2.txt

---

Tracker <report@bugs.python.org>
<http://bugs.python.org/issue1926\>

---

[chasonr]

It seems that I, or whoever writes any future test_nntplib.py, would
have to understand how existing tests such as test_smtplib.py work. It
looks like that one is setting up some kind of miniature mail server and
accepting a connection on localhost -- neat trick, binding a server
socket to port 0 so it still works if you have a real mail server running.

Anyway, getting good coverage isn't something I can bang out in an
afternoon. Class NNTPBase and its derivatives (I'm talking post-patch
of course) have lots of methods and I want to cover them as well as
possible.

[janssen]

Yeah, it took me a couple of months to do a reasonable test case for the SSL
code.

Perhaps we could test against an existing NNTP server? Like Google's?

Bill

On Jan 27, 2008 6:59 PM, Ray Chason <report@bugs.python.org> wrote:

Ray Chason added the comment:

It seems that I, or whoever writes any future test_nntplib.py, would
have to understand how existing tests such as test_smtplib.py work. It
looks like that one is setting up some kind of miniature mail server and
accepting a connection on localhost -- neat trick, binding a server
socket to port 0 so it still works if you have a real mail server running.

Anyway, getting good coverage isn't something I can bang out in an
afternoon. Class NNTPBase and its derivatives (I'm talking post-patch
of course) have lots of methods and I want to cover them as well as
possible.

---

Tracker <report@bugs.python.org>
<http://bugs.python.org/issue1926\>

---

[pitrou]

Yes, a test could use Google or gmane (according to the FAQ, nntps is supported: http://gmane.org/faq.php ).
The test should skip gracefully (using the skipTest() API) if the connection fails, so that network errors or service unavailability don't make buildbots go red.

[pitrou]

The patch might need a little reworking to make it work under 3.x, although probably not much.
It should also, IMHO, take an instance of the new SSLContext class (*) as a parameter, rather than the keyfile and certfile args.

(*) http://docs.python.org/dev/library/ssl.html#ssl-contexts

[giampaolo]

Unfortunately nntplib lacks a test suite.
I created bpo-9791 to address this issue.

[pitrou]

It should be noted that there are two possibilities for encrypted NNTP:

• NNTPS, that is NNTP-over-SSL on port 563, as proposed here
• plain NNTP with the STARTTLS capability as described in RFC 4642

For the record, gmane provides the former (on snews.gmane.org:563) but not the latter.

[jelie]

Regarding these two possibilities, please note that the first one is discouraged (per RFC 4642).
STARTTLS is the preferrable way to start a TLS session.

In some existing implementations, TCP port 563 has been dedicated to
NNTP over TLS. These implementations begin the TLS negotiation
immediately upon connection and then continue with the initial steps
of an NNTP session. This use of TLS on a separate port is
discouraged for the reasons documented in Section 7 of "Using TLS
with IMAP, POP3 and ACAP" [TLS-IMAPPOP].

[ajvant]

At Antoine's suggestion I've written a new patch for this for 3.2, adding support for both SSL on 563 and STARTTLS on a normal connection. A NNTP_SSL class supports the former and a NNTP.starttls() method supports the latter. As a side effect of getting starttls working I had to add a NNTP.login() method as well. (see below)

The patch updates nntplib.py, test_nntplib.py, nntplib.rst, and Misc/NEWS.

Some implementation notes:

By default, NNTP objects attempt to log in during initialization,and there didn't seem to be any support for logging in later. This was a problem as STARTTLS has to be issued before logging in. In order to get around this and handle a STARTTLS nuance involving CAPABILITIES, I moved the authentication part of _NNTPBase.__init__ into a method of its own, NNTP.login(), and another piece of it into NNTP.getcapabilities(). This doesn't affect the observable behavior of NNTP() or the existing methods.

As a side note, IMHO NNTP shouldn't attempt to log in at initialization at all. The user, password, and usenetrc args should not be part of the constructors and the program using the library should be able to choose when or if to log in after connecting. I couldn't think of any way to do this without breaking existing programs, though. At the very least I think usenetrc should default to False, but that would break programs that rely on the default behavior too, I think.

I updated the test cases, with a caveat. They use gmane to perform the tests. Gmane doesn't support starttls. Therefor the tests currently only check for correct failure rather than correct success. There's test code for the starttls() method but it never runs. I checked the functionality against eternal-september to be sure it does, in fact, correctly succeed as well, but I'm not about to put my login details in the automatic test script. ;-)

The NNTPS-on-563 test, OTOH, works exactly like the existing NNTP test and seems to work fine. It was a hell of a lot easier to implement that part too.

Hope I didn't miss anything. First time doing this for a public project. Apparently the personal-itch theory is correct.

<click>

[ajvant]

Also, Julien: Thanks for mentioning the 563-discouraged thing. I added a note about it to the new documentation before submitting.

[pitrou]

Andrew, thank you for posting a patch.
I have a couple of comments:

• the command-line option for the SSLContext won't work, since a context is a custom object, not a string; I would rework this part anyway, since I don't think separate options for the "SSL host" are useful (I'd rather add a SSL-enabling flag; also, STARTTLS can be queried from the capabilities)

• on a stylistic note, there are a couple of places where you use tabs for indentation; also, comments should have a space after the '#' ('# xxx' and not '#xxx')

• not moving methods around (such as getwelcome) would make it easier to review the real changes

• in test_starttls, I would clearly report that the server doesn't support STARTTLS, e.g.:

        except nntplib.NNTPPermanentError:
            self.skip("STARTTLS not supported by server")
  

• starttls() should probably test the tls_on attribute first and raise a ValueError if True (as you point out, a client mustn't attempt to start a new TLS session if one is already active).

All in all, it looks good though.

[jelie]

Thanks a lot for having implemented STARTTLS, Andrew!
That's great news!

+ Since the order in which certain operations need to be done varies
+ between normal, SSL, and STARTTLS connections varies, some
+ initialization must be done in the subclasses.

One "varies" is enough.

[ajvant]

On 2 Nov 2010 at 17:05, Antoine Pitrou wrote:

• the command-line option for the SSLContext won't work, since a
  context is a custom object, not a string; I would rework this part
  anyway, since I don't think separate options for the "SSL host" are
  useful (I'd rather add a SSL-enabling flag; also, STARTTLS can be
  queried from the capabilities)

I'm not sure I understand which part you're talking about here....the
tests don't take a context for anything that I can see, and the
mechanism I use in nntplib (a context as a named argument) is the
same one used in smtplib and poplib for <prot>_SSL.

(if you can suggest a method-of-use that breaks it, that may enlighten
me better)

• on a stylistic note, there are a couple of places where you use tabs
  for indentation; also, comments should have a space after the '#' ('#
  xxx' and not '#xxx')

Meh. My editor wasn't set up properly when I started. I thought I'd
eliminated all the tabs. Guess not. There's probably a way to grep
for lines that have tabs in them, I'll fix it.

• not moving methods around (such as getwelcome) would make it easier
  to review the real changes

I'll see what I can do. Some amount of that was necessary to fit
STARTTLS in. (because init assumed you wanted to log in right away
and never again)

• in test_starttls, I would clearly report that the server doesn't
  support STARTTLS, e.g.:

Can do. I didn't know about self.skip; I don't really understand the
testing framework, I was just working by comparison to the other
tests.

• starttls() should probably test the tls_on attribute first and
  raise a ValueError if True (as you point out, a client mustn't attempt
  to start a new TLS session if one is already active).

...I actually meant to do exactly that, but forgot to put the check
in. Thank you.

--

Andrew.

[pitrou]

> * the command-line option for the SSLContext won't work, since a
> context is a custom object, not a string; I would rework this part
> anyway, since I don't think separate options for the "SSL host" are
> useful (I'd rather add a SSL-enabling flag; also, STARTTLS can be
> queried from the capabilities)

I'm not sure I understand which part you're talking about here....the
tests don't take a context for anything that I can see, and the
mechanism I use in nntplib (a context as a named argument) is the
same one used in smtplib and poplib for <prot>_SSL.

I'm talking about the code under "if __name__ == '__main__'".
Specifically the "-c" option for specifying an SSL context.

> * in test_starttls, I would clearly report that the server doesn't
> support STARTTLS, e.g.:

Can do. I didn't know about self.skip; I don't really understand the
testing framework, I was just working by comparison to the other
tests.

Well, actually it's "self.skipTest(...)". My bad.

[ajvant]

On 2 Nov 2010 at 23:22, Antoine Pitrou wrote:

I'm talking about the code under "if __name__ == '__main__'".
Specifically the "-c" option for specifying an SSL context.

Now I feel silly. Partly because of the mistake, partly because I did
test_nntplib.py last and by the time I was done I'd forgotten there
was another section of test code involved.

Thanks for the advice; I'll fix it as best I can in the next day or
two and submit a new version.

--

Andrew

[jelie]

• starttls() should probably test the tls_on attribute first and
  raise a ValueError if True (as you point out, a client mustn't attempt
  to start a new TLS session if one is already active).

...I actually meant to do exactly that, but forgot to put the check
in. Thank you.

And it should also make sure the user is not already authenticated.
And that STARTTLS is advertised (if nntp_version >=2).

[StevenJ]

I too was just looking at NNTPS support because I have a need for it. The latest patch looks like great work and I think the things it implements are needed for this library. But it seems to me that the latest patch combines 3 things which might otherwise be able to be separately considered. NNTPS, START_TLS (RFC 4642) extension and AUTHINFO extension (RFC 4643). It may be that START_TLS and AUTHINFO are indivisible, I need to read those more, but shouldn't that be a new topic of discussion as this feature request is for NNTPS support?

I also don't understand the difficulty with plain NNTPS as it doesn't need a new interface a very simple patch (attached) achieves NNTPS?? (Most of the patch is test case and variant defaults. The actual NNTPS code is just:
+ # Make sure we can actually use ssl if its attempted.
+ if ssl_context:
+ self.sslcontext = ssl_context
+ self.sock = self.sslcontext.wrap_socket(self.sock)

I also don't understand why START_TLS and AUTHINFO need to change how the module is interfaced to (separating log in/authentication, etc), my reading of START_TLS and AUTHINFO seem to me that it should all be under the covers. It even explains this in Section 7 of "Using TLS
with IMAP, POP3 and ACAP" [TLS-IMAPPOP]. That the idea is "It just works". So surely if someone uses this module and they do not specify NNTPS and it supports START_TLS and AUTHINFO and so does the server then it just works. Otherwise it seems a bunch of NNTP Extension requirements and processing spills over to the users of this module when they can probably be contained locally??

Perhaps there needs to be a separate feature request "START_TLS and AUTHINFO extension support for nntplib" so the issues and any necessary interface changes can be considered in isolation from simple NNTP over SSL?

I think it would be nice to have NNTPS in for 3.2.

[jelie]

Hi Steven,

I also don't understand why START_TLS and AUTHINFO need to change
how the module is interfaced to (separating log in/authentication, etc)

Because once you have used AUTHINFO, STARTTLS is no longer a valid command in a session.
The authentication part is currently delt with in the init function (in nntplib) so it needs to be separated because one could want to first use STARTTLS, and then AUTHINFO. Currently, AUTHINFO is sent just after the initial log in; it is therefore better to have AUTHINFO handled in another function.

[StevenJ]

Hi Julien,

> I also don't understand why START_TLS and AUTHINFO need to change
> how the module is interfaced to (separating log in/authentication, etc)

Because once you have used AUTHINFO, STARTTLS is no longer a valid
command in a session.

I understand this, but doesn't this mean the init function needs to change to something like:

init:
if capability STARTTLS is advertised
STARTLS stuff
reget capabilities because STARTTLS changed them probably

Now handle AUTHINFO Stuff

Is there a case where a server advertises STARTTLS and one would not use it? If so then couldn't that just be handled with an option to the class inhibiting it?

This is one of the reasons why I proposed dividing the job across two features.

1. Simple NNTPS which seems to be the most common secured NNTP in use at the moment and is easy to implement, and verify (either as an improvement to the NNTP class or as a new NNTP_SSL class).
2. SASL AUTHINFO/STARTTLS Extension handling implementation and improvements.

There are also other bugs with properly handling capabilities at start related to this, are there not? http://bugs.python.org/issue10287

[jelie]

Hi Steven,

I agree with what you suggest for the implementation.

Is there a case where a server advertises STARTTLS
and one would not use it?

Yes, the overhead added by the encryption. It is what people usually mention for the reason not to use it.
However, the TLS protocol allows to negotiate compression; in this case, it is very powerful! LIST ACTIVE answers are for instance a lot faster. The same for OVER answers.

[jelie]

(Note that smtplib can give ideas for an implementation of AUTHINFO SASL with PLAIN, LOGIN and CRAM-MD5 mechanisms.)

[ajvant]

Here's a second version of the previous patch taking into account the errors Antoine noticed and some odds and ends from the other comments. Specifically:

Comments fixed and tabs (I think...I hope...) all removed.

Added explicit skip to test_nntplib when attempting to test starttls using a server that doesn't support it.

Exceptions raised when issuing STARTTLS for: Authorization already done, TLS already active, server not advertising STARTTLS.

Method moving: I'm not sure why the first patch seemed to think I was e.g. moving getwelcome (I wasn't, as far as I knew), but this time I put the new functions at the end of the class and it's no longer doing it. (still a bit of a mess for the code moved from __init__ to login, starttls, and readermode though)

Selftest cli option: I threw out what I did before (as pointed out, it didn't work anyway) and added a -l flag to use SSL. It uses port 563 for the test only if the user hasn't asked for a non-default port.

I'd like to address some of the objections other users have to the way I implemented it the first time.

NNTPS and STARTTLS separation:

One objection noted that nntps and STARTTLS support are really separate things and should be handled as such. Actually they're probably right. I did both at once because I view them as simply two ways of performing the same function, as someone mentioned earlier in the thread.

Unnecessary Method Separation:

One objection noted that separating out login and starttls was unnecessary and the calling program shouldn't have to worry about these steps. I disagree; I think the caller should be able to choose when and if to log in or engage encryption. (in fact I think usenetrc should be false by default for this reason, but I figure that would break backwards compatibility for programs that rely on it being true by default, and I'm not sure what the rules are regarding this)

More significant than my own potentially newbie-ish opinion is that the RFC suggests as a valid use case the idea of a client starting up TLS or authentication in reaction to a 483 command response, rather than right off the bat. I'm pretty sure this is impossible under the current setup, where login/encryption happens only at initialization and there's no method exposed to do it later.

If I'm going to get overruled on this, I guess the other option is to add a starttls argument to the constructors. If so, I'd suggest it have settings to forbid starttls, use it if it's advertised, or insist on it (by raising an exception if it can't be used). Perhaps false/none/true respectively.

Existing issue for AUTHINFO and MODE READER:

One objection noted there's already an issue open for them (bpo-10287). Which is true, but the AUTHINFO and MODE READER changes were necessary to make STARTTLS work as intended. (in my opinion, I suppose) So I did them as I went along. I'm not sure if I violated custom there, but my apologies if so.

At least having them functioned out will make most of what's mentioned in 10287 easier to fix, I suppose.

Randomness:

BTW, I've been maintaining the readermode_afterauth thing because of the comment next to it, but it sounds from that other issue like it's not supposed to be there at all...which kind of restores my faith in the universe since it smelled bad from the start to me.

[pitrou]

Thanks for the patch, Andrew. It looks mostly good.

I would rename setreadermode to _setreadermode (there's no reason to
make it public IMO). Also, I would not explicitly check "STARTTLS" in
the capabilities. It the server doesn't support it, it will issue an
error anyway. Some servers might support it without advertising it, who
knows?

I can take care of all that when committing, you don't need to submit a
new patch (you can of course, if you want).

(in fact I think usenetrc should be false by default for this reason,
but I figure that would break backwards compatibility for programs
that rely on it being true by default, and I'm not sure what the rules
are regarding this)

In 3.2, the reworked nntplib already breaks compatibility. It is
reasonable to switch the default for usenetrc to False; but I prefer to
do it in a separate commit for clarity.

[jelie]

More significant than my own potentially newbie-ish
opinion is that the RFC suggests as a valid use case
the idea of a client starting up TLS or authentication
in reaction to a 483 command response, rather than right
off the bat.

Yes. (Currently, it would only be TLS with nntplib, because
SASL mechanisms which negotiate an encryption layer are not
implemented yet in nntplib. But in case someone wishes to test,
I have the following capabilities on my news server,
news.trigofacile.com :
AUTHINFO USER SASL
SASL GSSAPI OTP PLAIN NTLM LOGIN DIGEST-MD5 CRAM-MD5
STARTTLS
)

I'm pretty sure this is impossible under the current setup,
where login/encryption happens only at initialization and
there's no method exposed to do it later.

Absolutely.

I've been maintaining the readermode_afterauth thing
[...]
it smelled bad from the start to me.

Yep. According to RFC 4643:

Additionally, the client MUST NOT issue a MODE READER
command after authentication, and a server MUST NOT advertise the
MODE-READER capability.

[ajvant]

On 6 Nov 2010 at 11:48, Antoine Pitrou wrote:

I would rename setreadermode to _setreadermode (there's no reason to
make it public IMO). Also, I would not explicitly check "STARTTLS" in
the capabilities. It the server doesn't support it, it will issue an
error anyway. Some servers might support it without advertising it,
who knows?

I can take care of all that when committing, you don't need to submit
a new patch (you can of course, if you want).

I'd appreciate it if you took care of it, if you feel the patch is otherwise
commit-worthy. It takes a pretty excessive amount of time to convince
myself that it's really doing what I want it to (and only what I want it to).

I forgot to include a note for Misc/NEWS in the new patch version,
by the way.

> (in fact I think usenetrc should be false by default for this
> reason, but I figure that would break backwards compatibility for
> programs that rely on it being true by default, and I'm not sure
> what the rules are regarding this)

In 3.2, the reworked nntplib already breaks compatibility. It is
reasonable to switch the default for usenetrc to False; but I prefer
to do it in a separate commit for clarity.

Okay. For the time being I left a note in the documentation pointing
out that it had to be set False for starttls to be useful.

--

Andrew

[StevenJ]

The only comment I have is, if the caller needs to organise when to auth and instigate tls then for completeness getcapabilities() should have an option to force a reget of the current capabilities, in line with rfc3977 5.2.2:

An NNTP client MAY cache the results of this command, but MUST NOT
rely on the correctness of any cached results, whether from earlier
in this session or from a previous session, MUST cope gracefully
with the cached status being out of date, and SHOULD (if caching
results) provide a way to force the cached information to be
refreshed.

As it stands, the nntplib can cause the cached capabilities to be refreshed at certain points automatically (as it should), but I
think it should be possible for the caller of the method to also specify that fresh capabilities are required and not cached ones.

something like this perhaps? :

mynntp.getcapabilites(refresh=True)

[ajvant]

On 6 Nov 2010 at 17:23, StevenJ wrote:

As it stands, the nntplib can cause the cached capabilities to be
refreshed at certain points automatically (as it should), but I think
it should be possible for the caller of the method to also specify
that fresh capabilities are required and not cached ones.

I agree. I actually added a way to do this when I functioned out
__init__, so that starttls could refresh it when required.

It was ugly and not exposed to the caller though.

something like this perhaps? :

mynntp.getcapabilites(refresh=True)

My method sucked, this one doesn't. Might belong in a separate issue
though. (I feel like a bit of a hypocrite saying that now)

--

Andrew

[pitrou]

I have committed the patch in r86365, and I've made usenetrc False by default in r86366. Thanks for contributing!

[ajvant]

On 9 Nov 2010 at 18:59, Antoine Pitrou wrote:

I have committed the patch in r86365, and I've made usenetrc
False by default in r86366. Thanks for contributing!

Woot. I thank you.

Regarding usenetrc, the NNTP.login and NNTP.starttls documentation
assumed it was true by default (correct when I wrote it) and
mentioned it had to be forced false for starttls to work in some
cases.

I'm glad that's not the case anymore but the documentation as I wrote
it is wrong now. :-)

--

Andrew