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.

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

Traffic filtering #2030

Closed
1 of 3 tasks
Tracked by #1769
jamilbk opened this issue Aug 24, 2023 · 24 comments · Fixed by #4779
Closed
1 of 3 tasks
Tracked by #1769

Traffic filtering #2030

jamilbk opened this issue Aug 24, 2023 · 24 comments · Fixed by #4779
Assignees
Labels
area/connlib Firezone's core connectivity library area/gateway Issues involving the Firezone Gateway area/portal Portal, panel, web, control plane, you name it! area/website Issues involving the firezone.dev website business_value/high Required by > 50% of our customer base complexity/epic Large, multi-issue changes affecting multiple components kind/feature New feature or request kind/feedback Issue created as a direct result of customer feedback
Milestone

Comments

@jamilbk
Copy link
Member

jamilbk commented Aug 24, 2023

Image

  • connlib: control protocol additions
  • connlib: gateway packet filter implementation
  • firezone/product#654
@jamilbk jamilbk changed the title traffic filtering epic: traffic filtering Aug 28, 2023
@jamilbk jamilbk changed the title epic: traffic filtering traffic filtering Sep 11, 2023
@jamilbk jamilbk transferred this issue from another repository Sep 12, 2023
@jamilbk jamilbk transferred this issue from firezone/private-to-public Sep 12, 2023
@jamilbk jamilbk added kind/feature New feature or request area/gateway Issues involving the Firezone Gateway area/connlib Firezone's core connectivity library complexity/medium Something that should not take more than a day or two. business_value/critical Required by 100% of our customer base labels Sep 12, 2023
@jamilbk jamilbk added this to the 1.0 milestone Sep 12, 2023
@jamilbk jamilbk mentioned this issue Sep 25, 2023
6 tasks
@jamilbk jamilbk changed the title traffic filtering Traffic filtering Nov 10, 2023
@jamilbk jamilbk removed the business_value/critical Required by 100% of our customer base label Dec 26, 2023
@jamilbk jamilbk added the kind/feedback Issue created as a direct result of customer feedback label Feb 16, 2024
@jamilbk jamilbk removed this from the 1.0 GA milestone Mar 25, 2024
@jamilbk jamilbk added the area/portal Portal, panel, web, control plane, you name it! label Apr 22, 2024
@jamilbk
Copy link
Member Author

jamilbk commented Apr 22, 2024

Time to make this happen :-)

