Allow app to accept/reject/retry before handshake begins #1752
Conversation
|
Thank you for working on this! I've wanted this feature for a long time, and I'm excited about the amount of discretion it gives applications for practically no increase in intrinsic API complexity. It may be a few days before I have time to dig into reviewing this properly, but I can see it's turned out to be a fairly large change. Any chance you could break it up into a series of smaller incremental, self-contained commits? |
There was a problem hiding this comment.
I skimmed your changes to try and give some high-level feedback.
In particular, the changes to the -proto Endpoint are just too hard to review as one commit. Suggest you refactor into an API that better fits your proposal before making the final change to split off the API that allows the caller to make a decision on handling incoming connections.
|
Thanks for the review! Splitting this into more incremental commits and making the other changes you mentioned makes sense to me. Just so you know, though, it might be a little bit until I can dedicate more time to this--but I'll get back to you when it's ready to be looked at again. |
gretchenfrage
force-pushed
the
1661
branch
4 times, most recently
from
75351ff
to
ea71c80
Compare
|
Should be ready for re-review now. I split this up into various commits which should make this a lot easier to see what I'm doing if you go through them one at a time, and made some of the other changes you requested too.
I don't really see how this would work--at least, I'm not sure how to reconcile that idea with Ralith's request that this functionality be placed into the existing
Yeah that's fair--I've refactored that area in general, it should be a lot cleaner now. |
Alright, I've done a considerable refactor which I think achieves a good compromise between the various desires here, and results in a smaller PR previously. I'm no longer introducing different internal states to Awaiting
Furthermore, it implements |
|
I'm not sure how to feel about having both |
|
After sleeping on it I think the current naming is probably the sweet spot. |
Huh alright. I mean I'd be fine renaming it to |
If it helps clarify the situation: when changing server applications, I suspect what might end up being done most naturally in most cases is just changing functions which accept a diff --git a/perf/src/bin/perf_server.rs b/perf/src/bin/perf_server.rs
index 169ca65e..4b710268 100644
--- a/perf/src/bin/perf_server.rs
+++ b/perf/src/bin/perf_server.rs
@@ -123,7 +123,7 @@ async fn run(opt: Opt) -> Result<()> {
Ok(())
}
-async fn handle(handshake: quinn::Connecting, opt: Arc<Opt>) -> Result<()> {
+async fn handle(handshake: quinn::IncomingConnection, opt: Arc<Opt>) -> Result<()> {
let connection = handshake.await.context("handshake failed")?;
debug!("{} connected", connection.remote_address());
tokio::try_join!(Thus it's the new |
|
I re-added the
It would not actually be that simple, because |
|
All comments should be resolved, and I just refactored the commit history and force-pushed again so it should be more clean now. |
|
There were some non-trivialities to removing Updated branch to remove Previous version of this branch added On main, Other changes I made to this branch:
|
|
I went through and s/reject/refuse/ in the API for consistency w/ the spec and general IETF convention, now that we no longer need to worry about not matching |
Subsequent commits will allow the user to achieve the removed behavior manually and more flexibly. - Removes concurrent_connections from ServerConfig, as well as corresponding check in early_validate_first_packet. - Adds ConnectionError::CidsExhausted error, although it is not yet instantiated. - Renames ConnectError variant TooManyConnections to CidsExhausted. - Renames proto Endpoint internal method is_full to cids_exhausted. - Adds method open_connections to Endpoint (both proto and quinn). - Removes Endpoint method reject_new_connection from Endpoint (both proto and quinn). - Deletes obselete tests concurrent_connections_full and reject_new_connections.
This commit factors out the two fields of a quinn::Endpoint's State necessary to process a proto::Transmit into a new sub-struct, TransmitState. This is to alleviate borrowing issues, because proto::Transmit will soon be called from more call sites than previously. The bulk of this code change is just moving around existing code. Co-authored-by: Dirkjan Ochtman <dirkjan@ochtman.nl>
This commit refactors the logic for a quinn_proto::Endpoint accepting an incoming connection so that it constructs an explicit Incoming struct containing all the necessary state to accept/reject/retry the connection-creating packet. However, the external API stays the same. The bulk of this code change is just moving around existing code. Additionally, adds some gitignore lines I was using for coverage testing.
This commit removes use_retry from the server config and provides a public API for the user to manually accept/refuse/retry incoming connections before a handshake begins, and inspect properties such as an incoming connection's remote address and whether that address is validated when doing so. In quinn-proto, Incoming is made public, as well as Endpoint's accept/ refuse/retry methods which operate on it. The DatagramEvent::NewConnection event is modified to return an incoming but not yet accepted connection. In quinn, awaiting Endpoint::accept now yields a new quinn::Incoming type, rather than quinn::Connecting. The new quinn::Incoming type has all the methods its quinn_proto equivalent has, as well as an accept method to (fallibly) transition it into a Connecting, and also refuse, retry, and ignore methods. Furthermore, quinn::Incoming implements IntoFuture with the output type Result<Connection, ConnectionError>>, which is the same as the Future output type of Connecting. This lets server code which was straightforwardly awaiting the result of quinn::Endpoint::accept work with little to no modification. The test accept_after_close was removed because the functionality it was testing for no longer exists.
This commit adds a new --block option to the server example to
illustate in a simplified way the general structure one would use to
implement IP address blocking with the new accept/reject/retry API.
For example:
cargo run --example server ./ --listen 127.0.0.1:4433 --stateless-retry --block 127.0.0.1:8065
cargo run --example client https://127.0.0.1:4433/Cargo.toml --host localhost --bind 127.0.0.1:8065
One thing to note is that that example places the reject condition
before the retry condition. This expends slightly less effort rejecting
connections, but does create a blocked IP address oracle for an attacker
who can do address spoofing.
This commit adds a new --connection-limit option to the server example to illustrate how a user could implement a limit to the number of connections open at a time with the new "incoming" API and Endpoint::open_connections method rather than with the now-removed concurrent_connections ServerConfig parameter.
|
Made requested tweaks and rebased over main. |
|
Tests passed, so should be ready to merge I think! |
@gretchenfrage , @djc there are some unresolved conversations -- I think we just need to resolve them. I do not see any real outstanding issues. |
|
@lijunwangs there is no need to keep litigating this -- please assume I'm well aware of the state. |
|
@gretchenfrage many thanks for all your work on this, and sorry for my slow feedback cycle(s). |
|
@djc -- sorry for pushing for this -- it is a critical improvement for our use case. Thank you all! |
Closes #1661
Please see individual commit messages.