feat: http local proxy implementation #50

GRbit · 2023-08-16T18:27:25Z

Implementation of a local HTTP proxy that can use transport.StreamDialer interface.

This is considered useful because many applications (e.g., browsers) can use proxy servers out of the box, and local proxy servers could be used to easily wrap a secure tunnel without additional user privileges.

To demo this functionality you may run local HTTP proxy as follows:

$ KEY=ss://ENCRYPTION_KEY@HOST:PORT/
$ go run github.com/Jigsaw-Code/outline-sdk/x/local-proxy@latest -transport "$KEY" -addr localhost:54321

or, to test before merging, being in outline-sdk/x folder

KEY=ss://ENCRYPTION_KEY@HOST:PORT/
go run ./local-proxy -transport "$KEY" -addr localhost:54321

The main question to discuss is how should proxy interface look like. So far I come up with this:

type Proxy interface {
	// GetAddr returns the address of the proxy server if it is configured to listen on a specific address.
	// If address is not configured and the server is not started, it returns an empty string.
	GetAddr() string

	// StartServer starts the proxy server. If no address is specified, it will listen on a random localhost port
	StartServer(addr ...string) error

	// StopServer stops the proxy server
	StopServer() error

	// GetError returns the error that caused the proxy server to stop or nil if the server is still running or not started
	GetError() error
}

Will be happy to receive any comments

fortuna

Thanks a lot for your contribution. It's exciting to see the first circumvention tool by an external contributor using our sdk!

x/go.mod

x/local-proxy/README.md

x/local-proxy/main.go

x/local-proxy/access_key.go

x/local-proxy/proxy/proxy.go

x/local-proxy/proxy/httpproxy/handler.go

Co-authored-by: Vinicius Fortuna <[email protected]>

x/go.mod

x/local-proxy/proxy/proxy.go

x/local-proxy/proxy/httpproxy/handler.go

fortuna · 2023-08-21T20:05:42Z

x/local-proxy/proxy/httpproxywrapper/handler.go

+
+		prx.address = prx.l.Addr().String()
+	default:
+		prx.l, err = net.Listen("tcp", prx.address)


Just pass the input address here. No need for all the logic above. The standard library already handles the different cases.

Thank you for noticing that.
I thought that it might be a problem if the proxy is started on :54321 because it'll not be a localhost address (like localhost:54321). But it causes problem only in case it's not a localhost address in browser settings, so in fact it's ok.

The only question I have is, maybe we should enforce listening on localhost by default? In another way, a proxy will be available to other devices on the network.

fortuna · 2023-08-21T20:08:17Z

x/local-proxy/proxy/httpproxywrapper/handler.go

+		}
+	}()
+
+	// sleep some time to check if server failed to start


Remove this wait. It's a race condition, and the caller can implement it if they really want it. Also, most of the errors should happen on the Listen.

Make sense, caller can just check an error after some time, so better remove it.

But can you tell me more about race condition thing? I think I'm missing something. Isn't it ok to send a message to channel from one goroutine and read from another?

Imagine it takes around 100ms for the error to happen. You may or may not return the error, depending on the mood of the machine. You may get different results when running the same code multiple times.

fortuna · 2023-08-22T21:12:07Z

x/local-proxy/proxy/proxy.go

+	// If address is not configured and the server is not started, it returns an empty string.
+	GetAddr() string
+	// StartServer starts the proxy server. If no address is specified, it will listen on a random localhost port
+	StartServer(addr string) error


Let's make StartServer a standalone function that returns a Proxy object. (Similar how Listen returns a Listener). That way it will never be in an invalid state.

Sounds nice. But I'm not sure about "never be in an invalid state" part. So, I can see StartServer (or better StartProxy) function returning a Proxy with interface like this:

type Proxy interface { Addr() string // removing "Get" like in Listener Stop() error GetError() error // not sure if it should be "Error()", because it will implement an "error" interface }

I can't come up with an example when things ca go wrong, but it seems for me that they can, and the proxy can broke for some reason. Will it be what you meant or is there something else I should do about never being in invalid state?

UPD: just in case, does this StartProxy definition looks good to you?
func StartServer(d transport.StreamDialer, addr string) (Proxy, error)

Yes, something like that.

x/local-proxy/README.md

fortuna

I figure it's taking a lot more time for me to review this a bit each day with lots of context switches. I think the code is ok, I'll merge and then send another PR with the refactorings I have in mind so we can move forward more quickly,

fortuna · 2023-08-24T15:16:54Z

x/local-proxy/proxy/httpproxywrapper/handler.go

+		}
+	}()
+
+	// sleep some time to check if server failed to start


Imagine it takes around 100ms for the error to happen. You may or may not return the error, depending on the mood of the machine. You may get different results when running the same code multiple times.

fortuna · 2023-08-24T16:27:07Z

x/local-proxy/main.go

This can move to x/examples/local-proxy

fortuna · 2023-08-24T16:27:36Z

x/local-proxy/main.go

