From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28582 invoked by alias); 4 Sep 2016 04:13:05 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 39168 Received: (qmail 16201 invoked from network); 4 Sep 2016 04:13:05 -0000 X-Qmail-Scanner-Diagnostics: from out4-smtp.messagingengine.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1. Clear:RC:0(66.111.4.28):SA:0(0.0/5.0):. Processed in 1.14527 secs); 04 Sep 2016 04:13:05 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=0.0 required=5.0 tests=T_DKIM_INVALID autolearn=unavailable autolearn_force=no version=3.4.1 X-Envelope-From: d.s@daniel.shahaf.name X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | Received-SPF: none (ns1.primenet.com.au: domain at daniel.shahaf.name does not designate permitted sender hosts) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= daniel.shahaf.name; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=z0NRlkkXEFDYsMHt Cb2AvNp9xg4=; b=tHVzNNYfY3PImNWJ+dTflAB/HTsROpGRp7CZhpSxC6aGhij6 1Y9HMrLzTzMmSRSGmqrbR2/Qx+ohBda0OMRm/00GfGutyM3u9fYVp9VT62OqYCli ArSKilm60EEAs61fVH30DzRqJuKk9T4dPqCa3Hahdi1d/yE/5ThkbfBOqo8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-sasl-enc:x-sasl-enc; s=smtpout; bh=z0NRlkkXEFDYsMH tCb2AvNp9xg4=; b=EId01XriWGw30A6+ruxORe59NUiBWzEIUhW6UzypHgX83dr bb/C7TBNSiQBJfSe33M8ny4N5nz/XPhErVjqBLhqIgqyhBH3Uf+6fN14O9CGZ3Tp l1s63vB/FxaKfOTZZbJPywKkWr9NsJ9YsbFXuk+cw4OdIaHLtO3i55tHm4So= X-Sasl-enc: S1pKD7VYA9sg2V6H7GshILg5KHWJ2bLQBDMhzvzJiwFQ 1472961735 Date: Sun, 4 Sep 2016 04:01:42 +0000 From: Daniel Shahaf To: Oliver Kiddle Cc: zsh-workers@zsh.org Subject: Re: [PATCH] _virsh (Was: Re: zsh virsh completion) Message-ID: <20160904040142.GA5216@fujitsu.shahaf.local2> References: <8eb6dce0-50d7-5ab2-503a-194c1de2e45d@redhat.com> <20160713045957.GA3893@tarsus.local2> <9968da53-c1fd-fa2a-f30c-c74f884d2478@redhat.com> <20160720065832.GA28939@tarsus.local2> <699166a0-b0f0-452c-2561-b7e3cc952062@redhat.com> <25001.1469117569@hydra.kiddle.eu> <20160722071927.GG2521@tarsus.local2> <12554.1472678120@hydra.kiddle.eu> <20160902052342.GA8514@fujitsu.shahaf.local2> <66037.1472828529@hydra.kiddle.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <66037.1472828529@hydra.kiddle.eu> User-Agent: Mutt/1.5.23 (2014-03-12) 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 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 > 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 . , 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