-
-
Notifications
You must be signed in to change notification settings - Fork 174
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
Honor http_proxy environment variables #1111
base: master
Are you sure you want to change the base?
Changes from all commits
5a42813
9565c21
3abce76
32a9d60
2a8df33
8081074
0d3e09c
c73f7f1
81abb92
d6edb12
c3a7575
fb46808
aa1c8c2
86b1f6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -271,10 +271,22 @@ main(int argc, char **argv) | |
if (has_arg(argc, argv, "http-proxy")) { | ||
sentry_options_set_proxy(options, "http://127.0.0.1:8080"); | ||
} | ||
if (has_arg(argc, argv, "http-proxy-auth")) { | ||
sentry_options_set_proxy( | ||
options, "http://user:[email protected]:8080"); | ||
} | ||
|
||
if (has_arg(argc, argv, "socks5-proxy")) { | ||
sentry_options_set_proxy(options, "socks5://127.0.0.1:1080"); | ||
} | ||
if (has_arg(argc, argv, "socks5-proxy-auth")) { | ||
sentry_options_set_proxy( | ||
options, "socks5://user:[email protected]:1080"); | ||
} | ||
|
||
if (has_arg(argc, argv, "proxy-from-env")) { | ||
sentry_options_set_read_proxy_from_environment(options, true); | ||
} | ||
|
||
sentry_init(options); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -276,6 +276,29 @@ sentry_options_get_http_proxy(const sentry_options_t *opts) | |
return sentry_options_get_proxy(opts); | ||
} | ||
|
||
void | ||
sentry_options_set_read_proxy_from_environment(sentry_options_t *opts, int val) | ||
{ | ||
opts->read_proxy_from_environment = !!val; | ||
} | ||
|
||
int | ||
sentry_options_get_read_proxy_from_environment(const sentry_options_t *opts) | ||
{ | ||
return opts->read_proxy_from_environment; | ||
} | ||
|
||
void | ||
sentry__set_proxy_from_environment(sentry_options_t *opts) | ||
{ | ||
const char *https_proxy = getenv("https_proxy"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we support upper-case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am fine with only accepting the lower-case versions, but since this is a "thing", we should clearly document the behavior. |
||
if (https_proxy) { | ||
sentry_options_set_proxy(opts, https_proxy); | ||
} else { | ||
sentry_options_set_proxy(opts, getenv("http_proxy")); | ||
JoshuaMoelans marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
void | ||
sentry_options_set_ca_certs(sentry_options_t *opts, const char *path) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,8 @@ typedef struct { | |
sentry_dsn_t *dsn; | ||
wchar_t *user_agent; | ||
wchar_t *proxy; | ||
wchar_t *proxy_username; | ||
wchar_t *proxy_password; | ||
sentry_rate_limiter_t *ratelimiter; | ||
HINTERNET session; | ||
HINTERNET connect; | ||
|
@@ -51,10 +53,47 @@ sentry__winhttp_bgworker_state_free(void *_state) | |
sentry__dsn_decref(state->dsn); | ||
sentry__rate_limiter_free(state->ratelimiter); | ||
sentry_free(state->user_agent); | ||
sentry_free(state->proxy_username); | ||
sentry_free(state->proxy_password); | ||
sentry_free(state->proxy); | ||
sentry_free(state); | ||
} | ||
|
||
// Function to extract and set credentials TODO replace with `sentry__url_parse` | ||
void | ||
set_proxy_credentials(winhttp_bgworker_state_t *state, const char *proxy) | ||
{ | ||
const char *at_sign = strchr(proxy, '@'); | ||
if (at_sign) { | ||
const char *credentials = proxy + 7; // Skip "http://" | ||
char *colon = strchr(credentials, ':'); | ||
if (colon && colon < at_sign) { | ||
size_t user_len = colon - credentials; | ||
size_t pass_len = at_sign - colon - 1; | ||
char *user = (char *)malloc(user_len + 1); | ||
char *pass = (char *)malloc(pass_len + 1); | ||
strncpy(user, credentials, user_len); | ||
user[user_len] = '\0'; | ||
strncpy(pass, colon + 1, pass_len); | ||
pass[pass_len] = '\0'; | ||
|
||
// Convert user and pass to LPCWSTR | ||
int user_wlen = MultiByteToWideChar(CP_UTF8, 0, user, -1, NULL, 0); | ||
int pass_wlen = MultiByteToWideChar(CP_UTF8, 0, pass, -1, NULL, 0); | ||
wchar_t *user_w = (wchar_t *)malloc(user_wlen * sizeof(wchar_t)); | ||
wchar_t *pass_w = (wchar_t *)malloc(pass_wlen * sizeof(wchar_t)); | ||
MultiByteToWideChar(CP_UTF8, 0, user, -1, user_w, user_wlen); | ||
MultiByteToWideChar(CP_UTF8, 0, pass, -1, pass_w, pass_wlen); | ||
|
||
state->proxy_username = user_w; | ||
state->proxy_password = pass_w; | ||
|
||
sentry_free(user); | ||
sentry_free(pass); | ||
} | ||
} | ||
} | ||
|
||
static int | ||
sentry__winhttp_transport_start( | ||
const sentry_options_t *opts, void *transport_state) | ||
|
@@ -71,7 +110,12 @@ sentry__winhttp_transport_start( | |
// ensure the proxy starts with `http://`, otherwise ignore it | ||
if (opts->proxy && strstr(opts->proxy, "http://") == opts->proxy) { | ||
const char *ptr = opts->proxy + 7; | ||
Comment on lines
110
to
112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apparently our Windows transport doesn't take |
||
const char *at_sign = strchr(ptr, '@'); | ||
const char *slash = strchr(ptr, '/'); | ||
if (at_sign && (!slash || at_sign < slash)) { | ||
ptr = at_sign + 1; | ||
set_proxy_credentials(state, opts->proxy); | ||
} | ||
if (slash) { | ||
char *copy = sentry__string_clone_n(ptr, slash - ptr); | ||
state->proxy = sentry__string_to_wstr(copy); | ||
|
@@ -103,6 +147,7 @@ sentry__winhttp_transport_start( | |
SENTRY_WARN("`WinHttpOpen` failed"); | ||
return 1; | ||
} | ||
|
||
return sentry__bgworker_start(bgworker); | ||
} | ||
|
||
|
@@ -209,6 +254,12 @@ sentry__winhttp_send_task(void *_envelope, void *_state) | |
SENTRY_DEBUGF( | ||
"sending request using winhttp to \"%s\":\n%S", req->url, headers); | ||
|
||
if (state->proxy_username && state->proxy_password) { | ||
WinHttpSetCredentials(state->request, WINHTTP_AUTH_TARGET_PROXY, | ||
WINHTTP_AUTH_SCHEME_BASIC, state->proxy_username, | ||
state->proxy_password, 0); | ||
} | ||
|
||
if (WinHttpSendRequest(state->request, headers, (DWORD)-1, | ||
(LPVOID)req->body, (DWORD)req->body_len, (DWORD)req->body_len, 0)) { | ||
WinHttpReceiveResponse(state->request, NULL); | ||
|
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.
With this new option, I'm not sure we still need to support
no_proxy
; if users want to use the environment variable value in their own code, they can just not set this flag to true (which is the default anyway).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.
Yes and no. You always have to consider that this is a feature that the users of our users require from them.
So, for instance, if your application ignores the
http_proxy
env-vars, your users don't care about the topic and accept that they have to manually configure the proxy config for that application (or even have no proxy config at all).At some point, you might provide a UI to your users that does something like the following:
You can route this directly to our proposed options interface. However, once your users have configured the app to read from the environment, they will want to stay with it because it allows them to have a centrally managed proxy configuration rather than defining it separately in every application.
This is where
no_proxy
enters the picture. So, yes, resetting the proxy config of that app solves the problem, but then the users of our users are essentially back to square 1. Again, this is not a requirement for an initial implementation of that feature but a likely follow-up.