Handling for HTTP(S)_PROXY and NO_PROXY

[unterwegi]

Currently, the documentation mentions that applications should handle proxy server related environment variables on their own.

We are currently doing that in our app and we were wondering, if we should contribute the handling we've built back to the library.
Our implementation doesn't just read the HTTP_PROXY/HTTPS_PROXY variables, it also handles NO_PROXY lists, which have some nasty pitfalls to consider when you want to handle those correctly.
https://about.gitlab.com/blog/we-need-to-talk-no-proxy/ has a good explanation and some hints, which our implementation follows (prefer lowercase variable names, domain suffix and CIDR matching in NO_PROXY, no DNS lookups, ...).

This extra proxy environment handling should be an optional feature of course and not affect current setups.

First, we would need a way to provide the NO_PROXY list to the client, could be a new function related to the current set_proxy functions:

client.set_no_proxy({"foo.org", "bar.org"});

Of course, the parts that reference the configured proxy need to check if the target host matches a no_proxy list entry and simply don't use the proxy if it does.

Also, maybe a helper could then make sense for parsing the environment variables and calling the respective proxy setup functions.

client.set_proxy_from_env();

I could start a PR if this feature sounds like something you want.

[yhirose]

@unterwegi Thank you for bringing this up. I’m fine with implementing this additional feature for the proxy, but I am quite concerned about security. Changes like this can easily introduce security vulnerabilities. That’s why I asked Opus to generate enough test cases to help prevent any accidental issues at #2448. (However, there might still be some necessary test cases missing.)

As for the changes in httplib.h, I admit that it looks pretty lengthy, and it could be simpler without keep the code secure.

Could you send a pull request including your changes? I'll take a look at it and making the final pull request from the reference pull requests.

Thanks!

[unterwegi]

@yhirose I've created #2449 which is a somewhat translated version of what we currently have in our own app.

It tries to reuse many parts of the existing PR #2448, which to be honest, handles more as our current implementation. 😅

The generated test cases match what we already test in our own test suite.

I focused on the no_proxy handling for the most part, with this being the meaningful differences from the existing PR:

• The no_proxy entries are stored in vector of strings, the parsing and conversion only happens lazily when checking an URL against the no_proxy list. This might be worse for performance as the strings might be converted multiple times, but it makes the implementation smaller as the intermediate structs are not needed.
• The set_proxy_from_env() function only checks the lowercase version of the proxy variables. apply_proxy_url() is just copied from your PR as it is basically the same implementation we have.

In my opinion, #2448 looks very good already, but maybe you can find some useful bits in my attempt.

[yhirose]

@unterwegi thank you for the PR #2449! I'll take a look at it, and merged 721a945 to #2448. As for the lazy parsing and conversion, I didn't merge it since the current implementation has better performance. Here is the estimate from Opus.

Parse / conversion operations per request:

	#2448 (cached)	#2449 (lazy)
is_proxy_enabled_for_host call sites	8 per request	8 per request
Parse operations per call	0	N (one per entry)
Parse operations per request	0	8 × N
inet_pton calls per request	0 (1–2 on first call)	8 × N × 1–2
Heap allocations per request	0	8 × N × 3–5

Concrete numbers:

NO_PROXY size	#2448 parses	#2449 parses	#2449 inet_pton	#2449 heap allocs
5 entries (typical)	0	40	40–80	120–200
20 entries (corp VPN)	0	160	160–320	480–800

[yhirose]

@unterwegi I was actually thinking of whether I should merge this pull request or not. It's because I intentionally didn't support any common environment variables such as SSL_CERT_FILE and SSL_CERT_DIR that curl, Python requests and Go support for a long time. In the case of cpp-httplib, it needs to call set_ca_cert_path(file, dir) explicitly. So this is the first time in cpp-httplib to control behaviors through environment variables. It's kind of a breaking policy change and I need to carefully ponder over it.

[unterwegi]

@yhirose Sorry for the delay, I was traveling for the last couple of days.

thank you for the PR #2449! I'll take a look at it, and merged 721a945 to #2448. As for the lazy parsing and conversion, I didn't merge it since the current implementation has better performance. Here is the estimate from Opus.

OK, considering these estimates, pre-parsing the entries for sure makes sense!

I was actually thinking of whether I should merge this pull request or not. It's because I intentionally didn't support any common environment variables such as SSL_CERT_FILE and SSL_CERT_DIR that curl, Python requests and Go support for a long time. In the case of cpp-httplib, it needs to call set_ca_cert_path(file, dir) explicitly. So this is the first time in cpp-httplib to control behaviors through environment variables. It's kind of a breaking policy change and I need to carefully ponder over it.

It totally makes sense from a library point of view to only support configuration via dedicated functions instead of parsing the environment. Sorry, I was not aware of that policy and I don't think you should break it because of this request.

Instead of dropping the PR, would it be possible to still keep set_no_proxy(const std::vector<std::string> &patterns) and note in the docs that NO_PROXY should be parsed by the user if needed? Would be totally fine for us.