add security allow list lints #439
Conversation
|
This pull request introduces 3 alerts when merging 94f99cf into ad7aedd - view on LGTM.com new alerts:
|
|
Seems there are bunch of flake8 and lgtm complaints. Generally okay. Only thing we will have to figure out how to override those descriptions pointing to suse, so it could be reused by the other distributions too. (alternatively we would have to keep it in opensuse branch but it seems okay for master). |
|
Yes, the original author's C++ background really showed through in that code. I've added a commit that should address your comments. |
So, how about: I adapt |
We have it already in progress I think @kstreitova #432 it just needs a bit of polishing. Also whats weird is that one test failing on filtering in travis right now, it should not be failing :) |
rpmlint/checks/Allowlisting.py
Outdated
| ############################################################################# | ||
| # Author : Matthias Gerstner | ||
| # Purpose : reusable code for dealing with security allow lists | ||
| ############################################################################# |
There was a problem hiding this comment.
The whole comments are moot, just convert them to module docstring if it makes sense.
rpmlint/checks/CheckDBUSServices.py
Outdated
| @@ -0,0 +1,15 @@ | |||
| from .Allowlisting import AbstractSimpleAllowlistCheck | |||
There was a problem hiding this comment.
The imports should be by default absolute unless the path grows crazy complex.
| 'texlive', | ||
| 'texlive.texlive', | ||
| 'otrs', # bsc#1118049 | ||
| ) |
There was a problem hiding this comment.
I bet this should be in a config file as a toml list, also any insight where this default came from?
| self.output.add_info('E', pkg, 'permissions-fscaps', f'{f} has fscaps "{pkgfile.filecaps}"') | ||
|
|
||
| mode = pkgfile.mode | ||
| owner = pkgfile.user + ':' + pkgfile.group |
There was a problem hiding this comment.
f'{pkgfile.user}:{pkgfile.group}' ?
There was a problem hiding this comment.
I would prefer to stay with pkgfile.user + ':' + pkgfile.group.
| if need_set_permissions: | ||
| if 'permissions' not in (x[0] for x in pkg.prereq): | ||
| self.output.add_info('E', pkg, 'permissions-missing-requires', | ||
| "missing 'permissions' in PreReq") |
There was a problem hiding this comment.
This sounds wrong, PreReq is obsolete var, now we use Requires(phase): to state the deps.
| if found_suseconfig: | ||
| # TODO: nothing in tumbleweed uses this anymore. can probable be dropped. | ||
| self.output.add_info('I', pkg, 'permissions-suseconfig-obsolete', | ||
| '%run_permissions is obsolete') |
There was a problem hiding this comment.
I would probably drop this one. Also it is auto-removed by spec-cleaner too.
|
|
||
|
|
||
| permissions-missing-requires=""" | ||
| Please add 'PreReq: permissions' |
There was a problem hiding this comment.
PreReq is obsolete variable.
There was a problem hiding this comment.
Should be Requires used here?
There was a problem hiding this comment.
No Requires(phase): one.
There was a problem hiding this comment.
I think it should be a plain Requires, because the verifyscript also has that dependency.
|
I'm going to do a more in-depth review of this a bit later, but I've got some simple feedback:
|
|
@maltek Can you please provide links to spec files from which you created the |
Should be these: |
but still allow it, to allow packagers to move to normal 'Requires' on their own pace
|
I've rebased the PR and updated it based on the comments and #469. (From #469:)
Well, kind of. There's a few of new checks there, but they are much less important than the ones here for openSUSE. Waiting another few months won't help with the fork situation. |
Thank you for the port! |
Done. |
| @@ -0,0 +1,40 @@ | |||
| import os | |||
| import os.path | |||
There was a problem hiding this comment.
maybe use of pathlib will be better option
There was a problem hiding this comment.
I don't personally see how it'd provide a benefit over a single call to os.path.join.
But I don't care much either way, so I'll use that if that's what you prefer.
|
@dcantrell Could you please take a look over this? |
|
Looking over this PR now. I have some initial thoughts, but will have more to post on Monday. Currently on a long weekend and only have my phone available. Thanks. |
|
Various comments:
Overall this PR feels very SuSE-specific and not general enough to apply to a wide range of RPM-based distributions. The code itself contains a lot of vendor specifics and hardcoded paths and things unique to SuSE releases. I am also not a fan of the data format described in the README. For https://github.com/rpminspect/rpminspect, I have fileinfo lists per product release defined in the vendor's rpminspect-data package. Here's what the rawhide one looks like: https://github.com/rpminspect/rpminspect-data-fedora/blob/master/fileinfo/rawhide Separate from that are the capabilities lists per product release: https://github.com/rpminspect/rpminspect-data-fedora/blob/master/capabilities/rawhide I have tried to keep these files minimal so they are easily maintained by all contributors. The files in question can move among packages in the distributions, but we can still enforce the same settings on the file that is ultimately put on the filesystem. |
Thanks for them, they are useful.
I agree. That format should be verified in the project where you have the configuration file.
It seems to me logical to have it grouped on a package basis and having an optional comment (or a bug link) seems fine to. [mypackage]
bug_url = 'https://bugzilla.opensuse.org/show_bug.cgi?id=1177680'
comment = 'Chroot duplicates of some uncritical character devices. urandom was historically packaged non-world-writable.'
[mypackage.digests."/tmp/a"]
algorithm = 'sha256'
hash = 'd536dc68e198189149048a907ea6d56a7ee9fc732ae8fec5a4072ad06640e359'
[mypackage.digests."/tmp/b"]
algorithm = 'sha256'
hash = 'asdfasdfa'
I like the idea of matching that against the
I guess a TOML configuration for that can be added.
I guess the paths that are searched can be also factored out into a TOML configuration?
This is a generic problem, so far we've added a bunch of RPM files that are built from: I can't find any documentation for the
|
This improves on it. I would suggest bug_url be a list because it is possible that you may want to reference both the distribution bug as well as one or more upstream bugs (or even a CVE). algorithm feels unnecessary as that can be determined by the digest hash, but only if you require the full digest hash. My preference is towards requiring unabbreviated digest hashes and doing away with 'algorithm' and just inferring that in the code.
Anything that gets those sorts of lists out of code and in to runtime-maintainable files.
They should be, yes.
rpmfluff began life inside the Red Hat rpmdiff project (internal tool) but is now a Python module project. https://pypi.org/project/rpmfluff/ While there is value in running tests against known RPMs files, being able to construct them directly from the test suite is also useful. Especially for external contributors. rpmfluff lets you create RPM files for testing purposes for tools like this. It does not take a spec file. rpmfluff is not perfect, but it is a timesaver for test suites. If you point me at an example spec file, I can show you how I would synthesize that with rpmfluff. |
This took a lot longer than I'd hoped. This PR adds the allow lists from openSUSE/rpmlint-checks developed in parallel to this rewrite.
I'm not sure if the links to the openSUSE wiki in the description texts are acceptable here. If not, we'll have to find some way to change these texts in openSUSE. We're doing something pretty rude here (telling a packager they can't do what they want to) - the least we have to do is tell the packager the exact steps they can take to get the allowed lists to be adapted.