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

New SSL implementation based on ssl.MemoryBIO #66750

Closed
pitrou opened this issue Oct 5, 2014 · 28 comments
Closed

New SSL implementation based on ssl.MemoryBIO #66750

pitrou opened this issue Oct 5, 2014 · 28 comments
Labels
expert-asyncio type-feature A feature request or enhancement

Comments

@pitrou
Copy link
Member

pitrou commented Oct 5, 2014

BPO 22560
Nosy @gvanrossum, @pitrou, @vstinner, @giampaolo, @1st1
Files
  • sslproto.patch
  • sslproto3.patch
  • sslproto-4.patch
  • 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 = None
    closed_at = <Date 2015-01-15.08:46:17.477>
    created_at = <Date 2014-10-05.22:28:22.765>
    labels = ['type-feature', 'expert-asyncio']
    title = 'New SSL implementation based on ssl.MemoryBIO'
    updated_at = <Date 2015-03-23.15:00:17.902>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2015-03-23.15:00:17.902>
    actor = 'berker.peksag'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-01-15.08:46:17.477>
    closer = 'vstinner'
    components = ['asyncio']
    creation = <Date 2014-10-05.22:28:22.765>
    creator = 'pitrou'
    dependencies = []
    files = ['36821', '36945', '37638']
    hgrepos = []
    issue_num = 22560
    keywords = ['patch']
    message_count = 28.0
    messages = ['228628', '228632', '228634', '228652', '229507', '229835', '229916', '232169', '233282', '233283', '233299', '233300', '233301', '233302', '233303', '233306', '233311', '233617', '233618', '233631', '233697', '233971', '233972', '233973', '234041', '234064', '234065', '234066']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'geertj', 'pitrou', 'vstinner', 'giampaolo.rodola', 'python-dev', 'sbt', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue22560'
    versions = ['Python 3.5']

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 5, 2014

    Now that bpo-21965 is implemented, it is possible to improve SSL support in asyncio by making it independent of how the underlying event loop works (e.g. whether it is a Unix-like reactor or a proactor).

    @pitrou pitrou added expert-asyncio type-feature A feature request or enhancement labels Oct 5, 2014
    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 6, 2014

    Here is a proof-of-concept patch. I've only tested it under Linux, but it should be possible to write a simple _make_ssl_transport() for the BaseProactorEventLoop.

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Oct 6, 2014

    This is awesome news!

    Since this is 3.5 only, I guess this means the end of my attempts to keep the asyncio source code identical in the Tulip repo (from which I occasionally create builds that work with Python 3.3) and in the 3.4 and 3.5 branches. I guess that's okay, people should be switching to 3.5 anyway. I'm not sure what approach to follow to keep the branches at least somewhat in sync -- perhaps tulip and 3.4 can still be kept identical, with changes merged from 3.4 into 3.5 (default) but 3.5 differing in some places? Or perhaps the code can be kept identical with the exception of the sslproto.py file, and conditional import of the latter?

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 6, 2014

    Or perhaps the code can be kept identical with the exception of the
    sslproto.py file, and conditional import of the latter?

    I think that's reasonable, yes. The _SelectorSslTransport is still there and can be used if the ssl module is not recent enough.

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 16, 2014

    Here is an updated patch. It hooks into the Proactor event loop (tested under Windows) and also adds a fallback for older Pythons (with tests).

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 22, 2014

    Does someone want to review this?

    @vstinner
    Copy link
    Member

    vstinner commented Oct 24, 2014

    I will try to take a look next week.

    @pitrou
    Copy link
    Member Author

    pitrou commented Dec 5, 2014

    From bpo-22768:

    """

    Maybe
    transport.get_extra_info('socket').getpeercert(True)
    would be okay, no patch needed?

    That will be problematic with bpo-22560. The clear-text socket object and the SSL object become unrelated, and it would be logical for get_extra_info('socket') to return the clear-text socket, so either a get_extra_info('ssl') would be needed, or we should expose the SSL properties directly as extra info members.
    """

    @pitrou
    Copy link
    Member Author

    pitrou commented Jan 1, 2015

    Ping :-)

    @pitrou
    Copy link
    Member Author

    pitrou commented Jan 1, 2015

    Note this could probably help https://twitter.com/icgood/status/549915951165358080, which Victor seems to care about :-)

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Jan 1, 2015

    Maybe we should just accept this without review? I really don't have time to review 600+ lines of code, sorry.

    @vstinner
    Copy link
    Member

    vstinner commented Jan 2, 2015

    Maybe we should just accept this without review? I really don't have time to review 600+ lines of code, sorry.

    SSL/TLS is very important and the patch is large, a review is required. I posted a first review with a lot of comments.

    @vstinner
    Copy link
    Member

    vstinner commented Jan 2, 2015

    Sorry for the delay. I understood that the change targets the proactor event loop, and I was busy to fix annoying random bugs in this code (it's not done yet, see for example the issue bpo-23095 for the most recent bug). Windows is not my favorite OS, I am less intersted to fix Windows specific bugs, but sometimes I try to fix them.

    I still don't understand if the change is (or should be) specific to the proactor event loop or not. Antoine, can you please elaborate the rationale of your patch?

    Is the "legacy" code only used on Python 3.4 and older? Is ssl.MemoryBIO always present in Python 3.5 and newer?

    I would like to see benchmarks of memory BIO vs current code on Linux and Windows. Even if it would make the code simpler to always use memory BIO, I care of network performances. Maybe we may only use memory BIO for the proactor event loop?

    Do you know if other applications use memory BIO for networking with SSL?

    Did you try your patch on Python 3.3? On Linux and Windows?

    Anyway, great job!

    @vstinner
    Copy link
    Member

    vstinner commented Jan 2, 2015

    Note this could probably help https://twitter.com/icgood/status/549915951165358080, which Victor seems to care about :-)

    Copy of the tweet: "@gvanrossum Will we be seeing TLS upgrade support (e.g. STARTTLS) soon in asyncio / tulip? All threads and issues on the subject seem stale."

    How will this patch help to support STARTTLS? Could you please elaborate Antoine?

    @vstinner
    Copy link
    Member

    vstinner commented Jan 2, 2015

    FYI Twisted supports SSL with IOCP using pyOpenSSL 0.10 (released in 2009) or newer. The support is based on twisted.protocols.tls.TLSMemoryBIOFactory.

    It looks like the memory BIO implementation is now preferred on all platforms. See the twisted.internet._newtls module:
    """
    This module implements memory BIO based TLS support. It is the preferred
    implementation and will be used whenever pyOpenSSL 0.10 or newer is installed
    (whenever L{twisted.protocols.tls} is importable).

    @SInCE: 11.1
    """

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Jan 2, 2015

    Oh, I think I understand how this could help STARTTLS. Glyph once explained it to me. STARTTLS takes an existing non-TLS Transport and layers a TLS Transport on top of it. This requires the TLS layer to read/write from the underlying Transport using the standard Transport/Protocol interface (i.e. call transport.write() to write bytes, expect protocol.data_received() to be called when bytes are read). The existing (3.4) ssl module cannot do this because the TLS implementation needs to wrap the socket directly; but (presumably) the BIO-based TLS implementation can do this.

    @pitrou
    Copy link
    Member Author

    pitrou commented Jan 2, 2015

    Antoine, can you please elaborate the rationale of your patch?

    The patch adds SSL support for proactor-based event loops (any event loop supporting plain sockets, actually, so it could also work for libuv etc.).

    Is the "legacy" code only used on Python 3.4 and older? Is ssl.MemoryBIO always present in Python 3.5 and newer?

    Yes and yes.

    I would like to see benchmarks of memory BIO vs current code on Linux and Windows.

    Do you have such benchmarks?

    Maybe we may only use memory BIO for the proactor event loop?

    It sounds better to exercise the same code path under all platforms.

    Did you try your patch on Python 3.3?

    No.

    How will this patch help to support STARTTLS?

    Guido explained this one :-)

    @vstinner
    Copy link
    Member

    vstinner commented Jan 8, 2015

    I updated sslproto3.patch with my remarks: sslproto-4.patch

    Main differences with sslproto3.patch (unsorted):

    • write_eof raises NotImplementedError
    • fix write_buffer_size: use data, not offset
    • use tuples in the write backlog
    • data_received exits the loop when it gets an empty string (ignore following data, but an empty string must be at the end of the list according to Antoine)
    • SSLPipe._write_backlog is now a deque (should be more efficient since we remove the head of the list in _process_write_backlog)
    • always store and pass the server hostname, even if SNI is not supported
    • deny calling shutdown() twice
    • Remove ConnectionAbortedError special case: I would like to reproduce to understand it, and document it
    • Set the default read parameter to 256 KB instead of 64 KB: reuse constant from selector_events.py
    • SSLPipe: use a different attribute to store the shutdown callback

    Remarks:

    • sslproto looks to be based on gruvi/ssl.py of the gruvi project written by Geert
      Jansen. We should mention the author in the commit.
    • _process_write_backlog(): I don't understand why offset is set to 1 for handshake and shutdown

    @vstinner
    Copy link
    Member

    vstinner commented Jan 8, 2015

    I prefer to use the same code on all platforms. I don't like the idea of SSL bugs specific to Windows.

    With this change, it becomes possible to support "STARTTLS". IMO supporting this feature is more important than performance, even if I only expect a low overflow of the new _SSLPipe layer, so the benchmark is not needed.

    @vstinner
    Copy link
    Member

    vstinner commented Jan 8, 2015

    Oh, I wrote the patch for Tulip. Patch regenerated to use Python paths.

    @vstinner
    Copy link
    Member

    vstinner commented Jan 8, 2015

    For STARTTLS, see also this issue:
    https://code.google.com/p/tulip/issues/detail?id=79

    @vstinner vstinner changed the title Add loop-agnostic SSL implementation to asyncio New SSL implementation based on ssl.MemoryBIO Jan 9, 2015
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 13, 2015

    New changeset 432b817611f2 by Victor Stinner in branch '3.4':
    Issue bpo-22560: New SSL implementation based on ssl.MemoryBIO
    https://hg.python.org/cpython/rev/432b817611f2

    @vstinner
    Copy link
    Member

    vstinner commented Jan 13, 2015

    I commited sslproto-4.patch to Python 3.4, Python 3.5 and Tulip with minor changes:

    • remove write_eof(), as suggested by Yury
    • define SSLProtocol after its transport in sslproto.py
    • add a docstring to SSLProtocol
    • test sslcontext with "is not None"
    • _make_ssl_transport: waiter parameter is now optional

    Thanks Antoine for this great contribution! Thanks also to Geert Jansen since sslproto.py looks to be based on his work.

    Even if I ran the test suite on Python 3.3, 3.4 and 3.5 on Linux, and on Python 3.5 on Windows, I prefer to wait one or two days to see how buildbots appreciate this enhancement.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 13, 2015

    New changeset b9fbbe7103e7 by Victor Stinner in branch 'default':
    Issue bpo-22560, asyncio doc: ProactorEventLoop now supports SSL!
    https://hg.python.org/cpython/rev/b9fbbe7103e7

    @vstinner
    Copy link
    Member

    vstinner commented Jan 14, 2015

    While I tried to write an unit test to reproduce a bug (cancelled waiter), I noticed that the handshake callback of the protocol can be called indirectly from _process_write_backlog(). So _process_write_backlog() may be called indirectly from _process_write_backlog(), whereas this function doesn't support reentrant call.

    A fix is to modify the handshake callback to schedule a call to _process_write_backlog(), instead of calling it immediatly.

    Note: The shutdown callback doesn't have this issue, it only calls transport.close().

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 15, 2015

    New changeset fb3761de0d3c by Victor Stinner in branch '3.4':
    Issue bpo-22560: Fix SSLProtocol._on_handshake_complete()
    https://hg.python.org/cpython/rev/fb3761de0d3c

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 15, 2015

    New changeset 3c37825d85d3 by Victor Stinner in branch '3.4':
    Issue bpo-22560: Fix typo: call -> call_soon
    https://hg.python.org/cpython/rev/3c37825d85d3

    @vstinner
    Copy link
    Member

    vstinner commented Jan 15, 2015

    Buildbots are happy. There is no remaining things to do, I close the issue.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    expert-asyncio type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants