-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
libpod: Drop checks for paths in sqlite+boltdb #23447
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cgwalters The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Cockpit tests failed for commit 389e7b7. @martinpitt, @jelly, @mvollmer please check. |
The original logic here is old, dating to containers@7eb5ce9 and got inherited when the sqlite database was added. Since then, various changes have landed here especially around canonicalizing symbolic links. However, this code *still* often causes problems; most recently in https://gitlab.com/fedora/bootc/base-images/-/issues/20 where it seems like the way Anaconda has the system set up trips this up again. I can certainly believe that things can go wrong if one overrides/reconfigures e.g. the runtime state dir to be different. But there's also a lot of other ways to break podman...and it's trivial to subvert this check with a bind mount over the absolute path, pointing to some arbitrary different place. In general, encoding file names into files that are potentially owned by the user is ugly...it can trip up basic things like migrating a home directory, etc. Since I am not aware of a common misconfiguration that these checks block, and I am *very* aware of a lot of times they have incorrectly blocked correct situations...just drop the checks. If we *do* need to do some more validation later, I think we could say encode the directory inodes for at least the volume dir. And the runtime dir could have the inode for the root, but not the other way around. Signed-off-by: Colin Walters <walters@verbum.org>
389e7b7
to
1943115
Compare
For compatibility we need to keep writing the values of course...but hopefully at some point we can just insert dummy values there instead for a further minor cleanup. |
These do still fulfill a purpose in my mind - preventing users from causing a nonfunctional, difficult to recover from configuration by swapping paths in the config file while containers are active. They may be doing that intentionally - your home directory migration is an example - but our database has a lot of paths stored in it, so if you remove the check code and do the migration, things will still be very broken (containers unable to fully remove because we don't know where all their files live, for example). This forces folks to use I have been considering adding a "trust me, I know what I'm doing" flag to |
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.
There is still the call to runtime.mergeDBConfig(dbConfig)
so all the paths are overwritten silently anyway so the question to me is what exactly is this validation good for. I really don't know.
(Overall I am in favour of doing this)
In general it is impossible for me to the follow the initial path setup code with any reasonable logic. I have no idea what it is supposed to do in what places. I don't understand it and we just keep braking things in very weird ways, changes like 6293ec2 are just not understandable for me regardless of how much time I try to spend on this.
I guess there is the problem around the tmpdir handing when users log in with su without XDG_RUNTIME_DIR set and then later running a command in proper session with XDG_RUNTIME_DIR and then they end up with different paths which will certainly break a lot of stuff which is something we want to catch...
And in practice, you have seen people do this? How often? More often than the false positives?
And which specific paths have you seen swapped? I'm guessing it's mostly the root (and not the runroot or volumes)?
I think this is an interesting part - we hence don't need to check if there aren't any containers, which would let us move a lot of checking to a later dynamic point. If we just did that, it would help a lot I think. Combining these...I am pretty sure we can add highly reliable sanity checking that the runtime dir is paired with its correct static root, e.g. again via inode comparison or so. |
sketching out runroot+root binding:
With something like that we could do verification both ways and it would detect things like bind mounts pointing to a new place unexpectedly, which file path comparison wouldn't. |
I know support does see this a fair bit. Is it catching issues there, or being a hindrance? I think 50/50, but I'll ask to be sure. Path moves usually seem to be tmpdir related - a result of systemd misconfiguration (no enable-linger, Podman starts to use /tmp, someone notices, lingering is enabled, now we need to swap back to |
What about moving the tmpdir actually under the storage root, as its own tmpfs? |
How would we create the tmpfs mount? AS rootless we first would need to join the userns but in order to so we must read the pidfile from the tmpdir so catch 22. And creating the initial mount would be racy without locks so this complicates things more I think. And for rootless it will make the file inaccessible outside the user+mountns which makes it harder to look at them (not that there would be reason to so for a normal user but I think tests depend on it) |
That sounds better, but keep in mind file handles must be supported by the fs and are privileged AFAIK thus will nor work rootless. But yes some device+inode mapping seems better if paths change, however as @mheon mentioned the actual db will store full paths in container configs and so on so if a path actually moved with the same inode it might still break much later. Although I know this is not a problem for you build use cases where this will work better so maybe it would be good enough? |
Can we just detect the situation when we have containers in the root, but the tmpdir is uninitialized(empty)/nonexistent, and make that the error case? |
Eww? |
A friendly reminder that this PR had no activity for 30 days. |
@mheon @Luap99 @cgwalters what should we do with this one? |
The original logic here is old, dating to
7eb5ce9 and got inherited when the sqlite database was added.
Since then, various changes have landed here especially around canonicalizing symbolic links.
However, this code still often causes problems; most recently in https://gitlab.com/fedora/bootc/base-images/-/issues/20 where it seems like the way Anaconda has the system set up trips this up again.
I can certainly believe that things can go wrong if one overrides/reconfigures e.g. the runtime state dir to be different. But there's also a lot of other ways to break podman...and it's trivial to subvert this check with a bind mount over the absolute path, pointing to some arbitrary different place.
In general, encoding file names into files that are potentially owned by the user is ugly...it can trip up basic things like migrating a home directory, etc.
Since I am not aware of a common misconfiguration that these checks block, and I am very aware of a lot of times they have incorrectly blocked correct situations...just drop the checks.
If we do need to do some more validation later, I think we could say encode the directory inodes for at least the volume dir. And the runtime dir could have the inode for the root, but not the other way around.