chore(connlib): use proxy ip to match filters instead of translated by conectado · Pull Request #6960 · firezone/firezone

conectado · 2024-10-08T02:25:34Z

Previously, when a gateway checked if a packet was allowed through, it used the real IP of the DNS resource that the client was trying to communicate with.

The problem with this was that if there's an overlapping CIDR resource with the real IP this would allow a user to send packets using the filters for that resource instead of the DNS resource filters.

This can be confusing for users as packet can flow unexpectedly to the resources even if the filter doesn't permit it, so we use the IP of the packet before we translate it to the real IP to match the filters.

This doesn't change the security of this feature as a user can just change the IP of the packet with the dst of the DNS or the cidr resource according to what they need.

Fixes #6806

vercel · 2024-10-08T02:25:38Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
firezone	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 10, 2024 4:08pm

conectado · 2024-10-09T00:55:24Z

I manually tested this with the local docker-compose as it's hard to do automated testing until we do non-icmp packets in proptests

thomaseizinger

Left some comments with ideas.

thomaseizinger · 2024-10-09T01:58:16Z

    fn ensure_allowed_dst(&mut self, packet: &IpPacket) -> anyhow::Result<()> {
        let dst = packet.destination();
+
+        // Note a Gateway with Internet resource should never get packets for other resources


Can you open an issue where we can brainstorm design ideas how we can model this better?

thomaseizinger · 2024-10-09T02:03:41Z

    ipv4: Ipv4Addr,
    ipv6: Ipv6Addr,
    resources: HashMap<ResourceId, ResourceOnGateway>,
+    internet_resource_enabled: bool,


Isn't this implicit from having an Internet Resource in the hash map? Why track this separately?

If this is just a cache, let's document that. Plus, should we have a follow-up to make it an either resources or Internet resource in here once Internet Sites are released?

Let's do it as part of #6982

thomaseizinger · 2024-10-09T02:06:57Z

+
+            debug_assert!(resource.is_dns());
+
+            let mut filter_engine = FilterEngine::empty();


Is there a way we can de-duplicate this with the above bit?

LMK what you think now

thomaseizinger · 2024-10-09T02:07:20Z

+}
+
 #[cfg(test)]
 mod tests {


Would it be very difficult to at least add a unit-test? I think some kind of automated test would be good, doesn't have to be a proptest.

I can do it, I'd need to replicate the whole proxy-ips thingy to have it mean something. Let me take another look at this.

I can do it, I'd need to replicate the whole proxy-ips thingy to have it mean something. Let me take another look at this.

It shouldn't be that difficult, should it? All we need is to define a few IP addresses, doesn't have to be 8, 1 proxy IP should be enough, right?

thomaseizinger · 2024-10-09T02:11:14Z

I manually tested this with the local docker-compose as it's hard to do automated testing until we do non-icmp packets in proptests

I think doing a TCP handshake in the proptests should be pretty easy now that we have etherparse. Do you want to try adding that? I don't think it will help with one much because testing filters might be tricky but it would be a good addition regardless :)

conectado · 2024-10-09T02:30:02Z

I manually tested this with the local docker-compose as it's hard to do automated testing until we do non-icmp packets in proptests

I think doing a TCP handshake in the proptests should be pretty easy now that we have etherparse. Do you want to try adding that? I don't think it will help with one much because testing filters might be tricky but it would be a good addition regardless :)

yeah, I'd like to do that. But why do you think adding filters would be hard? It's encoding the expected packets?

thomaseizinger · 2024-10-09T06:11:31Z

I manually tested this with the local docker-compose as it's hard to do automated testing until we do non-icmp packets in proptests

I think doing a TCP handshake in the proptests should be pretty easy now that we have etherparse. Do you want to try adding that? I don't think it will help with one much because testing filters might be tricky but it would be a good addition regardless :)

yeah, I'd like to do that. But why do you think adding filters would be hard? It's encoding the expected packets?

I think so yes. If we randomly sample the filters, I think we will need to essentially re-build the filter matching engine in the tests to compute, whether the packet will make it through.

What we can do is perhaps make a simpler model by only adding resources with certain filters, like HTTP, HTTPS, SSH etc. If there is only a certain list to choose from, we can for example easily test that packets on other ports don't go through.

That would still have the benefit of checking what we fix in this PR I think. We do still miss out on some of the overlapping filters logic then but perhaps that is a good trade-off? Similarly to DNS names where we only use 1 level of subdomains for wildcard resources, the proptest could only use a certain subset of the filter features and we leave the rest to more specific unit tests.

thomaseizinger · 2024-10-09T22:09:12Z

+            tracing::trace!(%addr, filters = ?filter_engine, "Installing new filters");
+            self.filters.insert(*addr, filter_engine);


Could we deduplicate this too? I think it should be possible to have have a function that takes ip and list of filters, creates a filter engine, logs it and inserts it, right?

It's possible but I'd need a wrapper for the network table so it can be done by self.filter or a static function. Otherwise the borrow checker won't allow to take self as mutable as I'm already holding self.resources as immutable.

I could also just clone self.resources but would require making ResourceOnGateway clonable.

updated with the static function version

thomaseizinger

Great stuff!

conectado · 2024-10-10T16:08:25Z

@thomaseizinger added some unit-test! With this it should be ready for merge

thomaseizinger

Awesome, thanks for adding tests!

In #6960, split up the filter handling by resource type. This uncovered an issue that was already known in and fixed in the portal in #6962: Upstream DNS servers must not be in any of the reserved IP ranges. Related: #7060.

chore(connlib): use proxy ip to match filters instead of translated

3e0bd3c

conectado added 2 commits October 8, 2024 11:33

update changelog

8b41f1e

make clippy happy

6199cfc

vercel Bot deployed to Preview October 8, 2024 14:39 View deployment

fix(connlib): don't panic on resource removal

a04a3f1

vercel Bot deployed to Preview October 8, 2024 21:55 View deployment

fix allowing internet resource packets through

bdb9eb9

vercel Bot deployed to Preview October 9, 2024 00:44 View deployment

fix changelog

bfd7a1b

vercel Bot deployed to Preview October 9, 2024 00:49 View deployment

conectado marked this pull request as ready for review October 9, 2024 00:54

conectado requested a review from thomaseizinger October 9, 2024 00:55

thomaseizinger approved these changes Oct 9, 2024

View reviewed changes

fix changelog

5d2839c

vercel Bot had a problem deploying to Preview October 9, 2024 02:40 Failure

conectado mentioned this pull request Oct 9, 2024

refactor connlib model disjoint internet site and non-internet site #6982

Closed

apply pr feedback

3a7d2af

vercel Bot deployed to Preview October 9, 2024 02:47 View deployment

thomaseizinger approved these changes Oct 9, 2024

View reviewed changes

Comment thread rust/connlib/tunnel/src/peer.rs Outdated

wip

32d3d57

vercel Bot deployed to Preview October 9, 2024 11:37 View deployment

refactor filter calculation for gateway

ee3ec2b

vercel Bot deployed to Preview October 9, 2024 16:46 View deployment

thomaseizinger approved these changes Oct 9, 2024

View reviewed changes

dedup inserting new filters

6f7a6cc

vercel Bot deployed to Preview October 10, 2024 04:05 View deployment

thomaseizinger approved these changes Oct 10, 2024

View reviewed changes

conectado added 2 commits October 10, 2024 12:54

adds test that dns and cidr filters dont mix

c550a60

check that internet resource doesnt allow traffic for dns

0443d02

vercel Bot deployed to Preview October 10, 2024 16:08 View deployment

conectado requested a review from thomaseizinger October 10, 2024 16:08

thomaseizinger approved these changes Oct 10, 2024

View reviewed changes

conectado added this pull request to the merge queue Oct 10, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 10, 2024

jamilbk added this pull request to the merge queue Oct 13, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 13, 2024

thomaseizinger added this pull request to the merge queue Oct 15, 2024

Merged via the queue into main with commit eedee56 Oct 15, 2024

thomaseizinger deleted the chore/firezone/merge-traffic-filters-by-type branch October 15, 2024 23:35

thomaseizinger mentioned this pull request Oct 16, 2024

chore(connlib): don't generate DNS servers in reserved IP ranges #7068

Merged


		debug_assert!(resource.is_dns());

		let mut filter_engine = FilterEngine::empty();

		tracing::trace!(%addr, filters = ?filter_engine, "Installing new filters");
		self.filters.insert(*addr, filter_engine);

Uh oh!

Conversation

conectado commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel Bot commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

conectado commented Oct 9, 2024

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger commented Oct 9, 2024

Uh oh!

conectado commented Oct 9, 2024

Uh oh!

thomaseizinger commented Oct 9, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

conectado commented Oct 10, 2024

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

conectado commented Oct 8, 2024 •

edited

Loading

vercel Bot commented Oct 8, 2024 •

edited

Loading