On Tue, Apr 25, 2023 at 09:43:38AM -0500, 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@xxxxxxxxxx>
> > > ---
> > > 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.
Right, good point. Without trying it myself, could we utilize subparsers for
this? IOW, have:
./run binary <binary> <args>
where 'binary' is a keyword arg for the run script. Since --help is
positional in argparse when you use subparsers, we could have the main parser
with help, but the 'binary' subparser without it - that way we could
theoretically pass --help to whatever binary we actually want the run script to
wrap. Like I said though, I haven't tried ^this so I'm just tossing in whatever
I just made up in my head.
>
> >
> > > +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.
Sorry, I completely overlooked the lack of 'sys.' in there, you're absolutely
right.
Erik
