[opensuse] Add FileDigestCheck

[marxin]

No description provided.

[marxin]

@mgerstner

I've got few notes about File Digest check:

• '{group}-file-digest-mismatch' was removed; if you have multiple digest groups, you can't know which one should be the match and isn't
• each FileDigestLocation has mandatory attribute FollowSymlinks, hope it's acceptable as we have just couple of these
• I split checking of symlinks and ghost files in FileDigestCheck; if there's an error, file digest groups are not iterated at all

[mgerstner]

I looked at the code and tested the new check. The logic should be okay now. I still have a couple of detailed comments though that I will add individually soon.

[mgerstner]

I find it a bit confusing with many things going on in parallel in the review/discussion. Maybe we should first agree on the suitable configuration structure before continuing with the other details. I still don't find the current suggestion very intuitive. I dived a bit deeper into what TOML provides and have come up with this approach:

[[FileDigestGroup."sarg"]]
note = "some package with a lot of cron jobs"

    [FileDigestGroup."sarg".audits."bsc#4711"]
    digests = [
	{
		path = "/etc/cron.daily/suse.de-sarg",
		algorithm = "sha256",
		hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
	},
	{
		path = "/etc/cron.weekly/suse.de-sarg",
		algorithm = "sha256",
		hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
	}
    ]

    [FileDigestGroup."sarg".audits."bsc#4712"]
    digests = [
	{
		path = "/etc/cron.weekly/suse.de-sarg",
		algorithm = "sha256",
		hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
	},
	{
		path = "/etc/cron.weekly/suse.de-sarg",
		algorithm = "sha256",
		hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
	}
    ]

[[FileDigestGroup."someotherpackage"]]
note = "some other package"

    [FileDigestGroup."someotherpackage".audits."bsc#815"]
    digests = [
	{
		path = "/some/path",
		algorithm = "skip"
	},
	{
		path = "/root/somewhere",
		algorithm = "sha256",
		hash = "d536dc687798189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
	}
    ]

This approach still repeats the package names in the sub-tables, but this is due to a limitation of TOML not allowing newlines in inline-tables, see toml-lang/toml#516. The resulting python data structure looks like this:

{'FileDigestGroup': {'sarg': [{'audits': {'bsc#4711': {'digests': [{'algorithm': 'sha256',
                                                                    'hash': 'd536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359',
                                                                    'path': '/etc/cron.daily/suse.de-sarg'},
                                                                   {'algorithm': 'sha256',
                                                                    'hash': 'd536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359',
                                                                    'path': '/etc/cron.weekly/suse.de-sarg'}]},
                                          'bsc#4712': {'digests': [{'algorithm': 'sha256',
                                                                    'hash': 'd536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359',
                                                                    'path': '/etc/cron.weekly/suse.de-sarg'},
                                                                   {'algorithm': 'sha256',
                                                                    'hash': 'd536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359',
                                                                    'path': '/etc/cron.weekly/suse.de-sarg'}]}},
                               'note': 'some package with a lot of cron jobs'}],
                     'someotherpackage': [{'audits': {'bsc#815': {'digests': [{'algorithm': 'skip',
                                                                               'path': '/some/path'},
                                                                              {'algorithm': 'sha256',
                                                                               'hash': 'd536dc687798189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359',
                                                                               'path': '/root/somewhere'}]}},
                                           'note': 'some other package'}]}}

What do you think?

[marxin]

I like the suggested format with one small nit: replacing [[FileDigestGroup."sarg"]] with [FileDigestGroup."sarg"].

I've just pushed a patch that supports your format.

[marxin]

@mgerstner Do you feel the current shape of FileDigestCheck supports what you need?

[mgerstner]

Okay the configuration syntax looks good now.

The check still behaves erroneous, though. _calculate_errors_for_digest_group() does not consider self.digest_configurations, i.e. it applies digest groups to all files, not only the files that start with the expected prefix path.

In the -file-digest-mismatch error case I'd like to see the expected and encountered digests.

I also get a KeyError in line 111, because the check tries to access a file in the package that isn't even existing. Is there a possibility to show the full exception backtrace when running rpmlint and a check fails internally? Because this error here was not very helpful:

