Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-94172: Remove keyfile, certfile and check_hostname parameters #94173

Merged
merged 2 commits into from
Nov 3, 2022
Merged

gh-94172: Remove keyfile, certfile and check_hostname parameters #94173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4089dc
gh-94172: Remove keyfile, certfile and check_hostname parameters
vstinner Jun 23, 2022
1ba8627
Address Serhiy's review
vstinner Nov 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Doc/whatsnew/3.12.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,16 @@ Removed
<https://github.com/sphinx-contrib/sphinx-lint>`_.
(Contributed by Julien Palard in :gh:`98179`.)

* Remove the *keyfile*, *certfile* and *check_hostname* parameters, deprecated
since Python 3.6, in modules: :mod:`ftplib`, :mod:`http.client`,
:mod:`imaplib`, :mod:`poplib` and :mod:`smtplib`. Use the *context* parameter
(*ssl_context* in :mod:`imaplib`) instead.
(Contributed by Victor Stinner in :gh:`94172`.)

* :mod:`ftplib`: Remove the ``FTP_TLS.ssl_version`` class attribute: use the
*context* parameter instead.
(Contributed by Victor Stinner in :gh:`94172`.)


Porting to Python 3.12
======================
Expand Down
24 changes: 4 additions & 20 deletions Lib/ftplib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,28 +713,12 @@ class FTP_TLS(FTP):
'221 Goodbye.'
>>>
'''
ssl_version = ssl.PROTOCOL_TLS_CLIENT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you provide a SSLContext, you get different protocol than FTP_TLS.ssl_version. This removal is not required, it's more to make the API consistent with the other modified modules: the SSLContext constructor is the only place setting default parameter values. Do you prefer to keep it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know whether it is documented, but it specifies the default protocol if you do not provide an SSLContext. The user can override it in FTP_TLS or subclasses.

Who added this attribute? Ask him.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All protocols except PROTOCOL_TLS, TLS_CLIENT, and TLS_SERVER are deprecated anyways because OpenSSL is going to remove them eventually.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiran: Do you see a reason to keep ssl_version to specify a different "ssl version" when context parameter is omitted?

The practical problem is more that applications relying on this feature may not notice that overriden ssl_version are now ignored silently.

Note: I hate mentions of "ssl", since SSL no longer exists, it was replaced with TLS, but the Python module is still called "ssl"... as OpenSSL ;-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no reason to keep the variable here. ssl.PROTOCOL_TLS_CLIENT is the only correct and fully supported value. In 3.13 I will remove all the other protocols. Only ssl.PROTOCOL_TLS_CLIENT and ssl.PROTOCOL_TLS_SERVER will stay.


def __init__(self, host='', user='', passwd='', acct='',
keyfile=None, certfile=None, context=None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you remove positional parameters, you have to make the following parameters keyword-only.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, done. I didn't document this change. Is it worth to document it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were similar changes documented in the past?

Please add tests that only such positional arguments are accepted. In most cases None is a valid argument both for the first deleted parameter and for the first remaining parameter, so you can just add None as an excessive positional argument.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were similar changes documented in the past?

In commit 9baf242, I removed a buffering parameter in bz2.BZ2File. I see that I wrote ".. versionchanged:: 3.9 (...) The compresslevel parameter became keyword-only." in the doc.

Honestly, I don't think that it should be documented. I don't expect people to call such constructor with 6 parameters or more. I'm only aware of people who called CodeType() with tons of arguments: problem solved with CodeType.replace() method addition.

timeout=_GLOBAL_DEFAULT_TIMEOUT, source_address=None, *,
encoding='utf-8'):
if context is not None and keyfile is not None:
raise ValueError("context and keyfile arguments are mutually "
"exclusive")
if context is not None and certfile is not None:
raise ValueError("context and certfile arguments are mutually "
"exclusive")
if keyfile is not None or certfile is not None:
import warnings
warnings.warn("keyfile and certfile are deprecated, use a "
"custom context instead", DeprecationWarning, 2)
self.keyfile = keyfile
self.certfile = certfile
*, context=None, timeout=_GLOBAL_DEFAULT_TIMEOUT,
source_address=None, encoding='utf-8'):
if context is None:
context = ssl._create_stdlib_context(self.ssl_version,
certfile=certfile,
keyfile=keyfile)
context = ssl._create_stdlib_context()
self.context = context
self._prot_p = False
super().__init__(host, user, passwd, acct,
Expand All @@ -749,7 +733,7 @@ def auth(self):
'''Set up secure control connection by using TLS/SSL.'''
if isinstance(self.sock, ssl.SSLSocket):
raise ValueError("Already using TLS")
if self.ssl_version >= ssl.PROTOCOL_TLS:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is the right way to do, should this change be backported as a bug fix? I think it deserves a separate issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. Sadly, I don't know well the ftplib module. I'm not comfortable to change it in a stable Python version. In case of doubt, I prefer to leave the code as it is in stable branches, unless someone reports a bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SSLv2 and SSLv3 are no longer supported. You can remove the AUTH SSL block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I read Modules/_ssl.c, it's not obvious that SSL is no longer supported. There is still conditonal code:

#ifndef OPENSSL_NO_SSL2
    PyModule_AddIntConstant(m, "PROTOCOL_SSLv2",
                            PY_SSL_VERSION_SSL2);
#endif
#ifndef OPENSSL_NO_SSL3
    PyModule_AddIntConstant(m, "PROTOCOL_SSLv3",
                            PY_SSL_VERSION_SSL3);
#endif

These constants are still documented but marked as "deprecated" in the doc: https://docs.python.org/dev/library/ssl.html#ssl.PROTOCOL_SSLv2

I would prefer to fully remove support for SSL in the _ssl/ssl modules, before changing the ftplib module.

Another example: https://docs.python.org/dev/library/ssl.html#ssl.OP_NO_SSLv3 still exists (at least in the doc).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's documented in ssl.rst. I'm using the word "supported" in the sense of a social contract. The presence of the constants do not mean that a user can expect they are doing anything sensible.

if self.context.protocol >= ssl.PROTOCOL_TLS:
resp = self.voidcmd('AUTH TLS')
else:
resp = self.voidcmd('AUTH SSL')
Expand Down
25 changes: 3 additions & 22 deletions Lib/http/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1414,33 +1414,14 @@ class HTTPSConnection(HTTPConnection):

default_port = HTTPS_PORT

# XXX Should key_file and cert_file be deprecated in favour of context?

def __init__(self, host, port=None, key_file=None, cert_file=None,
timeout=socket._GLOBAL_DEFAULT_TIMEOUT,
source_address=None, *, context=None,
check_hostname=None, blocksize=8192):
def __init__(self, host, port=None,
*, timeout=socket._GLOBAL_DEFAULT_TIMEOUT,
source_address=None, context=None, blocksize=8192):
super(HTTPSConnection, self).__init__(host, port, timeout,
source_address,
blocksize=blocksize)
if (key_file is not None or cert_file is not None or
check_hostname is not None):
import warnings
warnings.warn("key_file, cert_file and check_hostname are "
"deprecated, use a custom context instead.",
DeprecationWarning, 2)
self.key_file = key_file
self.cert_file = cert_file
if context is None:
context = _create_https_context(self._http_vsn)
if check_hostname is not None:
context.check_hostname = check_hostname
if key_file or cert_file:
context.load_cert_chain(cert_file, key_file)
# cert and key file means the user wants to authenticate.
# enable TLS 1.3 PHA implicitly even for custom contexts.
if context.post_handshake_auth is not None:
context.post_handshake_auth = True
self._context = context

def connect(self):
Expand Down
25 changes: 4 additions & 21 deletions Lib/imaplib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,40 +1285,23 @@ class IMAP4_SSL(IMAP4):

"""IMAP4 client class over SSL connection

Instantiate with: IMAP4_SSL([host[, port[, keyfile[, certfile[, ssl_context[, timeout=None]]]]]])
Instantiate with: IMAP4_SSL([host[, port[, ssl_context[, timeout=None]]]])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is completely redundant, because the output of pydoc already contains the constructor signature. The same in other classes in this module, and maybe in other modules.

I would prefer to remove this line and edit the following parameters descriptions, and do it before merging this PR to make backporting easier.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is already big enough. If you want to enhance the documentation, please change it in a separated change. I don't see how updating this doc is a pre-requirement to remove the two parameters.


host - host's name (default: localhost);
port - port number (default: standard IMAP4 SSL port);
keyfile - PEM formatted file that contains your private key (default: None);
certfile - PEM formatted certificate chain file (default: None);
ssl_context - a SSLContext object that contains your certificate chain
and private key (default: None)
Note: if ssl_context is provided, then parameters keyfile or
certfile should not be set otherwise ValueError is raised.
timeout - socket timeout (default: None) If timeout is not given or is None,
the global default socket timeout is used

for more documentation see the docstring of the parent class IMAP4.
"""


def __init__(self, host='', port=IMAP4_SSL_PORT, keyfile=None,
certfile=None, ssl_context=None, timeout=None):
if ssl_context is not None and keyfile is not None:
raise ValueError("ssl_context and keyfile arguments are mutually "
"exclusive")
if ssl_context is not None and certfile is not None:
raise ValueError("ssl_context and certfile arguments are mutually "
"exclusive")
if keyfile is not None or certfile is not None:
import warnings
warnings.warn("keyfile and certfile are deprecated, use a "
"custom ssl_context instead", DeprecationWarning, 2)
self.keyfile = keyfile
self.certfile = certfile
def __init__(self, host='', port=IMAP4_SSL_PORT,
*, ssl_context=None, timeout=None):
if ssl_context is None:
ssl_context = ssl._create_stdlib_context(certfile=certfile,
keyfile=keyfile)
ssl_context = ssl._create_stdlib_context()
self.ssl_context = ssl_context
IMAP4.__init__(self, host, port, timeout)

Expand Down
26 changes: 5 additions & 21 deletions Lib/poplib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,35 +419,19 @@ def stls(self, context=None):
class POP3_SSL(POP3):
"""POP3 client class over SSL connection

Instantiate with: POP3_SSL(hostname, port=995, keyfile=None, certfile=None,
context=None)
Instantiate with: POP3_SSL(hostname, port=995, context=None)

hostname - the hostname of the pop3 over ssl server
port - port number
keyfile - PEM formatted file that contains your private key
certfile - PEM formatted certificate chain file
context - a ssl.SSLContext

See the methods of the parent class POP3 for more documentation.
"""

def __init__(self, host, port=POP3_SSL_PORT, keyfile=None, certfile=None,
timeout=socket._GLOBAL_DEFAULT_TIMEOUT, context=None):
if context is not None and keyfile is not None:
raise ValueError("context and keyfile arguments are mutually "
"exclusive")
if context is not None and certfile is not None:
raise ValueError("context and certfile arguments are mutually "
"exclusive")
if keyfile is not None or certfile is not None:
import warnings
warnings.warn("keyfile and certfile are deprecated, use a "
"custom context instead", DeprecationWarning, 2)
self.keyfile = keyfile
self.certfile = certfile
def __init__(self, host, port=POP3_SSL_PORT,
*, timeout=socket._GLOBAL_DEFAULT_TIMEOUT, context=None):
if context is None:
context = ssl._create_stdlib_context(certfile=certfile,
keyfile=keyfile)
context = ssl._create_stdlib_context()
self.context = context
POP3.__init__(self, host, port, timeout)

Expand All @@ -457,7 +441,7 @@ def _create_socket(self, timeout):
server_hostname=self.host)
return sock

def stls(self, keyfile=None, certfile=None, context=None):
def stls(self, context=None):
vstinner marked this conversation as resolved.
Show resolved Hide resolved
"""The method unconditionally raises an exception since the
STLS command doesn't make any sense on an already established
SSL/TLS session.
Expand Down
42 changes: 7 additions & 35 deletions Lib/smtplib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,14 +749,14 @@ def login(self, user, password, *, initial_response_ok=True):
# We could not login successfully. Return result of last attempt.
raise last_exception

def starttls(self, keyfile=None, certfile=None, context=None):
def starttls(self, *, context=None):
"""Puts the connection to the SMTP server into TLS mode.

If there has been no previous EHLO or HELO command this session, this
method tries ESMTP EHLO first.

If the server supports TLS, this will encrypt the rest of the SMTP
session. If you provide the keyfile and certfile parameters,
session. If you provide the context parameter,
the identity of the SMTP server and client can be checked. This,
however, depends on whether the socket module really checks the
certificates.
Expand All @@ -774,19 +774,8 @@ def starttls(self, keyfile=None, certfile=None, context=None):
if resp == 220:
if not _have_ssl:
raise RuntimeError("No SSL support included in this Python")
if context is not None and keyfile is not None:
raise ValueError("context and keyfile arguments are mutually "
"exclusive")
if context is not None and certfile is not None:
raise ValueError("context and certfile arguments are mutually "
"exclusive")
if keyfile is not None or certfile is not None:
import warnings
warnings.warn("keyfile and certfile are deprecated, use a "
"custom context instead", DeprecationWarning, 2)
if context is None:
context = ssl._create_stdlib_context(certfile=certfile,
keyfile=keyfile)
context = ssl._create_stdlib_context()
self.sock = context.wrap_socket(self.sock,
server_hostname=self._host)
self.file = None
Expand Down Expand Up @@ -1017,35 +1006,18 @@ class SMTP_SSL(SMTP):
compiled with SSL support). If host is not specified, '' (the local
host) is used. If port is omitted, the standard SMTP-over-SSL port
(465) is used. local_hostname and source_address have the same meaning
as they do in the SMTP class. keyfile and certfile are also optional -
they can contain a PEM formatted private key and certificate chain file
for the SSL connection. context also optional, can contain a
SSLContext, and is an alternative to keyfile and certfile; If it is
specified both keyfile and certfile must be None.
as they do in the SMTP class. context also optional, can contain a
SSLContext.

"""

default_port = SMTP_SSL_PORT

def __init__(self, host='', port=0, local_hostname=None,
keyfile=None, certfile=None,
timeout=socket._GLOBAL_DEFAULT_TIMEOUT,
*, timeout=socket._GLOBAL_DEFAULT_TIMEOUT,
source_address=None, context=None):
if context is not None and keyfile is not None:
raise ValueError("context and keyfile arguments are mutually "
"exclusive")
if context is not None and certfile is not None:
raise ValueError("context and certfile arguments are mutually "
"exclusive")
if keyfile is not None or certfile is not None:
import warnings
warnings.warn("keyfile and certfile are deprecated, use a "
"custom context instead", DeprecationWarning, 2)
self.keyfile = keyfile
self.certfile = certfile
if context is None:
context = ssl._create_stdlib_context(certfile=certfile,
keyfile=keyfile)
context = ssl._create_stdlib_context()
self.context = context
SMTP.__init__(self, host, port, local_hostname, timeout,
source_address)
Expand Down
6 changes: 3 additions & 3 deletions Lib/test/test_ftplib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -984,11 +984,11 @@ def test_context(self):
ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
ctx.check_hostname = False
ctx.verify_mode = ssl.CERT_NONE
self.assertRaises(ValueError, ftplib.FTP_TLS, keyfile=CERTFILE,
self.assertRaises(TypeError, ftplib.FTP_TLS, keyfile=CERTFILE,
context=ctx)
self.assertRaises(ValueError, ftplib.FTP_TLS, certfile=CERTFILE,
self.assertRaises(TypeError, ftplib.FTP_TLS, certfile=CERTFILE,
context=ctx)
self.assertRaises(ValueError, ftplib.FTP_TLS, certfile=CERTFILE,
self.assertRaises(TypeError, ftplib.FTP_TLS, certfile=CERTFILE,
keyfile=CERTFILE, context=ctx)

self.client = ftplib.FTP_TLS(context=ctx, timeout=TIMEOUT)
Expand Down
43 changes: 12 additions & 31 deletions Lib/test/test_httplib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1978,7 +1978,7 @@ def test_local_unknown_cert(self):
self.assertEqual(exc_info.exception.reason, 'CERTIFICATE_VERIFY_FAILED')

def test_local_good_hostname(self):
# The (valid) cert validates the HTTP hostname
# The (valid) cert validates the HTTPS hostname
import ssl
server = self.make_server(CERT_localhost)
context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
Expand All @@ -1991,46 +1991,29 @@ def test_local_good_hostname(self):
self.assertEqual(resp.status, 404)

def test_local_bad_hostname(self):
# The (valid) cert doesn't validate the HTTP hostname
# The (valid) cert doesn't validate the HTTPS hostname
import ssl
server = self.make_server(CERT_fakehostname)
context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
context.load_verify_locations(CERT_fakehostname)
h = client.HTTPSConnection('localhost', server.port, context=context)
with self.assertRaises(ssl.CertificateError):
h.request('GET', '/')
# Same with explicit check_hostname=True
with warnings_helper.check_warnings(('', DeprecationWarning)):
h = client.HTTPSConnection('localhost', server.port,
context=context, check_hostname=True)

# Same with explicit context.check_hostname=True
context.check_hostname = True
h = client.HTTPSConnection('localhost', server.port, context=context)
with self.assertRaises(ssl.CertificateError):
h.request('GET', '/')
# With check_hostname=False, the mismatching is ignored
context.check_hostname = False
with warnings_helper.check_warnings(('', DeprecationWarning)):
h = client.HTTPSConnection('localhost', server.port,
context=context, check_hostname=False)
h.request('GET', '/nonexistent')
resp = h.getresponse()
resp.close()
h.close()
self.assertEqual(resp.status, 404)
# The context's check_hostname setting is used if one isn't passed to
# HTTPSConnection.

# With context.check_hostname=False, the mismatching is ignored
context.check_hostname = False
h = client.HTTPSConnection('localhost', server.port, context=context)
h.request('GET', '/nonexistent')
resp = h.getresponse()
self.assertEqual(resp.status, 404)
resp.close()
h.close()
# Passing check_hostname to HTTPSConnection should override the
# context's setting.
with warnings_helper.check_warnings(('', DeprecationWarning)):
h = client.HTTPSConnection('localhost', server.port,
context=context, check_hostname=True)
with self.assertRaises(ssl.CertificateError):
h.request('GET', '/')
self.assertEqual(resp.status, 404)

@unittest.skipIf(not hasattr(client, 'HTTPSConnection'),
'http.client.HTTPSConnection not available')
Expand Down Expand Up @@ -2066,11 +2049,9 @@ def test_tls13_pha(self):
self.assertIs(h._context, context)
self.assertFalse(h._context.post_handshake_auth)

with warnings.catch_warnings():
warnings.filterwarnings('ignore', 'key_file, cert_file and check_hostname are deprecated',
DeprecationWarning)
h = client.HTTPSConnection('localhost', 443, context=context,
cert_file=CERT_localhost)
context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT, cert_file=CERT_localhost)
context.post_handshake_auth = True
h = client.HTTPSConnection('localhost', 443, context=context)
self.assertTrue(h._context.post_handshake_auth)


Expand Down
21 changes: 0 additions & 21 deletions Lib/test/test_imaplib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,15 +573,6 @@ def test_ssl_verified(self):
ssl_context=ssl_context)
client.shutdown()

# Mock the private method _connect(), so mark the test as specific
# to CPython stdlib
@cpython_only
def test_certfile_arg_warn(self):
with warnings_helper.check_warnings(('', DeprecationWarning)):
with mock.patch.object(self.imap_class, 'open'):
with mock.patch.object(self.imap_class, '_connect'):
self.imap_class('localhost', 143, certfile=CERTFILE)

class ThreadedNetworkedTests(unittest.TestCase):
server_class = socketserver.TCPServer
imap_class = imaplib.IMAP4
Expand Down Expand Up @@ -1070,18 +1061,6 @@ def test_logout(self):
rs = _server.logout()
self.assertEqual(rs[0], 'BYE', rs)

def test_ssl_context_certfile_exclusive(self):
with socket_helper.transient_internet(self.host):
self.assertRaises(
ValueError, self.imap_class, self.host, self.port,
certfile=CERTFILE, ssl_context=self.create_ssl_context())

def test_ssl_context_keyfile_exclusive(self):
with socket_helper.transient_internet(self.host):
self.assertRaises(
ValueError, self.imap_class, self.host, self.port,
keyfile=CERTFILE, ssl_context=self.create_ssl_context())


if __name__ == "__main__":
unittest.main()
Loading