Libguestfs - May 2022 - [libguestfs PATCH 0/2] generator/customize: invert SELinux relabeling default

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1554735
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2075718

I'm going to post four patches or patch-sets in response to this email.
Due to how the libguestfs-common module is organized & consumed, and how
the generator in libguestfs works, this work is very awkward. (See more
below.)

The idea is to *replace* "--selinux-relabel" with
"--no-selinux-relabel", and to invert the SELinux relabeling choice:
that is, to do it by default, and allow users to prevent it with
"--no-selinux-relabel" if they desire so. This is being requested in
the
above two BZs.

I've intentionally avoided introducing "--no-selinux-relabel" *in
addition* to "--selinux-relabel". While some utilities support a
similar
dual form (such as virt-builder's "--network" and
"--no-network"), with
one being the default, those options are special in that they are *not
shared* between different utilities, and they are not generated by the
generator in libguestfs. The key difference is that the *non-shared*
options use Getopt.Set and Getopt.Clear on the *same* boolean reference
cell, whereas the generator introduces a *separate* boolean reference
cell for each option it generates (and then it uses *either*
Getopt.Clear *or* Getopt.Set when the option is passed on the command
line, dependent on the default value of the reference cell). This means
that "--no-selinux-relabel" and "--selinux-relabel", if they
both
existed, would work on different booleans, and that would be the source
of a lot of fun (priority? command line order? documentation? etc etc).
So, nope to that.

Back to the structuring of these patches / patch sets. The generator
lives and runs solely in libguestfs. However, it generates such code as
well that is owned by libguestfs-common. Normally we don't notice,
because the generator overwrites "common" submodule contents with
identical files; thus, "git" does not complain about the submodule
checkout being modified locally. This no longer holds with these
patches. Therefore:

- as first step, libguestfs needs to be modified

- the generator is run as a part of "make", which creates a local diff
  in the "common" submodule checkout under the libguestfs worktree

- that diff is reflected to, and captured as a commit, in
  libguestfs-common

- this returns libguestfs to an "everything in sync" state, but more
  importantly

- it exposes the new stuff to virt-v2v and guestfs-tools,

- virt-v2v and guestfs-tools need to be updated to consider the
  disappearance of "--selinux-relabel".

The fact that documentation and test cases are shared in various ways
only makes this more complicated. For example, the virt-builder(1)
manual speaks words on SELinux in the auto-generated (and shared), and
the private (non-shared) sections *both*.

One thing to note is that libguestfs itself does not *consume* the
particular "common" contents that it generates. Therefore we don't
have
a reference loop in practice. What we have is this dependency graph:

  libguestfs (generator)
            |
            v
  libguestfs-common (generated content)
       /              \
       v               v
   guestfs-tools     virt-v2v

Because of that, the usual "update common submodule" hunk *need not*
be
squashed into the libguestfs (generator) patches, when merging this.
However, said "update common submodule" hunk does have to be squashed
into the (single) guestfs-tools and virt-v2v patches, when merging.

I meticulously tested this stuff:

- libguestfs:

  - "make check" and "make check-slow" complete fine

  - There is no documentation (under the "website/" subdir) that is
    updated by the patches.

- guestfs-tools:

  - Checked the rendered documentation regarding
"--no-selinux-relabel"
    that comes from "common":

      virt-builder.1.html
      virt-customize.1.html
      virt-sysprep.1.html

  - Checked the rendered documentation changes that come from
    guestfs-tools itself:

      virt-builder.1.html

  - Checked the "--help" output of:

      virt-builder
      virt-customize
      virt-sysprep

  - "make check" completes OK.

  - "make check-slow" completes OK:

    - PASS for test-firstboot-*.sh (Linux guests -- Windows guests are
      SKIPped),

    - same for test-settings-*.sh

      - except for "test-settings-ubuntu-18.04.sh". It fails for an
        independent reason: "libguestfs: error: download:
        /etc/sysconfig/network: No such file or directory"

    - PASS for test-selinuxrelabel.sh

    - "test-console-ubuntu-20.04.sh" fails for an independent reason:
      "didn't see login banner in serial console output" -- but no
      serial output was actually shown in the log.

- virt-v2v:

  - "make check" completes OK.

  - "make check-slow" completes fine

    - in particular, PASS for test-v2v-conversion-of-*.sh (Linux guests
      -- Windows guests are SKIPped)

Thanks,
Laszlo

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1554735
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2075718

Thanks,
Laszlo

Laszlo Ersek (2):
  generator/customize: document that "--selinux-relabel" checks for
    SELinux
  generator/customize: invert SELinux relabeling default

 generator/customize.ml | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

