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

Adding UDP support to Socks5 dialer #257

Merged
merged 55 commits into from
Jul 29, 2024
Merged

Adding UDP support to Socks5 dialer #257

merged 55 commits into from
Jul 29, 2024

Conversation

amircybersec
Copy link
Contributor

@amircybersec amircybersec commented Jul 2, 2024

This PR add packet dialing (UDP) support to Socks5 dialer. The test runs a local UDP echo (ping-pong) server and socks5 server and routes the UDP packet through the local proxy server to the echo server and back. This was tested with several remote servers including soax service.

@amircybersec amircybersec requested a review from fortuna July 8, 2024 21:21
@amircybersec
Copy link
Contributor Author

@fortuna I'd appreciate it if you could review this PR.

@fortuna fortuna marked this pull request as ready for review July 9, 2024 15:09
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
transport/socks5/stream_dialer.go Outdated Show resolved Hide resolved
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Almost there.

transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved

pkt = pkt[addrLen:] // Skip the address
port := binary.BigEndian.Uint16(pkt[:2])
fmt.Printf("Received packet from %d:%d\n", addr, port)
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to remove all these prints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fortuna sure! I removed most of logging prints but will do a clean up at the end.

transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
transport/socks5/stream_dialer.go Outdated Show resolved Hide resolved
transport/socks5/packet_dialer.go Fixed Show resolved Hide resolved
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
transport/socks5/packet_dialer.go Outdated Show resolved Hide resolved
@amircybersec amircybersec requested a review from fortuna July 24, 2024 18:49

go func() {
err := proxySrv.Serve(listener)
defer listener.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to where the listener is created. Otherwise the server is never shutdown.

Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs to be moved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fortuna There's a small detail here. I can move defer listener.Close() to right after where we start listening but when the test finishes and listener closes the server goroutine throws a closed connection error. Unfortunately the server does not accept a context parameter to cancel it; Once solution is to consider an exception for server's net.ErrClosed error and basically ignore it. Not the cleanest approach though. Do you have any thoughts on this?

	listener, err := net.Listen("tcp", "127.0.0.1:0")
	require.NoError(t, err)
	defer listener.Close()
	proxyServerAddress := listener.Addr().String()

	go func() {
		err := proxySrv.Serve(listener)
		if !errors.Is(err, net.ErrClosed) && err != nil {
			require.NoError(t, err) // Assert no error if it's not the expected close error
		}
	}()

transport/socks5/packet_listener_test.go Show resolved Hide resolved
transport/socks5/packet_listener_test.go Outdated Show resolved Hide resolved
transport/socks5/packet_listener_test.go Outdated Show resolved Hide resolved
transport/socks5/packet_listener_test.go Outdated Show resolved Hide resolved
Comment on lines 178 to 179
addrLen := int(addrType[0])
fqdn := make([]byte, addrLen)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed if you keep the type of addrLen as byte.
Why are you converting to int?

@amircybersec
Copy link
Contributor Author

@fortuna thanks again for the feedback. I applied the latest rounds of comments.

@amircybersec amircybersec requested a review from fortuna July 25, 2024 23:50
Copy link
Contributor

@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.

Looks good. Just one minor thing. Your test is leaking a goroutine


go func() {
err := proxySrv.Serve(listener)
defer listener.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs to be moved.

Copy link
Contributor

@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 all the changes! This is great work!

@amircybersec
Copy link
Contributor Author

@fortuna I resolved the merge conflict in x/go.mod and x/go.sum; I am requiring following version in mod file as before but the build is still failing. It seems like x/config/socks5 is still pointing older transport/socks5 package.

github.com/Jigsaw-Code/outline-sdk v0.0.17-0.20240708052545-4f51b366fe75

Not sure what I am doing wrong...

@amircybersec amircybersec merged commit 19f5184 into main Jul 29, 2024
6 checks passed
@amircybersec amircybersec deleted the fortuna-socks branch July 29, 2024 20:32
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.

2 participants