Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Package to setup system wide HTTP(S) Proxy on Desktop Platforms #190
Package to setup system wide HTTP(S) Proxy on Desktop Platforms #190
Changes from 7 commits
e793764
bbd9f82
ba98871
bcb602e
4cb44eb
c11ab04
fdf398f
6a5680c
f48a095
cc694c9
044d794
422ca10
e2247d5
f91cafc
65db63e
f48f419
ed4c93b
ceec53f
5952a45
0faac88
7339b23
7b895a5
b6f3c97
6c59ad8
a04317e
7dd57d0
75ea37c
bdc6d2d
f8906a1
2826419
d3020d3
34ce3c3
0522adb
7cf4822
1ef992a
029a9c4
3f736c9
1baff92
3c593f9
9d8c9c9
b6558e2
2594efb
984833e
66658ac
1dedbf7
630fc56
4765b77
3f15492
4700f68
8855f10
8d5f311
c7e805c
4405b78
38e2535
9cfad5f
4fd18a6
428f291
7490cd1
00f414c
17aeeb7
b3c6f61
e49e0da
136c2db
3fbb4ed
e5b3ad1
7d22038
235ad1d
7e4abd9
d9c10c8
3da2ff2
05e3d14
d71b726
7cdec08
77faf78
b34db8e
35273de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't set the SOCKS proxy. We need to specify what type of proxy we are setting somehow.
Two options:
SetWebProxy
,SetSecureWebProxy
,SetSOCKSProxy
...SetProxy(ProxyTypeWeb, host, port)
Perhaps use HTTP/HTTPS instead of Web/SecureWeb.
I tend to prefer the first option because it allows us to potentially pass different options to different types, but passing the types is easier to implement. Up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fortuna I agree it would be helpful to export different functions to set different types of proxies and perhaps one method that sets them all at once such as
SetProxyAll()
. This way developers can choose one method if they just want to set a specific type of proxy.Also, I realized that HTTP proxy also allows for setting username/password for authentication. Do you thing we should expose that in the API?
Regarding socks proxy, I am researching on how to do it on Windows but if I can't get it working, we can just return
platform not supported
error forSetSocksProxy
on Windows.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fortuna I did some further research on this. The table below summarizes system-wide support for various types of proxies on desktop platforms.
Windows does not explicitly distinguish between HTTP and HTTPS proxy. Also, username/password authentication is not supported.
MacOS SOCKS does not seems to correctly support authentication even though it accepts credentials [ref].
Username/Pass authentication is not currently supported for HTTPS proxy [ref].
Given the poor support for username/password authentication, do you think it makes sense to add authentication option to the
sysproxy
package?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably return not supported on iOS. I think you can check GOOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't recreate the command. You already created it in the caller. I don't think runCommand is needed., since you can call cmd.Run() in the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably return not supported on Android. I think you can check GOOS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should enable manual mode after you set the proxies, so whoever observing this change can get the right values. Also in case one of them fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it support other modes? If so, do we need to backup the previous settings and restore it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jyyi1 The other modes are
auto
, andmanual
. Choosingmanual
, 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 whenUnsetProxy
is called.auto
uses a link to.pac
file that contains the proxy settings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reference to this setting we can link to?
I found this: https://github.com/GNOME/gsettings-desktop-schemas/blob/e4cce887ae6030840cda7efc1eb401df26f65bd8/schemas/org.gnome.system.proxy.gschema.xml.in#L8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also erase the individual proxy entries we added, in case someone else enables the manual mode later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove this file so we get a build error instead of run time error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fortuna this approach was suggested by @jyyi1 which does what you have in mind (we get a build error instead of run time error). The functions in this file gets called if the platform we are building on is not
darwin
orlinux
, orwindows
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, I believe I need to explicitly exclude
ios
andandroid
in the build constraints in this form://go:build darwin && !ios
insysproxy_darwin.go
file and//go:build linux && !android
insysproxy_linux.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to error, and return early if failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to error and return if error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this option mean? Do we need to unset it in our
UnsetProxy
function?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar question to the
MigrationProxy
configuration, do we need to unset it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to unset the registry here, so we don't leave it in an unusable state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.