steffengy / schannel-rs

Schannel API-bindings for rust (provides an interface for native SSL/TLS using windows APIs)
MIT License
49 stars 51 forks source link

tls_stream: treat NotConnected the same way as WouldBlock #72

Closed Keruspe closed 4 years ago

Keruspe commented 4 years ago

This means the underlying stream is not yet connected, which should be very similar to WouldBlock in handling

steffengy commented 4 years ago

Do you have a testcase / reproduction for this? I do not see in which case the socket (which is connected beforehand) is not already connected during the handshake.

Keruspe commented 4 years ago

Hmm, I can try and make a minimal reproducer, though now that I think of it, it might be caused by mio. When WouldBlock is hit while connecting, mio returns a stream that is not yet connected. Using github actions + native-tls, my test passes fine for linux and macos, but fails for windows. "NotConnected" is listed as a retriable error alongside WouldBlock in openssl, which explains why it works for linux, and I guess the macos one does the same, but the windows test hit an assertion because of this

Keruspe commented 4 years ago

You can see here the failure on windows with schannel: https://github.com/Keruspe/mio-schannel-notconnected/actions/runs/145333117

The code is quite simple: https://github.com/Keruspe/mio-schannel-notconnected/blob/master/src/lib.rs

Relevant logs:

---- test_google stdout ----
thread 'test_google' panicked at 'Fails with NotConnected on windows: Failure(Os { code: 10057, kind: NotConnected, message: "A request to send or receive data was disallowed because the socket is not connected and (when sending on a datagram socket using a sendto call) no address was supplied." })', src\lib.rs:16:12
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
steffengy commented 4 years ago

Ok, so an error occurs because you're passing in an (async) stream that is not yet connected. You should ensure to wait until the tcp-socket is connected, as is usual in examples using mio/tokio with schannel/native-tls/... (e.g. tokio-native-tls).

I'm opposed to this change, since the failure reason of "WSAENOTCONN" is not apparent to the library, so it could lead to infinite handshake loops, misdetections and hard to debug issues, since it seems like schannel throws an error but actually the stream passed in is invalid in some fashion.

Keruspe commented 4 years ago

Right, thanks for the feedback and for being that quick!