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 9A8D069307 for ; Mon, 22 Mar 2021 10:04:02 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 90E8A2128E for ; Mon, 22 Mar 2021 10:04: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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 620D921284 for ; Mon, 22 Mar 2021 10:04: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 28D59426D2 for ; Mon, 22 Mar 2021 10:04:01 +0100 (CET) Message-ID: Date: Mon, 22 Mar 2021 10:03: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 , =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= , Dylan Whyte References: <20210319133503.5398-1-d.whyte@proxmox.com> <1616402168.dbhrg0pqgc.astroid@nora.none> From: Thomas Lamprecht In-Reply-To: <1616402168.dbhrg0pqgc.astroid@nora.none> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 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: Mon, 22 Mar 2021 09:04:02 -0000 On 22.03.21 09:39, Fabian Gr=C3=BCnbichler 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=20 weird and >> possible unexpected >> >> * In Proxmox VE and Proxmox Mail Gateway we have a datacenter/admin co= nfig for >> the http(s) proxy, and do not rely on the environment variables - wh= ich required >> a reload or restart of the PBS daemon(s) to get applied, also not su= re how >> systemd handles the http_one, as it may clear up env quite a bit and=20 we do not set >> an EnvironmentFile by default. Did you test this when running the da= emons not >> manually but under systemd supervision? >> >> * In PVE and PMG we also use the proxy configuration for writing out a= n 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=20 anyway for >> setting things like FQDN, email sender and now the proxy could fit i= n there too. >> >> * Don't just auto-magically use some env variable in a single http req= uest helper, >> but make it more explicit, from top of my head that could be: >> - add a Option to get/post function which some value=20 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=20 http proxy) >> >> * another patch handling the apt proxy auth, like we do in PVE/PMG; th= at can be >> future stuff, but is required to make the proxy handling somewhat co= mplete >> >>> >>> 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 aftera= ll. >>> >>> 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 =3D "src/lib.rs" >>> =20 >>> [dependencies] >>> apt-pkg-native =3D "0.3.2" >>> -base64 =3D "0.12" >>> +base64 =3D "0.13" >> >> why does this get upgraded? >=20 > chiming in since I did the packaging.. >=20 > hyper-proxy requires it (transitively). it's a drop-in update which=20 > we'll need to do at some point anyway, and seems easier to go in the=20 > upwards direction than patching stuff to use older deps. but=20 > alternatively, it should be possible to go down that route as well if=20 > prefered ;) >=20 no it can be fine, but some mentioning of the new requirement would be ni= ce ;-) 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 requi= red, and not an accident.