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. > 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 >