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

vstinner · 2022-06-23T13:40:51Z

Remove the keyfile, certfile and check_hostname parameters,
deprecated since Python 3.6, in modules: ftplib, http.client,
imaplib, poplib and smtplib. Use the context parameter (ssl_context
in imaplib) instead.

ftplib: Remove the FTP_TLS.ssl_version class attribute: use the
context parameter instead.

Issue: Remove deprecated keyfile, certfile and check_hostname parameters, in favor of context #94172

vstinner · 2022-06-23T13:42:09Z

cc @tiran @serhiy-storchaka

vstinner · 2022-06-23T13:42:17Z

cc @hugovk

serhiy-storchaka · 2022-06-23T13:49:40Z

Lib/ftplib.py


        def __init__(self, host='', user='', passwd='', acct='',
-                     keyfile=None, certfile=None, context=None,


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

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

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.

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.

serhiy-storchaka · 2022-06-23T13:51:46Z

Lib/ftplib.py

@@ -713,28 +713,13 @@ class FTP_TLS(FTP):
        '221 Goodbye.'
        >>>
        '''
-        ssl_version = ssl.PROTOCOL_TLS_CLIENT


Why is it removed?

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?

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.

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

@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 ;-)

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.

vstinner · 2022-06-23T14:21:56Z

Oh, HTTPSConnection of urllib.request has context and check_hostname parameters, and context can be None. http.client code to create the default context is non trivial, I don't want to copy it in urllib/request.py. Maybe http.client check_hostname parameter should be kept. What do you think @serhiy-storchaka?

vstinner · 2022-06-23T14:22:37Z

Right now, 4 tests are failing because of the urllib issue: test_site test_ssl test_urllib test_urllib2_localnet

serhiy-storchaka · 2022-06-23T15:59:34Z

Lib/ftplib.py

@@ -713,28 +713,13 @@ class FTP_TLS(FTP):
        '221 Goodbye.'
        >>>
        '''
-        ssl_version = ssl.PROTOCOL_TLS_CLIENT


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.

serhiy-storchaka · 2022-06-23T16:01:41Z

Lib/ftplib.py

@@ -749,7 +734,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:


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

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.

Lib/poplib.py

serhiy-storchaka · 2022-06-23T16:28:23Z

Oh, HTTPSConnection of urllib.request has context and check_hostname parameters, and context can be None. http.client code to create the default context is non trivial, I don't want to copy it in urllib/request.py. Maybe http.client check_hostname parameter should be kept.

Specifying check_hostname for urllib.request.HTTPSConnection currently produces a warning, right? Therefore it should be removed.

tiran · 2022-06-23T19:50:23Z

Lib/ftplib.py

@@ -713,28 +713,13 @@ class FTP_TLS(FTP):
        '221 Goodbye.'
        >>>
        '''
-        ssl_version = ssl.PROTOCOL_TLS_CLIENT


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

tiran · 2022-06-23T19:51:40Z

Lib/ftplib.py

@@ -749,7 +734,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:


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

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).

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.

tiran · 2022-06-23T19:52:42Z

Lib/http/client.py

-            if check_hostname is None:
-                check_hostname = context.check_hostname
-            if check_hostname and not will_verify:
+            if context.check_hostname and (context.verify_mode == ssl.CERT_NONE):


You can remove the check. It is performed by SSLContext internally.

vstinner · 2022-06-24T07:02:04Z

I wrote PR #94193 to prepare urllib.request.

vstinner · 2022-06-24T16:28:43Z

URLopener of urllib.request has key_file and cert_file parameters which are passed to http.client.HTTPSConnection. These parameters are not directly deprecated, but the whole URLopener class is deprecated since Python 3.3!

vstinner · 2022-06-24T16:34:16Z

URLopener of urllib.request has key_file and cert_file parameters which are passed to http.client.HTTPSConnection.

I created PR #94232 to fix this issue.

vstinner · 2022-06-26T08:48:03Z

I created PR #94232 to fix this issue.

Merged. I rebased this PR on top of it. Currently, the PR still removes FTP_TLS.ssl_version. This removal seems controversial, I may just revert it.

tiran · 2022-06-26T12:22:35Z

I created PR #94232 to fix this issue.

Merged. I rebased this PR on top of it. Currently, the PR still removes FTP_TLS.ssl_version. This removal seems controversial, I may just revert it.

I suggest to remove it. In a future OpenSSL version it won't be able to change the TLS version. Even now it cannot be used to switch to TLS 1.3-only.

vstinner · 2022-06-26T23:14:47Z

FTP_TLS.ssl_version was added with FTP_TLS by commit ccd5e02 in 2009: patch by @giampaolo, merged by @pitrou.

Changes:

commit 9fe67ce (2015) changed the default from PROTOCOL_TLSv1 to PROTOCOL_SSLv23.
commit a170fa1 (2017) changed the default from PROTOCOL_SSLv23 to PROTOCOL_TLS_CLIENT.

vstinner · 2022-06-26T23:38:44Z

About removing resp = self.voidcmd('AUTH SSL'): I wrote PR #94312 to clarify that Python no longer supports SSLv2.

vstinner · 2022-07-04T14:10:45Z

I tried to run a code search with:

./search_pypi_top.py PYPI-2022-06-24-TOP-5000/ '(keyfile|key_file|certfile|cert_file|check_hostname)=' -o cert_file.txt -q

Problem: these parameter names are common and they are many usages which remain perfectly fine and supported, like (example from Python ssl module documentation):

context.load_cert_chain(certfile="mycertfile", keyfile="mykeyfile")

The code search finds around 1976 lines in 188 projects.

vstinner · 2022-07-05T15:03:33Z

@tiran @serhiy-storchaka: Would you mind to review the updated PR?

serhiy-storchaka

You missed updating the documentation.

Please add also new tests in place of the deleted tests.

serhiy-storchaka · 2022-07-05T17:48:13Z

Lib/imaplib.py

@@ -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]]]])


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.

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.

Lib/poplib.py

Lib/test/test_ftplib.py

Lib/test/test_httplib.py

Remove the keyfile, certfile and check_hostname parameters, deprecated since Python 3.6, in modules: ftplib, http.client, imaplib, poplib and smtplib. Use the context parameter (ssl_context in imaplib) instead. Parameters following the removed parameters become keyword-only parameters. ftplib: Remove the FTP_TLS.ssl_version class attribute: use the context parameter instead.

* poplib: stls() context parameter can be positional again * Keep test_ftplib tests but replace ValueError with TypeError * Reformat test_httplib * Keep but update test_tls13_pha() test

vstinner · 2022-11-02T17:15:33Z

I rebased my PR and addressed @serhiy-storchaka's review.

@serhiy-storchaka: Would you mind to review my updated PR?

vstinner · 2022-11-03T10:04:09Z

cc @hugovk

vstinner · 2022-11-03T17:33:20Z

I merged my PR which was open for 6 months. I prefer to merge it as soon as possible at the beginning of the Python 3.12 development cycle, to get feedback as soon as possible. If someone sees rooms for enhance, I suggest to open a separated PR ;-)

vstinner · 2022-11-03T17:33:57Z

Thanks a lot @tiran and @serhiy-storchaka for the many reviews!

hugovk · 2022-12-22T13:08:02Z

Please see PR #100431 for some docs updates I missed here.

bedevere-bot added the awaiting core review label Jun 23, 2022

vstinner marked this pull request as ready for review June 23, 2022 13:40

vstinner requested a review from a team as a code owner June 23, 2022 13:40

serhiy-storchaka requested a review from tiran June 23, 2022 13:48

serhiy-storchaka reviewed Jun 23, 2022

View reviewed changes

tiran reviewed Jun 23, 2022

View reviewed changes

vstinner mentioned this pull request Jun 24, 2022

gh-94172: urllib.request avoids deprecated check_hostname #94193

Merged

This was referenced Jun 28, 2022

Remove deprecated keyfile, certfile and check_hostname parameters, in favor of context #94172

Closed

ssl: Drop support for unsafe SSLv3 protocol, remove ssl.PROTOCOL_SSLv3 #94377

Closed

serhiy-storchaka reviewed Jul 5, 2022

View reviewed changes

vstinner added 2 commits November 2, 2022 17:51

Address Serhiy's review

1ba8627

* poplib: stls() context parameter can be positional again * Keep test_ftplib tests but replace ValueError with TypeError * Reformat test_httplib * Keep but update test_tls13_pha() test

vstinner merged commit ef0e72b into python:main Nov 3, 2022

bedevere-bot removed the awaiting core review label Nov 3, 2022

vstinner deleted the remove_certfile branch November 3, 2022 17:32

hugovk mentioned this pull request Dec 22, 2022

gh-94172: Update docs for params removed in 3.12 #100431

Merged


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

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

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

Conversation

vstinner commented Jun 23, 2022 • edited by bedevere-bot Loading

vstinner commented Jun 23, 2022

vstinner commented Jun 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Jun 23, 2022

vstinner commented Jun 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka commented Jun 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Jun 24, 2022

vstinner commented Jun 24, 2022

vstinner commented Jun 24, 2022

vstinner commented Jun 26, 2022

tiran commented Jun 26, 2022

vstinner commented Jun 26, 2022

vstinner commented Jun 26, 2022

vstinner commented Jul 4, 2022

vstinner commented Jul 5, 2022

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Nov 2, 2022 • edited Loading

vstinner commented Nov 3, 2022

vstinner commented Nov 3, 2022

vstinner commented Nov 3, 2022

hugovk commented Dec 22, 2022

vstinner commented Jun 23, 2022 •

edited by bedevere-bot

Loading

vstinner commented Nov 2, 2022 •

edited

Loading