Repository navigation
Add support for Memory BIO to _ssl #66164
Description
| BPO | 21965 |
|---|---|
| Nosy | @gvanrossum, @pitrou, @vstinner, @giampaolo, @tiran, @ezio-melotti, @alex, @bdarnell, @1st1, @dstufft |
| Files |
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 2014-10-05.22:22:59.394>
created_at = <Date 2014-07-12.09:08:54.520>
labels = ['extension-modules', 'type-feature']
title = 'Add support for Memory BIO to _ssl'
updated_at = <Date 2014-10-06.10:15:13.661>
user = 'https://bugs.python.org/geertj'bugs.python.org fields:
activity = <Date 2014-10-06.10:15:13.661>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2014-10-05.22:22:59.394>
closer = 'pitrou'
components = ['Extension Modules']
creation = <Date 2014-07-12.09:08:54.520>
creator = 'geertj'
dependencies = []
files = ['35928', '36189', '36191', '36248', '36475', '36483', '36791', '36806']
hgrepos = []
issue_num = 21965
keywords = ['patch']
message_count = 39.0
messages = ['222833', '223518', '223597', '224478', '224480', '224704', '224723', '224724', '224733', '224734', '224834', '224835', '224926', '224932', '224952', '225103', '225893', '225895', '225910', '225949', '226131', '226184', '226237', '226939', '228321', '228322', '228324', '228481', '228504', '228506', '228509', '228579', '228583', '228614', '228620', '228623', '228624', '228626', '228654']
nosy_count = 15.0
nosy_names = ['gvanrossum', 'geertj', 'janssen', 'pitrou', 'vstinner', 'giampaolo.rodola', 'christian.heimes', 'ezio.melotti', 'alex', 'python-dev', 'sbt', 'Ben.Darnell', 'yselivanov', 'dstufft', 'chatgris']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue21965'
versions = ['Python 3.5']
Activity
geertj commentedon Jul 12, 2014
The attached patch adds a _MemoryBIO type to _ssl, and a _wrap_bio() method to _SSLContext. The patch also includes tests.
For now I kept _wrap_bio() and _MemoryBIO semi-private. The reason is that it returns an _SSLSocket instead of an SSLSocket and this type has not been exposed before as part of the public API. Changing the result of _wrap_bio to return an SSLSocket is not appropriate IMHO because it should not inherit from socket.socket which would waste a file descriptor and None of the IO methods are relevant.
The patch works for me and gives no errors with --with-pydebug. I've also used it in an experimental branch of Gruvi and all the tests pass there too.
geertj commentedon Jul 20, 2014
Hi all (pitrou, haypo and all others) can I get some feedback on this patch?
Thanks!
pitrou commentedon Jul 21, 2014
The C part of the patch looks roughly ok to me (modulo a couple of comments). However, we must now find a way to expose this as a Python-level API.
geertj commentedon Aug 1, 2014
I added a new patch that addresses the comments.
geertj commentedon Aug 1, 2014
I've explored a few options for the Python-level API in the attachment "bio_python_options.py".
Me personally I prefer the more light weight option #3. This is both out of selfish interest (less work for me), but also I believe that memory BIOs are an API that will be used almost exclusively by framework authors, not by end users like SSLSocket itself. So a more lower-level (but perfectly valid IMHO) API would be appropriate.
geertj commentedon Aug 4, 2014
New patch with a Python-level API (option #3).
This needs some more tests, and docs.
pitrou commentedon Aug 4, 2014
I think the API choice looks reasonable, thank you (haven't looked at the patch in detail). A question though: does it support server-side SNI? AFAIR server-side SNI requires you to be able to change a SSL object's context.
pitrou commentedon Aug 4, 2014
Am adding the asyncio maintainers as well as Ben Darnell (Tornado) to the nosy list, for feedback.
geertj commentedon Aug 4, 2014
Yes, it does. See the following comment in _servername_callback():
/* Pass a PySSLSocket instance when using memory BIOs, but an ssl.SSLSocket
The C-level _ssl._SSLSocket object is passed to the servername callback. It has a "context" property that can be set.
I realize the above is an abstraction violation between the C and Python level. Now that we have an SSLObject Python level API, I could update the code to store a weakref to the SSLObject in the _SSLSocket (just like it does for SSLSocket). That way I can pass the Python level object into the callback. Any thoughts?
pitrou commentedon Aug 4, 2014
Le 04/08/2014 11:21, Geert Jansen a écrit :
I think it would make the exposed API nicer, although the implementation
would be a bit uglier. Given Python's philosophy, I think the nicer API
wins :-)
bdarnell commentedon Aug 5, 2014
Looks good to me. I've added exarkun and glyph to the nosy list since Twisted's experience with PyOpenSSL may provide useful feedback even though Twisted will presumably stick with what they've got instead of switching to this new interface.
pitrou commentedon Aug 5, 2014
By the way, this would allow ProactorEventLoop to support SSL, since it decouples the SSL protocol handling from the actual socket I/O.
exarkun commentedon Aug 6, 2014
Please do *not* add me to the nosy list of any issues.
13 remaining items
geertj commentedon Oct 3, 2014
I think the improved test coverage might still make it worthwhile. All tests are now exercising the SSLObject methods via SSLSocket. Also it's more future proof as the risk is less that you'd add a new method to SSLSocket without adding it to SSLObject as well.
It's not clear cut. Either way is fine I think.
That is to support the first argument passed to the sever name callback set with set_servername_callback(). This will be an SSLSocket or an SSLObject instance depending on who's using it.
pitrou commentedon Oct 4, 2014
One issue with the "owner" is that there is now a reference cycle between SSLSocket and SSLObject (something which the original design is careful to avoid by using weakrefs in the _ssl module).
geertj commentedon Oct 4, 2014
Note that owner is a weakref :) Did you look at the code?
pitrou commentedon Oct 4, 2014
Ahhh. I had forgotten about that. It may be worthwhile to add a comment in SSLObject.__init__, then. Also, can you provide a cumulated patch?
geertj commentedon Oct 4, 2014
Addded the comment about owner being a weakref, and added a new consolidated patch (ssl-memory-bio-5).
geertj commentedon Oct 5, 2014
Maybe an example is useful on how the Memory BIO stuff can be used to implement SSL on top of a proactor event loop. I just added support for this to my Gruvi project in the branch "feat-memory-bio":
An "SslPipe" utility class that uses the memory BIOs:
https://github.com/geertj/gruvi/blob/feat-memory-bio/gruvi/ssl.py#L23
A PEP-3156 style transport:
https://github.com/geertj/gruvi/blob/feat-memory-bio/gruvi/ssl.py#L234
And a backport of this for Python 2.7, 3,3 and 3.4:
https://github.com/geertj/gruvi/blob/feat-memory-bio/gruvi/_sslcompat.c
https://github.com/geertj/gruvi/blob/feat-memory-bio/gruvi/sslcompat.py
pitrou commentedon Oct 5, 2014
SSLPipe looks interesting. I wonder if it can be used to reimplement _SelectorSslTransport in asyncio.selector_events (at least as an experiment).
I'll take a look at the cumulated patch soon, thank you.
python-dev commentedon Oct 5, 2014
New changeset a79003f25a41 by Antoine Pitrou in branch 'default':
Issue bpo-21965: Add support for in-memory SSL to the ssl module.
https://hg.python.org/cpython/rev/a79003f25a41
geertj commentedon Oct 5, 2014
Thanks Antoine for merge!
Yes, it could be done quite easily. SslPipe has no dependency on other parts of Gruvi and if this is for Python 3.5 only then you don't need sslcompat either.
Basically you want to install a read callback on the socket that, when fired, reads from the socket and stuffs the bytes into the memory BIO. It should then write() the returning data back to the socket. If there's a short write, then it should install a write callback to retry the write.
The above is almost identical to what SslTransport in Gruvi does. The only different is that Gruvi uses a proactor on all platforms, so that it does not need to call read() itself but the callback is already called with the buffer.
pitrou commentedon Oct 5, 2014
Le 05/10/2014 23:24, Geert Jansen a écrit :
Yes, it works. Note that I had to modify SSLPipe to also notify of
handshake failures (by passing an argument to the handshake callback).
Here is draft diff against asyncio:
https://gist.github.com/pitrou/f04fa9cbfec88cc37050
However, I don't think this the right approach actually. Rather, the SSL
layer should be implemented as a Protocol object that's also able to act
as a transport for the actual application-level Protocol. It would
completely decouple it from the transport and event loop implementation
details.
(I think that's how Twisted does it, btw)
python-dev commentedon Oct 5, 2014
New changeset 8da1aa71cd73 by Antoine Pitrou in branch 'default':
Remove unused "block" argument in SSLObject.do_handshake() (issue bpo-21965)
https://hg.python.org/cpython/rev/8da1aa71cd73
pitrou commentedon Oct 5, 2014
I'm closing this issue, and will open a new one for asyncio and/or SSLPipe. Thank you very much, Geert!
vstinner commentedon Oct 6, 2014
I have some comments and suggestions to enhance the new API. I chose to open a new issue: bpo-22564.