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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 9979B6C102 for ; Fri, 19 Mar 2021 16:33:32 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 808FC245E6 for ; Fri, 19 Mar 2021 16:33:02 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 1F339245D6 for ; Fri, 19 Mar 2021 16:33:01 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id E0F784288D for ; Fri, 19 Mar 2021 16:33:00 +0100 (CET) Message-ID: Date: Fri, 19 Mar 2021 16:32:59 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:87.0) Gecko/20100101 Thunderbird/87.0 Content-Language: en-US To: Proxmox Backup Server development discussion , Dylan Whyte References: <20210319133503.5398-1-d.whyte@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210319133503.5398-1-d.whyte@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.045 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [http.rs, lib.rs] Subject: Re: [pbs-devel] [PATCH proxmox-backup] fix #3296: allow set subscription through proxy 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, 19 Mar 2021 15:33:32 -0000 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 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 > --- > > * 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 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)] >