refactor(connlib): remove concept of "allowed IPs" from Peer
#5077
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
I'd like to merge #4728 before we merge this. |
Terraform Cloud Plan Output |
In firezone, a wireguard tunnel between two nodes is always ephemeral. Thus, any packet coming out of a tunnel always comes from the same IP. There is no need to double check that the packets do indeed come from the TUN device of the other peer. If the remote has the correct WG key to send us packets, we should accept them. Checking the IP does not give us any additional security benefit.
thomaseizinger
force-pushed
the
chore/connlib/remove-allowed-ips
branch
from
07e1803
to
182f0fe
Compare
| let Some(peer) = self.peers.get_mut(&gateway_id) else { | ||
| continue; | ||
| }; | ||
|
|
||
| // First we remove the id from all allowed ips | ||
| for (network, resources) in peer | ||
| .allowed_ips | ||
| .iter_mut() | ||
| .filter(|(_, resources)| resources.contains(id)) | ||
| { | ||
| resources.remove(id); | ||
|
|
||
| if !resources.is_empty() { | ||
| continue; | ||
| } | ||
|
|
||
| // If the allowed_ips doesn't correspond to any resource anymore we | ||
| // clean up any related translation. | ||
| peer.translations.remove_by_left(&network.network_address()); | ||
| } | ||
|
|
||
| // We remove all empty allowed ips entry since there's no resource that corresponds to it | ||
| peer.allowed_ips.retain(|_, r| !r.is_empty()); | ||
|
|
||
| // If there's no allowed ip left we remove the whole peer because there's no point on keeping it around | ||
| if peer.allowed_ips.is_empty() { | ||
| self.peers.remove(&gateway_id); | ||
| self.update_site_status_by_gateway(&gateway_id, Status::Unknown); | ||
| // TODO: should we have a Node::remove_connection? | ||
| } |
There was a problem hiding this comment.
I think this is fine. I think what ends up happening here is that we have a connection to a gateway that we no longer need. We can clean up unused connections at a later point and I think it is probably a feature we should add to snownet and clean them up based on how often we see traffic going through even if we still have a resource mapping.
There was a problem hiding this comment.
If we keep track of gateways to resources ids like suggested here we can cleanup peers when no resource is around that corresponds to a gateway.
There was a problem hiding this comment.
Sounds good. I am planning to work on #5054 next.
Performance Test ResultsTCP
UDP
|
|
|
||
| // If the allowed_ips doesn't correspond to any resource anymore we | ||
| // clean up any related translation. | ||
| peer.translations.remove_by_left(&network.network_address()); |
| if self.allowed_ips.longest_match(packet.source()).is_none() { | ||
| return Err(connlib_shared::Error::UnallowedPacket(packet.source())); | ||
| } | ||
|
|
There was a problem hiding this comment.
@thomaseizinger I was just thinking about this, and removing this means that another user, could use your ip as a source to send packets to a resource.
This means they could send something like a RST packet to kill your conneciton.
For this reason it might be better to still have the allowed_ips
There was a problem hiding this comment.
We could however implement this as just 2 allowed_ipv4 and allowed_ipv6.
Which I might already use for #5049 to convert between ipv4 and ipv6 source ips
There was a problem hiding this comment.
I think since this is on the gateway it wouldn't affect #5080 too much?
There was a problem hiding this comment.
another user, could use your ip as a source to send packets to a resource.
Another user where? Is the packet coming out of the tunnel?
I'd agree that it is somewhat of a defense-in-depth thing. Technically, the source IP could be anything when coming out of the tunnel. But also, if somebody has access to the tunnel, they can also just mangle the IP to the one of the TUN device, no?
There was a problem hiding this comment.
Well, I mean this:
Say you have User A with public Key A and IP A and user B with public key B and IP B.
Without allowed_ips, user A could maliciously:
- Mangle their encapsulated packet to have IP B
- It's sent through the tunnel
- Packet is successfully decapuselated using public key A
- This associates the decapsulated packet to the conn-id of the corresponding public key A
- We get the peer based on the conn-id
- Now, w/o the
allowed_ipsthe packets just gets forwarded to the resource as long as they have access to it - Resource believes the packet comes from IP B (sans nat-table)(*)
(*) IP A could make the source port collide with whatever user B is using and interrupt their connection or something else
If we have the allowed_ips step 6 would prevent always that user A uses anything but their assigned source ips since this is ensured by the portal
There was a problem hiding this comment.
Makes sense! I'll add a test for this to ensure we don't break it! :)
In firezone, a wireguard tunnel between two nodes is always ephemeral. Thus, any packet coming out of a tunnel always comes from the same IP.
There is no need to double check that the packets do indeed come from the TUN device of the other peer. If the remote has the correct WG key to send us packets, we should accept them. Checking the IP does not give us any additional security benefit.