Todo:

  • portal:
    • part of Team plan
    • Allow editing existing Resources' traffic filters on Starter (to handle downgrade edge cases)
    • On starter, when adding resource, show Traffic filters but disabled, with "upgrade to unlock" (most users won't read announcements or read feature matrix)
  • gateway: implement traffic filters received from Portal
  • docs: add traffic filter docs section either in Resources or separately
  • announce in newsletter/socials
  • website: add to pricing matrix

@jamilbk jamilbk added business_value/high Required by > 50% of our customer base complexity/epic Large, multi-issue changes affecting multiple components area/website Issues involving the firezone.dev website and removed complexity/medium Something that should not take more than a day or two. labels Apr 22, 2024
@jamilbk
Copy link
Member Author

jamilbk commented Apr 22, 2024

Edge case with DNS Resources:

If DNS resources map to the same IP and you want to allow access to port 80 but not port 8080 for a User, this runs into an issue where the user would be allowed both.

@AndrewDryga
Copy link
Collaborator

We should explain that traffic filters work on IP level, so they apply after DNS name is resolved and then merged per dest up.

Resource A: a.mycorp.com:80
Resource B: b.mycorp.com:123

On Gateway:
RSLV a.mycorp.com -> 100.100.100.100
RSLV b.mycorp.com -> 100.100.100.100
Mapping 100.128.0.1 -> 100.100.100.100

On client:
You can access a.mycorp.com on both ports 80 and 123,
and you can access b.mycorp.com on both ports 80 and 123,

@jamilbk
Copy link
Member Author

jamilbk commented Apr 22, 2024

Possible solution? (involves lots of refactoring though):

github.com:80 -> 142.0.0.1
gitlab.com:443 -> 142.0.0.1

  1. client queries github.com

  2. receives 100.96.0.1

  3. application sends traffic from 100.100.0.1:40000 to 100.96.0.1:80

  4. client mangles packet 100.100.0.1:40000 -> 142.0.0.1:80: check that 80 is within 80, 443

  5. resource responds 142.0.0.1:80 -> 100.100.0.1:40000

  6. client mangles back 100.96.0.1:80 and replies to application

  7. client queries gitlab.com

  8. receives 100.96.0.2

  9. application sends traffic from 100.100.0.1:40000 to 100.96.0.2:80

  10. client mangles packet to 100.100.0.1:41000 -> 142.0.0.1:80

  11. resource responds 142.0.0.1:80 -> 100.100.0.1:41000

  12. client mangles back to 100.96.0.1:80 -> 100.100.0.1:40000

Gateway:

Resource: {
ports: [int]
}

Peer: {
CGNAT IP -> ResourceId1 {
mapped_src_ports: hash(): {orig_src_port, dst_ip} -> {dst_port, src_ip}
}

CGNAT IP -> ResourceId1 {
Name:
ruleset: (could be from multiple Resources -- list of ports to allow)
}
}

@conectado
Copy link
Collaborator

conectado commented Apr 22, 2024

Current approach to DNS resources

  1. Client makes DNS query for a resource.
  2. DNS resolution is requested to the gateway along with access
  3. Assuming that the portal allows it, gateway responds with the resolved DNS addresses and installs a permission for the resolved IPs for the given client
  4. The client creates or re-uses a translation for the resolved IP(s) to CGNAT space, keeping the invariant that the "real" IP for a given gateway peer always maps to the same CGNAT IP, regardless of whether or not it's the same resource
  5. The client then whenever sees an outgoing packet for the CGNAT IP translates it to the real IP
  6. Packets incoming from the real IP are translated to CGNAT space
  7. The gateway only accepts packets outgoing to the real IP

Drawback(regarding to traffic filtering)

The problem is that when 2 different DNS resources resolve to the same real IP, we can't distinguish traffic between them, so the Gateway must apply the rules only based on IPs.

New approach(proposed by @jamilbk )

Summary

The idea is to have the gateway do the packet mangling, the client would only ever see the cgnat ips.

Then, on the gateway, the translation is made between CGNAT and real IPs, however the translation would be made but not only ip would be translated but also, a new sport would be picked.

Then incoming traffic can be uniquely distinguished by the tuple (dport, daddr, saddr), where daddr is used to see what client peer the traffic is meant for and the tupple (dport, saddr) is used to restore the original (dport, saddr), which was determined originally with out-going traffic

Drawbacks

The problem still is that an attacker, can use the daddr of the other resource to have access of the dport for that resource.

And this requires a big refactor and a lot more state tracking on the gateway.

In general it seems impossible to apply rules meant for layer 5 to the traffic we see in layer 3

@AndrewDryga
Copy link
Collaborator

Would this solution work for two DNs resources with the same port filters for both of them? And what if port filter rule for those is more than half of the port range or partially overlapping large port ranges?

For example, active FTP port range is 20, 60000-65535.

@conectado
Copy link
Collaborator

Would this solution work for two DNs resources with the same port filters for both of them? And what if port filter rule for those is more than half of the port range or partially overlapping large port ranges?

For example, active FTP port range is 20, 60000-65535.

The approach is independent of the port filtering itself, is just a way to be able to distinguish between resources in the gateway.

@AndrewDryga
Copy link
Collaborator

But how do you distinguish when two port ranges overlap for the same IP but different resources?

@conectado
Copy link
Collaborator

But how do you distinguish when two port ranges overlap for the same IP but different resources?

If they are different resources the daddr would be different, so we apply rules only meant for that ip.

Or you mean for incoming traffic? I think we do only egress filtering

@conectado
Copy link
Collaborator

But how do you distinguish when two port ranges overlap for the same IP but different resources?

Also, I think we're not going to go with this approach for the reasons explained here:

Drawbacks

The problem still is that an attacker, can use the daddr of the other resource to have access of the dport for that resource.

And this requires a big refactor and a lot more state tracking on the gateway.

In general it seems impossible to apply rules meant for layer 5 to the traffic we see in layer 3

@conectado
Copy link
Collaborator

Seems like the portal is currently not sending ICMP filtering rules cc @AndrewDryga

@conectado
Copy link
Collaborator

Same for "Permit All" the filter field seems to come empty.

We could assume that an empty filter means "allow all" but doesn't seem like the best way to go.

@AndrewDryga
Copy link
Collaborator

@conectado I understand that empty filters = allow all might be a bit confusing but this semantic was chosen because it's an opt-in feature, so the value can be empty by default. We can discuss it on standup tomorrow and change if needed.

I pushed a fix for ICMP filters.

@AndrewDryga
Copy link
Collaborator

Or you mean for incoming traffic? I think we do only egress filtering

I was thinking that if you will use (dport, daddr, saddr) for routing packets then there are valid situations where they can be the same for multiple resources.

@conectado
Copy link
Collaborator

Or you mean for incoming traffic? I think we do only egress filtering

I was thinking that if you will use (dport, daddr, saddr) for routing packets then there are valid situations where they can be the same for multiple resources.

(dport, daddr, saddr) would be used to distinguish for incoming traffic and since for outgoing traffic we would pick a different sport the dport would always distinguish traffic for the same peer.

@conectado
Copy link
Collaborator

Continuing the discussion here #4779 (comment)

Let's say there are 2 overlapping CIDR resources on the client with different port filter rules
10.0.0.0/24 -> TCP/80
10.0.0.0/16 -> TCP/443

Right now the client has no information about port filtering and the request_connection is sent only based on IP, meaning, if client tries to access to 10.0.0.1:443 right now it picks resource based only on ip, the most specific one.

So the resource 10.0.0.0/24 would be sent to the gateway and traffic for port 443 wouldn't be allowed.

And without changes to the control protocol this can't be fixed.

@conectado
Copy link
Collaborator

The first implementation will be naive and simply have this problem, it will be solved with #4789

Ideally, we can add a warning when there are overlapping resources in the portal cc @AndrewDryga

@jamilbk
Copy link
Member Author

jamilbk commented Apr 26, 2024

@conectado Yeah actually, thinking more about it, I'm not sure I'm right about the user expectation on this one.

I think we should get it out there and get feedback on it before going down the difficult path of resolving filters across all overlapping CIDRs.

Just documenting well how this works is probably good enough for a good UX here.

@AndrewDryga
Copy link
Collaborator

@jamilbk yeah, that's why we decided not to do client filtering right away. It feels like a pretty rare edge case that we already know how to solve (add filtering on the client too). So all we have to do is to keep an issue open for it in case we need to come back and implement it later.

@jamilbk
Copy link
Member Author

jamilbk commented Apr 26, 2024

Adding note here after discussing with @conectado --

This would be required if we were to do the filtering on the Client as well, which we can save for later on, perhaps for #949. That would add another layer of security, since an attacker would need to compromise both the Gateway and Client in order to tamper with logged traffic.

github-merge-queue bot pushed a commit that referenced this issue Apr 26, 2024
)

