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

On 4/25/23 9:43 AM, Jonathon Jongsma wrote: > 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. So here's one case that becomes more annoying when using the above setup (the new 'args' catchall argument combined with parse_known_args()): ./run gdb -- libvirtd --verbose gdb: unrecognized option '--verbose' the ./run script eats the '--' that should go to gdb, so gdb tries to interpret the --verbose option rather than passing it on to libvirtd.