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

Support filtering IPs for outgoing traffic #702

Closed
eyalb181 opened this issue Nov 7, 2022 · 16 comments
Closed

Support filtering IPs for outgoing traffic #702

eyalb181 opened this issue Nov 7, 2022 · 16 comments
Assignees
Labels
enhancement New feature or request major user

Comments

@eyalb181
Copy link
Member

eyalb181 commented Nov 7, 2022

We should let users enable outgoing traffic, but then specify a list of IPs for which outgoing traffic would remain local.

Motivation

We want to let users mix and match resources to be used, locally and remote.

  • For example, if the process sends traffic to localhost, they can configure it to use the local machine's localhost rather than the remote pod's.
  • Filtering should be based on protocol, port, host, ip and subnet.

Implementation

outgoing config should support the field remote and local, each overriding the default behavior of the general setting.
The value for these fields will be a list of items with the following format:
[PROTOCOL://]IP/[subnet]:PORT
HOST:PORT
:PORT
HOST

For example:

remote = ["udp://1.1.1.0/24:1337", "1.1.5.0/24", "tcp://google.com:53", "google.com", ":53"]
local = ["localhost"]

will lead to the following behavior:

  1. Any TCP connection to 1.1.1.0/24 subnet with destination port 1337 will go from the remote
  2. Any connection to 1.1.5.0/24 subnet will go from the remote
  3. Connection to google.com resolved DNS will go from the remote if TCP (need dynamic management here probably).
  4. Connection to any IP with any protocol with port 53 will go from remote
  5. Any connection to localhost or it's resolved IPs will be local.

Details

This issue should solve the following use cases:

  1. DNS Resolving
  2. TCP connections
  3. UDP

Warnings

This feature has some sharp edges that might hurt, those edges come to mind:

  • Go/Custom DNS resolving (using UDP instead of just libc's getaddrinfo etc) - harder to cover, need to do some sort of MITM for the resolving. Can be deferred for a follow up issue.
  • State management - for example if we resolve google.com we would want to passthrough connections to it but we need to maintain some state dns <-> sockets that doesn't exist currently -> can be deferred to another issue
@eyalb181 eyalb181 added the enhancement New feature or request label Nov 7, 2022
@errietta
Copy link

errietta commented Nov 7, 2022

This would be great to have. We have an external API we use, and we have to allow list specific IPs. So we've allowlisted our k8s cluster. I want to be able to call that API, but the code will also write to the db, and I want to write to my local db not the remote one

@aviramha aviramha added the user label Nov 7, 2022
@eyalb181 eyalb181 added the major label Nov 7, 2022
@mentos1386
Copy link

My usecase is https://www.prisma.io/ which starts an "engine" that listens on a random port (it seems to me). Which makes it impossible to use, as the "in cluster pod" might not be listening on exactly the same port 😦

@aviramha
Copy link
Member

aviramha commented Mar 6, 2023

I have implemented #1154 until we get this done fully.
Next version you'll be able to control if outbound sockets to localhost would go through the pod or locally, same for listening sockets.

We need to decide whether we leave this config after we add the full filtering functionality or let users use the new one.
This issue doesn't cover Incoming so we should consider that too.

@mentos1386
Copy link

Another usacese we have is:

We are running mirrord in docker container (A).
We are running a database in a different docker container (B).
We want A to be able to connect to B.

This is currently not possible. As A would route all traffic via K8S, and K8S can't access B, as B is on the developers laptop not in the K8S.

To allow us to have mixed remote and local resources that we can connect to.

@meowjesty meowjesty self-assigned this Jun 16, 2023
meowjesty added a commit to meowjesty/mirrord that referenced this issue Jun 19, 2023
meowjesty added a commit to meowjesty/mirrord that referenced this issue Jul 10, 2023
github-merge-queue bot pushed a commit that referenced this issue Jul 14, 2023
* outgoing filter #702

* Update .lock

* Refactoring connect, move connect to local address into UserSocket impl.

* Revert to free function to avoid dup issue.

* Extracting values from config (unsatisfied).

* Changes im not sure about.

* Add outgoing filter parsing.

* Add some invalid cases for testing.

* Outgoing filter initialization.

* No need to re-create filters with TCP UDP distinction for ANY, as they will be checked in connect_outgoing ConnectType anyway.

* Filter is plugged in.

* Appease clippy

* Add default to config.

* Add default for remote/local config. Fix comparing resolved address, now compares user address on port 0 (resolved addresses always have port 0).

* inline comparison.

* Docs in layer.

* unused.

* remove comments.

* Improve bypass for filtered out. Add to analytics.

* Update schema.

* Log more stuff in connect_outgoing.

* Only bypass on selector if its not unix.

* Add more logs.

* Log on socket close.

* Revert localhost connect refactor.

* Add reference, fix compilation

* Dont bypass on unix stream, or on empty option for selector.

* Better if else.

* Better connect_to_local_address (question mark).

* Small cleanup and docs.

* Add docs for config.

* More docs for parsing.

* Appease clippy

* Modify outgoing test to take filter. Fix filter not properly checking ip port when unspecified.

* Docs detailing precedence.

* changelog

* Removed unused type.

* debug->trace

* Remove some more logs.

* Missed log.

* Fix broken doc link.

* s/input/rest as the return binding in the parser functions.

* Address (hehe) CR. Improve docs. Improve names for bindings. Remove many1 concats where digit1 is used.

* appease clipy

* Docs for outgoing local test.

* Only allow either remote or local to be specified. Remove intersection check (cant specify both anymore).

* deal with strs instead of bytes.

* Improve docs.

Co-authored-by: t4lz <t4lz.git@gmail.com>

* Fix check on connect_remote. Update docs.

* Convert outgoing filter into enum, to typefy that the user can only specify 1 variant.

* Improve config, now it works and no long allows remote + local together.

* Fix docs (update them to the new config).

* Mark with unstable.

* Update schema.

* Add error on empty values when using from_env outgoing filter.

* Change log level for connect.

* Improve config, take out inner filter struct.

* Remove outdated file.

* Fix config for tests.

* Run test on mac and linux

* Improve config path handling in test.

Co-authored-by: t4lz <t4lz.git@gmail.com>

* Working config.

* Build test for macos.

* Sanity check that missing remote address doesnt trigger daemon messages and hangs.

* Fix docs.

Co-authored-by: t4lz <t4lz.git@gmail.com>

* update schema

* fix docs

* fix missing closing code doc

* update schema

---------

Co-authored-by: t4lz <t4lz.git@gmail.com>
@meowjesty
Copy link
Member

Part of this was implemented in #1607!

Named hosts are coming in #1648.

@meowjesty
Copy link
Member

I think part of DNS resolving should be done in the DNS feature itself, we could expand it to also be filter-like, and have dns: ["cluster-service". "foo.com"] resolve remotely, while everything else resolves locally.

Doing so would help in the case where the user has (outgoing) local: ["localhost"], and the app tries to connect("localhost"), but if they have DNS enabled, it resolves to some cluster address, thus breaking the request/traffic.

@eyalb181
Copy link
Member Author

eyalb181 commented Aug 4, 2023

Makes sense to me. Should we still let the user configure a hostname filter under outgoing, though? Shouldn't it be moved entirely to the dns configuration field?

@t4lz
Copy link
Member

t4lz commented Aug 4, 2023

and have dns: ["cluster-service". "foo.com"] resolve remotely

I don't think we need an extra list, I think we can use the outgoing filter.
DNS only matters for outgoing traffic, so all names you are connecting to remotely should be resolved remotely and all names you are connecting to locally should be resolved locally, right?

@eyalb181
Copy link
Member Author

eyalb181 commented Aug 4, 2023

Yup, my mistake, you're right

@meowjesty
Copy link
Member

Putting this in the outgoing filter feels confusing to me, for example:

dns = on
local = ["service-a", "service-b"]

// service-a becomes 1.2.3.4:0
// service-b becomes 1.2.3.4:0
// service-c (which we want remote) becomes 1.2.3.4:0

dns = off
local = ["service-a", "service-b"]

// service-a becomes 127.0.0.1:0
// service-b becomes 127.0.0.1:0
// service-c (which we want remote) becomes 127.0.0.1:0

This is kinda what I expect, when I enable DNS resolving, I want things to be resolved on the agent. So changing the DNS behavior on the filter config like this:

dns = on
local = ["service-a", "service-b"]

// service-a becomes 127.0.01:0
// service-b becomes 127.0.0.1:0
// service-c (which we want remote) becomes 1.2.3.4:0

Feels a bit like the outgoing filter is going over its boundaries? Like, the 2 features enter in a bit of a contradiction (subverts expectations)? What do you guys think?

In my opinion, we should put DNS handling stuff in the DNS feature, the user would need something like:

dns.local = ["service-a", "service-b"]
local = ["service-a", "service-b"]

Is explicit better than implicit here?

@eyalb181
Copy link
Member Author

eyalb181 commented Aug 4, 2023

I think we definitely don't want the user to have to specify two lists.

  1. Note that in most cases, the dns field is going to be omitted entirely, so users aren't going to be like "but I asked for DNS to be done remotely".
  2. I'm not sure we need to support specifying outgoing requests for domain X to be done remotely, but DNS resolution for that same domain to be done locally, or vice versa

What I'd do is show a warning if the user specified DNS on/off explicitly, and also specified a hostname in the filter.

@meowjesty
Copy link
Member

tal's suggestion link:

On every outgoing connection - we send out DNS queries (probably via the agent) for all the names in the outgoing filter.
I think alternatively - we could avoid making any extra DNS queries and just check in the getaddrinfo hook if the queried name is in the filter - and resolve that IP in the filter if it is.
That would mean the feature is slightly different - we would only filter out connections by name if the application looked them up by name, but this makes sense in my opinion.

@eyalb181
Copy link
Member Author

eyalb181 commented Aug 7, 2023

Agree with the last line, @aviramha wdyt?
So getaddrinfo would read the filter and use it to decide whether to resolve locally or remotely?

@aviramha
Copy link
Member

aviramha commented Aug 8, 2023

Sounds good

github-merge-queue bot pushed a commit that referenced this issue Aug 21, 2023
* outgoing filter #702

* Update .lock

* Refactoring connect, move connect to local address into UserSocket impl.

* Revert to free function to avoid dup issue.

* Extracting values from config (unsatisfied).

* Changes im not sure about.

* Add outgoing filter parsing.

* Add some invalid cases for testing.

* Outgoing filter initialization.

* No need to re-create filters with TCP UDP distinction for ANY, as they will be checked in connect_outgoing ConnectType anyway.

* Filter is plugged in.

* Appease clippy

* Add default to config.

* Add default for remote/local config. Fix comparing resolved address, now compares user address on port 0 (resolved addresses always have port 0).

* inline comparison.

* Docs in layer.

* unused.

* remove comments.

* Improve bypass for filtered out. Add to analytics.

* Update schema.

* Log more stuff in connect_outgoing.

* Only bypass on selector if its not unix.

* Add more logs.

* Log on socket close.

* Revert localhost connect refactor.

* Add reference, fix compilation

* Dont bypass on unix stream, or on empty option for selector.

* Better if else.

* Better connect_to_local_address (question mark).

* Small cleanup and docs.

* Add docs for config.

* More docs for parsing.

* Appease clippy

* Modify outgoing test to take filter. Fix filter not properly checking ip port when unspecified.

* Docs detailing precedence.

* changelog

* Removed unused type.

* debug->trace

* Remove some more logs.

* Missed log.

* Adding DNS resolution to outgoing filter (issue #702).

* Notes on what to do in getaddrinfo

* Fix broken doc link.

* s/input/rest as the return binding in the parser functions.

* Change getaddrinfo, now its possible to resolve dns with it through the remote directly. Resolve DNS for named addresses in the outgoing filter when REMOTE_DNS is enabled.

* Address (hehe) CR. Improve docs. Improve names for bindings. Remove many1 concats where digit1 is used.

* appease clipy

* Docs for outgoing local test.

* Only allow either remote or local to be specified. Remove intersection check (cant specify both anymore).

* deal with strs instead of bytes.

* Improve docs.

Co-authored-by: t4lz <t4lz.git@gmail.com>

* Fix check on connect_remote. Update docs.

* Convert outgoing filter into enum, to typefy that the user can only specify 1 variant.

* Improve config, now it works and no long allows remote + local together.

* Fix docs (update them to the new config).

* Mark with unstable.

* Update schema.

* Add error on empty values when using from_env outgoing filter.

* Change log level for connect.

* Improve config, take out inner filter struct.

* Remove outdated file.

* Fix config for tests.

* Run test on mac and linux

* Improve config path handling in test.

Co-authored-by: t4lz <t4lz.git@gmail.com>

* Working config.

* Build test for macos.

* Sanity check that missing remote address doesnt trigger daemon messages and hangs.

* Fix docs.

Co-authored-by: t4lz <t4lz.git@gmail.com>

* update schema

* Fix compilation. Added some notes on how to improve DNS resolution.

* Appease clippy

* Fix filtering unresolved hosts (bool flag was wrong). Add a few logs. Outgoing named filter should be working now.

* Add test for DNS resolving filter.

* panic on unexpected message.

* use magic service

* trying to get the flow right

* the test keeps growing (and not working)

* Use e2e outgoing_traffic_udp_with_connect to test outgoing named filter.

* Remove integration changes.

* revert files

* Fix config

* Use dynamic internal service name in config.

* cleanup, fix docs

* changelog

* use service name as the random string for test file

* remove commented code

* Resolve DNS locally when local is used.

* Address review. Better length calculation. Improve name of closure. Dont reuse test. Better order for filtering.

* Warn on potential misuse of remote + dns turned off.

* new warning

Co-authored-by: t4lz <t4lz.git@gmail.com>

* Move warning to cli execution thingy.

* use warning to print warning

* Improving DNS resolving for connect filter. Now local = port 7777 resolves with the correct local address, and swap it on the users connect call.

* remote filter on port now resolves and connects to address from cluster

* fix local resolve dns for filters not having port

* simplify retrieval of connection address from dns_cache.

* docs

* appease clippy

* docs for DNS_CACHE

* cargo.lock

* improve docs. refactor some names. simplify local resolve check in dns_cache.

* debug -> trace

* cache -> reverse mapping

* docs for local selector

* improve docs

Co-authored-by: t4lz <t4lz.git@gmail.com>

* appease fmt lint

---------

Co-authored-by: t4lz <t4lz.git@gmail.com>
@meowjesty
Copy link
Member

The filter should now be supporting almost everything listed in the issue, except for custom resolvers:

Go/Custom DNS resolving (using UDP instead of just libc's getaddrinfo etc) - harder to cover, need to do some sort of MITM for the resolving. Can be deferred for a follow up issue.

A little bit of stateful DNS was added, to make the filter behave nicely with a mix of DNS and other ip/port configs, not the full thing mentioned in the issue though (there is no socket<->dns state).

@meowjesty
Copy link
Member

Closing this in favor of #1826

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request major user
Projects
None yet
Development

No branches or pull requests

6 participants