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

Refactor/mark #362

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Refactor/mark #362

wants to merge 9 commits into from

Conversation

Itsusinn
Copy link
Member

@Itsusinn Itsusinn commented Apr 9, 2024

A global mark

@ibigbug
Copy link
Member

ibigbug commented Apr 9, 2024

@Itsusinn
Copy link
Member Author

Itsusinn commented Apr 9, 2024

see this https://en.clash.wiki/advanced-usages/wireguard.html

We should allow overwriting the global mark by setting routing-mark in proxy conf?

Only use global mark when routing-mark is empty?

@ibigbug
Copy link
Member

ibigbug commented Apr 9, 2024

yeah that's an option too, since we hardcode 0xff now. @VendettaReborn thoughts?

@VendettaReborn
Copy link
Contributor

yeah that's an option too, since we hardcode 0xff now. @VendettaReborn thoughts?

yep, it should always have a value, so I think a default global value is fine. And the question is, what's the scope of routing-mark that user can customize?
The global mark is definitely needed, but how about the group or single outbound scale? I am not sure( the clash.meta has this option in every outbound, including the outbound group. But so far, i think it's kinda useless.)

@Itsusinn
Copy link
Member Author

Itsusinn commented Apr 9, 2024

@ibigbug @VendettaReborn
please check the major change in
clash_lib/src/session.rs
clash_lib/src/proxy/utils/socket_helpers.rs

@ibigbug
Copy link
Member

ibigbug commented Apr 9, 2024

well isn't this mostly duplicating changes in #343 ?

@Itsusinn
Copy link
Member Author

Itsusinn commented Apr 9, 2024

well isn't this mostly duplicating changes in #343 ?

Yes i cherry pick one commit from #343. just for respect the proxy node specific mark and iface conf
And further, this pr aims to make an agreement about mark between #343 and #350

@Itsusinn Itsusinn force-pushed the refactor/mark branch 2 times, most recently from a4bbbcb to fcd403d Compare April 9, 2024 13:21
@Itsusinn
Copy link
Member Author

Itsusinn commented Apr 9, 2024

the clash.meta has this option in every outbound, including the outbound group. But so far, i think it's kinda useless.

Agree as well, i cannot think of any use case

@Itsusinn Itsusinn force-pushed the refactor/mark branch 2 times, most recently from 85ac4df to a31ef35 Compare April 9, 2024 14:02
@VendettaReborn
Copy link
Contributor

well isn't this mostly duplicating changes in #343 ?

Yes i cherry pick one commit from #343. just for respect the proxy node specific mark and iface conf And further, this pr aims to make an agreement about mark between #343 and #350

Actually, i am thinking about re-using the shadowsocks-rust's infra: https://github.com/shadowsocks/shadowsocks-rust/blob/b37c6e4dfa13af7bc65b34606baa01e78927199e/crates/shadowsocks/src/net/option.rs#L47. They have done a great job in supporting the fwmark and bind_interface as well as other options across different platforms.

@VendettaReborn
Copy link
Contributor

@Itsusinn
Copy link
Member Author

Itsusinn commented Apr 9, 2024

Actually, i am thinking about re-using the shadowsocks-rust's infra: https://github.com/shadowsocks/shadowsocks-rust/blob/b37c6e4dfa13af7bc65b34606baa01e78927199e/crates/shadowsocks/src/net/option.rs#L47. They have done a great job in supporting the fwmark and bind_interface as well as other options across different platforms.

It seems they take fwmark and bind_interface options as an independent configuration (instead of add these options to every single proxy)?

@Itsusinn
Copy link
Member Author

Itsusinn commented Apr 11, 2024

I think we could merge this pr as a workaround for now.
I could change those things if they don't match our need.
@ibigbug @VendettaReborn

@VendettaReborn
Copy link
Contributor

I think we could merge this pr as a workaround for now. I could change those things if they don't match our need. @ibigbug @VendettaReborn

any ideas about on how to test it?

@Itsusinn
Copy link
Member Author

any ideas about on how to test it?

You mean unit test?
I dont have any idea. actually.

@VendettaReborn
Copy link
Contributor

any ideas about on how to test it?

You mean unit test? I dont have any idea. actually.

I've went through the issue list of clash.meta, and one of the most frequently asked issue is about tun, and it even got routing loop in some conditions. So I think we'd better develop some ways to avoid similar problems.

@VendettaReborn
Copy link
Contributor

For example, when testing fwmark & bind feature, we shall create a list of packet to example.org, with unique cookies in each http request's header. And we shall count the number of occurrence. Whenever the occurrence is 2, we knows that there must be some routing problem. What do u think? cc @Itsusinn

@ibigbug
Copy link
Member

ibigbug commented Apr 16, 2024 via email

@VendettaReborn
Copy link
Contributor

But it seems that we cannot get the binded device of a interface? the libc doesn't provide this api

@ibigbug
Copy link
Member

ibigbug commented Apr 16, 2024

You create a tun, and start a listener on the tun.

From client you send packet and bind iface to that tun.

And from the listener you expect receiving traffic.

Would this not work?

@Itsusinn
Copy link
Member Author

Itsusinn commented Apr 16, 2024

I've went through the issue list of clash.meta, and one of the most frequently asked issue is about tun, and it even got routing loop in some conditions. So I think we'd better develop some ways to avoid similar problems.

This pr actually dont cares about routing table. (auto-route do) It just make setting/getting mark/iface easier.

But indeed looping is a vital problem.

It highly depends how routing works.
On linux, tests are hard,we dont have a test environment that allows us use ip commands.
Even we have...

Anyway, i dont have any ideas about test. But i think this could be useful
https://chaochaogege.com/2021/08/01/57/

@VendettaReborn
Copy link
Contributor

VendettaReborn commented May 16, 2024

I think we can do the test for global proxy(tproxy/tun) by:
when a connection is intercepted via tproxy/tun, report error when the following conditions are satisfied:
a) the dst is one of the outbound handler's addresses; b)the process that owns the socket is clash-rs (since the lookup can be pretty slow, e.g. on linux, it has to lookup the proc file system, we shall only detect loop routing in debug mode or by some flags)

Thoughts? cc @ibigbug @Itsusinn

@ibigbug
Copy link
Member

ibigbug commented May 20, 2024

@VendettaReborn Do you see what are the common causes of the routing loop? Wrongly configed routing table? Can we do some validations during start up. And apply runtime checks as you described above?

@VendettaReborn
Copy link
Contributor

these cases are just a sum up of clash.meta's issues when i read through them:

  1. when clash.meta starts before network is connected
  2. when there is some network switch, such as the original default interface's ip address is lost when switching from work env to home env
  3. ... some wired problems

it' true that we shall prevent the routing loop as much as possible, even better with some proofs. But I still think some check may be helpful(maybe for debug?)

and the find process by socket shall also be supported for process-based rules.

@ibigbug
Copy link
Member

ibigbug commented May 20, 2024

Yeah I agree. I'll start working on the process rule soon.

@VendettaReborn VendettaReborn mentioned this pull request May 25, 2024
@Itsusinn Itsusinn mentioned this pull request Jun 12, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants