fix(connlib): resource filter deserialization (#4910)

There was an error on how resource filters were deserialized in the
gateway:

* we always assumed that there would be the ports included but the
portal sends no port down when the "all" range is allowed
* also we didn't support the resource_updated message, this fixes it,
and resources allow-list can be changes in-flight
This commit is contained in:
Gabi
2024-05-07 21:16:06 -03:00
committed by GitHub
parent 0c7c96dd07
commit c46967e1d6
5 changed files with 103 additions and 0 deletions

View File

@@ -72,10 +72,22 @@ pub enum Filter {
pub struct PortRange {
// TODO: we can use a custom deserializer
// or maybe change the control plane to use start and end would suffice
#[serde(default = "max_port")]
pub port_range_end: u16,
#[serde(default = "min_port")]
pub port_range_start: u16,
}
// Note: these 2 functions are needed since serde doesn't yet support default_value
// see serde-rs/serde#368
fn min_port() -> u16 {
0
}
fn max_port() -> u16 {
u16::MAX
}
impl ResourceDescription<ResourceDescriptionDns> {
pub fn into_resolved(
self,
@@ -99,6 +111,22 @@ impl ResourceDescription<ResourceDescriptionDns> {
}
}
impl ResourceDescription<ResourceDescriptionDns> {
pub fn id(&self) -> ResourceId {
match self {
ResourceDescription::Dns(r) => r.id,
ResourceDescription::Cidr(r) => r.id,
}
}
pub fn filters(&self) -> Vec<Filter> {
match self {
ResourceDescription::Dns(r) => r.filters.clone(),
ResourceDescription::Cidr(r) => r.filters.clone(),
}
}
}
impl ResourceDescription<ResolvedResourceDescriptionDns> {
pub fn addresses(&self) -> Vec<IpNetwork> {
match self {
@@ -139,6 +167,19 @@ mod tests {
assert_eq!(expected_filter, actual_filter);
}
#[test]
fn can_deserialize_empty_udp_filter() {
let msg = r#"{ "protocol": "udp" }"#;
let expected_filter = Filter::Udp(PortRange {
port_range_start: 0,
port_range_end: u16::MAX,
});
let actual_filter = serde_json::from_str(msg).unwrap();
assert_eq!(expected_filter, actual_filter);
}
#[test]
fn can_deserialize_tcp_filter() {
let msg = r#"{ "protocol": "tcp", "port_range_start": 10, "port_range_end": 20 }"#;
@@ -152,6 +193,19 @@ mod tests {
assert_eq!(expected_filter, actual_filter);
}
#[test]
fn can_deserialize_empty_tcp_filter() {
let msg = r#"{ "protocol": "tcp" }"#;
let expected_filter = Filter::Tcp(PortRange {
port_range_start: 0,
port_range_end: u16::MAX,
});
let actual_filter = serde_json::from_str(msg).unwrap();
assert_eq!(expected_filter, actual_filter);
}
#[test]
fn can_deserialize_icmp_filter() {
let msg = r#"{ "protocol": "icmp" }"#;

View File

@@ -153,6 +153,12 @@ where
None
}
pub fn update_resource(&mut self, resource: ResourceDescription) {
for peer in self.role_state.peers.iter_mut() {
peer.update_resource(&resource);
}
}
#[tracing::instrument(level = "debug", skip_all, fields(%resource, %client))]
pub fn remove_access(&mut self, client: &ClientId, resource: &ResourceId) {
let Some(peer) = self.role_state.peers.get_mut(client) else {

View File

@@ -216,6 +216,19 @@ impl ClientOnGateway {
self.recalculate_filters();
}
// Note: we only allow updating filters and names
// but names updates have no effect on the gateway
pub(crate) fn update_resource(
&mut self,
resource: &connlib_shared::messages::gateway::ResourceDescription,
) {
let Some(old_resource) = self.resources.get_mut(&resource.id()) else {
return;
};
old_resource.filters = resource.filters();
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

View File

@@ -192,6 +192,12 @@ impl Eventloop {
} => {
// TODO: Handle `init` message during operation.
}
phoenix_channel::Event::InboundMessage {
msg: IngressMessages::ResourceUpdated(resource_description),
..
} => {
self.tunnel.update_resource(resource_description);
}
phoenix_channel::Event::ErrorResponse { topic, req_id, res } => {
tracing::warn!(%topic, %req_id, "Request failed: {res:?}");
}

View File

@@ -75,6 +75,7 @@ pub enum IngressMessages {
InvalidateIceCandidates(ClientIceCandidates),
Init(InitGateway),
RelaysPresence(RelaysPresence),
ResourceUpdated(ResourceDescription),
}
/// A client's ice candidate message.
@@ -115,6 +116,9 @@ pub struct ConnectionReady {
#[cfg(test)]
mod test {
use super::*;
use connlib_shared::messages::gateway::Filter;
use connlib_shared::messages::gateway::PortRange;
use connlib_shared::messages::gateway::ResourceDescriptionDns;
use connlib_shared::messages::Turn;
use phoenix_channel::InitMessage;
use phoenix_channel::PhoenixMessage;
@@ -331,6 +335,26 @@ mod test {
assert_eq!(m, ingress_message);
}
#[test]
fn resource_updated() {
let message = r#"{"event":"resource_updated","ref":null,"topic":"gateway","payload":{"id":"57f9ebbb-21d5-4f9f-bf86-b25122fc7a43","name":"?.httpbin","type":"dns","address":"?.httpbin","filters":[{"protocol":"icmp"},{"protocol":"tcp"}]}}"#;
let m =
IngressMessages::ResourceUpdated(ResourceDescription::Dns(ResourceDescriptionDns {
id: "57f9ebbb-21d5-4f9f-bf86-b25122fc7a43".parse().unwrap(),
address: "?.httpbin".to_string(),
name: "?.httpbin".to_string(),
filters: vec![
Filter::Icmp,
Filter::Tcp(PortRange {
port_range_end: 65535,
port_range_start: 0,
}),
],
}));
let ingress_message = serde_json::from_str::<IngressMessages>(message).unwrap();
assert_eq!(m, ingress_message);
}
#[test]
fn relays_presence() {
let message = r#"