-- 
2.19.1.3.g30247aa5d201

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1554735
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2075718

Thanks
Laszlo

Laszlo Ersek (2):
  mlcustomize: refresh generated files
  remove non-generated "--selinux-relabel" options

 mlcustomize/customize-options.pod  | 23 +++++++++++---------
 mlcustomize/customize-synopsis.pod |  2 +-
 mlcustomize/customize_cmdline.ml   | 16 +++++++-------
 mlcustomize/customize_cmdline.mli  |  4 ++--
 mlcustomize/test-firstboot.sh      |  3 ---
 mlcustomize/test-selinuxrelabel.sh |  5 ++---
 6 files changed, 26 insertions(+), 27 deletions(-)

-- 
2.19.1.3.g30247aa5d201

Remove "--selinux-relabel" options.

Do not add any "--no-selinux-relabel" options; rely on the internal
check
for SELinux support instead ("is_selinux_guest" in
"common/mlcustomize/SELinux_relabel.ml").

"--no-selinux-relabel" becomes a real option for virt-sysprep now.
(Again?)

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1554735
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2075718
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 builder/templates/make-template.ml |  8 +-------
 customize/customize_run.ml         |  2 +-
 sysprep/main.ml                    |  2 --
 builder/virt-builder.pod           | 20 ++++----------------
 customize/test-settings.sh         |  3 ---
 sysprep/test-virt-sysprep-docs.sh  |  2 +-
 6 files changed, 7 insertions(+), 30 deletions(-)

diff --git a/builder/templates/make-template.ml
b/builder/templates/make-template.ml
index d87349404ee4..58603242670b 100755
--- a/builder/templates/make-template.ml
+++ b/builder/templates/make-template.ml
@@ -256,8 +256,7 @@ let rec main ()      printf "Sysprepping ...\n%!";
     let cmd        sprintf "virt-sysprep --quiet -a %s%s"
-              (quote tmpout)
-              (if is_selinux_os os then " --selinux-relabel" else
"") in
+              (quote tmpout) in
     if Sys.command cmd <> 0 then exit 1
   );
 
@@ -480,11 +479,6 @@ and can_sysprep_os = function
   | Debian _ | Ubuntu _ -> true
   | FreeBSD _ | Windows _ -> false
 
-and is_selinux_os = function
-  | RHEL _ | Alma _ | CentOS _ | CentOSStream _ | Fedora _ -> true
-  | Debian _ | Ubuntu _
-  | FreeBSD _ | Windows _ -> false
-
 and needs_uefi os arch    match os, arch with
   | Fedora _, Armv7
diff --git a/customize/customize_run.ml b/customize/customize_run.ml
index f2ee20413ece..99b5fe14d849 100644
--- a/customize/customize_run.ml
+++ b/customize/customize_run.ml
@@ -415,7 +415,7 @@ let run (g : G.guestfs) root (ops : ops)        warning
(f_"passwords could not be set for this type of guest")
   );
 
-  if ops.flags.selinux_relabel then (
+  if not ops.flags.no_selinux_relabel then (
     message (f_"SELinux relabelling");
     SELinux_relabel.relabel g
   );
diff --git a/sysprep/main.ml b/sysprep/main.ml
index 087d1a17f3e8..b760618ad58a 100644
--- a/sysprep/main.ml
+++ b/sysprep/main.ml
@@ -132,8 +132,6 @@ let main ()        [ L"mount-options" ],
Getopt.Set_string (s_"opts", mount_opts),  s_"Set mount options
(eg /:noatime;/var:rw,noatime)";
       [ L"network" ], Getopt.Set network,           s_"Enable
appliance network";
       [ L"no-network" ], Getopt.Clear network,      s_"Disable
appliance network (default)";
-      [ L"no-selinux-relabel" ], Getopt.Unit (fun () -> ()),
-                                              s_"Compatibility option,
does nothing";
       [ L"operation"; L"operations" ],  Getopt.String
(s_"operations", set_operations), s_"Enable/disable specific
operations";
     ] in
     let args = basic_args @ Sysprep_operation.extra_args () in
diff --git a/builder/virt-builder.pod b/builder/virt-builder.pod
index f7dd6cdad533..aeb505296887 100644
--- a/builder/virt-builder.pod
+++ b/builder/virt-builder.pod
@@ -131,12 +131,6 @@ To update the installed packages to the latest version:
 
  virt-builder debian-7 --update
 
-For guests which use SELinux, like Fedora and Red Hat Enterprise
-Linux, you may need to do SELinux relabelling after installing or
-updating packages (see L</SELINUX> below):
-
- virt-builder fedora-27 --update --selinux-relabel
-
 =head2 Customizing the installation
 
 There are many options that let you customize the installation.  These
@@ -972,7 +966,7 @@ command line.
 
 =item *
 
-SELinux relabelling is done (I<--selinux-relabel>).
+SELinux relabelling is done unless disabled with I<--no-selinux-relabel>.
 
 =back
 
@@ -1072,8 +1066,7 @@ A typical virt-builder command would be:
    --install puppet \
    --append-line '/etc/puppet/puppet.conf:[agent]' \
    --append-line '/etc/puppet/puppet.conf:server =
puppetmaster.example.com/' \
-   --run-command 'systemctl enable puppet' \
-   --selinux-relabel
+   --run-command 'systemctl enable puppet'
 
 The precise instructions vary according to the Linux distro.  For
 further information see:
@@ -1753,14 +1746,14 @@ two possible strategies it can use to ensure correct
labelling:
 
 =over 4
 
-=item Using I<--selinux-relabel>
+=item Automatic relabeling
 
 This runs L<setfiles(8)> just before finalizing the guest, which sets
 SELinux labels correctly in the disk image.
 
 This is the recommended method.
 
-=item I<--touch> F</.autorelabel>
+=item Using I<--no-selinux-relabel> I<--touch>
F</.autorelabel>
 
 Guest templates may already contain a file called F</.autorelabel> or
 you may touch it.
@@ -1771,11 +1764,6 @@ them, which is normal and harmless.
 
 =back
 
-Please note that if your guest uses SELinux, and you are doing operations
-on it which might create new files or change existing ones, you are
-recommended to use I<--selinux-relabel>.  This will help in making sure
-that files have the right SELinux labels.
-
 =head1 MACHINE READABLE OUTPUT
 
 The I<--machine-readable> option can be used to make the output more
