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 696D269EF1 for ; Wed, 24 Mar 2021 10:40:46 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5D05E36D00 for ; Wed, 24 Mar 2021 10:40:46 +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 460A236CF6 for ; Wed, 24 Mar 2021 10:40:45 +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 0894646449 for ; Wed, 24 Mar 2021 10:40:45 +0100 (CET) From: Fabian Ebner To: pbs-devel@lists.proxmox.com, =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= References: <20210322115945.1362-1-f.ebner@proxmox.com> <1616493992.n5qqqz9oq6.astroid@nora.none> Message-ID: <93bd0e38-56fb-70eb-1379-9b736b5fb7b5@proxmox.com> Date: Wed, 24 Mar 2021 10:40:43 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <1616493992.n5qqqz9oq6.astroid@nora.none> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.444 Adjusted score from AWL reputation of From: address KAM_ASCII_DIVIDERS 0.8 Spam that uses ascii formatting tricks 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) POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes 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: Wed, 24 Mar 2021 09:40:46 -0000 Am 23.03.21 um 11:29 schrieb Fabian Grünbichler: > On March 22, 2021 12:59 pm, Fabian Ebner wrote: >> List the configured repositories and have some basic checks for them. >> >> The plan is to use perlmod to make the Rust implementation available for PVE+PMG >> as well. >> >> There's still the question if introducing a digest is worth it. At the moment, >> the warnings returned by the checkrepositories call might not match up with the >> repositories returned previously, but that's a rather minor issue as editing >> 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 result in >> the PBS API call and use that? The latter seems like the more pragmatic approach >> 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 > field names using mixed case as shown below. Field values are > case-sensitive unless the description of the field says otherwise. > > the parser here is case-sensitive and chokes on .sources files which APT > happily parses & uses. > Thanks for catching that, I'll fix this in the next version. > the "options" part is missing support for the following "feature" ("man > sources.list"): > > Multivalue options also have -= and += as separators, which instead of > replacing the default with the given value(s) modify the default > value(s) to remove or include the given values. > > I haven't tested that one though ;) > The parser just uses "option+" and "option-" as the option keys then. It /could/ be interpreted on parsing, but if the user chose to use this notation, there's probably a reason and it should be written out the same way it came in again. I'll add a test though. > if a file cannot be parsed or is malformed (e.g. because I put "Uris" > instead of "URIs" in a .sources file ;)), the whole API call fails with > 400. it might be more user-friendly to mark indiviual .list/.sources > files as containing invalid entries which are not displayed, and still > return the rest? might make the result less actionable since we don't > have the complete picture, but it still might be better than a single > error message for one of X files.. > I did consider something like this for a bit, but not sure how to cleanly organize the API then. The call should still error out in my opinion and syntactic errors should be rather rare anyways (apt also just complains when it cannot parse). We could continue parsing and collect all the errors at once at least, but not sure if that's worth it? > the GUI already makes the API request when opening the 'Administration' > part, which then displays an error modal if any of the files is wrong - > even though we are not on the repositories tab at that moment. this is > confusing. also, opening the repositories tab triggers a (re?)load > anyhow.. > Surely not optimal. Hopefully easily fixed by removing the 'reload' call in 'initComponent'. Too used to having active on init... > we have a warning for Debian unstable, but none for Debian testing which > should also never be enabled on a production machine. > If there is an "upgrade suites" button/API call, then there would be warnings after using that. But since enabling that button/API call needs to happen anyways before each major release, I guess removing the e.g. 'bullseye' warnings then is just one more place to touch. Or maybe add a 'before_major_release' parameter to check_repositories and also to the UI? Then only the product specific code needs to be touched before each major release. Of course there still needs to be a new version of the library for after each major release. > we might want to match known-official repo URIs (ftp.*.debian.org, > deb.debian.org, download.proxmox.com, enterprise.proxmox.com) to mark > "potentially dangerous external repositories" (or to give the official > ones a "official mirror" badge or something like that) > Might be done as part of the check_repositories call if we go for warnings. If we go for badges, we'd need something new or change the interface for check_repositories. I feel like badges would be preferable though... > a digest based on raw file contents or some other stable means seems > sensible to me. hashing just the results of the parser is usually wrong > (digest mismatch with different parser versions, parser bugs causing > indeterminism with rare setups, ...). > I'll try and add a digest that's returned when parsing from the raw contents and then the API calls can re-compare against that before doing any further calls to the library. >> >> >> Changes from v2: >> * incorporate Wolfgang's feedback >> * improve main warning's UI >> >> 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. >> >> 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. >> >> >> proxmox-apt: >> >> Fabian Ebner (4): >> initial commit >> add files for Debian packaging >> add functions to check for Proxmox repositories >> add check_repositories function >> >> >> proxmox-backup: >> >> Fabian Ebner (4): >> depend on new proxmox-apt crate >> api: apt: add repositories call >> ui: add repositories >> add check_repositories_call >> >> Cargo.toml | 1 + >> src/api2/node/apt.rs | 78 +++++++++++++++++++++++++++++++++++++ >> www/ServerAdministration.js | 7 ++++ >> 3 files changed, 86 insertions(+) >> >> >> widget-toolkit: >> >> Fabian Ebner (2): >> add UI for APT repositories >> APT repositories: add warnings >> >> src/Makefile | 1 + >> src/node/APTRepositories.js | 295 ++++++++++++++++++++++++++++++++++++ >> 2 files changed, 296 insertions(+) >> create mode 100644 src/node/APTRepositories.js >> >> -- >> 2.20.1 >> >> >> >> _______________________________________________ >> pbs-devel mailing list >> pbs-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >> >> >> > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > >