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-79156: Add start_tls() method to streams API #91453

Merged
merged 2 commits into from
Apr 15, 2022

Conversation

arhadthedev
Copy link
Member

A reupload of gh-13143 with the ported whatsnew since the original author is waiting for three years already. For strange reason, the original branch is missing and the PR has no corresponding "{username} deleted the {branch} branch" notification line.

The PR is revived after a thread on python-dev:

Oleg Iarygin

Hi everyone,

Can PR gh-13143 be reopened and reviewed/merged when possible? It was closed in favor of the new asyncio streams API (gh-13251). Later, the API was cleanly removed (gh-16482) so the proposed PR became relevant again.

The added method allows to turn encryption on on demand (and off, as the author expressed his readiness). It's required for a CPython test suit, to port test.test_ftplib.DummyFTPServer from asyncore/asynchat before their removal according to PEP 594 (Removing dead batteries from the standard library).

Thank you in advance.

Jelle Zijlstra

I can't reopen the PR; I assume the branch was deleted. It's going to be easier to just open a new PR.

Linked to gh-79156; the original issue may be closed with the following stop_tls() PR.

Co-authored-by: Ian Good icgood@gmail.com

The existing event loop `start_tls()` method is not sufficient for
connections using the streams API. The existing StreamReader works
because the new transport passes received data to the original protocol.
The StreamWriter must then write data to the new transport, and the
StreamReaderProtocol must be updated to close the new transport
correctly.

The new StreamWriter `start_tls()` updates itself and the reader
protocol to the new SSL transport.
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Apr 11, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@arhadthedev
Copy link
Member Author

Ian Good signed the CLA before publishing the original PR.

@AlexWaygood
Copy link
Member

Ian Good signed the CLA before publishing the original PR.

Cc. @ambv

@ambv
Copy link
Contributor

ambv commented Apr 11, 2022

Sorry for the trouble! I'm off work now, I added this case to look at tomorrow morning.

@arhadthedev arhadthedev marked this pull request as draft April 11, 2022 20:38
@arhadthedev arhadthedev marked this pull request as ready for review April 12, 2022 06:00
@arhadthedev
Copy link
Member Author

Fixed a usage of the loop parameter removed in 3.10. Ready for review.

@icgood
Copy link
Contributor

icgood commented Apr 15, 2022

Just noticed this -- @arhadthedev @ambv anything I can do to help?

@ambv
Copy link
Contributor

ambv commented Apr 15, 2022

@icgood, can you click on the red button in #91453 (comment) to re-sign the CLA?

@ambv ambv merged commit 6217864 into python:main Apr 15, 2022
@arhadthedev arhadthedev deleted the asyncio-starttls branch April 15, 2022 12:28
@arhadthedev
Copy link
Member Author

@ambv Thank you a lot for merging!

The paired function, shutdown_tls is on the way. After that I'll close the issue.

handshake to complete before aborting the connection. ``60.0`` seconds
if ``None`` (default).

.. versionadded:: 3.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 3.11 isn't it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, missed it. Good catch, @kumaraditya303. @arhadthedev, you could fix this while working on shutdown_tls.

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

Successfully merging this pull request may close these issues.

None yet

6 participants