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

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

[Dylan Whyte]

when setting a subscription key, use http(s)_proxy as tunnel if
evironment variable is set.

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"
 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;
+    }
 
-    HTTP_CLIENT.request(request)
-        .map_err(Error::from)
-        .await
+    if !http_proxy.is_empty() {
+        let proxy = format!("http://{}/", http_proxy);
+        let proxy = {
+            let proxy_uri = proxy.parse().unwrap();
+            let proxy = Proxy::new(Intercept::All, proxy_uri);
+            let connector = HttpConnector::new();
+            let proxy_connector = ProxyConnector::from_proxy(connector, proxy).unwrap();
+            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)]
-- 
2.20.1

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

[Thomas Lamprecht]

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)]
> 

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

[Fabian Grünbichler]

On March 19, 2021 4:32 pm, Thomas Lamprecht wrote:
> 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?

chiming in since I did the packaging..

hyper-proxy requires it (transitively). it's a drop-in update which 
we'll need to do at some point anyway, and seems easier to go in the 
upwards direction than patching stuff to use older deps. but 
alternatively, it should be possible to go down that route as well if 
prefered ;)

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

[Thomas Lamprecht]

On 22.03.21 09:39, Fabian Grünbichler wrote:
> On March 19, 2021 4:32 pm, Thomas Lamprecht wrote:
>> 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?
> 
> chiming in since I did the packaging..
> 
> hyper-proxy requires it (transitively). it's a drop-in update which 
> we'll need to do at some point anyway, and seems easier to go in the 
> upwards direction than patching stuff to use older deps. but 
> alternatively, it should be possible to go down that route as well if 
> prefered ;)
> 

no it can be fine, but some mentioning of the new requirement would be nice ;-)
For what I can know this could come from some direct use of crates.io or what not,
so noting it specifically helps to understand that this is actually required, and
not an accident.