[issue22559] [backport] ssl.MemoryBIO
Geert Jansen added the comment: Glyph, if this is just for Twisted you could ship the "sslcompat" module that I'm shipping with Gruvi. It backports the async IO stuff and a few other pieces through an extension module. https://github.com/geertj/gruvi/blob/master/src/sslcompat.c and https://github.com/geertj/gruvi/blob/master/lib/gruvi/sslcompat.py But having this in the stdlib itself is much cleaner of course. -- ___ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue22559> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue24334] SSLSocket extra level of indirection
Geert Jansen added the comment: Apologies for the late reply. I made SSLSocket go through SSLObject so that the test suite that is primarily testing SSLSocket will test both. Also, this layering allows us to define some non-networked operations (such as SSL certificate checking and channel binding types) in a single location rather than duplicating them between SSLSocket and SSLObject. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue24334 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22564] ssl: post-commit review of the new memory BIO API
Geert Jansen added the comment: Hi Victor, see below my comments: * SSLSocket.read(), SSLOBject.read() and _ssl._SSLSocket.read() taking a buffer as the second positional argument. Both SSLSocket.read() and _SSLSocket.read() already accepted two arguments so I went for consistency. The former has been publicly documented in prior releases so I don't think it can be changed? * versionadded for server_hostname set to 3.5 This is when it was first documented. If it's more correct to specify when it was first implemented then I can put it to 3.2. * server_hostname property is idna encoded bytes instead of unicode Agreed that it should be changes to unicode. Currently SSLSocket.server_hostname is whatever was passed to the constructor, which can be unicode or an already encoded bytes instance. SSLObject.server_hostname on the other hand is always a bytes instance. Should SSLSocket.server_hostname also be changed to always return unicode even if a bytes was passed to the constructor? I'd tend to say yes especially because the attribute was not documented before. But it would be a change in behavior. Now that I think of it - since SSLSocket now uses SSLObject to check the hostname, and SSLObject exposes server_hostname as a bytes instance, is hostname checking currently broken for non-ascii hostnames? * Documentation suggestions. Mostly make sense. I will have a look. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22564 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22564] ssl: post-commit review of the new memory BIO API
Geert Jansen added the comment: +.. attribute:: SSLSocket.server_hostname + + A ``bytes`` instance (...) Ah, this is a mistake. It's actually always a str instance (on SSLObject as well). It is indeed, I stand corrected. I was confused by the decode - encode roundtrip that happens in _ssl. However I think that in theory SSLSocket.server_hostname could be a bytes, if a bytes was passed into the constructor. _ssl parses this argument with the et format which means it will let a correctly encoded byte string through. Not sure if anybody is using this though. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22564 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22564] ssl: post-commit review of the new memory BIO API
Geert Jansen added the comment: newPySSLSocket() expects a char* string and use PyUnicode_Decode() to decode bytes. Yup, and this value is available as SSLSocket._sslobj.server_hostname. But SSLSocket.server_hostname is not this, it is what was passed to the constructor which can be a bytes instance. For total cleanness maybe the constructor should raise a TypeError if server_hostname is passes as a bytes. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22564 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: 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 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22559] [backport] ssl.MemoryBIO
Changes by Geert Jansen gee...@gmail.com: -- nosy: +geertj ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22559 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: Thanks Antoine for merge! SSLPipe looks interesting. I wonder if it can be used to reimplement _SelectorSslTransport in asyncio.selector_events (at least as an experiment). 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22559] [backport] ssl.MemoryBIO
Geert Jansen added the comment: It seems that SSLSocket.close() doesn't actually close the socket, and that's why the server side read() blocks. It's a bit of a mystery to me how socket.close(), which is called by SSLSocket to do the actual close, is supposed to work. I don't see any calls to _sock.close() in there.. If I add the following 3 lines to socket.py then it works (but there's a few unexpected EOF errors in return). diff -r ae64614b66b7 Lib/socket.py --- a/Lib/socket.py Sat Oct 04 18:24:32 2014 -0400 +++ b/Lib/socket.py Sun Oct 05 18:16:51 2014 -0400 @@ -192,6 +192,9 @@ def close(self, _closedsocket=_closedsocket, _delegate_methods=_delegate_methods, setattr=setattr): # This function should not reference any globals. See issue #808164. +if hasattr(self._sock, '_dummy'): +return +self._sock.close() self._sock = _closedsocket() dummy = self._sock._dummy for method in _delegate_methods: I'm probably overlooking something b/c I can't imagine socket.close() being a no-op. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22559 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22559] [backport] ssl.MemoryBIO
Geert Jansen added the comment: Right, socket._socketobject mearly nulls out the reference to _socket.socket, and lets reference counting take care of the rest. Ugh this is bad... I thought close() was exactly there when you don't want to depend on refcounting for cleanup. * When do_handshake() raises an exception (say, a CertificateError), then a reference to a traceback is stored for sys.exc_info() * This traceback holds a reference to a frame where ssl.SSLObject is self * ssl.SSLObject holds a reference to _ssl._SSLSocket * Which holds a reference to _socket.socket On Python 3.x the last one above is a weakref. It seems like ``ssl.SSLSocket.close()`` should probably explicitly close the ``SSLObject`` somehow? I think this problem would appear on Python3 if you caught the exception manually and kept a reference to it? On Python 3.x socket.close() does a real close() on the socket, it seems. (thought it appears to have an app-level refcount for makefile()). I agree this is the best way but it seems very scary to make that change for 2.7. I think that closing the socket in SSLSocket.close(), as you suggest, would work (using socket._sock.close()), or or maybe you can make the Socket member in _SSLSocket a weakref? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22559 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22559] [backport] ssl.MemoryBIO
Geert Jansen added the comment: In Python2 socket._socketobject composes with _socket.socket, so we pass self._sock, that way it has teh right C-level fields. Unfortunately taking a weakref of _socket.socket is not allowed. I see, and agree that making it weakref-able would be a bad idea for a minor release. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22559 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: 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). Note that owner is a weakref :) Did you look at the code? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: Addded the comment about owner being a weakref, and added a new consolidated patch (ssl-memory-bio-5). -- Added file: http://bugs.python.org/file36806/ssl-memory-bio-5.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: New patch attached. This patch makes SSLSocket use SSLObject. The big benefit here is obviously test coverage. I decided against using SSLObject as a mixin, because all methods need to be reimplemented anyway because for SSLSocket they need to handle the non-SSL case. Instead, I made SSLSocket._sslobj an SSLObject rather than a _ssl._SSLSocket. The patch is rather small, so I kept it incremental to patch4. Test suite runs fine. I had to update one SSL test (test_unknown_channel_binding). Because the test for the binding type is now in SSLObject, a non-connected SSLSocket will return None even for an unknown binding. Arguably this is even more correct because the binding type can depend on the cryptographic protocol used, e.g. tls-unique doesn't work for SSLv2 (it's currently not checked and nobody cares about SSLv2, I'm just arguing from theory here). A second change is that the private _sslobj is now a different type. However since this is clearly an internal attribute, I think people that are using this should expect breakage. Antoine, please let me know if this is now ready for merging in your view or if not what you'd like me to do still. Thanks. -- Added file: http://bugs.python.org/file36791/ssl-memory-bio-4-incr2.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: Well... I would have expected this approach to yield a bigger reduction in code size. If it doesn't shrink the code, then I'm not sure it's worthwhile. What do you think? 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. (also, why do you have to add an owner attribute?) 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: Antoine, sorry for the delay, we just had a new kid and I changed jobs :) Let me try if I can create an updated patch that where SSLObject is a mixin for SSLSocket. I think the argument about writing tests once is important. Be back in a few days.. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: Thanks Antoine. See my comments below: - is it necessary to start exposing server_hostname, server_side and pending()? At the C level I need server_hostname and server_side exposed because they are needed to implement the cert check in do_handshake(). SSLObject gets a C-level _SSLSocket passed to its constructor and doesn't create it itself. So it can't store these attributes. At the Python level SSLSocket already had these, albeit undocumented, so that's why I added them to SSLObject as well. We can leave these undocumented at the Python level if you prefer. - SSLObject is a bit vague, should we call it SSLMemoryObject? or do you expect we may want to support other kinds of BIOs some day? OpenSSL calls the struct just SSL which I think is even less descriptive. I think the best description in words is an SSL protocol instance, however SSLProtocolInstance looks a bit too long to me. Maybe just SSLInstance, would that be better than SSLObject? I don't think we want to tie the name to the Memory BIO as I think that it may be useful some day to support other BIOs notably the Socket BIO. I believe that the overall _ssl/ssl code could be simplified by: - Making SSLSocket inherit from SSLObject and socket. - Remove all socket handling from _ssl and use a Socket BIO instead. - Implement the blocking semantics for do_handshake(), unwrap(), read() and write() at the Python level. For testing and benchmarks, the null BIO might be useful as well. - should the basic implementations in SSLObject be shared (using some kind of mixin) with SSLSocket, or is it unpractical to do so? It's possible but I am not sure it would simplify the code a lot. For example, there's no notion of a closed or an unwrapped socket in SSLObject. If you look at the cipher method for example. This is how it looks for SSLSocket: def cipher(self): self._checkClosed() if not self._sslobj: return None else: return self._sslobj.cipher() And this is how it looks for SSLObject: def cipher(self): return self._sslobj.cipher() To use SSLObject as a mixin it would have to be aware of these two uses of its subclasses. It could be done but I don't think it's 100% clean either. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: Adding small patch (incremental to patch #4) to fix a test failure. -- Added file: http://bugs.python.org/file36483/ssl-memory-bio-4-incr1.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: Updated patch. Contains: * An owner attribute on a _ssl.SSLSocket that is used as the first argument to the SNI servername callback (implemented as a weakref). * Documentation I think this covers all outstanding issues that were identified. Antoine, please let me know if you have further feedback or if not whether this can be committed. -- Added file: http://bugs.python.org/file36475/ssl-memory-bio-4.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: Antoine, yes, I just got back from holiday. I will have an updated patch tomorrow. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: Thanks to Ben and Glyph for their feedback. The memory BIO should allow ProactorEventLoop to support SSL. I say should because I have not looked at it myself. However, my Gruvi project is proactor (libuv) based and I have a private branch where SSL support is working using a proactor API. I need a few more days to create an updated patch. This patch will include Antoine's suggestion of passing the SSLObject instance to the servername callback, and an update to the docs. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: New patch with a Python-level API (option #3). This needs some more tests, and docs. -- Added file: http://bugs.python.org/file36248/ssl-memory-bio-3.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: 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. Yes, it does. See the following comment in _servername_callback(): /* Pass a PySSLSocket instance when using memory BIOs, but an ssl.SSLSocket * when using sockets. Note that the latter is not a subclass of the * former, but both do have a context property. THis supports the common * use case of setting this property in the servername callback. */ 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? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Changes by Geert Jansen gee...@gmail.com: Added file: http://bugs.python.org/file36189/ssl-memory-bio-2.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: I added a new patch that addresses the comments. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: 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. -- Added file: http://bugs.python.org/file36190/bio_python_options.py ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Changes by Geert Jansen gee...@gmail.com: Removed file: http://bugs.python.org/file36190/bio_python_options.py ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Changes by Geert Jansen gee...@gmail.com: Added file: http://bugs.python.org/file36191/bio_python_options.py ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
Geert Jansen added the comment: Hi all (pitrou, haypo and all others) can I get some feedback on this patch? Thanks! -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21965] Add support for Memory BIO to _ssl
New submission from Geert Jansen: 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. -- files: ssl-memory-bio.patch keywords: patch messages: 222833 nosy: geertj priority: normal severity: normal status: open title: Add support for Memory BIO to _ssl type: enhancement versions: Python 3.5 Added file: http://bugs.python.org/file35928/ssl-memory-bio.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue21965 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12641] Remove -mno-cygwin from distutils
Geert Jansen added the comment: *bump*. This is a critical bugfix that prevents I bet 90%+ of Python users on Windows compiling C extensions. It has been open for 2 years and it's a great disservice to people having to compile stuff on Windows. Oscar has been doing a terrific job of providing man patches. May I ask that a core dev finally takes some responsibility here, signs of on the patch, and get it applied? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12641 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12641] Remove -mno-cygwin from distutils
Geert Jansen added the comment: *bump* I just installed MinGW 2.6.2 32-bit on Windows XP. It doesn't accept -mnocygwin and there is no binary i686-pc-mingw32-gcc either. It would be great if you could agree on an approach and get this fixed. This impacts a lot of users that want to build extensions on Windows. In the mean time users can find a hack to work around the issue here: https://gist.github.com/4466320 -- nosy: +geertj ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12641 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
