[libvirt] [PATCH 0/6] audit: Improve disk auditing and add auditing of new device types
Peter Krempa (6): util: storage: Add helper to determine whether storage is local conf: audit: rng: Reorder new and old RNG device definitions conf: audit: Split out common steps to audit domain devices audit: disk: Refactor disk auditing to avoid auditing remote storage audit: Add auditing for serial/parallel/channel/console characted devs audit: Audit smartcard devices src/conf/domain_audit.c | 281 +++++++++++++++++++++++++--------------------- src/conf/domain_audit.h | 11 +- src/libvirt_private.syms | 2 + src/lxc/lxc_driver.c | 6 +- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hotplug.c | 38 +++---- src/util/virstoragefile.c | 7 ++ src/util/virstoragefile.h | 1 + 8 files changed, 198 insertions(+), 152 deletions(-) -- 1.9.3
There's a lot of places where we skip doing actions based on the locality of given storage type. The usual pattern is to skip it if: virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK Add a simple helper to simplify the pattern to virStorageSourceIsLocalStorage(src) --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 7 +++++++ src/util/virstoragefile.h | 1 + 3 files changed, 9 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ed56103..067dcad 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1910,6 +1910,7 @@ virStorageSourceClear; virStorageSourceFree; virStorageSourceGetActualType; virStorageSourceGetSecurityLabelDef; +virStorageSourceIsLocalStorage; virStorageSourceNewFromBacking; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 0c50de1..7ab640f 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1550,6 +1550,13 @@ virStorageSourceGetActualType(virStorageSourcePtr def) } +bool +virStorageSourceIsLocalStorage(virStorageSourcePtr src) +{ + return virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK; +} + + /** * virStorageSourceBackingStoreClear: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 48c7e02..6609023 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -320,6 +320,7 @@ void virStorageSourceAuthClear(virStorageSourcePtr def); void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); int virStorageSourceGetActualType(virStorageSourcePtr def); +bool virStorageSourceIsLocalStorage(virStorageSourcePtr src); void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceBackingStoreClear(virStorageSourcePtr def); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); -- 1.9.3
On 07/03/2014 12:04 PM, Peter Krempa wrote:
There's a lot of places where we skip doing actions based on the locality of given storage type. The usual pattern is to skip it if:
virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK
Add a simple helper to simplify the pattern to virStorageSourceIsLocalStorage(src)
Are you planning to use it anywhere out of those "lot of places"? :)
--- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 7 +++++++ src/util/virstoragefile.h | 1 + 3 files changed, 9 insertions(+)
ACK Jan
On 07/03/14 15:25, Ján Tomko wrote:
On 07/03/2014 12:04 PM, Peter Krempa wrote:
There's a lot of places where we skip doing actions based on the locality of given storage type. The usual pattern is to skip it if:
virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK
Add a simple helper to simplify the pattern to virStorageSourceIsLocalStorage(src)
Are you planning to use it anywhere out of those "lot of places"? :)
I stole this patch from my snapshot source handling refactor : "[libvirt] [PATCHv4 00/29] (for 1.2.7) qemu: Refactor handling of disk image metadata" That series didn't get any reviews and I needed that functionality. It's getting a lot of love in that series :)
--- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 7 +++++++ src/util/virstoragefile.h | 1 + 3 files changed, 9 insertions(+)
ACK
Jan
On 07/03/14 15:25, Ján Tomko wrote:
On 07/03/2014 12:04 PM, Peter Krempa wrote:
There's a lot of places where we skip doing actions based on the locality of given storage type. The usual pattern is to skip it if:
virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK
Add a simple helper to simplify the pattern to virStorageSourceIsLocalStorage(src)
Are you planning to use it anywhere out of those "lot of places"? :)
--- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 7 +++++++ src/util/virstoragefile.h | 1 + 3 files changed, 9 insertions(+)
ACK
Pushed now; Thanks. Peter
The audit functions usually take the old definition before the new one in the argument list. Unify RNG device to use the same order. --- src/conf/domain_audit.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 52c643d..91095b1 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -138,7 +138,7 @@ virDomainAuditDisk(virDomainObjPtr vm, static void virDomainAuditRNG(virDomainObjPtr vm, - virDomainRNGDefPtr newDef, virDomainRNGDefPtr oldDef, + virDomainRNGDefPtr oldDef, virDomainRNGDefPtr newDef, const char *reason, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -827,7 +827,7 @@ virDomainAuditStart(virDomainObjPtr vm, const char *reason, bool success) } if (vm->def->rng) - virDomainAuditRNG(vm, vm->def->rng, NULL, "start", true); + virDomainAuditRNG(vm, NULL, vm->def->rng, "start", true); if (vm->def->tpm) virDomainAuditTPM(vm, vm->def->tpm, "start", true); -- 1.9.3
On 07/03/2014 12:04 PM, Peter Krempa wrote:
The audit functions usually take the old definition before the new one in the argument list. Unify RNG device to use the same order. --- src/conf/domain_audit.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK
On 07/03/14 15:30, Ján Tomko wrote:
On 07/03/2014 12:04 PM, Peter Krempa wrote:
The audit functions usually take the old definition before the new one in the argument list. Unify RNG device to use the same order. --- src/conf/domain_audit.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK
Pushed; Thanks. Peter
Extract common operations done when creating an audit message to a separate generic function that can be reused and convert RNG, disk, FS and net audit to use it. --- src/conf/domain_audit.c | 175 ++++++++++++++++-------------------------------- 1 file changed, 57 insertions(+), 118 deletions(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 91095b1..4c4290c 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -93,46 +93,73 @@ virDomainAuditChardevPath(virDomainChrSourceDefPtr chr) } -void -virDomainAuditDisk(virDomainObjPtr vm, - const char *oldDef, const char *newDef, - const char *reason, bool success) +static void +virDomainAuditGenericDev(virDomainObjPtr vm, + const char *type, + const char *oldsrcpath, + const char *newsrcpath, + const char *reason, + bool success) { + char *newdev = NULL; + char *olddev = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname; char *oldsrc = NULL; char *newsrc = NULL; const char *virt; - virUUIDFormat(vm->def->uuid, uuidstr); - if (!(vmname = virAuditEncode("vm", vm->def->name))) { - VIR_WARN("OOM while encoding audit message"); + /* if both new and old source aren't provided don't log anything */ + if (!newsrcpath && !oldsrcpath) return; - } + + if (virAsprintfQuiet(&newdev, "new-%s", type) < 0) + goto no_memory; + + if (virAsprintfQuiet(&olddev, "old-%s", type) < 0) + goto no_memory; + + virUUIDFormat(vm->def->uuid, uuidstr); + + if (!(vmname = virAuditEncode("vm", vm->def->name))) + goto no_memory; if (!(virt = virDomainVirtTypeToString(vm->def->virtType))) { - VIR_WARN("Unexpected virt type %d while encoding audit message", vm->def->virtType); + VIR_WARN("Unexpected virt type %d while encoding audit message", + vm->def->virtType); virt = "?"; } - if (!(oldsrc = virAuditEncode("old-disk", VIR_AUDIT_STR(oldDef)))) { - VIR_WARN("OOM while encoding audit message"); - goto cleanup; - } - if (!(newsrc = virAuditEncode("new-disk", VIR_AUDIT_STR(newDef)))) { - VIR_WARN("OOM while encoding audit message"); - goto cleanup; - } + if (!(newsrc = virAuditEncode(newdev, VIR_AUDIT_STR(newsrcpath)))) + goto no_memory; + + if (!(oldsrc = virAuditEncode(olddev, VIR_AUDIT_STR(oldsrcpath)))) + goto no_memory; VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "virt=%s resrc=disk reason=%s %s uuid=%s %s %s", - virt, reason, vmname, uuidstr, - oldsrc, newsrc); + "virt=%s resrc=%s reason=%s %s uuid=%s %s %s", + virt, type, reason, vmname, uuidstr, oldsrc, newsrc); cleanup: + VIR_FREE(newdev); + VIR_FREE(olddev); VIR_FREE(vmname); VIR_FREE(oldsrc); VIR_FREE(newsrc); + return; + + no_memory: + VIR_WARN("OOM while encoding audit message"); + goto cleanup; +} + + +void +virDomainAuditDisk(virDomainObjPtr vm, + const char *oldDef, const char *newDef, + const char *reason, bool success) +{ + virDomainAuditGenericDev(vm, "disk", oldDef, newDef, reason, success); } @@ -141,13 +168,8 @@ virDomainAuditRNG(virDomainObjPtr vm, virDomainRNGDefPtr oldDef, virDomainRNGDefPtr newDef, const char *reason, bool success) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *vmname; const char *newsrcpath = NULL; const char *oldsrcpath = NULL; - char *oldsrc = NULL; - char *newsrc = NULL; - const char *virt; if (newDef) { switch ((virDomainRNGBackend) newDef->backend) { @@ -185,40 +207,7 @@ virDomainAuditRNG(virDomainObjPtr vm, } } - /* don't audit the RNG device if it doesn't use local resources */ - if (!oldsrcpath && !newsrcpath) - return; - - virUUIDFormat(vm->def->uuid, uuidstr); - if (!(vmname = virAuditEncode("vm", vm->def->name))) - goto no_memory; - - if (!(virt = virDomainVirtTypeToString(vm->def->virtType))) { - VIR_WARN("Unexpected virt type %d while encoding audit message", - vm->def->virtType); - virt = "?"; - } - - if (!(newsrc = virAuditEncode("new-rng", VIR_AUDIT_STR(newsrcpath)))) - goto no_memory; - - if (!(oldsrc = virAuditEncode("old-rng", VIR_AUDIT_STR(oldsrcpath)))) - goto no_memory; - - VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "virt=%s resrc=rng reason=%s %s uuid=%s %s %s", - virt, reason, vmname, uuidstr, - oldsrc, newsrc); - - cleanup: - VIR_FREE(vmname); - VIR_FREE(oldsrc); - VIR_FREE(newsrc); - return; - - no_memory: - VIR_WARN("OOM while encoding audit message"); - goto cleanup; + virDomainAuditGenericDev(vm, "rng", oldsrcpath, newsrcpath, reason, success); } @@ -227,45 +216,10 @@ virDomainAuditFS(virDomainObjPtr vm, virDomainFSDefPtr oldDef, virDomainFSDefPtr newDef, const char *reason, bool success) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *vmname; - char *oldsrc = NULL; - char *newsrc = NULL; - const char *virt; - - virUUIDFormat(vm->def->uuid, uuidstr); - if (!(vmname = virAuditEncode("vm", vm->def->name))) { - VIR_WARN("OOM while encoding audit message"); - return; - } - - if (!(virt = virDomainVirtTypeToString(vm->def->virtType))) { - VIR_WARN("Unexpected virt type %d while encoding audit message", vm->def->virtType); - virt = "?"; - } - - if (!(oldsrc = virAuditEncode("old-fs", - oldDef && oldDef->src ? - oldDef->src : "?"))) { - VIR_WARN("OOM while encoding audit message"); - goto cleanup; - } - if (!(newsrc = virAuditEncode("new-fs", - newDef && newDef->src ? - newDef->src : "?"))) { - VIR_WARN("OOM while encoding audit message"); - goto cleanup; - } - - VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "virt=%s resrc=fs reason=%s %s uuid=%s %s %s", - virt, reason, vmname, uuidstr, - oldsrc, newsrc); - - cleanup: - VIR_FREE(vmname); - VIR_FREE(oldsrc); - VIR_FREE(newsrc); + virDomainAuditGenericDev(vm, "fs", + oldDef ? oldDef->src : NULL, + newDef ? newDef->src : NULL, + reason, success); } @@ -274,34 +228,19 @@ virDomainAuditNet(virDomainObjPtr vm, virDomainNetDefPtr oldDef, virDomainNetDefPtr newDef, const char *reason, bool success) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; char newMacstr[VIR_MAC_STRING_BUFLEN]; char oldMacstr[VIR_MAC_STRING_BUFLEN]; - char *vmname; - const char *virt; - virUUIDFormat(vm->def->uuid, uuidstr); if (oldDef) virMacAddrFormat(&oldDef->mac, oldMacstr); + if (newDef) virMacAddrFormat(&newDef->mac, newMacstr); - if (!(vmname = virAuditEncode("vm", vm->def->name))) { - VIR_WARN("OOM while encoding audit message"); - return; - } - if (!(virt = virDomainVirtTypeToString(vm->def->virtType))) { - VIR_WARN("Unexpected virt type %d while encoding audit message", vm->def->virtType); - virt = "?"; - } - - VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "virt=%s resrc=net reason=%s %s uuid=%s old-net=%s new-net=%s", - virt, reason, vmname, uuidstr, - oldDef ? oldMacstr : "?", - newDef ? newMacstr : "?"); - - VIR_FREE(vmname); + virDomainAuditGenericDev(vm, "fs", + oldDef ? oldMacstr : NULL, + newDef ? newMacstr : NULL, + reason, success); } /** -- 1.9.3
On 07/03/2014 12:04 PM, Peter Krempa wrote:
Extract common operations done when creating an audit message to a separate generic function that can be reused and convert RNG, disk, FS and net audit to use it. --- src/conf/domain_audit.c | 175 ++++++++++++++++-------------------------------- 1 file changed, 57 insertions(+), 118 deletions(-)
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 91095b1..4c4290c 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -93,46 +93,73 @@ virDomainAuditChardevPath(virDomainChrSourceDefPtr chr) }
-void -virDomainAuditDisk(virDomainObjPtr vm, - const char *oldDef, const char *newDef, - const char *reason, bool success) +static void +virDomainAuditGenericDev(virDomainObjPtr vm, + const char *type, + const char *oldsrcpath, + const char *newsrcpath, + const char *reason, + bool success) { + char *newdev = NULL; + char *olddev = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname;
vmname can be used unitialized in the cleanup section on OOM
char *oldsrc = NULL; char *newsrc = NULL; const char *virt;
- virUUIDFormat(vm->def->uuid, uuidstr); - if (!(vmname = virAuditEncode("vm", vm->def->name))) { - VIR_WARN("OOM while encoding audit message");
+ /* if both new and old source aren't provided don't log anything */ + if (!newsrcpath && !oldsrcpath)
Please move this to the next commit and let this one be just code movement.
return; - } + + if (virAsprintfQuiet(&newdev, "new-%s", type) < 0) + goto no_memory; + + if (virAsprintfQuiet(&olddev, "old-%s", type) < 0) + goto no_memory; + + virUUIDFormat(vm->def->uuid, uuidstr); + + if (!(vmname = virAuditEncode("vm", vm->def->name))) + goto no_memory;
if (!(virt = virDomainVirtTypeToString(vm->def->virtType))) { - VIR_WARN("Unexpected virt type %d while encoding audit message", vm->def->virtType); + VIR_WARN("Unexpected virt type %d while encoding audit message", + vm->def->virtType); virt = "?"; }
@@ -274,34 +228,19 @@ virDomainAuditNet(virDomainObjPtr vm, virDomainNetDefPtr oldDef, virDomainNetDefPtr newDef, const char *reason, bool success) {
- - VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "virt=%s resrc=net reason=%s %s uuid=%s old-net=%s new-net=%s", - virt, reason, vmname, uuidstr, - oldDef ? oldMacstr : "?", - newDef ? newMacstr : "?"); - - VIR_FREE(vmname); + virDomainAuditGenericDev(vm, "fs",
The resrc should be "net" here, not "fs".
+ oldDef ? oldMacstr : NULL, + newDef ? newMacstr : NULL, + reason, success); }
/**
ACK with the nits fixed. Jan
On 07/03/14 15:29, Ján Tomko wrote:
On 07/03/2014 12:04 PM, Peter Krempa wrote:
Extract common operations done when creating an audit message to a separate generic function that can be reused and convert RNG, disk, FS and net audit to use it. --- src/conf/domain_audit.c | 175 ++++++++++++++++-------------------------------- 1 file changed, 57 insertions(+), 118 deletions(-)
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 91095b1..4c4290c 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -93,46 +93,73 @@ virDomainAuditChardevPath(virDomainChrSourceDefPtr chr) }
-void -virDomainAuditDisk(virDomainObjPtr vm, - const char *oldDef, const char *newDef, - const char *reason, bool success) +static void +virDomainAuditGenericDev(virDomainObjPtr vm, + const char *type, + const char *oldsrcpath, + const char *newsrcpath, + const char *reason, + bool success) { + char *newdev = NULL; + char *olddev = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname;
vmname can be used unitialized in the cleanup section on OOM
char *oldsrc = NULL; char *newsrc = NULL; const char *virt;
- virUUIDFormat(vm->def->uuid, uuidstr); - if (!(vmname = virAuditEncode("vm", vm->def->name))) { - VIR_WARN("OOM while encoding audit message");
+ /* if both new and old source aren't provided don't log anything */ + if (!newsrcpath && !oldsrcpath)
Please move this to the next commit and let this one be just code movement.
That condition is also necessary for this patch as auditing with both new and old definition missing doesn't make sense. Some of the other refactored functions rely on that. Peter
On 07/04/2014 11:08 AM, Peter Krempa wrote:
On 07/03/14 15:29, Ján Tomko wrote:
On 07/03/2014 12:04 PM, Peter Krempa wrote:
Extract common operations done when creating an audit message to a separate generic function that can be reused and convert RNG, disk, FS and net audit to use it. --- src/conf/domain_audit.c | 175 ++++++++++++++++-------------------------------- 1 file changed, 57 insertions(+), 118 deletions(-)
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 91095b1..4c4290c 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c
char *oldsrc = NULL; char *newsrc = NULL; const char *virt;
- virUUIDFormat(vm->def->uuid, uuidstr); - if (!(vmname = virAuditEncode("vm", vm->def->name))) { - VIR_WARN("OOM while encoding audit message");
+ /* if both new and old source aren't provided don't log anything */ + if (!newsrcpath && !oldsrcpath)
Please move this to the next commit and let this one be just code movement.
That condition is also necessary for this patch as auditing with both new and old definition missing doesn't make sense. Some of the other refactored functions rely on that.
Right, I missed the condition in virDomainAuditRNG. Jan
On 07/04/14 11:11, Ján Tomko wrote:
On 07/04/2014 11:08 AM, Peter Krempa wrote:
On 07/03/14 15:29, Ján Tomko wrote:
On 07/03/2014 12:04 PM, Peter Krempa wrote:
Extract common operations done when creating an audit message to a separate generic function that can be reused and convert RNG, disk, FS and net audit to use it. --- src/conf/domain_audit.c | 175 ++++++++++++++++-------------------------------- 1 file changed, 57 insertions(+), 118 deletions(-)
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 91095b1..4c4290c 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c
char *oldsrc = NULL; char *newsrc = NULL; const char *virt;
- virUUIDFormat(vm->def->uuid, uuidstr); - if (!(vmname = virAuditEncode("vm", vm->def->name))) { - VIR_WARN("OOM while encoding audit message");
+ /* if both new and old source aren't provided don't log anything */ + if (!newsrcpath && !oldsrcpath)
Please move this to the next commit and let this one be just code movement.
That condition is also necessary for this patch as auditing with both new and old definition missing doesn't make sense. Some of the other refactored functions rely on that.
Right, I missed the condition in virDomainAuditRNG.
Jan
I've fixed the uninitialized variable and the resource name for networks and I'm about to push this one in a minute. Thanks. Peter
Pass the virStorageSource struct to the auditing function and check if storage is local before auditting. --- src/conf/domain_audit.c | 25 ++++++++++++++++--------- src/conf/domain_audit.h | 4 ++-- src/lxc/lxc_driver.c | 6 +++--- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_hotplug.c | 21 ++++++++------------- 5 files changed, 31 insertions(+), 29 deletions(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 4c4290c..c4dcfa5 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -156,10 +156,21 @@ virDomainAuditGenericDev(virDomainObjPtr vm, void virDomainAuditDisk(virDomainObjPtr vm, - const char *oldDef, const char *newDef, - const char *reason, bool success) + virStorageSourcePtr oldDef, + virStorageSourcePtr newDef, + const char *reason, + bool success) { - virDomainAuditGenericDev(vm, "disk", oldDef, newDef, reason, success); + const char *oldsrc = NULL; + const char *newsrc = NULL; + + if (oldDef && virStorageSourceIsLocalStorage(oldDef)) + oldsrc = oldDef->path; + + if (newDef && virStorageSourceIsLocalStorage(newDef)) + newsrc = newDef->path; + + virDomainAuditGenericDev(vm, "disk", oldsrc, newsrc, reason, success); } @@ -738,12 +749,8 @@ virDomainAuditStart(virDomainObjPtr vm, const char *reason, bool success) { size_t i; - for (i = 0; i < vm->def->ndisks; i++) { - const char *src = virDomainDiskGetSource(vm->def->disks[i]); - - if (src) /* Skips CDROM without media initially inserted */ - virDomainAuditDisk(vm, NULL, src, "start", true); - } + for (i = 0; i < vm->def->ndisks; i++) + virDomainAuditDisk(vm, NULL, vm->def->disks[i]->src, "start", true); for (i = 0; i < vm->def->nfss; i++) { virDomainFSDefPtr fs = vm->def->fss[i]; diff --git a/src/conf/domain_audit.h b/src/conf/domain_audit.h index 70b09e5..58d25a4 100644 --- a/src/conf/domain_audit.h +++ b/src/conf/domain_audit.h @@ -39,8 +39,8 @@ void virDomainAuditStop(virDomainObjPtr vm, const char *reason) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virDomainAuditDisk(virDomainObjPtr vm, - const char *oldDef, - const char *newDef, + virStorageSourcePtr oldDef, + virStorageSourcePtr newDef, const char *reason, bool success) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index a9a87ea..251817d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4120,7 +4120,7 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver, cleanup: if (src) - virDomainAuditDisk(vm, NULL, src, "attach", ret == 0); + virDomainAuditDisk(vm, NULL, def->src, "attach", ret == 0); VIR_FREE(file); return ret; } @@ -4608,10 +4608,10 @@ lxcDomainDetachDeviceDiskLive(virDomainObjPtr vm, } if (lxcDomainAttachDeviceUnlink(vm, dst) < 0) { - virDomainAuditDisk(vm, src, NULL, "detach", false); + virDomainAuditDisk(vm, def->src, NULL, "detach", false); goto cleanup; } - virDomainAuditDisk(vm, src, NULL, "detach", true); + virDomainAuditDisk(vm, def->src, NULL, "detach", true); if (virCgroupDenyDevicePath(priv->cgroup, src, VIR_CGROUP_DEVICE_RWM) != 0) VIR_WARN("cannot deny device %s for domain %s", diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d45a161..b39c405 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12969,7 +12969,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } } - virDomainAuditDisk(vm, disk->src->path, source, "snapshot", ret >= 0); + virDomainAuditDisk(vm, disk->src, snap->src, "snapshot", ret >= 0); if (ret < 0) goto cleanup; @@ -15400,7 +15400,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDriveMirror(priv->mon, device, dest, format, bandwidth, flags); - virDomainAuditDisk(vm, NULL, dest, "mirror", ret >= 0); + virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) { qemuDomainPrepareDiskChainElementPath(driver, vm, disk, dest, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5e8aa4e..8d37813 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -154,9 +154,7 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, qemuDomainObjExitMonitor(driver, vm); } audit: - if (src) - virDomainAuditDisk(vm, virDomainDiskGetSource(origdisk), - src, "update", ret >= 0); + virDomainAuditDisk(vm, origdisk->src, disk->src, "update", ret >= 0); if (ret < 0) goto error; @@ -330,7 +328,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, } qemuDomainObjExitMonitor(driver, vm); - virDomainAuditDisk(vm, NULL, src, "attach", ret >= 0); + virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0); if (ret < 0) goto error; @@ -583,7 +581,7 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, } qemuDomainObjExitMonitor(driver, vm); - virDomainAuditDisk(vm, NULL, src, "attach", ret >= 0); + virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0); if (ret < 0) goto error; @@ -677,7 +675,7 @@ qemuDomainAttachUSBMassstorageDevice(virConnectPtr conn, } qemuDomainObjExitMonitor(driver, vm); - virDomainAuditDisk(vm, NULL, src, "attach", ret >= 0); + virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0); if (ret < 0) goto error; @@ -2489,7 +2487,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, qemuDomainObjExitMonitor(driver, vm); VIR_FREE(drivestr); - virDomainAuditDisk(vm, src, NULL, "detach", true); + virDomainAuditDisk(vm, disk->src, NULL, "detach", true); event = virDomainEventDeviceRemovedNewFromObj(vm, disk->info.alias); if (event) @@ -2942,16 +2940,14 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { qemuDomainObjExitMonitor(driver, vm); - virDomainAuditDisk(vm, virDomainDiskGetSource(detach), - NULL, "detach", false); + virDomainAuditDisk(vm, detach->src, NULL, "detach", false); goto cleanup; } } else { if (qemuMonitorRemovePCIDevice(priv->mon, &detach->info.addr.pci) < 0) { qemuDomainObjExitMonitor(driver, vm); - virDomainAuditDisk(vm, virDomainDiskGetSource(detach), - NULL, "detach", false); + virDomainAuditDisk(vm, detach->src, NULL, "detach", false); goto cleanup; } } @@ -2996,8 +2992,7 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { qemuDomainObjExitMonitor(driver, vm); - virDomainAuditDisk(vm, virDomainDiskGetSource(detach), - NULL, "detach", false); + virDomainAuditDisk(vm, detach->src, NULL, "detach", false); goto cleanup; } qemuDomainObjExitMonitor(driver, vm); -- 1.9.3
On 07/03/2014 12:05 PM, Peter Krempa wrote:
Pass the virStorageSource struct to the auditing function and check if storage is local before auditting.
*auditing This would be nice to document in auditlog.html
--- src/conf/domain_audit.c | 25 ++++++++++++++++--------- src/conf/domain_audit.h | 4 ++-- src/lxc/lxc_driver.c | 6 +++--- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_hotplug.c | 21 ++++++++------------- 5 files changed, 31 insertions(+), 29 deletions(-)
ACK Jan
On 07/03/14 15:30, Ján Tomko wrote:
On 07/03/2014 12:05 PM, Peter Krempa wrote:
Pass the virStorageSource struct to the auditing function and check if storage is local before auditting.
*auditing
This would be nice to document in auditlog.html
Well the auditlog doc states that we audit only *host resources*, thus auditing (sometimes) incomplete info about a remote storage volume is technically a bug.
--- src/conf/domain_audit.c | 25 ++++++++++++++++--------- src/conf/domain_audit.h | 4 ++-- src/lxc/lxc_driver.c | 6 +++--- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_hotplug.c | 21 ++++++++------------- 5 files changed, 31 insertions(+), 29 deletions(-)
ACK
Thanks, I'll push this one shortly. Peter
Add startup auditing and also hotplug auditing for said devices --- src/conf/domain_audit.c | 35 +++++++++++++++++++++++++++++++++++ src/conf/domain_audit.h | 7 +++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_hotplug.c | 17 +++++++++++------ 4 files changed, 54 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index c4dcfa5..b7f8123 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -155,6 +155,29 @@ virDomainAuditGenericDev(virDomainObjPtr vm, void +virDomainAuditChardev(virDomainObjPtr vm, + virDomainChrDefPtr oldDef, + virDomainChrDefPtr newDef, + const char *reason, + bool success) +{ + virDomainChrSourceDefPtr oldsrc = NULL; + virDomainChrSourceDefPtr newsrc = NULL; + + if (oldDef) + oldsrc = &oldDef->source; + + if (newDef) + newsrc = &newDef->source; + + virDomainAuditGenericDev(vm, "chardev", + virDomainAuditChardevPath(oldsrc), + virDomainAuditChardevPath(newsrc), + reason, success); +} + + +void virDomainAuditDisk(virDomainObjPtr vm, virStorageSourcePtr oldDef, virStorageSourcePtr newDef, @@ -772,6 +795,18 @@ virDomainAuditStart(virDomainObjPtr vm, const char *reason, bool success) virDomainAuditRedirdev(vm, redirdev, "start", true); } + for (i = 0; i < vm->def->nserials; i++) + virDomainAuditChardev(vm, NULL, vm->def->serials[i], "start", true); + + for (i = 0; i < vm->def->nparallels; i++) + virDomainAuditChardev(vm, NULL, vm->def->parallels[i], "start", true); + + for (i = 0; i < vm->def->nchannels; i++) + virDomainAuditChardev(vm, NULL, vm->def->channels[i], "start", true); + + for (i = 0; i < vm->def->nconsoles; i++) + virDomainAuditChardev(vm, NULL, vm->def->consoles[i], "start", true); + if (vm->def->rng) virDomainAuditRNG(vm, NULL, vm->def->rng, "start", true); diff --git a/src/conf/domain_audit.h b/src/conf/domain_audit.h index 58d25a4..3434feb 100644 --- a/src/conf/domain_audit.h +++ b/src/conf/domain_audit.h @@ -111,4 +111,11 @@ void virDomainAuditRedirdev(virDomainObjPtr vm, bool success) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +void virDomainAuditChardev(virDomainObjPtr vm, + virDomainChrDefPtr oldDef, + virDomainChrDefPtr newDef, + const char *reason, + bool success) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); + #endif /* __VIR_DOMAIN_AUDIT_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 067dcad..b04b099 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -116,6 +116,7 @@ virDomainPCIAddressValidate; virDomainAuditCgroup; virDomainAuditCgroupMajor; virDomainAuditCgroupPath; +virDomainAuditChardev; virDomainAuditDisk; virDomainAuditFS; virDomainAuditHostdev; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8d37813..5451118 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1460,18 +1460,20 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0) { qemuDomainObjExitMonitor(driver, vm); - goto cleanup; + goto audit; } if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) { /* detach associated chardev on error */ qemuMonitorDetachCharDev(priv->mon, charAlias); qemuDomainObjExitMonitor(driver, vm); - goto cleanup; + goto audit; } qemuDomainObjExitMonitor(driver, vm); ret = 0; + audit: + virDomainAuditChardev(vm, NULL, chr, "attach", ret == 0); cleanup: if (ret < 0 && need_remove) qemuDomainChrRemove(vmdef, chr); @@ -2751,6 +2753,7 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, char *charAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; + int rc; VIR_DEBUG("Removing character device %s from domain %p %s", chr->info.alias, vm, vm->def->name); @@ -2759,12 +2762,14 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorDetachCharDev(priv->mon, charAlias) < 0) { - qemuDomainObjExitMonitor(driver, vm); - goto cleanup; - } + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); qemuDomainObjExitMonitor(driver, vm); + virDomainAuditChardev(vm, chr, NULL, "detach", rc == 0); + + if (rc < 0) + goto cleanup; + event = virDomainEventDeviceRemovedNewFromObj(vm, chr->info.alias); if (event) qemuDomainEventQueue(driver, event); -- 1.9.3
On 07/03/2014 12:05 PM, Peter Krempa wrote:
Add startup auditing and also hotplug auditing for said devices --- src/conf/domain_audit.c | 35 +++++++++++++++++++++++++++++++++++ src/conf/domain_audit.h | 7 +++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_hotplug.c | 17 +++++++++++------ 4 files changed, 54 insertions(+), 6 deletions(-)
Missing changes in docs/auditlog.html.in
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index c4dcfa5..b7f8123 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c
virDomainAuditDisk(virDomainObjPtr vm, virStorageSourcePtr oldDef, virStorageSourcePtr newDef, @@ -772,6 +795,18 @@ virDomainAuditStart(virDomainObjPtr vm, const char *reason, bool success) virDomainAuditRedirdev(vm, redirdev, "start", true); }
+ for (i = 0; i < vm->def->nserials; i++) + virDomainAuditChardev(vm, NULL, vm->def->serials[i], "start", true); + + for (i = 0; i < vm->def->nparallels; i++) + virDomainAuditChardev(vm, NULL, vm->def->parallels[i], "start", true); + + for (i = 0; i < vm->def->nchannels; i++) + virDomainAuditChardev(vm, NULL, vm->def->channels[i], "start", true); + + for (i = 0; i < vm->def->nconsoles; i++) + virDomainAuditChardev(vm, NULL, vm->def->consoles[i], "start", true); +
I wonder if working around the first console aliased to the first serial port (or was it the other way around?) is worth it to prevent logging the same device twice. ACK with the docs added. Jan
--- src/conf/domain_audit.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index b7f8123..a906d88 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -177,6 +177,51 @@ virDomainAuditChardev(virDomainObjPtr vm, } +static void +virDomainAuditSmartcard(virDomainObjPtr vm, + virDomainSmartcardDefPtr def, + const char *reason, + bool success) +{ + const char *database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE; + size_t i; + + if (def) { + switch ((virDomainSmartcardType) def->type) { + case VIR_DOMAIN_SMARTCARD_TYPE_HOST: + virDomainAuditGenericDev(vm, "smartcard", + NULL, "nss-smartcard-device", + reason, success); + break; + + case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES: + for (i = 0; i < VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES; i++) { + virDomainAuditGenericDev(vm, "smartcard", NULL, + def->data.cert.file[i], + reason, success); + } + + if (def->data.cert.database) + database = def->data.cert.database; + + virDomainAuditGenericDev(vm, "smartcard", + NULL, database, + reason, success); + break; + + case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: + virDomainAuditGenericDev(vm, "smartcard", NULL, + virDomainAuditChardevPath(&def->data.passthru), + reason, success); + break; + + case VIR_DOMAIN_SMARTCARD_TYPE_LAST: + break; + } + } +} + + void virDomainAuditDisk(virDomainObjPtr vm, virStorageSourcePtr oldDef, @@ -807,6 +852,9 @@ virDomainAuditStart(virDomainObjPtr vm, const char *reason, bool success) for (i = 0; i < vm->def->nconsoles; i++) virDomainAuditChardev(vm, NULL, vm->def->consoles[i], "start", true); + for (i = 0; i < vm->def->nsmartcards; i++) + virDomainAuditSmartcard(vm, vm->def->smartcards[i], "start", true); + if (vm->def->rng) virDomainAuditRNG(vm, NULL, vm->def->rng, "start", true); -- 1.9.3
participants (2)
-
Ján Tomko -
Peter Krempa