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

feat: honor http_proxy environment variables #1111

Open
wants to merge 59 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
5a42813
read http_proxy from env. variable
JoshuaMoelans Jan 9, 2025
9565c21
add proxy read from env option
JoshuaMoelans Jan 10, 2025
3abce76
update CHANGELOG.md
JoshuaMoelans Jan 10, 2025
32a9d60
Add proxy-from-env command + fix order of init
JoshuaMoelans Jan 10, 2025
2a8df33
Merge branch 'master' into joshua/feat/http_proxy_from_env_variables
JoshuaMoelans Jan 10, 2025
8081074
reorder https->http proxy fetching
JoshuaMoelans Jan 13, 2025
0d3e09c
add proxy_auth tests
JoshuaMoelans Jan 14, 2025
c73f7f1
Merge branch 'master' into joshua/feat/http_proxy_from_env_variables
JoshuaMoelans Jan 14, 2025
81abb92
fix CHANGELOG.md
JoshuaMoelans Jan 14, 2025
d6edb12
fix CHANGELOG.md
JoshuaMoelans Jan 14, 2025
c3a7575
fix test to avoid cmake build failed
JoshuaMoelans Jan 17, 2025
fb46808
add WinHTTP proxy username and password PoC
JoshuaMoelans Jan 17, 2025
aa1c8c2
remove double set_proxy_credentials call
JoshuaMoelans Jan 17, 2025
86b1f6c
fix typos and some mem cleaning
JoshuaMoelans Jan 17, 2025
a38ea1f
add docs
JoshuaMoelans Jan 20, 2025
8015666
user + test 'sentry__url_parse' for proxy auth
JoshuaMoelans Jan 21, 2025
59f192e
Merge branch 'master' into joshua/feat/http_proxy_from_env_variables
JoshuaMoelans Jan 21, 2025
95a08e3
cleanup
JoshuaMoelans Jan 21, 2025
26966f5
add first proxy from environment test
JoshuaMoelans Jan 22, 2025
dd78ce5
add IPv6 proxy from env test
JoshuaMoelans Jan 23, 2025
da3c91e
proxy-from-env cleanup
JoshuaMoelans Jan 24, 2025
0259fe8
rework integration tests structure
JoshuaMoelans Jan 27, 2025
2ffd6d1
add tests
JoshuaMoelans Jan 28, 2025
5f88d31
move crashpad test & reusable start_mitmdump
JoshuaMoelans Jan 28, 2025
3cfefb3
test cleanup
JoshuaMoelans Jan 28, 2025
3692328
Merge branch 'master' into joshua/feat/http_proxy_from_env_variables
JoshuaMoelans Jan 28, 2025
718af59
fix null-string passing
JoshuaMoelans Jan 28, 2025
4a97426
clean up os.environ
JoshuaMoelans Jan 28, 2025
8411d8b
add check if request was proxied
JoshuaMoelans Jan 28, 2025
4d63147
fix uninitialized proxy_url
JoshuaMoelans Jan 29, 2025
0c64f82
add proxy_env finally check/cleanup function
JoshuaMoelans Jan 29, 2025
b78aa92
extract proxy test cleanup/check function
JoshuaMoelans Jan 29, 2025
75f7edf
fix proxy_test_finally + update CHANGELOG.md
JoshuaMoelans Jan 29, 2025
576b8da
add assert functions
JoshuaMoelans Jan 29, 2025
0a1a9d3
add all-platform crashpad proxy from env reading
JoshuaMoelans Feb 3, 2025
429cf7c
fix crashpad proxy-from-env test
JoshuaMoelans Feb 3, 2025
40482c4
read proxy from env winHTTP transport
JoshuaMoelans Feb 3, 2025
391fba1
add non-localhost hostname set/check/usage
JoshuaMoelans Feb 4, 2025
d44ae35
fix CI to be platform specific
JoshuaMoelans Feb 4, 2025
61f3ce2
third time's the charm
JoshuaMoelans Feb 4, 2025
f65939b
replace the netloc replace with a string replace
JoshuaMoelans Feb 4, 2025
7b8a3e9
remove accidental proxy host for non-proxy test
JoshuaMoelans Feb 4, 2025
234661c
add ignore Curl_getaddrinfo_ex to leaks.txt
JoshuaMoelans Feb 4, 2025
b9f07cd
refactor proxy tests
JoshuaMoelans Feb 5, 2025
3e54dc4
remove incorrect 'fallback' logic comments + update CONTRIBUTING.md
JoshuaMoelans Feb 5, 2025
49c335d
add `http_proxy` reading
JoshuaMoelans Feb 5, 2025
c0cce33
update CHANGELOG.md
JoshuaMoelans Feb 5, 2025
f877b67
read is_secure from dsn
JoshuaMoelans Feb 5, 2025
46ad5b0
cleaner proxy reading
JoshuaMoelans Feb 5, 2025
a2508cd
check null dsn
JoshuaMoelans Feb 5, 2025
1b857ec
add tests
JoshuaMoelans Feb 5, 2025
fedd7fe
change env clean order
JoshuaMoelans Feb 5, 2025
a526df1
update crashpad to handle empty linux proxy
JoshuaMoelans Feb 6, 2025
142acef
forgot ;
JoshuaMoelans Feb 6, 2025
882c640
linux fix with no_proxy + revert crashpad
JoshuaMoelans Feb 6, 2025
969ed41
return to <empty> proxy workaround for Linux crashpad transport
JoshuaMoelans Feb 6, 2025
5bad4d4
strcmp
JoshuaMoelans Feb 6, 2025
45a9f35
pointer check
JoshuaMoelans Feb 6, 2025
93678e0
update CONTRIBUTING.md
JoshuaMoelans Feb 6, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
# Changelog

## Unreleased

**Features**:

- Honor `http_proxy` environment variables. ([#1111](https://github.com/getsentry/sentry-native/pull/1111))

## 0.7.18

**Features**:

- Add support for Xbox Series X/S. ([#1100](https://github.com/getsentry/sentry-native/pull/1100))
- Add option to set debug log level. ([#1107](https://github.com/getsentry/sentry-native/pull/1107))
- Add `traces_sampler` ([#1108](https://github.com/getsentry/sentry-native/pull/1108))
- Add `traces_sampler`. ([#1108](https://github.com/getsentry/sentry-native/pull/1108))
- Provide support for C++17 compilers when using the `crashpad` backend. ([#1110](https://github.com/getsentry/sentry-native/pull/1110), [crashpad#116](https://github.com/getsentry/crashpad/pull/116), [mini_chromium#1](https://github.com/getsentry/mini_chromium/pull/1))

## 0.7.17
Expand Down
3 changes: 3 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,10 @@ The example currently supports the following commands:
- `override-sdk-name`: Changes the SDK name via the options at runtime.
- `stack-overflow`: Provokes a stack-overflow.
- `http-proxy`: Uses a localhost `HTTP` proxy on port 8080.
- `http-proxy-auth`: Uses a localhost `HTTP` proxy on port 8080 with `user:password` as authentication.
- `socks5-proxy`: Uses a localhost `SOCKS5` proxy on port 1080.
- `socks5-proxy-auth`: Uses a localhost `SOCKS5` proxy on port 1080 with `user:password` as authentication.
- `proxy-from-env`: Reads the proxy settings from the environment variables `https_proxy` and `http_proxy` (in this order of precedence).
- `capture-transaction`: Captures a transaction.
- `traces-sampler`: Installs a traces sampler callback function when used alongside `capture-transaction`.

Expand Down
12 changes: 12 additions & 0 deletions examples/example.c
Original file line number Diff line number Diff line change
Expand Up @@ -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:password@127.0.0.1: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:password@127.0.0.1:1080");
}

if (has_arg(argc, argv, "proxy-from-env")) {
sentry_options_set_read_proxy_from_environment(options, true);
}

sentry_init(options);

Expand Down
11 changes: 11 additions & 0 deletions include/sentry.h
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,17 @@ SENTRY_API void sentry_options_set_http_proxy_n(
SENTRY_API const char *sentry_options_get_http_proxy(
const sentry_options_t *opts);

/**
* Sets whether to read the proxy settings from the environment.
JoshuaMoelans marked this conversation as resolved.
Show resolved Hide resolved
*/
SENTRY_API void sentry_options_set_read_proxy_from_environment(
sentry_options_t *opts, int val);
/**
* Returns whether to read the proxy settings from the environment.
*/
SENTRY_API int sentry_options_get_read_proxy_from_environment(
const sentry_options_t *opts);

/**
* Configures the path to a file containing ssl certificates for
* verification.
Expand Down
4 changes: 4 additions & 0 deletions src/sentry_core.c
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ sentry_init(sentry_options_t *options)
"the provided DSN \"%s\" is not valid", raw_dsn ? raw_dsn : "");
}

if (options->read_proxy_from_environment) {
sentry__set_proxy_from_environment(options);
}
Copy link
Member Author

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

Copy link
Collaborator

@supervacuus supervacuus Jan 13, 2025

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:

(   ) direct HTTP connection.
(   ) manually specify HTTP proxy: _________ .
( * ) read proxy from the environment.

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.


if (transport) {
if (sentry__transport_startup(transport, options) != 0) {
SENTRY_WARN("failed to initialize transport");
Expand Down
23 changes: 23 additions & 0 deletions src/sentry_options.c
Original file line number Diff line number Diff line change
Expand Up @@ -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");
JoshuaMoelans marked this conversation as resolved.
Show resolved Hide resolved
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)
{
Expand Down
5 changes: 5 additions & 0 deletions src/sentry_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ typedef struct sentry_options_s {
bool require_user_consent;
bool symbolize_stacktraces;
bool system_crash_reporter_enabled;
bool read_proxy_from_environment;

sentry_attachment_t *attachments;
sentry_run_t *run;
Expand Down Expand Up @@ -80,4 +81,8 @@ typedef struct sentry_options_s {
*/
sentry_options_t *sentry__options_incref(sentry_options_t *options);

/**
* Sets the proxy value by reading it from the environment.
*/
void sentry__set_proxy_from_environment(sentry_options_t *opts);
#endif
46 changes: 46 additions & 0 deletions src/transports/sentry_transport_winhttp.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -55,6 +57,38 @@ sentry__winhttp_bgworker_state_free(void *_state)
sentry_free(state);
}

// Function to extract and set credentials TODO replace with `sentry__url_parse`
JoshuaMoelans marked this conversation as resolved.
Show resolved Hide resolved
void
set_proxy_credentials(winhttp_bgworker_state_t *state, const char *proxy)
{
JoshuaMoelans marked this conversation as resolved.
Show resolved Hide resolved
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_user = user_w;
state->proxy_pass = pass_w;
}
}
}

static int
sentry__winhttp_transport_start(
const sentry_options_t *opts, void *transport_state)
Expand All @@ -71,7 +105,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;
JoshuaMoelans marked this conversation as resolved.
Show resolved Hide resolved
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);
Expand Down Expand Up @@ -103,6 +142,8 @@ sentry__winhttp_transport_start(
SENTRY_WARN("`WinHttpOpen` failed");
return 1;
}

set_proxy_credentials(state, opts->proxy);
return sentry__bgworker_start(bgworker);
}

Expand Down Expand Up @@ -209,6 +250,11 @@ sentry__winhttp_send_task(void *_envelope, void *_state)
SENTRY_DEBUGF(
"sending request using winhttp to \"%s\":\n%S", req->url, headers);

if (state->proxy_user && state->proxy_pass) {
WinHttpSetCredentials(state->request, WINHTTP_AUTH_TARGET_PROXY,
WINHTTP_AUTH_SCHEME_BASIC, state->proxy_user, state->proxy_pass, 0);
}

if (WinHttpSendRequest(state->request, headers, (DWORD)-1,
(LPVOID)req->body, (DWORD)req->body_len, (DWORD)req->body_len, 0)) {
WinHttpReceiveResponse(state->request, NULL);
Expand Down
19 changes: 14 additions & 5 deletions tests/test_integration_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,8 @@ def test_capture_minidump(cmake, httpserver):
],
)
@pytest.mark.parametrize("proxy_status", [(["off"]), (["on"])])
def test_capture_proxy(cmake, httpserver, run_args, proxy_status):
@pytest.mark.parametrize("proxy_auth", [(["off"]), (["on"])])
def test_capture_proxy(cmake, httpserver, run_args, proxy_status, proxy_auth):
if not shutil.which("mitmdump"):
pytest.skip("mitmdump is not installed")

Expand All @@ -635,12 +636,18 @@ def test_capture_proxy(cmake, httpserver, run_args, proxy_status):
if proxy_status == ["on"]:
# start mitmdump from terminal
if run_args == ["http-proxy"]:
proxy_process = subprocess.Popen(["mitmdump"])
proxy_command = ["mitmdump"]
if proxy_auth == ["on"]:
proxy_command.append("--proxyauth=user:password")
proxy_process = subprocess.Popen(proxy_command)
time.sleep(5) # Give mitmdump some time to start
if not is_proxy_running("localhost", 8080):
pytest.fail("mitmdump (HTTP) did not start correctly")
elif run_args == ["socks5-proxy"]:
proxy_process = subprocess.Popen(["mitmdump", "--mode", "socks5"])
proxy_command = ["mitmdump", "--mode", "socks5"]
if proxy_auth == ["on"]:
proxy_command.append("--proxyauth=user:password")
proxy_process = subprocess.Popen(proxy_command)
time.sleep(5) # Give mitmdump some time to start
if not is_proxy_running("localhost", 1080):
pytest.fail("mitmdump (SOCKS5) did not start correctly")
Expand All @@ -651,12 +658,14 @@ def test_capture_proxy(cmake, httpserver, run_args, proxy_status):
shutil.rmtree(tmp_path / ".sentry-native", ignore_errors=True)

httpserver.expect_request("/api/123456/envelope/").respond_with_data("OK")

current_run_arg = run_args[0]
if proxy_auth == ["on"]:
current_run_arg += "-auth"
run(
tmp_path,
"sentry_example",
["log", "start-session", "capture-event"]
+ run_args, # only passes if given proxy is running
+ [current_run_arg], # only passes if given proxy is running
check=True,
env=dict(os.environ, SENTRY_DSN=make_dsn(httpserver)),
)
Expand Down
Loading