9:43 a.m.
On 4/25/23 8:11 AM, Erik Skultety wrote:
On Mon, Apr 24, 2023 at 03:50:48PM -0500, Jonathon Jongsma wrote:
> When running libvirt from the build directory with the 'run' script, it
> will run as unconfined_t. This can result in unexpected behavior when
> selinux is enforcing due to the fact that the selinux policies are
> written assuming that libvirt is running with the
> system_u:system_r:virtd_t context. This patch adds a new --selinux
> option to the run script. When this option is specified, it will launch
> the specified binary using the 'runcon' utility to set its selinux
> context to the one mentioned above. Since this requires root privileges,
> setting the selinux context is not the default behavior and must be
> enabled with the command line switch.
A fiddled with writing custom selinux transition rules to achieve the same
thing a couple years back, but never finished it. No wonder, this is a much
cleaner approach.
I will only comment on the Python side of things, leaving the overall approach
and idea commenting to someone else.
>
> Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
> ---
> run.in | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 50 insertions(+), 6 deletions(-)
>
> diff --git a/run.in b/run.in
> index c6d3411082..4aa458b791 100644
> --- a/run.in
> +++ b/run.in
> @@ -40,6 +40,7 @@
> #
> # ----------------------------------------------------------------------
>
> +import argparse
> import os
> import os.path
> import random
> @@ -59,15 +60,20 @@ def prepend(env, varname, extradir):
>
> here = "@abs_builddir@"
>
> -if len(sys.argv) < 2:
> - print("syntax: %s BINARY [ARGS...]" % sys.argv[0], file=sys.stderr)
Since you decided to use argparse (yes please), we can drop ^this if we
properly set the arguments with argparse, it'll even generate the help for us.
This way it looks only like a partial solution. Argparse has great
documentation so you can just take one of the examples they list.
Yeah, I probably should have commented on why I used this 'partial'
approach. I tried a few different ways, including adding a positional
argument to argparse that would capture the target executable and its
arguments like so:
argsparse.add_argument("args",
nargs="+")
and then parsing with parser.parse_args() rather than
parse_known_args(). But that prevented me from passing arguments to the
target binary without inserting a '--' in to indicate that the run
script should stop parsing:
Fails:
# ./_build/run --selinux ./_build/src/libvirtd --verbose
usage: run [--selinux] args [args ...]
run: error: unrecognized arguments: --verbose
Works:
# ./_build/run --selinux -- ./_build/src/libvirtd --verbose
2023-04-25 14:26:32.175+0000: 1530463: info : libvirt version: 9.3.0
...
That seemed annoying to me.
Maybe we could keep using parse_known_args() with the 'args' argument
defined above, but I have a vague recollection that this caused some
other undesirable behavior so I switched back to the version I
submitted. I'll try to refresh my memory.
> +parser = argparse.ArgumentParser(add_help=False)
Why don't we want the automatic help?
Because then the run script would intercept the --help option and
prevent us from passing it to e.g. libvirt or virsh. Maybe that's not
something that we really care about, but I made the choice to pass as
much through to the executable as possible.
> +parser.add_argument('--selinux',
> + action='store_true',
> + help='Run in the appropriate selinux context')
> +
> +opts, args = parser.parse_known_args()
If you want to use ^this, then you need to be aware of prefix matching on the
options recognized by Argparse. IOW if one is to pass <args> to the <binary>
then none of the <args> can be a prefix of any of the long options argeparse
knows about (in this case --selinux), otherwise it'll consume it. Altough
unlikely, we should stay on the safe side and use:
argparse.ArgumentParser(..., allow_abbrev=False) [1]
[2] https://docs.python.org/3.11/library/argparse.html?highlight=argparse#all...
ok, I wasn't aware of that option.
> +
> +if len(args) < 1:
> + print("syntax: %s [--selinux] BINARY [ARGS...]" % sys.argv[0],
file=sys.stderr)
> sys.exit(1)
Same here, with argparse ^this is not needed if the args/options are defined
correctly.
>
> -prog = sys.argv[1]
> -args = sys.argv[1:]
> +prog = args[0]
argparse's parser obj has a 'prog' attribute [2].
[2] https://docs.python.org/3/library/argparse.html#prog
I think that's the wrong 'prog' though. That would give me the './run'
script, whereas I want the 'libvirtd' program (or whatever) that the
user wants to execute.
The rest looks good from Python POV, but like I said, although
I'm up for this
idea, I'll let someone else comment on that.
Regards,
Erik