This came up while working on #2030 and thinking about testing `Peer`.

Not entirely convinced of taking both `Instant` and `DateTime<Utc>` but
unless we convert the expiration to an instant, which would bring a
bunch of new problems, I don't see another way to do this.
@jamilbk jamilbk added this to the 04/24 milestone Apr 29, 2024
@conectado
Copy link
Collaborator

@AndrewDryga I just realized that now we can get some filters with messages "all", does it still mean that empty filters means allow all or allow none?

@jamilbk
Copy link
Member Author

jamilbk commented May 3, 2024

@AndrewDryga I just realized that now we can get some filters with messages "all", does it still mean that empty filters means allow all or allow none?

It should be permit all. Deny all is what happens when there's no policy.

@conectado
Copy link
Collaborator

@AndrewDryga I just realized that now we can get some filters with messages "all", does it still mean that empty filters means allow all or allow none?

It should be permit all. Deny all is what happens when there's no policy.

If that's the case I rhink we should remove the "all" value for filters. Having 2 ways to express the same onlu makes things more complex.

@jamilbk
Copy link
Member Author

jamilbk commented May 3, 2024

@AndrewDryga I just realized that now we can get some filters with messages "all", does it still mean that empty filters means allow all or allow none?

It should be permit all. Deny all is what happens when there's no policy.

If that's the case I rhink we should remove the "all" value for filters. Having 2 ways to express the same onlu makes things more complex.

Yeah, it might be confusing if Permit all doesn't do anything, or creates a Resource that can't be accessed if left unchecked.

Can discuss this issue at standup. I think this could be solved by making Permit all and the Filters sections a radio toggle.

github-merge-queue bot pushed a commit that referenced this issue May 7, 2024
This implements traffic filtering on the gateway. Filters are set on the
portal, per-resource, in an allow-list manner.

If no filters exist for a given resource all packets are allowed,
otherwise only packets that matches port/protocol for the filters are
allowed, otherwise they are dropped.

Filters can be either TCP, UDP or ICMP. For the first 2 multiple ports
can be given. Furthermore, multiple filters can exists for the same
resource.

To be able to add and remove filters with the same IP/CIDR we keep
around the whole list of filters for any given peer using an ID map and
recalculate the IP each time something is added is removed.

This allows us to remove filters and simply recalculate the allowlist
for each IP.

Furthermore, for any IP, all rules apply, meaning if there are multiple
IPs that apply for a resource all port/protocol combinations for that IP
will apply.

This works well right now for DNS resources, since access is requested
by DNS name, then the resource for that DNS name will arrive at the
gateway, and the port filtering will apply given that resource(and any
other resource with the same IP).

However, since the client has no idea of the filters, it can't request
the resource access based on the port/protocol combination and we are
still using the most specific("longest match") IP. This will mean that
for overlapping CIDR resources, only the rules for the most specific
will be used, even if the gateway supports applying them all, since it
will not have the other resources. This will be solved in #4789.

It can also lead to some weirdness, let's say that you have 10.0.0.0/24
-> TCP/80 and 10.0.0.0/16 -> TCP/443 for your user.

The user tries to access 10.0.0.1, and will then only be allowed port
80. At some point the user might access 10.1.0.1 and it will be allowed
port 443. But from that point on, the user will be allowed to access 80
and 443 in 10.0.0.1 because the rules correctly work on the gateway, the
problem is the client side. Again, #4789 will fix this.

Left for next PRs (in tentative order!):

- #4792 
- #4789 

Depends on: #4773.
Resolves #2030.
Resolves #4791.

---------

Co-authored-by: Jamil Bou Kheir <jamilbk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connlib Firezone's core connectivity library area/gateway Issues involving the Firezone Gateway area/portal Portal, panel, web, control plane, you name it! area/website Issues involving the firezone.dev website business_value/high Required by > 50% of our customer base complexity/epic Large, multi-issue changes affecting multiple components kind/feature New feature or request kind/feedback Issue created as a direct result of customer feedback
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants