Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Oliver Kiddle wrote on Wed, Aug 31, 2016 at 23:15:20 +0200:
> On 22 Jul, Daniel Shahaf wrote:
> > Secondly, you don't touch on what we would do when the 'gain-root' style
> > is unset.  Given Marko's later email that virt-admin is not usable by
> > non-root users, perhaps we should do this:
> 
> I'd suggest that it doesn't do anything special. Just run virsh
> list. Testing EUID might be correct but you can't actually be sure
> it won't work, perhaps due to groups or some RBAC mechanism. If it
> doesn't you'd end up with a message like:
>   No matches for: `domains'
> In some cases, _wanted -x should perhaps be used.
> 
> Putting the logic in _call_program also has an impact on this
> question because while virt-admin might be unusable to non-root
> users, some other commands might be partially usable. sudo may
> just allow it to get more matches.

All this makes sense.

However, your patch seems to implement something slightly different: the
patch executes sudo when (-p was passed, and) 'gain-privs' is either
unset or set to true, whereas the above paragraphs seem to be arguing
for *not* using sudo when the style is unset?  Perhaps I misunderstood
your argument.

I have no strong opinion regarding what gain-privs should default to
when unspecified.  

> How about something like the following? _sudo sets the _comp_priv_prefix
> variable to provide a prefix to match those for the current
> command-line. If _call_program is called with -p, it will default
> to using this prefix unless overridden, either by a gain-privs style
> or the command style with a - prefix.
> 

This approach also makes sense to me.  It does, however, require us
(completion file authors) to be *very* careful / disciplined with uses
of -p to avoid possibly executing parts of the command line as root.

Executing a command as sudo upon pressing <TAB> could have undesired
results in two ways: if the user _has_ sudo, executing the command with
sudo could surprise her; and if user _doesn't_ have sudo, executing the
command might result in an email from sudo to the root user.

This change might deserve a NEWS entry?

> This doesn't include an (( EUID )) check because it might be applicable
> where permissions other than root are gained.

I see: the privileges gained need not be privileges on the local system.

>
> It'd perhaps be useful if
> the -p option to _call_program also caused it to add something to
> $curcontext when looking up the command style but I'm not sure where
> that could be done as we already have the tag and argument fields
> filled. Any ideas?

I suppose it fits best in the "command" part of the context, e.g.,
.
    _f() { : $(_call_program lipsum echo lorem ipsum) }
    sudo f <TAB>
.
could look up :completion::complete:f-under-sudo::lipsum or so.
(Replace "f-under-sudo" by any string that identifies both 'f' and
'sudo' unambiguously.)

I don't think there's a compatibility problem here: if we add -p to an
existing _call_program invocation we should change the tag that
invocation uses, so might at the same time change the context the tag is
looked up in.

> Besides -u/--user, are other sudo options needed?
> 

All of these seem potentially needed:

-g -P    (whom to execute as)
-r -t    (on my system they're about SELinux; not sure about their meaning on other platforms)
-h       (where to execute)
-H -E -i (execution environment)

That's based on the sudo manpage in Debian stable, sudo 1.8.10.

Come to think of it, sudoers(5) allows per-command defaults, so if the
completion of the command 'foo' invokes «_call_program -b bar», the
sudoers(5) Defaults!foo clauses wouldn't fire.  Hopefully that will do
nothing worse than failing to find completion.

> Oliver
> 
> diff --git a/Completion/Base/Core/_main_complete b/Completion/Base/Core/_main_complete
> index 9c4cfac..c292ce7 100644
> --- a/Completion/Base/Core/_main_complete
> +++ b/Completion/Base/Core/_main_complete
> @@ -38,6 +38,7 @@ local func funcs ret=1 tmp _compskip format nm call match min max i num\
>        _saved_colors="$ZLS_COLORS" \
>        _saved_colors_set=${+ZLS_COLORS} \
>        _ambiguous_color=''
> +local -a _comp_priv_prefix

Could we please not introduce any more short opaque names?  "priv" is
ambiguous/unclear.  Perhaps _comp_gain_privs_prefix.  (Or perhaps the
style name should be "gain-privileges".)

> diff --git a/Completion/Unix/Command/_sudo b/Completion/Unix/Command/_sudo
> index 63ac37f..1ceefa2 100644
> --- a/Completion/Unix/Command/_sudo
> +++ b/Completion/Unix/Command/_sudo
> @@ -48,7 +48,7 @@ else
>      '(-H --set-home -i --login -s --shell -e --edit)'{-H,--set-home}"[set HOME variable to target user's home dir]" \
>      '(-P --preserve-groups -i -login -s --shell -e --edit)'{-P,--preserve-groups}"[preserve group vector instead of setting to target's]" \
>      '(-)1:command: _command_names -e'
> -    '*::arguments: _normal'
> +    '*::arguments:{ _comp_priv_prefix=( $words[1] -n ${(kv)opt_args[(I)(-u|--user)]} ) ; _normal }'

You removed the space that followed the last colon; wouldn't that cause
_arguments to try and exec the anonymous block with arguments?  I.e.,
"*::arguments:foo" executes «foo $expl», so wouldn't the above line
cause «{ ... ; _normal } $expl» to be executed?

Should $_comp_priv_prefix be left unset when (( ${+funcstack[(r)_ssh]} ))?

Thanks for taking care of this.

Cheers,

Daniel

>    )
>  fi
>  
>