choco icon indicating copy to clipboard operation
choco copied to clipboard

Do not require elevation for informative functions for Admin in non-admin shell

Open majkinetor opened this issue 7 years ago • 34 comments

Since version 0.10.4 choco by default "Run with highestAvailable Execution Level by default". This can be disabled via manifest.

However, its ideal for this to be dynamic. The elevation should happen on install, update or uninstall and not on list, info, version and other informative or config functions.

I propose the very easy method to fix this: by changing the cinst, cup and cuninst wrappers to switch to higestAvailable level and restore it back after the run, so that user can set manifest to asInvoker. The non-wrapped commands would still elevate (or not, as per settings) on each command, but users will have an option this way.

With this in place it could be considered to set defaults to asInvoker.

majkinetor avatar May 22 '17 23:05 majkinetor

HighestAvailable Execution (the new setting from #1054) does a good job of only asking for elevation when an administrator is not in an administrative command shell, and only in that scenario. Everything else just continues to work as it normally does.

To be sure it is clear how elevation works in highestAvailable, let's understand the scenarios when elevation will be requested:

  • [ ] Admin account in elevated shell
  • [ ] Non-administrative account
  • [ ] SYSTEM account
  • [ ] Built-in administrator account on Server SKUs
  • [ x ] Administrator in an non-elevated shell

What about existing automation? Does is need to be changed for highestAvailable?

Most likely all existing automation is likely to already be okay with highestAvailable anyway based on the scenarios above. Most likely you are either using SYSTEM, a non-admin account, or using an admin with an administrative shell so it doesn't get hit by UAC for other things anyway.

Details on how this feature could be implemented per command

Assuming that one can determine when it should request for elevation based on all of the rules and constraints, doing so per command tends to become quite a bit more complex. It's why for #1054 we simply just exposed the app.manifest and set things appropriately, allowing a user to make that decision on what works best for them. But it is an all or nothing kind of approach.

One way to think of how the rules would work to elevate if this was moved forward. Elevate only if all of the following are true:

  • User is an administrator in a non-elevated shell
  • Not using background mode
  • Install is in original locked down location
  • We are not calling list, search, info, or push
  • User is not calling pack from a locked down location
  • Other things I will likely remember as this discussion continues...

Updated as conversation continues...

ferventcoder avatar May 23 '17 03:05 ferventcoder

@majkinetor and what about choco <command>? This is the primary way to interact with choco. An acceptable solution IMHO would work with this.

ferventcoder avatar May 23 '17 03:05 ferventcoder

The original issue that has changed this, along with the current way to change functionality back: https://github.com/chocolatey/choco/issues/1054

ferventcoder avatar May 23 '17 04:05 ferventcoder

This has been asked to be changed by more than 3 folks, however it is not a simple functionality to implement. It's typically one or the other, but not both. The reason the manifest was pulled out of choco.exe and set beside it was to allow for folks to choose what is best for them, but we set to highestAvailable by default.

ferventcoder avatar May 23 '17 04:05 ferventcoder

The only way I could see to do this is to set wrappers appropriately and have the shim inspect the command being passed to choco to decide if it should change the value.

Note that a non-administrator is not going to have permissions to change the file, and an administrator without UAC also will not have privileges. So either way you get a UAC popup if the application attempts to do so. We won't reduce the security of the files themselves. However we are open to finding the best options here.

ferventcoder avatar May 23 '17 04:05 ferventcoder

yeah, i think the idea of elevating just for those commands that require it is good. So if I ran choco install then it would prompt, but if I ran choco list -lo then it would not prompt and just run within the current command window (and as a bonus the results wouldn't disappear when it finished).

flcdrg avatar May 23 '17 04:05 flcdrg

@flcdrg I agree it would be beneficial. What I'm hoping to get out of this discussion are steps forward to implement this behavior in a way that is technically possible and feasible.

ferventcoder avatar May 23 '17 05:05 ferventcoder

@majkinetor also, you mentioned this in https://github.com/chocolatey/choco/issues/912#issuecomment-240172306:

You still need to open elevated prompt. For example my team never works in elevated one and in 95% of the cases choco install fails, then everybody remembers "yeah, admin". So its way inconvenient to do so and fully disabling UAC isn't recommended. Perhaps if choco automatically elevate like normal installers do this feature wouldn't be needed !!!

So #1054 fixed that problem, but in doing so it gave you a choice of choosing one way or another.

ferventcoder avatar May 23 '17 05:05 ferventcoder

So would an approach like https://stackoverflow.com/a/10905713/25702 be suitable? Or would you need to launch a child process elevated and wait for that to finish before the parent process exited?

flcdrg avatar May 23 '17 05:05 flcdrg

@flcdrg That is basically how ChocolateyGUI works. Should in theory be simpler to implement for choco. Edit: specifically shelling out to an independently elevated process. Definitely don't need any of Gui's RPC madness :P

Edit: Edit: Here be dragons warning. Assuming you just run pass args straight to an elevated choco process, you might still run into issues as I believe there's some quirks when redirecting stdout from an elevated process to an unelevated one on windows.

Edit: Edit: Edit: Go upvote https://github.com/PowerShell/PowerShell/issues/3232 so we get sudo and stuff like this isn't as painful in the first place :)

RichiCoder1 avatar May 23 '17 05:05 RichiCoder1

I'm guessing the parent would need to wait for the child so that things continued to work as expected when you're calling choco in a script and expect execution to happen synchronously.

flcdrg avatar May 23 '17 05:05 flcdrg

Yup. A simplish idea would to have the choco process check whether it's A) in an elevated process and B) whether the command in question is non-informative (or destructive as I call it). If no the to the former and yes to the latter, spin up a new process with runas, pass the appriopriate parameters verbatim (with maybe some additional ones so that the process knows it's a child), pipe the stdout and stderr back, and then just wait for the child process to finish executing (and set some arbitrary long timeout).

RichiCoder1 avatar May 23 '17 05:05 RichiCoder1

Keep in mind that not always would choco elevate - it depends on some factors of where the Chocolatey installation is, whether or not background mode is enabled or not (which allows non-admins to call choco install and have it redirected through a background service).

HighestAvailable does a good job of only asking for that elevation when an administrator is not in an administrative command shell, and that's exactly how it should behave otherwise.

Assuming that one can determine when it should request for elevation based on all of the rules and constraints, this issue gets to be quite a bit more complex. It's why for #1054 we simply just exposed the app.manifest and set things appropriately, allowing a user to make that decision on what works best for them.

Details on how this feature could be implemented

One way to think of how the rules would work to elevate if this was moved forward. Elevate only if all of the following are true:

  • User is an administrator in a non-elevated shell
  • Not using background mode
  • Install is in original locked down location
  • We are not calling list, search, info, or push
  • User is not calling pack from a locked down location
  • Other things I will likely remember as this discussion continues...

ferventcoder avatar May 23 '17 05:05 ferventcoder

  • We are not calling list, search, info, or push

and new, version

@majkinetor and what about choco ? This is the primary way to interact with choco.

I noted that The non-wrapped commands would still elevate. Clearly suboptimal but way better then nothing.

Note that a non-administrator is not going to have permissions to change the file, and an administrator without UAC also will not have privileges.

Yeah, forgot about this, so wrapper commands idea can't be used the way I describe it, the wrappers wold need to elevate themselves which is similar to what @RichiCoder1 mentions at https://github.com/chocolatey/choco/issues/1307#issuecomment-303296446

majkinetor avatar May 23 '17 07:05 majkinetor

and new, version

👍 And any others like download, support, outdated, source list (but not source change, or other config views).

Just of note, version has been deprecated and will be removed.

I noted that The non-wrapped commands would still elevate. Clearly suboptimal but way better then nothing.

This would be surprising behavior, which means it is unlikely to be added.

Likely the only way it could be done is to run a process to elevate when the right rules are in place.

ferventcoder avatar May 23 '17 13:05 ferventcoder

Likely the only way it could be done is to run a process to elevate when the right rules are in place.

And by that, each command already knows whether it should run with elevated context (although a bit naive) or not, so if we can find a good way to elevate first, then I'm good with pursuing this.

I do prefer asInvoker, but I also want choco to elevate automatically. However I do want to ensure the solution is feasible and elegant.

ferventcoder avatar May 23 '17 13:05 ferventcoder

This may sound a bit naive, but following could work:

All commands elevate as soon as they are called, but there might be a switch --dontelevate. With this switch, a user could use informative functions that do not require administrative privileges such as list, info ...

With this, all existing/out-there automation around choco would still work, and I wouldn't need to change a bit at work :joy:

mwallner avatar May 23 '17 16:05 mwallner

With this, all existing/out-there automation around choco would still work, and I wouldn't need to change a bit at work

I'm going to assume all existing automation is likely to already be okay with highestAvailable anyway based on the scenarios I laid out above where elevation is requested (https://github.com/chocolatey/choco/issues/1307#issuecomment-303284220). Most likely you are either using SYSTEM, a non-admin account, or using an admin with an administrative shell so it doesn't get hit by UAC for other things anyway.

ferventcoder avatar May 23 '17 18:05 ferventcoder

Most likely you are either using SYSTEM, a non-admin account, or using an admin with an administrative shell so it doesn't get hit by UAC for other things anyway.

Agreed.

mwallner avatar May 23 '17 18:05 mwallner

I'm going to assume all existing automation is likely to already be okay with highestAvailable anyway

My specific automation involves a scheduled task configured to run in the context of the interactive user, who usually is a member of Administrators. The task essentially invokes choco outdated, so it does not need and should not be elevated. This got broken in 0.10.4 (stops on the UAC prompt).

jberezanski avatar May 24 '17 21:05 jberezanski

"likely" ;)

ferventcoder avatar May 24 '17 22:05 ferventcoder

@jberezanski We also had a conversation about this and agreed it was a reasonable compromise at https://github.com/chocolatey/choco/issues/1054#issuecomment-288319580 - although it occurs to me that you may just be reiterating that conversation here.

ferventcoder avatar May 24 '17 22:05 ferventcoder

@ferventcoder Well, back then I wrote "Hopefully one day Choco will get smart enough to ask for elevation only for actions which require it." - that's why I'm declaring my support for this issue. Also, since that time I've realized this affects me not only in interactive, ad-hoc usage, but also in the automation scenario described above.

I like the idea outlined by @RichiCoder1. Redirecting input/output from the child process will not be trivial, however, because the typical way to elevate (Process.Start with UseShellExecute=true and Verb="runas") does not support stdin/stdout/stderr redirection, so it will require implementing a custom communication mechanism, using e.g. named pipes (a problem already solved by various installer frameworks, by the way).

jberezanski avatar May 25 '17 19:05 jberezanski

@jberezanski If you have specific examples for implementation or links, that would be most helpful. Whatever option it is has to be rock solid as we would have a very low tolerance for failures, plus it would need to be able to work in hundreds of diverse Windows environments. As long as those things can be accomplished, I'm 👍.

I do know Windows Installer (msiexec) has this figured out and elevates automatically later in the process of installing.

If named pipes is a way forward, keep in mind we are doing something like this with the Chocolatey Agent already and have seen it run into conflicts with other tools also using named pipes (having an elevated process means all listeners using named pipes have to run elevated). Example: "choco agent is running as a service and uses a net.pipe listener, that listener runs in an elevated security context. So ANY application that leverages the net.pipe listener will ALSO have to run in an elevated security context."

We have not determined the full validity of this aspect yet, but this could have implications running both choco.exe and the chocolatey agent service, as choco.exe would be a non-elevated listener. So we'd need to determine if it is feasible first.

ferventcoder avatar May 25 '17 19:05 ferventcoder

We have not determined the full validity of this aspect yet, but this could have implications running both choco.exe and the chocolatey agent service, as choco.exe would be a non-elevated listener. So we'd need to determine if it is feasible first.

I'm going to know how this turns out pretty quickly as we get the next version of the ChocolateyGUI working as it just moved to named pipes.

ferventcoder avatar May 25 '17 19:05 ferventcoder

No, I don't have any specific implementation examples to show (yet). I had done IPC over named pipes in .NET 2.0, but it was over 10 years ago and the code was proprietary anyway.

The problems you describe with the Chocolatey Agent surprise me. At present job, I have a production application consisting of multiple executables, some of them expose WCF endpoints over net.pipe. One exec is a NT service which runs as LocalSystem, another NT service runs as a low-privileged account (a virtual service account, to be precise), yet another exec is a worker process started on-demand under various security contexts. There have never been any conflicts between them (or with third-party apps) with regard to named pipe listeners (but care is taken to ensure uniqueness of pipe names). The application runs on OSes ranging from newest Win10 to XP.

In this case, however, I wouldn't use the messaging infrastructure of WCF/net.pipe, just simple text over NamedPipeServerStream/NamedPipeClientStream. The pipe names should be autogenerated (guids, for instance) and passed to the child process as command line arguments. The pipe creator (the low-priv parent process) should set the appropriate access rights to maximize security (the exact set is to be determined). I'll think about it some more and probably write a POC.

jberezanski avatar May 25 '17 20:05 jberezanski

There have never been any conflicts between them (or with third-party apps) with regard to named pipe listeners (but care is taken to ensure uniqueness of pipe names). The application runs on OSes ranging from newest Win10 to XP.

Surprising for us as well as the name isn't shared.

ferventcoder avatar May 25 '17 22:05 ferventcoder

GUI actually uses WCF over named pipes to do IPC to it's subprocess, and I don't believe we've run into any pipe security issues with GUI being no being elevated and Subprocess being elevated. It must be a configurable knob,

RichiCoder1 avatar May 26 '17 01:05 RichiCoder1

@RichiCoder1 the report we had was about listeners, so it would only be the GUI, not the subprocess. Again, we haven't had time to dive in to really investigate just yet.

ferventcoder avatar May 26 '17 19:05 ferventcoder

Maybe the right answer is to go back to "asInvoker" until we can find a good answer here?

ferventcoder avatar Jun 04 '17 10:06 ferventcoder