Re: [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars

[Stefan Hanreich <s.hanreich@proxmox.com>]

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@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