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

Jump to bottom

start_tls() difficult when using asyncio.start_server() #79156

Closed
icgood mannequin opened this issue Oct 13, 2018 · 10 comments
Closed

start_tls() difficult when using asyncio.start_server() #79156

icgood mannequin opened this issue Oct 13, 2018 · 10 comments
Assignees
@asvetlov
Labels
3.11 only security fixes topic-asyncio type-feature A feature request or enhancement

Comments

@icgood
Copy link
Mannequin

icgood mannequin commented Oct 13, 2018

BPO 34975
Nosy @gpshead, @asvetlov, @1st1, @icgood
PRs
  • bpo-34975: Add start_tls() method to streams API #13143
    		•
    Superseder
  • bpo-36889: Merge StreamWriter and StreamReader into just asyncio.Stream
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/asvetlov'
closed_at = None
created_at = <Date 2018-10-13.20:03:13.550>
labels = ['3.11', 'type-feature', 'expert-asyncio']
title = 'start_tls() difficult when using asyncio.start_server()'
updated_at = <Date 2022-04-08.00:05:00.270>
user = 'https://github.com/icgood'

    bugs.python.org fields:

    activity = <Date 2022-04-08.00:05:00.270>
actor = 'gregory.p.smith'
assignee = 'asvetlov'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2018-10-13.20:03:13.550>
creator = 'icgood'
dependencies = []
files = []
hgrepos = []
issue_num = 34975
keywords = ['patch']
message_count = 9.0
messages = ['327665', '327674', '327678', '342012', '342215', '343679', '355586', '355587', '416953']
nosy_count = 4.0
nosy_names = ['gregory.p.smith', 'asvetlov', 'yselivanov', 'icgood']
pr_nums = ['13143']
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = '36889'
type = 'enhancement'
url = 'https://bugs.python.org/issue34975'
versions = ['Python 3.11']
    @icgood
    Copy link
    Mannequin Author

    icgood mannequin commented Oct 13, 2018

    There does not seem to be a public API for replacing the transport of the StreamReader / StreamWriter provided to the callback of a call to asyncio.start_server().

    The only way I have found to use the new SSL transport is to update protected members of the StreamReaderProtocol object, e.g.:

        async def callback(reader, writer):
        loop = asyncio.get_event_loop()
        transport = writer.transport
        protocol = transport.get_protocol()
        new_transport = await loop.start_tls(
            transport, protocol, ssl_context, server_side=True)
        protocol._stream_reader = StreamReader(loop=loop)
        protocol._client_connected_cb = do_stuff_after_start_tls
        protocol.connection_made(new_transport)

    async def do_stuff_after_start_tls(ssl_reader, ssl_writer): ...

    @icgood icgood mannequin added 3.7 (EOL) end of life topic-asyncio labels Oct 13, 2018
    @asvetlov
    Copy link
    Contributor

    Thanks for raising the problem.
    I'm in the middle of streams API refactoring.

    https://github.com/asvetlov/cpython/blob/async-streams/Lib/asyncio/streams.py#L801-L812 is a draft.

    A help is very appreciated.
    Would you pick up this snippet and make a pull request with tests and documentation update?

    @asvetlov asvetlov added 3.8 only security fixes and removed 3.7 (EOL) end of life labels Oct 13, 2018
    @asvetlov asvetlov self-assigned this Oct 13, 2018
    @1st1
    Copy link
    Member

    1st1 commented Oct 13, 2018

    One thing: I'm -1 on adding starttls to current stream api; let's add it only to the new one (same for sendfile)

    @icgood
    Copy link
    Mannequin Author

    icgood mannequin commented May 9, 2019

    I added start_tls() to StreamWriter. My implementation returns a new StreamWriter that should be used from then on, but it could be adapted to modify the current writer in-place (let me know).

    I've added docs, an integration test, and done some additional "real-world" testing with an IMAP server I work on. Specifically, "openssl s_client -connect xxx -starttls imap" works like a charm, and no errors/warnings are logged on disconnect.

    @icgood icgood mannequin added the type-feature A feature request or enhancement label May 9, 2019
    @asvetlov
    Copy link
    Contributor

    I believe stream transport should be replaced.
    If you have time please do this change.

    Also please take a look at bpo-36889 for merging StreamWriter and StreamReader.
    It can simplify the replacement strategy.

    Anyway, please keep your PR open at least.
    The plan is: either merge your PR before writer/reader merging or land unified streams first and adopt the PR to new API.

    Another thing that should be done before 3.8 feature freeze is support for sendfile in streams API. Hopefully, it is much simpler: no need for transport changing etc.

    @asvetlov
    Copy link
    Contributor

    Fixed by bpo-36889

    @icgood
    Copy link
    Mannequin Author

    icgood mannequin commented Oct 28, 2019

    bpo-36889 was reverted, so this is not resolved.

    I'm guessing this needs to be moved to 3.9 now too. Is my original PR worth revisiting? https://github.com/python/cpython/pull/13143/files

    @icgood icgood mannequin reopened this Oct 28, 2019
    @asvetlov
    Copy link
    Contributor

    I think it should be closed; Yuri thinks that current streams API is frozen for the sake of shiny brand new streams (don't exist yet).

    @gpshead
    Copy link
    Member

    gpshead commented Apr 8, 2022

    https://discuss.python.org/t/need-reconsideration-of-bpo-34975-add-start-tls-method-to-streams-api/14720 would like to see this reconsidered. reopening.

    @gpshead gpshead added 3.11 only security fixes and removed 3.8 only security fixes labels Apr 8, 2022
    @gpshead gpshead reopened this Apr 8, 2022
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @arhadthedev arhadthedev mentioned this issue Apr 11, 2022
    Merged
    ambv pushed a commit that referenced this issue Apr 15, 2022
    @arhadthedev @icgood
    gh-79156: Add start_tls() method to streams API (#91453)
    6217864 
    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.

Co-authored-by: Ian Good <icgood@gmail.com>
    @kumaraditya303 kumaraditya303 mentioned this issue Apr 16, 2022
    Closed
    @kumaraditya303
    Copy link
    Contributor

    Fixed by #91453

    @arhadthedev arhadthedev mentioned this issue Oct 4, 2022
    Closed
    @gvanrossum gvanrossum mentioned this issue Oct 4, 2022
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @asvetlov asvetlov

    Labels
    3.11 only security fixes topic-asyncio type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @gpshead @1st1 @asvetlov @kumaraditya303