$ rpmlint /var/cache/zypp/packages/download.opensuse.org-oss_2/x86_64/ruby2.7-rubygem-arel-doc-9.0.0-2.10.x86_64.rpm
(none): E: fatal error while reading /var/cache/zypp/packages/download.opensuse.org-oss_2/x86_64/ruby2.7-rubygem-arel-doc-9.0.0-2.10.x86_64.rpm: '/usr/sbin/sarg-reports'

For contextual files like /usr/sbin/sarg-reports the reported "group" is None, because the prefix of this path is not in a cron directory. It could make sense to assign each FileDigestGroup a type. Now that I think about it, something like this will most certainly become necessary, because we need to be able to keep parallel whitelists for cron, systemd-tmpfiles etc. Say we want to keep separate xml files for them:

$ cat /etc/xdg/rpmlint/cron_whitelist.xml

    [FileDigestLocation.cron]
    FollowSymlinks = true
    Locations = [
        "/etc/cron.d/",
        "/etc/cron.hourly/",
        "/etc/cron.daily/",
        "/etc/cron.weekly/",
        "/etc/cron.monthly/"
    ]

    [FileDigestGroup."sarg"]
    note = """
    Builds statistics based on Squid logfile metadata.
    Include sarg-reports which is SUSE specific and important for privilege dropping.
    """
      [FileDigestGroup."sarg".audits."bsc#4712"]
      digests = [
        {
          path = "/etc/cron.daily/suse.de-sarg",
          algorithm = "sha256",
          hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
        },
        {
          path = "/etc/cron.monthly/suse.de-sarg",
          algorithm = "sha256",
          hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
        },
        {
          path = "/etc/cron.weekly/suse.de-sarg",
          algorithm = "sha256",
          hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
        },
        {
          path = "/usr/sbin/sarg-reports",
          algorithm = "sha256",
          hash = "00ad25400adc2031cd09f9b8f9e56c448c93b6b89a702d36dce6a385d79e637c"
        },
      ]

$cat /etc/xdg/rpmlint/tmpfiles_whitelist.xml

    [FileDigestLocation."systemd-tmpfiles"]
    FollowSymlinks = true
    Locations = [
        "/usr/lib/tmpfiles.d/"
    ]

    [FileDigestGroup."sarg"]
    note = """some sarg tmpfiles.d entry"""

      [FileDigestGroup."sarg".audits."bsc#1234"]
      digests  = [
        {
          path = "/usr/lib/tmpfiles.d/sarg.conf",
          algorithm = "sha256",
          hash = "<some-hash>"
        }
      ]

These two entries should be treated seperately. So we need to encode the type
of whitelisting somehow in the FileDigestGroup, something like [FileDigestGroup.cron."sarg"] and [FileDigestGroup."systemd-tmpfiles"."sarg"].

[marxin]

Okay the configuration syntax looks good now.

The check still behaves erroneous, though. _calculate_errors_for_digest_group() does not consider self.digest_configurations, i.e. it applies digest groups to all files, not only the files that start with the expected prefix path.

Ok, the function _calculate_errors_for_digest_group() reports 3 types of errors now:

• # report errors for secured files not covered by the digest group
• # report errors for missing files mentioned in the digest group
• # report errors for invalid digests

Which one is not good, or please show me an example.

In the -file-digest-mismatch error case I'd like to see the expected and encountered digests.

Sure, will fix that.

I also get a KeyError in line 111, because the check tries to access a file in the package that isn't even existing. Is there a possibility to show the full exception backtrace when running rpmlint and a check fails internally? Because this error here was not very helpful:

$ rpmlint /var/cache/zypp/packages/download.opensuse.org-oss_2/x86_64/ruby2.7-rubygem-arel-doc-9.0.0-2.10.x86_64.rpm
(none): E: fatal error while reading /var/cache/zypp/packages/download.opensuse.org-oss_2/x86_64/ruby2.7-rubygem-arel-doc-9.0.0-2.10.x86_64.rpm: '/usr/sbin/sarg-reports'

Sure, just apply the following patch:

