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.

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

merged 2 commits into from
Nov 3, 2022

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jun 23, 2022

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.

@vstinner vstinner marked this pull request as ready for review June 23, 2022 13:40
@vstinner vstinner requested a review from a team as a code owner June 23, 2022 13:40
@vstinner
Copy link
Member Author

cc @tiran @serhiy-storchaka

@vstinner
Copy link
Member Author

cc @hugovk

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

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.

@@ -713,28 +713,13 @@ 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.

@vstinner
Copy link
Member Author

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
Copy link
Member Author

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

@@ -713,28 +713,13 @@ 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.

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.

@@ -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:
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.

Lib/poplib.py Show resolved Hide resolved
@serhiy-storchaka
Copy link
Member

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.

@@ -713,28 +713,13 @@ 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.

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

@@ -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:
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.

Comment on lines 1429 to 1417
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):
Copy link
Member

Choose a reason for hiding this comment

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

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

@vstinner
Copy link
Member Author

I wrote PR #94193 to prepare urllib.request.

@vstinner
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

tiran commented Jun 26, 2022

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
Copy link
Member Author

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
Copy link
Member Author

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

@vstinner
Copy link
Member Author

vstinner commented Jul 4, 2022

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
Copy link
Member Author

vstinner commented Jul 5, 2022

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

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

You missed updating the documentation.

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

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

Lib/poplib.py Outdated Show resolved Hide resolved
Lib/test/test_ftplib.py Outdated Show resolved Hide resolved
Lib/test/test_httplib.py Outdated Show resolved Hide resolved
Lib/test/test_httplib.py Outdated Show resolved Hide resolved
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
Copy link
Member Author

vstinner commented Nov 2, 2022

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

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

@vstinner
Copy link
Member Author

vstinner commented Nov 3, 2022

cc @hugovk

@vstinner vstinner merged commit ef0e72b into python:main Nov 3, 2022
@vstinner vstinner deleted the remove_certfile branch November 3, 2022 17:32
@vstinner
Copy link
Member Author

vstinner commented Nov 3, 2022

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
Copy link
Member Author

vstinner commented Nov 3, 2022

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

@hugovk
Copy link
Member

hugovk commented Dec 22, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants