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 transparent proxy related unix socket options as possible #510

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

Conversation

cavivie
Copy link

@cavivie cavivie commented Apr 28, 2024

Changes:

  • Move (set_)ip_transparent to sys unix module
  • Add target_os=android/fuchisa for (set_)ip_transparent]
  • Deprecate (set_)ip_transparent in favour of (set_)ip_transparent_v4
  • Add (set_)ip_transparent_v6 for target_os=android/linux
  • Deprecate (set_)ip_freebind(ipv6) in favour of (set)ip_bindany_(v4|v6)
  • Add target_os=freebsd/openbsd for (set_)ip_bindany_(v4|v6)

@cavivie cavivie marked this pull request as draft April 28, 2024 10:04
@cavivie cavivie force-pushed the master branch 3 times, most recently from 285d1fb to f9ae19c Compare April 28, 2024 10:25
@cavivie cavivie marked this pull request as ready for review April 28, 2024 10:28
@cavivie
Copy link
Author

cavivie commented Jul 31, 2024

Will this request cause any problems? If so, please let me know. Otherwise, should I close this PR? I don't want to keep it open for a long time.

@Thomasdezeeuw
Copy link
Collaborator

Sorry I missed this pr, will try to review this later this week.

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you split this up into multiple commits?

  • the move to src/sys/unix.rs
  • adding target_os = "fuchsia"
  • adding (set_)ip_transparent_v6
  • adding (set_)ip_bindany_v4 (instead of ip_bindany)
  • adding (set_)ip_bindany_v6
  • deprecate ip_transparent in favour of ip_transparent_v4, we're slowly moving to _v4 and _v6 versions

@cavivie
Copy link
Author

cavivie commented Aug 1, 2024

Can you split this up into multiple commits?

  • the move to src/sys/unix.rs

  • adding target_os = "fuchsia"

  • adding (set_)ip_transparent_v6

  • adding (set_)ip_bindany_v4 (instead of ip_bindany)

  • adding (set_)ip_bindany_v6

  • deprecate ip_transparent in favour of ip_transparent_v4, we're slowly moving to _v4 and _v6 versions

OK, do this work later.

@cavivie
Copy link
Author

cavivie commented Aug 2, 2024

@Thomasdezeeuw Quick question, SO_BINDANY supports both v4 and v6 on OpenBSD, should we use (set_)so_bindany or (set_)so_bindany_(v4|v6)?

@cavivie cavivie force-pushed the master branch 4 times, most recently from bd2a678 to 59b2607 Compare August 2, 2024 11:37
@cavivie cavivie force-pushed the master branch 2 times, most recently from 8aa300f to 9ec630d Compare August 2, 2024 11:46
@cavivie
Copy link
Author

cavivie commented Aug 2, 2024

The last freebsd ci failed and I can't determine exactly why. The only thing that is different is that the old code was able to pass ci and the ci image version was freebsd-13-2, the new one seems to have changed to freebsd-13-2-release-amd.

@cavivie cavivie requested a review from Thomasdezeeuw August 2, 2024 12:16
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting up the commit, makes it much easier to review this way.

I'm not familiar with these options myself. But looking at the docs it seems like these options do roughly the same thing, they are just set differently on different OS? Is that correct?

If so, I think we should try and merge the functions such that the user doesn't need a bunch of cfg to make the correct call depending on the OS.

@cavivie
Copy link
Author

cavivie commented Aug 6, 2024

As the title says, we should not merge these behaviors, some options themselves are not the meaning of transparent proxy, they are necessary settings to complete transparent proxy, but not the only settings.
Although their functionalities overlap somewhat, due to their different design purposes and use cases, they have distinct focuses and implementations in practice.

UPD:
For example, IP_FREEBIND in Linux is more likeIP_BINDANY in FreeBSD, while IP_TRANSPARENT is mainly an option for transparent proxy, but this option is not available on FreeBSD, so the IP_BINDANY option is required.
https://github.com/torvalds/linux/blob/b446a2dae984fa5bd56dd7c3a02a426f87e05813/include/net/inet_sock.h#L421-L427

@cavivie cavivie requested a review from Thomasdezeeuw August 6, 2024 11:21
@cavivie
Copy link
Author

cavivie commented Aug 6, 2024

@Thomasdezeeuw Or we should merge (set_)freebind/(set_)ip_bindany(v4|v6)/(set)so_bindany, which should be equivalent options on different platforms. For Linux, we only keep (set)ip_transparent(v4|v6). WDYT?

UDP: if so, which one should we take?

#[cfg(all(feature = "all", any(target_os = "android", target_os = "fuchsia", target_os = "linux")))]
pub fn set_ip_bindany_v4() -> io::Result<()> { /*IP_FREEBIND*/ }

#[cfg(all(feature = "all", target_os = "freebsd"))]
pub fn set_ip_bindany_v4() -> io::Result<()> { /*IP_BINDANY*/ }

#[cfg(all(feature = "all", target_os = "openbsd"))]
pub fn set_ip_bindany_v4() -> io::Result<()> { /*SO_BINDANY*/ }

or

#[cfg(all(feature = "all", any(target_os = "android", target_os = "fuchsia", target_os = "linux", target_os = "freebsd", target_os = "openbsd")))]
pub fn set_ip_bindany_v4() -> io::Result<()> {
    #[cfg(any(target_os = "android", target_os = "fuchsia", target_os = "linux"))]
    unsafe { /*IP_FREEBIND*/ }
    
    #[cfg(target_os = "freebsd")]
    unsafe { /*IP_BINDANY*/ }
    
    #[cfg(target_os = "openbsd")]
    unsafe { /*SO_BINDANY*/ }
}

@Thomasdezeeuw
Copy link
Collaborator

I prefer the second option with proper document as to what options are being set, iff the options are actually the same. We have something similar for the keep alive options.

@cavivie
Copy link
Author

cavivie commented Aug 7, 2024

Now coded according to the new requirements. All changes can be seen in the PR description detail.

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.

2 participants