async-rs / async-tls

A TLS implementation over AsyncRead and AsyncWrite
https://async.rs
Apache License 2.0
167 stars 47 forks source link

Attempt to port to rustls 0.20 #47

Closed silverjam closed 2 years ago

silverjam commented 2 years ago

Just what the title says. Attempted to port to rustls 0.20 (should close #45) -- got (most) things compiling but tests are not happy.

❯ cargo test --all
   Compiling async-tls v0.10.0 (/home/ubuntu/dev/async-tls)
    Finished test [unoptimized + debuginfo] target(s) in 2.93s
     Running unittests (target/debug/deps/async_tls-739fa847c8772885)

running 5 tests
test rusttls::stream::test_stream::stream_handshake_eof ... ok
test rusttls::stream::test_stream::stream_handshake ... ok
test rusttls::stream::test_stream::stream_bad ... ok
test rusttls::stream::test_stream::stream_good ... FAILED
test rusttls::stream::test_stream::stream_eof has been running for over 60 seconds

Digging further, the stream_good test seems to have issues with handshaking, so it throws an early EOF:

Error: Kind(UnexpectedEof)
thread 'rusttls::stream::test_stream::stream_good' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/test/src/lib.rs:195:5
silverjam commented 2 years ago

cc @skade

mara-schulke commented 2 years ago

We need this

dignifiedquire commented 2 years ago

maybe sth in here can help: https://github.com/tokio-rs/tls/pull/64/files

mfelsche commented 2 years ago

The stream_good test can be fixed in a similar fashion to https://github.com/tokio-rs/tls/pull/64/files#diff-b3e359ce37f61d9fd2fc6e31203aea06fc4875e20af0a202ad78126904e1343eR120 . The UnexpectedEof error originates here: https://docs.rs/rustls/latest/src/rustls/conn.rs.html#235 and needs a little tweaking in Stream too.

The stream_eof test is getting a WouldBlock error from the connection Reader in Stream, as it didn't see an eof nor received a close notify. This needs to be caught at the Stream i think, in a similar way as tokio-tls is doing it. Though I am not quite sure about the flags.

I was trying to get both to work, but with little success yet.

Apart from fixing the test behaviour with UnexpectedEof (terminated TCP connections without properly closed TLS session) I think this upgrade also needs to clarify if UnexpectedEof should be forwarded to the caller or handled inside async-tls. (I would prefer the former).

silverjam commented 2 years ago

Apart from fixing the test behaviour with UnexpectedEof (terminated TCP connections without properly closed TLS session) I think this upgrade also needs to clarify if UnexpectedEof should be forwarded to the caller or handled inside async-tls. (I would prefer the former).

Agreed, forwarding to the caller seems appropriate (and expected).

mfelsche commented 2 years ago

I have fixed all the tests in my branch that i based on your work: https://github.com/mfelsche/async-tls/tree/update-rustls

@silverjam feel free to cherry-pick the commits, or do you see another way how to bring the changes back here?

mfelsche commented 2 years ago

@silverjam anything i can do to bring this forward? Should i take over and create a new PR from your and my commits?

silverjam commented 2 years ago

@silverjam anything i can do to bring this forward? Should i take over and create a new PR from your and my commits?

I don't have the time to work on this at the moment so happy to hand off to you! Thank you.

mfelsche commented 2 years ago

I created a new PR #48 that obsoletes this one and contains your commits @silverjam Thanks for starting this!