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

QUIC support #1334

Closed
wants to merge 247 commits into from
Closed

Conversation

Demi-Marie
Copy link
Contributor

@Demi-Marie Demi-Marie commented Dec 6, 2019

I am using the latest quinn git master. While I could backport to the latest release, I would prefer to get the existing code working first.

Current approach:

  • Use quinn-proto (the bare state machine)
  • Each connection has a pair of HashMaps to store wakers, one for readers and one for writers. When I/O on a stream becomes possible, the corresponding waker is awoken.
  • The mutable state is protected with mutexes, so libp2p-quic is thread-safe.
  • The state machine is only advanced on a background task.
  • Channels are used for incoming connections (to an endpoint) and incoming streams (to a connection). They are also used to send packets generated by a connection to the background task for transmission.
  • Strict mutex aquisition ordering is upheld to prevent deadlocks. Specifically, if both the mutex protecting the endpoint and the mutex protecting a connection need to be taken, the mutex protecting the endpoint must be taken first.

What I would like feedback on:

  • Should I be using channels in places where I am currently using mutexes?
  • To avoid potential memory exhaustion denial of service attacks, I am using a bounded channel for outgoing packets. If it fills up, packets are dropped. Since QUIC is a reliable protocol, I expect that these will be retransmitted, which is much better than having to buffer an unlimited amount of data.
  • How should I handle the case where a connection arrives, but we are not ready for it? We can’t buffer an unlimited number of them, so at some point we will need to impose backpressure. The current plan is to rely on quinn-proto limiting the number of connections that it returns but have not yet been accepted.

Edit

The code is now essentially complete and is ready for review.

This requires a branch of `quinn` that has not yet been merged.
It now incudes the changes needed by libp2p.
Also update to stable futures.
There are a LOT of `unimplemented!()` calls here!
All future versions will use ‘quinn-proto’ directly.
There are plenty of `unimplemented!()` calls, and TLS certificate
processing has not even been started, but this should still be a good
start.
The code compiles and is approaching a point at which it could be
useful.
@tomaka
Copy link
Member

tomaka commented Dec 6, 2019

How should I handle the case where a connection arrives, but we are not ready for it? We can’t buffer an unlimited number of them, so at some point we will need to impose backpressure. The current plan is to rely on quinn-proto limiting the number of connections that it returns but have not yet been accepted.

When you receive a connection or a substream, you wake up whatever was calling poll_inbound and/or whatever was polling the stream of incoming connections. If these things are not fast enough, the right solution is indeed freezing the QUIC background task entirely until the rest of the program is ready to accept more.

To avoid potential memory exhaustion denial of service attacks, I am using a bounded channel for outgoing packets. If it fills up, packets are dropped. Since QUIC is a reliable protocol, I expect that these will be retransmitted, which is much better than having to buffer an unlimited amount of data.

I don't really understand what you mean here. The data has to exist somewhere in memory for it to be retransmitted.
To me the idea here should be the same, in that we can prevent people from writing data by making write_substream return Poll::Pending.

@tomaka
Copy link
Member

tomaka commented Dec 6, 2019

I am delaying TLS certificate handling because I am quite confident in being able to do it reasonably well, whereas this is my first time writing code that interacts with futures at a low level.

The low-level futures code is indeed far from being simple, but the TLS certificate is what stopped me in my latest attempt at implementing QUIC, and I don't think that it's just a detail that can be dismissed as "we'll do it later as it's so easy".

@Demi-Marie
Copy link
Contributor Author

How should I handle the case where a connection arrives, but we are not ready for it? We can’t buffer an unlimited number of them, so at some point we will need to impose backpressure. The current plan is to rely on quinn-proto limiting the number of connections that it returns but have not yet been accepted.

When you receive a connection or a substream, you wake up whatever was calling poll_inbound and/or whatever was polling the stream of incoming connections. If these things are not fast enough, the right solution is indeed freezing the QUIC background task entirely until the rest of the program is ready to accept more.

The problem is that the QUIC background task handles all I/O. If I freeze it, existing connections will stop working.

To avoid potential memory exhaustion denial of service attacks, I am using a bounded channel for outgoing packets. If it fills up, packets are dropped. Since QUIC is a reliable protocol, I expect that these will be retransmitted, which is much better than having to buffer an unlimited amount of data.

I don't really understand what you mean here. The data has to exist somewhere in memory for it to be retransmitted.
To me the idea here should be the same, in that we can prevent people from writing data by making write_substream return Poll::Pending.

I do indeed do that when possible. I only drop a packet if a channel reports that it has capacity, but no longer has space when I try to write to it. I could buffer it myself, but that seems ugly.

@burdges
Copy link

