From: Daniel Shahaf <d.s@daniel.shahaf.name>
To: Oliver Kiddle <okiddle@yahoo.co.uk>
Cc: zsh-workers@zsh.org
Subject: Re: [PATCH] _virsh (Was: Re: zsh virsh completion)
Date: Fri, 2 Sep 2016 05:23:42 +0000 [thread overview]
Message-ID: <20160902052342.GA8514@fujitsu.shahaf.local2> (raw)
In-Reply-To: <12554.1472678120@hydra.kiddle.eu>
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
>
>
next prev parent reply other threads:[~2016-09-02 5:24 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-11 11:52 zsh virsh completion Marko Myllynen
2016-07-11 15:03 ` Roman Neuhauser
2016-07-11 16:40 ` Bart Schaefer
2016-07-11 22:07 ` Oliver Kiddle
2016-07-12 10:23 ` Marko Myllynen
2016-07-13 4:59 ` Daniel Shahaf
2016-07-18 12:06 ` Marko Myllynen
2016-07-20 6:58 ` [PATCH] _virsh (Was: Re: zsh virsh completion) Daniel Shahaf
2016-07-20 8:36 ` Marko Myllynen
2016-07-21 6:57 ` Daniel Shahaf
2016-07-21 12:32 ` Marko Myllynen
2016-07-22 6:30 ` Daniel Shahaf
2016-07-22 8:17 ` Marko Myllynen
2016-07-21 16:12 ` Oliver Kiddle
2016-07-21 16:19 ` Marko Myllynen
2016-07-22 7:19 ` Daniel Shahaf
2016-08-31 21:15 ` Oliver Kiddle
2016-09-02 5:23 ` Daniel Shahaf [this message]
2016-09-02 15:02 ` Oliver Kiddle
2016-09-04 4:01 ` Daniel Shahaf
2016-09-07 6:39 ` Bart Schaefer
2016-09-09 22:09 ` Oliver Kiddle
2016-09-11 9:08 ` Daniel Shahaf
2016-09-14 23:19 ` Oliver Kiddle
2016-09-04 21:24 ` Daniel Shahaf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160902052342.GA8514@fujitsu.shahaf.local2 \
--to=d.s@daniel.shahaf.name \
--cc=okiddle@yahoo.co.uk \
--cc=zsh-workers@zsh.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).