fix(iroh)!: Update dependencies & fix 0-RTT with newer rustls by pulling the expected NodeId out of the ServerName in verifiers (#3290)

## Description

This is code that will both update deps like #3288 but also makes 0-RTT
tests pass again.

They were failing due to a [change in
`rustls`](https://github.com/rustls/rustls/releases/tag/v%2F0.23.24):
> Behavior change: Clients no longer offer resumption between different
`ClientConfig`s that share a resumption store but do not share server
certificate verification and client authentication credentials. If you
share a resumption store between multiple `ClientConfig`s, please ensure
their server certificate verification and client authentication
credentials are also shared. Please read the new documentation on the
`ClientConfig::resumption` item for details.
>
> Additionally, if you share a resumption store or ticketer between
multiple `ServerConfig`s, please see the new documentation on
`ServerConfig` about this.

Essentially what they're doing is [checking that the certificate
verifiers are `Arc::prt_eq` and depending on that allow or disallow
0-RTT](https://github.com/rustls/rustls/pull/2361/files#diff-9d58cb90e1df25604b88544aca7ea999ccd23bc5657384e196e77f407c0f4acfR256).

When running the tests with rustls logging enabled, I confirmed that
this is indeed what's happening:
> [TRACE rustls::msgs::persist] resumption not allowed between different
ServerCertVerifiers

---

The fix isn't straight-forward.
The reason we're creating new `ServerVerifier`s every time is that we're
passing in the expected remote node ID every time.

There used to be no other way of grabbing the other node's ID in the
verifier, until, coincidentally #3146 landed, which changed the server
name that's set when connecting from always being `localhost` to
`<base32 NodeId>.iroh.invalid`.

I'm now pulling in that value from the `ServerName` that's passed to the
`ServerCertificateVerifier` in `verify_server_cert`.
As far as I understand that value *should* be a client-set value not a
value we get over the network.

The downside of doing this is that we'll not be compatible with iroh
versions that don't use the `<base32 NodeId>.iroh.invalid` server names.

---

There's an unrelated change in here that's changing `make_client_config`
and `make_server_config` infallible, as was done in #3161, which will
definitely also have merge conflicts with this PR.

## Breaking Changes

- `Router::spawn` is now a plain function instead of an `async fn`
- `Router::spawn` is now infallible, instead of returning
`anyhow::Result<()>`

## Change checklist
<!-- Remove any that are not relevant. -->
- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] All breaking changes documented.
- [x] List all breaking changes in the above "Breaking Changes" section.

---------

Co-authored-by: dignifiedquire <me@dignifiedquire.com>

Author: matheus23

Cargo.lock

@@ -9,6 +9,7 @@ allow = [
     "BSD-2-Clause",
     "BSD-3-Clause",
     "BSL-1.0",                        # BOSL license
+    "CDLA-Permissive-2.0",            # Community Data License Agreement Permissive 2.0 due to webpki-root-certs
     "ISC",
     "MIT",
     "Zlib",

deny.toml

@@ -70,7 +70,7 @@ async fn start_accept_side() -> Result<Router> {
     let endpoint = Endpoint::builder().discovery_n0().bind().await?;
 
     // Build our protocol handler and add our protocol, identified by its ALPN, and spawn the node.
-    let router = Router::builder(endpoint).accept(ALPN, Echo).spawn().await?;
+    let router = Router::builder(endpoint).accept(ALPN, Echo).spawn();
 
     Ok(router)
 }

iroh/examples/echo.rs

@@ -85,7 +85,7 @@ async fn main() -> Result<()> {
     let builder = Router::builder(endpoint);
 
     // Add our protocol, identified by our ALPN, to the node, and spawn the node.
-    let router = builder.accept(ALPN, proto.clone()).spawn().await?;
+    let router = builder.accept(ALPN, proto.clone()).spawn();
 
     match args.command {
         Command::Listen { text } => {

iroh/examples/search.rs

@@ -22,7 +22,6 @@ use std::{
 };
 
 use anyhow::{bail, Context, Result};
-use data_encoding::BASE32_DNSSEC;
 use ed25519_dalek::{pkcs8::DecodePublicKey, VerifyingKey};
 use iroh_base::{NodeAddr, NodeId, RelayUrl, SecretKey};
 use iroh_relay::RelayMap;
@@ -78,16 +77,6 @@ pub use super::magicsock::{
 /// is still no connection the configured [`Discovery`] will be used however.
 const DISCOVERY_WAIT_PERIOD: Duration = Duration::from_millis(500);
 
-/// Maximum amount of TLS tickets we will cache (by default) for 0-RTT connection
-/// establishment.
-///
-/// 8 tickets per remote endpoint, 32 different endpoints would max out the required storage:
-/// ~200 bytes per session + certificates (which are ~387 bytes)
-/// So 8 * 32 * (200 + 387) = 150.272 bytes, assuming pointers to certificates
-/// are never aliased pointers (they're Arc'ed).
-/// I think 150KB is an acceptable default upper limit for such a cache.
-const MAX_TLS_TICKETS: usize = 8 * 32;
-
 type DiscoveryBuilder = Box<dyn FnOnce(&SecretKey) -> Option<Box<dyn Discovery>> + Send + Sync>;
 
 /// Defines the mode of path selection for all traffic flowing through
@@ -175,9 +164,8 @@ impl Builder {
             .unwrap_or_else(|| SecretKey::generate(rand::rngs::OsRng));
         let static_config = StaticConfig {
             transport_config: Arc::new(self.transport_config),
-            tls_auth: self.tls_auth,
+            tls_config: tls::TlsConfig::new(self.tls_auth, secret_key.clone()),
             keylog: self.keylog,
-            secret_key: secret_key.clone(),
         };
         #[cfg(not(wasm_browser))]
         let dns_resolver = self.dns_resolver.unwrap_or_default();
@@ -191,7 +179,7 @@ impl Builder {
             1 => Some(discovery.into_iter().next().expect("checked length")),
             _ => Some(Box::new(ConcurrentDiscovery::from_services(discovery))),
         };
-        let server_config = static_config.create_server_config(self.alpn_protocols)?;
+        let server_config = static_config.create_server_config(self.alpn_protocols);
 
         let metrics = EndpointMetrics::default();
 
@@ -544,22 +532,21 @@ impl Builder {
 /// Configuration for a [`quinn::Endpoint`] that cannot be changed at runtime.
 #[derive(Debug)]
 struct StaticConfig {
-    tls_auth: tls::Authentication,
-    secret_key: SecretKey,
+    tls_config: tls::TlsConfig,
     transport_config: Arc<quinn::TransportConfig>,
     keylog: bool,
 }
 
 impl StaticConfig {
     /// Create a [`quinn::ServerConfig`] with the specified ALPN protocols.
-    fn create_server_config(&self, alpn_protocols: Vec<Vec<u8>>) -> Result<ServerConfig> {
-        let quic_server_config =
-            self.tls_auth
-                .make_server_config(&self.secret_key, alpn_protocols, self.keylog)?;
+    fn create_server_config(&self, alpn_protocols: Vec<Vec<u8>>) -> ServerConfig {
+        let quic_server_config = self
+            .tls_config
+            .make_server_config(alpn_protocols, self.keylog);
         let mut server_config = ServerConfig::with_crypto(Arc::new(quic_server_config));
         server_config.transport_config(self.transport_config.clone());
 
-        Ok(server_config)
+        server_config
     }
 }
 
@@ -596,8 +583,6 @@ pub struct Endpoint {
     rtt_actor: Arc<rtt_actor::RttHandle>,
     /// Configuration structs for quinn, holds the transport config, certificate setup, secret key etc.
     static_config: Arc<StaticConfig>,
-    /// Cache for TLS session keys we receive.
-    session_store: Arc<dyn rustls::client::ClientSessionStore>,
 }
 
 impl Endpoint {
@@ -616,7 +601,7 @@ impl Endpoint {
     ///
     /// This is for internal use, the public interface is the [`Builder`] obtained from
     /// [Self::builder]. See the methods on the builder for documentation of the parameters.
-    #[instrument("ep", skip_all, fields(me = %static_config.secret_key.public().fmt_short()))]
+    #[instrument("ep", skip_all, fields(me = %static_config.tls_config.secret_key.public().fmt_short()))]
     async fn bind(static_config: StaticConfig, msock_opts: magicsock::Options) -> Result<Self> {
         let msock = magicsock::MagicSock::spawn(msock_opts).await?;
         trace!("created magicsock");
@@ -626,9 +611,6 @@ impl Endpoint {
             msock: msock.clone(),
             rtt_actor: Arc::new(rtt_actor::RttHandle::new(msock.metrics.magicsock.clone())),
             static_config: Arc::new(static_config),
-            session_store: Arc::new(rustls::client::ClientSessionMemoryCache::new(
-                MAX_TLS_TICKETS,
-            )),
         };
         Ok(ep)
     }
@@ -637,10 +619,9 @@ impl Endpoint {
     ///
     /// This will only affect new incoming connections.
     /// Note that this *overrides* the current list of ALPNs.
-    pub fn set_alpns(&self, alpns: Vec<Vec<u8>>) -> Result<()> {
-        let server_config = self.static_config.create_server_config(alpns)?;
+    pub fn set_alpns(&self, alpns: Vec<Vec<u8>>) {
+        let server_config = self.static_config.create_server_config(alpns);
         self.msock.endpoint().set_server_config(Some(server_config));
-        Ok(())
     }
 
     // # Methods for establishing connectivity.
@@ -758,28 +739,16 @@ impl Endpoint {
         let client_config = {
             let mut alpn_protocols = vec![alpn.to_vec()];
             alpn_protocols.extend(options.additional_alpns);
-            let quic_client_config = self.static_config.tls_auth.make_client_config(
-                &self.static_config.secret_key,
-                node_id,
-                alpn_protocols,
-                Some(self.session_store.clone()),
-                self.static_config.keylog,
-            )?;
+            let quic_client_config = self
+                .static_config
+                .tls_config
+                .make_client_config(alpn_protocols, self.static_config.keylog);
             let mut client_config = quinn::ClientConfig::new(Arc::new(quic_client_config));
             client_config.transport_config(transport_config);
             client_config
         };
 
-        // We used to use a constant "localhost" for this - however, that would put all of
-        // the TLS session tickets we receive into the same bucket in the TLS session ticket cache.
-        // So we choose something that'd dependent on the NodeId.
-        // We cannot use hex to encode the NodeId, as that'd encode to 64 characters, but we only
-        // have 63 maximum per DNS subdomain. Base32 is the next best alternative.
-        // We use the `.invalid` TLD, as that's specified (in RFC 2606) to never actually resolve
-        // "for real", unlike `.localhost` which is allowed to resolve to `127.0.0.1`.
-        // We also add "iroh" as a subdomain, although those 5 bytes might not be necessary.
-        // We *could* decide to remove that indicator in the future likely without breakage.
-        let server_name = &format!("{}.iroh.invalid", BASE32_DNSSEC.encode(node_id.as_bytes()));
+        let server_name = &tls::name::encode(node_id);
         let connect = self.msock.endpoint().connect_with(
             client_config,
             mapped_addr.private_socket_addr(),
@@ -883,15 +852,15 @@ impl Endpoint {
 
     /// Returns the secret_key of this endpoint.
     pub fn secret_key(&self) -> &SecretKey {
-        &self.static_config.secret_key
+        &self.static_config.tls_config.secret_key
     }
 
     /// Returns the node id of this endpoint.
     ///
     /// This ID is the unique addressing information of this node and other peers must know
     /// it to be able to connect to this node.
     pub fn node_id(&self) -> NodeId {
-        self.static_config.secret_key.public()
+        self.static_config.tls_config.secret_key.public()
     }
 
     /// Returns the current [`NodeAddr`] for this endpoint.
@@ -1571,7 +1540,7 @@ impl Future for IncomingFuture {
             Poll::Ready(Ok(inner)) => {
                 let conn = Connection {
                     inner,
-                    tls_auth: this.ep.static_config.tls_auth,
+                    tls_auth: this.ep.static_config.tls_config.auth,
                 };
                 try_send_rtt_msg(&conn, this.ep, None);
                 Poll::Ready(Ok(conn))
@@ -1644,7 +1613,7 @@ impl Connecting {
             Ok((inner, zrtt_accepted)) => {
                 let conn = Connection {
                     inner,
-                    tls_auth: self.ep.static_config.tls_auth,
+                    tls_auth: self.ep.static_config.tls_config.auth,
                 };
                 let zrtt_accepted = ZeroRttAccepted {
                     inner: zrtt_accepted,
@@ -1699,7 +1668,7 @@ impl Future for Connecting {
             Poll::Ready(Ok(inner)) => {
                 let conn = Connection {
                     inner,
-                    tls_auth: this.ep.static_config.tls_auth,
+                    tls_auth: this.ep.static_config.tls_config.auth,
                 };
                 try_send_rtt_msg(&conn, this.ep, *this.remote_node_id);
                 Poll::Ready(Ok(conn))

iroh/src/endpoint.rs

@@ -3460,9 +3460,8 @@ mod tests {
         secret_key: &SecretKey,
         tls_auth: crate::tls::Authentication,
     ) -> ServerConfig {
-        let quic_server_config = tls_auth
-            .make_server_config(secret_key, vec![], false)
-            .expect("should generate valid config");
+        let quic_server_config = crate::tls::TlsConfig::new(tls_auth, secret_key.clone())
+            .make_server_config(vec![], false);
         let mut server_config = ServerConfig::with_crypto(Arc::new(quic_server_config));
         server_config.transport_config(Arc::new(quinn::TransportConfig::default()));
         server_config
@@ -4042,8 +4041,8 @@ mod tests {
         secret_key: SecretKey,
         tls_auth: tls::Authentication,
     ) -> anyhow::Result<Handle> {
-        let quic_server_config =
-            tls_auth.make_server_config(&secret_key, vec![ALPN.to_vec()], true)?;
+        let quic_server_config = tls::TlsConfig::new(tls_auth, secret_key.clone())
+            .make_server_config(vec![ALPN.to_vec()], true);
         let mut server_config = ServerConfig::with_crypto(Arc::new(quic_server_config));
         server_config.transport_config(Arc::new(quinn::TransportConfig::default()));
 
@@ -4110,13 +4109,13 @@ mod tests {
     ) -> Result<quinn::Connection> {
         let alpns = vec![ALPN.to_vec()];
         let quic_client_config =
-            tls_auth.make_client_config(&ep_secret_key, node_id, alpns, None, true)?;
+            tls::TlsConfig::new(tls_auth, ep_secret_key.clone()).make_client_config(alpns, true);
         let mut client_config = quinn::ClientConfig::new(Arc::new(quic_client_config));
         client_config.transport_config(transport_config);
         let connect = ep.connect_with(
             client_config,
             mapped_addr.private_socket_addr(),
-            "localhost",
+            &tls::name::encode(node_id),
         )?;
         let connection = connect.await?;
         Ok(connection)

iroh/src/magicsock.rs

@@ -12,8 +12,7 @@
 //!
 //! let router = Router::builder(endpoint)
 //!     .accept(b"/my/alpn", Echo)
-//!     .spawn()
-//!     .await?;
+//!     .spawn();
 //! # Ok(())
 //! # }
 //!
@@ -78,8 +77,7 @@ use crate::{
 ///
 /// let router = Router::builder(endpoint)
 ///     // .accept(&ALPN, <something>)
-///     .spawn()
-///     .await?;
+///     .spawn();
 ///
 /// // wait until the user wants to
 /// tokio::signal::ctrl_c().await?;
@@ -255,7 +253,7 @@ impl RouterBuilder {
     }
 
     /// Spawns an accept loop and returns a handle to it encapsulated as the [`Router`].
-    pub async fn spawn(self) -> Result<Router> {
+    pub fn spawn(self) -> Router {
         // Update the endpoint with our alpns.
         let alpns = self
             .protocols
@@ -264,10 +262,7 @@ impl RouterBuilder {
             .collect::<Vec<_>>();
 
         let protocols = Arc::new(self.protocols);
-        if let Err(err) = self.endpoint.set_alpns(alpns) {
-            shutdown(&self.endpoint, protocols.clone()).await;
-            return Err(err);
-        }
+        self.endpoint.set_alpns(alpns);
 
         let mut join_set = JoinSet::new();
         let endpoint = self.endpoint.clone();
@@ -333,11 +328,11 @@ impl RouterBuilder {
         let task = task::spawn(run_loop_fut);
         let task = AbortOnDropHandle::new(task);
 
-        Ok(Router {
+        Router {
             endpoint: self.endpoint,
             task: Arc::new(Mutex::new(Some(task))),
             cancel_token: cancel,
-        })
+        }
     }
 }
 
@@ -441,7 +436,7 @@ mod tests {
     #[tokio::test]
     async fn test_shutdown() -> Result<()> {
         let endpoint = Endpoint::builder().bind().await?;
-        let router = Router::builder(endpoint.clone()).spawn().await?;
+        let router = Router::builder(endpoint.clone()).spawn();
 
         assert!(!router.is_shutdown());
         assert!(!endpoint.is_closed());
@@ -481,10 +476,7 @@ mod tests {
         let e1 = Endpoint::builder().bind().await?;
         // deny all access
         let proto = AccessLimit::new(Echo, |_node_id| false);
-        let r1 = Router::builder(e1.clone())
-            .accept(ECHO_ALPN, proto)
-            .spawn()
-            .await?;
+        let r1 = Router::builder(e1.clone()).accept(ECHO_ALPN, proto).spawn();
 
         let addr1 = r1.endpoint().node_addr().await?;