commit: 4e69c9d901c37d631cfff6316103e4be53b214c3 Author: Arthur Zamarin <arthurzam <AT> gentoo <DOT> org> AuthorDate: Sun Oct 30 18:36:27 2022 +0000 Commit: Arthur Zamarin <arthurzam <AT> gentoo <DOT> org> CommitDate: Mon Oct 31 18:29:55 2022 +0000 URL: https://gitweb.gentoo.org/proj/pkgcore/pkgcheck.git/commit/?id=4e69c9d9
AcctCheck: use metadata/qa-policy.conf for dynamic id range Load UID and GID range from metadata/qa-policy.conf under the repo, validate the format is as expected, and use the values to set expected ids. Resolves: https://github.com/pkgcore/pkgcheck/issues/356 Signed-off-by: Arthur Zamarin <arthurzam <AT> gentoo.org> src/pkgcheck/checks/acct.py | 60 +++++++++++++++++++++++++++++---------- tests/checks/test_acct.py | 69 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 107 insertions(+), 22 deletions(-) diff --git a/src/pkgcheck/checks/acct.py b/src/pkgcheck/checks/acct.py index cb0053e3..4f144023 100644 --- a/src/pkgcheck/checks/acct.py +++ b/src/pkgcheck/checks/acct.py @@ -2,14 +2,16 @@ import re from collections import defaultdict +from configparser import ConfigParser from functools import partial from itertools import chain from pkgcore.ebuild import restricts from pkgcore.restrictions import packages +from snakeoil.osutils import pjoin from .. import results, sources -from . import GentooRepoCheck, RepoCheck +from . import GentooRepoCheck, RepoCheck, SkipCheck class MissingAccountIdentifier(results.VersionResult, results.Warning): @@ -35,13 +37,16 @@ class ConflictingAccountIdentifiers(results.Error): @property def desc(self): - return ( - f"conflicting {self.kind} id {self.identifier} usage: " - f"[ {', '.join(self.pkgs)} ]") + pkgs = ', '.join(self.pkgs) + return f"conflicting {self.kind} id {self.identifier} usage: [ {pkgs} ]" class OutsideRangeAccountIdentifier(results.VersionResult, results.Error): - """UID/GID outside allowed allocation range.""" + """UID/GID outside allowed allocation range. + + To view the range accepted for this repository, look at the file + ``metadata/qa-policy.conf`` in the section ``user-group-ids``. + """ def __init__(self, kind, identifier, **kwargs): super().__init__(**kwargs) @@ -50,16 +55,20 @@ class OutsideRangeAccountIdentifier(results.VersionResult, results.Error): @property def desc(self): - return ( - f"{self.kind} id {self.identifier} outside permitted " - f"static allocation range (0..499, 60001+)") + return (f"{self.kind} id {self.identifier} outside permitted " + f"static allocation range") class AcctCheck(GentooRepoCheck, RepoCheck): """Various checks for acct-* packages. Verify that acct-* packages do not use conflicting, invalid or out-of-range - UIDs/GIDs. + UIDs/GIDs. This check uses a special file ``metadata/qa-policy.conf`` + located within the repository. It should contain a ``user-group-ids`` + section containing two keys: ``uid-range`` and ``gid-range``, which consist + of a comma separated list, either ``<n>`` for a single value or ``<m>-<n>`` + for a range of values (including both ends). In case this file doesn't + exist or is wrongly defined, this check is skipped. """ _restricted_source = (sources.RestrictionRepoSource, (packages.OrRestriction(*( @@ -76,14 +85,37 @@ class AcctCheck(GentooRepoCheck, RepoCheck): r'ACCT_(?P<var>USER|GROUP)_ID=(?P<quot>[\'"]?)(?P<id>[0-9]+)(?P=quot)') self.seen_uids = defaultdict(partial(defaultdict, list)) self.seen_gids = defaultdict(partial(defaultdict, list)) + uid_range, gid_range = self.load_ids_from_configuration(self.options.target_repo) self.category_map = { - 'acct-user': (self.seen_uids, 'USER', (65534,)), - 'acct-group': (self.seen_gids, 'GROUP', (65533, 65534)), + 'acct-user': (self.seen_uids, 'USER', tuple(uid_range)), + 'acct-group': (self.seen_gids, 'GROUP', tuple(gid_range)), } + def parse_config_id_range(self, config: ConfigParser, config_key: str): + id_ranges = config['user-group-ids'].get(config_key, None) + if not id_ranges: + raise SkipCheck(self, f"metadata/qa-policy.conf: missing value for {config_key}") + try: + for id_range in map(str.strip, id_ranges.split(',')): + start, *end = map(int, id_range.split('-', maxsplit=1)) + if len(end) == 0: + yield range(start, start + 1) + else: + yield range(start, end[0] + 1) + except ValueError: + raise SkipCheck(self, f"metadata/qa-policy.conf: invalid value for {config_key}") + + def load_ids_from_configuration(self, repo): + config = ConfigParser() + if not config.read(pjoin(repo.location, 'metadata', 'qa-policy.conf')): + raise SkipCheck(self, "failed loading 'metadata/qa-policy.conf'") + if 'user-group-ids' not in config: + raise SkipCheck(self, "metadata/qa-policy.conf: missing section user-group-ids") + return self.parse_config_id_range(config, 'uid-range'), self.parse_config_id_range(config, 'gid-range') + def feed(self, pkg): try: - seen_id_map, expected_var, extra_allowed_ids = self.category_map[pkg.category] + seen_id_map, expected_var, allowed_ids = self.category_map[pkg.category] except KeyError: return @@ -96,9 +128,7 @@ class AcctCheck(GentooRepoCheck, RepoCheck): yield MissingAccountIdentifier(f"ACCT_{expected_var}_ID", pkg=pkg) return - # all UIDs/GIDs must be in <750, with special exception - # of nobody/nogroup which use 65534/65533 - if found_id >= 750 and found_id not in extra_allowed_ids: + if not any(found_id in id_range for id_range in allowed_ids): yield OutsideRangeAccountIdentifier(expected_var.lower(), found_id, pkg=pkg) return diff --git a/tests/checks/test_acct.py b/tests/checks/test_acct.py index cd8f852f..57273705 100644 --- a/tests/checks/test_acct.py +++ b/tests/checks/test_acct.py @@ -1,4 +1,7 @@ -from pkgcheck.checks import acct +import textwrap + +import pytest +from pkgcheck.checks import acct, SkipCheck from pkgcore.test.misc import FakeRepo from snakeoil.cli import arghparse @@ -11,17 +14,27 @@ class TestAcctUser(misc.ReportTestCase): kind = 'user' + @pytest.fixture(autouse=True) + def _setup(self, tmp_path): + (metadata := tmp_path / 'metadata').mkdir() + (metadata / 'qa-policy.conf').write_text(textwrap.dedent("""\ + [user-group-ids] + uid-range = 0-749,65534 + gid-range = 0-749,65533,65534 + """)) + self.location = str(tmp_path) + def mk_check(self, pkgs): - self.repo = FakeRepo(pkgs=pkgs, repo_id='test') - check = self.check_kls(arghparse.Namespace(target_repo=self.repo, gentoo_repo=True)) + repo = FakeRepo(pkgs=pkgs, repo_id='test', location=self.location) + check = self.check_kls(arghparse.Namespace(target_repo=repo, gentoo_repo=True)) return check def mk_pkg(self, name, identifier, version=1, ebuild=None): if ebuild is None: - ebuild = f''' -inherit acct-{self.kind} -ACCT_{self.kind.upper()}_ID="{identifier}" -''' + ebuild = textwrap.dedent(f'''\ + inherit acct-{self.kind} + ACCT_{self.kind.upper()}_ID="{identifier}" + ''') return misc.FakePkg(f'acct-{self.kind}/{name}-{version}', ebuild=ebuild) def test_unmatching_pkgs(self): @@ -33,6 +46,7 @@ ACCT_{self.kind.upper()}_ID="{identifier}" def test_correct_ids(self): pkgs = (self.mk_pkg('foo', 100), self.mk_pkg('bar', 200), + self.mk_pkg('test', 749), self.mk_pkg('nobody', 65534)) check = self.mk_check(pkgs) self.assertNoReport(check, pkgs) @@ -110,3 +124,44 @@ class TestAcctGroup(TestAcctUser): pkg = self.mk_pkg('nogroup', 65533) check = self.mk_check((pkg,)) self.assertNoReport(check, pkg) + + +class TestQaPolicyValidation(misc.ReportTestCase): + + def mk_check(self, tmp_path, content): + if content: + (metadata := tmp_path / 'metadata').mkdir() + (metadata / 'qa-policy.conf').write_text(textwrap.dedent(content)) + repo = FakeRepo(repo_id='test', location=str(tmp_path)) + return acct.AcctCheck(arghparse.Namespace(target_repo=repo, gentoo_repo=True)) + + def test_missing_qa_policy(self, tmp_path): + with pytest.raises(SkipCheck, match="failed loading 'metadata/qa-policy.conf'"): + self.mk_check(tmp_path, None) + + def test_missing_section(self, tmp_path): + with pytest.raises(SkipCheck, match="missing section user-group-ids"): + self.mk_check(tmp_path, '''\ + [random] + x = 5 + ''') + + def test_missing_config(self, tmp_path): + with pytest.raises(SkipCheck, match="missing value for gid-range"): + self.mk_check(tmp_path, '''\ + [user-group-ids] + uid-range = 0-749 + ''') + + @pytest.mark.parametrize('value', ( + 'start-end', + '0-749-1500', + ',150', + )) + def test_invalid_value(self, tmp_path, value): + with pytest.raises(SkipCheck, match="invalid value for uid-range"): + self.mk_check(tmp_path, f'''\ + [user-group-ids] + uid-range = {value} + gid-range = 0-749 + ''')
