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

Improve socket API coverage #1248

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

Conversation

mtaylor91
Copy link

Hi guys!

I've been playing around with writing a transparent proxy in Rust and found that the advanced networking API support for Linux is a bit lacking. This includes the original changes submitted for TPROXY support in #1025 (rebased on a more recent version of master), as well as changes to potentially close out #1064.

I'm new to rust, so any feedback is appreciated!

Thanks!

  • Mike

vi and others added 4 commits May 19, 2020 23:53
This beings `OrigDstAddrV4` and `OrigDstAddrV6` `recvmsg` messages
and associated socket options
`RecvOrigDstAddrV4` and `RecvOrigDstAddrV6`.

Should close nix-rust#1023.
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

This looks like a good start. In addition to my inline comments, all new functionality needs documentation, tests, and a CHANGELOG message.

@@ -529,6 +529,17 @@ pub enum ControlMessageOwned {
#[cfg(target_os = "linux")]
UdpGroSegments(u16),

#[cfg(any(
Copy link
Member

Choose a reason for hiding this comment

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

You should also enable the ORIGDSTADDR stuff for FreeBSD.

/// is overwritten by [`TPROXY`][1], which in turn may be used to
/// make a transparent proxy server.
///
/// [1]:https://www.kernel.org/doc/Documentation/networking/tproxy.txt
Copy link
Member

Choose a reason for hiding this comment

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

This is the wrong man page link. I can't find the correct man page for Linux, but here's the link for FreeBSD: ip(4) (note that the section for IP_ORIGDSTADDR has some typos; I just fixed them upstream and in time the link target will be rebuilt).

/// is overwritten by [`TPROXY`][1], which in turn may be used to
/// make a transparent proxy server.
///
/// [1]:https://www.kernel.org/doc/Documentation/networking/tproxy.txt
Copy link
Member

Choose a reason for hiding this comment

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

Also not a useful man page. I can't find any documentation on this control message type. Best not to include any man page link, then.

@@ -677,6 +698,52 @@ pub enum ControlMessage<'a> {
#[cfg(any(target_os = "freebsd", target_os = "dragonfly"))]
ScmCreds,

/// A message of type `IP_ORIGDSTADDR` which is triggered by setting
/// `RecvOrigDstAddrV4` socket option and is used to get original IPv4
/// UDP or TCP destination address when actual destination address
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure it works for TCP? In FreeBSD at least, it's only for UDP.

target_os = "macos",
target_os = "netbsd",
))]
Ipv4PacketInfo(&'a libc::in_pktinfo),
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add doc comments here?

target_os = "openbsd",
target_os = "netbsd",
))]
Ipv6PacketInfo(&'a libc::in6_pktinfo),
Copy link
Member

Choose a reason for hiding this comment

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

And a doc comment here, too?

target_os = "linux",
target_os = "android",
))]
OrigDstAddrV4(&'a libc::sockaddr_in),
Copy link
Member

Choose a reason for hiding this comment

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

There is no point in defining ControlMessage::OrigDstAddrV[46]. ControlMessage is for use with sendmsg only. recvmsg uses ControlMessageOwned instead.

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.

3 participants