From: Stefan Hanreich <s.hanreich@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars
Date: Fri, 16 Sep 2022 09:08:15 +0200 [thread overview]
Message-ID: <7ecef7a9-afda-f0fd-55d1-924d4819745b@proxmox.com> (raw)
In-Reply-To: <6d281db9-90e8-9164-6979-4b73b04cc627@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
next prev parent reply other threads:[~2022-09-16 7:16 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-15 14:08 Stefan Hanreich
2022-09-16 6:58 ` Thomas Lamprecht
2022-09-16 7:08 ` Stefan Hanreich [this message]
2022-09-16 8:09 ` Thomas Lamprecht
2022-09-16 8:17 ` Stefan Hanreich
2022-09-16 8:34 ` Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7ecef7a9-afda-f0fd-55d1-924d4819745b@proxmox.com \
--to=s.hanreich@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
--cc=t.lamprecht@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox