Skip to content
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

Change dbus interfacing for making rootless podman containers work #2208

Closed
YJDoc2 opened this issue Jul 26, 2023 · 13 comments
Closed

Change dbus interfacing for making rootless podman containers work #2208

YJDoc2 opened this issue Jul 26, 2023 · 13 comments
Assignees

Comments

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jul 26, 2023

ref : #719 , #1171
check #1171 (comment) , #1171 (comment) first.

This is a summarized version of discussion utam0k, yihuaf and me had on discord regarding the changes needed for making youki work with podman rootless.


For youki to be able to run in rootless way via podman, we need to (atleast) change the way we have implemented dbus interface. The reason for this is as follows :

When running under podman rootless, we need to connect to a socket at /run/user/(uid)/bus and then authenticate this socket to dbus using some kind of authentication mechanism. We need to access this socket path as root (because this is at /run, and thus would be a protected path(?)); however, when we authenticate with dbus, we must use the actual user id that has started the (non-root) podman process (i.e. 1000 etc)

Currently we use dbus-rs, which exposes rust binding to libdbus via ffi calls. Now libdbus provides one function to open a socket at given path, and another function to register the connection (which does all the authentication etc.) This creates a problem for us because we need to do both of these things using two different uids. (connect with uid 0, authenticate with actual uid) . Because libdbus (not dbus-rs) finds the effective uid (euid) internally , and then somehow stores euid across these two calls (probably storing it in a structure along with socket ; but not sure) we cannot change the it by doing seteuid call between these two steps.

On a side note, our current code does a Connection::session() https://github.com/containers/youki/blob/main/crates/libcgroups/src/systemd/manager.rs#L188 which is incorrect because it connects to the "default" dbus session bus, and we want to connect on the /run/user/(uid)/bus path. What runc does is that it uses two different libraries in go, one to connect to a socket and other to perform authentication. Both of these libraries have co-operating data-structures, so this can be done there.

The three ways to solve this are :

  • We don't support running rootless under podman :( We support rootful mode, but not rootless
  • We own everything about dbus communication in youki. That means we completely remove dbus-rs as a dependency, and implement manually the connection, authentication and everything else. Again, not the best way, because there will need to be a lot of code, we will need to be sure that it works correctly, and we will need to write a lot of tests to validate that.
  • We try switching to zbus. zbus is an alternate dbus library, but instead of ffi, it implements the authentication natively. Now, this still doesn't let us do the connection and the authentication separately, but I think we can ask them if we can add this method, and in worst case, fork and add code that we need ; keep maintaining it. This is again not a good solution, because maintaining a fork is a difficult task ; even apart from that we have to make a lot of changes to switch from dbus to zbus, as this file is generated specifically for dbus.

* if * we choose to move to zbus, I think a more sensible approach would be to first test in a branch if it will allow us to do what we want (separate connection and authentication) and then first move from dbus to zbus in one pr, and add the custom auth in separate pr. Still a lot of work, but ok.
All in all, none of the options I can think of are particularly good, so want to hear your opinion on them first. Feel free to ask how I found of things I have listed above if you think I might be wrong or missed something or have a better way.

Some Related links :
#1171 (comment)
diwic/dbus-rs#443
https://github.com/opencontainers/runc/blob/main/libcontainer/cgroups/systemd/user.go#L21

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Jul 26, 2023

I have been working on switching from dbus to zbus, and checking if youki can run rootless via podman.
Some caveats :

  • zbus is pretty bulky and the release compiled binary has an increase in size from 8.5 MB (dbus-rs) to 11 MB zbus)
  • zbus is async first, and provides sync support that is just a wrapper over async
  • The default feature async in zbus uses asyncio, which spawns threads in background. This is not acceptable to us, as we need to do unshare(userns) call , and that requires the calling process to not have threads. To make it work, we must disable default features and enable tokio feature, which works without spawning threads.

There are few other changes needed in non-dbus related code as well in order to correctly run youki rootless.

I have made a PR on my fork for

cc @utam0k @yihuaf

@yihuaf
Copy link
Collaborator

yihuaf commented Jul 31, 2023

@YJDoc2 Thank you for all the great work in putting this together and this is great!

The default feature async in zbus uses asyncio, which spawns threads in background. This is not acceptable to us, as we need to do unshare(userns) call , and that requires the calling process to not have threads.

I agree. In addition to the reason you stated, there are also a lot of undefined behaviors around forking a multi-threaded process without immediately exec into something else, which we have to do in youki.

The three ways to solve

I believe there is also a 4th way. We can implement just enough of dbus related logic in youki without forking zbus. We can look to zbus for ideas and borrow code, but we also don't need the entire zbus implementation. Our usecase for this is strictly around the dbus and cgroups. So the end result should be much simpler, and shedding 8+MB for binary size along the way. This would be a great improvement for us. In addition, getting rid of libdbus dependency by itself is a worthy optimization in my opinion.


Since it is clear we want to solve this issue one way or another (supporting rootless under podman is a very important requirement), I would suggest we can do the development in the main repo. We can wrap the experimentation code into a feature and don't compile by default, until we are ready. In this way, we don't have to support everything in a single big PR merge.

There are a few things I think will be helpful:

  • Fix the bug where we always connect to the default dbus session for rootful mode first. We should have a different issue tracking this.
  • Fix any other minor issue until we can reproduce the bug mentioned in this issue with the podman test.
  • Refactor the systemd related code to allow for a new feature that uses an alternative code path. I have not looked at the code, but I think you mentioned connect and authenticate being two places where we need to implement.

Feel free to break this into parts that can be worked on in parallel and let us know how we can help.

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Jul 31, 2023

I believe there is also a 4th way. We can implement just enough of dbus related logic in youki without forking zbus. We can look to zbus for ideas and borrow code, but we also don't need the entire zbus implementation. Our usecase for this is strictly around the dbus and cgroups. So the end result should be much simpler, and shedding 8+MB for binary size along the way. This would be a great improvement for us. In addition, getting rid of libdbus dependency by itself is a worthy optimization in my opinion.

This is what I meant in owning everything of dbus communication :)

I'm not sure if this work can be done behind a feature, but I'll check and do it that way if possible. What I'm thinking right now is that even apart from the dbus communication, there are several places that need changes for rootless to work properly with podman, as the logic we have done (from youki's side) is not correct. That can be changed without breaking anything, so in the WIP PR I have linked above, I'll first make it all work with zbus ; then port the non-dbus changes via a PR.

At that point, we will be ready to make prodman rootless work either with out own dbus communication or zbus, as needed. Then I'll work on the custom dbus communication implementation, and make PRs for that.

I'll try to think more on how I can I split this as per the three parts you have mentioned. wdyt?

@yihuaf
Copy link
Collaborator

yihuaf commented Jul 31, 2023

Sounds like a good plan :)

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Aug 10, 2023

@utam0k can you help me with understanding something? In our current code, we have a Rootless structure which we use. The conditions for constructing that here check that if a user namespace is given in spec and its path is None . However, in the rootless_required function here we only check if the current ID of user is 0 or not . If I'm correct, these conditions are not exactly same, as if we are root user, the spec might still contain the user namespace ; and if we are non root user , we must create rootless container even if spec does not have user ns (of course, we must check that in such case we already are in a user namesapce) .

So what I am confused about it iis the purpose of Rootless structure and its naming a bit misleading? Should it be named NewUserNs or something similar? Can you help me understanding this better? Thanks!

@utam0k
Copy link
Member

utam0k commented Aug 11, 2023

If I'm correct, these conditions are not exactly same, as if we are root user, the spec might still contain the user namespace ; and if we are non root user , we must create rootless container even if spec does not have user ns (of course, we must check that in such case we already are in a user namespace) .

Since I think you are right, NewUserNs or something sounds more accurate 👍

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Aug 19, 2023

@utam0k @yihuaf can I ask you to take a look at https://github.com/YJDoc2/dbus_native ? This does all the implementation of dbus interfacing code that is needed by youki in Rust native, without needing libdbus or other external dependencies (except nix crate). I'm planning to copy that code into youki and replace dbus-rs, and wanted to have it checked a bit before I make the PR. The code is almost final, except changing the Error type to youki-error compatible once I copy it over. There is also one small unsafe which covert string from a vec directly because we know its a valid utf-8 , but I can change that to safe version + unwrap when I copy over.

As a Note this does not implement the complete dbus functionality, just the part that is needed by youki. I have also tried to keep the proxy interface same, so we have minimal changes is our current client implementation.

Thanks :)

@utam0k
Copy link
Member

utam0k commented Aug 20, 2023

@YJDoc2 🎉 🎉 I'll take a look at your repository 👀

@utam0k
Copy link
Member

utam0k commented Aug 20, 2023

@YJDoc2 The overview of its design looks good to me. Let's start to develop in youki repository. By starting development in the youki repository, even if it is not fully worked, review, etc, can be done in smaller cycles.

@yihuaf
Copy link
Collaborator

yihuaf commented Aug 20, 2023

I agree with @utam0k. This is more than good enough to move into the youki repo and start to be developed. We can lock this behind a feature initially.

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Aug 21, 2023

Thank you @utam0k @yihuaf for taking a look! Once #2279 is updated and merged, I'll open a PR which will move and integrate this code into youki.
Also, I was thinking that as long as all current CI and tests pass, we can switch from dbus-rs to this implementation ; however, if you want to lock it behind a feature, that seems fine too. Can you tell me what other conditions/checks we will use for eventually moving this from a feature to default in future, so I can keep that in mind when moving the code? Thanks!

@utam0k
Copy link
Member

utam0k commented Aug 21, 2023

Can you tell me what other conditions/checks we will use for eventually moving this from a feature to default in future, so I can keep that in mind when moving the code? Thanks!

Why don't you ask @orimanabu to verify it, and if it's not a problem, then become default it?

This was referenced Sep 19, 2023
@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Oct 5, 2023

Completed via #2370 🎉

@YJDoc2 YJDoc2 closed this as completed Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants