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

[Jonathon Jongsma]

On 4/25/23 11:54 AM, Erik Skultety wrote:
> 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 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.
> 
> 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?

I did play around a little more and couldn't immediately find a better 
way to do it. If you find a better approach, I'm happy to consider it. I 
didn't initially consider using something like a a subparser (as you 
mentioned in a different email) because that would change the way that 
the script is invoked and I didn't want to disrupt others' development 
workflow. But if people are OK with that, then that obviously opens up 
more options.

Jonathon