[libvirt] [PATCH] apparmor: QEMU monitor socket moved
The directory name changed in a89f05ba8df095875f5ec8a9065a585af63a010b. --- src/security/virt-aa-helper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d7226..0ded671 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1366,6 +1366,8 @@ main(int argc, char **argv) LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%s/monitor.sock\" rw,\n", LOCALSTATEDIR, ctl->def->name); + virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-*-%.*s/monitor.sock\" rw,\n", + LOCALSTATEDIR, 20, ctl->def->name); virBufferAsprintf(&buf, " \"%s/run/libvirt/**/%s.pid\" rwk,\n", LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"/run/libvirt/**/%s.pid\" rwk,\n", -- 2.8.0.rc3
On Thu, Mar 31, 2016 at 05:00:09PM +0200, Guido Günther wrote:
The directory name changed in a89f05ba8df095875f5ec8a9065a585af63a010b. --- src/security/virt-aa-helper.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d7226..0ded671 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1366,6 +1366,8 @@ main(int argc, char **argv) LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%s/monitor.sock\" rw,\n", LOCALSTATEDIR, ctl->def->name); + virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-*-%.*s/monitor.sock\" rw,\n",
Shouldn't this be domain-%d-... with the %d being ctl->def->id? Or is it not known at this point? Then I think it should allow only numbers between the dashes. If that's possible. Another question, though: shouldn't there be also vnc.sock in case that is enabled? Basically we create this (and the qemu/channel/target/domain-...) directory just for that particular domain, so it should have access to the whole directory. Also the channel/target one, I believe. Or did I miss something? Thanks, Martin
+ LOCALSTATEDIR, 20, ctl->def->name); virBufferAsprintf(&buf, " \"%s/run/libvirt/**/%s.pid\" rwk,\n", LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"/run/libvirt/**/%s.pid\" rwk,\n", -- 2.8.0.rc3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi Martin, On Fri, Apr 01, 2016 at 01:11:21PM +0200, Martin Kletzander wrote:
On Thu, Mar 31, 2016 at 05:00:09PM +0200, Guido Günther wrote:
The directory name changed in a89f05ba8df095875f5ec8a9065a585af63a010b. --- src/security/virt-aa-helper.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d7226..0ded671 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1366,6 +1366,8 @@ main(int argc, char **argv) LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%s/monitor.sock\" rw,\n", LOCALSTATEDIR, ctl->def->name); + virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-*-%.*s/monitor.sock\" rw,\n",
Shouldn't this be domain-%d-... with the %d being ctl->def->id? Or is it not known at this point? Then I think it should allow only numbers between the dashes. If that's possible.
It is if we change virt-aa-helper slightly to not only parse VIR_DOMAIN_DEF_PARSE_INACTIVE:
From 13a93c59af04785317d3e33b4f5c308cf4c9c3de Mon Sep 17 00:00:00 2001 Message-Id: <13a93c59af04785317d3e33b4f5c308cf4c9c3de.1459516663.git.agx@sigxcpu.org> From: =?UTF-8?q?Guido=20G=C3=BCnther?= <agx@sigxcpu.org> Date: Thu, 31 Mar 2016 15:44:59 +0200 Subject: [PATCH] apparmor: QEMU monitor socket moved To: libvir-list@redhat.com
The directory name changed in a89f05ba8df095875f5ec8a9065a585af63a010b. This unbreaks launching QEMU/KVM VMs with apparmor enabled. It also adds the directory for the qemu guest-agent socket which is not known when parsing the domain XML. --- src/security/virt-aa-helper.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d7226..50d2a08 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -762,8 +762,8 @@ get_definition(vahControl * ctl, const char *xmlStr) } ctl->def = virDomainDefParseString(xmlStr, - ctl->caps, ctl->xmlopt, - VIR_DOMAIN_DEF_PARSE_INACTIVE); + ctl->caps, ctl->xmlopt, 0); + if (ctl->def == NULL) { vah_error(ctl, 0, _("could not parse XML")); goto exit; @@ -1366,6 +1366,10 @@ main(int argc, char **argv) LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%s/monitor.sock\" rw,\n", LOCALSTATEDIR, ctl->def->name); + virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%d-%.*s/*\" rw,\n", + LOCALSTATEDIR, ctl->def->id, 20, ctl->def->name); + virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/channel/target/domain-%d-%.*s/*\" rw,\n", + LOCALSTATEDIR, ctl->def->id, 20, ctl->def->name); virBufferAsprintf(&buf, " \"%s/run/libvirt/**/%s.pid\" rwk,\n", LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"/run/libvirt/**/%s.pid\" rwk,\n", -- 2.8.0.rc3
Another question, though: shouldn't there be also vnc.sock in case that is enabled? Basically we create this (and the qemu/channel/target/domain-...) directory just for that particular domain, so it should have access to the whole directory. Also the channel/target one, I believe. Or did I miss something?
I added the channel/target one as well since the socket path does not seem to be available when we parse the XML. Since the directories are domain specific I went for allowing access to the whole dir. While the above works as expected on the _first_ start of a domain it fails on the second start since (at least in 1.3.3~rc1) we don't change the domain id in the directory names on subsequent starts: First start: # virsh list Id Name State ---------------------------------------------------- 1 jessie running # find /var/lib/libvirt/ -name 'domain-*-jessie' /var/lib/libvirt/qemu/channel/target/domain-1-jessie /var/lib/libvirt/qemu/domain-1-jessie Second start: # virsh list Id Name State ---------------------------------------------------- 2 jessie running # find /var/lib/libvirt/ -name 'domain-*-jessie' /var/lib/libvirt/qemu/channel/target/domain-1-jessie /var/lib/libvirt/qemu/domain-1-jessie Shouldn't that be: /var/lib/libvirt/qemu/channel/target/domain-2-jessie /var/lib/libvirt/qemu/domain-2-jessie Is this on purpose or rather a bug in the code that generates the socket paths? Cheers, -- Guido
On Fri, Apr 01, 2016 at 03:30:37PM +0200, Guido Günther wrote:
Hi Martin, On Fri, Apr 01, 2016 at 01:11:21PM +0200, Martin Kletzander wrote:
On Thu, Mar 31, 2016 at 05:00:09PM +0200, Guido Günther wrote:
The directory name changed in a89f05ba8df095875f5ec8a9065a585af63a010b. --- src/security/virt-aa-helper.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d7226..0ded671 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1366,6 +1366,8 @@ main(int argc, char **argv) LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%s/monitor.sock\" rw,\n", LOCALSTATEDIR, ctl->def->name); + virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-*-%.*s/monitor.sock\" rw,\n",
Shouldn't this be domain-%d-... with the %d being ctl->def->id? Or is it not known at this point? Then I think it should allow only numbers between the dashes. If that's possible.
It is if we change virt-aa-helper slightly to not only parse VIR_DOMAIN_DEF_PARSE_INACTIVE:
From 13a93c59af04785317d3e33b4f5c308cf4c9c3de Mon Sep 17 00:00:00 2001 Message-Id: <13a93c59af04785317d3e33b4f5c308cf4c9c3de.1459516663.git.agx@sigxcpu.org> From: =?UTF-8?q?Guido=20G=C3=BCnther?= <agx@sigxcpu.org> Date: Thu, 31 Mar 2016 15:44:59 +0200 Subject: [PATCH] apparmor: QEMU monitor socket moved To: libvir-list@redhat.com
The directory name changed in a89f05ba8df095875f5ec8a9065a585af63a010b.
This unbreaks launching QEMU/KVM VMs with apparmor enabled. It also adds the directory for the qemu guest-agent socket which is not known when parsing the domain XML. --- src/security/virt-aa-helper.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d7226..50d2a08 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -762,8 +762,8 @@ get_definition(vahControl * ctl, const char *xmlStr) }
ctl->def = virDomainDefParseString(xmlStr, - ctl->caps, ctl->xmlopt, - VIR_DOMAIN_DEF_PARSE_INACTIVE); + ctl->caps, ctl->xmlopt, 0); + if (ctl->def == NULL) { vah_error(ctl, 0, _("could not parse XML")); goto exit; @@ -1366,6 +1366,10 @@ main(int argc, char **argv) LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%s/monitor.sock\" rw,\n", LOCALSTATEDIR, ctl->def->name); + virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%d-%.*s/*\" rw,\n", + LOCALSTATEDIR, ctl->def->id, 20, ctl->def->name); + virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/channel/target/domain-%d-%.*s/*\" rw,\n", + LOCALSTATEDIR, ctl->def->id, 20, ctl->def->name); virBufferAsprintf(&buf, " \"%s/run/libvirt/**/%s.pid\" rwk,\n", LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"/run/libvirt/**/%s.pid\" rwk,\n", -- 2.8.0.rc3
That looks nicer, exactly what I had in mind.
Another question, though: shouldn't there be also vnc.sock in case that is enabled? Basically we create this (and the qemu/channel/target/domain-...) directory just for that particular domain, so it should have access to the whole directory. Also the channel/target one, I believe. Or did I miss something?
I added the channel/target one as well since the socket path does not seem to be available when we parse the XML. Since the directories are domain specific I went for allowing access to the whole dir.
While the above works as expected on the _first_ start of a domain it fails on the second start since (at least in 1.3.3~rc1) we don't change the domain id in the directory names on subsequent starts:
First start: # virsh list Id Name State ---------------------------------------------------- 1 jessie running # find /var/lib/libvirt/ -name 'domain-*-jessie' /var/lib/libvirt/qemu/channel/target/domain-1-jessie /var/lib/libvirt/qemu/domain-1-jessie
Second start: # virsh list Id Name State ---------------------------------------------------- 2 jessie running # find /var/lib/libvirt/ -name 'domain-*-jessie' /var/lib/libvirt/qemu/channel/target/domain-1-jessie /var/lib/libvirt/qemu/domain-1-jessie
Shouldn't that be: /var/lib/libvirt/qemu/channel/target/domain-2-jessie /var/lib/libvirt/qemu/domain-2-jessie
Is this on purpose or rather a bug in the code that generates the socket paths?
Definitely a bug in the code. I'll fix this (right after fixing another one for this release). Thanks for spotting that out. I'd say ACK for the second version if you're OK with only my ACK and a promise that I'll look into that aforementioned bug.
Cheers, -- Guido
Hi, On Fri, Apr 01, 2016 at 03:51:08PM +0200, Martin Kletzander wrote:
On Fri, Apr 01, 2016 at 03:30:37PM +0200, Guido Günther wrote:
Hi Martin, On Fri, Apr 01, 2016 at 01:11:21PM +0200, Martin Kletzander wrote:
On Thu, Mar 31, 2016 at 05:00:09PM +0200, Guido Günther wrote:
The directory name changed in a89f05ba8df095875f5ec8a9065a585af63a010b. --- src/security/virt-aa-helper.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d7226..0ded671 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1366,6 +1366,8 @@ main(int argc, char **argv) LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%s/monitor.sock\" rw,\n", LOCALSTATEDIR, ctl->def->name); + virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-*-%.*s/monitor.sock\" rw,\n",
Shouldn't this be domain-%d-... with the %d being ctl->def->id? Or is it not known at this point? Then I think it should allow only numbers between the dashes. If that's possible.
It is if we change virt-aa-helper slightly to not only parse VIR_DOMAIN_DEF_PARSE_INACTIVE:
From 13a93c59af04785317d3e33b4f5c308cf4c9c3de Mon Sep 17 00:00:00 2001 Message-Id: <13a93c59af04785317d3e33b4f5c308cf4c9c3de.1459516663.git.agx@sigxcpu.org> From: =?UTF-8?q?Guido=20G=C3=BCnther?= <agx@sigxcpu.org> Date: Thu, 31 Mar 2016 15:44:59 +0200 Subject: [PATCH] apparmor: QEMU monitor socket moved To: libvir-list@redhat.com
The directory name changed in a89f05ba8df095875f5ec8a9065a585af63a010b.
This unbreaks launching QEMU/KVM VMs with apparmor enabled. It also adds the directory for the qemu guest-agent socket which is not known when parsing the domain XML. --- src/security/virt-aa-helper.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d7226..50d2a08 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -762,8 +762,8 @@ get_definition(vahControl * ctl, const char *xmlStr) }
ctl->def = virDomainDefParseString(xmlStr, - ctl->caps, ctl->xmlopt, - VIR_DOMAIN_DEF_PARSE_INACTIVE); + ctl->caps, ctl->xmlopt, 0); + if (ctl->def == NULL) { vah_error(ctl, 0, _("could not parse XML")); goto exit; @@ -1366,6 +1366,10 @@ main(int argc, char **argv) LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%s/monitor.sock\" rw,\n", LOCALSTATEDIR, ctl->def->name); + virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%d-%.*s/*\" rw,\n", + LOCALSTATEDIR, ctl->def->id, 20, ctl->def->name); + virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/channel/target/domain-%d-%.*s/*\" rw,\n", + LOCALSTATEDIR, ctl->def->id, 20, ctl->def->name); virBufferAsprintf(&buf, " \"%s/run/libvirt/**/%s.pid\" rwk,\n", LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"/run/libvirt/**/%s.pid\" rwk,\n", -- 2.8.0.rc3
That looks nicer, exactly what I had in mind.
Another question, though: shouldn't there be also vnc.sock in case that is enabled? Basically we create this (and the qemu/channel/target/domain-...) directory just for that particular domain, so it should have access to the whole directory. Also the channel/target one, I believe. Or did I miss something?
I added the channel/target one as well since the socket path does not seem to be available when we parse the XML. Since the directories are domain specific I went for allowing access to the whole dir.
While the above works as expected on the _first_ start of a domain it fails on the second start since (at least in 1.3.3~rc1) we don't change the domain id in the directory names on subsequent starts:
First start: # virsh list Id Name State ---------------------------------------------------- 1 jessie running # find /var/lib/libvirt/ -name 'domain-*-jessie' /var/lib/libvirt/qemu/channel/target/domain-1-jessie /var/lib/libvirt/qemu/domain-1-jessie
Second start: # virsh list Id Name State ---------------------------------------------------- 2 jessie running # find /var/lib/libvirt/ -name 'domain-*-jessie' /var/lib/libvirt/qemu/channel/target/domain-1-jessie /var/lib/libvirt/qemu/domain-1-jessie
Shouldn't that be: /var/lib/libvirt/qemu/channel/target/domain-2-jessie /var/lib/libvirt/qemu/domain-2-jessie
Is this on purpose or rather a bug in the code that generates the socket paths?
Definitely a bug in the code. I'll fix this (right after fixing another one for this release). Thanks for spotting that out. I'd say ACK for the second version if you're OK with only my ACK and a promise that I'll look into that aforementioned bug.
Pushed now to unbreak apparmor before 1.3.3. If Jamie or Cedric (or anybody else caring about the apparmor suport) have any comments I'm happy to rework this then. Cheers and thanks, -- Guido
Cheers, -- Guido
participants (2)
-
Guido Günther -
Martin Kletzander