-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CVE-2024-1753 fix for main + pasta setup changes #22079
CVE-2024-1753 fix for main + pasta setup changes #22079
Conversation
Bump to the version of Buidah in it's main branch to get the CVE-2024-1753 fix. [NO NEW TESTS NEEDED] Signed-off-by: tomsweeneyredhat <[email protected]>
I will say I was not expecting Buildah to report itself as 1.35.1-#, rather, I thought it would be 1.35.0-# or perhaps 1.36.0-dev-* which is the version in the top of main. |
Cockpit tests failed for commit 079bfb0. @martinpitt, @jelly, @mvollmer please check. |
yeah that is normal, the go logic to determine the version is not perfect so it can guess things wrong, in any case the version doesn't mean anything as we would still not not from when the vendor really is. Really the interesting part to figure out what is being vendored is the |
Looking at the test failures I think my pasta changes in c/common caused them, I guess given you already vendored the required changes I need I could just push my fix commits to this PR here to avoid duplicate vendoring work. |
By default we just ignored any localhost reolvers, this is problematic for anyone with more complicated dns setups, i.e. split dns with systemd-reolved. To address this we now make use of the build in dns proxy in pasta. As such we need to set the default nameserver ip now. A second change is the option to exclude certain ips when generating the host.containers.internal ip. With that we no longer set it to the same ip as is used in the netns. The fix is not perfect as it could mean on a system with a single ip we no longer add the entry, however given the previous entry was incorrect anyway this seems like the better behavior. Fixes containers#22044 Signed-off-by: Paul Holzinger <[email protected]>
Always teardown the network, trying to reuse the netns has caused a significant amount of bugs in this code here. It also never worked for containers with user namespaces. So once and for all simplify this by never reusing the netns. Originally this was done to have a faster restart of containers but with netavark now we are much faster so it shouldn't be that noticeable in practice. It also makes more sense to reconfigure the netns as it is likely that the container exited due some broken network state in which case reusing would just cause more harm than good. The main motivation for this change was the pasta change to use --dns-forward by default. As the restarted contianer had no idea what nameserver to use as pasta just kept running. Signed-off-by: Paul Holzinger <[email protected]>
@mheon PTAL at my pasta changes, as we discussed I also removed the netns reuse logic in the restart case. |
Cockpit tests failed for commit 15b8bb7. @martinpitt, @jelly, @mvollmer please check. |
Sure, LGTM |
Yeah, this is set to merge
…On Tue, Mar 19, 2024 at 16:32 Tom Sweeney ***@***.***> wrote:
Thanks @Luap99 <https://github.com/Luap99> Rawhide appears to be the only
test hanging, and I think that's expected. @mheon
<https://github.com/mheon>, can this be merged?
—
Reply to this email directly, view it on GitHub
<#22079 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3AOCFXHT56RVTT5YTZ5Q3YZCOENAVCNFSM6AAAAABE4JUCP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBYGA3TOMZRGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, TomSweeneyRedHat The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
e5059fc
into
containers:main
Bump to the version of Buidah in its main branch to get the CVE-2024-1753 fix.
[NO NEW TESTS NEEDED]
Does this PR introduce a user-facing change?