QUIC support #98

emillynge · 2022-01-12T21:44:59Z

Since @juzi5201314 indicated that he would close #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 #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 #95 which is much more experimental.

Just to kickstart the review, I want to address some comments in #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.

rapiz1 · 2022-01-13T02:54:56Z

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

rapiz1

Some initial and simple feedback. Also please check the output of CI Lints

Cargo.toml

rapiz1 · 2022-01-13T02:43:07Z

Cargo.toml

+quinn = { version = "0.8.0", optional = true}
+rustls = { version = "0.20", default-features = false, features = ["quic"], optional = true }
+rustls-pemfile = { version = "0.2.1", optional = true }
+openssl = { version = "0.10.38", optional = true }


I see the only usage of openssl is openssl::pkcs12::Pkcs12, which is a large library and requires effort to make it compile on multiple platforms. Since we're already using rustls, it will be better to manage to get rid of openssl. There are some pkcs12 crates on crates.io, if we insist pkcs12 for QUIC. But I wonder what method does QUIC natively provide to work with certificates and so on?

I'm absolutely not a fan of pkcs. I just wanted to stick with the existing pattern.

Looking into native-tls I notice that they also use openssl for parsing p12. So I don't see the benefit of using a different crate when the method I'm using is already being compiled in via native-tls

it matters when you compile with feature quic and without tls :)
openssl is really a pain to compile. Dropping it will possibly make the feature combination server,client,quic available on more platforms than server,client,tls.
openssl already causes trouble for cross compiling rathole and I have to degrade cross to work around. Someone requests for a official risc-v release but I can't do it simply because of the cross compiling stuff about openssl.
tls chooses to use openssl only because it supports IP as hostname and that's many people want. I wish to shift all tls dependencies to rustls at certain point.

So, I just tried out the only really promising crate that does not end up relying on openssl: https://github.com/hjiayz/p12

Unfortunately it choked on the p12 file I created myself. As I said, I am absolutely not a fan of PKCS12 - precisely because you never known quite which libraries, are compatible with with PKCS12 formats. Unless of course you choose a huge lib like openssl that has almost everything covered. (See rustls/rustls#150 where they decline to add support n rusttls)
I don't really want to pull in a half baked pkcs12 parser that only works for some users.

I would suggest the following in a separate PR:

implement reading certs and keys from PEM files directly, as an alternative to supplying pfx file.

create a new "pfx" feature that pulls in openssl for users who want it and enables support for pfx in QUIC transport.

Yeah, i think native-tls depends on openssl when compiled on Linux platform. When compiled on Mac or Windows, I think it uses platform specific deps instead.

The way I have setup cargo.toml forces openssl to be compiled regardless of platform.

I would need to figure out how to do this per platform scoping to fix the builds.

I may have to, at least temporarily, restrict usage of pfx in quic to Linux only.

BTW, I'm open to the change of specifying the certificate in other ways, if pkcs12 is troublesome to work with.

Pkcs is often very troublesome. Usually the problem stems from jks keystores that pretend to be p12 stores, yet not entirely compatible.
Also a problem the other way around, when a Java app expects a jks but is provided with an openssl p12.

I will implement reading from pem files.

Can we scope quic-pfx to be available on Linux only?

I will implement reading from pem files.
Can we scope quic-pfx to be available on Linux only?

If we have an alternative way to use the certificate, I think we can drop pfx entirely in this PR, as you previously suggested

native-tls unfortunately will not support PEM files only.
Only certs can be loaded from PEM files, the keys must still come from pfx.

src/transport/quic.rs

src/server.rs

tests/integration_test.rs

rapiz1 · 2022-01-13T03:05:57Z

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 🤔

tests/for_udp/quic_transport.toml

src/transport/quic.rs

emillynge · 2022-01-13T13:16:14Z

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.

rapiz1 · 2022-01-13T13:22:57Z

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

emillynge · 2022-01-13T15:02:55Z

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

rapiz1

Looks great. Some nitpicking

tests/for_udp/quic_transport.toml

src/transport/quic.rs

src/transport/noise.rs

src/transport/quic.rs

rapiz1 · 2022-01-13T17:08:10Z

src/transport/quic.rs

+    }
+
+    async fn connect(&self, addr: &str) -> Result<Self::Stream> {
+        let mut endpoint = quinn::Endpoint::client("[::]:0".parse().unwrap())


I'm surprised that this works for connecting to IPv4 address since it's binding to a V6 address. Some dual stack magic inside?

I thibnk there's quite a bit of magic in quinn. I have not looked.

In that case, we can see how the test going on different platforms after you have lint fixed.

So the tests failed.
See below for how to handle the address stuff. You can refactor out a function called get_bind_addr_for_udp or so and call it

rathole/src/helper.rs

Lines 43 to 58 in 1fe3509

/// Create a UDP socket and connect to `addr`

pub async fn udp_connect<A: ToSocketAddrs>(addr: A) -> Result<UdpSocket> {

let addr = lookup_host(addr)

.await?

.next()

.ok_or(anyhow!("Failed to lookup the host"))?;

let bind_addr = match addr {

SocketAddr::V4(_) => "0.0.0.0:0",

SocketAddr::V6(_) => ":::0",

};

let s = UdpSocket::bind(bind_addr).await?;

s.connect(addr).await?;

Ok(s)

}

emillynge · 2022-01-17T22:11:45Z

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

emillynge · 2022-01-17T22:56:05Z

I see tests failing.

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

Dunno why the tests are failing

rapiz1 · 2022-01-18T03:30:47Z

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:

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.
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 · 2022-01-27T11:02:44Z

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

emillynge · 2022-01-27T13:39:49Z

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.

Gurkengewuerz · 2022-04-12T21:57:57Z

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

cssivision · 2022-04-19T04:11:59Z

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

emillynge · 2022-04-19T07:13:33Z

@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?

Gurkengewuerz · 2022-04-19T08:37:20Z

@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

aa51513 · 2023-03-08T09:24:59Z

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?

rapiz1 requested changes Jan 13, 2022

View reviewed changes

rapiz1 reviewed Jan 13, 2022

View reviewed changes

tests/for_udp/quic_transport.toml Outdated Show resolved Hide resolved

juzi5201314 reviewed Jan 13, 2022

View reviewed changes

src/transport/quic.rs Outdated Show resolved Hide resolved

emillynge force-pushed the quic branch from cdabda6 to 7ca0451 Compare January 13, 2022 15:13

rapiz1 requested changes Jan 13, 2022

View reviewed changes

rapiz1 mentioned this pull request Jan 14, 2022

WIP: Change Transport to allow for implementing partially reliable transport like QUIC #95

Open

emillynge force-pushed the quic branch from 74dda1c to 746d974 Compare January 17, 2022 22:10

add QUIC transport based on bi streams

3a280b0

emillynge force-pushed the quic branch from 746d974 to 3a280b0 Compare January 17, 2022 22:14

QUIC transport remove dependency on native-tls which may not be present

7f083c2

rapiz1 force-pushed the main branch from 7d209f3 to 47991fb Compare January 21, 2022 15:46

rapiz1 force-pushed the main branch 4 times, most recently from a6601fd to 33b31dd Compare October 1, 2023 08:48

rapiz1 force-pushed the main branch 4 times, most recently from 78f1157 to 97541af Compare October 1, 2023 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QUIC support #98

QUIC support #98

emillynge commented Jan 12, 2022

rapiz1 commented Jan 13, 2022

rapiz1 left a comment •

edited

Loading

rapiz1 Jan 13, 2022

emillynge Jan 13, 2022

emillynge Jan 13, 2022

rapiz1 Jan 13, 2022 •

edited

Loading

emillynge Jan 13, 2022

emillynge Jan 14, 2022

rapiz1 Jan 14, 2022

emillynge Jan 14, 2022 •

edited

Loading

rapiz1 Jan 15, 2022

emillynge Jan 17, 2022

rapiz1 commented Jan 13, 2022

emillynge commented Jan 13, 2022

rapiz1 commented Jan 13, 2022

emillynge commented Jan 13, 2022

rapiz1 left a comment

rapiz1 Jan 13, 2022

emillynge Jan 13, 2022

rapiz1 Jan 14, 2022

rapiz1 Jan 18, 2022

emillynge commented Jan 17, 2022

emillynge commented Jan 17, 2022

rapiz1 commented Jan 18, 2022 •

edited

Loading

rapiz1 commented Jan 27, 2022

emillynge commented Jan 27, 2022

Gurkengewuerz commented Apr 12, 2022

cssivision commented Apr 19, 2022

emillynge commented Apr 19, 2022 •

edited

Loading

Gurkengewuerz commented Apr 19, 2022

aa51513 commented Mar 8, 2023

	/// Create a UDP socket and connect to `addr`
	pub async fn udp_connect<A: ToSocketAddrs>(addr: A) -> Result<UdpSocket> {
	let addr = lookup_host(addr)
	.await?
	.next()
	.ok_or(anyhow!("Failed to lookup the host"))?;

	let bind_addr = match addr {
	SocketAddr::V4(_) => "0.0.0.0:0",
	SocketAddr::V6(_) => ":::0",
	};

	let s = UdpSocket::bind(bind_addr).await?;
	s.connect(addr).await?;
	Ok(s)
	}

QUIC support #98

Are you sure you want to change the base?

QUIC support #98

Conversation

emillynge commented Jan 12, 2022

rust_tls vs native_tls

Releasing UDP sockets in tests

rapiz1 commented Jan 13, 2022

rapiz1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rapiz1 Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emillynge Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rapiz1 commented Jan 13, 2022

emillynge commented Jan 13, 2022

rapiz1 commented Jan 13, 2022

emillynge commented Jan 13, 2022

rapiz1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emillynge commented Jan 17, 2022

emillynge commented Jan 17, 2022

rapiz1 commented Jan 18, 2022 • edited Loading

rapiz1 commented Jan 27, 2022

emillynge commented Jan 27, 2022

Gurkengewuerz commented Apr 12, 2022

cssivision commented Apr 19, 2022

emillynge commented Apr 19, 2022 • edited Loading

Gurkengewuerz commented Apr 19, 2022

aa51513 commented Mar 8, 2023

rapiz1 left a comment •

edited

Loading

rapiz1 Jan 13, 2022 •

edited

Loading

emillynge Jan 14, 2022 •

edited

Loading

rapiz1 commented Jan 18, 2022 •

edited

Loading

emillynge commented Apr 19, 2022 •

edited

Loading