build(docker): replace apt-key with keyring; use gnupg instead of gnupg2#3844
Conversation
|
Welcome @parisnakitakejser! It looks like this is your first PR to falcosecurity/falco 🎉 |
0917549 to
0ed0bda
Compare
leogr
left a comment
There was a problem hiding this comment.
Hey @parisnakitakejser, thanks for looking into this!
The apt-key modernization and the gnupg2 👉 gnupg change are good improvements 👍
However, upgrading the base image to debian:13-slim and dropping gcc-11 is something we can't do. The falco-driver-loader image exists to compile kernel modules and legacy eBPF probes on-the-fly on user systems. The GCC version must be compatible with the one used to build the running kernel, and different kernel versions require different GCC versions (e.g., kernel 5.x needs gcc-11, kernel 6.0-6.4 needs gcc-12).
By bumping to Debian 13, the default gcc would jump to 14.x (only valid for kernel 6.9+) and gcc-11 would no longer be available from apt. That would break driver compilation for anyone running kernel 5.x or 6.0-6.8.
N.B. the main falcosecurity/falco image is already based on Wolfi (not Debian) and is not affected by Debian CVEs. The -debian variant and the driver-loader image are Debian-based specifically because of the build toolchain requirements. They use old Debian versions on purpose, and the CVEs reported can't be really exploited because the affected tools are only used to build the driver (during the init phase) and not to run Falco.
May you consider splitting this PR? The apt-key and gnupg fixes could be submitted separately and would likely be accepted. The base image bump needs a broader discussion since it impacts driver compatibility.
Thanks 🙏
/kind cleanup
/area build
0ed0bda to
73c88ce
Compare
Keep the Debian 12 base image and gcc-11 toolchain for driver-loader compatibility while replacing apt-key usage and gnupg2 package references where safe. Signed-off-by: Paris Nakita Kejser <hi@pnk.sh>
73c88ce to
eacdd29
Compare
I have tryed to do what you explain, let me know if i miss something thanks :) |
|
Hi @parisnakitakejser, as you noticed I adjusted the title and put back the original template that should be used for the PR description, please fill it to reflect your changes and feel free to ping me if you need assistance. |
If you can help me to wirte the description in the right way, like you did with the title i will be very happy it's look you are better then me to do that part, :) |
leogr
left a comment
There was a problem hiding this comment.
Thanks for the rework, this addresses my previous feedback nicely 👍
Just left a tiny nit about the keyring filename (see inline).
Also, the same apt-key + gnupg2 pattern is still present in docker/driver-loader-buster/Dockerfile. May you consider updating it here, too, or leave it for a follow-up PR? Since that image is a legacy one, I would not block on it.
Thanks 🙏
Co-authored-by: Leonardo Grasso <me@leonardograsso.com> Signed-off-by: Paris Nakita Kejser <parisnakitakejser@users.noreply.github.com>
|
/milestone 0.44.0 |
|
LGTM label has been added. DetailsGit tree hash: b46e0131d04b49a562db240e7456a847d2c0f92f |
sgaist
left a comment
There was a problem hiding this comment.
I updated the description as well as unless you are aware of the gnupg2 vs gnupg package situation in Debian, it is not necessarily an obvious thing to do.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: leogr, parisnakitakejser, sgaist The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
Any specific area of the project related to this PR?
/area build
What this PR does / why we need it:
gnupg2 is, at the time of this patch, a dummy transitional package. gnupg is the official package name that provides gnupg 2.
This PR updates the Dockerfiles to correct the package name.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: