fix: Copilot review — version checks, cert validation, type safety

irvingoujAtDevolution · claude · irvingoujAtDevolution · commit fe7aa96b62c6 · 2026-04-10T13:53:33.000-04:00
- Hoist protocol version validation before match in both gateway and
  agent control loops (single check, no per-variant boilerplate)
- Validate ConnectResponse protocol version in connect_via_agent
- ServerCertStatus enum for ensure_server_cert (expiry + hostname SAN)
- send.finish() after proxy copy (graceful QUIC EOF)
- Fix constant_time_eq doc (inaccurate timing claim)
- Extract ALPN to agent_tunnel_proto::ALPN_PROTOCOL constant
- Destruct EnrollResponse at parameter level for readability
- ValidatedTunnelConf: make wrong state unrepresentable at type level
  (dto::TunnelConf for JSON, TunnelConf for runtime with non-optional fields)

Co-Authored-By: Claude Opus 4.6 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/crates/agent-tunnel-proto/src/lib.rs b/crates/agent-tunnel-proto/src/lib.rs
@@ -23,7 +23,7 @@ pub use control::{ControlMessage, DomainAdvertisement, MAX_CONTROL_MESSAGE_SIZE}
 pub use error::ProtoError;
 pub use session::{ConnectRequest, ConnectResponse, MAX_SESSION_MESSAGE_SIZE};
 pub use stream::{ControlRecvStream, ControlSendStream, ControlStream, SessionStream};
-pub use version::{CURRENT_PROTOCOL_VERSION, MIN_SUPPORTED_VERSION, validate_protocol_version};
+pub use version::{ALPN_PROTOCOL, CURRENT_PROTOCOL_VERSION, MIN_SUPPORTED_VERSION, validate_protocol_version};
 
 /// Current wall-clock time in milliseconds since UNIX epoch.
 pub fn current_time_millis() -> u64 {
diff --git a/crates/agent-tunnel-proto/src/version.rs b/crates/agent-tunnel-proto/src/version.rs
@@ -1,3 +1,6 @@
+/// ALPN protocol identifier for the QUIC agent tunnel, including version suffix.
+pub const ALPN_PROTOCOL: &[u8] = b"gw-agent-tunnel/1";
+
 /// Current protocol version.
 pub const CURRENT_PROTOCOL_VERSION: u16 = 1;
 
diff --git a/devolutions-agent/src/config.rs b/devolutions-agent/src/config.rs
@@ -20,11 +20,84 @@ pub struct Conf {
     pub remote_desktop: RemoteDesktopConf,
     pub pedm: dto::PedmConf,
     pub session: dto::SessionConf,
-    pub tunnel: dto::TunnelConf,
+    pub tunnel: TunnelConf,
     pub proxy: dto::ProxyConf,
     pub debug: dto::DebugConf,
 }
 
+/// Validated tunnel configuration — required fields are guaranteed present.
+///
+/// Constructed from `dto::TunnelConf` via `TryFrom`. If the tunnel is disabled
+/// or not yet enrolled, the `enabled` field is `false` and path fields are empty
+/// (but the struct is always constructible).
+#[derive(Debug, Clone)]
+pub struct TunnelConf {
+    pub enabled: bool,
+    pub gateway_endpoint: String,
+    pub client_cert_path: Utf8PathBuf,
+    pub client_key_path: Utf8PathBuf,
+    pub gateway_ca_cert_path: Utf8PathBuf,
+    pub advertise_subnets: Vec<String>,
+    pub advertise_domains: Vec<String>,
+    pub auto_detect_domain: bool,
+    pub heartbeat_interval_secs: u64,
+    pub route_advertise_interval_secs: u64,
+    pub server_spki_sha256: Option<String>,
+}
+
+impl TryFrom<dto::TunnelConf> for TunnelConf {
+    type Error = anyhow::Error;
+
+    fn try_from(conf: dto::TunnelConf) -> anyhow::Result<Self> {
+        if !conf.enabled {
+            // Disabled tunnel — return a placeholder with defaults.
+            return Ok(Self {
+                enabled: false,
+                gateway_endpoint: String::new(),
+                client_cert_path: Utf8PathBuf::new(),
+                client_key_path: Utf8PathBuf::new(),
+                gateway_ca_cert_path: Utf8PathBuf::new(),
+                advertise_subnets: Vec::new(),
+                advertise_domains: Vec::new(),
+                auto_detect_domain: true,
+                heartbeat_interval_secs: 60,
+                route_advertise_interval_secs: 30,
+                server_spki_sha256: None,
+            });
+        }
+
+        // Enabled tunnel — all required fields must be present.
+        let client_cert_path = conf
+            .client_cert_path
+            .context("tunnel enabled but client_cert_path not configured")?;
+        let client_key_path = conf
+            .client_key_path
+            .context("tunnel enabled but client_key_path not configured")?;
+        let gateway_ca_cert_path = conf
+            .gateway_ca_cert_path
+            .context("tunnel enabled but gateway_ca_cert_path not configured")?;
+
+        anyhow::ensure!(
+            !conf.gateway_endpoint.is_empty(),
+            "tunnel enabled but gateway_endpoint is empty"
+        );
+
+        Ok(Self {
+            enabled: true,
+            gateway_endpoint: conf.gateway_endpoint,
+            client_cert_path,
+            client_key_path,
+            gateway_ca_cert_path,
+            advertise_subnets: conf.advertise_subnets,
+            advertise_domains: conf.advertise_domains,
+            auto_detect_domain: conf.auto_detect_domain,
+            heartbeat_interval_secs: conf.heartbeat_interval_secs.unwrap_or(60),
+            route_advertise_interval_secs: conf.route_advertise_interval_secs.unwrap_or(30),
+            server_spki_sha256: conf.server_spki_sha256,
+        })
+    }
+}
+
 impl Conf {
     pub fn from_conf_file(conf_file: &dto::ConfFile) -> anyhow::Result<Self> {
         let data_dir = get_data_dir();
@@ -49,7 +122,12 @@ impl Conf {
             remote_desktop,
             pedm: conf_file.pedm.clone().unwrap_or_default(),
             session: conf_file.session.clone().unwrap_or_default(),
-            tunnel: conf_file.tunnel.clone().unwrap_or_default(),
+            tunnel: conf_file
+                .tunnel
+                .clone()
+                .unwrap_or_default()
+                .pipe(TunnelConf::try_from)
+                .context("invalid tunnel config")?,
             proxy: conf_file.proxy.clone().unwrap_or_default(),
             debug: conf_file.debug.clone().unwrap_or_default(),
         })
diff --git a/devolutions-agent/src/enrollment.rs b/devolutions-agent/src/enrollment.rs
@@ -118,7 +118,13 @@ async fn request_enrollment(
 fn persist_enrollment_response(
     agent_name: &str,
     advertise_subnets: Vec<String>,
-    enroll_response: EnrollResponse,
+    EnrollResponse {
+        agent_id,
+        client_cert_pem,
+        gateway_ca_cert_pem,
+        quic_endpoint,
+        server_spki_sha256,
+    }: EnrollResponse,
     key_pem: &str,
 ) -> anyhow::Result<PersistedEnrollment> {
     let config_path = config::get_conf_file_path();
@@ -132,19 +138,19 @@ fn persist_enrollment_response(
     std::fs::create_dir_all(&cert_dir)
         .with_context(|| format!("failed to create certificate directory: {}", cert_dir))?;
 
-    let client_cert_path = cert_dir.join(format!("{}-cert.pem", enroll_response.agent_id));
-    let client_key_path = cert_dir.join(format!("{}-key.pem", enroll_response.agent_id));
+    let client_cert_path = cert_dir.join(format!("{agent_id}-cert.pem"));
+    let client_key_path = cert_dir.join(format!("{agent_id}-key.pem"));
     let gateway_ca_path = cert_dir.join("gateway-ca.pem");
 
     // Write the locally-generated private key first (before cert/CA from the network).
     std::fs::write(&client_key_path, key_pem)
-        .with_context(|| format!("failed to write client private key: {}", client_key_path))?;
+        .with_context(|| format!("failed to write client private key: {client_key_path}"))?;
 
-    std::fs::write(&client_cert_path, &enroll_response.client_cert_pem)
-        .with_context(|| format!("failed to write client certificate: {}", client_cert_path))?;
+    std::fs::write(&client_cert_path, &client_cert_pem)
+        .with_context(|| format!("failed to write client certificate: {client_cert_path}"))?;
 
-    std::fs::write(&gateway_ca_path, &enroll_response.gateway_ca_cert_pem)
-        .with_context(|| format!("failed to write gateway CA certificate: {}", gateway_ca_path))?;
+    std::fs::write(&gateway_ca_path, &gateway_ca_cert_pem)
+        .with_context(|| format!("failed to write gateway CA certificate: {gateway_ca_path}"))?;
 
     // Restrict permissions on cert/key files (owner-only on Unix).
     #[cfg(unix)]
@@ -167,7 +173,7 @@ fn persist_enrollment_response(
 
     let tunnel_conf = config::dto::TunnelConf {
         enabled: true,
-        gateway_endpoint: enroll_response.quic_endpoint.clone(),
+        gateway_endpoint: quic_endpoint.clone(),
         client_cert_path: Some(client_cert_path.clone()),
         client_key_path: Some(client_key_path.clone()),
         gateway_ca_cert_path: Some(gateway_ca_path.clone()),
@@ -176,19 +182,19 @@ fn persist_enrollment_response(
         auto_detect_domain: existing_tunnel.map(|t| t.auto_detect_domain).unwrap_or(true),
         heartbeat_interval_secs: Some(60),
         route_advertise_interval_secs: Some(30),
-        server_spki_sha256: Some(enroll_response.server_spki_sha256.clone()),
+        server_spki_sha256: Some(server_spki_sha256),
     };
 
     conf_file.tunnel = Some(tunnel_conf);
 
     config::save_config(&conf_file)?;
 
     Ok(PersistedEnrollment {
-        agent_id: enroll_response.agent_id,
+        agent_id,
         agent_name: agent_name.to_owned(),
         client_cert_path,
         client_key_path,
         gateway_ca_path,
-        quic_endpoint: enroll_response.quic_endpoint,
+        quic_endpoint,
     })
 }
diff --git a/devolutions-agent/src/tunnel.rs b/devolutions-agent/src/tunnel.rs
@@ -181,18 +181,9 @@ async fn run_single_connection(conf_handle: &ConfHandle, shutdown_signal: &mut S
     let agent_conf = conf_handle.get_conf();
     let tunnel_conf = &agent_conf.tunnel;
 
-    let cert_path = tunnel_conf
-        .client_cert_path
-        .as_ref()
-        .context("client_cert_path not configured")?;
-    let key_path = tunnel_conf
-        .client_key_path
-        .as_ref()
-        .context("client_key_path not configured")?;
-    let ca_path = tunnel_conf
-        .gateway_ca_cert_path
-        .as_ref()
-        .context("gateway_ca_cert_path not configured")?;
+    let cert_path = &tunnel_conf.client_cert_path;
+    let key_path = &tunnel_conf.client_key_path;
+    let ca_path = &tunnel_conf.gateway_ca_cert_path;
 
     let advertise_subnets: Vec<Ipv4Network> = tunnel_conf
         .advertise_subnets
@@ -291,7 +282,7 @@ async fn run_single_connection(conf_handle: &ConfHandle, shutdown_signal: &mut S
         .with_client_auth_cert(certs, key)
         .context("build rustls client config with client auth")?;
 
-    client_crypto.alpn_protocols = vec![b"gw-agent-tunnel/1".to_vec()];
+    client_crypto.alpn_protocols = vec![agent_tunnel_proto::ALPN_PROTOCOL.to_vec()];
 
     let mut transport = quinn::TransportConfig::default();
     transport
@@ -356,8 +347,8 @@ async fn run_single_connection(conf_handle: &ConfHandle, shutdown_signal: &mut S
 
     // -- Main loop: accept incoming session streams + periodic tasks --
 
-    let route_interval = tunnel_conf.route_advertise_interval_secs.unwrap_or(30);
-    let heartbeat_interval_secs = tunnel_conf.heartbeat_interval_secs.unwrap_or(60);
+    let route_interval = tunnel_conf.route_advertise_interval_secs;
+    let heartbeat_interval_secs = tunnel_conf.heartbeat_interval_secs;
     let mut route_tick = tokio::time::interval(Duration::from_secs(route_interval));
     let mut heartbeat_tick = tokio::time::interval(Duration::from_secs(heartbeat_interval_secs));
     // Skip the first immediate tick (we already sent the initial RouteAdvertise).
@@ -414,15 +405,16 @@ async fn run_control_reader<R: tokio::io::AsyncRead + Unpin>(mut ctrl: ControlRe
         loop {
             let message = ctrl.recv().await.context("recv control message")?;
 
+            let protocol_version = message.protocol_version();
+            if agent_tunnel_proto::validate_protocol_version(protocol_version)
+                .inspect_err(|e| warn!(%protocol_version, %e, "Ignoring control message: unsupported version"))
+                .is_err()
+            {
+                continue;
+            }
+
             match message {
-                ControlMessage::HeartbeatAck {
-                    protocol_version,
-                    timestamp_ms,
-                } => {
-                    if let Err(e) = agent_tunnel_proto::validate_protocol_version(protocol_version) {
-                        warn!(%protocol_version, %e, "Ignoring HeartbeatAck: unsupported protocol version");
-                        continue;
-                    }
+                ControlMessage::HeartbeatAck { timestamp_ms, .. } => {
                     let rtt = current_time_millis().saturating_sub(timestamp_ms);
                     debug!(rtt_ms = rtt, "Received HeartbeatAck");
                 }
@@ -495,6 +487,9 @@ async fn run_session_proxy(advertise_subnets: Vec<Ipv4Network>, send: quinn::Sen
         r1.inspect_err(|e| debug!(%e, "QUIC->TCP copy ended"))?;
         r2.inspect_err(|e| debug!(%e, "TCP->QUIC copy ended"))?;
 
+        // Gracefully finish the QUIC send stream (signals EOF to peer).
+        let _ = send.finish();
+
         Ok(())
     }
     .await
diff --git a/devolutions-gateway/src/agent_tunnel/cert.rs b/devolutions-gateway/src/agent_tunnel/cert.rs
@@ -179,10 +179,14 @@ impl CaManager {
         let cert_path = self.data_dir.join(SERVER_CERT_FILENAME);
         let key_path = self.data_dir.join(SERVER_KEY_FILENAME);
 
-        if cert_path.exists() && key_path.exists() {
-            // TODO: check cert expiry and regenerate if near/past expiration (365-day validity).
-            info!(%cert_path, "Using existing agent tunnel server certificate");
-            return Ok((cert_path, key_path));
+        match check_server_cert(&cert_path, &key_path, hostname) {
+            ServerCertStatus::Valid => {
+                info!(%cert_path, "Using existing agent tunnel server certificate");
+                return Ok((cert_path, key_path));
+            }
+            status => {
+                info!(%cert_path, ?status, "Generating server certificate");
+            }
         }
 
         info!(%hostname, "Generating agent tunnel server certificate");
@@ -301,7 +305,7 @@ impl CaManager {
             .with_single_cert(server_cert_chain, server_private_key)
             .context("build rustls ServerConfig")?;
 
-        tls_config.alpn_protocols = vec![b"gw-agent-tunnel/1".to_vec()];
+        tls_config.alpn_protocols = vec![agent_tunnel_proto::ALPN_PROTOCOL.to_vec()];
 
         Ok(tls_config)
     }
@@ -378,6 +382,65 @@ pub fn extract_agent_id_from_der(der_bytes: &[u8]) -> anyhow::Result<Uuid> {
     anyhow::bail!("no urn:uuid: SAN found in certificate")
 }
 
+// ---------------------------------------------------------------------------
+// Server certificate validation
+// ---------------------------------------------------------------------------
+
+/// Why an existing server certificate cannot be reused.
+#[derive(Debug)]
+enum ServerCertStatus {
+    /// Certificate is valid and matches the configured hostname.
+    Valid,
+    /// Certificate or key file does not exist yet.
+    NotFound,
+    /// Certificate expires within 7 days.
+    ExpiringSoon,
+    /// Certificate's DNS SAN does not match the configured hostname.
+    HostnameMismatch,
+    /// Certificate file is corrupt or unparseable.
+    Unreadable,
+}
+
+fn check_server_cert(cert_path: &Utf8Path, key_path: &Utf8Path, hostname: &str) -> ServerCertStatus {
+    if !cert_path.exists() || !key_path.exists() {
+        return ServerCertStatus::NotFound;
+    }
+
+    let Ok(pem_str) = std::fs::read_to_string(cert_path) else {
+        return ServerCertStatus::Unreadable;
+    };
+    let Ok(parsed) = pem::parse(&pem_str) else {
+        return ServerCertStatus::Unreadable;
+    };
+    let Ok((_, cert)) = x509_parser::parse_x509_certificate(parsed.contents()) else {
+        return ServerCertStatus::Unreadable;
+    };
+
+    // Expiry: reject if < 7 days remaining.
+    let not_after = cert.validity().not_after.to_datetime();
+    let threshold = time::OffsetDateTime::now_utc() + Duration::from_secs(7 * SECS_PER_DAY);
+    if not_after <= threshold {
+        return ServerCertStatus::ExpiringSoon;
+    }
+
+    // Hostname: reject if DNS SAN doesn't match the configured hostname.
+    let san_matches = cert.extensions().iter().any(|ext| {
+        if let x509_parser::extensions::ParsedExtension::SubjectAlternativeName(san) = ext.parsed_extension() {
+            san.general_names
+                .iter()
+                .any(|name| matches!(name, x509_parser::extensions::GeneralName::DNSName(h) if *h == hostname))
+        } else {
+            false
+        }
+    });
+
+    if !san_matches {
+        return ServerCertStatus::HostnameMismatch;
+    }
+
+    ServerCertStatus::Valid
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/devolutions-gateway/src/agent_tunnel/listener.rs b/devolutions-gateway/src/agent_tunnel/listener.rs
diff --git a/devolutions-gateway/src/api/tunnel.rs b/devolutions-gateway/src/api/tunnel.rs
