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

Tue Apr 25 15:40:45 UTC 2023

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

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.

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