[ I realized that we were discussing adding this feature, in various private email, IRC, and this long bugzilla thread: https://bugzilla.redhat.com/show_bug.cgi?id=1060423 That's not how we should do things. Let's discuss it on the mailing list. ] One thing that virt-customize/virt-sysprep/virt-builder have to do is relabel SELinux guests. What we do at the moment is run: if load_policy && fixfiles restore; then rm -f /.autorelabel else touch /.autorelabel echo '%s: SELinux relabelling failed, will relabel at boot instead.' fi while chrooted into the guest (using the 'guestfs_sh' API). This has a number of problems: - It has to load the policy using 'load_policy', but this doesn't work sometimes: * RHEL 5 load_policy takes a parameter. * Doesn't work if appliance kernel is significantly different from guest kernel version, because the binary policy format changes irregularly and is not backwards compatible. * Requires the appliance [host] kernel to be compiled with LSM/SELinux support. - Touching /.autorelabel is often broken, eg. it's broken in Fedora 20 because of systemd (RHBZ#1049656). - /etc/resolv.conf will not be relabelled if guestfs network is on, because of resolv.conf shenanigans in libguestfs.git/daemon/command.c - It requires running guest code, which we'd like to avoid. What would be nice would be to have an API to just do this relabelling. Libguestfs could change this API as required to handle different guests. Dan Walsh helpfully pointed out to us that we've been doing it wrong all along :-) A much better way to relabel is to run: setfiles /etc/selinux/targeted/contexts/files/file_contexts DIR where 'file_contexts' is a file which contains the default labels for files (a set of regexps), and 'DIR' is the directory at which relabelling starts. Note that 'setfiles' would be the libguestfs appliance binary, so no guest binary needs to be run. A simple API could just look like this: guestfs_selinux_relabel (g); which would always use the 'targeted' policy from the guest, and always start relabelling at the root. This would work fine for virt-builder. For Colin's requirements for Project Atomic, I suspect he will want to be able to set the file_contexts file and the root directory, but I'll leave him to describe what would be useful. A couple of notes: - I'd like to avoid baking in assumptions from the 'setfiles' command as far as possible. libguestfs APIs last for many years and some have caused us many years of regret (but that's our job) :-/ - Is it a good idea to tie this into inspection in some way -- for example, inspection could provide us with the path to the current or default SELinux policy. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Dan Walsh helpfully pointed out to us that we've been doing it wrong all along :-) A much better way to relabel is to run:
setfiles /etc/selinux/targeted/contexts/files/file_contexts DIR
Yes, this is what I'm doing with OSTree. However in the general cross labeling case it has a subtle issue with PCRE: http://comments.gmane.org/gmane.comp.security.selinux/20214 There is of course always the potential issue for incompatible future changes in the file_contexts format. My current workaround is: https://github.com/cgwalters/rpm-ostree/commit/0cb346b798aead0fd544e2c9ef45f...
On Sat, May 24, 2014, at 10:39 AM, Colin Walters wrote:
Dan Walsh helpfully pointed out to us that we've been doing it wrong all along :-) A much better way to relabel is to run:
setfiles /etc/selinux/targeted/contexts/files/file_contexts DIR
Yes, this is what I'm doing with OSTree. However in the general cross labeling case it has a subtle issue with PCRE: http://comments.gmane.org/gmane.comp.security.selinux/20214
There is of course always the potential issue for incompatible future changes in the file_contexts format.
My current workaround is: https://github.com/cgwalters/rpm-ostree/commit/0cb346b798aead0fd544e2c9ef45f...
FWIW I don't have an immediate major need for the relabeling API because we use Anaconda which does initial labeling there. The remaining use cases for libguestfs apps of debug/repair are still valid, but those aren't critical path.
On Saturday 24 May 2014 16:25:14 Richard W.M. Jones wrote:
What would be nice would be to have an API to just do this relabelling. Libguestfs could change this API as required to handle different guests.
Dan Walsh helpfully pointed out to us that we've been doing it wrong all along :-) A much better way to relabel is to run:
setfiles /etc/selinux/targeted/contexts/files/file_contexts DIR
where 'file_contexts' is a file which contains the default labels for files (a set of regexps), and 'DIR' is the directory at which relabelling starts. Note that 'setfiles' would be the libguestfs appliance binary, so no guest binary needs to be run.
A simple API could just look like this:
guestfs_selinux_relabel (g);
which would always use the 'targeted' policy from the guest, and always start relabelling at the root. This would work fine for virt-builder.
For Colin's requirements for Project Atomic, I suspect he will want to be able to set the file_contexts file and the root directory, but I'll leave him to describe what would be useful.
I have been experiment/working on this last week (fighting against two SELinux, the one to make happy in the guest, and the active-but-not- enforcing one eventually present in the appliance), and now I have something working. As you said, there are various possibilities in configuring such selinux_relabel API; in my first implementation I chose to have it take as mandatory argument the root of the guest to relabel, and it is able to discover the configured policy there. Of course I am open to suggestions/complains/etc about it. -- Pino Toscano
Move the current logic of the SELinux relabel to a new daemon function, so a) within the daemon it is much easier to do changes without having files touched or moved as side effects of other daemon calls b) clients can just call this new API and be done with it, with no need to think by themselves about what to do to proper do the relabel c) the logic can be updated anytime at once within libguestfs --- customize/customize_run.ml | 10 +--------- daemon/selinux.c | 27 +++++++++++++++++++++++++++ generator/actions.ml | 12 ++++++++++++ src/MAX_PROC_NR | 2 +- 4 files changed, 41 insertions(+), 10 deletions(-) diff --git a/customize/customize_run.ml b/customize/customize_run.ml index 4d83e90..2650e19 100644 --- a/customize/customize_run.ml +++ b/customize/customize_run.ml @@ -300,15 +300,7 @@ exec >>%s 2>&1 if ops.flags.selinux_relabel then ( msg (f_"SELinux relabelling"); - let cmd = sprintf " - if load_policy && fixfiles restore; then - rm -f /.autorelabel - else - touch /.autorelabel - echo '%s: SELinux relabelling failed, will relabel at boot instead.' - fi - " prog in - do_run ~display:"load_policy && fixfiles restore" cmd + g#selinux_relabel "/" ); (* Clean up the log file: diff --git a/daemon/selinux.c b/daemon/selinux.c index 1c1446d..d0a3832 100644 --- a/daemon/selinux.c +++ b/daemon/selinux.c @@ -90,3 +90,30 @@ do_getcon (void) OPTGROUP_SELINUX_NOT_AVAILABLE #endif /* !HAVE_LIBSELINUX */ + +int +do_selinux_relabel (const char *root) +{ + CLEANUP_FREE char *cmd = NULL, *out = NULL; + const char cmd_fmt[] = + "if load_policy && fixfiles restore; then\n" + " rm -f %.*s/.autorelabel\n" + "else\n" + " touch %.*s/.autorelabel\n" + " echo 'SELinux relabelling failed, will relabel at boot instead.'\n" + "fi\n"; + int len = strlen (root); + + if (root[len - 1] == '/') + --len; + + if (asprintf (&cmd, cmd_fmt, len, root, len, root) == -1) { + reply_with_perror ("asprintf"); + return -1; + } + + out = do_sh (cmd); + if (verbose) + fprintf (stderr, "%s\n", out); + return out == NULL ? -1 : 0; +} diff --git a/generator/actions.ml b/generator/actions.ml index ed65c6e..a59fe31 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -11924,6 +11924,18 @@ New (SVR4) portable format with a checksum. =back" }; + { defaults with + name = "selinux_relabel"; + style = RErr, [Pathname "root"], []; + proc_nr = Some 420; + shortdesc = "do the SELinux relabel of the files"; + longdesc = "\ +This does a relabel of the files of the system under the specified +C<root> according to the SELinux policy in the system mounted in that +C<root>. + +See the documentation about SELINUX in L<guestfs(3)>." }; + ] (* Non-API meta-commands available only in guestfish. diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR index 7b53aa0..816d01b 100644 --- a/src/MAX_PROC_NR +++ b/src/MAX_PROC_NR @@ -1 +1 @@ -419 +420 -- 1.9.3
Rewrite the relabel API to read the policy configured in the guest, invoking setfiles (added as part of the appliance, as part of policycoreutils) to relabel the specified root. In case of failure at any point of the process, a touch of .autorelabel in the root is tried as last-attempt measure to do the relabel. Considering that running SELinux tools in the appliance might be affected by the SELinux state (leading to wrong results), selinux_relabel now bails out if SELinux is enabled in the appliance. As a result of this, virt-builder and virt-customize explicitly disable it if the relabel is enabled. --- appliance/packagelist.in | 1 + builder/builder.ml | 5 +- customize/customize_main.ml | 5 +- daemon/selinux.c | 198 ++++++++++++++++++++++++++++++++++++++++---- generator/actions.ml | 7 ++ 5 files changed, 197 insertions(+), 19 deletions(-) diff --git a/appliance/packagelist.in b/appliance/packagelist.in index e4a9c8b..6336048 100644 --- a/appliance/packagelist.in +++ b/appliance/packagelist.in @@ -217,6 +217,7 @@ dnl Enabling this pulls out 140 extra packages dnl into the appliance: dnl ocfs2-tools parted +policycoreutils procps procps-ng psmisc diff --git a/builder/builder.ml b/builder/builder.ml index e7be901..a0ef6d7 100644 --- a/builder/builder.ml +++ b/builder/builder.ml @@ -585,7 +585,10 @@ let main () = (match smp with None -> () | Some smp -> g#set_smp smp); g#set_network network; - g#set_selinux ops.flags.selinux_relabel; + (* If a relabel is needed, make sure to turn SELinux off to avoid + * awkward interactions with the relabel process. + *) + if ops.flags.selinux_relabel then g#set_selinux false; (* The output disk is being created, so use cache=unsafe here. *) g#add_drive_opts ~format:output_format ~cachemode:"unsafe" output_filename; diff --git a/customize/customize_main.ml b/customize/customize_main.ml index 00d5bae..a6c6643 100644 --- a/customize/customize_main.ml +++ b/customize/customize_main.ml @@ -193,7 +193,10 @@ read the man page virt-customize(1). (match memsize with None -> () | Some memsize -> g#set_memsize memsize); (match smp with None -> () | Some smp -> g#set_smp smp); g#set_network network; - g#set_selinux ops.flags.selinux_relabel; + (* If a relabel is needed, make sure to turn SELinux off to avoid + * awkward interactions with the relabel process. + *) + if ops.flags.selinux_relabel then g#set_selinux false; (* Add disks. *) add g dryrun; diff --git a/daemon/selinux.c b/daemon/selinux.c index d0a3832..879bc61 100644 --- a/daemon/selinux.c +++ b/daemon/selinux.c @@ -21,6 +21,8 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <sys/types.h> +#include <sys/stat.h> #ifdef HAVE_SELINUX_SELINUX_H #include <selinux/selinux.h> @@ -31,6 +33,8 @@ #include "actions.h" #include "optgroups.h" +GUESTFSD_EXT_CMD(str_setfiles, setfiles); + #if defined(HAVE_LIBSELINUX) int @@ -92,28 +96,188 @@ OPTGROUP_SELINUX_NOT_AVAILABLE #endif /* !HAVE_LIBSELINUX */ int -do_selinux_relabel (const char *root) +optgroup_selinuxtools_available (void) +{ + return prog_exists (str_setfiles); +} + +#define SELINUXTYPE "SELINUXTYPE" + +static int +has_selinux_mountpoint (void) +{ + static int has_mp = -1; + if (has_mp == -1) { + struct stat sb; + dev_t root_dev; + + if (stat ("/", &sb) == -1) + return -1; + root_dev = sb.st_dev; + + if (stat ("/sys/fs/selinux", &sb) == 0 && sb.st_dev != root_dev) { + has_mp = 1; + return has_mp; + } + + if (stat ("/selinux", &sb) == 0 && sb.st_dev != root_dev) { + has_mp = 1; + return has_mp; + } + } + return has_mp; +} + +static int +read_selinux_policy (const char *file, char **policy) +{ + FILE *f; + ssize_t r; + CLEANUP_FREE char *line = NULL; + size_t len; + + f = fopen (file, "r"); + if (f == NULL) + return -1; + + while ((r = getline (&line, &len, f)) != -1) { + if ((size_t) r >= sizeof (SELINUXTYPE) + 1 + && STRPREFIX (line, SELINUXTYPE "=")) { + *policy = strndup (line + sizeof (SELINUXTYPE), r - sizeof (SELINUXTYPE) - 1); + if (*policy == NULL) { + int errno_save = errno; + fclose (f); + errno = errno_save; + return -1; + } + trim (*policy); + break; + } + } + + fclose (f); + + return 0; +} + +static int +length_without_training_slash (const char *path) { - CLEANUP_FREE char *cmd = NULL, *out = NULL; - const char cmd_fmt[] = - "if load_policy && fixfiles restore; then\n" - " rm -f %.*s/.autorelabel\n" - "else\n" - " touch %.*s/.autorelabel\n" - " echo 'SELinux relabelling failed, will relabel at boot instead.'\n" - "fi\n"; - int len = strlen (root); - - if (root[len - 1] == '/') + int len = strlen (path); + if (path[len - 1] == '/') --len; + return len; +} + +int +do_selinux_relabel (const char *root) +{ + CLEANUP_FREE char *selinux_config = NULL; + CLEANUP_FREE char *policy = NULL; + CLEANUP_FREE char *sys_rootpath = NULL; + CLEANUP_FREE char *sys_filecontextpath = NULL; + CLEANUP_FREE char *sys_autorelabelpath = NULL; + CLEANUP_FREE char *err = NULL; + CLEANUP_FREE char *out = NULL; + int r; + int len; + const size_t MAX_ARGS = 16; + const char *argv[MAX_ARGS]; + size_t i = 0; - if (asprintf (&cmd, cmd_fmt, len, root, len, root) == -1) { - reply_with_perror ("asprintf"); + if (has_selinux_mountpoint () > 0) { + reply_with_error ("cannot work when SELinux is enabled in the " + "appliance\n"); return -1; } - out = do_sh (cmd); + len = length_without_training_slash (root); + + if (asprintf (&selinux_config, "%s%.*s/etc/selinux/config", + sysroot, len, root) == -1) { + if (verbose) + fprintf (stderr, "asprintf/selinux_config failed\n"); + goto do_autorelabel; + } + + r = read_selinux_policy (selinux_config, &policy); + if (r == -1) { + if (verbose) + fprintf (stderr, "cannot read policy from %s\n", selinux_config); + goto do_autorelabel; + } + if (verbose) + fprintf (stderr, "policy in %s: %s\n", root, policy); + + if (policy[0] == '\0') + goto do_autorelabel; + + /* Make the root path relative to /sysroot. */ + sys_rootpath = sysroot_path (root); + if (sys_rootpath == NULL) { + if (verbose) + fprintf (stderr, "sysroot_path failed\n"); + goto do_autorelabel; + } + + len = length_without_training_slash (sys_rootpath); + + if (asprintf (&sys_filecontextpath, + "%.*s/etc/selinux/%s/contexts/files/file_contexts", + len, sys_rootpath, policy) == -1) { + if (verbose) + fprintf (stderr, "asprintf/file_contexts failed\n"); + goto do_autorelabel; + } + + ADD_ARG (argv, i, str_setfiles); + ADD_ARG (argv, i, "-W"); if (verbose) - fprintf (stderr, "%s\n", out); - return out == NULL ? -1 : 0; + ADD_ARG (argv, i, "-v"); + ADD_ARG (argv, i, "-r"); + ADD_ARG (argv, i, sys_rootpath); + ADD_ARG (argv, i, sys_filecontextpath); + ADD_ARG (argv, i, sys_rootpath); + ADD_ARG (argv, i, NULL); + + r = commandv (verbose ? &out : NULL, &err, argv); + if (r == -1) { + if (verbose) + fprintf (stderr, "failed setfiles %s for %s: %s\n", policy, root, err); + goto do_autorelabel; + } + if (verbose) + fprintf (stderr, "output from setfiles:\n%s\n", out); + + if (asprintf (&sys_autorelabelpath, "%.*s/.autorelabel", + len, sys_rootpath) == -1) { + if (verbose) + fprintf (stderr, "asprintf/autorelabelpath failed\n"); + /* We can safely exit in this case as well, since at most there is a + * ~/.autorelabel left in the guest. + */ + return 0; + } + + unlink (sys_autorelabelpath); + + return 0; + + do_autorelabel: + /* All of the above failed -- try to touch .autorelabel in the root + * and hope for the best. + */ + if (verbose) + fprintf (stderr, "trying /.autorelabel\n"); + + len = length_without_training_slash (root); + + if (asprintf (&sys_autorelabelpath, "%.*s/.autorelabel", + len, root) == -1) { + if (verbose) + fprintf (stderr, "asprintf/autorelabelpath failed\n"); + return -1; + } + + return do_touch (sys_autorelabelpath); } diff --git a/generator/actions.ml b/generator/actions.ml index a59fe31..3bb91c8 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -11928,12 +11928,19 @@ New (SVR4) portable format with a checksum. name = "selinux_relabel"; style = RErr, [Pathname "root"], []; proc_nr = Some 420; + optional = Some "selinuxtools"; shortdesc = "do the SELinux relabel of the files"; longdesc = "\ This does a relabel of the files of the system under the specified C<root> according to the SELinux policy in the system mounted in that C<root>. +I<Note:> the relabel does not work when SELinux is enabled (see +C<guestfs_set_selinux>), as the kernel and the policy in the +appliance might not be compatible with the policy found inside the +guest mounted at C<root>; hence, this function will error out in this +case. + See the documentation about SELINUX in L<guestfs(3)>." }; ] -- 1.9.3
On Mon, May 26, 2014 at 11:21:59AM +0200, Pino Toscano wrote:
Rewrite the relabel API to read the policy configured in the guest, invoking setfiles (added as part of the appliance, as part of policycoreutils) to relabel the specified root. In case of failure at any point of the process, a touch of .autorelabel in the root is tried as last-attempt measure to do the relabel.
Considering that running SELinux tools in the appliance might be affected by the SELinux state (leading to wrong results), selinux_relabel now bails out if SELinux is enabled in the appliance. As a result of this, virt-builder and virt-customize explicitly disable it if the relabel is enabled.
- g#set_selinux ops.flags.selinux_relabel; + (* If a relabel is needed, make sure to turn SELinux off to avoid + * awkward interactions with the relabel process. + *) + if ops.flags.selinux_relabel then g#set_selinux false;
This defaults to false, so AFAICT you could just remove this hunk. Or call g#set_selinux false unconditionally to make your intention explicit? (Same for the customize_main.ml hunk)
+ len = length_without_training_slash (root); + + if (asprintf (&selinux_config, "%s%.*s/etc/selinux/config", + sysroot, len, root) == -1) { + if (verbose) + fprintf (stderr, "asprintf/selinux_config failed\n"); + goto do_autorelabel; + } + + r = read_selinux_policy (selinux_config, &policy); + if (r == -1) { + if (verbose) + fprintf (stderr, "cannot read policy from %s\n", selinux_config); + goto do_autorelabel; + } + if (verbose) + fprintf (stderr, "policy in %s: %s\n", root, policy); + + if (policy[0] == '\0') + goto do_autorelabel;
You'll probably find this is much easier to write and a lot more robust using augeas calls. But yes, generally looks good. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On Tuesday 27 May 2014 09:08:27 Richard W.M. Jones wrote:
On Mon, May 26, 2014 at 11:21:59AM +0200, Pino Toscano wrote:
Rewrite the relabel API to read the policy configured in the guest, invoking setfiles (added as part of the appliance, as part of policycoreutils) to relabel the specified root. In case of failure at any point of the process, a touch of .autorelabel in the root is tried as last-attempt measure to do the relabel.
Considering that running SELinux tools in the appliance might be affected by the SELinux state (leading to wrong results), selinux_relabel now bails out if SELinux is enabled in the appliance. As a result of this, virt-builder and virt-customize explicitly disable it if the relabel is enabled.
- g#set_selinux ops.flags.selinux_relabel; + (* If a relabel is needed, make sure to turn SELinux off to avoid + * awkward interactions with the relabel process. + *) + if ops.flags.selinux_relabel then g#set_selinux false;
This defaults to false, so AFAICT you could just remove this hunk. Or call g#set_selinux false unconditionally to make your intention explicit?
(Same for the customize_main.ml hunk)
Yes, that together its comment above is done to make that setting explicit, so it is not changed in the future creating issues.
+ len = length_without_training_slash (root); + + if (asprintf (&selinux_config, "%s%.*s/etc/selinux/config", + sysroot, len, root) == -1) { + if (verbose) + fprintf (stderr, "asprintf/selinux_config failed\n"); + goto do_autorelabel; + } + + r = read_selinux_policy (selinux_config, &policy); + if (r == -1) { + if (verbose) + fprintf (stderr, "cannot read policy from %s\n", selinux_config); + goto do_autorelabel; + } + if (verbose) + fprintf (stderr, "policy in %s: %s\n", root, policy); + + if (policy[0] == '\0') + goto do_autorelabel;
You'll probably find this is much easier to write and a lot more robust using augeas calls.
Good idea, I will use it. Thanks, -- Pino Toscano
I'm still anxious for Colin to tell us if this API is suitable for his needs. I'm guessing that OStree does not have /etc/selinux/config, so this API would not work for him ... (Unfortunately I deleted my very old OStree VM so I'll have to download a new one to see how it works). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Hi, Im trying to expand an linux image but it does not seem to get bigger after everything is completed. The thing i noticed was, i am trying to expand a /dev/vda1 within the image but virt-resize keeps referring it as /dev/sda1. See below: # virt-resize --dry-run --expand /dev/vda1 test.qcow2 test-new.qcow2 Examining test.qcow2 ... ********** Summary of changes: /dev/sda1: This partition will be resized from 13.4G to 23.4G. The filesystem ext3 on /dev/sda1 will be expanded using the 'resize2fs' method. /dev/sda2: This partition will be left alone. ********** Is this a bug or am i doing something wrong? Thx!
On Mon, May 26, 2014 at 10:59:26PM +0200, Info || Vertixo B.V. wrote:
Hi,
Im trying to expand an linux image but it does not seem to get bigger after everything is completed.
When you say "it does not seem to get bigger" how do you determine that? What precise commands did you use to determine the size of the filesystem?
From the log below it appears everything is working fine.
The thing i noticed was, i am trying to expand a /dev/vda1 within the image but virt-resize keeps referring it as /dev/sda1. See below:
libguestfs uses a canonical naming scheme for devices, where "/dev/sda1" means "the first partition on the first device". It has nothing to do with how the guest refers to devices -- eg. a Windows guest might refer to this partition as "C:". See: http://libguestfs.org/guestfs.3.html#block-device-naming
# virt-resize --dry-run --expand /dev/vda1 test.qcow2 test-new.qcow2 Examining test.qcow2 ... ********** Summary of changes:
/dev/sda1: This partition will be resized from 13.4G to 23.4G. The filesystem ext3 on /dev/sda1 will be expanded using the 'resize2fs' method. /dev/sda2: This partition will be left alone. **********
Is this a bug or am i doing something wrong?
Thx!
Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
participants (5)
-
Colin Walters -
Colin Walters -
Info || Vertixo B.V. -
Pino Toscano -
Richard W.M. Jones