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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.


+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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux