SSL/TLS sni use in smtp,pop,imap,nntp,ftp client libs by default

[14786669-223b-4a34-98e8-6cef54412420]

BPO	10852
Nosy	@orsenthil, @pitrou, @giampaolo, @tiran, @alex, @bitdancer, @dstufft
Files	sni-pop-smtp-imap-nntp.patch: sni for other ssl capable libraries issue10852-sni.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: ```python assignee = 'https://github.com/tiran' closed_at = created_at = labels = ['3.7', 'type-feature', 'library'] title = 'SSL/TLS sni use in smtp,pop,imap,nntp,ftp client libs by default' updated_at = user = 'https://bugs.python.org/grooverdan' ``` bugs.python.org fields: ```python activity = actor = 'christian.heimes' assignee = 'christian.heimes' closed = True closed_date = closer = 'christian.heimes' components = ['Library (Lib)'] creation = creator = 'grooverdan' dependencies = [] files = ['20299', '26949'] hgrepos = [] issue_num = 10852 keywords = ['patch'] message_count = 17.0 messages = ['125621', '125623', '125647', '125670', '126920', '126929', '126958', '168632', '168781', '168782', '168784', '168850', '177261', '181804', '181890', '275024', '275026'] nosy_count = 11.0 nosy_names = ['janssen', 'orsenthil', 'pitrou', 'giampaolo.rodola', 'christian.heimes', 'alex', 'r.david.murray', 'grooverdan', 'daniel-black', 'dstufft', 'fweimer'] pr_nums = [] priority = 'high' resolution = 'out of date' stage = 'resolved' status = 'closed' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue10852' versions = ['Python 3.6', 'Python 3.7'] ```

[14786669-223b-4a34-98e8-6cef54412420]

Like r85793, sni is enabled by default for url and https classes. This continues the consistency throughout the python libraries by adding it to other places where wrap_socket is used to instigate a SSL/TLS connection.

[14786669-223b-4a34-98e8-6cef54412420]

dup bpo-10853

[pitrou]

Oops, I hadn't noticed you had closed it.

[pitrou]

I understand this patch relies on bpo-10851. As I said there, I would rather have SSLContext become the primary API, and the stdlib standardize on it. Part of the stdlib, as you have witnessed, already allows the user to pass a custom SSLContext, which is very simple way of allowing for custom user settings. There are two open issues for imaplib (bpo-8808) and smtplib (bpo-8809).

[14786669-223b-4a34-98e8-6cef54412420]

ok. should library/ssl.rst be updated to use a SSLContext example?

[pitrou]

Well, there are already such examples: http://docs.python.org/dev/library/ssl.html#examples Do you think they are not visible enough?

[14786669-223b-4a34-98e8-6cef54412420]

I thought previous comments you wanted SSLContext to become the primary api rather than wrap_socket? Coding this into the examples is probably a good way of making this happen.

[a1c2f9a2-bc1b-443d-9fee-08b2050cafec]

Antoine,

I copied off your http example for all the other protocols.

tested with:

import smtplib

a = smtplib.SMTP_SSL('gmail-smtp-in.l.google.com.')
a.starttls()

a = smtplib.SMTP_SSL('mail.internode.on.net')
a = smtplib.SMTP_SSL('smtp.gmail.com')

import ftplib
# http://secureftp-test.com/

f = ftplib.FTP_TLS('ftp.secureftp-test.com')
f.auth()

import imaplib
i = imaplib.IMAP4('calmail.berkley.edu')
i.starttls()

i = imaplib.IMAP4_SSL('mail.internode.on.net')

import poplib

p = poplib.POP3_SSL('calmail.berkley.edu')

import  nntplib 
n = nntplib.NNTP_SSL('news.internode.on.net')

I did a network capture and saw the hostname in the SNI header

[a1c2f9a2-bc1b-443d-9fee-08b2050cafec]

previous patch had dumb error and even failed test suit. Now fixed.

[pitrou]

Thanks for the patch, Daniel. 3.3 is nearing the release candidate phase, so I'm re-targetting to 3.4. I'll take a detailed look soon.

(I suppose there's no easy way to write automated tests for this, unfortunately)

[pitrou]

By the way, could you sign a contributor agreement? You can find instructions at http://www.python.org/psf/contrib/

[a1c2f9a2-bc1b-443d-9fee-08b2050cafec]

Thanks for the patch Daniel. 3.3 is nearing the release candidate phase, so I'm re-targetting to 3.4. I'll take a detailed look soon.

Welcome. Just noticed conflicts with bpo-4473 in the client POP implementation. Hopefully they are close anyway.

(I suppose there's no easy way to write automated tests for this, unfortunately) Well since bpo-8109 writes the server SNI its getting easier.

In Lib/test/test_ssl.py combined with the changes of bpo-8109 it would seem that changing ConnectionHandler.run to respond to "AUTH TLS", "AUTH SSL" (ftp) and "STLS" for pop (preempt bpo-4473).

Changing server_params_test to support a proper arguments that correspond the the client protocol would be the way to do it.

By the way, could you sign a contributor agreement yes - emailed in.

[a1c2f9a2-bc1b-443d-9fee-08b2050cafec]

the one error in the previous review corrected.

[pitrou]

I'm getting a test failure in test_ftplib:

\====================================================================== ERROR: test_data_connection (test.test_ftplib.TestTLS_FTPClass) ----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/antoine/cpython/default/Lib/test/test_ftplib.py", line 834, in test_data_connection
    with self.client.transfercmd('list') as sock:
  File "/home/antoine/cpython/default/Lib/ftplib.py", line 386, in transfercmd
    return self.ntransfercmd(cmd, rest)[0]
  File "/home/antoine/cpython/default/Lib/ftplib.py", line 756, in ntransfercmd
    self.context.load_cert_chain(self.certfile, self.keyfile)
TypeError: certfile should be a valid filesystem path

Also, since we now have SNI server support, perhaps it's easier to test the change :-)

[14786669-223b-4a34-98e8-6cef54412420]

Ack. Have fix. Simple if self.certfile or self.keyfile: test added before load_cert_chain.

part way through developing test. Thinking bpo-17181 would help.

[tiran]

Good idea, but the patch is outdated. We can enforce verification by changing ssl._create_stdlib_context.

[tiran]

Oh sorry, this is about SNI not verified context. All protocols support SNI for some time.