Package to setup system wide HTTP(S) Proxy on Desktop Platforms #190

amircybersec · 2024-02-26T20:39:40Z

sysproxy package sets up the system wide Web (HTTP/HTTPS) or SOCK proxy settings on:

MacOS
Linux (Gnome)
Windows

The package includes tests that sets the settings parameters and read them back to ensure changes have been properly made in the system settings.

The package exports the following methods:

SetWebProxy(host, port)
DisableWebProxy()
SetSOCKSProxy(host, port)
DisableSOCKSProxy()

This reverts commit e793764.

amircybersec · 2024-02-26T21:41:39Z

@jyyi1 would you be able to review this PR for me whenever you get a chance since Vini is out?

I tested this will x/examples/fyne-proxy example and I am planning to make a separate PR to add the changes to fyne-proxy example; however, if I made both PRs at the same time, the build check for the example PR would fail, since imported sysproxy package would still not be in the SDK before this PR merge.

Here are the changes to fyne-proxy example to set/unset system proxy:
https://github.com/amircybersec/outline-sdk/commit/24377b4a8a77bcbcf3bbc64e1ebb7f139024c87b

Please let me know if you have any thoughts on adding tests to this package...

jyyi1

Thanks @amircybersec ! This is very helpful, I just added some comments.

x/sysproxy/doc.go

x/sysproxy/sysproxy_darwin.go

x/sysproxy/sysproxy_windows.go

amircybersec · 2024-03-01T04:17:10Z

@jyyi1 Thanks a lot for the feedback. I applied your comments and make the suggested changes. I tested the changes across all platforms again to make sure I didn't break anything. Please let me know if there's any other areas for improvement.

jyyi1

Thanks! The general logic looks good, with some additional comments.

x/sysproxy/doc.go

x/sysproxy/sysproxy_darwin.go

jyyi1 · 2024-03-04T18:38:50Z

x/sysproxy/sysproxy_linux.go