diff --git a/rpmlint/lint.py b/rpmlint/lint.py
index 1d75625..419a629 100644
--- a/rpmlint/lint.py
+++ b/rpmlint/lint.py
@@ -255,6 +255,7 @@ class Lint(object):
                     self.run_checks(pkg)
         except Exception as e:
             print_warning(f'(none): E: fatal error while reading {pname}: {e}')
+            raise e
             sys.exit(3)
 
     def run_checks(self, pkg):

For contextual files like /usr/sbin/sarg-reports the reported "group" is None, because the prefix of this path is not in a cron directory. It could make sense to assign each FileDigestGroup a type. Now that I think about it, something like this will most certainly become necessary, because we need to be able to keep parallel whitelists for cron, systemd-tmpfiles etc. Say we want to keep separate xml files for them:

Oh yes, that's really a limitation.

$ cat /etc/xdg/rpmlint/cron_whitelist.xml

    [FileDigestLocation.cron]
    FollowSymlinks = true
    Locations = [
        "/etc/cron.d/",
        "/etc/cron.hourly/",
        "/etc/cron.daily/",
        "/etc/cron.weekly/",
        "/etc/cron.monthly/"
    ]

    [FileDigestGroup."sarg"]
    note = """
    Builds statistics based on Squid logfile metadata.
    Include sarg-reports which is SUSE specific and important for privilege dropping.
    """
      [FileDigestGroup."sarg".audits."bsc#4712"]
      digests = [
        {
          path = "/etc/cron.daily/suse.de-sarg",
          algorithm = "sha256",
          hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
        },
        {
          path = "/etc/cron.monthly/suse.de-sarg",
          algorithm = "sha256",
          hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
        },
        {
          path = "/etc/cron.weekly/suse.de-sarg",
          algorithm = "sha256",
          hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
        },
        {
          path = "/usr/sbin/sarg-reports",
          algorithm = "sha256",
          hash = "00ad25400adc2031cd09f9b8f9e56c448c93b6b89a702d36dce6a385d79e637c"
        },
      ]

$cat /etc/xdg/rpmlint/tmpfiles_whitelist.xml

    [FileDigestLocation."systemd-tmpfiles"]
    FollowSymlinks = true
    Locations = [
        "/usr/lib/tmpfiles.d/"
    ]

    [FileDigestGroup."sarg"]
    note = """some sarg tmpfiles.d entry"""

      [FileDigestGroup."sarg".audits."bsc#1234"]
      digests  = [
        {
          path = "/usr/lib/tmpfiles.d/sarg.conf",
          algorithm = "sha256",
          hash = "<some-hash>"
        }
      ]

These two entries should be treated seperately. So we need to encode the type
of whitelisting somehow in the FileDigestGroup, something like [FileDigestGroup.cron."sarg"] and [FileDigestGroup."systemd-tmpfiles"."sarg"].

What about:

[FileDigestGroup."squid"]
type = "cron"
note = """
Builds statistics based on Squid logfile metadata.
Include sarg-reports which is SUSE specific and important for privilege dropping.
"""

where when checking the FileDigestGroup."squid" we can report all errors with the prefix taken from type attribute.
That can be applied to all files mentioned in the group, including e.g. /usr/sbin/sarg-reports. Is it a viable approach?
It can simplify the check quite well.

[mgerstner]

The check still behaves erroneous, though. _calculate_errors_for_digest_group() does not consider self.digest_configurations, i.e. it applies digest groups to all files, not only the files that start with the expected prefix path.

Ok, the function _calculate_errors_for_digest_group() reports 3 types of errors now:

• # report errors for secured files not covered by the digest group
• # report errors for missing files mentioned in the digest group
• # report errors for invalid digests

Which one is not good, or please show me an example.

Maybe I didn't express the problem good enough. The problem is that even if a package doesn't
contain the files listed in the whitelisting, an error will be displayed. Consider this example:

# zypper download ruby2.7-rubygem-arel-doc
# rpmlint /var/cache/zypp/packages/^Cwnload.opensuse.org-oss_2/x86_64/ruby2.7-rubygem-arel-doc-9.0.0-2.10.x86_64.rpm
[...]
ruby2.7-rubygem-arel-doc.x86_64: E: unknown-file-digest-unauthorized /etc/cron.daily/suse.de-sar
ruby2.7-rubygem-arel-doc.x86_64: E: unknown-file-digest-unauthorized /etc/cron.monthly/suse.de-sarg
ruby2.7-rubygem-arel-doc.x86_64: E: unknown-file-digest-unauthorized /etc/cron.weekly/suse.de-sarg
ruby2.7-rubygem-arel-doc.x86_64: E: unknown-file-digest-unauthorized /usr/sbin/sarg-reports
[...]

What about:

[FileDigestGroup."squid"]
type = "cron"
note = """
Builds statistics based on Squid logfile metadata.
Include sarg-reports which is SUSE specific and important for privilege dropping.
"""

where when checking the FileDigestGroup."squid" we can report all errors with the prefix taken from type attribute.
That can be applied to all files mentioned in the group, including e.g. /usr/sbin/sarg-reports. Is it a viable approach?
It can simplify the check quite well.

Yes it sounds good.

[marxin]

Maybe I didn't express the problem good enough. The problem is that even if a package doesn't
contain the files listed in the whitelisting, an error will be displayed. Consider this example:

You are right, that were bogus errors. I've just pushed a version with simplified logic that respects the newly added type property.

[mgerstner]

Okay we're getting close, however there is still a problem with the type property. When I want to add two different types of restrictions to the same package:

[FileDigestGroup."sarg"]                                                          
type = "cron"                                                                     
note = """                                                                        
Builds statistics based on Squid logfile metadata.                                
Include sarg-reports which is SUSE specific and important for privilege dropping.
"""                                                                               
  [FileDigestGroup."sarg".audits."bsc#4711"]                                      
  digests = [                                                                     
    {                                                                             
      path = "/etc/cron.daily/suse.de-sarg",                                      
      algorithm = "sha256",                                                       
      hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
    }                                                                             
  ]

[FileDigestGroup."sarg"]                                                          
type = "apache"                                                                   
note = """Indians are around"""                                                
  [FileDigestGroup."sarg".audits."bsc#815"]                                    
  digests = [                                                                  
    {                                                                          
      path = "/etc/apache3/something",                                         
      algorithm = "sha256",                                                    
      hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
    }                                                                          
  ]

Then I receive an error:

rpmlint /var/cache/zypp/packages/download.opensuse.org-oss_2/x86_64/sarg-2.4.0-3.3.x86_64.rpm
(none): W: error parsing configuration files: What? sarg already exists?

So we would need to change the data structure to allow for this.

[marxin]

Then I receive an error:

rpmlint /var/cache/zypp/packages/download.opensuse.org-oss_2/x86_64/sarg-2.4.0-3.3.x86_64.rpm
(none): W: error parsing configuration files: What? sarg already exists?

So we would need to change the data structure to allow for this.

That's really problem. It will need something like:

rpmlint/test/configs/test.config

Line 60 in f6f7671

[FileDigestGroup."squid"."cron".audits."bsc#4712"]

However, I'm considering flattening of the [[FileDigestGroup]]. What do you think about it?
It seems easier to edit and read the rules.

[mgerstner]

Then I receive an error:

rpmlint /var/cache/zypp/packages/download.opensuse.org-oss_2/x86_64/sarg-2.4.0-3.3.x86_64.rpm
(none): W: error parsing configuration files: What? sarg already exists?

So we would need to change the data structure to allow for this.

That's really problem. It will need something like:

rpmlint/test/configs/test.config

Line 60 in f6f7671

[FileDigestGroup."squid"."cron".audits."bsc#4712"]

However, I'm considering flattening of the [[FileDigestGroup]]. What do you think about it?
It seems easier to edit and read the rules.

Hmm, I don't see big advantages or disadvantages. The hierarchical approach before made it clearer that the different digest groups are related IMO but I can also live with this new approach.

It looks like there is a regression now, though: The package field is not evaluated, so the digests are considered for all packages. Can you maybe add this to your tests that the config only is evaluated for matching package names?

Also I noticed the check doesn't work when there are not [[FileDigestGroup]] entries at all, it fails with a KeyError.

[marxin]

Hmm, I don't see big advantages or disadvantages. The hierarchical approach before made it clearer that the different digest groups are related IMO but I can also live with this new approach.

