Support yum for hermetic build#1715
Conversation
|
rhel6 image fails when gathering rpms from buildroot. It could be fixed by this. My concerns: Btw, I was able to successfully build rhel6 package with this change. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for yum in query_packages_location to enable hermetic builds on older systems. However, a high-severity command injection vulnerability was identified in mock/py/mockbuild/installed_packages.py where the dnf_cmd parameter is used without proper validation, potentially allowing arbitrary command execution. Additionally, the chrootpath is ignored for yum, which could lead to incorrect behavior within a chroot, and there's a portability concern with a hardcoded path to the repoquery executable.
| query_locations_cmd = ['/usr/bin/repoquery'] | ||
| else: | ||
| # dnf | ||
| query_locations_cmd = [dnf_cmd] |
There was a problem hiding this comment.
The dnf_cmd parameter is used directly to build a command for execution without proper validation. An attacker who can control the value of dnf_cmd can execute arbitrary commands on the system. The value should be validated against an allow-list of expected commands (e.g., /bin/dnf, /usr/bin/yum) before being used.
| # yum (yum-utils must be installed) | ||
| if not os.path.exists('/usr/bin/repoquery'): | ||
| raise mockbuild.exception.Error("Missing /usr/bin/repoquery") | ||
| query_locations_cmd = ['/usr/bin/repoquery'] |
There was a problem hiding this comment.
When using yum, the chrootpath is ignored. This will cause repoquery to use the host's repository configuration instead of the one inside the chroot, which is incorrect. The repoquery command for yum should be configured to use the chroot's configuration. You can achieve this by using the --config flag to point to the yum.conf file inside the chroot.
| query_locations_cmd = ['/usr/bin/repoquery'] | |
| query_locations_cmd = ['/usr/bin/repoquery'] | |
| if chrootpath: | |
| query_locations_cmd += ['--config', os.path.join(chrootpath, 'etc/yum.conf')] |
| query_locations_cmd += [f"--installroot={chrootpath}"] | ||
| if 'yum' in dnf_cmd: | ||
| # yum (yum-utils must be installed) | ||
| if not os.path.exists('/usr/bin/repoquery'): |
There was a problem hiding this comment.
Hardcoding the path to repoquery as /usr/bin/repoquery is not portable and may fail on systems where it's installed in a different location. This goes against the style guide's principle of compatibility across different Linux platforms. It would be more robust to search for the executable in the system's PATH. Consider adding a helper function to mock/py/mockbuild/util.py to find executables, similar to shutil.which, to ensure compatibility.
References
- Repository style guide rule on compatibility: 'Code should run on both new and old Linux platforms. Including Fedora rawhide, all supported RHELs, openSUSE etc.' Hardcoding paths can break this compatibility. (link)
- Repository style guide rule on internal utils: 'The file mock/py/mockbuild/util.py contains several helper functions that helps with compatibility. The code should use these functions when possible instead standard Python functions.' A new helper could be added here. (link)
tkopecek
force-pushed
the
hermetic-rhel6
branch
from
b067986 to
0941200
Compare
| sys.exit(1) | ||
|
|
||
| subprocess.check_call(["createrepo_c", options.output_repo]) | ||
| subprocess.check_call(["createrepo_c", "--compatibility", options.output_repo]) |
There was a problem hiding this comment.
I am surprised that this is enough to make ancient clients happy. Nice.
There was a problem hiding this comment.
It doesn't work for all repodata features, but it is enough for basic (rpms-only non-external repo (no mergerepo_c)) repo.
|
Can you please add a Towncrier snippet? Otherwise looks good. |
Added - should I squash it? |
Yes, please |
| @@ -0,0 +1,5 @@ | |||
| ``--calculate-dependencies`` command now support also ``yum`` package manager | |||
| for building older distributions. Using correctt bootstrap image with yum now | |||
|
LGTM, hermetic test-case passes, +1 |
tkopecek
force-pushed
the
hermetic-rhel6
branch
from
cd8b2ba to
6c0ccd6
Compare
praiskup
merged commit 3a77f8c
into
rpm-software-management:main
2 participants
No description provided.