trying a different approach to requesting for admin privs #144
Conversation
|
I like this, it's a good start. This probably still needs some light touches for spinner compatibility, and rather than complicate every spot that sudo comes into place, we might as well flag the command structs involved with an |
| @@ -62,8 +62,9 @@ func (cmd *Stop) StopOutrigger() error { | |||
| util.Command("runas", "/noprofile", "/user:Administrator", "route", "DELETE", "172.17.0.0").Run() | |||
| util.Command("runas", "/noprofile", "/user:Administrator", "route", "DELETE", "172.17.42.1").Run() | |||
| } else { | |||
| util.Command("sudo", "-s", "--", "'", "cat", "/dev/null", ";", "route", "-n", "delete", "-net", "172.17.0.0", "'").Run() | |||
| util.Command("sudo", "-s", "--", "'", "cat", "/dev/null", ";", "route", "-n", "delete", "-net", "172.17.42.1", "'").Run() | |||
| util.EscalatePrivilege() | |||
There was a problem hiding this comment.
Should we consider moving this to the command wrappers that check on whether a command is privileged? If we did that, it's universally the way we trigger for the password prompt if it's going to be needed.
There was a problem hiding this comment.
Or, if we request priv escalation (and it is granted), then we can remove the "sudo" on each of the commands themselves, which was my longer term rationale but I didnt; want to rewrite all of rig :)
Another thing I thought of was that a command could have as part of it;s config, the fact that i needs priv escalation and we can initiate that before we do anything to ensure the work can be done.
There was a problem hiding this comment.
Moving to privilege escalation on the rig process and dropping sudo from all the commands is likely a better long term approach. I agree that would require a fairly substantial adjustment of the codebase that I don't think we want/need to undertake at the moment.
Adding the flag about the need for privilege elevation would be an inherent need since the current mechanism relies on spotting sudo in the command. Do you foresee an elevation/release pattern wrapping commands which need it or are you thinking once privileges are elevated all subsequent commands just run with the elevation?
There was a problem hiding this comment.
I haven't quite thought it all the way through but my initial reaction is just request escalation around every block that needs it. the first will prompt for it and then subsequents will just just validate and will pass with no prompt if it is already escalated.
This raises an interesting question thought. If we have raised privs via sudo -v are only commands run with a sudo prefix done at that elevated level, or are all commands? meaning for these route deletes if a sudo -v has previously succeeded will the regular route command have the privs, or will it only need those privs if it is run via sudo.
There was a problem hiding this comment.
My expectation would be that subsequent commands need sudo unless we change so that we escalate privileges on the running rig process. We're not actually elevating privileges here, instead what we're really doing here is forcing a prompt for the password for sudo if we need it via a "safe" command that doesn't have ill interactions with the spinner.
Then we assume subsequent commands won't need the password prompt because they execute within a short enough time window. If someone configures sudo such that you need the password each time I expect we'd be getting prompted on every command but we'd only execute the "safe to not interfere with the spinner" version of getting prompted once.
There was a problem hiding this comment.
So do we want to get this in as a replacement for the cat /dev/null and work on the rest of it as we come across it?
There was a problem hiding this comment.
I'm ok with pushing this one on through. I don't think the things we've talked about here are critical enough to block it, especially since we don't know yet whether we'll even go any further on trying to pull sudo out of all the commands.
* Add explicit privilege prompt to improve sudo UX (#138) * Explicitly prompt for privilege escallation * Remove password prompt part of privilege message * Expand sudo detection. * Tidy up timing issues. * Consolidate messaging and avoid newline in verbose. * Cleanup ToString, sudo contains, cover more exec methods. * Lint does not catch all of fmt. * Remove unnecessary password prompt from networking cleanup. * Remove color reset and cat /dev/null to clear route text. * trying a different approach to requesting for admin privs (#144)
A different take on privs. This could be expanded later to wrap requests into blocks surrounded by a request to escalate privileges. For now I just cleans up the existing.