+	"github.com/Jigsaw-Code/outline-sdk/x/local-proxy/proxy/httpproxywrapper"
+)
+
+func makeStreamDialer(transportConfig string) (transport.StreamDialer, error) {


You can now use MakeStreamDialer in x/examples/internal/config

fortuna · 2023-08-24T16:33:40Z

x/local-proxy/proxy/httpproxywrapper/callbacks.go

+	"github.com/go-httpproxy/httpproxy"
+)
+
+func BasicAuth(usersPasswords map[string]string) func(ctx *httpproxy.Context, authType, user, pass string) bool {


This is never used. Delete

fortuna · 2023-08-24T16:38:10Z

x/local-proxy/main.go

+		if err = proxy.StopServer(); err != nil {
+			log.Println("Failed to stop server: ", err.Error())
+		}


Suggested change

if err = proxy.StopServer(); err != nil {

log.Println("Failed to stop server: ", err.Error())

}

if err = listener.Close(); err != nil {

log.Println("Failed to stop server: ", err.Error())

}

fortuna · 2023-08-24T16:46:51Z

x/local-proxy/proxy/proxy.go

+	// If address is not configured and the server is not started, it returns an empty string.
+	GetAddr() string
+	// StartServer starts the proxy server. If no address is specified, it will listen on a random localhost port
+	StartServer(addr string) error


Yes, something like that.

fortuna · 2023-08-24T16:52:09Z

x/local-proxy/main.go

+	proxy := httpproxy.NewConnectHandler(dialer)
+	proxy.OnError = httpproxy.OnError
+
+	if err := proxy.StartServer(*addrFlag); err != nil {
+		log.Fatal("Proxy server failed")
+	}


Ah, I lost the comment. I'd like to see the code like this:

Suggested change

proxy := httpproxy.NewConnectHandler(dialer)

proxy.OnError = httpproxy.OnError

if err := proxy.StartServer(*addrFlag); err != nil {

log.Fatal("Proxy server failed")

}

listener := net.Listen("tcp", *addrFlag)

if err := http.Serve(listener, httpproxy.NewHandler(dialer)); err != nil {

log.Fatal("Proxy server failed")

}

This way it's clear what is provided by the code. Also, the caller may want to provide a different listener (e.g. using TLS).

You can stop the server by calling listener.Close()

We can provide convenience functions later. But we need the http.Handler building block.

fortuna

Let's get this merged so we can iterate

GRbit · 2023-08-26T09:30:40Z

Thank you for your work on this and all the time you've put in. Indeed, it's incredibly difficult to review PR with such a difference in time zones.

proxy interface; http local proxy implementation

77a2d77

fortuna self-requested a review August 17, 2023 20:58

fortuna requested changes Aug 17, 2023

View reviewed changes

fortuna marked this pull request as ready for review August 17, 2023 21:24

GRbit and others added 3 commits August 18, 2023 10:34

Update x/local-proxy/README.md

0495502

Co-authored-by: Vinicius Fortuna <[email protected]>

Update x/local-proxy/main.go

6d65bd1

Co-authored-by: Vinicius Fortuna <[email protected]>

local-prox: println -> log.Println; rm access_key.go

2be283e

fortuna requested changes Aug 18, 2023

View reviewed changes

x/go.mod Outdated Show resolved Hide resolved

x/local-proxy/proxy/proxy.go Outdated Show resolved Hide resolved

x/local-proxy/proxy/httpproxy/handler.go Outdated Show resolved Hide resolved

using httpproxy library as 3rd party dependency

82769f3

fortuna requested changes Aug 22, 2023

View reviewed changes

local-proxy: readme typo; remove start timeout

a2e62ac

fortuna reviewed Aug 24, 2023

View reviewed changes

Merge branch 'main' into local-proxies-draft

0d27687

fortuna reviewed Aug 24, 2023

View reviewed changes

fortuna approved these changes Aug 24, 2023

View reviewed changes

fortuna merged commit 605df87 into Jigsaw-Code:main Aug 24, 2023
4 checks passed

fortuna mentioned this pull request Aug 24, 2023

chore: clean up local-proxy #57

Merged

GRbit deleted the local-proxies-draft branch August 26, 2023 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: http local proxy implementation #50

feat: http local proxy implementation #50

GRbit commented Aug 16, 2023

fortuna left a comment

fortuna Aug 21, 2023

GRbit Aug 23, 2023

fortuna Aug 21, 2023

GRbit Aug 23, 2023

fortuna Aug 24, 2023

fortuna Aug 22, 2023

GRbit Aug 23, 2023 •

edited

Loading

fortuna Aug 24, 2023

fortuna left a comment

fortuna Aug 24, 2023

fortuna Aug 24, 2023

fortuna Aug 24, 2023

fortuna Aug 24, 2023

fortuna Aug 24, 2023

fortuna Aug 24, 2023

fortuna Aug 24, 2023

fortuna Aug 24, 2023

fortuna left a comment

GRbit commented Aug 26, 2023

feat: http local proxy implementation #50

feat: http local proxy implementation #50

Conversation

GRbit commented Aug 16, 2023

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

GRbit Aug 23, 2023 • edited Loading

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

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

GRbit commented Aug 26, 2023

GRbit Aug 23, 2023 •

edited

Loading