mirror of
https://github.com/outbackdingo/firezone.git
synced 2026-01-27 10:18:54 +00:00
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
This commit is contained in:
1
.github/workflows/_rust.yml
vendored
1
.github/workflows/_rust.yml
vendored
@@ -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:
|
||||
# <https://github.com/rust-lang/cargo/issues/5999>
|
||||
|
||||
@@ -183,3 +183,4 @@ cc f90e2fe4827f91aba42bf4806b53d1a1e7df7d1fd912d3a2cf32774eb0006f8a
|
||||
cc 964a23e1cc7b6b3ef5f65b48228ecb13c4fbb136f053f33ee254c21c8f404f6e
|
||||
cc 589929cc32dca7f976c0b06d0f165ebaa35a4a8bf8dbcc0145041e36d0153b9c
|
||||
cc b7f1a952ce4b23c0dfb2383e3fa245222e3093b2a35bc83569c934481993e5e3
|
||||
cc 19a34eafc023d6b960a37fa25f47909ca5dc2ba58311f1748a7b9e791abcd17a
|
||||
|
||||
@@ -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<ClientId, BTreeSet<ResourceId>>,
|
||||
) {
|
||||
for (client, resources) in authorizations {
|
||||
let Some(client) = self.peers.get_mut(&client) else {
|
||||
continue;
|
||||
};
|
||||
|
||||
client.retain_authorizations(resources);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn handle_p2p_control_packet(
|
||||
|
||||
@@ -122,6 +122,8 @@ pub struct InitGateway {
|
||||
pub relays: Vec<Relay>,
|
||||
#[serde(default)]
|
||||
pub account_slug: Option<String>,
|
||||
#[serde(default)]
|
||||
pub authorizations: Vec<Authorization>,
|
||||
}
|
||||
|
||||
#[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,
|
||||
|
||||
@@ -233,6 +233,17 @@ impl ClientOnGateway {
|
||||
self.recalculate_filters();
|
||||
}
|
||||
|
||||
pub(crate) fn retain_authorizations(&mut self, authorization: BTreeSet<ResourceId>) {
|
||||
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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
@@ -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<RelayId, Host<u64>>),
|
||||
|
||||
/// De-authorize access to a resource whilst the Gateway is network-partitioned from the portal.
|
||||
DeauthorizeWhileGatewayIsPartitioned(ResourceId),
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -38,6 +38,10 @@ export default function Gateway() {
|
||||
Fixes an issue where connections would sometimes take up to 90s to
|
||||
establish.
|
||||
</ChangeItem>
|
||||
<ChangeItem pull="9896">
|
||||
Fixes a security issue where resource authorizations would not get
|
||||
revoked if the Gateway was disconnected from the portal.
|
||||
</ChangeItem>
|
||||
</Unreleased>
|
||||
<Entry version="1.4.12" date={new Date("2025-06-30")}>
|
||||
<ChangeItem pull="9657">
|
||||
|
||||
Reference in New Issue
Block a user