search

outbrain / orchestrator-agent

MySQL replication topology manager - agent (daemon)
Apache License 2.0
35 stars 53 forks source link

Mark commands that need elevated privileges explicitly #9

Closed grierj closed 8 years ago

grierj commented 9 years ago

This is beneficial for whitelisting sudo commands. The previous setup required sudo to allow "bash" which is a privilege escalation. This way allows us to whitelist specific commands that need escalation and run non-privileged commands without sudo.

Commands that are configured via the config file don't need to be marked with sudoCmd since you can specify sudo in the configuration

shlomi-noach commented 9 years ago

What is the criteria for your selection of functions for which to apply the sudoCmd()? For example, RemoveLV is included but MountLV is not.

grierj commented 9 years ago

An oversight on my part. Anything that should require a root user to run should have the wrapper. I'll fix it shortly.

grierj commented 9 years ago

I did another pass over the functions, looks like MountLV has sudoCmd here: https://github.com/grierj/orchestrator-agent/blob/f7bc815ced528d36a279c82cb71bb2e9a3b3f33b/src/github.com/outbrain/orchestrator-agent/osagent/osagent.go#L240

It's a little buried in the function. Should I break out the sudoCmd modification from being inlined in the commandOutput function? That might make things less clustered together.

So outside of you pointing me at that, I can't find any others that I think would need to run at elevated privilege, sorry if I'm totally missing something.

I did find that blkid runs without sudo, however a little digging suggest that you want to run it as root when possible: "Note that blkid reads information directly from devices and for non-root users it returns cached unverified information."

shlomi-noach commented 8 years ago

I agree that blkid should run with sudo. Ack on the MountLV buried sudoCmd. This seems good to me; I don't feel there's a need for uncluttering, but of course I'm always happy to get less clutter