On Fri, Mar 30, 2012 at 10:30:01AM -0400, Laine Stump wrote:
> On 03/30/2012 06:25 AM, Daniel P. Berrange wrote:
> > On Fri, Mar 30, 2012 at 03:13:31AM -0400, Laine Stump wrote:
> >> On 03/29/2012 05:52 PM, Daniel P. Berrange wrote:
> >>> On Thu, Mar 29, 2012 at 04:38:50PM -0400, Laine Stump wrote:
> >>>> + if (oldbridge &&
> >>>> + virNetDevBridgeRemovePort(oldbridge, olddev->ifname) < 0) {
> >>>> + return -1;
> >>>> + }
> >>>> + if (virNetDevBridgeAddPort(newbridge, olddev->ifname) < 0) {
> >>>> + if (virNetDevBridgeAddPort(oldbridge, olddev->ifname) < 0) {
> >>>> + qemuReportError(VIR_ERR_OPERATION_FAILED,
> >>>> + _("unable to recover former state by adding port"
> >>>> + "to bridge %s"), oldbridge);
> >>>> + }
> >>>> + return -1;
> >>>> + }
> >>> I think you need to emit 2 audit notifications here, one for the bridge
> >>> being removed and one for the bridge being added.
> >> That does sound like a good idea, but the current virDomainAuditNet()
> >> function only reports MAC address, and virDomainAuditNetDevice() only
> >> reports "/dev/net/tun" - neither of them gives any information about the
> >> name of tap device or which bridge it is being attached to.
> >>
> >> Right now, here are the audit messages that are logged when I do a full
> >> device detach/attach of a network device:
> >>
> >> type=VIRT_RESOURCE msg=audit(1333090567.694:1051): pid=0 uid=0
> >> auid=4294967295 ses=4294967295
> >> subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net
> >> reason=detach vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd
> >> old-net=52:54:00:00:01:81 new-net=?: exe="/usr/sbin/libvirtd" hostname=?
> >> addr=? terminal=? res=success'
> >>
> >> type=VIRT_RESOURCE msg=audit(1333090573.195:1053): pid=0 uid=0
> >> auid=4294967295 ses=4294967295
> >> subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net
> >> reason=open vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd
> >> net=52:54:00:00:01:81 path="/dev/net/tun" rdev=0A:C8:
> >> exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success'
> >> type=VIRT_RESOURCE msg=audit(1333090573.196:1054): pid=0 uid=0
> >> auid=4294967295 ses=4294967295
> >> subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net
> >> reason=open vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd
> >> net=52:54:00:00:01:81 path="/dev/vhost-net" rdev=0A:EE:
> >> exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success'
> >> type=VIRT_RESOURCE msg=audit(1333090574.092:1055): pid=0 uid=0
> >> auid=4294967295 ses=4294967295
> >> subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net
> >> reason=attach vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd
> >> old-net=? new-net=52:54:00:00:01:81: exe="/usr/sbin/libvirtd" hostname=?
> >> addr=? terminal=? res=success'
> >>
> >> It does a good job of telling me the MAC address that's going to be used
> >> by the domain, but nothing about how it's connected to the network.
> > Hmm, this seems flawed to me. The intent of the auditing code we added
> > was to provide information about any host resource that the VM is
> > associated with. So I'm really surprised we're not providing any info
> > about the host network interface. I suspect this should be fixed.
> >
> >> If we're staying within the current boundaries of reporting, is there
> >> really value in logging a pair of messages that are ultimately just
> >> telling us that the same mac address was detached then immediately
> >> reattached, but not saying anything about what it was connected to?
> >> Alternately, if we're going to start reporting about changes in network
> >> connection, shouldn't we also be reporting what those connections are to
> >> begin with? (I think that's a change in scope of what's being audited,
> >> and requires a separate patch if we decide we want to do that).
> > I think we should still audit this change, even though current audit
> > rules appear broken.
>
> Okay. Would the patch I've attached here be adequate? If so, I'll squash
> it into the rest of the patch.
>
> Beyond that, to fix the problem of general inadequacy in the current
> audits, would it be enough to add tap device and bridge names to the
> current attach and detach log messages? Or is the audit message format
> very strict, and we would need to do a separate log message for tap
> device and for bridge device?
> From 3b277f2c2138b98145d6b5e18c844be5d43b8b29 Mon Sep 17 00:00:00 2001
> From: Laine Stump <laine@xxxxxxxxx>
> Date: Fri, 30 Mar 2012 10:13:37 -0400
> Subject: [PATCH] qemu: add audit logs when switching bridges
>
> This adds in a standard audit log for detaching and attaching a
> network device when the bridge being used is changed.
>
> The discussion about this led to the idea that the audit logs being
> generated are insufficient, since they don't say anything about which
> tap device is used, nor about which bridge it is attached to, but that
> should be fixed by a separate patch, and this gets the current patch
> properly wired into the infrastructure.
> ---
> src/qemu/qemu_hotplug.c | 8 +++++---
> 1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 66837c4..7699e9d 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1194,7 +1194,8 @@ static virDomainNetDefPtr qemuDomainFindNet(virDomainObjPtr vm,
> }
>
> static
> -int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev,
> +int qemuDomainChangeNetBridge(virDomainObjPtr vm,
> + virDomainNetDefPtr olddev,
> virDomainNetDefPtr newdev)
> {
> const char *oldbridge = olddev->data.bridge.brname;
> @@ -1221,9 +1222,11 @@ int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev,
> }
> return -1;
> }
> + virDomainAuditNet(vm, NULL, olddev, "detach", true);
This needs to be moved up, otherwise if we successfully detach, but
fail to attach we'll not emit the audit message. Also we I think
we should log "detach", false if detaching fails, and likewise
'attach', false' if attaching fails
> VIR_FREE(olddev->data.bridge.brname);
> olddev->data.bridge.brname = newdev->data.bridge.brname;
> newdev->data.bridge.brname = NULL;
> + virDomainAuditNet(vm, NULL, olddev, "attach", true);
> return 0;
> }
>
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list