classification
Title: NNTPS support in nntplib
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.2
process
Status: closed Resolution: fixed
Dependencies: 9360 Superseder:
Assigned To: Nosy List: StevenJ, ajvant, chasonr, christian.heimes, giampaolo.rodola, janssen, jelie, pitrou
Priority: normal Keywords: patch

Created on 2008-01-24 18:41 by chasonr, last changed 2010-11-09 20:10 by ajvant. This issue is now closed.

Files
File name Uploaded Description Edit
python-nntps-patch-1.txt chasonr, 2008-01-24 18:41 Patch for NNTPS support in nntplib
python-nntps-patch-2.txt chasonr, 2008-01-25 20:48
python_nntp_ssl_patch1.diff ajvant, 2010-11-02 08:38 New nntps/starttls patch, for 3.2. review
nntps_only_patch.diff StevenJ, 2010-11-04 01:22
python_nntp_ssl_patch2.diff ajvant, 2010-11-06 11:05 review
Messages (35)
msg61648 - (view) Author: Ray Chason (chasonr) Date: 2008-01-24 18:41
This patch adds SSL support to nntplib.  It is a followup to issue
#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.
msg61654 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-24 20:38
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.
msg61668 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-01-25 02:52
Unfortunately, it uses the deprecated socket.ssl calls.  Re-worked to
use the new SSL module, it would be OK.
msg61684 - (view) Author: Ray Chason (chasonr) Date: 2008-01-25 14:53
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]]]]]]])
msg61685 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-01-25 17:06
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/.
msg61689 - (view) Author: Ray Chason (chasonr) Date: 2008-01-25 20:48
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.
msg61733 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-01-27 02:11
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>
> __________________________________
>
msg61764 - (view) Author: Ray Chason (chasonr) Date: 2008-01-28 02:59
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.
msg61772 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-01-28 04:46
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>
> __________________________________
>
msg98511 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-01-29 14:16
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.
msg115774 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-07 15:21
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
msg115793 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-09-07 18:53
Unfortunately nntplib lacks a test suite.
I created #9791 to address this issue.
msg116382 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-14 11:16
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.
msg120166 - (view) Author: Julien ÉLIE (jelie) Date: 2010-11-01 20:30
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].
msg120209 - (view) Author: Andrew Vant (ajvant) Date: 2010-11-02 08:38
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>
msg120210 - (view) Author: Andrew Vant (ajvant) Date: 2010-11-02 08:43
Also, Julien: Thanks for mentioning the 563-discouraged thing. I added a note about it to the new documentation before submitting.
msg120239 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-11-02 17:05
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.
msg120242 - (view) Author: Julien ÉLIE (jelie) Date: 2010-11-02 18:26
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.
msg120285 - (view) Author: Andrew Vant (ajvant) Date: 2010-11-02 23:18
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.
msg120289 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-11-02 23:22
> > * 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.
msg120295 - (view) Author: Andrew Vant (ajvant) Date: 2010-11-03 00:54
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
msg120334 - (view) Author: Julien ÉLIE (jelie) Date: 2010-11-03 18:10
>> * 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).
msg120368 - (view) Author: (StevenJ) Date: 2010-11-04 01:22
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.
msg120428 - (view) Author: Julien ÉLIE (jelie) Date: 2010-11-04 19:14
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.
msg120463 - (view) Author: (StevenJ) Date: 2010-11-05 01:04
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
msg120525 - (view) Author: Julien ÉLIE (jelie) Date: 2010-11-05 19:30
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.
msg120526 - (view) Author: Julien ÉLIE (jelie) Date: 2010-11-05 19:36
(Note that smtplib can give ideas for an implementation of AUTHINFO SASL with PLAIN, LOGIN and CRAM-MD5 mechanisms.)
msg120606 - (view) Author: Andrew Vant (ajvant) Date: 2010-11-06 11:05
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 (#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.
msg120608 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-11-06 11:48
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.
msg120610 - (view) Author: Julien ÉLIE (jelie) Date: 2010-11-06 11:56
> 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.
msg120612 - (view) Author: Andrew Vant (ajvant) Date: 2010-11-06 13:19
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
msg120641 - (view) Author: (StevenJ) Date: 2010-11-06 17:22
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)
msg120645 - (view) Author: Andrew Vant (ajvant) Date: 2010-11-06 18:22
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
msg120889 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-11-09 18:59
I have committed the patch in r86365, and I've made usenetrc False by default in r86366. Thanks for contributing!
msg120896 - (view) Author: Andrew Vant (ajvant) Date: 2010-11-09 20:10
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
History
Date User Action Args
2010-11-09 20:10:41ajvantsetmessages: + msg120896
2010-11-09 18:59:48pitrousetstatus: open -> closed
resolution: fixed
messages: + msg120889

stage: patch review -> resolved
2010-11-06 18:22:30ajvantsetmessages: + msg120645
2010-11-06 17:23:00StevenJsetmessages: + msg120641
2010-11-06 13:19:25ajvantsetmessages: + msg120612
2010-11-06 11:56:59jeliesetmessages: + msg120610
2010-11-06 11:48:23pitrousetmessages: + msg120608
2010-11-06 11:05:34ajvantsetfiles: + python_nntp_ssl_patch2.diff

messages: + msg120606
2010-11-05 19:36:17jeliesetmessages: + msg120526
2010-11-05 19:30:49jeliesetmessages: + msg120525
2010-11-05 01:06:09brian.curtinsetnosy: - brian.curtin
2010-11-05 01:04:07StevenJsetmessages: + msg120463
2010-11-04 19:14:01jeliesetmessages: + msg120428
2010-11-04 01:22:44StevenJsetfiles: + nntps_only_patch.diff
nosy: + StevenJ
messages: + msg120368

2010-11-03 18:10:18jeliesetmessages: + msg120334
2010-11-03 00:54:10ajvantsetmessages: + msg120295
2010-11-02 23:22:46pitrousetmessages: + msg120289
2010-11-02 23:18:38ajvantsetmessages: + msg120285
2010-11-02 18:26:27jeliesetmessages: + msg120242
2010-11-02 17:05:37pitrousetmessages: + msg120239
2010-11-02 08:43:46ajvantsetmessages: + msg120210
2010-11-02 08:38:33ajvantsetfiles: + python_nntp_ssl_patch1.diff
nosy: + ajvant
messages: + msg120209

2010-11-01 20:30:09jeliesetnosy: + jelie
messages: + msg120166
2010-09-14 11:16:58pitrousetmessages: + msg116382
2010-09-14 11:10:12pitrousetdependencies: + nntplib cleanup
2010-09-07 18:53:00giampaolo.rodolasetmessages: + msg115793
2010-09-07 15:21:37pitrousetversions: - Python 2.7
nosy: + giampaolo.rodola

messages: + msg115774

assignee: janssen ->
stage: test needed -> patch review
2010-09-07 15:18:15pitroulinkissue1053365 superseder
2010-04-20 20:48:49pitrousetfiles: - unnamed
2010-04-20 20:48:46pitrousetfiles: - unnamed
2010-01-29 14:29:08brian.curtinsetnosy: + brian.curtin
2010-01-29 14:16:49pitrousetnosy: + pitrou
messages: + msg98511
2010-01-29 04:32:41brian.curtinsetstage: test needed
versions: + Python 2.7, Python 3.2, - Python 2.6
2008-01-28 04:46:52janssensetfiles: + unnamed
messages: + msg61772
2008-01-28 02:59:34chasonrsetmessages: + msg61764
2008-01-27 02:11:15janssensetfiles: + unnamed
messages: + msg61733
2008-01-25 20:48:55chasonrsetfiles: + python-nntps-patch-2.txt
messages: + msg61689
2008-01-25 17:06:05janssensetmessages: + msg61685
2008-01-25 14:53:56chasonrsetmessages: + msg61684
2008-01-25 02:52:01janssensetmessages: + msg61668
2008-01-24 20:38:56christian.heimessetversions: + Python 2.6, - Python 2.5
nosy: + christian.heimes, janssen
messages: + msg61654
priority: normal
assignee: janssen
keywords: + patch
2008-01-24 18:41:34chasonrcreate