-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Disable drivers based on whether nomad is running as root. #89
Conversation
@@ -52,6 +54,12 @@ func NewQemuDriver(ctx *DriverContext) Driver { | |||
} | |||
|
|||
func (d *QemuDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, error) { | |||
// Only enable if we are root. This check also disables on Windows as | |||
// Geteuid() returns -1. | |||
if syscall.Geteuid() != 0 { |
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.
I think Windows supports QEMU so we should have the if runtime.GOOS != "windows"
check here too.
@@ -7,6 +7,8 @@ import ( | |||
"github.com/hashicorp/nomad/nomad/mock" | |||
"github.com/hashicorp/nomad/nomad/structs" | |||
"github.com/hashicorp/nomad/testutil" | |||
|
|||
clientTestUtil "github.com/hashicorp/nomad/client/testutil" |
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.
Weird for the package to have uppercasing. Just do ctestutil or something
👍 |
Disable drivers based on whether nomad is running as root.
@dadgar @cbednarski This makes sense in most cases, but I am wondering about the getting started guide and other super-basic use cases. It was pretty helpful for me to be able to just use the exec driver as it's the lowest common denominator and I could coax it into doing whatever I wanted on my local machine, but now I would need to run Nomad as root locally on my laptop, even in dev mode. I may have missed some slack backlog somewhere, but I don't understand why drivers such as exec would make a decision such that "oh, you aren't root? then you can't exec anything". Operators might want to force Nomad clients down into a super unprivileged user anyways, using the OS as the isolation mechanism and only allowing binding on certain ports, access to certain paths on the filesystem, etc. rather than depending on Nomad to do the right thing for them. For those cases you are kind of SOL, when you see the "this has to run as root" message, and I'm guessing the security crowd would have a field day with that. Just thinking out loud here - let me know if I missed anything. |
@ryanuber I agree completely. Dev / test / demo QOL is poorer with the root requirement. However, I think we can resolve this with additional time after 0.1. The tradeoff we're making now is developer convenience vs. production readiness and doing actual scheduling (i.e. resource constraints). As @dadgar posted out we need root for various features like cgroups, chroot, and mount, which are key components of nomad's isolation functionality. Even something as simple as setuid so nomad can fork/exec as "nobody" requires root permissions. FWIW docker has to run as root for the same reasons. I don't think it's too bad of a requirement for running nomad even though the dev workflow has some extra hoops for now. |
Enterprise/2019 29 jan ossmerger
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
The isolation capabilities of drivers other than "docker" go down dramatically if nomad is not running as root. Driver fingerprinters now check for this case and disable themselves.