Re: [libvirt PATCH] run: add ability to set selinux context

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&gt; > > --- > > 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.