burdges commented Dec 9, 2019

Is freezing the lower level layer really how QUIC specifies the application of back pressure? I thought we could do much more fine grained back pressure?

@Demi-Marie
Copy link
Contributor Author

Demi-Marie commented Dec 10, 2019

Is freezing the lower level layer really how QUIC specifies the application of back pressure? I thought we could do much more fine grained back pressure?

Backpressure should be as fine-grained as possible. If it isn’t, that is a bug.

Much of the I/O code simply has not been written yet. Is that what you are referring to?

@infinity0
Copy link

the right solution is indeed freezing the QUIC background task entirely until the rest of the program is ready to accept more.

The problem is that the QUIC background task handles all I/O. If I freeze it, existing connections will stop working.

To apply backpressure, simply don't send acks. In this case it means the QUIC layer should not send an ack to the incoming connection request until the application responds by accepting the poll_inbound.

This implements message transmission.  The code relies on UDP datagram
sending not blocking indefinitely.  Since UDP does not retransmit
packets, this should be a decent assumption in practice.
@tomaka
Copy link
Member

tomaka commented Oct 1, 2020

requires either synchronization or explicit message-passing

And that should be done by the higher-level code, such as rust-libp2p, not the QUIC library itself.
I prefer to have is a library that doesn't block us in any way, rather than a library that handles everything for us an opinionated way, and that ends up conflicting with our code.

@DemiMarie
Copy link
Contributor

I agree. That’s why I prefer quinn-proto.

@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 6, 2021

I can make a PR for https://github.com/ipfs-rust/libp2p-quic, but need to finalize the noise protocol it uses and document it. Also there is more perf tuning to do.

@DemiMarie
Copy link
Contributor

I can make a PR for https://github.com/ipfs-rust/libp2p-quic, but need to finalize the noise protocol it uses and document it. Also there is more perf tuning to do.

Thank you! (I’m the author of this PR).

@dvc94ch dvc94ch mentioned this pull request Jul 16, 2021
@dvc94ch
Copy link
Contributor

dvc94ch commented Jul 17, 2021

So I opened #2144 . It is inspired by the work in this PR but it doesn't share any code. It works with the latest released libp2p/quinn and uses noise instead of tls. I think it would be possible to get the tls implementation from this PR and integrate it in the future, however I'm not particularly incentivized to do it, so contributions welcome. But I don't think it needs to be blocked on this feature. Also there isn't any tokio support, which someone may want to add in the future.

One other difference is the use of unbounded channels. This is for two reasons. 1. it simplifies the implementation, and 2. I haven't found a benchmark that performs better by requiring task switching than just buffering the data. This also matches my experience with benchmarks in other projects, like netsim-embed for example.

@burdges
Copy link

burdges commented Jul 17, 2021

Which Noise? What curve?

@dvc94ch
Copy link
Contributor

dvc94ch commented Jul 17, 2021

The cryptographic details are documented and implemented here [0]. It uses an IKpsk1 handshake with ed25519 keys and a chacha8poly1305 cipher. The psk1 is optional and is used instead of the pnet protocol that is used with tcp for private swarms.

@DemiMarie
Copy link
Contributor

So I opened #2144 . It is inspired by the work in this PR but it doesn't share any code. It works with the latest released libp2p/quinn and uses noise instead of tls. I think it would be possible to get the tls implementation from this PR and integrate it in the future, however I'm not particularly incentivized to do it, so contributions welcome. But I don't think it needs to be blocked on this feature. Also there isn't any tokio support, which someone may want to add in the future.

As the person who wrote the relevant TLS code: I no longer work for Parity, but I am willing to answer (in my spare time) questions about the relevant code that I wrote. That said, my understanding is that Noise is a cleaner protocol anyway and so should be preferred. Additionally, the TLS specification for libp2p has some significant design weaknesses, namely signing the raw public key and not the SubjectPublicKeyInfo.

One other difference is the use of unbounded channels. This is for two reasons. 1. it simplifies the implementation, and 2. I haven't found a benchmark that performs better by requiring task switching than just buffering the data. This also matches my experience with benchmarks in other projects, like netsim-embed for example.

Do you have proofs that the channels will not grow without limit?

The cryptographic details are documented and implemented here [0]. It uses an IKpsk1 handshake with ed25519 keys and a chacha8poly1305 cipher. The psk1 is optional and is used instead of the pnet protocol that is used with tcp for private swarms.

Why ChaCha8 instead of ChaCha12? My understanding is that using ChaCha12 instead of ChaCha20 is considered quite safe, but using ChaCha8 only provides a 1-round safety margin.

@dvc94ch
Copy link
Contributor

dvc94ch commented Jul 17, 2021

