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. +