Use the IP PacketConn to specify the local proxy IP #110

bemasc · 2022-01-05T14:09:00Z

This allows the proxy to determine the destination address of incoming
UDP packets and specify the source address of outgoing UDP packets. It
requires duplicating the UDP service because it uses the x/net/ipv[4,6]
packages, which require us to know the address family of each incoming
packet before it is received.

bemasc · 2022-01-05T14:09:44Z

This is an alternative to #109. I haven't figured out how to test it yet, and it doesn't have any automated tests yet (although the existing tests do pass).

This allows the proxy to determine the destination address of incoming UDP packets and specify the source address of outgoing UDP packets. It requires duplicating the UDP service because it uses the x/net/ipv[4,6] packages, which require us to know the address family of each incoming packet before it is received.

fortuna · 2022-01-05T15:49:58Z

net/net.go

+		if cm != nil {
+			dst = cm.Dst
+		} else if runtime.GOOS != "windows" {
+			err = errors.New("control data is missing")


Why is this error for Windows only? It seems that for any system the control data would be missing at this point.

Same for ipv6.

The x/net/ipv* libraries do not support control data on Windows, so cm is documented to be nil there. On other OSes, it should never be nil.

fortuna · 2022-01-05T16:04:51Z

service/udp.go

@@ -135,7 +135,7 @@ func (s *udpService) Serve(clientConn net.PacketConn) error {
 			}()

 			// Attempt to read an upstream packet.
-			clientProxyBytes, clientAddr, err := clientConn.ReadFrom(cipherBuf)
+			clientProxyBytes, clientAddr, proxyIP, err := onet.ReadFromWithDst(clientConn, cipherBuf)


This change is intrusive and adds another thing to keep track of. I think we can do better by composing behavior.
Instead of returning the proxyIP, can you return a "bound" clientConn that holds the proxyIP and clientAddr internally and uses that for all the subsequent writes and reads? .
This way we don't need to pass proxyIP and clientAddr around.

You will still need to expose those fields for makeNATKey though.

A full version of this would require moving the NAT code from service/ to net/, leaving the UDP code in service/ looking much like the current TCP code. I see the appeal of this, but after attempting to implement it I think it's not worthwhile. In particular, the multiplexing of blocking reads from multiple underlying sockets generates a lot of complexity, especially if we try to avoid allocating a fresh buffer for every packet (as in the current design).

After considering various options, I think the current design provides the best balance of simplicity and performance. I've reorganized the functionality to present a cleaner interface, but I have not attempted to provide any "virtual Conn" wrapping.

fortuna · 2022-01-05T16:07:59Z

service/udp.go

@@ -420,7 +430,7 @@ func (m *natmap) Close() error {
 var maxAddrLen int = len(socks.ParseAddr("[2001:db8::1]:12345"))

 // copy from target to client until read timeout
-func timedCopy(clientAddr net.Addr, clientConn net.PacketConn, targetConn *natconn,
+func timedCopy(clientAddr *net.UDPAddr, proxyIP net.IP, clientConn net.PacketConn, targetConn *natconn,


This could take the bound clientConn and the targetConn only, following my suggestion.

I've changed this to take a shadowsocksUDPWriter.

fortuna · 2022-01-05T16:31:52Z

net/net.go

+// that are bound to `0.0.0.0` or `::`.
+func ReadFromWithDst(conn net.PacketConn, b []byte) (n int, src *net.UDPAddr, dst net.IP, err error) {
+	var tmpSrc net.Addr
+	if conn.LocalAddr().Network() == "udp4" {


I'm not a fan of this type switch because you already have per type code when you create the UDP services and because the PacketConn here is an incomplete type spec: you only accept the udp4 and udp6 ones.

Instead, you could introduce the concept of a BindingPacketConn with an Accept() method that returns the BoundPacketConn that I mention below.

You can have separate factory functions for IP 4 and 6, so you won't need the type switch. The higher-level concept will make the code easier to read because it will allow us to understand the different concerns separately.

OK, I've fully separated the IPv4 and IPv6 factories.

fortuna · 2022-01-05T16:35:23Z

integration_test/integration_test.go

@@ -72,7 +72,7 @@ func startTCPEchoServer(t testing.TB) (*net.TCPListener, *sync.WaitGroup) {
 }

 func startUDPEchoServer(t testing.TB) (*net.UDPConn, *sync.WaitGroup) {


It would be great to exercise the new behavior somehow, so we can be confident it's not breaking things.
Also add IPv6 testing.

For testing, perhaps you can listen on two separate IPs, like 127.0.0.1 and 127.0.0.2?

I was able to test using 127.0.0.1 and 127.0.0.2, but I can't easily write IPv6 tests because (1) IPv6 doesn't have an analogue to 127.0.0.2 and (2) my workstation (and presumably some other potential tests environments) has no IPv6 support.

fortuna

This is actually a hard PR to review, I'm sorry for the delay.
Writing simple, maintainable code is difficult.

fortuna · 2022-01-24T16:03:50Z

net/udp_any.go

+	}
+	if cm != nil {
+		dst = cm.Dst
+	} else if runtime.GOOS != "windows" {


I think a better way to do this is to add a build header flag, like // +build darwin linux

This way we force people to not use it on windows or js. They will have to pick another implementation, and that's a decision to be made at development time.

OK, done. Note that this means outline-ss-server no longer supports Windows at all, instead of degrading gracefully to the old behavior.

fortuna · 2022-01-24T17:32:26Z

net/udp_any.go

+// from the expected source IP.
+type UDPAnyConn interface {
+	net.PacketConn
+	ReadToFrom(p []byte) (n int, src *net.UDPAddr, dst net.IP, err error)


Do we need to couple read and write?

I've been finding a lot cleaner to keep them separate when possible.

See my comment about a Shadowsocks writer below.

As noted above, we also need a few other PacketConn methods, so it's not as simple as decomposing this into a Reader and a Writer.

fortuna · 2022-01-24T17:32:48Z

service/udp.go

@@ -475,7 +485,7 @@ func timedCopy(clientAddr net.Addr, clientConn net.PacketConn, targetConn *natco
 			if err != nil {
 				return onet.NewConnectionError("ERR_PACK", "Failed to pack data to client", err)
 			}
-			proxyClientBytes, err = clientConn.WriteTo(buf, clientAddr)
+			proxyClientBytes, err = clientConn.WriteToFrom(buf, clientAddr, proxyIP)


This is getting complicated. I shouldn't have to care about the clientAddr and proxyIP to read the shadowsocks piping logic. I just need to know where to write to.

Perhaps we can introduce a Client Writer, which holds the client address and proxy IP and exposes a write(buf) method. This way the piping logic can call clientWriter.Write(buf).

There is an issue with debugUDPAddr, which needs the client address, so we would have to keep passing it.

However, perhaps we implement a ShadowsocksWriter.ReadFrom(targetConn) method, which implements the forwarding of one packet and can call an internal writeToClient(clientConn) method that encapsulates the clientAddr and proxyIP. This would remove the need for the awkward unnamed function we have in the loop. We could potentially implement the ControlMessage logic there too, to reduce layering (it will be easy to refactor that later if needed).

You will need to move the handling of the expiration error to outside the call.

ShadowsocksWriter can hold pkt, clientConn, clientAddr, proxyIP.

I believe the new layering would separate concerns and improve readability and maintenance.

OK, I've introduced boundWriter and shadowsocksUDPWriter types to separate these concerns more clearly.

fortuna · 2022-01-24T17:33:27Z

net/udp_any.go

+// these cases, net.PacketConn is not sufficient to enable sending a reply
+// from the expected source IP.
+type UDPAnyConn interface {
+	net.PacketConn


Why does it need to implement PacketConn? Do we ever call the PacketConn methods outside this class? Seems like an easy mistake to make, let's prevent that.

At a minimum, we need Close() and SetReadDeadline(). The tests additionally rely on being able to call LocalAddr(), and use a unified fake for UDPAnyConn and PacketConn. If you prefer, we could add these three methods, hide the underlying PacketConn, and add a separate test double.

Note that all the PacketConn methods really do work correctly, so this type is accurate.

NOUIY · 2022-01-26T23:06:20Z

go.mod

@@ -8,6 +8,7 @@ require (
 	github.com/shadowsocks/go-shadowsocks2 v0.1.4-0.20201002022019-75d43273f5a5
 	github.com/stretchr/testify v1.6.1
 	golang.org/x/crypto v0.0.0-20201016220609-9e8e0b390897
+	golang.org/x/net v0.0.0-20211216030914-fe4d6282115f // indirect


name: Codecov
uses: codecov/[email protected]

bemasc requested a review from fortuna January 5, 2022 14:09

bemasc force-pushed the bemasc-ipconn branch from 2acd026 to e73f380 Compare January 5, 2022 14:20

fortuna requested changes Jan 5, 2022

View reviewed changes

NOUIY approved these changes Jan 18, 2022

View reviewed changes

Ben Schwartz added 2 commits January 20, 2022 16:50

Encapsulate local IP function in new interface

af846a0

Fix data race in the new integration test

125c4e5

bemasc marked this pull request as ready for review January 21, 2022 01:59

bemasc requested a review from fortuna January 21, 2022 01:59

fortuna requested changes Jan 24, 2022

View reviewed changes

Add shadowsocksUDPWriter and boundWriter

e6b4fec

bemasc requested a review from fortuna January 25, 2022 22:46

NOUIY approved these changes Jan 26, 2022

View reviewed changes

sbruens closed this Mar 5, 2024

sbruens deleted the bemasc-ipconn branch March 5, 2024 22:57

fortuna restored the bemasc-ipconn branch March 6, 2024 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the IP PacketConn to specify the local proxy IP #110

Use the IP PacketConn to specify the local proxy IP #110

bemasc commented Jan 5, 2022

bemasc commented Jan 5, 2022

fortuna Jan 5, 2022

bemasc Jan 5, 2022

fortuna Jan 5, 2022

bemasc Jan 20, 2022

fortuna Jan 5, 2022

bemasc Jan 25, 2022

fortuna Jan 5, 2022

bemasc Jan 20, 2022

fortuna Jan 5, 2022

fortuna Jan 5, 2022

bemasc Jan 20, 2022

fortuna left a comment

fortuna Jan 24, 2022

bemasc Jan 25, 2022

fortuna Jan 24, 2022

bemasc Jan 25, 2022

fortuna Jan 24, 2022

bemasc Jan 25, 2022 •

edited

Loading

fortuna Jan 24, 2022

bemasc Jan 25, 2022

NOUIY Jan 26, 2022

		@@ -72,7 +72,7 @@ func startTCPEchoServer(t testing.TB) (net.TCPListener, sync.WaitGroup) {
		}

		func startUDPEchoServer(t testing.TB) (net.UDPConn, sync.WaitGroup) {

Use the IP PacketConn to specify the local proxy IP #110

Use the IP PacketConn to specify the local proxy IP #110

Conversation

bemasc commented Jan 5, 2022

bemasc commented Jan 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortuna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bemasc Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bemasc Jan 25, 2022 •

edited

Loading