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

Make SSLStream more robust against cancellation #198

Open
njsmith opened this issue Jun 11, 2017 · 3 comments
Open

Make SSLStream more robust against cancellation #198

njsmith opened this issue Jun 11, 2017 · 3 comments
Labels
cancellation polish TLS Relevant to our TLS/SSL implementation

Comments

@njsmith
Copy link
Member

njsmith commented Jun 11, 2017

This requires adding a bit of complexity in how Stream.send_all methods respond to cancellation in general. (It might also help with #147...) See discussion here: #107 (comment)

The motivating use case is that it would make it pretty straightforward to add trio support to the next generation of urllib3. (If I can convince @Lukasa that this isn't a terrible idea to start with. Stay tuned...)

@njsmith
Copy link
Member Author

njsmith commented Jul 21, 2017

A bandaid approach to fixing this would be to track SSLStream's brokenness separately for sending and receiving -- so when a transport_stream.send_all is cancelled, make a note that we should error out if anyone tries to call transport_stream.send_all again, but allow calls to transport_stream.receive_some.

The reason this is a bit of a bandaid solution is that if we support renegotiation (or in TLS 1.3, post-handshake authentication, which is essentially the same thing, sigh), then calling ssl_stream.receive_some might trigger a call to transport_stream.send_all, so the above is not enough to guarantee that ssl_stream.receive_some survives after the cancellation of ssl_stream.send_all.

OTOH, it turns out that renegotiation is even more deprecated than I realized – libressl ripped it out entirely, and rather more to the point, golang and boringssl only support it in the specific case where (a) the client has specifically requested the library enable renegotiation (it's disabled by default), (b) the server initiates the renegotiation, (c) at a moment when the connection is idle and the client is blocked on receiving from the server. (golang details, BoringSSL details) (Basically this is to handle the specific case where Azure reads a HTTP/1 request and then initiates a renegotiation to request a client cert before sending a response. Really, exactly that one case.) And TLS 1.3's post-handshake authentication is basically intended just for this case as well. BoringSSL doesn't currently plan to support it at all. And of course I already knew that openssl is somewhat buggy here, and NSS at least claims that renegotiation requires an idle channel in their docs. This is all very annoying given that trio actually does support full-duplex renegotiation and it was a lot of work, but... oh well.

The reason this is relevant is that if this is the only time renegotiation is performed, then in fact the bandaid solution described above is sufficient.

(Also ideally I think the way we'd support post-handshake authentication is (a) client has to specifically opt-in when they create the SSLStream, which determines whether we offer the extension as part of our hello, (b) if they've opted in, receive_some may return a magic object representing the CertificateRequest, and then (c) they have to send it back at their leisure. This would be pretty ergonomic, directly follow the protocol, avoid callbacks, and allow us avoid complications in the state machine (i.e. it would remain the case that only receive calls receive, only send calls send). Whether this will be viable given the limitations of actual implementations remains to be seen...)

If we really wanted to drop support for renegotiation down to the BoringSSL baseline, then we could also simplify SSLStream internally I guess... make it an error for SSLStream.send_all to report that it needs to call receive, and in SSLStream.receive_some take the outer send lock if it says it needs to write some. Not sure if this is a good idea or not.

Requiring users to opt-in to renegotiation support would be nice. Even with stdlib ssl we could make it so that getting WantRead from write or data from read raised an error unless some flag was set.

@njsmith
Copy link
Member Author

njsmith commented Jul 28, 2017

As noted here I looked again at urllib3 and actually I think this might not even be necessary for what they need.

@njsmith
Copy link
Member Author

njsmith commented Nov 24, 2018

There's a lot more discussion here: #741

@oremanj oremanj added cancellation TLS Relevant to our TLS/SSL implementation labels May 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cancellation polish TLS Relevant to our TLS/SSL implementation
Projects
None yet
Development

No branches or pull requests

2 participants