ev3dev / brickstrap

Tool for bootstrapping Debian and creating bootable image files for embedded systems
MIT License
35 stars 26 forks source link

Introduce unshare-harder variant of brickstrap.sh. #14

Closed cmacq2 closed 8 years ago

cmacq2 commented 8 years ago

This is Debian specific fallback version which will first prod the kernel into allowing ordinary users to unshare, as this is a necessary for user-unshare.pl on recent-ish Debian. Any changes to original kernel settings are reverted on exit. This workaround avoids having to run the entire brickstrap with sudo/su or to change the default unshare policy permanently. Only the initial prodding and reverting need sudo/su privileges.

By request the unshare-harder variant was split off into a PR of its own, from #12 Continuing the comment thread:

cmacq2 commented 8 years ago

This is a somewhat more polished effort based on the first:

dlech commented 8 years ago

I appreciate the idea here, but I'm still not keen on including this.

I've added a check for this in brickstrap.sh and added a bit to the readme in https://github.com/ev3dev/brickstrap/commit/f5b9bf0621b1aab8e66086c3c671428cbb12f0de.

The two things I don't care for are:

  1. Since brickstrap is generally long running, it is highly likely that not only will you have to enter your password when it starts, you will also have to enter your password again when it ends, so if you run brickstrap more than once, this is not really saving you time vs. manually running sudo sysctl -w kernel.unprivileged_userns_clone=1 and sudo sysctl -w kernel.unprivileged_userns_clone=0 (and you can copy and paste from the brickstrap error message if you forget what the command is).
  2. It hides what is actually be done from the user. Since this is a security issue, the user should know exactly what setting he/she is changing.
cmacq2 commented 8 years ago

Fair enough. I don't really appreciate the security issue, in that there's functionally no difference between the user manually running the commands or manually using a fallback script that does it for them. There's something to be said for not doing more than necessary though, in case we get it 'wrong'.

So let's abandon this PR and consider the core issue solved/fixed.

By the way I've left a small review note on your commit.