WIP: disorder #1

derlaft · 2024-11-02T11:52:22Z

No description provided.

fortuna

Thanks for the contribution. This is exciting!

fortuna · 2024-11-02T14:59:49Z

transport/disorder/stream_dialer.go

Same comments as in the other PRs: let's keep all the new stuff in the x module until they have comprehensive tests and are validated. That also simplifies development.

fortuna · 2024-11-02T15:05:38Z

transport/disorder/writer.go

+		data = data[written:]
+	}
+	w.resetTTL.Do(func() {
+		err = syscall.SetsockoptInt(w.fd, syscall.IPPROTO_IP, syscall.IP_TTL, defaultTTL)


Ideally this should work on Windows as well, and both IPv4 and IPv6, so providers can easily specify strategies that work everywhere. It would be great if you can validate that.

By the way, I don't know if it helps, but there's

https://pkg.go.dev/golang.org/x/net/ipv4#Conn.SetTTL

https://pkg.go.dev/golang.org/x/net/ipv6#Conn.SetHopLimit

Awesome, didn't know about this package

Ideally this should work on Windows as well

I think for windows, mac and ios we would need a completely different implementation. I personally don't have any ways to test on those platforms, so I am thinking about disabling this code anywhere but on linux

Here is an example of a prototype I was working some time ago: https://github.com/Jigsaw-Code/outline-sdk/edit/fortuna-disorder/transport/split/writer.go

https://pkg.go.dev/golang.org/x/net/ipv4#Conn.SetTTL

this will only work on Linux btw (but will at least compile on other platforms, which is maybe a bit better)

Why do you think it will only work on Linux? I don't see any documentation saying that. They usually say when it doesn't work on other platforms.

hmm, I might have mis-read the code when I checked it, looks like the implementations are there

in any case, some testing is required on each platform, even if API is there there's no guarantee it works the same way

fortuna · 2024-11-02T15:07:06Z

transport/disorder/stream_dialer.go

+		return nil, fmt.Errorf("setting tcp NO_DELAY failed: %w", err)
+	}
+
+	file, err := tcpConn.File()


Use syscall.RawConn.Control instead

changed to use x/net

fortuna · 2024-11-02T15:07:39Z

transport/disorder/stream_dialer.go

+	// this strategy only works when we set TCP as a strategy
+	tcpConn, ok := innerConn.(*net.TCPConn)
+	if !ok {
+		return nil, fmt.Errorf("split strategy only works with direct TCP connections")


Suggested change

return nil, fmt.Errorf("split strategy only works with direct TCP connections")

return nil, fmt.Errorf("disorder strategy only works with direct TCP connections")

fortuna · 2024-11-02T15:09:04Z

transport/disorder/stream_dialer.go

+		return nil, fmt.Errorf("split strategy only works with direct TCP connections")
+	}
+
+	err = tcpConn.SetNoDelay(true)


Isn't this the default? Also, I'd rather not silently change settings. The base dialer provider should be responsible for that.

Perhaps we check if it's set and error if not. Though I don't see an easy way to check.

Add to the comments that the TCPConn must have NoDelay set.

Isn't this the default?

Honestly no idea. In Linux it's default now (but only for recent kernels, most android likely will need years to adopt the change), but in golang no idea.

I've added this in an attempt to debug something, but the issue was somewhere else, so this is no longer needed

It's enabled by default in Go, so no need to set it: https://pkg.go.dev/net#TCPConn.SetNoDelay

fortuna · 2024-11-03T16:28:55Z

x/disorder/stream_dialer.go

+		conn := ipv6.NewConn(conn)
+		oldTTL, _ = conn.HopLimit()
+		err = conn.SetHopLimit(ttl)
+	}


This should fail if we can't set the TTL

error check for both SetTTL and SetHopLimit is on lines 88-90

fortuna · 2024-11-03T16:29:21Z

x/disorder/stream_dialer.go

+	}
+
+	if oldTTL == 0 {
+		oldTTL = defaultTTL


What's the rationale of returning a default if it's zero? Why not return zero if it's zero?

I was not sure what happens by default (what getsockopt returns if ttl was not changed), also need to check what happens when we set ttl to 0

fortuna · 2024-11-03T16:30:53Z

x/disorder/stream_dialer.go

+		return nil, err
+	}
+
+	oldTTL, err := setTtl(innerConn, 1)


Please move this to the Writer, so the Writer can be used independent of the Dialer.

the idea was to fail early if something (like setting socket options) is not supported or lacks permissions

fortuna · 2024-11-03T16:32:20Z

x/disorder/stream_dialer.go