diff --git a/customize/test-settings.sh b/customize/test-settings.sh
index ed4c90f2eb37..e8b492dd15be 100755
--- a/customize/test-settings.sh
+++ b/customize/test-settings.sh
@@ -61,9 +61,6 @@ case "$guestname" in
         extra[${#extra[*]}]='/etc/inittab:
                                 s,^#([1-9].*respawn.*/sbin/getty.*),$1,'
         ;;
-    fedora*|rhel*|centos*)
-        extra[${#extra[*]}]='--selinux-relabel'
-        ;;
     *)
         ;;
 esac
diff --git a/sysprep/test-virt-sysprep-docs.sh
b/sysprep/test-virt-sysprep-docs.sh
index 51500b5e9799..9d0298d68557 100755
--- a/sysprep/test-virt-sysprep-docs.sh
+++ b/sysprep/test-virt-sysprep-docs.sh
@@ -25,4 +25,4 @@ $top_srcdir/podcheck.pl "$srcdir/virt-sysprep.pod"
virt-sysprep \
     --path $top_srcdir/common/options \
     --insert sysprep-extra-options.pod:__EXTRA_OPTIONS__ \
     --insert sysprep-operations.pod:__OPERATIONS__ \
-    --ignore=--dryrun,--dump-pod,--dump-pod-options,--no-selinux-relabel
+    --ignore=--dryrun,--dump-pod,--dump-pod-options
-- 
2.19.1.3.g30247aa5d201

Remove "--selinux-relabel" options.

Do not add any "--no-selinux-relabel" options; rely on the internal
check
for SELinux support instead ("is_selinux_guest" in
"common/mlcustomize/SELinux_relabel.ml").

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1554735
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2075718
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 tests/test-v2v-conversion-of.sh | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/tests/test-v2v-conversion-of.sh b/tests/test-v2v-conversion-of.sh
index 5a974d1b74c5..5c5cae7cd62a 100755
--- a/tests/test-v2v-conversion-of.sh
+++ b/tests/test-v2v-conversion-of.sh
@@ -53,13 +53,6 @@ fi
 # Some guests need special virt-builder parameters.
 # See virt-builder --notes "$guestname"
 declare -a extra
-case "$guestname" in
-    fedora*|rhel*|centos*)
-        extra[${#extra[*]}]='--selinux-relabel'
-        ;;
-    *)
-        ;;
-esac
 
 # Don't try to update Windows versions.
 case "$guestname" in
-- 
2.19.1.3.g30247aa5d201

On Tue, May 10, 2022 at 12:09:38PM +0200, Laszlo Ersek
wrote:
> Back to the structuring of these patches / patch sets. The generator
> lives and runs solely in libguestfs. However, it generates such code as
> well that is owned by libguestfs-common. Normally we don't notice,
> because the generator overwrites "common" submodule contents with
> identical files; thus, "git" does not complain about the
submodule
> checkout being modified locally.
Right, this was a known compromise to make splitting up the libguestfs
repo tractable.  Originally the generator could write into the common/
subdirectory freely because there was only a single repo.  When
splitting, I chose to commit the generated files, and since they
change infrequently this hasn't been a problem.
> This no longer holds with these
> patches. Therefore:
> 
> - as first step, libguestfs needs to be modified
> 
> - the generator is run as a part of "make", which creates a local
diff
>   in the "common" submodule checkout under the libguestfs
worktree
> 
> - that diff is reflected to, and captured as a commit, in
>   libguestfs-common
> 
> - this returns libguestfs to an "everything in sync" state, but
more
>   importantly
> 
> - it exposes the new stuff to virt-v2v and guestfs-tools,
> 
> - virt-v2v and guestfs-tools need to be updated to consider the
>   disappearance of "--selinux-relabel".
This is the right way to do it.
> The fact that documentation and test cases are shared in various ways
> only makes this more complicated. For example, the virt-builder(1)
> manual speaks words on SELinux in the auto-generated (and shared), and
> the private (non-shared) sections *both*.
> 
> One thing to note is that libguestfs itself does not *consume* the
> particular "common" contents that it generates. Therefore we
don't have
> a reference loop in practice. What we have is this dependency graph:
> 
>   libguestfs (generator)
>             |
>             v
>   libguestfs-common (generated content)
>        /              \
>        v               v
>    guestfs-tools     virt-v2v
> 
> Because of that, the usual "update common submodule" hunk *need
not* be
> squashed into the libguestfs (generator) patches, when merging this.
> However, said "update common submodule" hunk does have to be
squashed
> into the (single) guestfs-tools and virt-v2v patches, when merging.
To be clear, while there isn't a separate "update common
submodule"
commit in libguestfs, the commit hash of libguestfs -> common
submodule *will* still change (in the same commit that changes the
generator)?
> I meticulously tested this stuff:
> 
> - libguestfs:
> 
>   - "make check" and "make check-slow" complete fine
> 
>   - There is no documentation (under the "website/" subdir) that
is
>     updated by the patches.
> 
> - guestfs-tools:
> 
>   - Checked the rendered documentation regarding
"--no-selinux-relabel"
>     that comes from "common":
> 
>       virt-builder.1.html
>       virt-customize.1.html
>       virt-sysprep.1.html
> 
>   - Checked the rendered documentation changes that come from
>     guestfs-tools itself:
> 
>       virt-builder.1.html
> 
>   - Checked the "--help" output of:
> 
>       virt-builder
>       virt-customize
>       virt-sysprep
> 
>   - "make check" completes OK.
> 
>   - "make check-slow" completes OK:
> 
>     - PASS for test-firstboot-*.sh (Linux guests -- Windows guests are
>       SKIPped),
> 
>     - same for test-settings-*.sh
> 
>       - except for "test-settings-ubuntu-18.04.sh". It fails for
an
>         independent reason: "libguestfs: error: download:
>         /etc/sysconfig/network: No such file or directory"
> 
>     - PASS for test-selinuxrelabel.sh
> 
>     - "test-console-ubuntu-20.04.sh" fails for an independent
reason:
>       "didn't see login banner in serial console output" --
but no
>       serial output was actually shown in the log.
> 
> - virt-v2v:
> 
>   - "make check" completes OK.
> 
>   - "make check-slow" completes fine
> 
>     - in particular, PASS for test-v2v-conversion-of-*.sh (Linux guests
>       -- Windows guests are SKIPped)
I looked at the patches through the mailing list archives and
everything looks fine to me so you might as well push it all.  If
there are any problems I'm sure we'll find out soon enough.

For the whole series:

Acked-by: Richard W.M. Jones <rjones at redhat.com>

(You don't need to add this because I guess it will mess up your
carefully curated git commit hashes!)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

So I didn't realise that these commits break --selinux-relabel, eg:

$ rpm -q guestfs-tools
guestfs-tools-1.49.1-1.fc37.x86_64
$ virt-builder fedora-36 --selinux-relabel
virt-builder: unrecognized option '--selinux-relabel'
Try ?virt-builder --help? or consult virt-builder(1) for more information.

This is kind of bad since it breaks existing scripts.  We just had a
bug report about this from someone who is testing CentOS Stream 9.1
(where I backported the patches already).

This option should continue to exist, but do nothing.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit