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@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.
>
> 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?
Regards,
Erik
