Re: [pbs-devel] [PATCH proxmox-backup] fix #3296: allow set subscription through proxy

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

On 19.03.21 14:35, Dylan Whyte wrote:
> when setting a subscription key, use http(s)_proxy as tunnel if
> evironment variable is set.

first thanks for sending a patch for this important featire.

A few high level comments/issues I see:
* this now uses proxies for all current and future uses of the tools::http::post
  function, but not the other http request helpers from that tool, IMO weird and
  possible unexpected

* In Proxmox VE and Proxmox Mail Gateway we have a datacenter/admin config for
  the http(s) proxy, and do not rely on the environment variables - which required
  a reload or restart of the PBS daemon(s) to get applied, also not sure how
  systemd handles the http_one, as it may clear up env quite a bit and we do not set
  an EnvironmentFile by default. Did you test this when running the daemons not
  manually but under systemd supervision?

* In PVE and PMG we also use the proxy configuration for writing out an APT config
  on the apt api upgrade path

So, what would be nice to have is:

* A config similar to PVE/PMG; we wanted to add a PBS wide node config anyway for
  setting things like FQDN, email sender and now the proxy could fit in there too.

* Don't just auto-magically use some env variable in a single http request helper,
  but make it more explicit, from top of my head that could be:
  - add a Option<ProxyConnector> to get/post function which some value is used over
    the static HTTP_CLIENT
  - add a separate post_proxied method

  In any case, the "get the ProxyConnector" part may be nicer to live in its own
  method (possibly with getting the node config and checking it for an http proxy)

* another patch handling the apt proxy auth, like we do in PVE/PMG; that can be
  future stuff, but is required to make the proxy handling somewhat complete

> 
> Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
> ---
> 
> * required packages can be found in nasi/iso/packages/hyper-proxy
> 
> Note that proxy authorization/authentication is not implemented yet.
> hyper-proxy implements it using the 'headers' crate, which we do
> not have as a direct dependency. I figured i'd leave it for a
> follow up patch, just in case we decide not to use hyper-proxy afterall.
> 
>  Cargo.toml        |  3 ++-
>  src/tools/http.rs | 30 +++++++++++++++++++++++++++---
>  2 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index 9483831c..5a8bcc81 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -24,7 +24,7 @@ path = "src/lib.rs"
>  
>  [dependencies]
>  apt-pkg-native = "0.3.2"
> -base64 = "0.12"
> +base64 = "0.13"

why does this get upgraded?

>  bitflags = "1.2.1"
>  bytes = "1.0"
>  crc32fast = "1"
> @@ -74,6 +74,7 @@ xdg = "2.2"
>  zstd = { version = "0.4", features = [ "bindgen" ] }
>  nom = "5.1"
>  crossbeam-channel = "0.5"
> +hyper-proxy = { version = "0.9", default-features = false, features = ["openssl-tls"] }
>  
>  [features]
>  default = []
> diff --git a/src/tools/http.rs b/src/tools/http.rs
> index d08ce451..057f2abb 100644
> --- a/src/tools/http.rs
> +++ b/src/tools/http.rs
> @@ -7,6 +7,7 @@ use std::pin::Pin;
>  
>  use hyper::{Uri, Body};
>  use hyper::client::{Client, HttpConnector};
> +use hyper_proxy::{Proxy, ProxyConnector, Intercept};
>  use http::{Request, Response};
>  use openssl::ssl::{SslConnector, SslMethod};
>  use futures::*;
> @@ -77,10 +78,33 @@ pub async fn post(
>          .header(hyper::header::CONTENT_TYPE, content_type)
>          .body(body)?;
>  
> +    let mut http_proxy = "".to_string();
> +    if let Ok(proxy) = std::env::var("https_proxy") {
> +        http_proxy = proxy;
> +    } else if let Ok(proxy) = std::env::var("http_proxy") {
> +        http_proxy = proxy;
> +    }

Above can be written nicer, in the sense of avoiding a mutable and in
general, for such optional values it may be more sensible to use an Option<T>
instead of checking empty strings for that. An option expresses better whats
actually wanted here.

Two options:

let http_proxy = if let Ok(proxy) = std::env::var("https_proxy") {
    Some(proxy)
} else if let Ok(proxy) = std::env::var("http_proxy") {
    Some(proxy)
} else {
    None
}


alternatively, shorter but 

let http_proxy = std::env::var("https_proxy").or(std::env::var("http_proxy")).ok();

>  
> -    HTTP_CLIENT.request(request)
> -        .map_err(Error::from)
> -        .await
> +    if !http_proxy.is_empty() {

with either variant above we'd do now:

if let Some(proxy) = proxy {
     ...

> +        let proxy = format!("http://{}/", http_proxy);
> +        let proxy = {
> +            let proxy_uri = proxy.parse().unwrap();

not a good idea to panic if above errors...
.unwrap() should almost never be used..

> +            let proxy = Proxy::new(Intercept::All, proxy_uri);
> +            let connector = HttpConnector::new();
> +            let proxy_connector = ProxyConnector::from_proxy(connector, proxy).unwrap();

same here with unwarp, handle the error explicitly.

> +            proxy_connector
> +        };
> +
> +        let client = Client::builder().build(proxy);
> +
> +        client.request(request)
> +            .map_err(Error::from)
> +            .await
> +    } else {
> +        HTTP_CLIENT.request(request)
> +            .map_err(Error::from)
> +            .await
> +    }
>  }
>  
>  #[derive(Clone)]
>