Skip to content

Commit

Permalink
fix(connlib): deterministically route packets in case of overlap (#5082)
Browse files Browse the repository at this point in the history 
Currently, we only consult the IP ranges of our configured resources for
the initial connection to a gateway. Once a connection is established,
packets are routed based on an IP range associated with that gateway.
This is inconsistent and actually causes problems in case the user
configures overlapping resources. In particular, adding a resource with
an overlapping but narrower IP network range to a client that is already
connected to a gateway with an overlapping but wider range will cause
all packets for the newly added resource to be routed to the already
connected gateway.

To fix this, we consult the IP network table of resources for each
packet to figure out, which resource is the most appropriate one. Then,
we pick the gateway that is configured for this resource. If we aren't
connected to that gateway or if we don't know about a gateway for this
resource, we emit a connection intent.

In case the portal wants to use an already connected gateway for that
resource, we handle that using the "reuse connection" message to the
portal.

In fixing this, I also realised that I think this has (positive) audit
consequences. In particular, this will now correctly report access to a
resource if it is overlapping as described above (i.e. a narrower
overlapping resource is added whilst being connected to one with a wider
range). I believe that previously, this access would have not been
reported because we would have simply routed the packet to the already
connected gateway.

Fixes: #5054.
  • Loading branch information
@thomaseizinger
thomaseizinger committed May 25, 2024
1 parent 9b085ea commit 97ae522
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 118 deletions.
2 changes: 2 additions & 0 deletions rust/connlib/tunnel/proptest-regressions/tests.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,5 @@ cc a0b8c281bd6af69051cbbd75b7b8b4536e34bc653c93cff023d7389b04a93ac1 # shrinks to
cc 1691bc65aeb05519f0b8260d401fd679a1d4d17e33b34b9fdd2b5b46e0b7085f # shrinks to (initial_state, transitions, seen_counter) = (ReferenceState { now: Instant { tv_sec: 28695, tv_nsec: 35917448 }, utc_now: 2024-05-21T01:16:24.705071359Z, client: SimNode { id: ClientId(370da223-692a-9056-a112-89ebbb2dce03), state: [110, 16, 21, 47, 10, 169, 122, 21, 171, 78, 228, 176, 10, 219, 13, 227, 33, 27, 218, 49, 19, 174, 93, 113, 127, 105, 160, 28, 196, 217, 219, 110], ip4_socket: None, ip6_socket: Some([::ffff:127.0.0.1]:32927), tunnel_ip4: 100.70.5.224, tunnel_ip6: ::ffff:143.211.81.183, span: Span { name: "client", level: Level(Error), target: "firezone_tunnel::tests", id: Id(57005), module_path: "firezone_tunnel::tests", line: 1054, file: "connlib/tunnel/src/tests.rs" } }, gateway: SimNode { id: GatewayId(432fc066-88ba-6d6b-c66f-edf484f0cedd), state: [27, 255, 104, 33, 198, 118, 202, 78, 135, 92, 163, 134, 244, 39, 41, 133, 96, 10, 106, 15, 100, 244, 123, 241, 244, 67, 4, 168, 199, 185, 138, 149], ip4_socket: Some(197.157.239.81:1428), ip6_socket: Some([4179:ad2c:abd:cbb4:3520:1b25:47d9:4b0c]:56849), tunnel_ip4: 100.91.72.11, tunnel_ip6: ::ffff:0.0.0.0, span: Span { name: "gateway", level: Level(Error), target: "firezone_tunnel::tests", id: Id(57005), module_path: "firezone_tunnel::tests", line: 1055, file: "connlib/tunnel/src/tests.rs" } }, relay: SimRelay { id: RelayId(533d43e8-650b-64d0-be61-f496af032fbe), state: 10959706197027164454, ip_stack: Dual { ip4: 182.38.26.219, ip6: fc1:f22f:37c6:61fc:45e3:a9ce:7b2e:ee90 }, allocations: {}, span: Span { name: "relay", level: Level(Error), target: "firezone_tunnel::tests", id: Id(57005), module_path: "firezone_tunnel::tests", line: 1037, file: "connlib/tunnel/src/tests.rs" }, .. }, client_cidr_resources: {}, connected_resources: {}, gateway_received_icmp_packets: [] }, [AddCidrResource(ResourceDescriptionCidr { id: ResourceId(00000000-0000-0000-0000-000000000000), address: V4(Ipv4Network { network_address: 0.0.0.0, netmask: 32 }), name: "aaaa", address_description: "sodhxc", sites: [Site { name: "zbcrxpj", id: SiteId(73d9c9bd-76fc-976b-3406-79978de91c27) }, Site { name: "ngcarw", id: SiteId(6dd336e2-4da4-9420-2830-030d4001f6f8) }, Site { name: "sokappy", id: SiteId(1c66a22d-4ec3-3af6-042b-8deffcc96a7a) }, Site { name: "ddeckw", id: SiteId(05877cc0-abce-5b88-4a8b-cec56677b75e) }, Site { name: "dgxjp", id: SiteId(49fcf247-0717-c475-9528-4239296f40e4) }, Site { name: "tgcvhldh", id: SiteId(f355b3f1-6985-d4d4-c1bf-5cc8f1e2d7c3) }] }), AddCidrResource(ResourceDescriptionCidr { id: ResourceId(f121092f-0c97-c814-dbfc-73a2705a777a), address: V4(Ipv4Network { network_address: 0.0.0.0, netmask: 32 }), name: "fqhkj", address_description: "fsgcu", sites: [Site { name: "shnps", id: SiteId(d03d3893-f788-bcb2-65d7-91dd13a6052e) }, Site { name: "ghhk", id: SiteId(b21516a7-b15e-48cb-0178-e768224a5d46) }, Site { name: "xwwk", id: SiteId(2b309495-0bb0-5dcb-4ba3-9697c19e4d94) }, Site { name: "lhgzqanaqc", id: SiteId(d903ce14-3485-2c2e-dba7-3d783fe5568e) }, Site { name: "dtalsidlh", id: SiteId(8dfcce4e-6496-3408-2e6e-02c91b64bf05) }, Site { name: "nzpurg", id: SiteId(d4382a12-0853-2a6a-e53f-9d15fff31b58) }, Site { name: "tjltskkgf", id: SiteId(c425b8b0-a4e9-5e45-4b1e-102200a02851) }, Site { name: "sbsvmcbktg", id: SiteId(9024bb4f-1edb-8cbd-ecc0-49f1df0cc0e2) }, Site { name: "ugvid", id: SiteId(d3bd6108-fafd-fe4a-4aa4-c8c068cbcbb8) }, Site { name: "hylnofbb", id: SiteId(6af88c22-637e-46ab-47b4-1b88657cf8b2) }] })], None)
cc 4855625684cfe850e8b965097038ac8fed979cd21abd5284eb25e48136fcbb3e
cc bbc56c960f18a6115b3c114f2c5280a813fa2a12f21af383ad4154863fb944bc
cc 71165314a457a9f39928d882ffcc2ae02aedc2a35ce55b387c990b1bc5022a38 # shrinks to (initial_state, transitions, seen_counter) = (ReferenceState { now: Instant { tv_sec: 18548, tv_nsec: 387490499 }, utc_now: 2024-05-22T05:00:47.375132041Z, client: SimNode { id: ClientId(a89380c1-5379-5a2b-c5da-47fdeb956f75), state: PrivateKey("d348d4c5fa2ad05dfc686f289fb7084a16a095e208000844bc0ef7566b25addb"), ip4_socket: Some(127.0.0.1:41241), ip6_socket: Some([6e67:9b39:4a3f:a896:9e02:dc56:353e:65b3]:58740), tunnel_ip4: 100.111.145.90, tunnel_ip6: ::ffff:63.59.65.126 }, gateway: SimNode { id: GatewayId(d680e99a-31a7-9d8b-1052-6b85edfe42bc), state: PrivateKey("d39211badb6305b5b583574ee303b6a112b66311c38b4c22d68d69cb8d917bae"), ip4_socket: Some(82.215.208.98:18290), ip6_socket: Some([f122:b9a6:b3fb:bf35:319d:f921:d911:d012]:57327), tunnel_ip4: 100.81.19.26, tunnel_ip6: 3bfa:312f:c388:91b9:f6ce:95a6:a07:8784 }, relay: SimRelay { id: RelayId(a908052e-8823-7892-b1e2-e2c64deab06a), ip_stack: Dual { ip4: 75.190.170.204, ip6: ::ffff:0.235.25.126 }, allocations: {} }, client_cidr_resources: {}, connected_resources: {}, gateway_received_icmp_packets: [] }, [AddCidrResource(ResourceDescriptionCidr { id: ResourceId(00000000-0000-0000-0000-000000000000), address: V4(Ipv4Network { network_address: 0.0.0.0, netmask: 28 }), name: "aaaa", address_description: "bmuf", sites: [Site { name: "zvkyrxzs", id: SiteId(67566938-ef2b-b442-64e0-f0f6fd05634d) }, Site { name: "hvzhiban", id: SiteId(4b7a5400-0fb2-2e42-444a-bc94fbaeed2e) }, Site { name: "fjmyjoh", id: SiteId(13340020-2717-880f-ba45-c823725c2234) }, Site { name: "fgrhxuvi", id: SiteId(8c260658-d7fb-c49c-c9f0-769cd8ced648) }] }), SendICMPPacketToIp4Resource { r_idx: Index(0) }, AddCidrResource(ResourceDescriptionCidr { id: ResourceId(00000000-0000-0000-0000-000000000001), address: V4(Ipv4Network { network_address: 0.0.0.0, netmask: 28 }), name: "aaaa", address_description: "clkriftb", sites: [Site { name: "fgfg", id: SiteId(6b8d5470-3636-4879-2a32-94790962e6fd) }] }), SendICMPPacketToIp4Resource { r_idx: Index(11791212112709260679) }], None)
cc c9b812a8526dad4b00e366ea6b6d60054a4dd64c58849f1244f6fb2ba9d3686b # shrinks to (initial_state, transitions, seen_counter) = (ReferenceState { now: Instant { tv_sec: 20170, tv_nsec: 301610515 }, utc_now: 2024-05-22T05:27:49.998877399Z, client: SimNode { id: ClientId(00000000-0000-0000-0000-000000000000), state: PrivateKey("0000000000000000000000000000000000000000000000000000000000000000"), ip4_socket: Some(127.0.0.1:1), ip6_socket: None, tunnel_ip4: 100.64.0.1, tunnel_ip6: ::ffff:0.0.0.0 }, gateway: SimNode { id: GatewayId(00000000-0000-0000-0000-000000000000), state: PrivateKey("0000000000000000000000000000000000000000000000000000000000000001"), ip4_socket: None, ip6_socket: Some([::ffff:0.0.0.0]:12607), tunnel_ip4: 100.66.167.161, tunnel_ip6: ::ffff:155.93.248.155 }, relay: SimRelay { id: RelayId(c4e3332a-6f75-3a59-3ed8-15fbf26de134), ip_stack: Dual { ip4: 115.115.97.13, ip6: ::ffff:196.17.152.128 }, allocations: {} }, client_cidr_resources: {}, connected_resources: {}, gateway_received_icmp_packets: [] }, [AddCidrResource(ResourceDescriptionCidr { id: ResourceId(00000000-0000-0000-0000-000000000000), address: V4(Ipv4Network { network_address: 127.0.0.1, netmask: 32 }), name: "aaaa", address_description: "mgnx", sites: [Site { name: "njmblcn", id: SiteId(b6f995cd-2c8e-a35d-117f-1e7affe3bc7b) }, Site { name: "angxwaw", id: SiteId(c1772dc0-60c6-ebfc-bcb5-978b288bbabf) }, Site { name: "vblitlg", id: SiteId(f41e2663-0254-e054-2353-0855dabea706) }, Site { name: "plcfnotvz", id: SiteId(72f3a8cc-8015-bf96-adcd-b40df5f25383) }, Site { name: "lflx", id: SiteId(214a47c8-5ce9-3e33-4738-da1a36d79d68) }, Site { name: "shfamjx", id: SiteId(1f2c00bf-3437-9ac9-c9d3-e66ebf3a1558) }, Site { name: "offgdto", id: SiteId(edb8c103-1dda-01f7-d648-47ccfad1eb97) }, Site { name: "gzss", id: SiteId(e2058be3-2fe7-5459-b8b3-eaee2c434828) }, Site { name: "ovougquk", id: SiteId(1410820a-7236-ac4d-e649-f276f3418e51) }] }), SendICMPPacketToIp4Resource { r_idx: Index(0) }, AddCidrResource(ResourceDescriptionCidr { id: ResourceId(00000000-0000-0000-0000-000000000001), address: V4(Ipv4Network { network_address: 127.0.0.1, netmask: 32 }), name: "aaaa", address_description: "llgt", sites: [Site { name: "jmcxcphun", id: SiteId(190023c3-f08f-0b3c-ca68-ac5d45127341) }, Site { name: "mxofshkeo", id: SiteId(c39f0553-11d6-ddb0-1bc2-57e841e038fc) }, Site { name: "kaasxisn", id: SiteId(7c30df2b-95f9-0a6c-ebc7-92fd0a131376) }, Site { name: "hhitinvik", id: SiteId(39c252f6-4848-e5b7-1fe3-a4b886258e9a) }] }), SendICMPPacketToIp4Resource { r_idx: Index(0) }, SendICMPPacketToRandomIp { dst: 127.0.0.1 }], None)
160 changes: 74 additions & 86 deletions rust/connlib/tunnel/src/client.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,9 +315,7 @@ pub struct ClientState {
buffered_dns_queries: VecDeque<DnsQuery<'static>>,

/// The (internal) IPs we have assigned for a certain DNS resource.
///
/// We assign one internal IP per externally resolved IP.
dns_resources_internal_ips: HashMap<DnsResource, HashSet<IpAddr>>,
dns_resources_internal_ips: HashMap<IpAddr, DnsResource>,
/// DNS queries we can only answer once we have connected to the resource.
///
/// See [`dns::ResolveStrategy`] for details.
Expand Down Expand Up @@ -417,6 +415,7 @@ impl ClientState {
self.node.public_key()
}

#[tracing::instrument(level = "trace", skip_all, fields(dest))]
pub(crate) fn encapsulate<'s>(
&'s mut self,
packet: MutableIpPacket<'_>,
Expand All @@ -430,19 +429,39 @@ impl ClientState {
Err(non_dns_packet) => non_dns_packet,
};

let Some(peer) = self.peers.peer_by_ip_mut(dest) else {
self.on_connection_intent_ip(dest, now);
tracing::Span::current().record("dest", tracing::field::display(dest));

if is_definitely_not_a_resource(dest) {
return None;
}

let Some(resource) = self.get_resource_by_destination(dest) else {
tracing::trace!("Unknown resource");
return None;
};

let Some(peer) = self.peer_by_resource_mut(resource) else {
// If the resource are intending to is a DNS resource (i.e. we resolved an IP for it), look up the original name.
let domain = self
.dns_resources_internal_ips
.values()
.find_map(|r| (r.id == resource).then_some(r.address.clone()));

self.on_connection_intent_to_resource(resource, domain, now);
return None;
};

let packet = peer.transform_tun_to_network(packet);
let gateway_id = peer.id();

let transmit = self
.node
.encapsulate(peer.id(), packet.as_immutable(), now)
.encapsulate(gateway_id, packet.as_immutable(), now)
.inspect_err(|e| tracing::debug!("Failed to encapsulate: {e}"))
.ok()??;

tracing::trace!("Encapsulated packet");

Some(transmit)
}

Expand Down Expand Up @@ -550,17 +569,8 @@ impl ClientState {
.get(&resource_id)
.ok_or(Error::UnknownResource)?;

let domain = self.get_awaiting_connection(&resource_id)?.domain.clone();

if self.is_connected_to(resource_id, &domain) {
return Err(Error::UnexpectedConnectionDetails);
}

let awaiting_connection = self
.awaiting_connection
.get(&resource_id)
.ok_or(Error::UnexpectedConnectionDetails)?
.clone();
let awaiting_connection = self.get_awaiting_connection(&resource_id)?.clone();
let domain = awaiting_connection.domain.clone();

self.resources_gateways.insert(resource_id, gateway_id);

Expand Down Expand Up @@ -601,7 +611,7 @@ impl ClientState {
username: offer.credentials.username,
password: offer.credentials.password,
},
domain: awaiting_connection.domain,
domain,
},
}));
}
Expand Down Expand Up @@ -656,8 +666,10 @@ impl ClientState {
})
.collect();

