introduce a mechanism for setting maximum number of connections #19
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce a lightweight handshake that is performed after the server side of a connection is able to fully complete the mTLS process and verify the client side's certificate. Adding this handshake eliminates the client side from prematurely thinking that the connection has been fully established. Before this patch the process looked like: 1. Client initiates contact with the server 2. Server presents its TLS certificate to the client 3. Client verifies the server's certificate <-- Client thinks the connection is complete 4. Client presents its TLS certificate to the server 5. Server verifies the client's certificate <-- Server thinks the connection is complete After this patch this looks like: 1. Client initiates contact with the server 2. Server presents its TLS certificate to the client 3. Client verifies the server's certificate 4. Client presents its TLS certificate to the server 5. Server verifies the client's certificate 6. Server sends ACK <-- Server thinks the connection is complete 7. Client recieves ACK <-- Client thinks the connection is complete
aschran
approved these changes
Dec 16, 2022
| /// Maximum number of concurrent connections to have established at a given point in time. | ||
| /// | ||
| /// This limit is applied in the following ways: | ||
| /// - Inbound connections from [`KnownPeers`] with [`PeerAffinity::High`] bypass this limit. All |
There was a problem hiding this comment.
small request - Can you reword this to make it clearer whether the "limit bypass" cases count towards the limit, even if they bypass it? (I think yes but not 100% sure)
| /// Maximum number of concurrent connections to attempt to establish at a given point in time. | ||
| /// | ||
| /// If unspecified, this will default to `100`. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub max_concurrent_outstanding_connecting_connections: Option<usize>, | ||
|
|
||
| /// Maximum number of concurrent connections to have established at a given point in time. |
| /// the other. This is specifically required so that regaurdless of which peer initiaties a | ||
| /// connection, both sides will be able to know the PeerId of the other side. One challenge with | ||
| /// this is that due to the ordering of how certs are exchanged, the client side may think the | ||
| /// connection is fully established when in reality the server may still reject the connection. To |
There was a problem hiding this comment.
Seems like an extra round trip shouldn't be necessary at least as far as the QUIC protocol is concerned. Is this becuase quinn lacks a "hook" for anemo code to inspect the client identity as part of QUIC establishment, and only lets you look at who the client is after the QUIC handshake is finished? Or what's the missing thing?
There was a problem hiding this comment.
So this isn't a quinn issue but actually sort of an issue with the QUIC protocol (at least my read of it is) take a look at the commit message for the commit that introduces this functionality:
wire: introduce a lightweight handshake
Introduce a lightweight handshake that is performed after the server
side of a connection is able to fully complete the mTLS process and
verify the client side's certificate. Adding this handshake eliminates
the client side from prematurely thinking that the connection has been
fully established.
Before this patch the process looked like:
1. Client initiates contact with the server
2. Server presents its TLS certificate to the client
3. Client verifies the server's certificate <-- Client thinks the connection is complete
4. Client presents its TLS certificate to the server
5. Server verifies the client's certificate <-- Server thinks the connection is complete
After this patch this looks like:
1. Client initiates contact with the server
2. Server presents its TLS certificate to the client
3. Client verifies the server's certificate
4. Client presents its TLS certificate to the server
5. Server verifies the client's certificate
6. Server sends ACK <-- Server thinks the connection is complete
7. Client recieves ACK <-- Client thinks the connection is complete
| let connection = connecting.await?; | ||
|
|
||
| // TODO close the connection explicitly with a reason once we have machine | ||
| // readable errors |
There was a problem hiding this comment.
Maybe add explicit reference to issue #13? (or is this referring to some other machine readable errors?)
Introduce PeerAffinity::Never in order to configure peers which a node would not like to connect with. This acts as a denylist as inbound connections from peers with affinity never are rejected.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.