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 B2B247B55E for ; Wed, 12 May 2021 12:21:42 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A86BEBE9F for ; Wed, 12 May 2021 12:21:12 +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 A7094BE91 for ; Wed, 12 May 2021 12:21:11 +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 805FE41A04 for ; Wed, 12 May 2021 12:21:11 +0200 (CEST) To: Thomas Lamprecht , Proxmox Backup Server development discussion References: <20210511065429.22763-1-dietmar@proxmox.com> <4a78fa42-02de-664f-90f6-eaa16fe9ea31@proxmox.com> From: Dietmar Maurer Message-ID: Date: Wed, 12 May 2021 12:21:09 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <4a78fa42-02de-664f-90f6-eaa16fe9ea31@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-SPAM-LEVEL: Spam detection results: 0 AWL 0.221 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pbs-devel] applied: [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:21:42 -0000 applied with suggested changes On 5/12/21 12:01 PM, Thomas Lamprecht wrote: > 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)); >> >>