◐ Shell
reader mode source ↗
Skip to content

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

Merged
vstinner merged 2 commits into
python:mainfrom
vstinner:remove_certfile
Nov 3, 2022
Merged

gh-94172: Remove keyfile, certfile and check_hostname parameters#94173
vstinner merged 2 commits into
python:mainfrom
vstinner:remove_certfile

Conversation

@vstinner

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

Copy link
Copy Markdown
Member

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

vstinner commented Jun 23, 2022

Copy link
Copy Markdown
Member Author

cc @tiran @serhiy-storchaka

@vstinner

vstinner commented Jun 23, 2022

Copy link
Copy Markdown
Member Author

cc @hugovk

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

vstinner commented Jun 23, 2022

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

vstinner commented Jun 23, 2022

Copy link
Copy Markdown
Member Author

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

@serhiy-storchaka

serhiy-storchaka commented Jun 23, 2022

Copy link
Copy Markdown
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.

@vstinner vstinner mentioned this pull request Jun 24, 2022
Merged
@vstinner

vstinner commented Jun 24, 2022

Copy link
Copy Markdown
Member Author

I wrote PR #94193 to prepare urllib.request.

@vstinner

vstinner commented Jun 24, 2022

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

vstinner commented Jun 24, 2022

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

vstinner commented Jun 26, 2022

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

tiran commented Jun 26, 2022

Copy link
Copy Markdown
Member

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

vstinner commented Jun 26, 2022

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

vstinner commented Jun 26, 2022

Copy link
Copy Markdown
Member Author

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

This was referenced Jun 28, 2022
Closed
Closed
@vstinner

vstinner commented Jul 4, 2022

Copy link
Copy Markdown
Member Author

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

vstinner commented Jul 5, 2022

Copy link
Copy Markdown
Member Author

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

serhiy-storchaka
serhiy-storchaka reviewed Jul 5, 2022
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

You missed updating the documentation.

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

@vstinner
gh-94172: Remove keyfile, certfile and check_hostname parameters …
e4089dc 
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.
@vstinner
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

vstinner commented Nov 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

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

@vstinner

vstinner commented Nov 3, 2022

Copy link
Copy Markdown
Member Author

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

vstinner commented Nov 3, 2022

Copy link
Copy Markdown
Member Author

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

vstinner commented Nov 3, 2022

Copy link
Copy Markdown
Member Author

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

@hugovk hugovk mentioned this pull request Dec 22, 2022
Merged
@hugovk

hugovk commented Dec 22, 2022

Copy link
Copy Markdown
Member

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

donbarbos added a commit to donbarbos/typeshed that referenced this pull request Jan 11, 2026
@donbarbos
[stdlib] Deprecate keyfile, certfile and check_hostname parameters …
46f2965 
Source: python/cpython#94173
@donbarbos donbarbos mentioned this pull request Jan 11, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tiran tiran tiran left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vstinner @serhiy-storchaka @tiran @hugovk @bedevere-bot