diff --git a/.github/workflows/pr-repoconf.yml b/.github/workflows/pr-repoconf.yml new file mode 100644 index 00000000..e3223468 --- /dev/null +++ b/.github/workflows/pr-repoconf.yml @@ -0,0 +1,46 @@ +name: PR-Repoconf + +# What a generated .repo section tells dnf to verify is a security decision, and +# it is only observable by building an SDK and reading the file out of it. These +# tests are the substitute for that build, so they have to run somewhere. + +on: + pull_request: + types: [opened, synchronize, reopened] + paths: + - 'meta-avocado/lib/**' + - 'meta-avocado/tests/**' + - 'meta-avocado/recipes-avocado/sdk/**' + - '.github/workflows/pr-repoconf.yml' + push: + branches: [scarthgap] + paths: + - 'meta-avocado/lib/**' + - 'meta-avocado/tests/**' + - 'meta-avocado/recipes-avocado/sdk/**' + workflow_dispatch: + +permissions: + contents: read + +# Cancel superseded runs so a burst of `synchronize` pushes does not stack up. +concurrency: + group: repoconf-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + repoconf: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + + # Standard library only - no install step, so a dependency failure cannot + # silently shrink the suite. + - name: Run the repo-config renderer tests + run: | + python -m unittest discover -v \ + -s meta-avocado/tests -p 'test_*.py' diff --git a/docs/local-package-feed.md b/docs/local-package-feed.md index 471c783c..99a36b20 100644 --- a/docs/local-package-feed.md +++ b/docs/local-package-feed.md @@ -200,13 +200,23 @@ curl -s -o /dev/null -w '%{http_code}\n' \ Package signing is a separate question and no part of the build signs RPMs today, so `AVOCADO_REPO_GPGCHECK` stays at `0` regardless of what the index does. -**These variables do not reach every repo the SDK reads.** They govern the -per-target sections generated by `avocado-sdk-metadata.bb`. The `[avocado-sdk]` -section is a static file installed by `avocado-sdk-repos.bb`, so it carries a -hardcoded `gpgcheck=0` and no `repo_gpgcheck` no matter what these are set to. -Enabling metadata verification therefore leaves that one repository unverified, -and it is the repository every SDK container reads. Bringing it under the same -renderer has to happen before verification can be called complete. +Both variables reach every `.repo` section the build installs: the per-target +sections generated by `avocado-sdk-metadata.bb`, and the `[avocado-sdk]` section +the SDK container itself reads, generated by `avocado-sdk-repos.bb`. Both render +through `meta-avocado/lib/avocado_sdk_metadata/repoconf.py`, so setting one +variable is enough - no `.repo` section holds a hardcoded value. + +One hardcoded value sits outside that set. `[main] gpgcheck=True` in +`avocado-sdk-repos`' `dnf.conf` is the container-wide default for a repo that +sets nothing. Every section rendered above sets `gpgcheck` explicitly and the +per-repo value wins, so it changes nothing today - but a `.repo` file added to +the container by someone else inherits it, and no build variable reaches it. + +The `[avocado-sdk]` section writes no `priority`, so it sits at dnf's default of +99. That is deliberate: `avocado-cli` points `reposdir` at both this file and +the per-target config, and dnf ranks priority across that merged set rather than +per file. The per-target sections hold 1 through N, so 99 keeps `[avocado-sdk]` +below them - where the static file this replaced effectively sat. --- diff --git a/meta-avocado/lib/avocado_sdk_metadata/__init__.py b/meta-avocado/lib/avocado_sdk_metadata/__init__.py index 98813136..a9506fc9 100644 --- a/meta-avocado/lib/avocado_sdk_metadata/__init__.py +++ b/meta-avocado/lib/avocado_sdk_metadata/__init__.py @@ -1 +1,8 @@ # SPDX-License-Identifier: Apache-2.0 + +# BitBake hashes an addpylib module's functions into task signatures only for +# the submodules named here: bb/parse/ast.py's PyLibNode.eval reads BBIMPORTS +# and calls bb.codeparser.add_module_functions once per entry. A submodule left +# out is invisible to every task hash, so editing it leaves sstate valid and +# ships the previously generated file. +BBIMPORTS = ["repoconf"] diff --git a/meta-avocado/lib/avocado_sdk_metadata/repoconf.py b/meta-avocado/lib/avocado_sdk_metadata/repoconf.py index 8fff6d86..e631e109 100644 --- a/meta-avocado/lib/avocado_sdk_metadata/repoconf.py +++ b/meta-avocado/lib/avocado_sdk_metadata/repoconf.py @@ -23,13 +23,17 @@ signatures breaks every install, so the switch belongs with the publisher change, not ahead of it. -devtool-debt: only the sections generated by avocado-sdk-metadata.bb render -through here. The [avocado-sdk] section is a static file installed by -avocado-sdk-repos.bb and carries a hardcoded gpgcheck=0 with no repo_gpgcheck. -Ceiling: adequate only while metadata verification is off everywhere. -Upgrade trigger: bring that recipe through this renderer before anything sets -AVOCADO_REPO_METADATA_GPGCHECK=1, or the SDK repo every container reads stays -unverified while the generated ones are verified. +Every ``.repo`` section the tree installs renders through here: the per-target +sections from avocado-sdk-metadata.bb, and the SDK container's own +``[avocado-sdk]`` from avocado-sdk-repos.bb. That is what makes one switch +enough - a section rendered anywhere else would sit at whatever it was +hardcoded to while the rest moved. + +One hardcoded verification value remains and is deliberately out of scope here: +``[main] gpgcheck=True`` in avocado-sdk-repos' dnf.conf. It is dnf's default for +a repo that sets nothing, and every section rendered here sets ``gpgcheck`` +explicitly, so it decides nothing today - it would decide for a repo added to +the container by someone else. """ # Relative to the repository root, which is also what baseurl points at. @@ -55,6 +59,12 @@ def render_repo_section( ``baseurl_path`` is appended to the ``${repo_url}`` placeholder, which is substituted by the client at SDK install time rather than at build time. + ``priority`` of None writes no priority line, leaving the section at dnf's + default of 99. It stays a required argument so that forgetting it is a + TypeError rather than a section that silently sorts below its siblings - + the per-target file ranks its sections 1..N and depends on every one of + them carrying a priority. + Raises: ValueError: ``baseurl_path`` is empty, which would point the client at the feed root, or either verification switch is not a dnf boolean. @@ -83,6 +93,32 @@ def render_repo_section( # same step that publishes the signature, so referencing it while # verification is off points at a 404. lines.append(f"gpgkey={baseurl}/{_KEY_RELPATH}") - lines.append(f"priority={priority}") + if priority is not None: + lines.append(f"priority={priority}") return "\n".join(lines) + "\n\n" + + +def render_sdk_repo_section(*, gpgcheck="0", repo_gpgcheck="0"): + """Return the ``[avocado-sdk]`` block the SDK container's own dnf reads. + + The three literals live here rather than in the recipe because a recipe is + a BitBake task the test suite cannot import: spelled there, a typo in the + section name or the path would pass every test and 404 on the container's + first transaction. + + No priority. avocado-cli merges this file with the per-target repo config + in one transaction (it sets ``reposdir`` to both directories), and dnf + ranks priority globally across that merged set rather than per file. The + per-target sections occupy 1..N, so leaving this at dnf's default of 99 + ranks it below them - which is where the static file this replaced + effectively sat. + """ + return render_repo_section( + section="avocado-sdk", + name="Avocado SDK", + baseurl_path="$releasever/sdk/all", + priority=None, + gpgcheck=gpgcheck, + repo_gpgcheck=repo_gpgcheck, + ) diff --git a/meta-avocado/recipes-avocado/sdk/avocado-sdk-metadata.bb b/meta-avocado/recipes-avocado/sdk/avocado-sdk-metadata.bb index 94600bd7..d9e437dc 100644 --- a/meta-avocado/recipes-avocado/sdk/avocado-sdk-metadata.bb +++ b/meta-avocado/recipes-avocado/sdk/avocado-sdk-metadata.bb @@ -47,7 +47,11 @@ PACKAGE_ARCH = "all_avocadosdk" python do_compile() { import os import bb - import avocado_sdk_metadata.repoconf as repoconf + # Imported and called fully qualified, not aliased. BitBake records the + # literal call spelling and matches it against the registered name + # avocado_sdk_metadata.repoconf.; an alias never matches, so the + # renderer's source would drop out of this task's hash. + import avocado_sdk_metadata.repoconf deploy_dir_rpm = d.getVar('DEPLOY_DIR_RPM') machine = d.getVar('MACHINE') @@ -84,7 +88,7 @@ python do_compile() { # section or bump priority, but the arch is tracked above. return False written_sections.add(repo_section_name) - repo_f.write(repoconf.render_repo_section( + repo_f.write(avocado_sdk_metadata.repoconf.render_repo_section( section=repo_section_name, name=repo_name, baseurl_path=repo_url_path, @@ -159,7 +163,7 @@ python do_compile() { def _write_additional_target_repo(repo_f, priority): """Write the target-ext repo entry at the end.""" - repo_f.write(repoconf.render_repo_section( + repo_f.write(avocado_sdk_metadata.repoconf.render_repo_section( section=f"{machine_short_name}-target-ext", name=f"{machine_short_name}-target-ext", baseurl_path=f"$releasever/target/{machine_short_name}-ext", diff --git a/meta-avocado/recipes-avocado/sdk/avocado-sdk-repos.bb b/meta-avocado/recipes-avocado/sdk/avocado-sdk-repos.bb index 49db988b..9688fa67 100644 --- a/meta-avocado/recipes-avocado/sdk/avocado-sdk-repos.bb +++ b/meta-avocado/recipes-avocado/sdk/avocado-sdk-repos.bb @@ -5,17 +5,11 @@ LIC_FILES_CHKSUM = "file://${COMMON_LICENSE_DIR}/Apache-2.0;md5=89aea4e17d99a7ca PV = "${SDK_VERSION}" SRC_URI = " \ - file://avocado-sdk.repo \ file://dnf.conf \ " S = "${WORKDIR}" -REPO_BASE = "${AVOCADO_REPO_BASE}" - -# Monitor AVOCADO_REPO_BASE for changes -vardeps += "AVOCADO_REPO_BASE" - FILES:${PN} += "${sysconfdir}/yum.repos.d/avocado-sdk.repo" inherit update-alternatives @@ -27,11 +21,38 @@ ALTERNATIVE_LINK_NAME[dnf_conf] = "${sysconfdir}/dnf/dnf.conf" ALTERNATIVE_LINK_NAME[rpm_platform] = "${sysconfdir}/rpm/platform" ALTERNATIVE_LINK_NAME[rpmrc] = "${sysconfdir}/rpmrc" +# The [avocado-sdk] section is generated rather than shipped as a static file +# so that the verification switches reach it. A hardcoded gpgcheck=0 would stay +# unverified while every generated section switched over, and the repository +# left out is the one every SDK container reads. +python do_compile() { + import os + # Imported and called fully qualified, not aliased. BitBake records the + # literal call spelling and matches it against the registered name + # avocado_sdk_metadata.repoconf.; an alias never matches, so the + # renderer's source would drop out of this task's hash. + import avocado_sdk_metadata.repoconf + + # Package signatures. Distinct from AVOCADO_REPO_METADATA_GPGCHECK, which + # is the one a signed repomd.xml needs. See + # meta-avocado/lib/avocado_sdk_metadata/repoconf.py for why they differ. + gpg_check = d.getVar('AVOCADO_REPO_GPGCHECK') or '0' + repo_metadata_gpg_check = d.getVar('AVOCADO_REPO_METADATA_GPGCHECK') or '0' + + out_dir = os.path.join(d.getVar('WORKDIR'), 'generated-files') + os.makedirs(out_dir, exist_ok=True) + + with open(os.path.join(out_dir, 'avocado-sdk.repo'), 'w') as repo_f: + repo_f.write(avocado_sdk_metadata.repoconf.render_sdk_repo_section( + gpgcheck=gpg_check, + repo_gpgcheck=repo_metadata_gpg_check, + )) +} + do_install() { # Add Avocado SDK repo install -d ${D}${sysconfdir}/yum.repos.d - install -m 0644 ${WORKDIR}/avocado-sdk.repo ${D}${sysconfdir}/yum.repos.d/avocado-sdk.repo - sed -i "s|{REPO_BASE}|${REPO_BASE}|g" ${D}${sysconfdir}/yum.repos.d/avocado-sdk.repo + install -m 0644 ${WORKDIR}/generated-files/avocado-sdk.repo ${D}${sysconfdir}/yum.repos.d/avocado-sdk.repo install -d ${D}${sysconfdir}/dnf install -m 644 ${WORKDIR}/dnf.conf ${D}${sysconfdir}/dnf/dnf.conf.${PN} diff --git a/meta-avocado/recipes-avocado/sdk/avocado-sdk-repos/avocado-sdk.repo b/meta-avocado/recipes-avocado/sdk/avocado-sdk-repos/avocado-sdk.repo deleted file mode 100644 index 1ca86dfd..00000000 --- a/meta-avocado/recipes-avocado/sdk/avocado-sdk-repos/avocado-sdk.repo +++ /dev/null @@ -1,5 +0,0 @@ -[avocado-sdk] -name=Avocado SDK -baseurl=${repo_url}/$releasever/sdk/all -enabled=1 -gpgcheck=0 diff --git a/meta-avocado/tests/test_repoconf.py b/meta-avocado/tests/test_repoconf.py index d55f51b4..3ba1f60c 100644 --- a/meta-avocado/tests/test_repoconf.py +++ b/meta-avocado/tests/test_repoconf.py @@ -115,6 +115,97 @@ def test_priority_is_rendered_from_an_integer(self): self.assertEqual(parse(self._render(priority=11))["priority"], "11") +class PriorityIsOmissibleButNotOptional(unittest.TestCase): + """A section can decline to rank itself; a caller cannot decline to say so. + + dnf ranks priority globally across every repo it loads, defaulting to 99. + The per-target file ranks its own sections 1..N and breaks if one of them + silently lands at 99, so omitting the argument stays a TypeError while + passing None explicitly writes no line. + """ + + def test_passing_none_writes_no_priority_line(self): + text = repoconf.render_repo_section( + section="s", name="n", baseurl_path="p", priority=None + ) + + self.assertNotIn("priority", parse(text)) + + def test_omitting_the_argument_is_still_an_error(self): + # The guard the per-target file depends on: a section added there + # without a priority must not render, it must fail. + with self.assertRaises(TypeError): + repoconf.render_repo_section(section="s", name="n", baseurl_path="p") + + def test_a_zero_priority_is_still_written(self): + # Only None omits the line. Zero is a real dnf priority and must not + # be swallowed by a falsy test. + text = repoconf.render_repo_section( + section="s", name="n", baseurl_path="p", priority=0 + ) + + self.assertEqual(parse(text)["priority"], "0") + + def test_the_separator_survives_the_missing_line(self): + # The priority line was last, so dropping it must not take the blank + # separator with it. + text = repoconf.render_repo_section( + section="s", name="n", baseurl_path="p", priority=None + ) + + self.assertTrue(text.endswith("\n\n")) + + +class SdkRepoSection(unittest.TestCase): + """Pin the block the SDK container's own dnf reads. + + These literals used to be a checked-in file a reviewer could diff. Now they + are code, so this is what stands between a typo in the section name or the + path and a container whose first dnf transaction 404s. + """ + + def test_the_whole_block_is_pinned(self): + self.assertEqual( + repoconf.render_sdk_repo_section(), + "[avocado-sdk]\n" + "name=Avocado SDK\n" + "baseurl=${repo_url}/$releasever/sdk/all\n" + "enabled=1\n" + "gpgcheck=0\n" + "repo_gpgcheck=0\n" + "\n", + ) + + def test_it_declines_to_rank_itself(self): + # avocado-cli merges this file with the per-target config in one + # transaction, where the target sections hold 1..N. Staying at dnf's + # default of 99 keeps this below them, as the static file it replaced + # effectively was. + self.assertNotIn("priority", parse(repoconf.render_sdk_repo_section())) + + def test_metadata_verification_reaches_it(self): + # The whole point of routing this section through the renderer: the + # switch has to reach the one repo every SDK container reads. + parsed = parse(repoconf.render_sdk_repo_section(repo_gpgcheck="1")) + + self.assertEqual(parsed["repo_gpgcheck"], "1") + self.assertEqual( + parsed["gpgkey"], + "${repo_url}/$releasever/sdk/all/repodata/repomd.xml.key", + ) + + def test_package_signature_checking_reaches_it_independently(self): + parsed = parse(repoconf.render_sdk_repo_section(gpgcheck="1")) + + self.assertEqual(parsed["gpgcheck"], "1") + self.assertEqual(parsed["repo_gpgcheck"], "0") + + def test_a_bad_switch_value_is_rejected_here_too(self): + # The wrapper must not become a way around the boolean check. + with self.assertRaises(ValueError): + repoconf.render_sdk_repo_section(repo_gpgcheck="yes") + + class RenderRepoSectionRejectsBadInput(unittest.TestCase): def test_an_empty_baseurl_path_is_rejected(self): # A section with no path silently points every client at the feed root.