[libvirt] Re: [et-mgmt-tools] Re: Patch to python-virtinst to allow it to choose svirt labels

[Cole Robinson]

Cole Robinson wrote:
> Daniel J Walsh wrote:
> 

<snip>

> 
> Updated patch attached, I'll reply with patch specific comments later.
> 
> Thanks,
> Cole
> 

Two major pieces missing from this patch:

1) Some way to skip all this. If selinux isn't enabled on the host, or
the user doesn't request labeling, we shouldn't do the work.

2) Support in CapabilitiesParser for the host security policy, otherwise
we have no way of knowing if selinux is enabled on a remote host.

> diff -r 026a37ccd417 virtinst/Guest.py
> --- a/virtinst/Guest.py	Sat Feb 21 14:36:07 2009 -0500
> +++ b/virtinst/Guest.py	Sat Feb 21 15:09:49 2009 -0500
> @@ -27,6 +27,8 @@
>  import libvirt
>  import CapabilitiesParser
>  import VirtualGraphics
> +import selinux

This import should be moved into gen_seclabels, and probably wrapped in
a try...except block, and if the import fails (libselinux-python isn't
isntalled) we just skip adding the labels to the xml. This will make it
easier for other distros who may not want the selinux support to avoid
problems.

> +import random
>  
>  import osdict
>  from virtinst import _virtinst as _
> @@ -71,6 +73,8 @@
>          self._cpuset = None
>          self._graphics_dev = None
>          self._consolechild = None
> +        self._seclabel = None
> +        self._imagelabel = None
>  
>          self._os_type = None
>          self._os_variant = None
> @@ -101,6 +105,16 @@
>  
>          self._caps = CapabilitiesParser.parse(self.conn.getCapabilities())
> 
> +        (self.default_seclabel,
> +         self.default_imagelabel) =  self._default_seclabels()
> +
> +        while self._seclabel == None:
> +            seclabel, imagelabel = self.gen_seclabels()
> +            if self.is_conflict_seclabel(self.conn, seclabel):
> +                continue
> +            self.set_seclabel(seclabel)
> +            self.set_imagelabel(imagelabel)
> +
>  
>      def get_installer(self):
>          return self._installer
> @@ -110,6 +124,20 @@
>          self._installer = val
>      installer = property(get_installer, set_installer)
>  
> +    # Security context used to secure guest image 
> +    def get_imagelabel(self):
> +        return self._imagelabel
> +    def set_imagelabel(self, val):
> +        self._imagelabel = val
> +    imagelabel = property(get_imagelabel, set_imagelabel)
> +
> +    # Security context used to secure guest process
> +    def get_seclabel(self):
> +        return self._seclabel
> +    def set_seclabel(self, val):
> +        self._seclabel = val
> +    seclabel = property(get_seclabel, set_seclabel)
> +
>      # Domain name of the guest
>      def get_name(self):
>          return self._name
> @@ -407,6 +435,19 @@
>              xml = _util.xml_append(xml, sound_dev.get_xml_config())
>          return xml
>  
> +    def _get_seclabel_xml(self):
> +        xml = ""
> +        if self._seclabel != None:
> +            xml = """
> +  <seclabel model='selinux'>
> +    <label>%s</label>
> +    <image>%s</image>
> +  </seclabel>
> +""" % ( self._seclabel, self._imagelabel)
> +            print xml
> +        return xml
> +
> +

This doesn't look like it matches the latest svirt libvirt patches: I
don't see an <image> tag in those.

>      def _get_device_xml(self, install=True):
>          xml = ""
>  
> @@ -494,6 +535,7 @@
>    <devices>
>  %(devices)s
>    </devices>
> +  %(secxml)s
>  </domain>
>  """ % { "type": self.type,
>          "name": self.name, \
> @@ -504,7 +546,8 @@
>          "maxramkb": self.maxmemory * 1024, \
>          "devices": self._get_device_xml(install), \
>          "osblob": osblob, \
> -        "action": action }
> +        "action": action, 
> +        "secxml": self._get_seclabel_xml()}
>  
>  
>      def start_install(self, consolecb=None, meter=None, removeOld=False,
> @@ -537,6 +580,8 @@
>          """Ensure that devices are setup"""
>          for disk in self._install_disks:
>              disk.setup(progresscb)
> +            # Not sure of this, might want to put this in VirtualDisk class
> +            selinux.setfilecon(disk.path, self._imagelabel)

Yes, this should be in the VirtualDisk class, in the setup method.
VirtualDisk would also need an imagelabel attribute in that case. It may
be nice to also keep the Guest one, rather than force the user to fill
in the imagelabel for every VirtualDisk object being added.

Either way, this will break the remote case as is.

>          for nic in self._install_nics:
>              nic.setup(self.conn)
>  
> @@ -631,6 +676,80 @@
>          if self.domain is not None:
>              raise RuntimeError, _("Domain has already been started!")
>  
> +    def _default_seclabels(self):
> +        try:
> +            fd = open(selinux.selinux_virtual_domain_context_path(), 'r')
> +        except OSError, (err_no, msg):
> +            raise RuntimeError(_("Failed to find SELinux virtual domains "
> +                                "context: %s: %s %s" %
> +                                (selinux.selinux_virtual_domain_context_path(),
> +                                 err_no, msg)))
> +
> +        label = fd.read()
> +        fd.close()
> +        try:
> +            fd = open(selinux.selinux_virtual_image_context_path(), 'r')
> +        except OSError, (err_no, msg):
> +            raise RuntimeError(_("Failed to find SELinux virtual image "
> +                               "context: %s: %s %s" %
> +                               (selinux.selinux_virtual_domain_context_path(),
> +                                err_no, msg)))
> +

What version of libselinux-python were these functions added in? They
don't seem present on F9 at least.

Also, this labeling would only be relevant on the local host. Maybe this
info should be exposed by libvirt capabilities?

If we do get this info from capabilities, then we may as well just
remove the dependency on libselinux-python, and just use 'chcon' directly.

The rest looks reasonable.

Thanks,
Cole