From 3e71a916674a52789029a31482599adf2e0d711a Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 18 Jul 2025 03:04:54 +0800 Subject: [PATCH] feat(gateway): revoke unlisted authorizations upon `init` (#9896) When receiving an `init` message from the portal, we will now revoke all authorizations not listed in the `authorizations` list of the `init` message. We (partly) test this by introducing a new transition in our proptests that de-authorizes a certain resource whilst the Gateway is simulated to be partitioned. It is difficult to test that we cannot make a connection once that has happened because we would have to simulate a malicious client that knows about resources / connections or ignores the "remove resource" message. Testing this is deferred to a dedicated task. We do test that we hit the code path of revoking the resource authorization and because the other resources keep working, we also test that we are at least not revoking the wrong ones. Resolves: #9892 --- .github/workflows/_rust.yml | 1 + .../tunnel/proptest-regressions/tests.txt | 1 + rust/connlib/tunnel/src/gateway.rs | 13 +++++++ rust/connlib/tunnel/src/messages/gateway.rs | 8 ++++ rust/connlib/tunnel/src/peer.rs | 11 ++++++ rust/connlib/tunnel/src/tests/reference.rs | 23 +++++++++++ rust/connlib/tunnel/src/tests/sut.rs | 28 ++++++++++++++ rust/connlib/tunnel/src/tests/transition.rs | 3 ++ rust/gateway/src/eventloop.rs | 38 ++++++++++++++----- website/src/components/Changelog/Gateway.tsx | 4 ++ 10 files changed, 121 insertions(+), 9 deletions(-) diff --git a/.github/workflows/_rust.yml b/.github/workflows/_rust.yml index 28b41364e..dbd092308 100644 --- a/.github/workflows/_rust.yml +++ b/.github/workflows/_rust.yml @@ -111,6 +111,7 @@ jobs: rg --count --no-ignore "ICMP Error error=V4TimeExceeded" "$TESTCASES_DIR" rg --count --no-ignore "ICMP Error error=V6TimeExceeded" "$TESTCASES_DIR" rg --count --no-ignore "Forwarding query for DNS resource to corresponding site" "$TESTCASES_DIR" + rg --count --no-ignore "Revoking resource authorization" "$TESTCASES_DIR" env: # diff --git a/rust/connlib/tunnel/proptest-regressions/tests.txt b/rust/connlib/tunnel/proptest-regressions/tests.txt index 6af7700f2..112833b8e 100644 --- a/rust/connlib/tunnel/proptest-regressions/tests.txt +++ b/rust/connlib/tunnel/proptest-regressions/tests.txt @@ -183,3 +183,4 @@ cc f90e2fe4827f91aba42bf4806b53d1a1e7df7d1fd912d3a2cf32774eb0006f8a cc 964a23e1cc7b6b3ef5f65b48228ecb13c4fbb136f053f33ee254c21c8f404f6e cc 589929cc32dca7f976c0b06d0f165ebaa35a4a8bf8dbcc0145041e36d0153b9c cc b7f1a952ce4b23c0dfb2383e3fa245222e3093b2a35bc83569c934481993e5e3 +cc 19a34eafc023d6b960a37fa25f47909ca5dc2ba58311f1748a7b9e791abcd17a diff --git a/rust/connlib/tunnel/src/gateway.rs b/rust/connlib/tunnel/src/gateway.rs index d1c5ddefb..928e27e1c 100644 --- a/rust/connlib/tunnel/src/gateway.rs +++ b/rust/connlib/tunnel/src/gateway.rs @@ -450,6 +450,19 @@ impl GatewayState { pub fn update_tun_device(&mut self, config: IpConfig) { self.tun_ip_config = Some(config); } + + pub fn retain_authorizations( + &mut self, + authorizations: BTreeMap>, + ) { + for (client, resources) in authorizations { + let Some(client) = self.peers.get_mut(&client) else { + continue; + }; + + client.retain_authorizations(resources); + } + } } fn handle_p2p_control_packet( diff --git a/rust/connlib/tunnel/src/messages/gateway.rs b/rust/connlib/tunnel/src/messages/gateway.rs index 8ca2bdf58..ea0ffc363 100644 --- a/rust/connlib/tunnel/src/messages/gateway.rs +++ b/rust/connlib/tunnel/src/messages/gateway.rs @@ -122,6 +122,8 @@ pub struct InitGateway { pub relays: Vec, #[serde(default)] pub account_slug: Option, + #[serde(default)] + pub authorizations: Vec, } #[derive(Debug, Deserialize, Clone, PartialEq, Eq)] @@ -167,6 +169,12 @@ pub struct AllowAccess { pub client_ipv6: Ipv6Addr, } +#[derive(Debug, Deserialize, Clone)] +pub struct Authorization { + pub client_id: ClientId, + pub resource_id: ResourceId, +} + #[derive(Debug, Deserialize, Clone)] pub struct RejectAccess { pub client_id: ClientId, diff --git a/rust/connlib/tunnel/src/peer.rs b/rust/connlib/tunnel/src/peer.rs index 0125c0dba..6c3cbed8f 100644 --- a/rust/connlib/tunnel/src/peer.rs +++ b/rust/connlib/tunnel/src/peer.rs @@ -233,6 +233,17 @@ impl ClientOnGateway { self.recalculate_filters(); } + pub(crate) fn retain_authorizations(&mut self, authorization: BTreeSet) { + for (resource, _) in self + .resources + .extract_if(|resource, _| !authorization.contains(resource)) + { + tracing::info!(%resource, "Revoking resource authorization"); + } + + self.recalculate_filters(); + } + // Call this after any resources change // // This recalculate the ip-table rules, this allows us to remove and add resources and keep the allow-list correct diff --git a/rust/connlib/tunnel/src/tests/reference.rs b/rust/connlib/tunnel/src/tests/reference.rs index e3c1e1109..3fda599b4 100644 --- a/rust/connlib/tunnel/src/tests/reference.rs +++ b/rust/connlib/tunnel/src/tests/reference.rs @@ -250,6 +250,10 @@ impl ReferenceState { |resources_id| Transition::DisableResources(BTreeSet::from_iter(resources_id)), ) }) + .with_if_not_empty(1, state.client.inner().all_resource_ids(), |resources_id| { + sample::select(resources_id) + .prop_map(Transition::DeauthorizeWhileGatewayIsPartitioned) + }) .with_if_not_empty( 10, state.client.inner().ipv4_cidr_resource_dsts(), @@ -516,6 +520,9 @@ impl ReferenceState { state.client.exec_mut(|client| client.reset_connections()); } } + Transition::DeauthorizeWhileGatewayIsPartitioned(resource) => state + .client + .exec_mut(|client| client.remove_resource(resource)), }; state @@ -703,6 +710,22 @@ impl ReferenceState { } Transition::Idle => true, Transition::PartitionRelaysFromPortal => true, + Transition::DeauthorizeWhileGatewayIsPartitioned(r) => { + let has_resource = state.client.inner().has_resource(*r); + let has_gateway_for_resource = state + .portal + .gateway_for_resource(*r) + .is_some_and(|g| state.gateways.contains_key(g)); + let has_tcp_connection = state + .client + .inner() + .tcp_connection_tuple_to_resource(*r) + .is_some(); + + // Don't deactivate resources we don't have. It doesn't hurt but makes the logs of reduced testcases weird. + // Also don't deactivate resources where we have TCP connections as those would get interrupted. + has_resource && has_gateway_for_resource && !has_tcp_connection + } } } diff --git a/rust/connlib/tunnel/src/tests/sut.rs b/rust/connlib/tunnel/src/tests/sut.rs index ec4c76cb6..b7ddcd9bb 100644 --- a/rust/connlib/tunnel/src/tests/sut.rs +++ b/rust/connlib/tunnel/src/tests/sut.rs @@ -23,6 +23,7 @@ use rand::SeedableRng; use rand::distributions::DistString; use sha2::Digest; use snownet::{NoTurnServers, Transmit}; +use std::collections::BTreeSet; use std::iter; use std::{ collections::BTreeMap, @@ -329,6 +330,33 @@ impl TunnelTest { state.deploy_new_relays(new_relays, now, to_remove); } + Transition::DeauthorizeWhileGatewayIsPartitioned(rid) => { + let client_id = state.client.inner().id; + let new_authorized_resources = { + let mut all_resources = + BTreeSet::from_iter(ref_state.client.inner().all_resource_ids()); + all_resources.remove(&rid); + + all_resources + }; + + state.client.exec_mut(|c| c.sut.remove_resource(rid)); + + if let Some(gid) = ref_state.portal.gateway_for_resource(rid) + && let Some(g) = state.gateways.get_mut(gid) + { + g.exec_mut(|g| { + // This is partly an `init` message. + // The relays don't change so we don't bother setting them. + g.sut.retain_authorizations(BTreeMap::from([( + client_id, + new_authorized_resources, + )])) + }); + } else { + tracing::error!(%rid, "No gateway for resource"); + } + } }; state.advance(ref_state, &mut buffered_transmits); diff --git a/rust/connlib/tunnel/src/tests/transition.rs b/rust/connlib/tunnel/src/tests/transition.rs index 2be47c957..375740050 100644 --- a/rust/connlib/tunnel/src/tests/transition.rs +++ b/rust/connlib/tunnel/src/tests/transition.rs @@ -83,6 +83,9 @@ pub(crate) enum Transition { /// /// In this case, we won't receive a `relays_presence` but instead we will receive relays with the same ID yet different credentials. RebootRelaysWhilePartitioned(BTreeMap>), + + /// De-authorize access to a resource whilst the Gateway is network-partitioned from the portal. + DeauthorizeWhileGatewayIsPartitioned(ResourceId), } #[derive(Debug, Clone)] diff --git a/rust/gateway/src/eventloop.rs b/rust/gateway/src/eventloop.rs index 908147411..5ae70e88e 100644 --- a/rust/gateway/src/eventloop.rs +++ b/rust/gateway/src/eventloop.rs @@ -8,14 +8,14 @@ use firezone_telemetry::{Telemetry, analytics}; use firezone_tunnel::messages::gateway::{ AllowAccess, ClientIceCandidates, ClientsIceCandidates, ConnectionReady, EgressMessages, - IngressMessages, RejectAccess, RequestConnection, + IngressMessages, InitGateway, RejectAccess, RequestConnection, }; use firezone_tunnel::messages::{ConnectionAccepted, GatewayResponse, Interface, RelaysPresence}; use firezone_tunnel::{ DnsResourceNatEntry, GatewayTunnel, IPV4_TUNNEL, IPV6_TUNNEL, IpConfig, ResolveDnsRequest, }; use phoenix_channel::{PhoenixChannel, PublicKeyParam}; -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet}; use std::convert::Infallible; use std::future::Future; use std::net::{IpAddr, SocketAddrV4, SocketAddrV6}; @@ -376,10 +376,17 @@ impl Eventloop { Instant::now(), ), phoenix_channel::Event::InboundMessage { - msg: IngressMessages::Init(init), + msg: + IngressMessages::Init(InitGateway { + interface, + config: _, + account_slug, + relays, + authorizations, + }), .. } => { - if let Some(account_slug) = init.account_slug { + if let Some(account_slug) = account_slug { Telemetry::set_account_slug(account_slug.clone()); analytics::identify(RELEASE.to_owned(), Some(account_slug)) @@ -387,13 +394,26 @@ impl Eventloop { self.tunnel.state_mut().update_relays( BTreeSet::default(), - firezone_tunnel::turn(&init.relays), + firezone_tunnel::turn(&relays), Instant::now(), ); self.tunnel.state_mut().update_tun_device(IpConfig { - v4: init.interface.ipv4, - v6: init.interface.ipv6, + v4: interface.ipv4, + v6: interface.ipv6, }); + self.tunnel + .state_mut() + .retain_authorizations(authorizations.into_iter().fold( + BTreeMap::new(), + |mut authorizations, next| { + authorizations + .entry(next.client_id) + .or_default() + .insert(next.resource_id); + + authorizations + }, + )); if self .set_interface_tasks @@ -404,7 +424,7 @@ impl Eventloop { let mut tun_device_manager = tun_device_manager.lock().await; tun_device_manager - .set_ips(init.interface.ipv4, init.interface.ipv6) + .set_ips(interface.ipv4, interface.ipv6) .await .context("Failed to set TUN interface IPs")?; tun_device_manager @@ -412,7 +432,7 @@ impl Eventloop { .await .context("Failed to set TUN routes")?; - Ok(init.interface) + Ok(interface) } }) .is_err() diff --git a/website/src/components/Changelog/Gateway.tsx b/website/src/components/Changelog/Gateway.tsx index e4293b784..96361e728 100644 --- a/website/src/components/Changelog/Gateway.tsx +++ b/website/src/components/Changelog/Gateway.tsx @@ -38,6 +38,10 @@ export default function Gateway() { Fixes an issue where connections would sometimes take up to 90s to establish. + + Fixes a security issue where resource authorizations would not get + revoked if the Gateway was disconnected from the portal. +