fix(agent-tunnel): address Copilot review feedback on PR #1741

- routing.rs: when `explicit_agent_id` is set but the gateway has no
  tunnel handle, return `Err` instead of silently falling back to a
  direct connect. A token that names a specific `jet_agent_id` is
  declaring a required network boundary; silent fallback would bypass
  it.
- api/fwd.rs, generic_client.rs, rd_clean_path.rs, api/kdc_proxy.rs:
  use `TargetAddr::as_addr()` (which brackets IPv6) instead of
  `format!("{host}:{port}")` or `to_string()` (which includes scheme).
  Fixes two real bugs: IPv6 targets were malformed (`::1:443` vs
  `[::1]:443`), and kdc_proxy was passing `tcp://host:88` to the
  tunnel target parser — which only accepts bare `host:port`.
- rdp_proxy.rs: add a `TODO(agent-tunnel)` documenting that CredSSP
  Kerberos network requests cannot currently traverse the agent
  tunnel because `send_network_request` hardcodes `None` for the
  handle. Edge case (KDC behind a NAT'd site only reachable via an
  enrolled agent); plumbing the handle through `RdpProxy` is a
  follow-up.
- tests/agent_tunnel_routing.rs: replace a flaky `thread::sleep(10ms)`
  (Windows timer resolution is ~16 ms) with an explicit
  `set_received_at_for_test` helper. Adds two new tests for the new
  explicit-agent-without-handle error path.
- registry.rs: expose `set_received_at_for_test` for the above.
- agent-tunnel-proto/control.rs: fix a stale doc comment that claimed
  `subnets` is IPv4+IPv6 (it is IPv4-only; `Vec<Ipv4Network>`).

Author: irvingoujAtDevolution

crates/agent-tunnel-proto/src/control.rs

