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

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Oliver Kiddle wrote on Fri, Sep 02, 2016 at 17:02:09 +0200:
> Daniel Shahaf wrote:
> > 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.
> 
> The code is what I intended.
> 
> It also requires sudo to be present on the command-line being typed
> so it seemed reasonable. And it is consistent with the remote-access
> style.  I don't have a strong opinion either, however. It was the
> unconditionally hard-coded sudo that I did have a strong opinion
> on.
> 
> > 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.
> 
> If a user does:
>   sudo virsh start --domain <tab>
> then yes, it might send off a mail to root but they'd get that anyway
> when they actually try to run the command they're composing.
> 

Well, we shouldn't spam the root user.  In particular, I'm concerned
about several completion attempts in quick succession (pretty common,
both when typing in a command and when using completion as a man page
/ reference) causing several sudo emails in quick succession.  Sysadmins
tend to be jumpy about this sort of thing.

Perhaps using sudo should be opt-in on the part of the user; that is:
when gain-privileges is unset, either behave as though it's set to
false, or perhaps prompt the user (via the minibuffer?) for permission
to use sudo.  (And cache the user's reply for next time?)

Just thinking out loud here.

> > > 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.
> 
> Or it might be something else like switch to the httpd user to run the
> command.
> 

It might be, yes, but how is this an example of why testing EUID is not
appropriate?  If a completion function needs to run a command as httpd,
then it has every reason to test EUID:
.
    case $EUID in
    (`id -u httpd`) _call_program $desc $args;;
    (`id -u  root`) (UID=`id -u httpd` && _call_program $desc $args);;
                (*) _call_program -p $desc $args;;
    esac

By the way: assigning to UID doesn't set $? :
.
    % UID=42; echo $?
    zsh: failed to change user ID: operation not permitted
    0

> > > 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.,
> 
> > could look up :completion::complete:f-under-sudo::lipsum or so.
> 
> Perhaps f/sudo? / can't appear in the command name.
> 

+1, sounds good.

How do we handle the case that >1 precommand wants to set _comp_priv_prefix?
For example, suppose ssh learnt to set $_comp_priv_prefix (I'm not
proposing to teach it to do so; this is just for the sake of example),
and somebody completed
.
    ssh -t $host sudo virt-admin <TAB>
.
, would _comp_priv_prefix then be set to (ssh -t $host sudo) and would the
style context be f/sudo/ssh?

> > 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.
> 
> At worst, the command will be blocked with a root e-mail. User will need
> to set the command or gain-privileges style.
> 
> I also wonder if the prefix perhaps has other uses. With env for
> instance.
> 

You can set the 'command' style to an anonymous function; I'm sure
people have found creative uses for this.

E.g., poor man's profiling:
% zstyle \* command '-() { typeset -F SECONDS; { "$@" } always { print -ru 10 -- "$SECONDS" } }'

Perhaps it's possible to use a prefix to arrange for the command to run
asynchronously.

> > > +    '*::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
> 
> The { … } form is a specifically handled form by _arguments and it
> won't insert any arguments. So I think this should be fine.
> 

My bad — I missed the fact that the {} form of a spec's action is
documented because I had misgrepped the man page.

> I felt that _comp_priv_prefix itself ought to be documented but
> there wasn't an obvious place to put it. I could try to squeeze it
> in under gain-privileges but is there any thoughts on adding a
> section on standard variables as follows?
>

I don't have a strong opinion on this question.

> Zsh convention would probably be to use "parameters" instead of
> "variables" and I'll do that if anyone objects but we might be doing
> our user's a favour by ditching that particular convention.

+1 to "variables".

> +++ b/Doc/Zsh/compsys.yo
> @@ -1815,6 +1815,14 @@ In the case of the tt(_match) completer, the style may also be set to
> @@ -4020,6 +4022,13 @@ execute.  The var(string)s from the call to tt(_call_program), or from the
> +If the option `tt(-p)' is supplied it indicates that the command output
> +is influenced by the permissions it is run with. Unless disabled with
> +the tt(gain-privileges) style or overidden with the tt(command) style,

Typo: "overridden"

Also, 'command' only overrides 'gain-privileges' if the former's value
starts with a hyphen.

> +tt(_call_program) will make use of commands such as tt(sudo) if
> +present on the command-line, to match the permissions to whatever the
> +final command is likely to run under.

Style: suggest to strike "to whatever".

> @@ -4875,7 +4884,38 @@ the same meaning as for tt(_description).
> +item(tt(_comp_caller_options))(
> +The completion system uses tt(setopt) to set a number of options. This
> +allows functions to be written without concern for compatibility with
> +every possible combination of user options. However, sometimes completion
> +needs to know what the user's option preferences are. These are saved
> +in the tt(_comp_caller_options) associative array.

What are the keys and values of the array?  Suggested text:

    The array maps option names, spelled in lowercase without
    underscores, to the string `tt(on)' or `tt(off)'.

It wouldn't be correct to say 'the keys are the same as those of
$options' because keys with underscores or mixed case work in $options
but not int $_comp_caller_options.

> +item(tt(_comp_priv_prefix))(
> +Completion functions such as tt(_sudo) can set the tt(_comp_priv_prefix)
> +array to a command that may then be used by tt(_call_program) to
> +match the permissions when calling programs to generate matches.

Suggest to change "command" to "prefix command" (assuming that's
a well-known term for "a command that does execve(argv+1)", such as env,
nohup, etc.) to make it clear to authors of functions such as _doas what
they should set $_comp_priv_prefix to.

Also, see above about f/ssh/sudo; if we accept that use-case then the
docs here might say the array should be appended to, not clobbered.

> 

While we're about _call_program, it inspects the variable $debug_fd.
That variable is set by _complete_debug but nothing in the normal
completion codepath unsets it, so if the user happens to have a variable
by that name, then _call_program would use it.

I assume $debug_fd needs to be either documented or unset in
_main_complete or renamed as _comp_*.

Cheers,

Daniel