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 D19087B544 for ; Wed, 12 May 2021 12:01:54 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BF87ABBE1 for ; Wed, 12 May 2021 12:01:24 +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 id C6352BBD2 for ; Wed, 12 May 2021 12:01:23 +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 A03C246543 for ; Wed, 12 May 2021 12:01:23 +0200 (CEST) Message-ID: <4a78fa42-02de-664f-90f6-eaa16fe9ea31@proxmox.com> Date: Wed, 12 May 2021 12:01:21 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:89.0) Gecko/20100101 Thunderbird/89.0 Content-Language: en-US To: Proxmox Backup Server development discussion , Dietmar Maurer References: <20210511065429.22763-1-dietmar@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210511065429.22763-1-dietmar@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.007 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) 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] correctly set apt proxy configuration 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: Wed, 12 May 2021 10:01:54 -0000 looks mostly good, a few comments in line. On 11.05.21 08:54, Dietmar Maurer wrote: > --- > src/api2/node/apt.rs | 41 ++++++++++++++++++++++++++++++++--------- > src/api2/node/config.rs | 9 ++++++++- > src/config/node.rs | 4 +++- > src/tools/http.rs | 18 +++++++++++++++--- > 4 files changed, 58 insertions(+), 14 deletions(-) > > diff --git a/src/api2/node/apt.rs b/src/api2/node/apt.rs > index 1e57ea8d..83ccc460 100644 > --- a/src/api2/node/apt.rs > +++ b/src/api2/node/apt.rs > @@ -5,11 +5,11 @@ use std::collections::HashMap; > use proxmox::list_subdirs_api_method; > use proxmox::api::{api, RpcEnvironment, RpcEnvironmentType, Permission}; > use proxmox::api::router::{Router, SubdirMap}; > +use proxmox::tools::fs::{replace_file, CreateOptions}; > > use crate::config::node; > use crate::server::WorkerTask; > -use crate::tools::{apt, SimpleHttp, subscription}; > - > +use crate::tools::{apt, SimpleHttp, http::ProxyConfig, subscription}; > use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY}; > use crate::api2::types::{Authid, APTUpdateInfo, NODE_SCHEMA, UPID_SCHEMA}; > > @@ -47,10 +47,36 @@ fn apt_update_available(_param: Value) -> Result { > Ok(json!(cache.package_status)) > } > > +pub fn update_apt_proxy_config(proxy_config: Option<&ProxyConfig>) -> Result<(), Error> { > + > + const PROXY_CFG_FN: &str = "/etc/apt/apt.conf.d/76pveproxy"; // use same file as PVE > + > + if let Some(proxy_config) = proxy_config { > + let proxy = proxy_config.to_proxy_string()?; > + let data = format!("Acquire::http::Proxy \"{}\";\n", proxy); > + replace_file(PROXY_CFG_FN, data.as_bytes(), CreateOptions::new())?; > + } else { > + let _ = std::fs::remove_file(PROXY_CFG_FN); I do not like such results thrown away, I mean it is unlikely, but if the removal fails the proxy would still be configured for apt, which may come as a surprise for an user. Can we do something like: std::fs::remove_file(PROXY_CFG_FN).map_err(|err| { format_err!("failed to remove proxy config '{}' - {}", PROXY_CFG_FN, err) })?; here? > + } > + > + Ok(()) > +} > + > +fn read_and_update_proxy_config() -> Result, Error> { > + let proxy_config = if let Ok((node_config, _digest)) = node::config() { > + node_config.http_proxy() > + } else { > + None > + }; > + update_apt_proxy_config(proxy_config.as_ref())?; > + > + Ok(proxy_config) > +} > + > fn do_apt_update(worker: &WorkerTask, quiet: bool) -> Result<(), Error> { > if !quiet { worker.log("starting apt-get update") } > > - // TODO: set proxy /etc/apt/apt.conf.d/76pbsproxy like PVE > + read_and_update_proxy_config()?; > > let mut command = std::process::Command::new("apt-get"); > command.arg("update"); > @@ -153,6 +179,7 @@ pub fn apt_update_database( > } > > #[api( > + protected: true, > input: { > properties: { > node: { > @@ -195,12 +222,7 @@ fn apt_get_changelog( > bail!("Package '{}' not found", name); > } > > - let proxy_config = if let Ok((node_config, _digest)) = node::config() { > - node_config.http_proxy() > - } else { > - None > - }; > - > + let proxy_config = read_and_update_proxy_config()?; > let mut client = SimpleHttp::new(proxy_config); > > let changelog_url = &pkg_info[0].change_log_url; > @@ -235,6 +257,7 @@ fn apt_get_changelog( > Ok(json!(changelog)) > > } else { > + eprintln!("TZEST2"); please remove the left over debug print above > let mut command = std::process::Command::new("apt-get"); > command.arg("changelog"); > command.arg("-qq"); // don't display download progress > diff --git a/src/api2/node/config.rs b/src/api2/node/config.rs > index 92c0100e..290e9284 100644 > --- a/src/api2/node/config.rs > +++ b/src/api2/node/config.rs > @@ -4,6 +4,7 @@ use proxmox::api::schema::Updatable; > use proxmox::api::{api, Permission, Router, RpcEnvironment}; > > use crate::api2::types::NODE_SCHEMA; > +use crate::api2::node::apt::update_apt_proxy_config; > use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY}; > use crate::config::node::{NodeConfig, NodeConfigUpdater}; > > @@ -78,5 +79,11 @@ pub fn update_node_config( > > config.update_from(updater, &delete)?; > > - crate::config::node::save_config(&config) > + let http_proxy = config.http_proxy(); > + > + crate::config::node::save_config(&config)?; > + > + update_apt_proxy_config(http_proxy.as_ref())?; would be enough and IMO still readable if we'd just that in one line here: update_apt_proxy_config(config.http_proxy().as_ref())?; but no hard feelings here > + > + Ok(()) > } > diff --git a/src/config/node.rs b/src/config/node.rs > index 31a2c5ab..33f3e2ad 100644 > --- a/src/config/node.rs > +++ b/src/config/node.rs > @@ -151,6 +151,7 @@ impl NodeConfig { > AcmeDomainIter::new(self) > } > > + /// Returns the parsed ProxyConfig > pub fn http_proxy(&self) -> Option { > if let Some(http_proxy) = &self.http_proxy { > match ProxyConfig::parse_proxy_url(&http_proxy) { > @@ -162,7 +163,8 @@ impl NodeConfig { > } > } > > - pub fn set_proxy(&mut self, http_proxy: Option) { > + /// Sets the HTTP proxy configuration > + pub fn set_http_proxy(&mut self, http_proxy: Option) { > self.http_proxy = http_proxy; > } > > diff --git a/src/tools/http.rs b/src/tools/http.rs > index 998e7647..0821992a 100644 > --- a/src/tools/http.rs > +++ b/src/tools/http.rs > @@ -43,7 +43,7 @@ pub(crate) fn build_authority(host: &str, port: u16) -> Result > pub struct ProxyConfig { > pub host: String, > pub port: u16, > - pub authorization: Option, // Proxy-Authorization header value > + pub authorization: Option, // user:pass > pub force_connect: bool, > } > > @@ -94,7 +94,7 @@ impl ProxyConfig { > > let authority_vec: Vec<&str> = proxy_authority.as_str().rsplitn(2, '@').collect(); > let authorization = if authority_vec.len() == 2 { > - Some(format!("Basic {}", base64::encode(authority_vec[1]))) > + Some(authority_vec[1].to_string()) It seems like all changes to this file and config/node.rs could haven been its own patch, as they are not directly related to setting the apt proxy, but I'm fine with it as is too. > } else { > None > }; > @@ -107,6 +107,15 @@ impl ProxyConfig { > }) > }).map_err(|err| format_err!("parse_proxy_url failed: {}", err)) > } > + > + /// Assemble canonical proxy string (including scheme and port) > + pub fn to_proxy_string(&self) -> Result { > + let authority = build_authority(&self.host, self.port)?; > + Ok(match self.authorization { > + None => format!("http://{}", authority), > + Some(ref authorization) => format!("http://{}@{}", authorization, authority) > + }) > + } > } > > #[derive(Clone)] > @@ -241,7 +250,10 @@ impl hyper::service::Service for HttpsConnector { > > let mut connect_request = format!("CONNECT {0}:{1} HTTP/1.1\r\n", host, port); > if let Some(authorization) = authorization { > - connect_request.push_str(&format!("Proxy-Authorization: {}\r\n", authorization)); > + connect_request.push_str(&format!( > + "Proxy-Authorization: Basic {}\r\n", > + base64::encode(authorization), > + )); > } > connect_request.push_str(&format!("Host: {0}:{1}\r\n\r\n", host, port)); > >