Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

connlib: packets are routed based on connected gateways and not resources #5054

Closed
Tracked by #5036
thomaseizinger opened this issue May 21, 2024 · 3 comments · Fixed by #5082
Closed
Tracked by #5036

connlib: packets are routed based on connected gateways and not resources #5054

thomaseizinger opened this issue May 21, 2024 · 3 comments · Fixed by #5082
Assignees
@thomaseizinger
Labels
area/connlib Firezone's core connectivity library business_value/medium Required by between 10% and 50% of our customer base complexity/low Something that should not take more than a few hours. kind/bug Something isn't working

Comments

@thomaseizinger
Copy link
Member

thomaseizinger commented May 21, 2024 •
edited
Loading

Describe the bug

We currently have a mismatch in how we handle the first and subsequent packets of overlapping IP ranges of two resources.

To Reproduce

  1. Add resource foo with IP range 0.0.0.0/8 accessible via gateway A
  2. Send a packet and establish a connection to gateway A
  3. Add resource bar with IP range 0.0.0.0/16 (overlapping but more specific) accessible via gateway B
  4. Send a packet to the narrower range (bar)
  5. Packet will be routed to gateway A because we check which IP ranges gateway A covers (based on all the resources we connected to)

Expected behavior

Packet in (5) should be routed to gateway B.
@thomaseizinger thomaseizinger added the needs triage Issues opened by the public or need further labeling label May 21, 2024
@thomaseizinger thomaseizinger mentioned this issue May 21, 2024
Open
@thomaseizinger
Copy link
Member Author

I discussed this with @conectado. The fix for this is easy, we simply need to always check the longest_match of each incoming packet against the list of resources and route it to the gateway that is responsible for serving this particular resource.

For now, I'll implement the current behaviour in my tests in #4728 and we can then fix it in a separate PR.

@thomaseizinger thomaseizinger added kind/bug Something isn't working business_value/medium Required by between 10% and 50% of our customer base complexity/low Something that should not take more than a few hours. area/connlib Firezone's core connectivity library and removed needs triage Issues opened by the public or need further labeling labels May 21, 2024
@jamilbk Slack
Copy link
Member

jamilbk commented May 21, 2024

Fix sgtm!

@jamilbk
Copy link
Member

jamilbk commented May 21, 2024

Note this should apply for DNS matches too ideally (if it doesn't already).

@thomaseizinger thomaseizinger self-assigned this May 22, 2024
@conectado conectado mentioned this issue May 22, 2024
Closed
@thomaseizinger thomaseizinger mentioned this issue May 22, 2024
Merged
@thomaseizinger thomaseizinger linked a pull request May 22, 2024 that will close this issue
fix(connlib): deterministically route packets in case of overlap #5082
Merged
github-merge-queue bot pushed a commit that referenced this issue May 25, 2024
@thomaseizinger
fix(connlib): deterministically route packets in case of overlap (#5082)
97ae522 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thomaseizinger thomaseizinger

Labels
area/connlib Firezone's core connectivity library business_value/medium Required by between 10% and 50% of our customer base complexity/low Something that should not take more than a few hours. kind/bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(connlib): deterministically route packets in case of overlap firezone/firezone
2 participants
@jamilbk @thomaseizinger