Implementation OOB and disOOB strategies from byeDPI #329

siriusfreak · 2024-11-13T15:21:44Z

This PR contains the implementation OOB and disOOB strategies from https://github.com/hufrea/byedpi/blob/main/desync.c

google-cla · 2024-11-13T15:21:49Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

fortuna · 2024-11-13T16:15:32Z

x/go.mod

 	golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d // indirect
 	google.golang.org/protobuf v1.33.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )
+
+replace github.com/Jigsaw-Code/outline-sdk => /Users/sirius/repos/outline-sdk/


Revert. For development, you can use Go workspaces instead.

fortuna · 2024-11-13T16:17:03Z

x/oob/dialer.go

+	opts        sockopt.TCPOptions
+	oobByte     byte
+	oobPosition int64
+	disOOB      bool


Shouldn't this be called just disorder?

Also, can we compose with the disorder from #323 instead of reproducing the logic here?

/cc @PeterZhizhin

I don't see a clear way how OOB and disorder can be piped together to achieve desired behavior.

That said, I believe we can extract the following API from disorder package:

func withDisorder(tcpOpts *scokopt.TCPOptions, targetHopLimit int, functionToCall func() error) error

The function will do the following:

Get the current hop limit.

Set the hop limit to target targetHopLimit.

Calls function functionToCall

Restores the hop limit.

After we have #324 merged, we will also have it wait for the socket send all bytes before resetting the hop limit.

The withDisorder function will be called in both OOB strategy and disorder strategy.

What do you think?

fortuna · 2024-11-13T16:22:38Z

x/oob/unix_ops.go

+	return syscall.Sendmsg(int(fd), data, nil, nil, flags)
+}
+
+func getSocketDescriptor(conn *net.TCPConn) (SocketDescriptor, error) {


Unused, delete. Also, the docs say this file descriptor may not be reliable: https://pkg.go.dev/net#TCPConn.File

fortuna · 2024-11-13T16:22:53Z

x/oob/windows_ops.go

+	return syscall.WSASend(syscall.Handle(fd), &wsaBuf[0], 1, &bytesSent, uint32(flags), nil, nil)
+}
+
+func getSocketDescriptor(conn *net.TCPConn) (SocketDescriptor, error) {


fortuna · 2024-11-13T16:24:12Z

x/oob/writer.go

+	disOOB bool,
+	delay time.Duration,
+) io.Writer {
+	return &oobWriter{conn: conn, opts: opts, oobPosition: oobPosition, oobByte: oobByte, disOOB: disOOB, delay: delay}


Suggested change

return &oobWriter{conn: conn, opts: opts, oobPosition: oobPosition, oobByte: oobByte, disOOB: disOOB, delay: delay}

// TODO: Implement io.ReaderFrom.

return &oobWriter{conn: conn, opts: opts, oobPosition: oobPosition, oobByte: oobByte, disOOB: disOOB, delay: delay}

fortuna · 2024-11-13T16:29:01Z

x/oob/writer.go

+		written = int(w.oobPosition)
+		secondPart[0] = tmp
+
+		if w.disOOB {


Clean up with a defer from the other if-block instead. It makes it clearer what it's cleaning up and ensures it's always called. Keeps all the disorder logic in one place.

fortuna · 2024-11-13T16:29:34Z

x/oob/writer.go

+	oobPosition int64
+	sd          SocketDescriptor
+	oobByte     byte // Byte to send as OOB
+	disOOB      bool // Flag to enable disOOB mode


Rename to disorder

fortuna · 2024-11-13T16:41:45Z

x/oob/unix_ops.go

The convention in go is to have the platform as a name suffix. Use ops_unix.go instead. Same for windows.

fortuna · 2024-11-13T16:43:49Z

x/oob/unix_ops.go

+type SocketDescriptor int
+
+func sendTo(fd SocketDescriptor, data []byte, flags int) (err error) {
+	return syscall.Sendmsg(int(fd), data, nil, nil, flags)


Use https://pkg.go.dev/golang.org/x/sys/unix#SendmsgN, so we can return the number of bytes written in case of error.

fortuna · 2024-11-13T16:48:06Z

x/oob/writer.go

+	var written int
+	var err error
+
+	if w.oobPosition > 0 && w.oobPosition < int64(len(data))-1 {


There's a corner case here. Probably worth capturing in the tests.

If the oob position aligns with the end of a write, it will never happen.

The problem is that you are dependent on an extra byte from the following write to push the OOB byte.

There's a way to fix that without requiring memory allocation. The SendMsg call can take the OOB as a separate buffer!

On Windows, the WSASend call takes a list of buffers, so just append the OOB to that list.

PeterZhizhin · 2024-11-13T17:33:25Z

x/oob/writer.go

+	var written int
+	var err error
+
+	if w.oobPosition > 0 && w.oobPosition < int64(len(data))-1 {


This somewhat copies the logic from split. Consider changing oobPosition to runAtPacketN from disorder: https://github.com/Jigsaw-Code/outline-sdk/blob/main/x/disorder/writer.go#L32

We can then pipe split together with oob to achieve oob byte to be sent at a defined call number.

For example: disorder:1|split:123. This transport will send first 123 bytes normally. Then will send the URG byte. Then will send the rest of the packet.

PeterZhizhin

Amazing, thank you!

PeterZhizhin · 2024-11-13T17:34:20Z

x/oob/writer.go

+	return &oobWriter{conn: conn, opts: opts, oobPosition: oobPosition, oobByte: oobByte, disOOB: disOOB, delay: delay}
+}
+
+func (w *oobWriter) Write(data []byte) (int, error) {


func (w *oobWriter) Write(data []byte) (written int, err error) {

PeterZhizhin · 2024-11-13T17:39:57Z

x/configurl/oob.go

+		}
+		params := config.URL.Opaque
+
+		splitStr := strings.Split(params, ":")


nit: maybe named arguments? https://github.com/Jigsaw-Code/outline-sdk/pull/324/files#diff-974d5fac785c258e952052dd1f6aa5b012bf2e4e43eb708de99420175a67e99b

But it's a matter of preference. @fortuna should know better.

Also, maybe you can make most arguments optional? I believe most users can use default timeout, default URG byte value, etc.

PeterZhizhin · 2024-11-13T17:43:48Z

x/oob/writer.go

+
+	// Use Control to execute Sendto on the file descriptor
+	var sendErr error
+	err = rawConn.Control(func(fd uintptr) {


nit: semantically it's a Write rather than Control. Feel free to keep as is, if you feel like it.

PeterZhizhin · 2024-11-13T17:45:51Z

x/oob/writer.go

+		var oldTTL int
+		if w.disOOB {
+			w.setTTL.Do(func() {
+				oldTTL, err = w.opts.HopLimit()


Take a look at the suggestion in https://github.com/Jigsaw-Code/outline-sdk/pull/329/files#r1840884687 that could simplify this and reduce code duplication.

siriusfreak added 20 commits November 2, 2024 12:56

OOB and disOOB implementation

639ff72

OOB and disOOB implementation

75fb663

OOB and disOOB implementation

a50b014

OOB and disOOB implementation

aaae1e5

OOB and disOOB implementation

a25de7a

OOB and disOOB implementation

250a731

OOB and disOOB implementation

ffb58dc

OOB and disOOB implementation

5cd7504

OOB and disOOB implementation

a0e3a8c

OOB and disOOB implementation

b65ad95

OOB and disOOB implementation

9e0d954

OOB and disOOB implementation

d16f104

OOB and disOOB implementation

84b696d

OOB and disOOB implementation

2e298a8

OOB and disOOB implementation

5a3532d

OOB and disOOB implementation

c8f3af7

OOB and disOOB implementation

175eda6

merge main:

8b9fff0

review fixes

afde648

add test

82bceb5

add CA

5b7b486

fortuna requested changes Nov 13, 2024

View reviewed changes

PeterZhizhin reviewed Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation OOB and disOOB strategies from byeDPI #329

Implementation OOB and disOOB strategies from byeDPI #329

siriusfreak commented Nov 13, 2024

google-cla bot commented Nov 13, 2024

fortuna Nov 13, 2024

fortuna Nov 13, 2024

PeterZhizhin Nov 13, 2024

fortuna Nov 13, 2024

fortuna Nov 13, 2024

fortuna Nov 13, 2024

fortuna Nov 13, 2024

fortuna Nov 13, 2024

fortuna Nov 13, 2024

fortuna Nov 13, 2024

fortuna Nov 13, 2024

PeterZhizhin Nov 13, 2024

PeterZhizhin left a comment

PeterZhizhin Nov 13, 2024

PeterZhizhin Nov 13, 2024

PeterZhizhin Nov 13, 2024

PeterZhizhin Nov 13, 2024

	return &oobWriter{conn: conn, opts: opts, oobPosition: oobPosition, oobByte: oobByte, disOOB: disOOB, delay: delay}
	// TODO: Implement io.ReaderFrom.
	return &oobWriter{conn: conn, opts: opts, oobPosition: oobPosition, oobByte: oobByte, disOOB: disOOB, delay: delay}

Implementation OOB and disOOB strategies from byeDPI #329

Are you sure you want to change the base?

Implementation OOB and disOOB strategies from byeDPI #329

Conversation

siriusfreak commented Nov 13, 2024

google-cla bot commented Nov 13, 2024

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

PeterZhizhin 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