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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.

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

> +
>  
>  .. 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?

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

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