[libvirt] [PATCH] selinux: distinguish failure to label from request to avoid label

[Daniel P. Berrange]

On Mon, Aug 12, 2013 at 10:19:47PM -0600, Eric Blake wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=924153
> 
> Commit 904e05a2 (v0.9.9) added a per-<disk> seclabel element with
> an attribute relabel='no' in order to try and minimize the
> impact of shutdown delays when an NFS server disappears.  The idea
> was that if a disk is on NFS and can't be labeled in the first
> place, there is no need to attempt the (no-op) relabel on domain
> shutdown.  Unfortunately, the way this was implemented was by
> modifying the domain XML so that the optimization would survive
> libvirtd restart, but in a way that is indistinguishable from an
> explicit user setting.  Furthermore, once the setting is turned
> on, libvirt avoids attempts at labeling, even for operations like
> snapshot or blockcopy where the chain is being extended or pivoted
> onto non-NFS, where SELinux labeling is once again possible.  As
> a result, it was impossible to do a blockcopy to pivot from an
> NFS image file onto a local file.
> 
> The solution is to separate the semantics of a chain that must
> not be labeled (which the user can set even on persistent domains)
> vs. the optimization of not attempting a relabel on cleanup (a
> live-only annotation), and using only the user's explicit notation
> rather than the optimization as the decision on whether to skip
> a label attempt in the first place.  When upgrading an older
> libvirtd to a newer, an NFS volume will still attempt the relabel;
> but as the avoidance of a relabel was only an optimization, this
> shouldn't cause any problems.
> 
> In the ideal future, libvirt will eventually have XML describing
> EVERY file in the backing chain, with each file having a separate
> <seclabel> element.  At that point, libvirt will be able to track
> more closely which files need a relabel attempt at shutdown.  But
> until we reach that point, the single <seclabel> for the entire
> <disk> chain is treated as a hint - when a chain has only one
> file, then we know it is accurate; but if the chain has more than
> one file, we have to attempt relabel in spite of the attribute,
> in case part of the chain is local and SELinux mattered for that
> portion of the chain.
> 
> * src/conf/domain_conf.h (_virSecurityDeviceLabelDef): Add new
> member.
> * src/conf/domain_conf.c (virSecurityDeviceLabelDefParseXML):
> Parse it, for live images only.
> (virSecurityDeviceLabelDefFormat): Output it.
> (virDomainDiskDefParseXML, virDomainChrSourceDefParseXML)
> (virDomainDiskSourceDefFormat, virDomainChrDefFormat)
> (virDomainDiskDefFormat): Pass flags on through.
> * src/security/security_selinux.c
> (virSecuritySELinuxRestoreSecurityImageLabelInt): Honor labelskip
> when possible.
> (virSecuritySELinuxSetSecurityFileLabel): Set labelskip, not
> norelabel, if labeling fails.
> * docs/formatdomain.html.in (seclabel): Document new xml.
> * docs/schemas/domaincommon.rng (devSeclabel): Allow it in RNG.
> * tests/qemuxml2argvdata/qemuxml2argv-seclabel-*-skiplabel.xml:
> * tests/qemuxml2argvdata/qemuxml2argv-seclabel-*-skiplabel.args:
> * tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-*-skiplabel.xml:
> New test files.
> * tests/qemuxml2argvtest.c (mymain): Run the new tests.
> * tests/qemuxml2xmltest.c (mymain): Likewise.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> 
>  docs/formatdomain.html.in                          |  6 ++-
>  docs/schemas/domaincommon.rng                      | 27 +++++++------
>  src/conf/domain_conf.c                             | 47 ++++++++++++++++------
>  src/conf/domain_conf.h                             |  3 +-
>  src/security/security_selinux.c                    | 10 ++++-
>  .../qemuxml2argv-seclabel-dynamic-skiplabel.args   |  5 +++
>  .../qemuxml2argv-seclabel-dynamic-skiplabel.xml    | 32 +++++++++++++++
>  .../qemuxml2argv-seclabel-static-skiplabel.args    |  5 +++
>  .../qemuxml2argv-seclabel-static-skiplabel.xml     | 33 +++++++++++++++
>  tests/qemuxml2argvtest.c                           |  2 +
>  .../qemuxml2xmlout-seclabel-dynamic-skiplabel.xml  | 31 ++++++++++++++
>  tests/qemuxml2xmltest.c                            |  8 ++--
>  12 files changed, 178 insertions(+), 31 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-skiplabel.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-skiplabel.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-static-skiplabel.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-static-skiplabel.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-dynamic-skiplabel.xml

The changes look reasonable, but I'd be alot happier if the
securityselinuxlabeltest.c was covering this scenario. We
already have that test using an LD_PRELOAD hack to mock the
selinux APIs.  It ought to be possible to extend it to return
the same errno conditions you'd see on NFS, when given certain
filenames, to allow this code to be validated.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|