[libvirt] [PATCH 02/12] Refactor setup & cleanup of security labels in security driver

[Daniel P. Berrange]

On Wed, Jan 20, 2010 at 12:41:29PM -0500, Stephen Smalley wrote:
> On Wed, 2010-01-20 at 17:19 +0100, Daniel Veillard wrote:
> > On Wed, Jan 20, 2010 at 03:14:59PM +0000, Daniel P. Berrange wrote:
> > > The current security driver architecture has the following
> > > split of logic
> > > 
> > >  * domainGenSecurityLabel
> > > 
> > >     Allocate the unique label for the domain about to be started
> > > 
> > >  * domainGetSecurityLabel
> > > 
> > >     Retrieve the current live security label for a process
> > > 
> > >  * domainSetSecurityLabel
> > > 
> > >     Apply the previously allocated label to the current process
> > >     Setup all disk image / device labelling
> > > 
> > >  * domainRestoreSecurityLabel
> > > 
> > >     Restore the original disk image / device labelling.
> > >     Release the unique label for the domain
> > > 
> > > The 'domainSetSecurityLabel' method is special because it runs
> > > in the context of the child process between the fork + exec.
> > > 
> > > This is require in order to set the process label. It is not
> > > required in order to label disks/devices though. Having the
> > > disk labelling code run in the child process limits what it
> > > can do.
> > > 
> > > In particularly libvirtd would like to remember the current
> > > disk image label, and only change shared image labels for the
> > > first VM to start. This requires use & update of global state
> > > in the libvirtd daemon, and thus cannot run in the child
> > > process context.
> > > 
> > > The solution is to split domainSetSecurityLabel into two parts,
> > > one applies process label, and the other handles disk image
> > > labelling. At the same time domainRestoreSecurityLabel is
> > > similarly split, just so that it matches the style. Thus the
> > > previous 4 methods are replaced by the following 6 new methods
> > > 
> > >  * domainGenSecurityLabel
> > > 
> > >     Allocate the unique label for the domain about to be started
> > >     No actual change here.
> > > 
> > >  * domainReleaseSecurityLabel
> > > 
> > >    Release the unique label for the domain
> > > 
> > >  * domainGetSecurityProcessLabel
> > > 
> > >    Retrieve the current live security label for a process
> > >    Merely renamed for clarity.
> > > 
> > >  * domainSetSecurityProcessLabel
> > > 
> > >    Apply the previously allocated label to the current process
> > > 
> > >  * domainRestoreSecurityAllLabel
> > > 
> > >     Restore the original disk image / device labelling.
> > > 
> > >  * domainSetSecurityAllLabel
> > > 
> > >     Setup all disk image / device labelling
> > > 
> > > The SELinux and AppArmour drivers are then updated to comply with
> > > this new spec. Notice that the AppArmour driver was actually a
> > > little different. It was creating its profile for the disk image
> > > and device labels in the 'domainGenSecurityLabel' method, where as
> > > the SELinux driver did it in 'domainSetSecurityLabel'. With the
> > > new method split, we can have consistency, with both drivers doing
> > > that in the domainSetSecurityAllLabel method.
> > 
> >   A bit complex, thanks for the explanations !
> > 
> > > NB, the AppArmour changes here haven't been compiled so may not
> > > build.
> > > ---
> > >  src/qemu/qemu_driver.c           |   21 ++++++++---
> > >  src/security/security_apparmor.c |   66 ++++++++++++++++++++++-----------
> > >  src/security/security_driver.h   |   30 +++++++++------
> > >  src/security/security_selinux.c  |   75 +++++++++++++++++++++++++-------------
> > >  4 files changed, 127 insertions(+), 65 deletions(-)
> > [...]
> > > +static int
> > > +SELinuxSetSecurityAllLabel(virConnectPtr conn,
> > > +                           virDomainObjPtr vm)
> > > +{
> > > +    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
> > > +    int i;
> > > +
> > > +    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> > > +        return 0;
> > > +
> > > +    for (i = 0 ; i < vm->def->ndisks ; i++) {
> > > +        /* XXX fixme - we need to recursively label the entriy tree :-( */
> > 
> >   while we're are there let's fix the entriy/entire typo
> 
> Not sure what this is supposed to mean, but you could exec chcon -R on
> the directory tree if you want to recursively relabel it.

It is tricky, because part of the this refactoring work is to allow me to
maintain a DB of the original contexts, so that the restore step at VM
shutdown can put back the original context, rather than resetting to the
defaults using matchpathcon().  So we need to handle the recursive
chcon directly in libvirt, rather than spawning an external command.
Fortunately, I don't expect anyone to ever use the VVFAT driver that
this comment in referring to, so I'm not particularly worrying about
fixing this any time soon.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|