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

Make nsenter in reboot command optional #896

Closed
bh185140 opened this issue Feb 8, 2024 · 9 comments
Closed

Make nsenter in reboot command optional #896

bh185140 opened this issue Feb 8, 2024 · 9 comments

Comments

@bh185140
Copy link

bh185140 commented Feb 8, 2024

Following #814 which added signal based reboot we could reduce the privilege of the kured pods. With this we could also look into reducing the privileges for the reboot command method which currently is limited by the need to enter the host namespace.

If nsenter was made optional or configurable, this allows a lot more flexibility in setting up reboot methods that are more secure. We can imitate signal reboot by sending signals as a command through /bin/kill. This can also indirectly solve #868 as we could also make the reboot command touch a reboot file.

@ckotzbauer
Copy link
Member

But this only would work, if you can use binaries which are already inside the kured-image right? So maybe you use a custom-image?

@bh185140
Copy link
Author

But this only would work, if you can use binaries which are already inside the kured-image right? So maybe you use a custom-image?

Yes, for the use cases I've listed

  • systemd shutdown through signal can be done with /bin/kill already in the image
  • path-based reboot mechanism #868 can be done with /bin/touch also already in the image on a mounted volume

I think it should be fairly small change that won't break any thing. Mainly need it for the reduced privileges as I can't use the new reboot signal methods since I need to run a small shell script in the command before hand.

@bh185140
Copy link
Author

Main driver for this is mostly I needed a mechanism to trigger shutdowns as well as reboot. I could open up a separate issue for it I suppose, not sure if there would be enough interest in supporting shutdown on a reboot daemon. Though this could be a smaller change to go in.

@ckotzbauer
Copy link
Member

Thanks for the explanations. Yes, both binaries are already present. For the path-based method we could just create a file with go-code at a configurable, mounted location. Shutdowns are not directly in scope of kured, I think.

@bh185140
Copy link
Author

Happy to open up a PR for this if you think this change could go into Kured.
I see currently nsenter is done with /usr/bin/nsenter", fmt.Sprintf("-m/proc/%d/ns/mnt", pid), "-- where the pid is a constant 1. Can possibly replace it with a default argument --reboot-command-prefix="/usr/bin/nsenter -m/proc/1/ns/mnt --". Alternatively just make it boolean parameter on where or not we add nsenter.
What do you think?

@bh185140
Copy link
Author

Raised a PR here if interested https://github.com/kubereboot/kured/pull/899/files

// PID set to 1, until we have a better discovery mechanism.

Saw this comment while making this. Could be another use case to support changing the PID as a configuration for rancher.

@bh185140
Copy link
Author

bh185140 commented Apr 4, 2024

@ckotzbauer Gentle bump for this thread

@ckotzbauer
Copy link
Member

Thanks for the hint, I did a review.

Copy link

github-actions bot commented Jun 4, 2024

This issue was automatically considered stale due to lack of activity. Please update it and/or join our slack channels to promote it, before it automatically closes (in 7 days).

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants