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

On Tue, Apr 25, 2023 at 10:40:45AM -0500, Jonathon Jongsma wrote: > 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. I see. Without trying it/playing with it myself, I don't have a better proposal at the moment, let me have a look at this tomorrow, I'll take your patch and see if anything I'm familiar with in argparse would work, if not or if you can figure out a better way in the meantime, I'll retract my comments to this patch, how about that?