* [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars
@ 2022-09-15 14:08 Stefan Hanreich
2022-09-16 6:58 ` Thomas Lamprecht
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hanreich @ 2022-09-15 14:08 UTC (permalink / raw)
To: pbs-devel
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.
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.
+
.. 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 {
+ 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() {
+ 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
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars
2022-09-15 14:08 [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars Stefan Hanreich
@ 2022-09-16 6:58 ` Thomas Lamprecht
2022-09-16 7:08 ` Stefan Hanreich
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Lamprecht @ 2022-09-16 6:58 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Stefan Hanreich
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars
2022-09-16 6:58 ` Thomas Lamprecht
@ 2022-09-16 7:08 ` Stefan Hanreich
2022-09-16 8:09 ` Thomas Lamprecht
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hanreich @ 2022-09-16 7:08 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox Backup Server development discussion
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars
2022-09-16 7:08 ` Stefan Hanreich
@ 2022-09-16 8:09 ` Thomas Lamprecht
2022-09-16 8:17 ` Stefan Hanreich
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Lamprecht @ 2022-09-16 8:09 UTC (permalink / raw)
To: Stefan Hanreich, Proxmox Backup Server development discussion
Am 16/09/2022 um 09:08 schrieb Stefan Hanreich:
> 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.
Sounds still like a x-y problem to me, e.g., the reporter doesn't want
a proxy, they want a way to bridge isolated networks to certain services
(apt, pbs, ...), but anyway, it's to small of a change to not do compared
to debating that with users semi-frequently ;-)
>> 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.
But the HttpClient::new code clearly spawns a login future due to the auth not being a API
token, so your test just seems brittle and you may got lucky with timing (i.e., future
didn't schedule until test exited already), seems a recipe for flaky tests to me...
Also, note that you do not test the client's proxy capabilities, but only what ProxyConfig
accepts, and that should go into the ProxyConfig implementation, if we really want to have
that. Besides belonging there it can be also much simpler/less complex to test..
>
>>> + env::set_var("ALL_PROXY", "https://localhost:8080");
>>> +
>>> + let mut http_client = HttpClient::new(
fyi, it would be more idiomatic rust to avoid the `mut` here and
just always do a `let http_client = ...` for subsequent declarations
too.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars
2022-09-16 8:09 ` Thomas Lamprecht
@ 2022-09-16 8:17 ` Stefan Hanreich
2022-09-16 8:34 ` Thomas Lamprecht
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hanreich @ 2022-09-16 8:17 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox Backup Server development discussion
On 9/16/22 10:09, Thomas Lamprecht wrote:
> Am 16/09/2022 um 09:08 schrieb Stefan Hanreich:
>> 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.
>
> Sounds still like a x-y problem to me, e.g., the reporter doesn't want
> a proxy, they want a way to bridge isolated networks to certain services
> (apt, pbs, ...), but anyway, it's to small of a change to not do compared
> to debating that with users semi-frequently ;-)
>
>
>>> 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.
>
> But the HttpClient::new code clearly spawns a login future due to the auth not being a API
> token, so your test just seems brittle and you may got lucky with timing (i.e., future
> didn't schedule until test exited already), seems a recipe for flaky tests to me...
>
> Also, note that you do not test the client's proxy capabilities, but only what ProxyConfig
> accepts, and that should go into the ProxyConfig implementation, if we really want to have
> that. Besides belonging there it can be also much simpler/less complex to test..
Yes, I was also not very happy with how I had to test it. If i really
wanted to test it properly I'd add tests to the ProxyConfig as well as
the HttpsConnector, but that seemed a bit out of scope for this patch
series, which is why I resorted to this (seemingly) basic test.
It was more of a basic sanity check to see if I didn't break some stuff.
Do you think it would make sense to create a separate patch series that
unit tests ProxyConfig with some (im)possible proxy settings?
I guess I'll just remove the tests for now.
>
>>
>>>> + env::set_var("ALL_PROXY", "https://localhost:8080");
>>>> +
>>>> + let mut http_client = HttpClient::new(
>
> fyi, it would be more idiomatic rust to avoid the `mut` here and
> just always do a `let http_client = ...` for subsequent declarations
> too.
>
I'll keep it in mind.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars
2022-09-16 8:17 ` Stefan Hanreich
@ 2022-09-16 8:34 ` Thomas Lamprecht
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2022-09-16 8:34 UTC (permalink / raw)
To: Stefan Hanreich, Proxmox Backup Server development discussion
Am 16/09/2022 um 10:17 schrieb Stefan Hanreich:
> Yes, I was also not very happy with how I had to test it. If i really wanted to test it properly I'd add tests to the ProxyConfig as well as the HttpsConnector, but that seemed a bit out of scope for this patch series, which is why I resorted to this (seemingly) basic test.
>
> It was more of a basic sanity check to see if I didn't break some stuff.
Sure, but test code should be near the thing actually tested. Note also that
if we would add https to ProxyConfig in the future it would fail a "distant"
crate in a bit confusing way (the "slightly confusing" part could probably be
avoided by checking either the actual error or a comment for why it's expected
to fail), all noticeable and thus fixable before rollout, so nothing _that bad_,
don't get me wrong, but it would Just Work™ otherwise.
I'm also a bit wondering how setting environment variables affects other tests in
general, iow. how isolated this all is. I'd not 100% sure if rust spawns a separate
process per test and also then it may be a bit cleaner to first get the current env
variable one is modifying and restoring that value at the end of the test. I don't
think that would be an issue here, as we shouldn't rely on any connections at all,
so more of a general wondering-thing.
> Do you think it would make sense to create a separate patch series that unit tests ProxyConfig with some (im)possible proxy settings?
The current ProxyConfig is 89 Lines with comments and generous extra lines
of rather straight forward code, so while a clear cut specific test definitively
wouldn't be hurting, investing to much time in any would probably amount to a
rather mediocre ROI. With that in mind: your choice, if you send a somewhat sensible
one I'll definitively apply it.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-16 8:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-15 14:08 [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars Stefan Hanreich
2022-09-16 6:58 ` Thomas Lamprecht
2022-09-16 7:08 ` Stefan Hanreich
2022-09-16 8:09 ` Thomas Lamprecht
2022-09-16 8:17 ` Stefan Hanreich
2022-09-16 8:34 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox