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

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.