Do you have proofs that the channels will not grow without limit?

not really, but I like code that looks good in benchmarks and is readable. devops may disagree.

Why ChaCha8 instead of ChaCha12? My understanding is that using ChaCha12 instead of ChaCha20 is considered quite safe, but using ChaCha8 only provides a 1-round safety margin.

in the too much crypto paper chacha8 was deemed sufficient. while rand uses chacha12 for paranoia reasons, they did discuss using chacha8 but deemed it not critical enough for performance compared to the risk. however for quic it has a large effect on performance. according to my early benchmarks using aes-gcm, encryption/decryption was 50% of the workload.

@dvc94ch
Copy link
Contributor

dvc94ch commented Jul 18, 2021

Section 3.3 covers ChaCha:

The best result on ChaCha is a key recovery attack on its 7-round version, with 2^237.7 time complexity (the exact unit is unclear) using output data from 2^96 instances of ChaCha, that is, 2^105 bytes of data. On 6 rounds, a similar attack can run in time & data 2^116 & 2^116, or 2^127.5 & 2^37.5. On 5 rounds, the attack becomes practical due to the lower diffusion, and runs in 2^16 time and data.

Note the 7-round attack is a security reduction from the claimed 256-bits of security, to "237.7" bits, and therefore is not a catastrophic attack.

@dvc94ch
Copy link
Contributor

dvc94ch commented Jul 18, 2021

so if I understand correctly an attack is only practical on 5 rounds, giving it a 3 round safety margin.

@dvc94ch
Copy link
Contributor

dvc94ch commented Jul 19, 2021

Please show your support for libp2p/specs#351 by liking it. Thanks!

@tomaka
Copy link
Member

tomaka commented Jul 21, 2021

One other difference is the use of unbounded channels. This is for two reasons. 1. it simplifies the implementation, and 2. I haven't found a benchmark that performs better by requiring task switching than just buffering the data. This also matches my experience with benchmarks in other projects, like netsim-embed for example.

The reason for bounded channels isn't related to performances at all, but for DoS resistance purposes. When using unbounded channels, the queue of messages can grow forever if someone sends messages faster than the rest of the software can process them. Since you have no control over the rate at which the socket receives messages and the rate at which the messages will be processed, you must have some sort of "stopping mechanism" that kills connections if they send more data than the node can handle.

I invite you to take a look at this comment.

@dvc94ch
Copy link
Contributor

dvc94ch commented Jul 21, 2021

so the reasons for my effort to get this upstreamed are the following:

The transport listen_on api in the swarm doesn't work that well for quic, or at least I haven't found a nice way to implement it. In ipfs-embed we call listen_on on the transport and then again on the swarm to make it work.

I think the relay makes some assumptions that are tcp specific and having a slightly different transport in the codebase might lead to a better design.

Some points for improvement did come out of this although mostly not that surprising:

  • add an alpn string to the handshake intialization string [0]
  • use of bounded channels [1]
  • support tls in addition to noise [2]
  • tokio support [3]

However I'm not sure there is a path forward currently to upstreaming it due to a lack of a libp2p spec, so I'll continue maintaining it out-of-tree.

@DemiMarie
Copy link
Contributor

However I'm not sure there is a path forward currently to upstreaming it due to a lack of a libp2p spec, so I'll continue maintaining it out-of-tree.

There already is a (flawed) spec for using TLS in QUIC. That can be improved with a single change.

@dvc94ch
Copy link
Contributor

dvc94ch commented Jul 21, 2021

I guess supporting tls wouldn't be too much work (since you already wrote the code). Supporting both via feature flags is probably a bit more involved. Although to be honest I have no clue how tls actually works.

@DemiMarie
Copy link
Contributor

I guess supporting tls wouldn't be too much work (since you already wrote the code). Supporting both via feature flags is probably a bit more involved. Although to be honest I have no clue how tls actually works.

rustls provides a high-quality implementation of the TLS protocol, and I got the necessary changes made upstream to allow it to be used in libp2p. I also wrote a library for low-level X.509 certificate parsing, and another one for basic ASN.1 serialization.

@elenaf9
Copy link
Contributor

elenaf9 commented Sep 22, 2022

Thank you for the huge work that has been done here @DemiMarie @tomaka!
I am closing this PR in favor of #2289, which is based on this PR / the tomaka/quiccc-again branch. Unfortunately its not visible in its git history, however you will definitely be mentioned as co-authors on merge.

@elenaf9 elenaf9 closed this Sep 22, 2022
elenaf9 added a commit to kpp/rust-libp2p that referenced this pull request Oct 4, 2022
After 4a317d the code is now completely based on libp2p#1334, thus the
authorship should be set accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.