+
+// setTtl changes the socket TTL and returns the old value
+// socket must be `*net.TCPConn`
+func setTtl(conn net.Conn, ttl int) (oldTTL int, err error) {


Let's rename this to setHopLimit, which is a preferred term. TTL is actually confusing, since it is not related to time, hop limit is clearer.

fortuna · 2024-11-03T16:34:50Z

x/configurl/disorder.go

+		prefixBytesStr := config.URL.Opaque
+		prefixBytes, err := strconv.Atoi(prefixBytesStr)
+		if err != nil {
+			return nil, fmt.Errorf("prefixBytes is not a number: %v. Split config should be in split:<number> format", prefixBytesStr)


Suggested change

return nil, fmt.Errorf("prefixBytes is not a number: %v. Split config should be in split:<number> format", prefixBytesStr)

return nil, fmt.Errorf("Disorder config should take a number, got %v: %w, prefixBytesStr, err)

I'd move away from specifying the format scheme, since that can be changed in registration.

fortuna · 2024-11-03T16:40:24Z

transport/disorder/writer.go

+		data = data[written:]
+	}
+	w.resetTTL.Do(func() {
+		err = syscall.SetsockoptInt(w.fd, syscall.IPPROTO_IP, syscall.IP_TTL, defaultTTL)


Why do you think it will only work on Linux? I don't see any documentation saying that. They usually say when it doesn't work on other platforms.

derlaft · 2024-11-03T16:58:27Z

x/disorder/writer.go

+	w.resetTTL.Do(func() {
+		_, err = setTtl(w.conn, w.oldTTL)
+	})


before resetting ttl back to the "normal" value we might need to make sure send buffers are empty (= the first packet already departed). if golang is too fast this might change ttl back to normal too fast

other methods using setsocksopt also suffer from the same

fortuna · 2024-11-05T23:18:09Z

x/disorder/writer.go

+// var _ io.ReaderFrom = (*splitWriterReaderFrom)(nil)
+
+// TODO
+func NewWriter(conn net.Conn, prefixBytes int64, oldTTL int) io.Writer {


oldTTL is not needed. You can read it from the connection.

fortuna

Could you send this code to the official repository?
Thanks!

fortuna · 2024-11-07T00:05:00Z

x/disorder/stream_dialer.go

+
+// setHopLimit changes the socket TTL for IPv4 (or HopLimit for IPv6) and returns the old value
+// socket must be `*net.TCPConn`
+func setHopLimit(conn net.Conn, ttl int) (oldTTL int, err error) {


Can you use Jigsaw-Code#316 instead?

fortuna · 2024-11-07T23:05:55Z

x/disorder/writer.go

+// var _ io.ReaderFrom = (*splitWriterReaderFrom)(nil)
+
+// TODO
+func NewWriter(conn net.Conn, prefixBytes int64, oldTTL int) io.Writer {


Please pass an io.Writer and the new sockopt.TCPOptions instead of net.Conn.

fortuna · 2024-11-07T23:06:07Z

x/disorder/writer.go

+		data = data[written:]
+	}
+	w.resetTTL.Do(func() {
+		_, err = setHopLimit(w.conn, w.oldTTL)


Please use the new https://pkg.go.dev/github.com/Jigsaw-Code/outline-sdk/x/sockopt instead

derlaft · 2024-11-13T07:02:55Z

Superseded by Jigsaw-Code#323

@PeterZhizhin please check the last commits were pushed where you expected them to be pushed

fortuna suggested changes Nov 2, 2024

View reviewed changes

derlaft force-pushed the disorder branch 2 times, most recently from 161f18f to db99e8a Compare November 2, 2024 16:33

alim-shapiev pushed a commit that referenced this pull request Nov 3, 2024

WIP #1

dac62ca

fortuna reviewed Nov 3, 2024

View reviewed changes

derlaft commented Nov 3, 2024

View reviewed changes

derlaft force-pushed the disorder branch 2 times, most recently from d35a4da to a910d10 Compare November 3, 2024 17:20

fortuna reviewed Nov 5, 2024

View reviewed changes

fortuna suggested changes Nov 7, 2024

View reviewed changes

derlaft added 2 commits November 9, 2024 22:15

WIP: disorder: a new linux-only transport

5327e6a

WIP: disorder: review stuff

42211fa

PeterZhizhin force-pushed the disorder branch 2 times, most recently from 4cb2ac5 to e5480ac Compare November 9, 2024 22:43

Fix PR comments

36def4b

PeterZhizhin force-pushed the disorder branch from e5480ac to 36def4b Compare November 9, 2024 22:44

PeterZhizhin added 2 commits November 12, 2024 21:31

Review fixes

5d6424a

review fixes

b6aef3d

PeterZhizhin force-pushed the disorder branch from 2d98f62 to b6aef3d Compare November 13, 2024 01:23

derlaft closed this Nov 13, 2024

	return nil, fmt.Errorf("split strategy only works with direct TCP connections")
	return nil, fmt.Errorf("disorder strategy only works with direct TCP connections")

	return nil, fmt.Errorf("prefixBytes is not a number: %v. Split config should be in split:<number> format", prefixBytesStr)
	return nil, fmt.Errorf("Disorder config should take a number, got %v: %w, prefixBytesStr, err)

WIP: disorder #1

WIP: disorder #1

Conversation

derlaft commented Nov 2, 2024

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

Choose a reason for hiding this comment

derlaft Nov 2, 2024 • 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

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

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

derlaft commented Nov 13, 2024

derlaft Nov 2, 2024 •

edited

Loading