[Libguestfs] [common PATCH 3/3] mlcustomize: do not relabel if not enforcing (RHBZ#1828952)

[Richard W.M. Jones]

On Wed, Sep 23, 2020 at 05:57:50PM +0200, Pino Toscano wrote:
> Do not attempt to relabel a guest in case its SELinux enforcing mode is
> not "enforcing", as it is either pointless, or it may fail because of an
> invalid policy configured.
> ---
>  mlcustomize/SELinux_relabel.ml | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/mlcustomize/SELinux_relabel.ml b/mlcustomize/SELinux_relabel.ml
> index 647aeda..db00e59 100644
> --- a/mlcustomize/SELinux_relabel.ml
> +++ b/mlcustomize/SELinux_relabel.ml
> @@ -24,6 +24,9 @@ open Printf
>  
>  module G = Guestfs
>  
> +exception SELinux_not_enforcing
> +(* Interal exception to signal a non-enforcing SELinux. *)
> +
>  (* Simple reimplementation of Array.mem, available only with OCaml >= 4.03. *)
>  let array_find a l =
>    List.mem a (Array.to_list l)
> @@ -35,12 +38,18 @@ let rec relabel (g : G.guestfs) =
>        use_setfiles g;
>        (* That worked, so we don't need to autorelabel. *)
>        g#rm_f "/.autorelabel"
> -    with Failure _ ->
> +    with
> +    | Failure _ ->
>        (* This is the fallback in case something in the setfiles
>         * method didn't work.  That includes the case where a non-SELinux
>         * host is processing an SELinux guest, and other things.
>         *)
>        g#touch "/.autorelabel"
> +    | SELinux_not_enforcing ->
> +      (* This means that SELinux was not configured to be in enforcing mode,
> +       * so silently accept this.
> +       *)
> +      ()
>    )
>  
>  and is_selinux_guest g =
> @@ -59,6 +68,21 @@ and use_setfiles g =
>    g#aug_load ();
>    debug_augeas_errors g;
>  
> +  (* Get the SELinux enforcing mode, eg "enforcing", "permissive",
> +   * "disabled".
> +   * Use "disabled" if not specified, just like libselinux seems to do.
> +   *)
> +  let typ = read_selinux_config_key g "SELINUX" "disabled" in
> +  (* Do not attempt any relabelling if the SELinux is not "enforcing":
> +   * - in "permissive" mode SELinux is still running, however nothing is
> +   *   enforced: this means labels can be wrong, and "it is fine"

I don't think it's fine.  As I showed here:

https://www.redhat.com/archives/libguestfs/2020-June/msg00115.html

in permissive mode labels are still being updated on disk.

TBH I don't understand what you said here:

https://www.redhat.com/archives/libguestfs/2020-June/msg00117.html

about "both the labels and the policy may be all wrong".  If the
administrator set the policy to permissive then labels ought still to
be updated when the guest is running, and we ought to try to keep them
updated if we can in v2v.  It's also fine for an administrator to
switch a system to permissive and then back to enforcing without
relabelling or rebooting.

> +   * - when "disabled" means SELinux is not running, so any relabelling
> +   *   is pointless (other than potentially fail due to an invalid
> +   *   SELINUXTYPE configuration)

Here you're correct.  Once the admin disabled SELinux, labels are
going to quickly get out of step with reality, and so attempting to
relabel in virt-v2v is indeed a waste of time.

I'd accept this series if you changed "not enforcing" to
"not enforcing or permissive".

Rich.

> +   *)
> +  if typ <> "enforcing" then
> +    raise SELinux_not_enforcing;
> +
>    (* Get the SELinux policy name, eg. "targeted", "minimum".
>     * Use "targeted" if not specified, just like libselinux does.
>     *)
> -- 
> 2.26.2
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v