refactor!(iroh-net): remove relay secret key (#2847)

The `secret_key` configuration of the relay server was actually dead code at this point, this removes it from the config and all associated dead code. This also removes the need to write to the config file of the `iroh-relay`, making deployments easier to secure. ## Breaking Changes - removed - the `secret_key` property of the `iroh-relay` config - `iroh_net::relay::server::ServerActorTask::secret_key` - `iroh_net::relay::server::ServerActorTask::public_key` - `iroh_net::relay::server::ServerActorTask::meta_cert` - field `secret_key` in `iroh_net::relay::server::RelayConfig` - changed - `iroh_net::relay::server::ServerActorTask::new` now takes no arguments Closes #2845
n0-computer · Oct 28, 2024 · 17f30b1 · 17f30b1
1 parent 8f75005
commit 17f30b1
Show file tree

Hide file tree

Showing 7 changed files with 40 additions and 222 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/iroh-net/Cargo.toml b/iroh-net/Cargo.toml
@@ -81,7 +81,6 @@ axum = { version = "0.7.4", optional = true }
 clap = { version = "4", features = ["derive"], optional = true }
 regex = { version = "1.7.1", optional = true }
 rustls-pemfile = { version = "2.1", optional = true }
-serde_with = { version = "3.3", optional = true }
 toml = { version = "0.8", optional = true }
 tracing-subscriber = { version = "0.3", features = ["env-filter"], optional = true }
 tokio-rustls-acme = { version = "0.4", optional = true }
@@ -140,7 +139,6 @@ iroh-relay = [
     "dep:toml",
     "dep:rustls-pemfile",
     "dep:regex",
-    "dep:serde_with",
     "dep:tracing-subscriber"
 ]
 metrics = ["iroh-metrics/metrics"]

diff --git a/iroh-net/src/bin/iroh-relay.rs b/iroh-net/src/bin/iroh-relay.rs
@@ -12,13 +12,11 @@ use anyhow::{anyhow, bail, Context as _, Result};
 use clap::Parser;
 use iroh_net::{
     defaults::{DEFAULT_HTTPS_PORT, DEFAULT_HTTP_PORT, DEFAULT_METRICS_PORT, DEFAULT_STUN_PORT},
-    key::SecretKey,
     relay::server as iroh_relay,
 };
 use serde::{Deserialize, Serialize};
-use serde_with::{serde_as, DisplayFromStr};
 use tokio_rustls_acme::{caches::DirCache, AcmeConfig};
-use tracing::{debug, info};
+use tracing::debug;
 use tracing_subscriber::{prelude::*, EnvFilter};
 
 /// The default `http_bind_port` when using `--dev`.
@@ -94,16 +92,8 @@ fn load_secret_key(
 /// Configuration for the relay-server.
 ///
 /// This is (de)serialised to/from a TOML config file.
-#[serde_as]
 #[derive(Debug, Clone, Serialize, Deserialize)]
 struct Config {
-    /// The iroh [`SecretKey`] for this relay server.
-    ///
-    /// If not specified a new key will be generated and the config file will be re-written
-    /// using it.
-    #[serde_as(as = "DisplayFromStr")]
-    #[serde(default = "SecretKey::generate")]
-    secret_key: SecretKey,
     /// Whether to enable the Relay server.
     ///
     /// Defaults to `true`.
@@ -178,7 +168,6 @@ impl Config {
 impl Default for Config {
     fn default() -> Self {
         Self {
-            secret_key: SecretKey::generate(),
             enable_relay: true,
             http_bind_addr: None,
             tls: None,
@@ -321,10 +310,6 @@ impl Config {
             .await
             .context("unable to read config")?;
         let config: Self = toml::from_str(&config_ser).context("config file must be valid toml")?;
-        if !config_ser.contains("secret_key") {
-            info!("generating new secret key and updating config file");
-            config.write_to_file(path).await?;
-        }
 
         Ok(config)
     }
@@ -430,7 +415,6 @@ async fn build_relay_config(cfg: Config) -> Result<iroh_relay::ServerConfig<std:
             .unwrap_or_default(),
     };
     let relay_config = iroh_relay::RelayConfig {
-        secret_key: cfg.secret_key.clone(),
         http_bind_addr: cfg.http_bind_addr(),
         tls,
         limits,

diff --git a/iroh-net/src/relay/server.rs b/iroh-net/src/relay/server.rs
@@ -28,7 +28,6 @@ use tokio_util::task::AbortOnDropHandle;
 use tracing::{debug, error, info, info_span, instrument, trace, warn, Instrument};
 
 use crate::{
-    key::SecretKey,
     relay::http::{LEGACY_RELAY_PROBE_PATH, RELAY_PROBE_PATH},
     stun,
 };
@@ -50,7 +49,6 @@ pub use self::{
 const NO_CONTENT_CHALLENGE_HEADER: &str = "X-Tailscale-Challenge";
 const NO_CONTENT_RESPONSE_HEADER: &str = "X-Tailscale-Response";
 const NOTFOUND: &[u8] = b"Not Found";
-const RELAY_DISABLED: &[u8] = b"relay server disabled";
 const ROBOTS_TXT: &[u8] = b"User-agent: *\nDisallow: /\n";
 const INDEX: &[u8] = br#"<html><body>
 <h1>Iroh Relay</h1>
@@ -94,8 +92,6 @@ pub struct ServerConfig<EC: fmt::Debug, EA: fmt::Debug = EC> {
 /// endpoint is only one of the services served.
 #[derive(Debug)]
 pub struct RelayConfig<EC: fmt::Debug, EA: fmt::Debug = EC> {
-    /// The iroh secret key of the Relay server.
-    pub secret_key: SecretKey,
     /// The socket address on which the Relay HTTP server should bind.
     ///
     /// Normally you'd choose port `80`.  The bind address for the HTTPS server is
@@ -244,9 +240,7 @@ impl Server {
                     None => relay_config.http_bind_addr,
                 };
                 let mut builder = http_server::ServerBuilder::new(relay_bind_addr)
-                    .secret_key(Some(relay_config.secret_key))
                     .headers(headers)
-                    .relay_override(Box::new(relay_disabled_handler))
                     .request_handler(Method::GET, "/", Box::new(root_handler))
                     .request_handler(Method::GET, "/index.html", Box::new(root_handler))
                     .request_handler(
@@ -518,16 +512,6 @@ async fn handle_stun_request(src_addr: SocketAddr, pkt: Vec<u8>, sock: Arc<UdpSo
     }
 }
 
-fn relay_disabled_handler(
-    _r: Request<Incoming>,
-    response: ResponseBuilder,
-) -> HyperResult<Response<BytesBody>> {
-    response
-        .status(StatusCode::NOT_FOUND)
-        .body(RELAY_DISABLED.into())
-        .map_err(|err| Box::new(err) as HyperError)
-}
-
 fn root_handler(
     _r: Request<Incoming>,
     response: ResponseBuilder,
@@ -712,7 +696,7 @@ mod tests {
 
     use bytes::Bytes;
     use http::header::UPGRADE;
-    use iroh_base::node_addr::RelayUrl;
+    use iroh_base::{key::SecretKey, node_addr::RelayUrl};
 
     use super::*;
     use crate::relay::{
@@ -723,7 +707,6 @@ mod tests {
     async fn spawn_local_relay() -> Result<Server> {
         Server::spawn(ServerConfig::<(), ()> {
             relay: Some(RelayConfig {
-                secret_key: SecretKey::generate(),
                 http_bind_addr: (Ipv4Addr::LOCALHOST, 0).into(),
                 tls: None,
                 limits: Default::default(),
@@ -752,7 +735,6 @@ mod tests {
         let _guard = iroh_test::logging::setup();
         let mut server = Server::spawn(ServerConfig::<(), ()> {
             relay: Some(RelayConfig {
-                secret_key: SecretKey::generate(),
                 http_bind_addr: (Ipv4Addr::LOCALHOST, 1234).into(),
                 tls: None,
                 limits: Default::default(),

diff --git a/iroh-net/src/relay/server/actor.rs b/iroh-net/src/relay/server/actor.rs
@@ -23,7 +23,7 @@ use tungstenite::protocol::Role;
 
 use crate::{
     defaults::timeouts::relay::SERVER_WRITE_TIMEOUT as WRITE_TIMEOUT,
-    key::{PublicKey, SecretKey},
+    key::PublicKey,
     relay::{
         codec::{
             recv_client_key, DerpCodec, PER_CLIENT_SEND_QUEUE_DEPTH, PROTOCOL_VERSION,
@@ -58,10 +58,6 @@ pub struct ServerActorTask {
     /// Optionally specifies how long to wait before failing when writing
     /// to a client
     write_timeout: Option<Duration>,
-    /// secret_key of the client
-    secret_key: SecretKey,
-    /// The DER encoded x509 cert to send after `LetsEncrypt` cert+intermediate.
-    meta_cert: Vec<u8>,
     /// Channel on which to communicate to the [`ServerActor`]
     server_channel: mpsc::Sender<ServerMessage>,
     /// When true, the server has been shutdown.
@@ -73,37 +69,30 @@ pub struct ServerActorTask {
     // TODO: stats collection
 }
 
-impl ServerActorTask {
-    /// TODO: replace with builder
-    pub fn new(key: SecretKey) -> Self {
+impl Default for ServerActorTask {
+    fn default() -> Self {
         let (server_channel_s, server_channel_r) = mpsc::channel(SERVER_CHANNEL_SIZE);
-        let server_actor = ServerActor::new(key.public(), server_channel_r);
+        let server_actor = ServerActor::new(server_channel_r);
         let cancel_token = CancellationToken::new();
         let done = cancel_token.clone();
         let server_task = AbortOnDropHandle::new(tokio::spawn(
-            async move { server_actor.run(done).await }
-                .instrument(info_span!("relay.server", me = %key.public().fmt_short())),
+            async move { server_actor.run(done).await }.instrument(info_span!("relay.server")),
         ));
-        let meta_cert = init_meta_cert(&key.public());
+
         Self {
             write_timeout: Some(WRITE_TIMEOUT),
-            secret_key: key,
-            meta_cert,
             server_channel: server_channel_s,
             closed: false,
             loop_handler: server_task,
             cancel: cancel_token,
         }
     }
+}
 
-    /// Returns the server's secret key.
-    pub fn secret_key(&self) -> &SecretKey {
-        &self.secret_key
-    }
-
-    /// Returns the server's public key.
-    pub fn public_key(&self) -> PublicKey {
-        self.secret_key.public()
+impl ServerActorTask {
+    /// Creates a new default `ServerActorTask`.
+    pub fn new() -> Self {
+        Self::default()
     }
 
     /// Closes the server and waits for the connections to disconnect.
@@ -142,17 +131,10 @@ impl ServerActorTask {
     pub fn client_conn_handler(&self, default_headers: HeaderMap) -> ClientConnHandler {
         ClientConnHandler {
             server_channel: self.server_channel.clone(),
-            secret_key: self.secret_key.clone(),
             write_timeout: self.write_timeout,
             default_headers: Arc::new(default_headers),
         }
     }
-
-    /// Returns the server metadata cert that can be sent by the TLS server to
-    /// let the client skip a round trip during start-up.
-    pub fn meta_cert(&self) -> &[u8] {
-        &self.meta_cert
-    }
 }
 
 /// Handle incoming connections to the Server.
@@ -163,7 +145,6 @@ impl ServerActorTask {
 #[derive(Debug)]
 pub struct ClientConnHandler {
     server_channel: mpsc::Sender<ServerMessage>,
-    secret_key: SecretKey,
     write_timeout: Option<Duration>,
     pub(crate) default_headers: Arc<HeaderMap>,
 }
@@ -172,7 +153,6 @@ impl Clone for ClientConnHandler {
     fn clone(&self) -> Self {
         Self {
             server_channel: self.server_channel.clone(),
-            secret_key: self.secret_key.clone(),
             write_timeout: self.write_timeout,
             default_headers: Arc::clone(&self.default_headers),
         }
@@ -236,17 +216,15 @@ impl ClientConnHandler {
 }
 
 struct ServerActor {
-    key: PublicKey,
     receiver: mpsc::Receiver<ServerMessage>,
     /// All clients connected to this server
     clients: Clients,
     client_counter: ClientCounter,
 }
 
 impl ServerActor {
-    fn new(key: PublicKey, receiver: mpsc::Receiver<ServerMessage>) -> Self {
+    fn new(receiver: mpsc::Receiver<ServerMessage>) -> Self {
         Self {
-            key,
             receiver,
             clients: Clients::new(),
             client_counter: ClientCounter::default(),
@@ -310,7 +288,7 @@ impl ServerActor {
 
                             report_usage_stats(&UsageStatsReport::new(
                                 "relay_accepts".to_string(),
-                                self.key.to_string(),
+                                "relay_server".to_string(), // TODO: other id?
                                 1,
                                 None, // TODO(arqu): attribute to user id; possibly with the re-introduction of request tokens or other auth
                                 Some(key.to_string()),
@@ -346,36 +324,6 @@ impl ServerActor {
     }
 }
 
-/// Initializes the [`ServerActor`] with a self-signed x509 cert
-/// encoding this server's public key and protocol version. "cmd/relay_server
-/// then sends this after the Let's Encrypt leaf + intermediate certs after
-/// the ServerHello (encrypted in TLS 1.3, not that is matters much).
-///
-/// Then the client can save a round trime getting that and can start speaking
-/// relay right away. (we don't use ALPN because that's sent in the clear and
-/// we're being paranoid to not look too weird to any middleboxes, given that
-/// relay is an ultimate fallback path). But since the post-ServerHello certs
-/// are encrypted we can have the client also use them as a signal to be able
-/// to start speaking relay right away, starting with its identity proof,
-/// encrypted to the server's public key.
-///
-/// This RTT optimization fails where there's a corp-mandated TLS proxy with
-/// corp-mandated root certs on employee machines and TLS proxy cleans up
-/// unnecessary certs. In that case we just fall back to the extra RTT.
-fn init_meta_cert(server_key: &PublicKey) -> Vec<u8> {
-    let mut params =
-        rcgen::CertificateParams::new([format!("derpkey{}", hex::encode(server_key.as_bytes()))]);
-    params.serial_number = Some((PROTOCOL_VERSION as u64).into());
-    // Windows requires not_after and not_before set:
-    params.not_after = time::OffsetDateTime::now_utc().saturating_add(30 * time::Duration::DAY);
-    params.not_before = time::OffsetDateTime::now_utc().saturating_sub(30 * time::Duration::DAY);
-
-    rcgen::Certificate::from_params(params)
-        .expect("fixed inputs")
-        .serialize_der()
-        .expect("fixed allocations")
-}
-
 struct ClientCounter {
     clients: HashMap<PublicKey, usize>,
     last_clear_date: Date,
@@ -412,6 +360,7 @@ impl ClientCounter {
 #[cfg(test)]
 mod tests {
     use bytes::Bytes;
+    use iroh_base::key::SecretKey;
     use tokio::io::DuplexStream;
     use tokio_util::codec::{FramedRead, FramedWrite};
     use tracing_subscriber::{prelude::*, EnvFilter};
@@ -446,11 +395,9 @@ mod tests {
 
     #[tokio::test]
     async fn test_server_actor() -> Result<()> {
-        let server_key = SecretKey::generate().public();
-
         // make server actor
         let (server_channel, server_channel_r) = mpsc::channel(20);
-        let server_actor: ServerActor = ServerActor::new(server_key, server_channel_r);
+        let server_actor: ServerActor = ServerActor::new(server_channel_r);
         let done = CancellationToken::new();
         let server_done = done.clone();
 
@@ -518,7 +465,6 @@ mod tests {
         let (server_channel_s, mut server_channel_r) = mpsc::channel(10);
         let client_key = SecretKey::generate();
         let handler = ClientConnHandler {
-            secret_key: client_key.clone(),
             write_timeout: None,
             server_channel: server_channel_s,
             default_headers: Default::default(),
@@ -580,8 +526,7 @@ mod tests {
         let _guard = iroh_test::logging::setup();
 
         // create the server!
-        let server_key = SecretKey::generate();
-        let server: ServerActorTask = ServerActorTask::new(server_key);
+        let server: ServerActorTask = ServerActorTask::new();
 
         // create client a and connect it to the server
         let key_a = SecretKey::generate();
@@ -656,8 +601,7 @@ mod tests {
             .ok();
 
         // create the server!
-        let server_key = SecretKey::generate();
-        let server: ServerActorTask = ServerActorTask::new(server_key);
+        let server: ServerActorTask = ServerActorTask::new();
 
         // create client a and connect it to the server
         let key_a = SecretKey::generate();