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

[Jonathon Jongsma]

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 at 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#allow-abbrev

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
>