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 7018172E7F; Thu, 17 Jun 2021 10:39:36 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5AE01191B2; Thu, 17 Jun 2021 10:39:06 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 4C8AC191A4; Thu, 17 Jun 2021 10:39:04 +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 2187C44115; Thu, 17 Jun 2021 10:39:04 +0200 (CEST) Date: Thu, 17 Jun 2021 10:39:02 +0200 From: Wolfgang Bumiller To: Fabian Ebner Cc: pve-devel@lists.proxmox.com, pbs-devel@lists.proxmox.com Message-ID: <20210617083902.32lau57pd3gjt5qe@wobu-vie.proxmox.com> References: <20210611114418.28772-1-f.ebner@proxmox.com> <20210611114418.28772-5-f.ebner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210611114418.28772-5-f.ebner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.874 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 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] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function 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: Thu, 17 Jun 2021 08:39:36 -0000 some non-blocking cleanups in case you do another version: On Fri, Jun 11, 2021 at 01:43:53PM +0200, Fabian Ebner wrote: > which checks for bad suites and official URIs. > > Signed-off-by: Fabian Ebner > --- > > Changes from v5: > * split out host_from_uri helper and also handle userinfo and port > * test an offical URI with port > * match all *.debian.org and *.proxmox.com as official to avoid (future) > false negatives. > * add bookworm and trixie codenames to the list of new_suites > > src/repositories/check.rs | 174 +++++++++++++++++++++- > src/repositories/mod.rs | 19 ++- > src/types.rs | 19 +++ > tests/repositories.rs | 97 +++++++++++- > tests/sources.list.d.expected/bad.sources | 30 ++++ > tests/sources.list.d/bad.sources | 29 ++++ > 6 files changed, 364 insertions(+), 4 deletions(-) > create mode 100644 tests/sources.list.d.expected/bad.sources > create mode 100644 tests/sources.list.d/bad.sources > > diff --git a/src/repositories/check.rs b/src/repositories/check.rs > index a682b69..585c28d 100644 > --- a/src/repositories/check.rs > +++ b/src/repositories/check.rs > @@ -1,6 +1,45 @@ > use anyhow::{bail, Error}; > > -use crate::types::{APTRepository, APTRepositoryFileType, APTRepositoryPackageType}; > +use crate::types::{ > + APTRepository, APTRepositoryFile, APTRepositoryFileType, APTRepositoryInfo, > + APTRepositoryPackageType, > +}; > + > +/// Splits the suite into its base part and variant. > +fn suite_variant(suite: &str) -> (&str, &str) { > + let variants = ["-backports-sloppy", "-backports", "-updates", "/updates"]; > + > + for variant in variants.iter() { > + if let Some(base) = suite.strip_suffix(variant) { > + return (base, variant); > + } > + } > + > + (suite, "") > +} > + > +/// Get the host part from a given URI. > +fn host_from_uri(uri: &str) -> Option<&str> { > + if let Some(begin) = uri.find("://") { You could shorten this via `?` (since the function itself also returns an `Option`): let begin = uri.find("://")?; > + let mut host = uri.split_at(begin + 3).1; > + > + if let Some(end) = host.find('/') { > + host = host.split_at(end).0; Personally I'd prefer `host = &host[..end]`, but it probably compiles to the same code in the end. > + } > + > + if let Some(begin) = host.find('@') { > + host = host.split_at(begin + 1).1; (Similarly: `host = &host[(begin + 1)..]`) > + } > + > + if let Some(end) = host.find(':') { > + host = host.split_at(end).0; > + } > + > + return Some(host); > + } > + > + None > +} > > impl APTRepository { > /// Makes sure that all basic properties of a repository are present and > @@ -102,4 +141,137 @@ impl APTRepository { > false > } > } > + > + /// Checks if old or unstable suites are configured and also that the > + /// `stable` keyword is not used. > + fn check_suites(&self, add_info: &mut dyn FnMut(String, String)) { > + let old_suites = [ > + "lenny", > + "squeeze", > + "wheezy", > + "jessie", > + "stretch", > + "oldoldstable", > + "oldstable", > + ]; > + > + let next_suite = "bullseye"; > + > + let new_suites = [ > + "bookworm", > + "trixie", > + "testing", > + "unstable", > + "sid", > + "experimental", > + ]; > + > + if self > + .types > + .iter() > + .any(|package_type| *package_type == APTRepositoryPackageType::Deb) > + { > + for suite in self.suites.iter() { maybe cache `suite_variant(suite).0` at this point let variant = suite_variant(suite).0; > + if old_suites > + .iter() > + .any(|base_suite| suite_variant(suite).0 == *base_suite) ^ then this could be if old_suites.contains(&variant) { I think > + { > + add_info( > + "warning".to_string(), > + format!("old suite '{}' configured!", suite), > + ); > + } > + > + if suite_variant(suite).0 == next_suite { > + add_info( > + "ignore-pre-upgrade-warning".to_string(), > + format!("suite '{}' should not be used in production!", suite), > + ); > + } > + > + if new_suites > + .iter() > + .any(|base_suite| suite_variant(suite).0 == *base_suite) ^ same > + { > + add_info( > + "warning".to_string(), > + format!("suite '{}' should not be used in production!", suite), > + ); > + } > + > + if suite_variant(suite).0 == "stable" { > + add_info( > + "warning".to_string(), > + "use the name of the stable distribution instead of 'stable'!".to_string(), > + ); > + } > + } > + } > + } > + > + /// Checks if an official host is configured in the repository. > + fn check_uris(&self) -> Option<(String, String)> { > + let official_host = |domains: &Vec<&str>| match domains.split_last() { Drop this entire beast (see below), but as a review of it: You can use the slice[1] & rest[2] pattern syntax here: #[allow(clippy::match_like_matches_macro)] match domains[..] { // the `[..]` part is required here [.., "proxmox", "com"] => true, [.., "debian", "org"] => true, _ => false, } Or more concise (but I do find the above a bit quicker to glance over, hence the 'clippy' hint ;-) ): matches!(domains[..], [.., "proxmox", "com"] | [.., "debian", "org"]); [1] https://doc.rust-lang.org/reference/patterns.html#slice-patterns [2] https://doc.rust-lang.org/reference/patterns.html#rest-patterns > + Some((last, rest)) => match rest.split_last() { > + Some((second_to_last, _rest)) => { > + (*last == "org" && *second_to_last == "debian") > + || (*last == "com" && *second_to_last == "proxmox") > + } > + None => false, > + }, > + None => false, > + }; > + > + for uri in self.uris.iter() { > + if let Some(host) = host_from_uri(uri) { > + let domains = host.split('.').collect(); ^ But instead of building a vector here, why not just do: if host == "proxmox.com" || host.ends_with(".proxmox.com") || host == "debian.org" || host.ends_with(".debian.org") { ... } > + > + if official_host(&domains) { > + return Some(("badge".to_string(), "official host name".to_string())); > + } > + } > + } > + > + None > + } > +} > + > +impl APTRepositoryFile { > + /// Checks if old or unstable suites are configured and also that the > + /// `stable` keyword is not used. > + pub fn check_suites(&self) -> Vec { > + let mut infos = vec![]; > + > + for (n, repo) in self.repositories.iter().enumerate() { > + let mut add_info = |kind, message| { > + infos.push(APTRepositoryInfo { > + path: self.path.clone(), > + number: n + 1, > + kind, > + message, > + }) > + }; > + repo.check_suites(&mut add_info); ^ minor nit: the `check_suites` you're calling here is only called at this one spot and private, so personally I'd prefer an `impl FnMut` or generic over a trait object, (also you could inline the closure here)