self.dns_resources_internal_ips
.insert(resource_description.clone(), addrs.clone());
for addr in addrs.clone() {
self.dns_resources_internal_ips
.insert(addr, resource_description.clone());
}

send_dns_answer(self, Rtype::AAAA, &resource_description, &addrs);
send_dns_answer(self, Rtype::A, &resource_description, &addrs);
Expand Down Expand Up @@ -700,10 +712,14 @@ impl ClientState {

Ok(None)
}
Some(dns::ResolveStrategy::DeferredResponse(resource)) => {
self.on_connection_intent_dns(&resource.0, now);
Some(dns::ResolveStrategy::DeferredResponse((resource, r_type))) => {
self.on_connection_intent_to_resource(
resource.id,
Some(resource.address.clone()),
now,
);
self.deferred_dns_queries
.insert(resource, packet.as_immutable().to_owned());
.insert((resource, r_type), packet.as_immutable().to_owned());

Ok(None)
}
Expand All @@ -728,37 +744,7 @@ impl ClientState {
self.resources_gateways.remove(&resource);
}

#[tracing::instrument(level = "debug", skip_all, fields(resource_address = %resource.address, resource_id = %resource.id))]
fn on_connection_intent_dns(&mut self, resource: &DnsResource, now: Instant) {
self.on_connection_intent_to_resource(resource.id, Some(resource.address.clone()), now)
}

#[tracing::instrument(level = "debug", skip_all, fields(resource_ip = %destination, resource_id))]
fn on_connection_intent_ip(&mut self, destination: IpAddr, now: Instant) {
if is_definitely_not_a_resource(destination) {
return;
}

let Some(resource_id) = self.get_cidr_resource_by_destination(destination) else {
if let Some(resource) = self
.dns_resources_internal_ips
.iter()
.find_map(|(r, i)| i.contains(&destination).then_some(r))
.cloned()
{
self.on_connection_intent_dns(&resource, now);
}

tracing::trace!("Unknown resource");

return;
};

tracing::Span::current().record("resource_id", tracing::field::display(&resource_id));

self.on_connection_intent_to_resource(resource_id, None, now)
}

#[tracing::instrument(level = "debug", skip_all, fields(%resource, ?domain))]
fn on_connection_intent_to_resource(
&mut self,
resource: ResourceId,
Expand Down Expand Up @@ -818,15 +804,6 @@ impl ClientState {
self.dns_mapping.clone()
}

fn is_connected_to(&self, resource: ResourceId, domain: &Option<DomainName>) -> bool {
let Some(resource) = self.resource_ids.get(&resource) else {
return false;
};

let ips = self.get_resource_ip(resource, domain);
ips.iter().any(|ip| self.peers.exact_match(*ip).is_some())
}

fn get_resource_ip(
&self,
resource: &ResourceDescription,
Expand All @@ -839,11 +816,7 @@ impl ClientState {
};

let description = DnsResource::from_description(dns_resource, domain.clone());
self.dns_resources_internal_ips
.get(&description)
.cloned()
.unwrap_or_default()
.into_iter()
dns::ips_of_resource(&self.dns_resources_internal_ips, &description)
.map(Into::into)
.collect()
}
Expand All @@ -854,7 +827,7 @@ impl ClientState {
pub fn cleanup_connected_gateway(&mut self, gateway_id: &GatewayId) {
self.update_site_status_by_gateway(gateway_id, Status::Unknown);
self.peers.remove(gateway_id);
self.dns_resources_internal_ips.retain(|resource, _| {
self.dns_resources_internal_ips.retain(|_, resource| {
!self
.resources_gateways
.get(&resource.id)
Expand All @@ -871,10 +844,18 @@ impl ClientState {
.chain(self.dns_mapping.left_values().copied().map(Into::into))
}

fn get_cidr_resource_by_destination(&self, destination: IpAddr) -> Option<ResourceId> {
self.cidr_resources
fn get_resource_by_destination(&self, destination: IpAddr) -> Option<ResourceId> {
let maybe_cidr_resource_id = self
.cidr_resources
.longest_match(destination)
.map(|(_, res)| res.id)
.map(|(_, r)| r.id);

let maybe_dns_resource_id = self
.dns_resources_internal_ips
.get(&destination)
.map(|r| r.id);

maybe_cidr_resource_id.or(maybe_dns_resource_id)
}

#[must_use]
Expand Down Expand Up @@ -912,18 +893,15 @@ impl ClientState {

self.peers.iter_mut().for_each(|p| p.expire_dns_track());

for resource in self.dns_resources_internal_ips.keys() {
let Some(gateway_id) = self.resources_gateways.get(&resource.id) else {
for resource in self.dns_resources_internal_ips.values() {
let Some(peer) = self.peer_by_resource(resource.id) else {
// filter inactive connections
continue;
};
// filter inactive connections
if self.peers.get(gateway_id).is_none() {
continue;
}

connections.push(ReuseConnection {
resource_id: resource.id,
gateway_id: *gateway_id,
gateway_id: peer.id(),
payload: Some(resource.address.clone()),
});
}
Expand Down Expand Up @@ -1054,21 +1032,17 @@ impl ClientState {
fn remove_resources(&mut self, ids: &[ResourceId]) {
for id in ids {
self.awaiting_connection.remove(id);
self.dns_resources_internal_ips.retain(|r, _| r.id != *id);
self.dns_resources_internal_ips.retain(|_, r| r.id != *id);
self.dns_resources.retain(|_, r| r.id != *id);
self.cidr_resources.retain(|_, r| r.id != *id);
self.deferred_dns_queries.retain(|(r, _), _| r.id != *id);

self.resource_ids.remove(id);

let Some(gateway_id) = self.resources_gateways.remove(id) else {
tracing::debug!("No gateway associated with resource");
continue;
};

let Some(peer) = self.peers.get_mut(&gateway_id) else {
let Some(peer) = self.peer_by_resource_mut(*id) else {
continue;
};
let gateway_id = peer.id();

// First we remove the id from all allowed ips
for (network, resources) in peer
Expand Down Expand Up @@ -1146,6 +1120,20 @@ impl ClientState {
) {
self.node.update_relays(to_remove, &to_add, now);
}

fn peer_by_resource(&self, resource: ResourceId) -> Option<&GatewayOnClient> {
let gateway_id = self.resources_gateways.get(&resource)?;
let peer = self.peers.get(gateway_id)?;

Some(peer)
}

fn peer_by_resource_mut(&mut self, resource: ResourceId) -> Option<&mut GatewayOnClient> {
let gateway_id = self.resources_gateways.get(&resource)?;
let peer = self.peers.get_mut(gateway_id)?;

Some(peer)
}
}

fn effective_dns_servers(
Expand Down

0 comments on commit 97ae522

Please sign in to comment.