From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 9825193267 for ; Fri, 16 Sep 2022 09:16:50 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 82CA120BF8 for ; Fri, 16 Sep 2022 09:16:20 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 16 Sep 2022 09:16:19 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 1B4A043424 for ; Fri, 16 Sep 2022 09:08:17 +0200 (CEST) Message-ID: <7ecef7a9-afda-f0fd-55d1-924d4819745b@proxmox.com> Date: Fri, 16 Sep 2022 09:08:15 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0 Content-Language: en-US To: Thomas Lamprecht , Proxmox Backup Server development discussion References: <20220915140857.1041222-1-s.hanreich@proxmox.com> <6d281db9-90e8-9164-6979-4b73b04cc627@proxmox.com> From: Stefan Hanreich In-Reply-To: <6d281db9-90e8-9164-6979-4b73b04cc627@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.801 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -1.816 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Sep 2022 07:16:50 -0000 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 >> --- >> 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@][: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