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

[Erik Skultety]

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.

> +parser = argparse.ArgumentParser(add_help=False)

Why don't we want the automatic help?

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

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

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