It will help as the implementation can be simplified a bit.

It looks like there is a regression now, though: The package field is not evaluated, so the digests are considered for all packages. Can you maybe add this to your tests that the config only is evaluated for matching package names?

Fixed that and it's now newly tests.

Also I noticed the check doesn't work when there are not [[FileDigestGroup]] entries at all, it fails with a KeyError.

I added [FileDigestGroup] to configdefaults.toml which provides empty default. Note that also other config values must have a default value.

[mgerstner]

The package is now correctly take in to account.

I'm sorry to say it still doesn't work as expected though. Having two separate check types doesn't work out, the problem must be somewhere in line 131 secured_paths = {...}, where paths from different check types are mixed together. I guess each check type must be considered separately. Example:

[FileDigestLocation.cron]                                                         
FollowSymlinks = true                                                             
Locations = [                                                                     
    "/etc/cron.d/",                                                               
    "/etc/cron.hourly/",                                                          
    "/etc/cron.daily/",                                                           
    "/etc/cron.weekly/",                                                          
    "/etc/cron.monthly/"                                                          
]                                                                                 
                                                                                  
[FileDigestLocation.apache]                                                       
FollowSymlinks = true                                                             
Locations = [                                                                     
    "/etc/apache2/conf.d",                                                        
] 

[[FileDigestGroup]]                                                            
package = "sarg"                                                               
type = "cron"                                                                  
note = """                                                                     
Builds statistics based on Squid logfile metadata.                             
Include sarg-reports which is SUSE specific and important for privilege dropping.
"""                                                                            
bug = "bsc#4712"                                                               
digests = [                                                                    
{                                                                              
        path = "/etc/cron.daily/suse.de-sarg",                                 
             algorithm = "sha256",                                             
             hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
},                                                                             
{                                                                              
        path = "/etc/cron.monthly/suse.de-sarg",                               
        algorithm = "sha256",                                                  
        hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
},                                                                             
{                                                                              
        path = "/etc/cron.weekly/suse.de-sarg",                                
        algorithm = "sha256",                                                  
        hash = "d536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359"
},                                                                             
{                                                                              
        path = "/usr/sbin/sarg-reports",                                       
        algorithm = "sha256",                                                  
        hash = "00ad25400bdc2031cd09f9b8f9e56c448c93b6b89a702d36dce6a385d79e637c"
}                                                                             
]

[[FileDigestGroup]]                                                            
package = "sarg"                                                               
type = "apache"                                                                
bug = "bsc#4713"                                                               
digests = [                                                                    
{                                                                              
     path = "/etc/apache2/conf.d/sarg-apache.conf",                            
     algorithm = "skip"
}                                                                              
]

This results in the following error:

root# rpmlint /var/cache/zypp/packages/download.opensuse.org-oss_2/x86_64/sarg-2.4.0-3.3.x86_64.rpm
sarg.x86_64: E: cron-file-digest-unauthorized /etc/apache2/conf.d/sarg-apache.conf

So the apache config file is treated as being part of the cron type check and the apache type whitelisting isn't used.

[marxin]

So the apache config file is treated as being part of the cron type check and the apache type whitelisting isn't used.

I'm sorry that it still does not work as expected :/

It's being caused by fact that we require that all package files (in a secured file digest location) must be covered by exactly one [[FileDigestGroup]]. How can I find out that multiple groups (with equal package and a different type) successfully cover files from a package?

[mgerstner]

So the apache config file is treated as being part of the cron type check and the apache type whitelisting isn't used.

I'm sorry that it still does not work as expected :/

It's being caused by fact that we require that all package files (in a secured file digest location) must be covered by exactly one [[FileDigestGroup]]. How can I find out that multiple groups (with equal package and a different type) successfully cover files from a package?

Groups of different type should not have any influence on each other. For each restriction type exactly one group of the same type should fully match.

[marxin]

Groups of different type should not have any influence on each other. For each restriction type exactly one group of the same type should fully match.

I see that makes sense. I fixed that and tested it on the package. We'll definitely need a better test coverage I guess.

[mgerstner]

Groups of different type should not have any influence on each other. For each restriction type exactly one group of the same type should fully match.

I see that makes sense. I fixed that and tested it on the package. We'll definitely need a better test coverage I guess.

Yes a more complex test case that checks all the corner cases would surely be helpful.

Okay the current state seems to finally work for the FileDigestCheck. Now there remains the question about the "error details". In rpmlint-1 we have Filter.addDetails() that we used to provide detailed instructions to packagers what to do about whitelisting issues. I implemented placeholders to avoid repeating the same paragraphs over and over again, see here. We provide a wiki URL to packagers here so they can always refer to up to date information how the review process works etc. It looks like rpmlint-1 only prints those details when invoked with the -I / --explain switch, which is obivously what happens when rpmlint runs in the OBS context.

How can we achieve this in rpmlint-2?

[marxin]

How can we achieve this in rpmlint-2?

This can be easily achieved by adding a description to rpmlint/descriptionsFileDigestCheck.toml. rpmlint-2 will be run in -v mode, so the explanation will be printed.

Similarly one can see:

$ ./lint.py  /home/marxin/BIG/rpm/sarg/binaries/sarg-2.4.0-3.3.x86_64.rpm -v
...

sarg.x86_64: W: manpage-not-compressed bz2 /usr/share/man/man1/sarg.1.gz
sarg.x86_64: W: manpage-not-compressed bz2 /usr/share/man/man8/sarg-reports.1.gz
This manual page is not compressed with the bz2 compression method
(does not have the bz2 extension). If the compression does not happen
automatically when the package is rebuilt, make sure that you have the
appropriate rpm helper and/or config packages for your target distribution
installed and try rebuilding again; if it still does not happen automatically,
you can compress this file in the %install section of the spec file.

sarg.x86_64: W: invalid-license GPL-2.0-or-later
The value of the License tag was not recognized.  Known values are:
''.

sarg.x86_64: E: incorrect-fsf-address /srv/www/htdocs/sarg-php/COPYING
sarg.x86_64: E: incorrect-fsf-address /usr/sbin/sarg-reports
sarg.x86_64: E: incorrect-fsf-address /usr/share/licenses/sarg/COPYING
sarg.x86_64: E: incorrect-fsf-address /usr/share/licenses/sarg/LICENSE
The Free Software Foundation address in this file seems to be outdated or
misspelled. Ask upstream to update the address, or if this is a license file,
possibly the entire file with a new copy available from the FSF.

[mgerstner]

How can we achieve this in rpmlint-2?

This can be easily achieved by adding a description to rpmlint/descriptionsFileDigestCheck.toml. rpmlint-2 will be run in -v mode, so the explanation will be printed.

Okay so the mechanism is clear. However we would like to add the openSUSE wiki URL to the explanation and we also would like to avoid redundancy. Currently we do this by using some generic text snippets that are formatted during runtime.

Since this is now a TOML file where the descriptions reside, doing it during runtime is not so easy. We could generate the description file from the repository via a script or something but that would complicate the installation somewhat. Any ideas?

[marxin]

Okay so the mechanism is clear. However we would like to add the openSUSE wiki URL to the explanation and we also would like to avoid redundancy. Currently we do this by using some generic text snippets that are formatted during runtime.

Since this is now a TOML file where the descriptions reside, doing it during runtime is not so easy. We could generate the description file from the repository via a script or something but that would complicate the installation somewhat. Any ideas?

I think it's a good idea to have snippets (variables). Should be addressed with 2c78ab1 that can be upstreamed later.

Hope it will work for your use case?

[mgerstner]

Okay snippets in error descriptions look good this should allow us to do what we want.

Testing this I found yet another regression, however: When there are no digest groups present for a package then no error is raised at all (return in FileDigestCheck.py:412).

[marxin]

Okay snippets in error descriptions look good this should allow us to do what we want.

Good. Will you please prepare a commit that will add the descriptions?

Testing this I found yet another regression, however: When there are no digest groups present for a package then no error is raised at all (return in FileDigestCheck.py:412).

Fixed that.

I'm suggesting to move the unreviewed FileMetadataCheck check into a different pull request. What do you think?
If you are fine, I can squash the changes and make a real pull request for FileDigestCheck?

[mgerstner]

Okay snippets in error descriptions look good this should allow us to do what we want.

Good. Will you please prepare a commit that will add the descriptions?

For this to be complete I would need to add configuration for all whitelisting restrictions we currently have in place, also add the actual whitelists as we already have them in rpmlint1 and then the error descriptions for each type of error we have. Would that be okay?

Testing this I found yet another regression, however: When there are no digest groups present for a package then no error is raised at all (return in FileDigestCheck.py:412).

Fixed that.

We really need some actual test coverage before we can bring that into production. We should implement various test cases:

• an RPM that does not contain files from any of the restricted locations should not trigger any errors
• an RPM that contains files in a restricted location but has no whitelisting entry should trigger an appropriate error
• an RPM that contains files in a restricted location but has one matching (among a choice of multiple groups of) whitelisting entry should not trigger an error.
• an RPM that contains files in a restricted location and a whitelisting entry contains matches but only for 2 of 3 files should trigger an error, and display the offending / missing file.
• an RPM that contains files in two different types of restricted locations and has matching whitelisting entries for both locations should not trigger an error.
• the symlink handling (follow, nofollow) should be covered by a testcase
• an RPM of a different (package) name should not be covered by a whitelisting entry, even if the digests match.

Probably I missed a couple of situations there.

I'm suggesting to move the unreviewed FileMetadataCheck check into a different pull request. What do you think?
If you are fine, I can squash the changes and make a real pull request for FileDigestCheck?

Yes that is a good idea, because this took quite a lot of time and we should "save" the progress we have made. Before we do that we should cover the points above, however, and I also want to involve Johannes to test the final variant of the checker, because I'm starting to get blind myself about this and we need a final fresh view on things before we move it into production.

[marxin]

For this to be complete I would need to add configuration for all whitelisting restrictions we currently have in place, also add the actual whitelists as we already have them in rpmlint1 and then the error descriptions for each type of error we have. Would that be okay?

Yes. We can likely make it a separate pull request to opensuse branch once this is merged.
Note we still have some time before rpmlint2 will be used in openSUSE:Factory.

[marxin]

Probably I missed a couple of situations there.

Sure, please submit such packages to https://build.opensuse.org/project/show/devel:openSUSE:Factory:rpmlint:tests and then add the created rpm files to tests. Leaving that to your team.

[marxin]

Yes that is a good idea, because this took quite a lot of time and we should "save" the progress we have made.

Good, I've just done that in #600.

[mgerstner]

For this to be complete I would need to add configuration for all whitelisting restrictions we currently have in place, also add the actual whitelists as we already have them in rpmlint1 and then the error descriptions for each type of error we have. Would that be okay?

Yes. We can likely make it a separate pull request to opensuse branch once this is merged.
Note we still have some time before rpmlint2 will be used in openSUSE:Factory.

Maybe we should at the moment just add some example configuration (commented out) so that users can quickly understand how the configuration os the checker is supposed to work. This can be part of the standard rpmlint, while the actual production configuration we need are then added to the opensuse branch.

[mgerstner]

Probably I missed a couple of situations there.

Sure, please submit such packages to https://build.opensuse.org/project/show/devel:openSUSE:Factory:rpmlint:tests and then add the created rpm files to tests. Leaving that to your team.

Can you explain to us how the connection between these test packages and the tests within the test sub-directory of rpmlint works? So should we only add the packages into OBS or also write the actual test?

[marxin]

Can you explain to us how the connection between these test packages and the tests within the test sub-directory of rpmlint works? So should we only add the packages into OBS or also write the actual test?

In order to create a rpm test-case, one needs something like OBS. So that's why we track .spec files in the devel:openSUSE:Factory:rpmlint:tests . And the built rpm packages are taken and copied into test folder in rpmlint.
And yes, you are supposed to write Python tests in test/test_xyz.py.

[marxin]

Maybe we should at the moment just add some example configuration (commented out) so that users can quickly understand how the configuration os the checker is supposed to work. This can be part of the standard rpmlint, while the actual production configuration we need are then added to the opensuse branch.

To be honest, based on the experience with #439, I don't think upstream is going to accept the pass.
It's quite openSUSE-specific and thus I recommend putting this entire pull request into opensuse branch.

[mgerstner]

