smtplib should support SSL contexts

[pitrou]

BPO	8809
Nosy	@terryjreedy, @jcea, @pitrou, @giampaolo, @bitdancer
Files	smtp_sslcontext.patch smtp_sslcontext_updated.patch smtp_sslcontext_updated2.patch smtp_sslcontext_updated3.patch smtp_sslcontext_test.patch

^{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 2011-05-18.16:05:43.624>
created_at = <Date 2010-05-24.16:15:41.699>
labels = ['type-feature', 'library']
title = 'smtplib should support SSL contexts'
updated_at = <Date 2011-05-18.16:17:41.783>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2011-05-18.16:17:41.783>
actor = 'kasun'
assignee = 'none'
closed = True
closed_date = <Date 2011-05-18.16:05:43.624>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2010-05-24.16:15:41.699>
creator = 'pitrou'
dependencies = []
files = ['21730', '21737', '21781', '21791', '22003']
hgrepos = []
issue_num = 8809
keywords = ['patch']
message_count = 18.0
messages = ['106369', '133463', '133474', '134098', '134120', '134121', '134124', '134181', '134443', '134479', '134481', '134528', '135529', '135561', '135981', '136248', '136249', '136250']
nosy_count = 10.0
nosy_names = ['terry.reedy', 'jcea', 'janssen', 'pitrou', 'giampaolo.rodola', 'r.david.murray', 'asdfasdfasdfasdfasdfasdfasdf', 'python-dev', 'thomas.scrace', 'kasun']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue8809'
versions = ['Python 3.3']

[pitrou]

3.2 introduces SSL contexts, which allow bundling SSL configuration options, certificates and private keys into a single (potentially long-lived) structure.
http://docs.python.org/dev/py3k/library/ssl.html#ssl.SSLContext

The SMTP_SSL constructor should allow passing an SSL context object instead of a key/cert pair.

[thomasscrace]

Is anybody working on this issue? If not, I think it looks like it might be a nice one for me to tackle. I'll go ahead unless there are any objections.

[pitrou]

Is anybody working on this issue? If not, I think it looks like it
might be a nice one for me to tackle. I'll go ahead unless there are
any objections.

Nobody is working on it AFAIK. Feel free to give it a try :)

[kasun]

I did a patch that allows smtplib.SMTP_SSL constructor to accept an optional SSLContext

[terryjreedy]

This sentence is awkward, at best.
"Alternative to a private key and a certificate key an optional SSLContext could be specified."

I suggest something like: "SSLcontext, also optional, is an alternative to keyfile and certfile; if it is specified, keyfile and certfile must be None."

[asdfasdfasdfasdfasdfasdfasdf]

It should also explain how the context can be used.
An example of how to use it to establish a 'secured' connection would be a nice to have.

[kasun]

I'm submitting a new patch with changes suggested by Terry. But I feel an example of how to use a SSLContext inside the Docstring of SMTP_SSL constructor is unnecessary since no smtp specific things are done to the passed SSLContext. Any ideas about this suggestion?

[pitrou]

Thanks for the patch. A couple of comments:

• it would be nice to have some tests, but of course there are no tests for SMTP_SSL currently. If you are motivated, perhaps you could investigate into adding some.
• there are some tab characters in your patch. Please only use spaces for indentation (see PEP-8 if you have any doubts); make sure your editor is configured for that :)
• starttls() should probably get a context parameter too. You can take a look at imaplib for an example.
• you need to update the API docs in Doc/library/smtplib.rst

I don't think an example of using a context is necessary. By using proper ReST markup a link will be generated to the SSLContext documentation, and that's probably enough for people to understand.

[kasun]

I did another patch based on feedback given. A test for SMTP_SSL wasn't added; will look into it later. Tab character that were present are removed. Also SSLContext support was added to starttls() but it is slightly different from the starttls() implementation of imaplib. Further feedback would be much appreciated.

[pitrou]

For the record, bpo-11927 reminds me that test_smtpnet actually has a trivial test for SMTP-over-SSL.

[pitrou]

So, thanks for the new patch. I tested it with my ISP's server and it works fine!

Two remaining issues though:

• in the SMTP_SSL constructor, you must add the "context" argument at the end of the argument list (after "timeout"), otherwise someone passing "timeout" as a positional argument will find their code breaking
• in the doc, you need a "versionchanged" tag for the "context" argument (in both instances)

Thank you again.

[kasun]

Thanks for the quick review. I'm submitting a new patch with changes suggested.

[pitrou]

Thank you, the patch looks good. A simple test could probably be added to test_smtpnet, along the lines of the existing tests, do you want to tackle that?

[kasun]

Yes, I would like to have a try at it

[kasun]

I added a test to smtpnet and submitting a separate patch for it as my test_smtpnet.py file was old and could have had conflicts.

I didn't use keys and certificates for the SSLContext as those would have to be shipped with the source. Looking forward for reviews and suggestions.

[python-dev]

New changeset 6737de76487b by Antoine Pitrou in branch 'default':
Issue bpo-8809: The SMTP_SSL constructor and SMTP.starttls() now support
http://hg.python.org/cpython/rev/6737de76487b

[pitrou]

Thank you Kasun. I did a couple of minor style fixes (whitespace at end of line, and spaces around the '=' assignment sign). I also added a test for starttls(), since it turns out gmail.com supports it on port 25.

[kasun]

Thanks Antoine