diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index fee10978b5..bbccce5524 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -850,6 +850,26 @@ jobs: apt_install build-essential libpcap-dev fi + # #2019: from Ubuntu 25.04, apparmor 4.1 ships an enforcing profile for + # /usr/bin/tshark (/etc/apparmor.d/tshark) whose own file rules read only + # /tmp and /usr/share/wireshark, so on an ubuntu-26.04 image `tshark -r` + # is denied every capture under $GITHUB_WORKSPACE -- "You don't have + # permission to read the file", a TSharkCrashException with retcode 3. + # The profile's `include if exists ` is its site hook, so + # one read rule for the checkout goes there, in the profile's own + # `file r ,` form, and `apparmor_parser -r` recompiles the profile + # with that include and replaces the loaded one. 24.04 has no such + # profile, so there this is a no-op. `pypcap-parity` runs the same body. + - name: Let tshark read the checkout past its AppArmor profile + if: env.ENGINE == 'PyShark' + run: | + set -eu + if [ -f /etc/apparmor.d/tshark ]; then + sudo mkdir -p /etc/apparmor.d/local + echo "file r ${GITHUB_WORKSPACE}/**," | sudo tee -a /etc/apparmor.d/local/tshark >/dev/null + sudo apparmor_parser -r /etc/apparmor.d/tshark + fi + - name: Collect this engine's installed .deb files for the cache if: >- env.APT_CACHE_SAVE == 'true' && steps.apt-plan.outputs.key != '' @@ -1222,6 +1242,19 @@ jobs: done exit 1 + # #2019: tshark's AppArmor profile on Ubuntu 25.04 and later; see + # `engine-tests`'s step of the same name, whose body this is. + - name: Let tshark read the checkout past its AppArmor profile + id: apparmor + continue-on-error: ${{ matrix.ktlo == true }} + run: | + set -eu + if [ -f /etc/apparmor.d/tshark ]; then + sudo mkdir -p /etc/apparmor.d/local + echo "file r ${GITHUB_WORKSPACE}/**," | sudo tee -a /etc/apparmor.d/local/tshark >/dev/null + sudo apparmor_parser -r /etc/apparmor.d/tshark + fi + - name: Collect the installed .deb files for the cache if: >- env.APT_CACHE_SAVE == 'true' && steps.apt-plan.outputs.key != '' diff --git a/docs/source/contributing/releasing.rst b/docs/source/contributing/releasing.rst index fd5db0d3f6..32a8ef467c 100644 --- a/docs/source/contributing/releasing.rst +++ b/docs/source/contributing/releasing.rst @@ -52,7 +52,7 @@ Editing ``__version__`` by hand without also moving :file:`CITATION.cff`'s (``:494-514``) asserts the two agree, and ``create-release.yml``'s ``unit-tests`` job (``:581-597``) calls ``unit-tests.yml`` with ``gate-only: true``, which runs the **full** suite rather than the tiered -subset an ordinary push runs (its ``gate`` job, ``unit-tests.yml:1738-1813``). +subset an ordinary push runs (its ``gate`` job, ``unit-tests.yml:1771-1846``). That gate runs whenever a release will -- a moved ``__version__`` is exactly what makes ``version_check``'s evidence read "not yet published" -- and every publishing job requires it to have succeeded (``:621,808,974,1210``), so a @@ -62,7 +62,7 @@ Either run the script, or move both fields by hand in the same commit. This is the **only** edit a person makes to get a release started -- the tag, the Release, and every upload are the workflow's job from here. Other checks run unconditionally on the same path, though none is a step in *this* -process: the ``changelog`` job (``unit-tests.yml:1581-1607``) fails outright if +process: the ``changelog`` job (``unit-tests.yml:1614-1640``) fails outright if ``CHANGELOG.md`` has drifted from its source entry under :file:`docs/source/changelog/`, and the release-body step warns, without failing, if that entry's heading still reads "unreleased" diff --git a/docs/source/contributing/workflows.rst b/docs/source/contributing/workflows.rst index 509d380c22..cfd70cdff6 100644 --- a/docs/source/contributing/workflows.rst +++ b/docs/source/contributing/workflows.rst @@ -376,7 +376,7 @@ the same commit several times over. It also skips ``changes``, Checks`_ below), which are gated out by their own ``if:`` rather than by having already run. ``changelog`` carries no ``if:`` at all and runs on every path regardless -- deliberately, per its own -comment (``unit-tests.yml:1569-1575``): ``create-release.yml`` feeds +comment (``unit-tests.yml:1602-1608``): ``create-release.yml`` feeds ``CHANGELOG.md`` to the GitHub Release body, so the release path is exactly where a drifted file must not go unchecked. Confirmed on run `36210743295 `__ (a @@ -689,7 +689,7 @@ grepping every workflow file for its name: - Where * - ``Required checks passed`` - job ``required-checks`` - - ``unit-tests.yml:1904`` + - ``unit-tests.yml:1937`` * - ``Compat Python 3.11`` - job ``compatibility``, matrix leg ``3.11`` - ``python-compatibility.yml:60``, ``.github/python-versions.toml:20`` @@ -711,22 +711,22 @@ which added ``Compat Python 3.15`` and removed ``Compat Python 3.10``. Moving a between the tiers below means editing the ruleset in the same change. ``Required checks passed`` is defined by exactly one job -(``unit-tests.yml:1904``). ``Compat Python`` is defined by one, the +(``unit-tests.yml:1937``). ``Compat Python`` is defined by one, the ``compatibility`` job in ``python-compatibility.yml`` (``:60``), whose matrix (``:74``) comes from :file:`.github/python-versions.toml`: the required legs are its supported tier (``python-versions.toml:20-24``), and its KTLO legs (``python-versions.toml:16-19``) have names ending in ``(KTLO)``, so no required context matches one. A comment at -``unit-tests.yml:1816`` mentions the string without defining it. +``unit-tests.yml:1849`` mentions the string without defining it. ``Required checks passed`` is an aggregate, not a single check run. The ruleset requires it and the ``Compat Python 3.11``-``3.15`` legs, nothing else, as -``unit-tests.yml``'s own comment (``unit-tests.yml:1815-1825``) records. It +``unit-tests.yml``'s own comment (``unit-tests.yml:1848-1858``) records. It ``needs:`` the gating jobs of ``unit-tests.yml`` -- ``test``, ``integration``, ``engine-tests`` and ``pypcap-parity``, plus ``project-tests``, the one run of the project pins, and ``changes`` for the docs-only path -- and checks each result explicitly -(``unit-tests.yml:1903-1959``). Rulesets match check names literally, with no +(``unit-tests.yml:1936-1992``). Rulesets match check names literally, with no wildcard, so requiring the matrix cells one by one would mean editing the ruleset whenever a matrix changes; the aggregate keeps the required list fixed. It accepts the gating jobs as ``skipped`` only on a docs-only pull request, where @@ -735,7 +735,7 @@ It accepts the gating jobs as ``skipped`` only on a docs-only pull request, wher ``python-compatibility.yml``, and a job cannot ``needs:`` a job in another workflow file. This job runs on Unit Tests' own ``push``/``pull_request`` triggers; the ``gate-only: true`` reusable calls documented above skip it via -``if: ${{ always() && inputs.gate-only != true }}`` (``unit-tests.yml:1906``), +``if: ${{ always() && inputs.gate-only != true }}`` (``unit-tests.yml:1939``), so it is never produced -- and never expected -- on those paths. .. _ktlo-tier: diff --git a/docs/source/pcapkit/foundation/engines/3rdparty.rst b/docs/source/pcapkit/foundation/engines/3rdparty.rst index 71be92925c..bf6f3c5310 100644 --- a/docs/source/pcapkit/foundation/engines/3rdparty.rst +++ b/docs/source/pcapkit/foundation/engines/3rdparty.rst @@ -126,6 +126,19 @@ support, as is used by :class:`pcapkit.foundation.extraction.Extractor`. |pyshark|_'s own resolver for exactly that reason, so a correctly configured install off :envvar:`PATH` is not refused. + **Read access under AppArmor.** From Ubuntu 25.04, :program:`tshark` runs + confined by an enforcing AppArmor profile, :file:`/etc/apparmor.d/tshark`, + which reads captures only under :file:`/tmp`, so a capture anywhere else + fails with *You don't have permission to read the file* -- which |pyshark|_ + raises as ``TSharkCrashException``. The check above cannot see this. To allow + a directory, add a rule for it to the profile's local include and reload the + profile: + + .. code-block:: shell + + echo 'file r /path/to/captures/**,' | sudo tee -a /etc/apparmor.d/local/tshark + sudo apparmor_parser -r /etc/apparmor.d/tshark + .. autoclass:: pcapkit.foundation.engines.pyshark.PyShark :no-members: :show-inheritance: diff --git a/tests/project/test_tshark_apparmor_2019.py b/tests/project/test_tshark_apparmor_2019.py new file mode 100644 index 0000000000..5fffedf7a0 --- /dev/null +++ b/tests/project/test_tshark_apparmor_2019.py @@ -0,0 +1,157 @@ +# -*- coding: utf-8 -*- +"""#2019: every job that installs tshark lets it read the checkout past its AppArmor profile. + +From Ubuntu 25.04, apparmor 4.1 confines ``/usr/bin/tshark`` with a profile whose own +file rules read only ``/tmp`` and ``/usr/share/wireshark``, so on an ``ubuntu-26.04`` +runner ``tshark -r`` is denied every capture under ``$GITHUB_WORKSPACE``. The step +after each tshark install appends one read rule for the checkout to the profile's +``local/tshark`` include and reloads the profile. These tests run that step's real +``run:`` script with ``/etc/apparmor.d`` swapped for a scratch directory and +``sudo``/``apparmor_parser`` stubbed, so the guard, the rule and the reload are all +exercised rather than read. + +""" + +from __future__ import annotations + +import os +import pathlib +import re +import shutil +import subprocess # nosec: B404 +import tempfile +import unittest + +from tests._workflows import WORKFLOWS, load, steps + +WORKFLOW = 'unit-tests.yml' +NAME = 'Let tshark read the checkout past its AppArmor profile' +#: (job, the tshark install step the AppArmor step must directly follow). +SITES = ( + ('engine-tests', "Install this engine's system packages"), + ('pypcap-parity', 'Install libpcap headers, a C toolchain, and tshark'), + ('pypcap-parity-ktlo', 'Install libpcap headers, a C toolchain, and tshark'), +) +#: The directory the step edits, replaced by a scratch one in these tests. +APPARMOR = '/etc/apparmor.d' +#: An AppArmor file rule in ``file ACCESS FILEGLOB,`` form granting read on a tree +#: (apparmor.d(5), FILE RULE), the form the shipped tshark profile itself uses. +RULE = re.compile(r'^file r (/\S+)/\*\*,$') +STUBS = { + 'sudo': 'exec env "$@"\n', + 'apparmor_parser': 'echo "$@" >> "$STUB_LOG"\n', +} + + +def apparmor_step(job_id: 'str') -> 'dict[str, object]': + """Job ``job_id``'s AppArmor step, as the workflow loader parses it.""" + for item in steps(WORKFLOW, job_id): + if item.get('name') == NAME: + return item + raise AssertionError(f'{job_id} has no step named {NAME!r}') + + +class TestEveryTsharkInstallIsFollowed(unittest.TestCase): + """The step sits right after each tshark install, with one body everywhere.""" + + def test_the_step_directly_follows_each_install(self) -> None: + """Each tshark install is followed at once by the step, before any test runs.""" + for job_id, install in SITES: + with self.subTest(job=job_id): + names = [item.get('name') for item in steps(WORKFLOW, job_id)] + self.assertIn(install, names) + self.assertEqual(names[names.index(install) + 1], NAME) + + def test_every_job_installing_tshark_is_covered(self) -> None: + """A new tshark install elsewhere in the workflows needs the step too.""" + found = set() + for path in sorted(WORKFLOWS.glob('*.yml')): + for job_id, spec in (load(path).get('jobs') or {}).items(): + for item in spec.get('steps') or (): + if re.search(r'apt-get install[^\n]*\btshark\b|apt_install tshark\b', + str(item.get('run', ''))): + found.add((path.name, job_id)) + self.assertEqual(found, {(WORKFLOW, job_id) for job_id, _ in SITES}) + + def test_one_body_everywhere(self) -> None: + """The jobs run one script, so a fix to it cannot reach only some of them.""" + bodies = {job_id: apparmor_step(job_id)['run'] for job_id, _ in SITES} + self.assertEqual(len(set(bodies.values())), 1, bodies) + + def test_engine_tests_runs_it_only_for_pyshark(self) -> None: + """Only the PyShark cell installs tshark, so only it reloads the profile.""" + self.assertEqual(apparmor_step('engine-tests').get('if'), "env.ENGINE == 'PyShark'") + + def test_a_ktlo_parity_leg_is_not_failed_by_it(self) -> None: + """Every parity leg runs it, and a KTLO leg's failure there stays a warning (#1672).""" + for job_id in ('pypcap-parity', 'pypcap-parity-ktlo'): + with self.subTest(job=job_id): + item = apparmor_step(job_id) + self.assertNotIn('if', item) + self.assertEqual(item.get('continue-on-error'), '${{ matrix.ktlo == true }}') + + +class TestTheStepRuns(unittest.TestCase): + """The step's real script, against a scratch ``/etc/apparmor.d``.""" + + def setUp(self) -> None: + if shutil.which('bash') is None: # pragma: no cover + self.skipTest('bash is not available') + tmp = tempfile.TemporaryDirectory(prefix='pcapkit-2019-') # pylint: disable=consider-using-with + self.addCleanup(tmp.cleanup) + self.tmp = pathlib.Path(tmp.name) + self.bin = self.tmp / 'bin' + self.bin.mkdir() + for command, body in STUBS.items(): + path = self.bin / command + path.write_text('#!/bin/bash\n' + body, encoding='utf-8') + path.chmod(0o755) + self.etc = self.tmp / 'apparmor.d' + self.etc.mkdir() + self.log = self.tmp / 'parser.log' + self.workspace = '/home/runner/work/PyPCAPKit/PyPCAPKit' + + def run_step(self) -> 'int': + """Run the step's script against the scratch directory; return its status.""" + script = str(apparmor_step('engine-tests')['run']) + self.assertEqual(script.count(APPARMOR), 4, + f'{NAME!r} no longer names {APPARMOR} four times') + script = script.replace(APPARMOR, str(self.etc)) + env = dict(os.environ, PATH=f'{self.bin}{os.pathsep}{os.environ["PATH"]}', + STUB_LOG=str(self.log), GITHUB_WORKSPACE=self.workspace) + return subprocess.run(['bash', '-e', '-c', script], env=env, check=False, + capture_output=True, text=True).returncode + + def parser_calls(self) -> 'list[str]': + """The arguments of every stubbed ``apparmor_parser`` call.""" + return self.log.read_text(encoding='utf-8').splitlines() if self.log.exists() else [] + + def test_with_the_profile_the_checkout_is_allowed_and_the_profile_reloaded(self) -> None: + """26.04: one read rule for the workspace lands in local/tshark, then ``-r`` reloads.""" + (self.etc / 'tshark').write_text('include if exists \n', encoding='utf-8') + self.assertEqual(self.run_step(), 0) + lines = (self.etc / 'local' / 'tshark').read_text(encoding='utf-8').splitlines() + self.assertEqual(len(lines), 1, lines) + match = RULE.match(lines[0]) + self.assertIsNotNone(match, lines[0]) + self.assertEqual(match.group(1), self.workspace) # type: ignore[union-attr] + self.assertEqual(self.parser_calls(), [f'-r {self.etc}/tshark']) + + def test_an_existing_local_file_is_appended_to(self) -> None: + """A site's own additions in local/tshark are kept.""" + (self.etc / 'tshark').write_text('include if exists \n', encoding='utf-8') + (self.etc / 'local').mkdir() + (self.etc / 'local' / 'tshark').write_text('file r /srv/**,\n', encoding='utf-8') + self.assertEqual(self.run_step(), 0) + lines = (self.etc / 'local' / 'tshark').read_text(encoding='utf-8').splitlines() + self.assertEqual(lines, ['file r /srv/**,', f'file r {self.workspace}/**,']) + + def test_without_the_profile_it_is_a_no_op(self) -> None: + """24.04: no profile, so nothing is written and nothing is reloaded.""" + self.assertEqual(self.run_step(), 0) + self.assertFalse((self.etc / 'local').exists()) + self.assertEqual(self.parser_calls(), []) + + +if __name__ == '__main__': + unittest.main()