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

feat: enable fwmark (SO_MARK) for outgoing sockets #202

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

Conversation

sabify
Copy link

@sabify sabify commented Aug 25, 2024

No description provided.

@sabify sabify requested a review from a team as a code owner August 25, 2024 17:41
service/tcp.go Outdated Show resolved Hide resolved
internal/integration_test/integration_test.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/main.go Outdated Show resolved Hide resolved
cmd/outline-ss-server/main.go Outdated Show resolved Hide resolved
@sbruens
Copy link

sbruens commented Sep 4, 2024

I have merged some large refactors we've been working on recently; apologies for the merge conflicts, and thank you for this contribution!

We may want to have some more dialer config options in future, so I suggest maybe a dialer wrapper in the config:

dialer:
  fwmark: ...

/cc @fortuna to weigh in on that

@fortuna
Copy link

fortuna commented Sep 4, 2024

@sabify thanks for the changes to inject the dialer. That's solid. As @sbruens pointed out, we need to pull the changes and I like the idea of putting the fwmark in a dialer config. That could be a place for other settings, like interface binding, routing, enable local network, etc)

@sabify
Copy link
Author

sabify commented Sep 9, 2024

@sbruens @fortuna I just made changes based on the refactored code.

service/tcp.go Outdated Show resolved Hide resolved
service/udp.go Outdated Show resolved Hide resolved
service/udp.go Outdated Show resolved Hide resolved
@fortuna
Copy link

fortuna commented Sep 11, 2024

By the way, for your use case, have you considered using a firewall rule based on the PID?

You can probably do things like:

sudo nft add rule filter output meta pid $PID mark set 0x1234

With iptables you can use cgroups and add the pid to it.

It's also possible to use network namespaces.

@sabify
Copy link
Author

sabify commented Sep 12, 2024

By the way, for your use case, have you considered using a firewall rule based on the PID?

You can probably do things like:

sudo nft add rule filter output meta pid $PID mark set 0x1234

With iptables you can use cgroups and add the pid to it.

It's also possible to use network namespaces.

This already adds too much complexity for even simple routing logic.

I made the changes to be linux-specific but it also opens room for other similar functionality in other platforms like freebsd's SO_USER_COOKIE.

Copy link

@fortuna fortuna 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 the changes. Looking good. I just have a few more tweaks.

service/udp_linux.go Outdated Show resolved Hide resolved
service/udp_linux.go Show resolved Hide resolved
service/tcp_other.go Show resolved Hide resolved
@sabify
Copy link
Author

sabify commented Sep 12, 2024

Changes applied.

@@ -243,7 +249,8 @@ func (s *SSServer) runConfig(config Config) (func() error, error) {
ciphers := service.NewCipherList()
ciphers.Update(cipherList)

sh := s.NewShadowsocksStreamHandler(ciphers)
tcpDialer := service.MakeValidatingTCPStreamDialer(onet.RequirePublicIP, 0)
Copy link

Choose a reason for hiding this comment

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

Shouldn't this take serviceConfig.Dialer.Fwmark?

Comment on lines +151 to +152
dialer := service.MakeTargetPacketListener(config.Dialer.Fwmark)
return s.NewShadowsocksPacketHandler(ciphers, dialer), nil
Copy link

Choose a reason for hiding this comment

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

Suggested change
dialer := service.MakeTargetPacketListener(config.Dialer.Fwmark)
return s.NewShadowsocksPacketHandler(ciphers, dialer), nil
listener := service.MakeTargetPacketListener(config.Dialer.Fwmark)
return s.NewShadowsocksPacketHandler(ciphers, listener), nil

@@ -259,7 +266,8 @@ func (s *SSServer) runConfig(config Config) (func() error, error) {
return err
}
slog.Info("UDP service started.", "address", pc.LocalAddr().String())
ph := s.NewShadowsocksPacketHandler(ciphers)
udpDialer := service.MakeTargetPacketListener(0)
Copy link

Choose a reason for hiding this comment

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

Same here, shouldn't it take the fwmark?

"syscall"
)

func SetFwmark(fd uintptr, fwmark uint) error {
Copy link

Choose a reason for hiding this comment

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

Take the syscall.RawConn as input instead of the fd to make it easier to use and remove the duplication.

@sabify
Copy link
Author

sabify commented Sep 28, 2024

@fortuna @sbruens I just gave you maintainer access to my fork and you are able to apply any of your concerns and code styles that fit best with the codebase. I may not be able to keep up with the rapid changes and requests in the codebase and this PR due to time constraints. Sorry for that and appreciate your work to land this feature. Thank you!

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