+
+func UnsetProxy() error {
+	// Execute Linux specific commands to unset proxy
+	return execCommand("gsettings", "set", "org.gnome.system.proxy", "mode", "none")


Does it support other modes? If so, do we need to backup the previous settings and restore it here?

@jyyi1 The other modes are auto, and manual. Choosing manual, the system uses the last used proxy settings that can be manually overwritten. I can potentially backup the previous state before setting the proxy to the new settings and restore the system settings back to the previous state when UnsetProxy is called.

auto uses a link to .pac file that contains the proxy settings.

jyyi1 · 2024-03-04T18:53:39Z

x/sysproxy/sysproxy_windows.go

+	defer key.Close()
+
+	values := map[string]interface{}{
+		"MigrateProxy":  1,


What does this option mean? Do we need to unset it in our UnsetProxy function?

@jyyi1 Documentation for MigrateProxy DWORD was sparse. From what I understood by reading up one some resources I found (1, 2), It appears that this is related to how Internet Explore / Edge migrate their proxy settings.

I am a bit conflicted now since the docs say that IE sets this value to 1 after migrating the settings. BTW, I followed the way GreenTunnel was setting up Windows proxy params and tested the code on Windows successfully. But there could be corner and untested cases that I missed.

If you recall in the office, we looked at Chrome proxy settings was in sync with system proxy settings and we could see the settings switching in the Browser on Windows. I did not however test this with IE. I am looking further into this...

Thanks for the information. I think we can ignore IE for now because Microsoft has already ended its support on June 15, 2022.

For the Edge, since it's using Chromium, I believe it should behave the same as Chrome.

Let's remove this setting and make sure everything works fine.

@jyyi1 It seems like "ProxyHttp1.1": 0, setting is also related to IE according to this. I am going to remove this one too and test the changes on Windows.

x/sysproxy/sysproxy_windows.go

jyyi1 · 2024-03-04T18:59:53Z

x/sysproxy/sysproxy_windows.go

+			return fmt.Errorf("unsupported value type")
+		}
+		if err != nil {
+			return err


Do we need to unset the registry here, so we don't leave it in an unusable state?

Good point. That's definitely a much safer approach. I will make a copy of settings param before making changes and revert then back to previous state at UnsetProxy.

x/sysproxy/sysproxy_windows.go

fortuna

This is exciting!

x/sysproxy/doc.go

fortuna · 2024-03-04T17:09:34Z

x/sysproxy/sysproxy_darwin.go

+// getActiveNetworkInterface finds the active network interface using shell commands.
+// https://keith.github.io/xcode-man-pages/networksetup.8.html#listnetworkserviceorder
+func getActiveNetworkInterface() (string, error) {
+	cmd := "networksetup -listnetworkserviceorder | grep `route -n get 0.0.0.0 | grep 'interface' | cut -d ':' -f2` -B 1 | head -n 1 | cut -d ' ' -f2"


Actually, can you put that on a separate getDefaultInterface()?

And then provide a getNetworkServiceForInterface(interfaceName string) that does the networksetup -listnetworkserviceorder call?

That will be a lot more readable.

x/sysproxy/sysproxy_darwin.go

fortuna · 2024-03-04T17:18:23Z

x/sysproxy/sysproxy_darwin.go

+	cmdStr := fmt.Sprintf("networksetup -set%sproxy \"%s\" %s %s", ptype, interfaceName, ip, port)
+	cmd := exec.Command("bash", "-c", cmdStr)


Call networksetup directly instead. There's no need for bash here.

fortuna · 2024-03-04T17:18:31Z

x/sysproxy/sysproxy_darwin.go

+// https://keith.github.io/xcode-man-pages/networksetup.8.html#setwebproxystate
+func removeProxyCommand(ptype ProxyType, interfaceName string) error {
+	cmdStr := fmt.Sprintf("networksetup -set%sproxystate \"%s\" off", ptype, interfaceName)
+	cmd := exec.Command("bash", "-c", cmdStr)


fortuna · 2024-03-04T20:18:35Z

x/sysproxy/sysproxy_linux.go

+
+func UnsetProxy() error {
+	// Execute Linux specific commands to unset proxy
+	return execCommand("gsettings", "set", "org.gnome.system.proxy", "mode", "none")


We should also erase the individual proxy entries we added, in case someone else enables the manual mode later.

fortuna · 2024-03-04T20:19:25Z

x/sysproxy/sysproxy_darwin.go

+	httpsProxyType ProxyType = "secureweb"
+)
+
+func SetProxy(ip string, port string) error {


We should probably return not supported on iOS. I think you can check GOOS.

fortuna · 2024-03-04T20:19:52Z

x/sysproxy/sysproxy_linux.go

+	ftpProxyType   ProxyType = "ftp"
+)
+
+func SetProxy(ip string, port string) error {


We should probably return not supported on Android. I think you can check GOOS?

fortuna · 2024-03-04T20:37:57Z

x/sysproxy/sysproxy_darwin.go

+// setProxyCommand sets the specified type of proxy on the given network interface.
+// https://keith.github.io/xcode-man-pages/networksetup.8.html#getsecurewebproxy
+func setProxyCommand(ptype ProxyType, interfaceName string, ip string, port string) error {
+	cmdStr := fmt.Sprintf("networksetup -set%sproxy \"%s\" %s %s", ptype, interfaceName, ip, port)


Can you do a switch based on the type, instead of interpolating the command?
Like

switch proxyType { case ProxyTypeHTTP: cmd = exec.Command("networksetup", "-setwebproxy", interfaceName, host, port) ... }

That will be a lot cleaner to read.

x/sysproxy/sysproxy_windows.go

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

redundant likes that got pushed in accidentally Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

remove print statement Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

remove orphan line Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

redundant code Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

fortuna · 2024-03-25T18:02:12Z

x/sysproxy/README.md

+| --- | ----------- | ------ | ------ |
+| HTTP | Yes | Yes | Yes
+| HTTPS | Yes | Yes | Yes
+| SOCKS | No | Yes | Yes


I think you do support Windows, but it's SOCKS4?

I found this socks server package that implements sock4, 4a, and 5:

https://github.com/cybozu-go/usocksd?tab=readme-ov-file

I wanted to test socks proxy on all platforms and see what versions they support in reality. I have tested linux and mac with socks 5 and they work fine. I could not get windows working with socks5 and it only worked with sock4 server in my tests. I have not tested mac and linux against socks4 though...Maybe we can do this later as TODO item.

I have updated doc.go to reflect this in the documentation language

fortuna · 2024-03-25T18:03:38Z

x/sysproxy/README.md

+| SOCKS | No | Yes | Yes
+
+
+`SetupWebProxy` implementation in this package setups both HTTP and HTTPS proxy when they distinguished by the platform. `SetupSOCKSProxy` sets up SOCKS proxy that proxies all TCP-based connections.


You need to explain what setting a HTTP proxy and setting a HTTPS proxy mean.

x/sysproxy/README.md

fortuna · 2024-03-25T18:06:15Z

x/sysproxy/README.md

+
+On Windows, The client only supports SOCKS4 spec and cannot connecto SOCKS5 proxy.
+
+Support for FTP Proxy setting was not included due lack of adoption and usage. Username and password authentication was not included due to potential unreliability and untestd behavior. Please note that in SOCKS and HTTP proxy, credentials are communicated in plain text.


I'd say that authentication is not supported because the intended usage it to connect to a proxy running on localhost.

fortuna · 2024-03-25T18:29:38Z

x/sysproxy/sysproxy_darwin.go

+	}
+}
+
+func getProxySettings(p ProxyType, interfaceName string) (*proxySettings, error) {


I think this is dead code. We can remove the get stuff.

@fortuna I am using getProxySettings in tests to ensure parameters are properly set by the setProxy action.

fortuna · 2024-03-25T18:30:09Z

x/sysproxy/sysproxy_linux.go

+
+func ClearWebProxy() error {
+	// clear settings
+	if err := setProxySettings(proxyTypeHTTP, "127.0.0.1", "0"); err != nil {


Same: can we use empty strings? It's more obvious.

sure, I will test it first to make sure it works.

@fortuna it seems like setting the strings to empty value has unintended consequences. For example, on macos, if I do

networksetup -setwebproxy Wi-Fi "" ""

and then get the setting values, the port is returned as 0 but address is empty:

networksetup -getwebproxy Wi-Fi Enabled: Yes Server: Port: 0 Authenticated Proxy Enabled: 0

This breaks my tests since set and read-back values do not match.

I am concerned changing this causes some stability issues if not throughly tested on all platforms. Since my tests are currently passing on all platforms and I have also tested the proxy behavior on all platforms end-to-end, let's use a default IP and port instead of empty string, if that is okay with you

this is moot since we are not clearing the settings anymore.

fortuna · 2024-03-25T18:30:50Z

x/sysproxy/sysproxy_darwin.go

+	}
+
+	// Unset the web proxy and secure web proxy
+	if err := disableProxy(proxyTypeHTTP, activeInterface); err != nil {


Disable first, then clear the proxy. Otherwise someone may try to use it with the bogus address. Same for the other platforms.

@fortuna I was doing that before (disabling first) but setting proxy parameters automatically change the proxy state to manual and turn it on. I had to change the order otherwise the proxy stayed in ON state.

I think I saw this odd behavior on Linux, we might be able to disable proxy on Windows and MacOS before clearing the settings. Need to investigate this again...

@fortuna I confirm both on MacOS and Linux changing the proxy server and port automatically turns on the proxy. On Windows, we have to explicitly enable and disable the proxy settings for changes to take effect. This means we must clear the settings before disabling the proxy - the reverse order would leave the proxy state ON.

If we do not clear / change the setting, we can just disable the proxy.

Thanks for testing. If we can't clear the settings, perhaps we just disable it then?

We would have to rename the method to DisableXPRoxy()

x/sysproxy/sysproxy_linux.go

fortuna · 2024-03-25T18:32:50Z

x/sysproxy/README.md

+
+For example, on MacOS, if you have HTTP Proxy setup and visit a website over HTTPS, your traffic does NOT go through the proxy. 
+
+However, if only HTTPS proxy is setup, both HTTP and HTTPS requests go through the HTTPS proxy.


If you set HTTPS, HTTP will use it? That's very counter intuitive.
Does it work the same in the other platforms? It would help to characterize that.

@fortuna The proxy server running locally is HTTP proxy server in both cases.

In the first experiment, I only setup system's client HTTP proxy settings. In this case, only HTTP requests get routed through the proxy. If I setup system's client HTTPS proxy settings, both HTTP and HTTPS requests get routed through the proxy.

I believe this is just the way the system decides which type of request to route to the proxy. HTTPS is basically the super set in this design.

In our case, we don't care much about this since we are settings up both at the same time on all platforms and call it WebProxy which ends up routing both HTTP and HTTPS requests through the proxy.

Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com>

x/sysproxy/doc.go

fortuna · 2024-03-26T18:30:41Z

x/sysproxy/sysproxy_darwin.go

+	}
+
+	// Unset the web proxy and secure web proxy
+	if err := disableProxy(proxyTypeHTTP, activeInterface); err != nil {


Thanks for testing. If we can't clear the settings, perhaps we just disable it then?

We would have to rename the method to DisableXPRoxy()

jyyi1

a few tweaks

x/sysproxy/doc.go

x/sysproxy/sysproxy_test.go

x/sysproxy/sysproxy_windows.go

x/sysproxy/sysproxy_darwin.go

x/sysproxy/sysproxy_other.go

x/sysproxy/sysproxy_test.go

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

amircybersec · 2024-04-01T23:39:45Z

@fortuna & @jyyi1 Thanks a lot for all your feedback! I applied the latest round of suggestions and look forward to have this PR merged :)

fortuna

Thanks for the contribution!

amircybersec and others added 7 commits January 15, 2024 23:21

add dynamic config support

e793764

Merge branch 'Jigsaw-Code:main' into main

bbd9f82

Revert "add dynamic config support"

ba98871

This reverts commit e793764.

Merge branch 'Jigsaw-Code:main' into main

bcb602e

Merge branch 'Jigsaw-Code:main' into main

4cb44eb

add sys proxy for desktop platforms

c11ab04

do nothing on unsupported platforms

fdf398f

amircybersec marked this pull request as ready for review February 26, 2024 21:30

amircybersec requested a review from jyyi1 February 26, 2024 21:30

jyyi1 reviewed Feb 28, 2024

View reviewed changes

amircybersec added 7 commits February 29, 2024 13:11

add proxyType and clean up code for Darwin

6a5680c

remove safeclose and clean up the package doc

f48a095

moved package docs to doc.go

cc694c9

return an error if platform is not supported

044d794

refactored runCommad

422ca10

added docs and clean up

e2247d5

linux code refactor and cleanup

f91cafc

amircybersec requested a review from jyyi1 March 1, 2024 04:17

jyyi1 reviewed Mar 4, 2024

View reviewed changes

fortuna requested changes Mar 4, 2024

View reviewed changes

amircybersec and others added 8 commits March 5, 2024 17:46

Update x/sysproxy/doc.go

65db63e

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Update x/sysproxy/sysproxy_darwin.go

f48f419

redundant likes that got pushed in accidentally Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Update x/sysproxy/sysproxy_darwin.go

ed4c93b

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Update x/sysproxy/sysproxy_windows.go

ceec53f

remove print statement Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Update x/sysproxy/sysproxy_windows.go

5952a45

remove orphan line Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Update x/sysproxy/sysproxy_windows.go

0faac88

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Update x/sysproxy/sysproxy_windows.go

7339b23

redundant code Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Update x/sysproxy/sysproxy_windows.go

7b895a5

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

typo fix

4fd18a6

fortuna requested changes Mar 25, 2024

View reviewed changes

amircybersec and others added 6 commits March 25, 2024 15:17

Update x/sysproxy/README.md

428f291

Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com>

Update x/sysproxy/README.md

7490cd1

Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com>

update go.mod file + delete readme

00f414c

update go.doc file

17aeeb7

doc polish

b3c6f61

Merge branch 'main' into sysproxy-amir

e49e0da

amircybersec requested a review from fortuna March 26, 2024 17:01

fortuna reviewed Mar 26, 2024

View reviewed changes

amircybersec added 6 commits March 27, 2024 16:57

changed clear proxy to disable proxy

136c2db

test: check if proxy setting is cleared

3fbb4ed

disable proxy instead of clearing it

e5b3ad1

linux: diable proxy instead of clearing it

7d22038

linux: get proxy mode bugfix

235ad1d

linux: fixed enabled comparison bug

7e4abd9

amircybersec requested a review from fortuna April 1, 2024 19:26

jyyi1 reviewed Apr 1, 2024

View reviewed changes

x/sysproxy/doc.go Outdated Show resolved Hide resolved

x/sysproxy/sysproxy_test.go Outdated Show resolved Hide resolved

x/sysproxy/sysproxy_windows.go Outdated Show resolved Hide resolved

x/sysproxy/sysproxy_windows.go Outdated Show resolved Hide resolved

fortuna approved these changes Apr 1, 2024

View reviewed changes

x/sysproxy/sysproxy_darwin.go Outdated Show resolved Hide resolved

x/sysproxy/sysproxy_other.go Outdated Show resolved Hide resolved

x/sysproxy/sysproxy_test.go Outdated Show resolved Hide resolved

amircybersec and others added 8 commits April 1, 2024 16:07

Update x/sysproxy/doc.go

d9c10c8

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Update x/sysproxy/sysproxy_test.go

3da2ff2

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Update x/sysproxy/sysproxy_windows.go

05e3d14

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

Update x/sysproxy/sysproxy_windows.go

d71b726

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

doc update

7cdec08

change unset to diable in other platforms

77faf78

exclude ios explictly

b34db8e

return joined errors

35273de

fortuna approved these changes Apr 2, 2024

View reviewed changes

amircybersec merged commit 48d8a48 into Jigsaw-Code:main Apr 2, 2024
4 checks passed

	cmd := "networksetup -listnetworkserviceorder \| grep `route -n get 0.0.0.0 \| grep 'interface' \| cut -d ':' -f2` -B 1 \| head -n 1 \| cut -d ' ' -f2"
	cmd := "networksetup -listnetworkserviceorder \| grep "$(route -n get 0.0.0.0 \| grep 'interface' \| cut -d ':' -f2)" -B 1 \| head -n 1 \| cut -d ' ' -f2"

		cmdStr := fmt.Sprintf("networksetup -set%sproxy \"%s\" %s %s", ptype, interfaceName, ip, port)
		cmd := exec.Command("bash", "-c", cmdStr)

		\| SOCKS \| No \| Yes \| Yes


		`SetupWebProxy` implementation in this package setups both HTTP and HTTPS proxy when they distinguished by the platform. `SetupSOCKSProxy` sets up SOCKS proxy that proxies all TCP-based connections.


		On Windows, The client only supports SOCKS4 spec and cannot connecto SOCKS5 proxy.

		Support for FTP Proxy setting was not included due lack of adoption and usage. Username and password authentication was not included due to potential unreliability and untestd behavior. Please note that in SOCKS and HTTP proxy, credentials are communicated in plain text.


		For example, on MacOS, if you have HTTP Proxy setup and visit a website over HTTPS, your traffic does NOT go through the proxy.

		However, if only HTTPS proxy is setup, both HTTP and HTTPS requests go through the HTTPS proxy.

Package to setup system wide HTTP(S) Proxy on Desktop Platforms #190

Package to setup system wide HTTP(S) Proxy on Desktop Platforms #190

Conversation

amircybersec commented Feb 26, 2024 • edited Loading

amircybersec commented Feb 26, 2024

jyyi1 left a comment

Choose a reason for hiding this comment

amircybersec commented Mar 1, 2024

jyyi1 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

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

amircybersec Mar 25, 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

amircybersec Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 left a comment

Choose a reason for hiding this comment

amircybersec commented Apr 1, 2024

fortuna left a comment

Choose a reason for hiding this comment

amircybersec commented Feb 26, 2024 •

edited

Loading

amircybersec Mar 25, 2024 •

edited

Loading

amircybersec Mar 26, 2024 •

edited

Loading