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 2EFC769779 for ; Tue, 23 Mar 2021 11:30:07 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1A4CF2CA91 for ; Tue, 23 Mar 2021 11:29:37 +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 1927B2CA87 for ; Tue, 23 Mar 2021 11:29:36 +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 D3B0E46365 for ; Tue, 23 Mar 2021 11:29:35 +0100 (CET) Date: Tue, 23 Mar 2021 11:29:25 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20210322115945.1362-1-f.ebner@proxmox.com> In-Reply-To: <20210322115945.1362-1-f.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1616493992.n5qqqz9oq6.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.028 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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. [apt.rs, proxmox.com] Subject: Re: [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI 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: Tue, 23 Mar 2021 10:30:07 -0000 On March 22, 2021 12:59 pm, Fabian Ebner wrote: > List the configured repositories and have some basic checks for them. >=20 > The plan is to use perlmod to make the Rust implementation available for = PVE+PMG > as well. >=20 > There's still the question if introducing a digest is worth it. At the mo= ment, > the warnings returned by the checkrepositories call might not match up wi= th the > repositories returned previously, but that's a rather minor issue as edit= ing > repositories is a rare operation. > Should a digest be added now to be future-proof? Should it live in the > proxmox-apt crate and be file-level, or would it be enough to hash the re= sult in > the PBS API call and use that? The latter seems like the more pragmatic a= pproach > and avoids cluttering the APT backend. gave this a spin, looks mostly good to me, some things I noticed: "man deb822" states: Field names are not case-sensitive, but it is usual to capitalize the=20 field names using mixed case as shown below. Field values are=20 case-sensitive unless the description of the field says otherwise. the parser here is case-sensitive and chokes on .sources files which APT=20 happily parses & uses. the "options" part is missing support for the following "feature" ("man=20 sources.list"): Multivalue options also have -=3D and +=3D as separators, which instead of= =20 replacing the default with the given value(s) modify the default=20 value(s) to remove or include the given values. I haven't tested that one though ;) if a file cannot be parsed or is malformed (e.g. because I put "Uris"=20 instead of "URIs" in a .sources file ;)), the whole API call fails with=20 400. it might be more user-friendly to mark indiviual .list/.sources=20 files as containing invalid entries which are not displayed, and still=20 return the rest? might make the result less actionable since we don't=20 have the complete picture, but it still might be better than a single=20 error message for one of X files.. the GUI already makes the API request when opening the 'Administration'=20 part, which then displays an error modal if any of the files is wrong -=20 even though we are not on the repositories tab at that moment. this is=20 confusing. also, opening the repositories tab triggers a (re?)load=20 anyhow.. we have a warning for Debian unstable, but none for Debian testing which=20 should also never be enabled on a production machine. we might want to match known-official repo URIs (ftp.*.debian.org,=20 deb.debian.org, download.proxmox.com, enterprise.proxmox.com) to mark=20 "potentially dangerous external repositories" (or to give the official=20 ones a "official mirror" badge or something like that) a digest based on raw file contents or some other stable means seems=20 sensible to me. hashing just the results of the parser is usually wrong=20 (digest mismatch with different parser versions, parser bugs causing=20 indeterminism with rare setups, ...). >=20 >=20 > Changes from v2: > * incorporate Wolfgang's feedback > * improve main warning's UI >=20 > Still missing: > * Upgrade suite/distribuiton button to be used before major release > upgrades (but it's really simply to add that now). > * perlmod magic and integration in PVE and PMG. >=20 > Changes v1 -> v2: > * Perl -> Rust > * PVE -> PBS > * Don't rely on regexes for parsing. > * Add writer and tests. > * UI: pin warnings to the repository they're for. > * Keep order of options consistent with configuration. > * Smaller things noted on the individual patches. >=20 >=20 > proxmox-apt: >=20 > Fabian Ebner (4): > initial commit > add files for Debian packaging > add functions to check for Proxmox repositories > add check_repositories function >=20 >=20 > proxmox-backup: >=20 > Fabian Ebner (4): > depend on new proxmox-apt crate > api: apt: add repositories call > ui: add repositories > add check_repositories_call >=20 > Cargo.toml | 1 + > src/api2/node/apt.rs | 78 +++++++++++++++++++++++++++++++++++++ > www/ServerAdministration.js | 7 ++++ > 3 files changed, 86 insertions(+) >=20 >=20 > widget-toolkit: >=20 > Fabian Ebner (2): > add UI for APT repositories > APT repositories: add warnings >=20 > src/Makefile | 1 + > src/node/APTRepositories.js | 295 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 296 insertions(+) > create mode 100644 src/node/APTRepositories.js >=20 > --=20 > 2.20.1 >=20 >=20 >=20 > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >=20 >=20 >=20 =