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

Re: [pbs-devel] [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: [pbs-devel] [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.

Re: [pbs-devel] [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: [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function

[Wolfgang Bumiller]

> On 06/18/2021 9:07 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
> 
>  
> 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

I still can't navigate to that in my browser ;-)

Re: [pbs-devel] [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: [pbs-devel] [pve-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: [pbs-devel] [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: [pbs-devel] [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: [pbs-devel] [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: [pbs-devel] [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
> 
> 
> 

Re: [pbs-devel] [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)