@@ -60,7 +60,7 @@ pub enum ControlMessage {
         protocol_version: u16,
         /// Monotonically increasing epoch within this agent process lifetime.
         epoch: u64,
-        /// Reachable subnets (IPv4 and IPv6).
+        /// Reachable IPv4 subnets.
         subnets: Vec<Ipv4Network>,
         /// DNS domains this agent can resolve, with source tracking.
         domains: Vec<DomainAdvertisement>,

crates/agent-tunnel/src/registry.rs

@@ -117,6 +117,16 @@ impl AgentPeer {
         self.last_seen.store(last_seen_ms, Ordering::Release);
     }
 
+    /// Overwrite `received_at` on the current route state.
+    ///
+    /// Intended for tests that need to assert ordering by arrival time without
+    /// relying on wall-clock `thread::sleep` — which is flaky on platforms with
+    /// coarse timer resolution (e.g. Windows ~16 ms).
+    pub fn set_received_at_for_test(&self, received_at: SystemTime) {
+        let mut state = self.route_state.write();
+        state.received_at = received_at;
+    }
+
     /// Returns the last-seen timestamp as milliseconds since UNIX epoch.
     pub fn last_seen_ms(&self) -> u64 {
         self.last_seen.load(Ordering::Acquire)

crates/agent-tunnel/src/routing.rs

@@ -67,7 +67,15 @@ pub async fn try_route(
     target_addr: &str,
 ) -> Result<Option<(TunnelStream, Arc<AgentPeer>)>> {
     let Some(handle) = handle else {
-        return Ok(None);
+        // An explicit `jet_agent_id` claim means the token requires routing via that
+        // specific agent; silently falling back to a direct connect would bypass the
+        // intended network boundary. Reject instead.
+        return match explicit_agent_id {
+            Some(id) => Err(anyhow!(
+                "agent {id} specified in token requires agent tunnel routing, but no tunnel handle is configured"
+            )),
+            None => Ok(None),
+        };
     };
 
     match resolve_route(handle.registry(), explicit_agent_id, target_host).await {

devolutions-gateway/src/api/fwd.rs

@@ -233,16 +233,16 @@ where
 
         let span = tracing::Span::current();
 
-        // Route via agent tunnel: explicit agent_id → subnet → domain → direct
+        // Route via agent tunnel: explicit agent_id → subnet → domain → direct.
+        // `as_addr()` is IPv6-safe (bracketed); `format!("{host}:{port}")` is not.
         let first_target = targets.first();
-        let target_str = format!("{}:{}", first_target.host(), first_target.port());
 
         if let Some((server_stream, _agent)) = agent_tunnel::routing::try_route(
             agent_tunnel_handle.as_deref(),
             claims.jet_agent_id,
             first_target.host(),
             claims.jet_aid,
-            &target_str,
+            first_target.as_addr(),
         )
         .await
         .map_err(ForwardError::BadGateway)?

devolutions-gateway/src/api/kdc_proxy.rs

@@ -164,14 +164,16 @@ pub async fn send_krb_message(
     agent_tunnel_handle: Option<&agent_tunnel::AgentTunnelHandle>,
 ) -> Result<Vec<u8>, HttpError> {
     // Route through agent tunnel using the SAME pipeline as connection forwarding.
-    let kdc_target = kdc_addr.to_string();
+    // `as_addr()` returns `host:port` (with IPv6 brackets), which is what the agent
+    // tunnel target parser expects — unlike `to_string()` which includes the scheme.
+    let kdc_target = kdc_addr.as_addr();
 
     if let Some((mut stream, _agent)) = agent_tunnel::routing::try_route(
         agent_tunnel_handle,
         None,
         kdc_addr.host(),
         uuid::Uuid::new_v4(),
-        &kdc_target,
+        kdc_target,
     )
     .await
     .map_err(|e| HttpError::bad_gateway().build(format!("KDC routing through agent tunnel failed: {e:#}")))?

devolutions-gateway/src/generic_client.rs

@@ -113,16 +113,16 @@ where
                     RecordingPolicy::Proxy => anyhow::bail!("can't meet recording policy"),
                 }
 
-                // Route via agent tunnel: explicit agent_id → subnet → domain → direct
+                // Route via agent tunnel: explicit agent_id → subnet → domain → direct.
+                // `as_addr()` is IPv6-safe (bracketed); `format!("{host}:{port}")` is not.
                 let first_target = targets.first();
-                let target_str = format!("{}:{}", first_target.host(), first_target.port());
 
                 if let Some((server_stream, _agent)) = agent_tunnel::routing::try_route(
                     agent_tunnel_handle.as_deref(),
                     claims.jet_agent_id,
                     first_target.host(),
                     claims.jet_aid,
-                    &target_str,
+                    first_target.as_addr(),
                 )
                 .await?
                 {

devolutions-gateway/src/rd_clean_path.rs

@@ -302,21 +302,22 @@ async fn connect_rdp_server(
     trace!(?targets, "Connecting to destination server");
 
     // Route through agent tunnel if available, otherwise connect directly.
+    // `as_addr()` is IPv6-safe (bracketed); `format!("{host}:{port}")` is not.
     let first_target = targets.first();
-    let target_str = format!("{}:{}", first_target.host(), first_target.port());
+    let target_addr = first_target.as_addr();
 
     let (mut server_stream, server_addr, selected_target): (ServerTransport, SocketAddr, &TargetAddr) =
         match agent_tunnel::routing::try_route(
             agent_tunnel_handle.map(AsRef::as_ref),
             claims.jet_agent_id,
             first_target.host(),
             claims.jet_aid,
-            &target_str,
+            target_addr,
         )
         .await
         {
             Ok(Some((quic_stream, _agent))) => {
-                info!(target = %target_str, "Routing RDP via agent tunnel");
+                info!(target = %target_addr, "Routing RDP via agent tunnel");
                 let placeholder_addr: SocketAddr = "0.0.0.0:0".parse().expect("valid placeholder");
                 (ServerTransport::Quic(quic_stream), placeholder_addr, first_target)
             }

devolutions-gateway/src/rdp_proxy.rs

@@ -637,6 +637,12 @@ where
 async fn send_network_request(request: &NetworkRequest) -> anyhow::Result<Vec<u8>> {
     let target_addr = TargetAddr::parse(request.url.as_str(), Some(88))?;
 
+    // TODO(agent-tunnel): plumb `agent_tunnel_handle` through `RdpProxy` and pass it here
+    // so CredSSP-originated Kerberos network requests can traverse the agent tunnel when
+    // the KDC is only reachable from an enrolled agent's network. Currently these requests
+    // always go out direct from the gateway host, which bypasses the transparent routing
+    // pipeline used by every other proxy path (fwd / rd_clean_path / generic_client /
+    // kdc_proxy). Edge case: KDC behind a NAT'd site with no direct path from the gateway.
     send_krb_message(&target_addr, &request.data, None)
         .await
         .map_err(|err| anyhow::Error::msg("failed to send KDC message").context(err))

devolutions-gateway/tests/agent_tunnel_routing.rs

@@ -4,7 +4,7 @@
 use std::sync::Arc;
 
 use agent_tunnel::registry::{AgentPeer, AgentRegistry};
-use agent_tunnel::routing::{RoutingDecision, resolve_route};
+use agent_tunnel::routing::{RoutingDecision, resolve_route, try_route};
 use agent_tunnel_proto::{DomainAdvertisement, DomainName};
 use ipnetwork::Ipv4Network;
 use uuid::Uuid;
@@ -137,14 +137,17 @@ async fn route_domain_match_returns_multiple_agents_ordered() {
     let peer_a = make_peer("agent-a");
     let subnet_a: Ipv4Network = "10.1.0.0/16".parse().expect("valid test subnet");
     peer_a.update_routes(1, vec![subnet_a], vec![domain("contoso.local")]);
+    // Pin `received_at` explicitly — do NOT rely on thread::sleep to advance
+    // SystemTime, since Windows timer resolution is ~16 ms and makes the
+    // ordering assertion below flaky.
+    peer_a.set_received_at_for_test(std::time::UNIX_EPOCH + std::time::Duration::from_secs(1));
     registry.register(Arc::clone(&peer_a)).await;
 
-    std::thread::sleep(std::time::Duration::from_millis(10));
-
     let peer_b = make_peer("agent-b");
     let id_b = peer_b.agent_id;
     let subnet_b: Ipv4Network = "10.2.0.0/16".parse().expect("valid test subnet");
     peer_b.update_routes(1, vec![subnet_b], vec![domain("contoso.local")]);
+    peer_b.set_received_at_for_test(std::time::UNIX_EPOCH + std::time::Duration::from_secs(2));
     registry.register(Arc::clone(&peer_b)).await;
 
     match resolve_route(&registry, None, "dc01.contoso.local").await {
@@ -155,3 +158,33 @@ async fn route_domain_match_returns_multiple_agents_ordered() {
         other => panic!("expected ViaAgent, got {other:?}"),
     }
 }
+
+// A token that names a specific `jet_agent_id` must NOT silently fall back to a
+// direct connect when the gateway has no agent-tunnel configured — doing so
+// would bypass the intended network boundary.
+#[tokio::test]
+async fn try_route_rejects_explicit_agent_when_handle_missing() {
+    let result = try_route(
+        None,
+        Some(Uuid::new_v4()),
+        "host.example.com",
+        Uuid::new_v4(),
+        "host.example.com:443",
+    )
+    .await;
+
+    assert!(result.is_err(), "expected Err for explicit agent with no handle");
+}
+
+// Without an explicit `jet_agent_id`, a missing handle just means "no tunneling
+// available" — falling back to a direct connect is correct.
+#[tokio::test]
+async fn try_route_without_explicit_agent_falls_through_when_handle_missing() {
+    let result = try_route(None, None, "host.example.com", Uuid::new_v4(), "host.example.com:443").await;
+
+    match result {
+        Ok(None) => {}
+        Ok(Some(_)) => panic!("expected Ok(None), got Ok(Some(_))"),
+        Err(e) => panic!("expected Ok(None), got Err: {e:#}"),
+    }
+}