rathole icon indicating copy to clipboard operation
rathole copied to clipboard

QUIC support

Open emillynge opened this issue 3 years ago • 15 comments

Since @juzi5201314 indicated that he would close https://github.com/rapiz1/rathole/pull/64 I now bring this PR. Many thanks to juzi for his PR, looking at your approach and reading the comments in the PR was very helpful!

This PR is aware of https://github.com/rapiz1/rathole/pull/93 and an effort has been made to ensure QuicTransport::accept is cancel safe. But honestly, it is very difficult to determine if that is in fact the case.

This PR is standalone and does not immediately require merging of https://github.com/rapiz1/rathole/pull/95 which is much more experimental.

Just to kickstart the review, I want to address some comments in https://github.com/rapiz1/rathole/pull/64

rust_tls vs native_tls

I may be possible to drop in a native_tls version of quinn-proto::crypto, but that is a lot of work, that I don't think I can put in, for this particular feature. Also, I might have to rewrite certain other parts of quinn as well that seems to, only be available when using the rust_tlsfeature of the quinncrate.

If the compilation time / executable size is a problem, then I would propose making QUIC a non-default feature.

Releasing UDP sockets in tests

I have spent a long time trying to get the tests working without using a pre-determined sleep between shutdown and restart of the server. This is what I came up with:

  • make sure that rathole::run does not exit before the currently running server instance coroutine has joined
  • add new async fn close(a: Self::Acceptor) method to the Transport trait such that graceful termination of the QUIC connections can be awaited
  • await close in Server::run to ensure that socket is released before coroutine joins

I hope that the above solution is acceptable. The only other option is to rebind to a new random UDP port from a drop on the Acceptor.

emillynge avatar Jan 12 '22 21:01 emillynge

examples/tls/testserver.pfx looks like a slipping file?

rapiz1 avatar Jan 13 '22 02:01 rapiz1

This PR is aware of #93 and an effort has been made to ensure QuicTransport::accept is cancel safe. But honestly, it is very difficult to determine if that is in fact the case.

Seems safe to me 🤔

rapiz1 avatar Jan 13 '22 03:01 rapiz1

examples/tls/testserver.pfx looks like a slipping file?

No, identity.pfx does not provide a certificate that QUIC will accept. AFAIK QUIC requires that the server cert contains a DNS SAN. identity.pfx has a SAN, but it is IP type.

emillynge avatar Jan 13 '22 13:01 emillynge

There are two files, examples/tls/test_server.pfx and examples/tls/testserver.pfx. Only the former has been used in toml files.

rapiz1 avatar Jan 13 '22 13:01 rapiz1

Ahh, yeah that file. that was not supposed to be committed. I will delete

emillynge avatar Jan 13 '22 15:01 emillynge

Squashed the branch since rebasing was getting unwieldy with so many small commits and reverts

emillynge avatar Jan 17 '22 22:01 emillynge

I see tests failing.

Can only test linux locally, and that passes on my desktop...

Dunno why the tests are failing

emillynge avatar Jan 17 '22 22:01 emillynge

Jan 18 02:53:19.113  INFO handle{service=echo}: rathole::client: Starting 092c79e8f80e559e404bcf660c48f3522b67aba9ff1484b0367e1a4ddef7431d
Jan 18 02:53:19.113  INFO handle{service=pingpong}: rathole::client: Starting c78862c4afddaa20fd3ff12e5e270480706499341ca5d1d7437ec9668556805b
Jan 18 02:53:20.031  INFO test{config_path="tests/for_udp/quic_transport.toml" t=Udp}: integration_test: start the server
Jan 18 02:53:20.039  INFO config_watcher{path="tests/for_udp/quic_transport.toml"}: rathole::config_watcher: Start watching the config
Jan 18 02:53:20.040  INFO rathole::server: Listening at 0.0.0.0:2332
Jan 18 02:53:20.182  INFO test{config_path="tests/for_tcp/quic_transport.toml" t=Tcp}: integration_test: start the server
Jan 18 02:53:20.189  INFO config_watcher{path="tests/for_tcp/quic_transport.toml"}: rathole::config_watcher: Start watching the config
Jan 18 02:53:20.194  INFO rathole::server: Listening at 0.0.0.0:2333
Jan 18 02:53:22.059  INFO test{config_path="tests/for_udp/quic_transport.toml" t=Udp}: integration_test: echo
test udp ... FAILED
Jan 18 02:53:22.231  INFO test{config_path="tests/for_tcp/quic_transport.toml" t=Tcp}: integration_test: echo
error: test failed, to rerun pass '--test integration_test'
test tcp ... FAILED
Jan 18 02:53:19.113  INFO handle{service=echo}: rathole::client: Starting 092c79e8f80e559e404bcf660c48f3522b67aba9ff1484b0367e1a4ddef7431d

shows that the client tried to connect to the server, but by the end of the test, it had not succeeded nor returned any feedback. This is possibly caused by a IPv6 bounded socket connecting to a IPv4 address.

I think this reveals two potential issues to address:

  1. QUIC connect can't return an error immediately when the target port is not listened, due to the nature of UDP, which is the cause of no feedback. This can be solved either by
    1. Enforce a timeout at https://github.com/emillynge/rathole/blob/7f083c21b7f0f637b28337c48c74f43830488aa4/src/client.rs#L393-L397
    2. Utilize some QUIC features or enforce a timeout in the transport connect implementation. I wonder if there's already some timeout setting for QUIC connect.
  2. As I previously mentioned, a IPv4 bind socket can't connect to a IPv6 address or vice versa. See https://docs.rs/quinn/latest/quinn/struct.Endpoint.html#method.client about dual stack.

rapiz1 avatar Jan 18 '22 03:01 rapiz1

Hi @emillynge What's the status of this? I think we are really close to get this merged.

rapiz1 avatar Jan 27 '22 11:01 rapiz1

Hi @emillynge What's the status of this? I think we are really close to get this merged.

I'm on vacation now, and then next week my parental leave is over and I'll have to go to work. I will try to wrap this up in the coming month, but my time is very limited now unfortunately.

emillynge avatar Jan 27 '22 13:01 emillynge

Hey @emillynge may I ask nicely what the situation is with this PR? 👀

Gurkengewuerz avatar Apr 12 '22 21:04 Gurkengewuerz

@emillynge if you don't mind, I can pick up this and finish this PR.

cssivision avatar Apr 19 '22 04:04 cssivision

@emillynge if you don't mind, I can pick up this and finish this PR.

@cssivision I would be very pleased if you do so.

Can I transfer/share ownership of PR and branch somehow?

emillynge avatar Apr 19 '22 07:04 emillynge

@cssivision can fork your repository with the PR branch and open a new one at least. But i think you can also add him as a member to your forked rathole repository so he can push changed. Correct me if i'am wrong

Gurkengewuerz avatar Apr 19 '22 08:04 Gurkengewuerz

What is the future of this pull request? @rapiz1

The reason I'm asking this is that I'm thinking about implementing a new feature of QUIC as a transport type for rathole

should I submit a whole new pull request?

aa51513 avatar Mar 08 '23 09:03 aa51513