feat(iroh)!: newtype ServerConfig and RetryError, organize the quinn re-exports
#3757
Conversation
`ServerConfig` is used in the public API in `Incoming::accept_with`. `quinn::ServerConfig` takes a `TransportConfig`. However, we want to maintain control over the `TransportConfig` fields, so we new typed it to `QuicTransportConfig`. So, we needed to new type `quinn::ServerConfig`, which ensures setting the `QuicTransportConfig` correctly. This also organizes the quinn imports and labels where each type comes from.
…pes that are re-exported by iroh
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3757/docs/iroh/ Last updated: 2025-12-12T03:28:29Z |
flub
left a comment
flub
left a comment
There was a problem hiding this comment.
Really nice to get all the imports cleaned up, thank you for slogging through that!
iroh/src/endpoint/connection.rs
Outdated
| self, | ||
| server_config: Arc<ServerConfig>, | ||
| ) -> Result<Accepting, ConnectionError> { | ||
| pub fn accept_with(self, server_config: ServerConfig) -> Result<Accepting, ConnectionError> { |
There was a problem hiding this comment.
Curious why the signature change? It does make sense to put the Arc into the user's control. Usually you'd call this many times with the same config and thus would be forced to clone the ServerConfig a lot.
There was a problem hiding this comment.
yea that's a good point and I will add it back, since it does save some resources, however I removed it because our new-typing kind of made cloning the inner quinn::ServerConfig necessary.
We need to have a non-arc'ed quinn::ServerConfig inside our ServerConfig so that we can edit it, so under the hood we have to clone the quinn::ServerConfig anyway 🙃
If I'm thinking about it the wrong way, I'm happy to be wrong.
iroh/src/endpoint/quic.rs
Outdated
| } | ||
|
|
||
| /// Create a default config with a particular handshake token key | ||
| pub fn new(crypto: Arc<dyn CryptoServerConfig>, token_key: Arc<dyn HandshakeTokenKey>) -> Self { |
There was a problem hiding this comment.
My inclination would be to forbid people from customising the ServerConfig::crypto entirely. Accessing it via the pub fn crypto function above is fine I think.
There was a problem hiding this comment.
Looking at the default code that creates this it seems that the token_key is normally 64 bytes of randomness put into ring::hkdf::Salt::new(hkdf::HKDF_SHA256, &[]).extract(&the_64_random_bytes). It feels like a tall order to ask folks to figure this out, though on the other hand it's a suitably high barrier to using this API which could be helpful.
Anyway, point is I think it's fine to allow folks to customise this value.
There was a problem hiding this comment.
Ah, seems like with_crypto was the API that allowed you to bypass constructing that token_key. We could make new without any arguments and have a with_token_key constructor that takes the token_key I guess.
There was a problem hiding this comment.
Okay, so we can't just create a ServerConfig using a new without any arguments from some outside source. To allow users to create a StaticConfig that doesn't totally bork iroh, I added a method on the Endpoint called create_server_config (bikeshed welcome). It uses the internal StaticConfig that contains the TLS options and default transport config options, but expects the method caller to supply alpns.
That way a user can then replace the bits we want to allow them to replace, without replacing expected iroh crypto.
There was a problem hiding this comment.
Ah, seems like with_crypto was the API that allowed you to bypass constructing that token_key. We could make new without any arguments and have a with_token_key constructor that takes the token_key I guess.
Oh, also the token_key (now set_token_key) method allows setting a different token key already, so I think we have that covered?
Description
Incoming::retryreturns aRetryError, which you can turn back into aquinn::Incoming. We want thatquinn::Incomingto instead be aniroh::endpoint::Incoming. This PR fixes that.Incoming::accept_withtakes aquinn::ServerConfig, however thequinn::ServerConfigallows you to override thequinn::TransportConfig, which we have new typed toQuicTransportConfig. So, this PR new typesquinn::ServerConfigtoquic::ServerConfig, and ensures that thequinn::TransportConfigis set appropriately for multipath use.This PR also explicitly comments on each quinn export and labels what type uses that export and where it enters the public iroh API.
Breaking Changes
iroh:- changed
-
Incoming::retrynow returns aniroh::endpoint::RetryError, rather than aquinn::RetryError-
Incoming::accept_withnow takes airoh::endpoint::ServerConfig, rather than aquinn::ServerConfigNotes & open questions
Okay. So. I impl'd all of the methods one-to-one from
quinn::ServerConfig, but I think there are a bunch of fields we may want to restrict folks from messing with when they useIncoming::accept_with, as currently, they can use this API to replace our normal TlsConfig entirely.I'm also unsure if there are any other bits in here (for example the
quinn::ServerConfig::migratefield) that may interfere with expected multipath usage.My proposal would be to either eliminate the
Incoming::accept_withmethod entirely (therefore removing the need to publically export theServerConfig), or reduceServerConfigin scope so that only fields that will not interfere with "business as usual" for multipath/iroh remain.Change checklist