[pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars
Stefan Hanreich
s.hanreich at proxmox.com
Fri Sep 16 09:08:15 CEST 2022
On 9/16/22 08:58, Thomas Lamprecht wrote:
> Am 15/09/2022 um 16:08 schrieb Stefan Hanreich:
>> In order to be able to use a proxy with the proxmox-backup-client, use
>> ProxyConfig for parsing proxy server config from the environment. Also
>> added a section in the documentation that describes how to configure the
>> environment if a proxy server should be used.
>
> Proxy config was more intended for the server, not the client side(s), and IMO
> proxy's never have any use outside of central surveillance of (tls) traffic, but
> well, we already got it and some user may want it, so can be fine, IMO close
> to a breaking change though, would at least require entries in all product's
> "noteable changes" section of their next point release.
Yes, it seemed like some users desperately wanted it, judging from the
bugzilla issue as well as the forum.
>>
>> Signed-off-by: Stefan Hanreich <s.hanreich at proxmox.com>
>> ---
>> docs/backup-client.rst | 7 +++++
>> pbs-client/src/http_client.rs | 57 +++++++++++++++++++++++++++++++++++
>> 2 files changed, 64 insertions(+)
>>
>> diff --git a/docs/backup-client.rst b/docs/backup-client.rst
>> index cc56d17e..45efc136 100644
>> --- a/docs/backup-client.rst
>> +++ b/docs/backup-client.rst
>> @@ -69,6 +69,13 @@ Environment Variables
>> When set, this value is used to verify the server certificate (only used if
>> the system CA certificates cannot validate the certificate).
>>
>> +``ALL_PROXY``
>> + When set, the client uses the specified HTTP proxy for all connections to the
>> + backup server. Currently only HTTP proxies are supported. Valid proxy
>> + configurations have the following format:
>> + `[http://][user:password@]<host>[:port]`. Default `port` is 1080, if not
>> + otherwise specified.
>
> Can you add a note here that we recommend using tunnels (e.g., wireguard) over
> HTTP proxies for shielding of hosts instead, maybe it helps to save some users
> from insecure proxy settings.
>
can do
>> +
>>
>> .. Note:: Passwords must be valid UTF-8 and may not contain newlines. For your
>> convenience, Proxmox Backup Server only uses the first line as password, so
>> diff --git a/pbs-client/src/http_client.rs b/pbs-client/src/http_client.rs
>> index 4ef1350b..e41d94c0 100644
>> --- a/pbs-client/src/http_client.rs
>> +++ b/pbs-client/src/http_client.rs
>> @@ -23,6 +23,7 @@ use proxmox_sys::linux::tty;
>>
>> use proxmox_async::broadcast_future::BroadcastFuture;
>> use proxmox_http::client::{HttpsConnector, RateLimiter};
>> +use proxmox_http::ProxyConfig;
>> use proxmox_http::uri::{build_authority, json_object_to_query};
>>
>> use pbs_api_types::percent_encoding::DEFAULT_ENCODE_SET;
>> @@ -389,6 +390,11 @@ impl HttpClient {
>> )))));
>> }
>>
>> + let proxy_config = ProxyConfig::from_proxy_env()?;
>> + if let Some(config) = proxy_config {
>
> can we log that we're using a proxy to ease debugging of connection issues?
>
can do
>> + https.set_proxy(config);
>> + }
>> +
>> let client = Client::builder()
>> //.http2_initial_stream_window_size( (1 << 31) - 2)
>> //.http2_initial_connection_window_size( (1 << 31) - 2)
>> @@ -1083,3 +1089,54 @@ impl H2Client {
>> Ok(request)
>> }
>> }
>> +
>> +#[cfg(test)]
>> +mod tests {
>> + use std::env;
>> + use super::*;
>> +
>> + #[test]
>> + fn test_proxy_config() {
>
> so, if I get this right this test requires the following in the build env:
> * needs a proxy to run on 8080
> * needs PBS running
> * needs to build/tetst as root
>
> All three are a complete no-go, makes bootstrapping harder and is just a flaky
> test all together, I'd just drop it completely... Anyway, NAK until fixed.
>
I don't think any of them are required actually, since it is just
checking whether it can instantiate the HttpClient. It never makes any
request to the outside. I ran this test as non-root on my local machine
without PBS nor proxy runnning and it runs just fine.
>> + env::set_var("ALL_PROXY", "https://localhost:8080");
>> +
>> + let mut http_client = HttpClient::new(
>> + "localhost",
>> + 8888,
>> + Authid::root_auth_id(),
>> + HttpClientOptions::new_non_interactive(
>> + String::from("test"),
>> + None
>> + ),
>> + );
>> +
>> + assert!(http_client.is_err());
>> +
>> + env::set_var("ALL_PROXY", "http://localhost:8080");
>> +
>> + http_client = HttpClient::new(
>> + "localhost",
>> + 8888,
>> + Authid::root_auth_id(),
>> + HttpClientOptions::new_non_interactive(
>> + String::from("test"),
>> + None
>> + ),
>> + );
>> +
>> + assert!(http_client.is_ok());
>> +
>> + env::remove_var("ALL_PROXY");
>> +
>> + http_client = HttpClient::new(
>> + "localhost",
>> + 8888,
>> + Authid::root_auth_id(),
>> + HttpClientOptions::new_non_interactive(
>> + String::from("test"),
>> + None
>> + ),
>> + );
>> +
>> + assert!(http_client.is_ok());
>> + }
>> +}
>> \ No newline at end of file
>
> ^- please check your editor to write sensible text out.
yes, i already noticed yesterday and configured my editor accordingly :D
More information about the pbs-devel
mailing list