[libvirt] [PATCH 1/8] Add access control filtering of domain objects

[Daniel P. Berrange]

On Tue, Jul 02, 2013 at 01:17:44PM -0600, Eric Blake wrote:
> On 06/27/2013 10:57 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> > 
> > Ensure that all APIs which list domain objects filter
> > them against the access control system.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> > ---
> >  src/conf/domain_conf.c           | 91 +++++++++++++++++++++++++++-------------
> >  src/conf/domain_conf.h           | 17 ++++++--
> >  src/libxl/libxl_driver.c         | 15 ++++---
> >  src/lxc/lxc_driver.c             | 15 ++++---
> >  src/openvz/openvz_driver.c       |  7 ++--
> >  src/parallels/parallels_driver.c | 14 ++++---
> >  src/qemu/qemu_driver.c           | 24 ++++++-----
> >  src/rpc/gendispatch.pl           | 42 ++++++++++++-------
> >  src/test/test_driver.c           | 13 +++---
> >  src/uml/uml_driver.c             | 15 ++++---
> >  src/vmware/vmware_driver.c       | 12 +++---
> >  11 files changed, 172 insertions(+), 93 deletions(-)
> 
> Big, but that's to be expected given our track record of multiple
> listing APIs.
> 
> >  
> >  int
> >  virDomainObjListNumOfDomains(virDomainObjListPtr doms,
> > -                             int active)
> > +                             bool active,
> 
> Nice conversion of int->bool on something that was already used as a bool.
> 
> > @@ -12654,8 +12658,7 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names,
> >      if (virDomainSnapshotListNamesEnsureACL(domain->conn, vm->def) < 0)
> >          goto cleanup;
> >  
> > -    n = virDomainSnapshotObjListGetNames(vm->snapshots, NULL, names, nameslen,
> > -                                         flags);
> > +    n = virDomainSnapshotObjListGetNames(vm->snapshots, NULL, names, nameslen, flags);
> 
> A bit of spurious reformatting here; you touch the same code later in
> the series, so you may want to rebase to avoid the churn, although I
> won't insist.
> 
> > @@ -12732,8 +12735,7 @@ qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot,
> >      if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot)))
> >          goto cleanup;
> >  
> > -    n = virDomainSnapshotObjListGetNames(vm->snapshots, snap, names, nameslen,
> > -                                         flags);
> > +    n = virDomainSnapshotObjListGetNames(vm->snapshots, snap, names, nameslen, flags);
> 
> and again
> 
> >  
> >  cleanup:
> >      if (vm)
> > @@ -12790,8 +12792,7 @@ qemuDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot,
> >      if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot)))
> >          goto cleanup;
> >  
> > -    n = virDomainListSnapshots(vm->snapshots, snap, snapshot->domain, snaps,
> > -                               flags);
> > +    n = virDomainListSnapshots(vm->snapshots, snap, snapshot->domain, snaps, flags);
> 
> Here too. :)

Opps, left over from code I backed out.

> > @@ -1775,18 +1788,18 @@ elsif ($mode eq "client") {
> >                      push @argvars, $arg;
> >                  }
> >  
> > -                if ($action eq "Check") {
> > -                    print "/* Returns: -1 on error, 0 on denied, 1 on allowed */\n";
> > -                } else {
> > -                    print "/* Returns: -1 on error (denied==error), 0 on allowed */\n";
> > -                }
> > -                print "int $apiname(" . join(", ", @argdecls) . ")\n";
> > +                print "/* Returns: $fail on error/denied, $pass on allowed */\n";
> > +                print "$ret $apiname(" . join(", ", @argdecls) . ")\n";
> 
> This is a semantic change for Check functions, but I can agree with it
> (in the security world, telling a person that they were denied due to
> security as opposed to some other reason is sometimes in itself a
> security hole due to the information leak).  But is it worth splitting
> into a separate patch, since it is an independent improvement used by
> all of the rest of the series, and not something fundamental to listing
> domains?

Yeah should be moved to a dedicated patch.

> ACK.
> 

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 :|