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.

Sign up for GitHub

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

trying a different approach to requesting for admin privs #144

Merged
merged 1 commit into from
Feb 8, 2018
Merged

trying a different approach to requesting for admin privs #144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions commands/stop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@febbraro febbraro Feb 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

util.Command("sudo", "route", "-n", "delete", "-net", "172.17.0.0").Run()
util.Command("sudo", "route", "-n", "delete", "-net", "172.17.42.1").Run()
}
cmd.out.Info("Networking cleanup completed")

Expand Down
7 changes: 7 additions & 0 deletions util/shell_exec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ func Convert(cmd *exec.Cmd) Executor {
return Executor{cmd}
}

// EscalatePrivilege attempts to gain administrative privilege
// @todo identify administrative escallation on Windows.
// E.g., "runas", "/noprofile", "/user:Administrator
func EscalatePrivilege() error {
return Command("sudo", "-v").Run()
}

// PassthruCommand is similar to ForceStreamCommand in that it will issue all output
// regardless of verbose mode. Further, this version of the command captures the
// exit status of any executed command. This function is intended to simulate
Expand Down