Re: [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

Re: [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

[Wolfgang Bumiller]

> On 06/18/2021 8:53 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
> 
>  
> Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller:
> > 
> >> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
> >>>> +            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")
> >>>       {
> >>>           ...
> >>>       }
> >>>
> >>
> >> Misses FQDNs?
> > 
> > Such as?
> > 
> 
> http://security.debian.org.

Why is that not caught by `.ends_with(".debian.org")`?

Re: [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

[Fabian Ebner]

Am 18.06.21 um 08:56 schrieb Wolfgang Bumiller:
> 
>> On 06/18/2021 8:53 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
>>
>>   
>> Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller:
>>>
>>>> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
>>>>>> +            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")
>>>>>        {
>>>>>            ...
>>>>>        }
>>>>>
>>>>
>>>> Misses FQDNs?
>>>
>>> Such as?
>>>
>>
>> http://security.debian.org.
> 
> Why is that not caught by `.ends_with(".debian.org")`?
> 

Because of the final dot. But it is likely very uncommon and simply 
splitting by '.' leads to false results with e.g. 
http://security..debian.org too, so it might not be worth worrying about...

Re: [pve-devel] [pbs-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

[Fabian Ebner]

Am 18.06.21 um 08:58 schrieb Fabian Ebner:
> Am 18.06.21 um 08:56 schrieb Wolfgang Bumiller:
>>
>>> On 06/18/2021 8:53 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
>>>
>>> Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller:
>>>>
>>>>> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
>>>>>>> +            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")
>>>>>>        {
>>>>>>            ...
>>>>>>        }
>>>>>>
>>>>>
>>>>> Misses FQDNs?
>>>>
>>>> Such as?
>>>>
>>>
>>> http://security.debian.org.
>>
>> Why is that not caught by `.ends_with(".debian.org")`?
>>
> 
> Because of the final dot. But it is likely very uncommon and simply 
> splitting by '.' leads to false results with e.g. 
> http://security..debian.org too, so it might not be worth worrying about...
> 

Sorry, should've been http://security.debian..org

> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 

Re: [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

[Wolfgang Bumiller]

> On 06/18/2021 8:58 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
> 
>  
> Am 18.06.21 um 08:56 schrieb Wolfgang Bumiller:
> > 
> >> On 06/18/2021 8:53 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
> >>
> >>   
> >> Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller:
> >>>
> >>>> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
> >>>>>> +            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")
> >>>>>        {
> >>>>>            ...
> >>>>>        }
> >>>>>
> >>>>
> >>>> Misses FQDNs?
> >>>
> >>> Such as?
> >>>
> >>
> >> http://security.debian.org.
> > 
> > Why is that not caught by `.ends_with(".debian.org")`?
> > 
> 
> Because of the final dot.

Splitting at '.' gives you an empty element in your vector, so that's the same in your code...

Feel free to just strip the final dot, though, if it makes you feel any better :-P

> But it is likely very uncommon and simply 

Do people even really do that, ever, outside of zone files?

> splitting by '.' leads to false results with e.g. 
> http://security..debian.org too, so it might not be worth worrying about...

That doesn't work anyway...

Re: [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

[Fabian Ebner]

Am 18.06.21 um 09:16 schrieb Wolfgang Bumiller:
> 
>> On 06/18/2021 8:58 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
>>
>>   
>> Am 18.06.21 um 08:56 schrieb Wolfgang Bumiller:
>>>
>>>> On 06/18/2021 8:53 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
>>>>
>>>>    
>>>> Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller:
>>>>>
>>>>>> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
>>>>>>>> +            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")
>>>>>>>         {
>>>>>>>             ...
>>>>>>>         }
>>>>>>>
>>>>>>
>>>>>> Misses FQDNs?
>>>>>
>>>>> Such as?
>>>>>
>>>>
>>>> http://security.debian.org.
>>>
>>> Why is that not caught by `.ends_with(".debian.org")`?
>>>
>>
>> Because of the final dot.
> 
> Splitting at '.' gives you an empty element in your vector, so that's the same in your code...
> 

Good to know.

> Feel free to just strip the final dot, though, if it makes you feel any better :-P
> 
>> But it is likely very uncommon and simply
> 
> Do people even really do that, ever, outside of zone files?
> 
>> splitting by '.' leads to false results with e.g.
>> http://security..debian.org too, so it might not be worth worrying about...
> 
> That doesn't work anyway...
> 

Yeah, I meant http://security.debian..org and thought it would be a 
false positive, but if there's an empty element in the vector, it isn't.

Re: [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

[Wolfgang Bumiller]

> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
> >> +            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")
> >      {
> >          ...
> >      }
> > 
> 
> Misses FQDNs?

Such as?

Re: [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

[Fabian Ebner]

Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller:
> 
>> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
>>>> +            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")
>>>       {
>>>           ...
>>>       }
>>>
>>
>> Misses FQDNs?
> 
> Such as?
> 

http://security.debian.org.

[pve-devel] [PATCH-SERIES v6] APT repositories API/UI

[Fabian Ebner]

List the configured repositories, have some basic checks for them, and
allow upgrading the package distribution before a major release.

The plan is to use perlmod to make the Rust implementation available for PVE+PMG
as well.


Changes from v5:
    * The main addition is how the upgrade call is enabled/disabled:
      The idea is to do it in the proxmox-apt library itself and bump the
      versions accordingly. It'll always be disabled in the master branch and
      after each major release the major version will be bumped, while the
      stable branch will bump the minor version when it enables the call. Each
      product can then depend on the new major version in its master branch and
      on the new minor version in its stable branch.
      Hope I didn't miss a much easier/better way to do this.
    * Replace buster/updates with bullseye-security for the upgrade.
    * Assert that current os-release is what we expect for the upgrade function.
    * Add link to the upgrade docs in the upgrade button confirm dialog.
    * Avoid false negatives by marking *.proxmox.com *.debian.org as official
      (e.g. snapshot.debian.org was missing)
    * Handle port and userinfo in URI for host detection.


Still missing (intended as followups):
    * integration in PMG.

Changes v4 -> v5:
    * some minor style improvements
    * call cargo clean with make clean in proxmox-apt
    * require 'deb' package type to detect enterprise/no-subscription repos
    * moved common_digest helper to the library
    * add replace_suite function in proxmox-apt and refactored suite_is_variant
      for re-use
    * ui: add reload button
    * ui: don't pass undefined digest parameter
    * add upgrade button
    * add RFC for PBS upgrade call
    * add RFCs for PVE integration


Changes v3 -> v4:
    * incorporate Fabian G.'s feedback:
        * switch to a per-file approach
        * check for official host names
        * fix case-sensitivity issue for .sources keys
        * include digests
    * fix write issue when there are no components (in case of an absolute suite)
    * add more tests


Changes v2 -> v3:
    * incorporate Wolfgang's feedback
    * improve main warning's UI

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 (9):
  initial commit
  add files for Debian packaging
  add functions to check for Proxmox repositories
  add check_repositories function
  add common_digest helper
  add release_upgrade function and constants for the current and upgrade
    suite
  bump version to 0.1.1-1
  update for bullseye
  bump version to 1.0.0-1

Fabian Ebner (2):
  allow upgrade to bullseye
  bump version to 0.2.0-1


proxmox-widget-toolkit:

Fabian Ebner (3):
  add UI for APT repositories
  APT repositories: add warnings
  add upgrade button

 src/Makefile                |   1 +
 src/node/APTRepositories.js | 477 ++++++++++++++++++++++++++++++++++++
 2 files changed, 478 insertions(+)
 create mode 100644 src/node/APTRepositories.js


proxmox-backup:

Fabian Ebner (6):
  depend on new proxmox-apt crate
  api: apt: add repositories call
  ui: add APT repositories
  api: apt: add check_repositories_call
  add upgrade_repositories call
  enable release upgrade for package repositories

 Cargo.toml                  |   1 +
 debian/control              |   1 +
 src/api2/node/apt.rs        | 177 +++++++++++++++++++++++++++++++++++-
 www/ServerAdministration.js |  13 +++
 4 files changed, 191 insertions(+), 1 deletion(-)


pve-rs:

Fabian Ebner (3):
  initial commit
  add files for Debian packaging
  apt: add upgrade_repositories call

Fabian Ebner (1):
  depend on proxmox-apt 0.2.0


pve-manager:

Fabian Ebner (5):
  api: apt: add call to list repositories
  ui: add panel for listing APT repositories
  api: apt: add call for repository check
  api: apt: add upgrade repos call
  ui: node config: enable release upgrade button for package
    repositories

 PVE/API2/APT.pm             | 267 ++++++++++++++++++++++++++++++++++++
 www/manager6/node/Config.js |   9 ++
 2 files changed, 276 insertions(+)

-- 
2.20.1

[pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

[Fabian Ebner]

which checks for bad suites and official URIs.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

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("://") {
+        let mut host = uri.split_at(begin + 3).1;
+
+        if let Some(end) = host.find('/') {
+            host = host.split_at(end).0;
+        }
+
+        if let Some(begin) = host.find('@') {
+            host = host.split_at(begin + 1).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() {
+                if old_suites
+                    .iter()
+                    .any(|base_suite| suite_variant(suite).0 == *base_suite)
+                {
+                    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)
+                {
+                    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() {
+            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();
+
+                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<APTRepositoryInfo> {
+        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);
+        }
+
+        infos
+    }
+
+    /// Checks for official URIs.
+    pub fn check_uris(&self) -> Vec<APTRepositoryInfo> {
+        let mut infos = vec![];
+
+        for (n, repo) in self.repositories.iter().enumerate() {
+            if let Some((kind, message)) = repo.check_uris() {
+                infos.push(APTRepositoryInfo {
+                    path: self.path.clone(),
+                    number: n + 1,
+                    kind,
+                    message,
+                });
+            }
+        }
+
+        infos
+    }
 }
diff --git a/src/repositories/mod.rs b/src/repositories/mod.rs
index b7919a9..c2bbc06 100644
--- a/src/repositories/mod.rs
+++ b/src/repositories/mod.rs
@@ -4,7 +4,7 @@ use anyhow::{bail, format_err, Error};
 
 use crate::types::{
     APTRepository, APTRepositoryFile, APTRepositoryFileError, APTRepositoryFileType,
-    APTRepositoryOption,
+    APTRepositoryInfo, APTRepositoryOption,
 };
 
 mod list_parser;
@@ -148,6 +148,23 @@ impl APTRepositoryFile {
     }
 }
 
+/// Provides additional information about the repositories.
+///
+/// The kind of information can be:
+/// `warnings` for bad suites.
+/// `ignore-pre-upgrade-warning` when the next stable suite is configured.
+/// `badge` for official URIs.
+pub fn check_repositories(files: &[APTRepositoryFile]) -> Vec<APTRepositoryInfo> {
+    let mut infos = vec![];
+
+    for file in files.iter() {
+        infos.append(&mut file.check_suites());
+        infos.append(&mut file.check_uris());
+    }
+
+    infos
+}
+
 /// Checks if the enterprise repository for the specified Proxmox product is
 /// configured and enabled.
 pub fn enterprise_repository_enabled(files: &[APTRepositoryFile], product: &str) -> bool {
diff --git a/src/types.rs b/src/types.rs
index bbd8e7e..057fffa 100644
--- a/src/types.rs
+++ b/src/types.rs
@@ -244,3 +244,22 @@ impl std::error::Error for APTRepositoryFileError {
         None
     }
 }
+
+#[api]
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
+#[serde(rename_all = "lowercase")]
+/// Additional information for a repository.
+pub struct APTRepositoryInfo {
+    /// Path to the defining file.
+    #[serde(skip_serializing_if = "String::is_empty")]
+    pub path: String,
+
+    /// Number of the associated respository within the file.
+    pub number: usize,
+
+    /// Info kind (e.g. "warning")
+    pub kind: String,
+
+    /// Info message
+    pub message: String,
+}
diff --git a/tests/repositories.rs b/tests/repositories.rs
index ffb1888..9b0cd56 100644
--- a/tests/repositories.rs
+++ b/tests/repositories.rs
@@ -3,9 +3,10 @@ use std::path::PathBuf;
 use anyhow::{bail, format_err, Error};
 
 use proxmox_apt::repositories::{
-    enterprise_repository_enabled, no_subscription_repository_enabled, write_repositories,
+    check_repositories, enterprise_repository_enabled, no_subscription_repository_enabled,
+    write_repositories,
 };
-use proxmox_apt::types::APTRepositoryFile;
+use proxmox_apt::types::{APTRepositoryFile, APTRepositoryInfo};
 
 #[test]
 fn test_parse_write() -> Result<(), Error> {
@@ -159,3 +160,95 @@ fn test_proxmox_repositories() -> Result<(), Error> {
 
     Ok(())
 }
+
+#[test]
+fn test_check_repositories() -> Result<(), Error> {
+    let test_dir = std::env::current_dir()?.join("tests");
+    let read_dir = test_dir.join("sources.list.d");
+
+    let absolute_suite_list = read_dir.join("absolute_suite.list");
+    let mut file = APTRepositoryFile::new(&absolute_suite_list)?.unwrap();
+    file.parse()?;
+
+    let infos = check_repositories(&vec![file]);
+
+    assert_eq!(infos.is_empty(), true);
+    let pve_list = read_dir.join("pve.list");
+    let mut file = APTRepositoryFile::new(&pve_list)?.unwrap();
+    file.parse()?;
+
+    let path_string = pve_list.into_os_string().into_string().unwrap();
+
+    let mut expected_infos = vec![];
+    for n in 1..=5 {
+        expected_infos.push(APTRepositoryInfo {
+            path: path_string.clone(),
+            number: n,
+            kind: "badge".to_string(),
+            message: "official host name".to_string(),
+        });
+    }
+
+    let mut infos = check_repositories(&vec![file]);
+
+    assert_eq!(infos.sort(), expected_infos.sort());
+
+    let bad_sources = read_dir.join("bad.sources");
+    let mut file = APTRepositoryFile::new(&bad_sources)?.unwrap();
+    file.parse()?;
+
+    let path_string = bad_sources.into_os_string().into_string().unwrap();
+
+    let mut expected_infos = vec![
+        APTRepositoryInfo {
+            path: path_string.clone(),
+            number: 1,
+            kind: "warning".to_string(),
+            message: "suite 'sid' should not be used in production!".to_string(),
+        },
+        APTRepositoryInfo {
+            path: path_string.clone(),
+            number: 2,
+            kind: "warning".to_string(),
+            message: "old suite 'lenny-backports' configured!".to_string(),
+        },
+        APTRepositoryInfo {
+            path: path_string.clone(),
+            number: 3,
+            kind: "warning".to_string(),
+            message: "old suite 'stretch/updates' configured!".to_string(),
+        },
+        APTRepositoryInfo {
+            path: path_string.clone(),
+            number: 4,
+            kind: "warning".to_string(),
+            message: "use the name of the stable distribution instead of 'stable'!".to_string(),
+        },
+        APTRepositoryInfo {
+            path: path_string.clone(),
+            number: 5,
+            kind: "ignore-pre-upgrade-warning".to_string(),
+            message: "suite 'bullseye' should not be used in production!".to_string(),
+        },
+        APTRepositoryInfo {
+            path: path_string.clone(),
+            number: 6,
+            kind: "warning".to_string(),
+            message: "suite 'testing' should not be used in production!".to_string(),
+        },
+    ];
+    for n in 1..=6 {
+        expected_infos.push(APTRepositoryInfo {
+            path: path_string.clone(),
+            number: n,
+            kind: "badge".to_string(),
+            message: "official URI".to_string(),
+        });
+    }
+
+    let mut infos = check_repositories(&vec![file]);
+
+    assert_eq!(infos.sort(), expected_infos.sort());
+
+    Ok(())
+}
diff --git a/tests/sources.list.d.expected/bad.sources b/tests/sources.list.d.expected/bad.sources
new file mode 100644
index 0000000..b630c89
--- /dev/null
+++ b/tests/sources.list.d.expected/bad.sources
@@ -0,0 +1,30 @@
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: sid
+Components: main contrib
+
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: lenny-backports
+Components: contrib
+
+Types: deb
+URIs: http://security.debian.org:80
+Suites: stretch/updates
+Components: main contrib
+
+Types: deb
+URIs: http://ftp.at.debian.org:80/debian
+Suites: stable
+Components: main
+
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: bullseye
+Components: main
+
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: testing
+Components: main
+
diff --git a/tests/sources.list.d/bad.sources b/tests/sources.list.d/bad.sources
new file mode 100644
index 0000000..1aab2ea
--- /dev/null
+++ b/tests/sources.list.d/bad.sources
@@ -0,0 +1,29 @@
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: sid
+Components: main contrib
+
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: lenny-backports
+Components: contrib
+
+Types: deb
+URIs: http://security.debian.org:80
+Suites: stretch/updates
+Components: main contrib
+
+Suites: stable
+URIs: http://ftp.at.debian.org:80/debian
+Components: main
+Types: deb
+
+Suites: bullseye
+URIs: http://ftp.at.debian.org/debian
+Components: main
+Types: deb
+
+Suites: testing
+URIs: http://ftp.at.debian.org/debian
+Components: main
+Types: deb
-- 
2.20.1

Re: [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

[Wolfgang Bumiller]

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 <f.ebner@proxmox.com>
> ---
> 
> 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<APTRepositoryInfo> {
> +        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)

Re: [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

[Fabian Ebner]

Am 17.06.21 um 10:39 schrieb Wolfgang Bumiller:
> 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 <f.ebner@proxmox.com>
>> ---
>>
>> 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")
>      {
>          ...
>      }
> 

Misses FQDNs? Thanks for the tips, I was not aware that one can do 
tail-matching with the matches! macro.

>> +
>> +                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<APTRepositoryInfo> {
>> +        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)
> 

Re: [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

[Fabian Grünbichler]

On June 11, 2021 1:43 pm, Fabian Ebner wrote:
> which checks for bad suites and official URIs.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> 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> {

this has false-positives for file:// URIs. the way it is currently used, 
it might make sense to limit it to http(s)?

> +    if let Some(begin) = uri.find("://") {
> +        let mut host = uri.split_at(begin + 3).1;
> +
> +        if let Some(end) = host.find('/') {
> +            host = host.split_at(end).0;
> +        }
> +
> +        if let Some(begin) = host.find('@') {
> +            host = host.split_at(begin + 1).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() {
> +                if old_suites
> +                    .iter()
> +                    .any(|base_suite| suite_variant(suite).0 == *base_suite)
> +                {
> +                    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)
> +                {
> +                    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() {
> +            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();
> +
> +                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<APTRepositoryInfo> {
> +        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);
> +        }
> +
> +        infos
> +    }
> +
> +    /// Checks for official URIs.
> +    pub fn check_uris(&self) -> Vec<APTRepositoryInfo> {
> +        let mut infos = vec![];
> +
> +        for (n, repo) in self.repositories.iter().enumerate() {
> +            if let Some((kind, message)) = repo.check_uris() {
> +                infos.push(APTRepositoryInfo {
> +                    path: self.path.clone(),
> +                    number: n + 1,
> +                    kind,
> +                    message,
> +                });
> +            }
> +        }
> +
> +        infos
> +    }
>  }
> diff --git a/src/repositories/mod.rs b/src/repositories/mod.rs
> index b7919a9..c2bbc06 100644
> --- a/src/repositories/mod.rs
> +++ b/src/repositories/mod.rs
> @@ -4,7 +4,7 @@ use anyhow::{bail, format_err, Error};
>  
>  use crate::types::{
>      APTRepository, APTRepositoryFile, APTRepositoryFileError, APTRepositoryFileType,
> -    APTRepositoryOption,
> +    APTRepositoryInfo, APTRepositoryOption,
>  };
>  
>  mod list_parser;
> @@ -148,6 +148,23 @@ impl APTRepositoryFile {
>      }
>  }
>  
> +/// Provides additional information about the repositories.
> +///
> +/// The kind of information can be:
> +/// `warnings` for bad suites.
> +/// `ignore-pre-upgrade-warning` when the next stable suite is configured.
> +/// `badge` for official URIs.
> +pub fn check_repositories(files: &[APTRepositoryFile]) -> Vec<APTRepositoryInfo> {
> +    let mut infos = vec![];
> +
> +    for file in files.iter() {
> +        infos.append(&mut file.check_suites());
> +        infos.append(&mut file.check_uris());
> +    }
> +
> +    infos
> +}
> +
>  /// Checks if the enterprise repository for the specified Proxmox product is
>  /// configured and enabled.
>  pub fn enterprise_repository_enabled(files: &[APTRepositoryFile], product: &str) -> bool {
> diff --git a/src/types.rs b/src/types.rs
> index bbd8e7e..057fffa 100644
> --- a/src/types.rs
> +++ b/src/types.rs
> @@ -244,3 +244,22 @@ impl std::error::Error for APTRepositoryFileError {
>          None
>      }
>  }
> +
> +#[api]
> +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
> +#[serde(rename_all = "lowercase")]
> +/// Additional information for a repository.
> +pub struct APTRepositoryInfo {
> +    /// Path to the defining file.
> +    #[serde(skip_serializing_if = "String::is_empty")]
> +    pub path: String,
> +
> +    /// Number of the associated respository within the file.
> +    pub number: usize,
> +
> +    /// Info kind (e.g. "warning")
> +    pub kind: String,
> +
> +    /// Info message
> +    pub message: String,
> +}
> diff --git a/tests/repositories.rs b/tests/repositories.rs
> index ffb1888..9b0cd56 100644
> --- a/tests/repositories.rs
> +++ b/tests/repositories.rs
> @@ -3,9 +3,10 @@ use std::path::PathBuf;
>  use anyhow::{bail, format_err, Error};
>  
>  use proxmox_apt::repositories::{
> -    enterprise_repository_enabled, no_subscription_repository_enabled, write_repositories,
> +    check_repositories, enterprise_repository_enabled, no_subscription_repository_enabled,
> +    write_repositories,
>  };
> -use proxmox_apt::types::APTRepositoryFile;
> +use proxmox_apt::types::{APTRepositoryFile, APTRepositoryInfo};
>  
>  #[test]
>  fn test_parse_write() -> Result<(), Error> {
> @@ -159,3 +160,95 @@ fn test_proxmox_repositories() -> Result<(), Error> {
>  
>      Ok(())
>  }
> +
> +#[test]
> +fn test_check_repositories() -> Result<(), Error> {
> +    let test_dir = std::env::current_dir()?.join("tests");
> +    let read_dir = test_dir.join("sources.list.d");
> +
> +    let absolute_suite_list = read_dir.join("absolute_suite.list");
> +    let mut file = APTRepositoryFile::new(&absolute_suite_list)?.unwrap();
> +    file.parse()?;
> +
> +    let infos = check_repositories(&vec![file]);
> +
> +    assert_eq!(infos.is_empty(), true);
> +    let pve_list = read_dir.join("pve.list");
> +    let mut file = APTRepositoryFile::new(&pve_list)?.unwrap();
> +    file.parse()?;
> +
> +    let path_string = pve_list.into_os_string().into_string().unwrap();
> +
> +    let mut expected_infos = vec![];
> +    for n in 1..=5 {
> +        expected_infos.push(APTRepositoryInfo {
> +            path: path_string.clone(),
> +            number: n,
> +            kind: "badge".to_string(),
> +            message: "official host name".to_string(),
> +        });
> +    }
> +
> +    let mut infos = check_repositories(&vec![file]);
> +
> +    assert_eq!(infos.sort(), expected_infos.sort());
> +
> +    let bad_sources = read_dir.join("bad.sources");
> +    let mut file = APTRepositoryFile::new(&bad_sources)?.unwrap();
> +    file.parse()?;
> +
> +    let path_string = bad_sources.into_os_string().into_string().unwrap();
> +
> +    let mut expected_infos = vec![
> +        APTRepositoryInfo {
> +            path: path_string.clone(),
> +            number: 1,
> +            kind: "warning".to_string(),
> +            message: "suite 'sid' should not be used in production!".to_string(),
> +        },
> +        APTRepositoryInfo {
> +            path: path_string.clone(),
> +            number: 2,
> +            kind: "warning".to_string(),
> +            message: "old suite 'lenny-backports' configured!".to_string(),
> +        },
> +        APTRepositoryInfo {
> +            path: path_string.clone(),
> +            number: 3,
> +            kind: "warning".to_string(),
> +            message: "old suite 'stretch/updates' configured!".to_string(),
> +        },
> +        APTRepositoryInfo {
> +            path: path_string.clone(),
> +            number: 4,
> +            kind: "warning".to_string(),
> +            message: "use the name of the stable distribution instead of 'stable'!".to_string(),
> +        },
> +        APTRepositoryInfo {
> +            path: path_string.clone(),
> +            number: 5,
> +            kind: "ignore-pre-upgrade-warning".to_string(),
> +            message: "suite 'bullseye' should not be used in production!".to_string(),
> +        },
> +        APTRepositoryInfo {
> +            path: path_string.clone(),
> +            number: 6,
> +            kind: "warning".to_string(),
> +            message: "suite 'testing' should not be used in production!".to_string(),
> +        },
> +    ];
> +    for n in 1..=6 {
> +        expected_infos.push(APTRepositoryInfo {
> +            path: path_string.clone(),
> +            number: n,
> +            kind: "badge".to_string(),
> +            message: "official URI".to_string(),
> +        });
> +    }
> +
> +    let mut infos = check_repositories(&vec![file]);
> +
> +    assert_eq!(infos.sort(), expected_infos.sort());
> +
> +    Ok(())
> +}
> diff --git a/tests/sources.list.d.expected/bad.sources b/tests/sources.list.d.expected/bad.sources
> new file mode 100644
> index 0000000..b630c89
> --- /dev/null
> +++ b/tests/sources.list.d.expected/bad.sources
> @@ -0,0 +1,30 @@
> +Types: deb
> +URIs: http://ftp.at.debian.org/debian
> +Suites: sid
> +Components: main contrib
> +
> +Types: deb
> +URIs: http://ftp.at.debian.org/debian
> +Suites: lenny-backports
> +Components: contrib
> +
> +Types: deb
> +URIs: http://security.debian.org:80
> +Suites: stretch/updates
> +Components: main contrib
> +
> +Types: deb
> +URIs: http://ftp.at.debian.org:80/debian
> +Suites: stable
> +Components: main
> +
> +Types: deb
> +URIs: http://ftp.at.debian.org/debian
> +Suites: bullseye
> +Components: main
> +
> +Types: deb
> +URIs: http://ftp.at.debian.org/debian
> +Suites: testing
> +Components: main
> +
> diff --git a/tests/sources.list.d/bad.sources b/tests/sources.list.d/bad.sources
> new file mode 100644
> index 0000000..1aab2ea
> --- /dev/null
> +++ b/tests/sources.list.d/bad.sources
> @@ -0,0 +1,29 @@
> +Types: deb
> +URIs: http://ftp.at.debian.org/debian
> +Suites: sid
> +Components: main contrib
> +
> +Types: deb
> +URIs: http://ftp.at.debian.org/debian
> +Suites: lenny-backports
> +Components: contrib
> +
> +Types: deb
> +URIs: http://security.debian.org:80
> +Suites: stretch/updates
> +Components: main contrib
> +
> +Suites: stable
> +URIs: http://ftp.at.debian.org:80/debian
> +Components: main
> +Types: deb
> +
> +Suites: bullseye
> +URIs: http://ftp.at.debian.org/debian
> +Components: main
> +Types: deb
> +
> +Suites: testing
> +URIs: http://ftp.at.debian.org/debian
> +Components: main
> +Types: deb
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>