tornado
tornado copied to clipboard
tornadoweb
Make SSLIOStream and HTTPServerRequest configurable
This PR just makes SSLIOStream and HTTPServerRequest inherit from Configurable to address https://github.com/tornadoweb/tornado/issues/2364
As far as I can tell, all the tests are still passing.
One drawback though is that there is some monkey patching needed, despite the Configurable, because TCPServer directly uses netutil.ssl_wrap_socket
code style checks:
./tornado/httputil.py:354:7: E111 indentation is not a multiple of four
./tornado/httputil.py:365:18: E128 continuation line under-indented for visual indent
./tornado/httputil.py:366:18: E128 continuation line under-indented for visual indent
./tornado/iostream.py:1469:7: E111 indentation is not a multiple of four
./tornado/iostream.py:1472:5: E303 too many blank lines (2)
./tornado/iostream.py:1505:5: E303 too many blank lines (2)
Sigh... my eyes were caught by the red warning, and I did not see the most important :-) I will fix it. Thanks !
Making HTTPServerRequest Configurable feels wrong to me - this should be a (relatively) dumb data class, and all the smarts should go into the connection class that is passed in to it. I think this combined with the fact that there's still monkey-patching needed in TCPServer indicates that we're missing something about the right way to support m2crypto.
If the creation of an IOStream in TCPServer were refactored behind a create_stream() method (and no longer called wrap_socket directly), what else would be needed? (Would it even make sense to make TCPServer Configurable if it's an interior node in the inheritance hierarchy? That's what I have in mind here, but maybe it doesn't really work that way)
To be honest, saying that I have little experience with Tornado and its internals is an understatement :-). I agree that monkey patching is ugly and would love to get ride of it but do not really know how. So I would be happy to adapt things such that the option between ssl & M2Crypto is more transparent, but I would need guidance for that. Just to add that we are currently running our setup with the modification here, and it runs happily so far, so nothing seems to be broken yet :-)
Yeah, this'll work, and you're welcome to keep using it in your own fork. I'd just rather have a cleaner design before merging something into the mainline. Unfortunately I don't have any bandwidth to help you come up with that design.
With the last commit, the monkey patching is not needed anymore. Does that look alright to you ?
As for the HTTPServerRequest class, the issue here is that it does not have a method for getting the certificate chain.
So how about implementing in the IOStream class the method get_peer_cert and get_peer_cert_chain (the later one will return NotImplemented), and call them from HTTPServerRequest. Like this, we only have a single Configurable, which is the IOStream
I've re implemented following what I proposed. Let me know if you think this looks more like the right way you expect
Yeah, this looks like what I was expecting. There's one more step needed to centralize all SSL knowledge in SSLIOStream: IOStream.start_tls needs to use this new mode instead of wrapping its own socket. I need to think about it some more, but maybe we should be deprecating the existing mode and plan to move everything to this style.
I'm wrapping up the 5.1 release so I'm going to leave this PR open a little longer, but I'll return to it once that's done.
Fine with me.
So just to make sure I understood properly: you're yet unsure whether we should modify IOStream.start_tls or just deprecate that ? If that's the case, I'm not going to try to do so, I'll jsut fix the travis errors
Hum, I have no clue why one test failed on the pypy build, while it did not on all the others
The command "LANG=C python $TARGET" exited with 1.
Any hint ? Wouldn't just restarting the build solve the issue ?
Unittests failed on autoreload module .
@chaen , forcing the CI build restart maybe work!
@chaen , submit a new commit or close this PR and reopen, the CI build will auto restart!
Pypy is just slow to start up so it hits a timeout in that autoreload test. I've updated it on master so if you rebase this PR the failure should go away.
Should I rebase now such that you can merge, or should I wait a bit longer ? Thanks !
I'm having second thoughts about this change because I think in 6.x we may want to move towards using more asyncio-based primitives (that is, an IOStream implementation built on asyncio protocols and transports instead of raw sockets). We wouldn't be able to remove the socket-based versions completely for backwards-compatibility reasons (so SSLIOStream() would still take a raw socket argument), but in this case TCPServer would be built around asyncio's create_server and would create AsyncIOStreams instead of IOStream/SSLIOStreams.
I'm reluctant to add new public surface area here when I can already see how it could become unsupportable in the near future (especially when this is already an invasive hack around a missing feature in the stdlib's ssl implementation).
I have no idea how all the async things are working (unfortunately I am still stuck with python 2.7 so...) So I do not quite understand where the problem sit. Can you explain me which part of the code will become problematic and why ? Maybe we can then find a way around Also, if I follow what you are saying, this means that 6.x will be python3 only, if async becomes mandatory ? I understand your reluctances. This being said, even though the primary reason for this change is a missing feature in the stdlib's ssl implementation, it opens more possiblity than just that. Anyway, I would really prefer this feature to be added to the stdlib, but well...
Yes, python-3.5 or later will be required for the next major release, tornado-6.0 (which is also where any new features will land at this point). https://github.com/tornadoweb/tornado/pull/2443
Can you explain me which part of the code will become problematic and why ?
The problem is that in the future, we might introduce AsyncioIOStream and AsyncioSSLIOStream (which would be built on asyncio protocols and transports instead of raw file descriptors. I don't think we'd be able to easily get to the underlying implementation to be able to use an alternative SSL implementation). TCPServer would start using these classes instead of the current IOStream implementations. This would silently switch away from your M2CryptoIOStream to AsyncioSSLIOStream.
If you're staying on Python 2, I think it's best for you to just continue to run your fork and close this PR. Even if this PR gets merged, it would land in Tornado 6.x which will be py3-only, so it won't do you any good (until you move to py3). In the longer term, you could continue to push for the necessary changes to the standard library (or maybe third-party asyncio implementations like uvloop. I think it would be possible to replace the ssl implementation at that level, assuming Tornado converts to use asyncio protocols).
I see. Thanks for the explanations.
How far are we from Tornado 6 ?
In fact, it seems like the planets line up nicely for once: the pySSL fix is now tagged for Python 3, and you are telling me that Tornado 6 will make Python 3 mandatory. So does it sound reasonable to you to implement my proposed fix/feature/enhancement/hack in the Tornado 5 series, with the clear statement and agreement that as of Tornado 6, it is going to disappear ? This would allow us to adopt tornado practically now, and still benefit from all the coming improvement and bug fixes, until we are ready to go python3.
How far are we from Tornado 6 ?
Hard to say. A few months? Depends on how much time I get to finish it up.
So does it sound reasonable to you to implement my proposed fix/feature/enhancement/hack in the Tornado 5 series, with the clear statement and agreement that as of Tornado 6, it is going to disappear ?
I don't intend to make another release in the Tornado 5 series or spend any more time on it. All my Tornado-related time is focused on Tornado 6 now.
So just to make suer: no bug fixes, no security fixes back ported, Tornado 5 is frozen ?
I think @bdarnell means that there will be no more feature-adding "minor" releases of Tornado 5.x, but there will still be security and significant-bug fixing "patch" releases of Tornado 5.1.x
Yes, exactly. Tornado 5.1 will still get security fixes and other major bug fixes if any arise, but otherwise it won't be changing.
Okay I see. And despite it being the last release of the serie 5, you consider this change to be too much of a change to be included, even if it preserves the interface and backward compatibility ?
I also wanted to ask you, since we are at it: if this approach would not work in Tornado 6 to integrate M2Crypto, what would be the correct approach then ? Maybe it is something I could work on then..
Okay I see. And despite it being the last release of the serie 5, you consider this change to be too much of a change to be included, even if it preserves the interface and backward compatibility ?
Yes. Release 5.1 is done and I do not intend to spend any more time on it except as needed by any major bugs that may be discovered. (This might change if enough volunteer effort emerges to maintain a 5.x branch).
I also wanted to ask you, since we are at it: if this approach would not work in Tornado 6 to integrate M2Crypto, what would be the correct approach then ?
To be clear, this approach would work in tornado 6.0. But it conflicts with things I'd like to do in later 6.x releases that I think would benefit more users (by improving performance across the board). Convincing me to include this in Tornado would require convincing me that the demand for it is worth the cost. Otherwise, my response will be that you should maintain and use your own fork with this patch.
The ideal solution would be to apply pressure to cpython core to act on https://bugs.python.org/issue18233 which would get you the feature you need without wholesale replacement of the TLS implementation. Or, in the long term in which we delegate more of IOStream to asyncio, this could be addressed by third-party asyncio implementations like uvloop.