From 74455244a2faf621b9baae1f45ea2564ffd7e9a8 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 22 May 2024 14:49:01 +1000 Subject: [PATCH 1/9] Check for definite non-resources before looking up peer --- rust/connlib/tunnel/src/client.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 1aa262465b0..3f35854b716 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -406,6 +406,10 @@ impl ClientState { Err(non_dns_packet) => non_dns_packet, }; + if is_definitely_not_a_resource(dest) { + return None; + } + let Some(peer) = self.peers.peer_by_ip_mut(dest) else { self.on_connection_intent_ip(dest, now); return None; @@ -711,10 +715,6 @@ impl ClientState { #[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 From 11955e0a0675f2f9b982befb4b945c69e55cb4be Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 22 May 2024 15:04:40 +1000 Subject: [PATCH 2/9] De-duplicate code --- rust/connlib/tunnel/src/client.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 3f35854b716..5dd7008585c 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -530,18 +530,13 @@ impl ClientState { .get(&resource_id) .ok_or(Error::UnknownResource)?; - let domain = self.get_awaiting_connection(&resource_id)?.domain.clone(); + let awaiting_connection = self.get_awaiting_connection(&resource_id)?.clone(); + let domain = awaiting_connection.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(); - self.resources_gateways.insert(resource_id, gateway_id); if self.peers.get(&gateway_id).is_some() { @@ -581,7 +576,7 @@ impl ClientState { username: offer.credentials.username, password: offer.credentials.password, }, - domain: awaiting_connection.domain, + domain, }, })); } From 2905ae71dfc544c66300d63ec4cf3707e7bc6868 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 22 May 2024 15:46:19 +1000 Subject: [PATCH 3/9] Always resolve gateway to use by checking longest_match of resources --- .../tunnel/proptest-regressions/tests.txt | 2 + rust/connlib/tunnel/src/client.rs | 74 +++++++++---------- rust/connlib/tunnel/src/peer_store.rs | 5 -- rust/connlib/tunnel/src/tests.rs | 23 +++--- 4 files changed, 49 insertions(+), 55 deletions(-) diff --git a/rust/connlib/tunnel/proptest-regressions/tests.txt b/rust/connlib/tunnel/proptest-regressions/tests.txt index f0b0cc82a63..eef37009882 100644 --- a/rust/connlib/tunnel/proptest-regressions/tests.txt +++ b/rust/connlib/tunnel/proptest-regressions/tests.txt @@ -14,3 +14,5 @@ cc 594acd79e77265b4880bdeae8a71dd5dd7ce66bbaed98fd897a6c5a455b7ed34 # shrinks to cc a0b8c281bd6af69051cbbd75b7b8b4536e34bc653c93cff023d7389b04a93ac1 # shrinks to (initial_state, transitions, seen_counter) = (ReferenceState { now: Instant { tv_sec: 14405, tv_nsec: 697936838 }, utc_now: 2024-05-20T09:38:40.366949661Z, client: SimNode { id: ClientId(984edfe0-1826-f6b3-9983-e9bba6378841), state: [96, 21, 99, 67, 204, 141, 84, 72, 60, 163, 67, 148, 201, 195, 22, 10, 95, 210, 80, 228, 96, 231, 169, 137, 154, 124, 186, 141, 205, 121, 93, 175], ip4_socket: Some(84.172.107.49:21582), ip6_socket: Some([::ffff:127.0.0.1]:29971), tunnel_ip4: 100.85.112.154, tunnel_ip6: ::ffff:160.37.183.226, span: Span { name: "client", level: Level(Error), target: "firezone_tunnel::tests", id: Id(57005), module_path: "firezone_tunnel::tests", line: 1058, file: "connlib/tunnel/src/tests.rs" } }, gateway: SimNode { id: GatewayId(805a51c3-9f7c-eaf9-7436-4d4094284578), state: [156, 166, 54, 146, 85, 172, 55, 22, 40, 24, 6, 65, 215, 181, 201, 187, 254, 23, 152, 87, 190, 48, 161, 219, 23, 140, 49, 21, 244, 109, 215, 228], ip4_socket: Some(77.225.216.121:43902), ip6_socket: Some([::ffff:0.0.0.0]:25178), tunnel_ip4: 100.93.155.193, 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: 1059, file: "connlib/tunnel/src/tests.rs" } }, relay: SimRelay { id: RelayId(1969418e-79e6-404a-9c74-29739b5b4574), state: 5250556452848747625, ip_stack: Dual { ip4: 117.157.182.129, ip6: ::ffff:158.74.84.2 }, allocations: {}, span: Span { name: "relay", level: Level(Error), target: "firezone_tunnel::tests", id: Id(57005), module_path: "firezone_tunnel::tests", line: 1041, 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: 28 }), name: "aaaa", address_description: "csepvwjvk", sites: [Site { name: "zogadakmxj", id: SiteId(d60e6868-f02e-0855-6dfa-5b5ebd5474fc) }, Site { name: "quiehtqsju", id: SiteId(9f0c22c7-b706-671b-cc92-2e5bbe01bee1) }] }), SendICMPPacketToIp4Resource { r_idx: Index(0) }, AddCidrResource(ResourceDescriptionCidr { id: ResourceId(00000000-0000-0000-0000-000001de412b), address: V6(Ipv6Network { network_address: 1dfe:a1e3:4239:b5d8:aabd:3c99:84b3:8133, netmask: 128 }), name: "ntgsmxjm", address_description: "civwpi", sites: [Site { name: "rvodytzql", id: SiteId(5be9cb5b-dd10-83f3-a91a-903e1eba57ef) }, Site { name: "eycc", id: SiteId(21527f8b-638c-d783-0ac0-270f489b7283) }, Site { name: "sraepwbiet", id: SiteId(f1256423-a894-5989-1f87-ee6fe7bfb60a) }, Site { name: "pjgqsffcm", id: SiteId(28be9cb9-2ebe-c4e5-404c-95dea09b62ac) }, Site { name: "ujwjxqmw", id: SiteId(d5a21c28-b757-8fe5-f374-7420aea6c7c1) }, Site { name: "skvlsi", id: SiteId(add3e62f-b0c3-fe6e-8e29-bea0fdf0c3ae) }] }), AddCidrResource(ResourceDescriptionCidr { id: ResourceId(defbe2b2-9187-e86f-3321-1be4797fe0fc), address: V4(Ipv4Network { network_address: 0.0.0.0, netmask: 28 }), name: "ynwxtcxkjs", address_description: "tqzdgrr", sites: [Site { name: "iypfpa", id: SiteId(38a0acb4-a879-bbc5-3fd4-dfbed77fe751) }, Site { name: "zntcw", id: SiteId(b82239f6-a96f-9f64-7377-f051b7ae6d08) }, Site { name: "zwicdbyrk", id: SiteId(94b6c779-98d9-3164-c053-864c29deb712) }, Site { name: "zfeomu", id: SiteId(4118685c-1d40-5487-2d62-bd801c9f3606) }, Site { name: "lkonsw", id: SiteId(4ecad64c-5a13-6336-bd4f-8fa82aef5b45) }, Site { name: "riavejmv", id: SiteId(3704d328-dafb-9265-8bd9-bb4338c8226e) }] }), SendICMPPacketToIp4Resource { r_idx: Index(7596995058529867034) }], None) 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 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) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 5dd7008585c..ad6dc90941b 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -393,6 +393,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<'_>, @@ -406,12 +407,35 @@ impl ClientState { Err(non_dns_packet) => non_dns_packet, }; + tracing::Span::current().record("dest", tracing::field::display(dest)); + if is_definitely_not_a_resource(dest) { return None; } - let Some(peer) = self.peers.peer_by_ip_mut(dest) else { - self.on_connection_intent_ip(dest, now); + let Some(resource) = self.get_cidr_resource_by_destination(dest) else { + if let Some(resource) = self + .dns_resources_internal_ips + .iter() + .find_map(|(r, i)| i.contains(&dest).then_some(r)) + .cloned() + { + self.on_connection_intent_dns(&resource, now); + return None; + } + + tracing::trace!("Unknown resource"); + + return None; + }; + + let Some(gateway) = self.resources_gateways.get(&resource.id) else { + self.on_connection_intent_cidr(&resource, now); + return None; + }; + + let Some(peer) = self.peers.get_mut(gateway) else { + self.on_connection_intent_cidr(&resource, now); return None; }; @@ -423,6 +447,8 @@ impl ClientState { .inspect_err(|e| tracing::debug!("Failed to encapsulate: {e}")) .ok()??; + tracing::trace!("Encapsulated packet"); + Some(transmit) } @@ -533,10 +559,6 @@ impl ClientState { let awaiting_connection = self.get_awaiting_connection(&resource_id)?.clone(); let domain = awaiting_connection.domain.clone(); - if self.is_connected_to(resource_id, &domain) { - return Err(Error::UnexpectedConnectionDetails); - } - self.resources_gateways.insert(resource_id, gateway_id); if self.peers.get(&gateway_id).is_some() { @@ -708,26 +730,9 @@ impl ClientState { 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) { - 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_ip = %resource.address, resource_id = %resource.id))] + fn on_connection_intent_cidr(&mut self, resource: &ResourceDescriptionCidr, now: Instant) { + self.on_connection_intent_to_resource(resource.id, None, now) } fn on_connection_intent_to_resource( @@ -789,15 +794,6 @@ impl ClientState { self.dns_mapping.clone() } - fn is_connected_to(&self, resource: ResourceId, domain: &Option) -> 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, @@ -842,10 +838,14 @@ impl ClientState { .chain(self.dns_mapping.left_values().copied().map(Into::into)) } - fn get_cidr_resource_by_destination(&self, destination: IpAddr) -> Option { + fn get_cidr_resource_by_destination( + &self, + destination: IpAddr, + ) -> Option { self.cidr_resources .longest_match(destination) - .map(|(_, res)| res.id) + .map(|(_, r)| r) + .cloned() } #[must_use] diff --git a/rust/connlib/tunnel/src/peer_store.rs b/rust/connlib/tunnel/src/peer_store.rs index 1165dc6fc4b..058b3ddc58c 100644 --- a/rust/connlib/tunnel/src/peer_store.rs +++ b/rust/connlib/tunnel/src/peer_store.rs @@ -72,11 +72,6 @@ where self.peer_by_id.remove(id) } - pub(crate) fn exact_match(&self, ip: IpNetwork) -> Option<&P> { - let ip = self.id_by_ip.exact_match(ip)?; - self.peer_by_id.get(ip) - } - pub(crate) fn get(&self, id: &TId) -> Option<&P> { self.peer_by_id.get(id) } diff --git a/rust/connlib/tunnel/src/tests.rs b/rust/connlib/tunnel/src/tests.rs index 1d92d6c7400..0893d042c45 100644 --- a/rust/connlib/tunnel/src/tests.rs +++ b/rust/connlib/tunnel/src/tests.rs @@ -79,8 +79,8 @@ struct ReferenceState { /// Which resources the clients is aware of. client_cidr_resources: IpNetworkTable, - /// The IP ranges we are connected to. - connected_resources: IpNetworkTable<()>, + /// The resources we have connected to. + connected_resources: HashSet, gateway_received_icmp_packets: VecDeque<(Instant, IpAddr, IpAddr)>, } @@ -733,25 +733,22 @@ impl ReferenceState { tracing::Span::current().record("dst", tracing::field::display(dst)); - // First, check if we are connected to this IP range. - // This is rather odd and waiting to be fixed in https://github.com/firezone/firezone/issues/5054. - if self.connected_resources.longest_match(dst).is_some() { - tracing::debug!("Connected to resource, expecting packet to be routed to gateway"); - - self.gateway_received_icmp_packets - .push_back((self.now, src, dst)); - return; - } - // Second, if we are not yet connected, check if we have a resource for this IP. let Some((_, resource)) = self.client_cidr_resources.longest_match(dst) else { tracing::debug!("No resource corresponds to IP"); return; }; + if self.connected_resources.contains(&resource.id) { + tracing::debug!("Connected to resource, expecting packet to be routed"); + self.gateway_received_icmp_packets + .push_back((self.now, src, dst)); + return; + } + // If we have a resource, the first packet will initiate a connection to the gateway. tracing::debug!("Not connected to resource, expecting to trigger connection intent"); - self.connected_resources.insert(resource.address, ()); + self.connected_resources.insert(resource.id); } /// Samples an [`Ipv4Addr`] from _any_ of our IPv4 CIDR resources. From b81f95ec110bc474db5c38750aeb6bf14c3d0b15 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 22 May 2024 16:35:49 +1000 Subject: [PATCH 4/9] Generalize connection intents --- rust/connlib/tunnel/src/client.rs | 65 ++++++++++++------------------- 1 file changed, 25 insertions(+), 40 deletions(-) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index ad6dc90941b..9610ee07500 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -413,29 +413,19 @@ impl ClientState { return None; } - let Some(resource) = self.get_cidr_resource_by_destination(dest) else { - if let Some(resource) = self - .dns_resources_internal_ips - .iter() - .find_map(|(r, i)| i.contains(&dest).then_some(r)) - .cloned() - { - self.on_connection_intent_dns(&resource, now); - return None; - } - + let Some(resource) = self.get_resource_by_destination(dest) else { tracing::trace!("Unknown resource"); return None; }; - let Some(gateway) = self.resources_gateways.get(&resource.id) else { - self.on_connection_intent_cidr(&resource, now); + let Some(gateway) = self.resources_gateways.get(&resource) else { + self.on_connection_intent_to_resource(resource, now); return None; }; let Some(peer) = self.peers.get_mut(gateway) else { - self.on_connection_intent_cidr(&resource, now); + self.on_connection_intent_to_resource(resource, now); return None; }; @@ -697,10 +687,10 @@ 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, now); self.deferred_dns_queries - .insert(resource, packet.as_immutable().to_owned()); + .insert((resource, r_type), packet.as_immutable().to_owned()); Ok(None) } @@ -725,22 +715,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 = %resource.address, resource_id = %resource.id))] - fn on_connection_intent_cidr(&mut self, resource: &ResourceDescriptionCidr, now: Instant) { - self.on_connection_intent_to_resource(resource.id, None, now) - } - - fn on_connection_intent_to_resource( - &mut self, - resource: ResourceId, - domain: Option, - now: Instant, - ) { + fn on_connection_intent_to_resource(&mut self, resource: ResourceId, now: Instant) { debug_assert!(self.resource_ids.contains_key(&resource)); let gateways = self @@ -762,6 +737,12 @@ impl ClientState { occupied.get_mut().last_intent_sent_at = now; } Entry::Vacant(vacant) => { + // 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 + .iter() + .find_map(|(r, _)| (r.id == resource).then_some(r.address.clone())); + vacant.insert(AwaitingConnectionDetails { domain, gateways: gateways.clone(), @@ -838,14 +819,18 @@ impl ClientState { .chain(self.dns_mapping.left_values().copied().map(Into::into)) } - fn get_cidr_resource_by_destination( - &self, - destination: IpAddr, - ) -> Option { - self.cidr_resources + fn get_resource_by_destination(&self, destination: IpAddr) -> Option { + let maybe_cidr_resource_id = self + .cidr_resources .longest_match(destination) - .map(|(_, r)| r) - .cloned() + .map(|(_, r)| r.id); + + let maybe_dns_resource_id = self + .dns_resources_internal_ips + .iter() + .find_map(|(r, i)| i.contains(&destination).then_some(r.id)); + + maybe_cidr_resource_id.or(maybe_dns_resource_id) } #[must_use] From a83b80e1d19d75092cd0b58c7ab310346ff4a89a Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 22 May 2024 16:40:27 +1000 Subject: [PATCH 5/9] Remove stray newline --- rust/connlib/tunnel/src/client.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 9610ee07500..d10a1f2efb9 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -415,7 +415,6 @@ impl ClientState { let Some(resource) = self.get_resource_by_destination(dest) else { tracing::trace!("Unknown resource"); - return None; }; From a06de193298a000db1c4360a92b126b45e09b0cb Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 22 May 2024 17:16:15 +1000 Subject: [PATCH 6/9] Pass on domain from deferred DNS response --- rust/connlib/tunnel/src/client.rs | 39 +++++++++++++++++++------------ 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index d10a1f2efb9..a0f13d9e734 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -418,13 +418,18 @@ impl ClientState { return None; }; - let Some(gateway) = self.resources_gateways.get(&resource) else { - self.on_connection_intent_to_resource(resource, now); - return None; - }; - - let Some(peer) = self.peers.get_mut(gateway) else { - self.on_connection_intent_to_resource(resource, now); + let Some(peer) = self + .resources_gateways + .get(&resource) + .and_then(|g: &GatewayId| self.peers.get_mut(g)) + 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 + .iter() + .find_map(|(r, _)| (r.id == resource).then_some(r.address.clone())); + + self.on_connection_intent_to_resource(resource, domain, now); return None; }; @@ -687,7 +692,11 @@ impl ClientState { Ok(None) } Some(dns::ResolveStrategy::DeferredResponse((resource, r_type))) => { - self.on_connection_intent_to_resource(resource.id, now); + self.on_connection_intent_to_resource( + resource.id, + Some(resource.address.clone()), + now, + ); self.deferred_dns_queries .insert((resource, r_type), packet.as_immutable().to_owned()); @@ -714,7 +723,13 @@ impl ClientState { self.resources_gateways.remove(&resource); } - fn on_connection_intent_to_resource(&mut self, resource: ResourceId, now: Instant) { + #[tracing::instrument(level = "debug", skip_all, fields(%resource, ?domain))] + fn on_connection_intent_to_resource( + &mut self, + resource: ResourceId, + domain: Option, + now: Instant, + ) { debug_assert!(self.resource_ids.contains_key(&resource)); let gateways = self @@ -736,12 +751,6 @@ impl ClientState { occupied.get_mut().last_intent_sent_at = now; } Entry::Vacant(vacant) => { - // 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 - .iter() - .find_map(|(r, _)| (r.id == resource).then_some(r.address.clone())); - vacant.insert(AwaitingConnectionDetails { domain, gateways: gateways.clone(), From 65d52a1594d9e9bd2b0ae147717e675c38c9eefe Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 22 May 2024 17:22:50 +1000 Subject: [PATCH 7/9] Reduce code duplication and simplify call sites --- rust/connlib/tunnel/src/client.rs | 40 +++++++++++++++++-------------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index a0f13d9e734..b4c53d99990 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -418,11 +418,7 @@ impl ClientState { return None; }; - let Some(peer) = self - .resources_gateways - .get(&resource) - .and_then(|g: &GatewayId| self.peers.get_mut(g)) - else { + 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 @@ -434,10 +430,11 @@ impl ClientState { }; 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()??; @@ -877,17 +874,14 @@ 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 { + 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()), }); } @@ -1025,14 +1019,10 @@ impl ClientState { 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 @@ -1101,6 +1091,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( From ad4df1c01bc10938ad379c7c48609474b411727a Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 24 May 2024 10:53:33 +1000 Subject: [PATCH 8/9] Fix compile error --- rust/connlib/tunnel/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/connlib/tunnel/src/tests.rs b/rust/connlib/tunnel/src/tests.rs index dee5b3de4b5..932c5f53164 100644 --- a/rust/connlib/tunnel/src/tests.rs +++ b/rust/connlib/tunnel/src/tests.rs @@ -812,7 +812,7 @@ impl ReferenceState { if self.connected_resources.contains(&resource.id) { tracing::debug!("Connected to resource, expecting packet to be routed"); - self.gateway_received_icmp_packets + self.gateway_received_icmp_requests .push_back((self.now, src, dst)); self.client_received_icmp_replies .push_back((self.now, dst, src)); From ba25239f276eec3e6a3539ec4b197a8950a95652 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 24 May 2024 15:45:41 +1000 Subject: [PATCH 9/9] Optimise mapping of assigned IPs to go from IP to `DnsResource` --- rust/connlib/tunnel/src/client.rs | 30 ++++++++++------------- rust/connlib/tunnel/src/dns.rs | 40 ++++++++++++++++++++----------- 2 files changed, 39 insertions(+), 31 deletions(-) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index ab3f937fb5d..859a7d49d9b 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -315,9 +315,7 @@ pub struct ClientState { buffered_dns_queries: VecDeque>, /// 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>, + dns_resources_internal_ips: HashMap, /// DNS queries we can only answer once we have connected to the resource. /// /// See [`dns::ResolveStrategy`] for details. @@ -446,8 +444,8 @@ impl ClientState { // 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 - .iter() - .find_map(|(r, _)| (r.id == resource).then_some(r.address.clone())); + .values() + .find_map(|r| (r.id == resource).then_some(r.address.clone())); self.on_connection_intent_to_resource(resource, domain, now); return None; @@ -668,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); @@ -816,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() } @@ -831,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) @@ -856,8 +852,8 @@ impl ClientState { let maybe_dns_resource_id = self .dns_resources_internal_ips - .iter() - .find_map(|(r, i)| i.contains(&destination).then_some(r.id)); + .get(&destination) + .map(|r| r.id); maybe_cidr_resource_id.or(maybe_dns_resource_id) } @@ -897,7 +893,7 @@ impl ClientState { self.peers.iter_mut().for_each(|p| p.expire_dns_track()); - for resource in self.dns_resources_internal_ips.keys() { + for resource in self.dns_resources_internal_ips.values() { let Some(peer) = self.peer_by_resource(resource.id) else { // filter inactive connections continue; @@ -1036,7 +1032,7 @@ 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); diff --git a/rust/connlib/tunnel/src/dns.rs b/rust/connlib/tunnel/src/dns.rs index 637ba320831..1e6ff5eed8f 100644 --- a/rust/connlib/tunnel/src/dns.rs +++ b/rust/connlib/tunnel/src/dns.rs @@ -99,7 +99,7 @@ impl ResolveStrategy { /// - Otherwise, a strategy for responding to the query pub(crate) fn parse<'a>( dns_resources: &HashMap, - dns_resources_internal_ips: &HashMap>, + dns_resources_internal_ips: &HashMap, dns_mapping: &bimap::BiMap, packet: IpPacket<'a>, ) -> Option, DnsQuery<'a>, (DnsResource, Rtype)>> { @@ -363,7 +363,7 @@ fn get_description( /// If we are not connected yet, the Client should defer the response and begin connecting. fn resource_from_question( dns_resources: &HashMap, - dns_resources_internal_ips: &HashMap>, + dns_resources_internal_ips: &HashMap, question: &Question, ) -> Option, DnsQueryParams, DnsResource>> { let name = ToName::to_vec(question.qname()); @@ -377,12 +377,16 @@ fn resource_from_question( }; let description = DnsResource::from_description(&description, name); - let Some(ips) = dns_resources_internal_ips.get(&description) else { + + if !dns_resources_internal_ips + .iter() + .any(|(_, r)| r == &description) + { return Some(ResolveStrategy::DeferredResponse(description)); - }; + } + Some(ResolveStrategy::LocalResponse(RecordData::A( - ips.iter() - .cloned() + ips_of_resource(dns_resources_internal_ips, &description) .filter_map(get_v4) .map(domain::rdata::A::new) .collect(), @@ -393,13 +397,16 @@ fn resource_from_question( return Some(ResolveStrategy::forward(name.to_string(), qtype)); }; let description = DnsResource::from_description(&description, name); - let Some(ips) = dns_resources_internal_ips.get(&description) else { + + if !dns_resources_internal_ips + .iter() + .any(|(_, r)| r == &description) + { return Some(ResolveStrategy::DeferredResponse(description)); - }; + } Some(ResolveStrategy::LocalResponse(RecordData::Aaaa( - ips.iter() - .cloned() + ips_of_resource(dns_resources_internal_ips, &description) .filter_map(get_v6) .map(domain::rdata::Aaaa::new) .collect(), @@ -409,10 +416,7 @@ fn resource_from_question( let Some(ip) = reverse_dns_addr(&name.to_string()) else { return Some(ResolveStrategy::forward(name.to_string(), qtype)); }; - let Some(resource) = dns_resources_internal_ips - .iter() - .find_map(|(r, ips)| ips.contains(&ip).then_some(r)) - else { + let Some(resource) = dns_resources_internal_ips.get(&ip) else { return Some(ResolveStrategy::forward(name.to_string(), qtype)); }; Some(ResolveStrategy::LocalResponse(RecordData::Ptr( @@ -429,6 +433,14 @@ fn resource_from_question( } } +pub(crate) fn ips_of_resource<'a>( + ips: &'a HashMap, + resource: &'a DnsResource, +) -> impl Iterator + 'a { + ips.iter() + .filter_map(move |(ip, r)| (r == resource).then_some(*ip)) +} + pub(crate) fn as_dns_message(pkt: &IpPacket) -> Option { let datagram = pkt.as_udp()?; TrustDnsMessage::from_vec(datagram.payload()).ok()