Okay I have informed @jsegitz about the current state of the check and asked him to look at it from his point of view if usability, feature set and robustness are all right. I am waiting for this statement before I continue.

Which process should be follow to complete the integration? I can create commits to add the error descriptions and port the actual production whitelisting entries from rpmlint1. Should I do this as a separate PR# once this PR# is merged?

[marxin]

Which process should be follow to complete the integration?

Currently, we do face problems with extensive dependencies of the patch:
https://jira.suse.com/browse/OBS-88

I hope @kstreitova can make a progress with taht.

I can create commits to add the error descriptions and port the actual production whitelisting entries from rpmlint1. Should I do this as a separate PR# once this PR# is merged?

I guess so.

May I please ask @kstreitova for a review of this pull request?

[mgerstner]

@jsegitz also pointed out that rpmlint is behaving badly when there are syntax errors in TOML configuration files like /etc/xdg/rpmlint/security.toml. Errors will look like:

(none): W: error parsing configuration files: Invalid inline table encountered (line 46 column 1 char 886)

So its not clear in which file exactly the error occured. And even worse, the rpmlint processing continues, bit without the security related checks. In such a case we need rpmlint to abort with an error such that package with potential dangerous content don't build successfully in protected projects on OBS.

Another things we noted is that at least in the packaging that you have in home:marxin:rpmlint-ring0/rpmlint, the configuration file in /etc/xdg/rpmlint all carry an executable bit.

[marxin]

@jsegitz also pointed out that rpmlint is behaving badly when there are syntax errors in TOML configuration files like /etc/xdg/rpmlint/security.toml. Errors will look like:

Very good point, I've just made #603 for it:

(none): W: error parsing configuration files: Invalid inline table encountered (line 46 column 1 char 886)

This is replaced with:
(none): E: fatal error while parsing configuration file configs/openSUSE/opensuse.toml: Unbalanced quotes (line 122 column 29 char 4640)

So its not clear in which file exactly the error occured. And even worse, the rpmlint processing continues, bit without the security related checks. In such a case we need rpmlint to abort with an error such that package with potential dangerous content don't build successfully in protected projects on OBS.

... and rpmlint prints a fatal error and ends.

Another things we noted is that at least in the packaging that you have in home:marxin:rpmlint-ring0/rpmlint, the configuration file in /etc/xdg/rpmlint all carry an executable bit.

Fixed here: home:scarabeus_iv:rpmlint-testbed/rpmlint.

[marxin]

We really need some actual test coverage before we can bring that into production. We should implement various test cases:

• an RPM that does not contain files from any of the restricted locations should not trigger any errors
• an RPM that contains files in a restricted location but has no whitelisting entry should trigger an appropriate error
• an RPM that contains files in a restricted location but has one matching (among a choice of multiple groups of) whitelisting entry should not trigger an error.
• an RPM that contains files in a restricted location and a whitelisting entry contains matches but only for 2 of 3 files should trigger an error, and display the offending / missing file.
• an RPM that contains files in two different types of restricted locations and has matching whitelisting entries for both locations should not trigger an error.
• the symlink handling (follow, nofollow) should be covered by a testcase
• an RPM of a different (package) name should not be covered by a whitelisting entry, even if the digests match.

Btw. I've just extended FakePkg syntax and one can cover all these situations with tests that don't need an RPM file. Please see:

rpmlint/test/test_file_digest.py

Lines 49 to 59 in a7f460d

	def test_from_simple_dummy_pkg(digestcheck):
	output, test = digestcheck
	with FakePkg('dummy') as pkg:
	pkg.add_file_with_content('/etc/polkit-1/rules.d/r.txt', 'Hello world')
	pkg.add_file_with_content('/root/sample.txt', 'Hello world')
	pkg.add_symlink_to('/etc/polkit-1/rules.d/r2.txt', '../../root/sample.txt')
	test.check(pkg)
	out = output.print_results(output.results)
	assert len(output.results) == 2
	assert 'dummy: E: polkit-file-digest-unauthorized /etc/polkit-1/rules.d/r.txt' in out
	assert 'dummy: E: polkit-file-digest-unauthorized /etc/polkit-1/rules.d/r2.txt' in out