Re: [libvirt] [PATCHv3] virt-aa-helper: Fix permissions for vhost-user socket files
Thanks Michal. Regards. On 2 July 2015 at 11:22, Michal Privoznik mpriv...@redhat.com wrote: On 01.07.2015 10:15, Michal Dubiel wrote: QEMU working in vhost-user mode communicates with the other end (i.e. some virtual router application) via unix domain sockets. This requires that permissions for the socket files are correctly written into /etc/apparmor.d/libvirt/libvirt-UUID.files. Signed-off-by: Michal Dubiel m...@semihalf.com --- Changes since v2: - Removed curly braces from one line 'if' block (syntax-check claims) src/security/virt-aa-helper.c | 28 +--- 1 file changed, 13 insertions(+), 15 deletions(-) ACKed and pushed. Congratulations on your first libvirt contribution! Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] virt-aa-helper: Fix permissions for vhost-user socket files
Hi Michal,
I've sent v3 of the patch with the curly braces removed from one line 'if'
blok. Sorry for that.
Regards,
Michal
On 30 June 2015 at 16:41, Michal Privoznik mpriv...@redhat.com wrote:
On 22.06.2015 12:47, Michal Dubiel wrote:
QEMU working in vhost-user mode communicates with the other end (i.e.
some virtual router application) via unix domain sockets. This requires
that permissions for the socket files are correctly written into
/etc/apparmor.d/libvirt/libvirt-UUID.files.
Signed-off-by: Michal Dubiel m...@semihalf.com
---
Changes since v1:
- Removed unnecessary stat() call and dead 'else' block
src/security/virt-aa-helper.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/src/security/virt-aa-helper.c
b/src/security/virt-aa-helper.c
index 35423b5..f39932e 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -32,7 +32,6 @@
#include unistd.h
#include errno.h
#include sys/types.h
-#include sys/stat.h
#include fcntl.h
#include getopt.h
#include sys/utsname.h
@@ -542,7 +541,6 @@ array_starts_with(const char *str, const char *
const *arr, const long size)
static int
valid_path(const char *path, const bool readonly)
{
-struct stat sb;
int npaths, opaths;
const char * const restricted[] = {
/bin/,
@@ -592,17 +590,6 @@ valid_path(const char *path, const bool readonly)
if (!virFileExists(path)) {
vah_warning(_(path does not exist, skipping file type
checks));
-} else {
-if (stat(path, sb) == -1)
-return -1;
-
-switch (sb.st_mode S_IFMT) {
-case S_IFSOCK:
-return 1;
-break;
-default:
-break;
-}
}
This leaves a one line body to the if(). Therefore 'syntax-check' is
sad. With that fixed I'm inclined to ACK the patch. But I'm not too
familiar with AppArmor, so unless somebody else gives another ACK, I'll
push this after the release.
opaths = sizeof(override)/sizeof(*(override));
@@ -1101,6 +1088,18 @@ get_files(vahControl * ctl)
}
}
+for (i = 0; i ctl-def-nnets; i++) {
+if (ctl-def-nets[i]
+ctl-def-nets[i]-type ==
VIR_DOMAIN_NET_TYPE_VHOSTUSER
+ctl-def-nets[i]-data.vhostuser) {
+virDomainChrSourceDefPtr vhu =
ctl-def-nets[i]-data.vhostuser;
+
+if (vah_add_file_chardev(buf, vhu-data.nix.path, rw,
+ vhu-type) != 0)
+goto cleanup;
+}
+}
+
if (ctl-def-virtType == VIR_DOMAIN_VIRT_KVM) {
for (i = 0; i ctl-def-nnets; i++) {
virDomainNetDefPtr net = ctl-def-nets[i];
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virt-aa-helper: Fix permissions for vhost-user socket files
On 19 June 2015 at 21:30, Serge Hallyn serge.hal...@ubuntu.com wrote:
Quoting Michal Dubiel (m...@semihalf.com):
QEMU working in vhost-user mode communicates with the other end (i.e.
some virtual router application) via unix domain sockets. This requires
that permissions for the socket files are correctly written into
/etc/apparmor.d/libvirt/libvirt-UUID.files.
Signed-off-by: Michal Dubiel m...@semihalf.com
---
src/security/virt-aa-helper.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/src/security/virt-aa-helper.c
b/src/security/virt-aa-helper.c
index 35423b5..a097aa6 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -592,19 +592,9 @@ valid_path(const char *path, const bool readonly)
if (!virFileExists(path)) {
vah_warning(_(path does not exist, skipping file type
checks));
-} else {
-if (stat(path, sb) == -1)
+} else if (stat(path, sb) == -1)
return -1;
Hi,
Why keep this bit? sb is not used later in the fn, and you
already know that access(2) didn't return ENOENT.
You are right, it is not needed. Thanks for pointing this out. I will
update the patch accordingly.
Regards,
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virt-aa-helper: Fix permissions for vhost-user socket files
Besides by changing from :
} else {
if ((x) == -1))
return -1;
to
} else if ((x) == -1) {
return -1;
Everything else inside the else if after the return -1 becomes dead
Yes it becomes dead, but there is nothing left in the 'else if' block.
Regards,
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Socket files in virt-aa-helper
Hi Serge,
This is exactly what I had done locally when I solved the permission issues
for my project, however removal of S_IFSOCK restriction is not enough to
make vhost-user work correctly. You need also the changes to get_files() I
wrote in the first e-mail about.
I'll send a patch for review (in separate thread) that adds these changes.
Regards,
Michal
On 18 June 2015 at 17:11, Serge Hallyn serge.hal...@ubuntu.com wrote:
Quoting Jamie Strandboge (ja...@canonical.com):
On 06/16/2015 08:40 AM, Michał Dubiel wrote:
Hi all,
May I kindly ask someone for some advice on this topic?
Regards,
Michal
On 21 May 2015 at 20:23, Michał Dubiel m...@semihalf.com
mailto:m...@semihalf.com wrote:
Hi guys,
I have got a question. I need to add apparmor support for
vhost-user socket
files used to communicate with the vhost-user server app. Those
ones defined
with something like:
interface type='vhostuser'
mac address='02:ed:f3:5d:de:f3'/
source type='unix'
path='/var/run/vrouter/uvh_vif_tapa8396c51-2a'
mode='client'/
model type='virtio'/
address type='pci' domain='0x' bus='0x00' slot='0x03'
function='0x0'/
/interface
I added something like this into get_files() function in
virt-aa-helper.c:
for (i = 0; i ctl-def-nnets; i++) {
if (ctl-def-nets[i]
ctl-def-nets[i]-type ==
VIR_DOMAIN_NET_TYPE_VHOSTUSER
ctl-def-nets[i]-data.vhostuser) {
virDomainChrSourceDefPtr vhu =
ctl-def-nets[i]-data.vhostuser;
if (vah_add_file_chardev(buf, vhu-data.nix.path,
rw,
vhu-type) != 0)
goto cleanup;
}
}
However, there is a restriction for the socket file types in
valid_path()
function:
switch (sb.st_mode S_IFMT) {
case S_IFSOCK:
return 1;
break;
default:
break;
}
That prevents this from working.
May I ask why the socket file types are restricted? Vhost-user
uses sockets
so if I want to use apparmor virt-aa-helper has to be able to add
the line
for the socket file into
/etc/apparmor.d/libvirt/libvirt-UUID.files.
They are restricted only because at the time virt-aa-helper.c was
written there
wasn't a valid use for them. There were more checks in this part of the
code but
over the years as more valid types were added to libvirt, they've been
removed
and now we are left with just this one. Since there is now a valid
usecase for
S_IFSOCK, it seems this can simply be removed.
So something like
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 35423b5..bb5e449 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -592,18 +592,8 @@ valid_path(const char *path, const bool readonly)
if (!virFileExists(path)) {
vah_warning(_(path does not exist, skipping file type checks));
-} else {
-if (stat(path, sb) == -1)
-return -1;
-
-switch (sb.st_mode S_IFMT) {
-case S_IFSOCK:
-return 1;
-break;
-default:
-break;
-}
-}
+} else if (stat(path, sb) == -1)
+return -1;
opaths = sizeof(override)/sizeof(*(override));
should be ok?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Socket files in virt-aa-helper
Thanks Jamie for the explanation.
Regards,
Michal
On 16 June 2015 at 17:15, Jamie Strandboge ja...@canonical.com wrote:
On 06/16/2015 08:40 AM, Michał Dubiel wrote:
Hi all,
May I kindly ask someone for some advice on this topic?
Regards,
Michal
On 21 May 2015 at 20:23, Michał Dubiel m...@semihalf.com
mailto:m...@semihalf.com wrote:
Hi guys,
I have got a question. I need to add apparmor support for vhost-user
socket
files used to communicate with the vhost-user server app. Those ones
defined
with something like:
interface type='vhostuser'
mac address='02:ed:f3:5d:de:f3'/
source type='unix'
path='/var/run/vrouter/uvh_vif_tapa8396c51-2a'
mode='client'/
model type='virtio'/
address type='pci' domain='0x' bus='0x00' slot='0x03'
function='0x0'/
/interface
I added something like this into get_files() function in
virt-aa-helper.c:
for (i = 0; i ctl-def-nnets; i++) {
if (ctl-def-nets[i]
ctl-def-nets[i]-type ==
VIR_DOMAIN_NET_TYPE_VHOSTUSER
ctl-def-nets[i]-data.vhostuser) {
virDomainChrSourceDefPtr vhu =
ctl-def-nets[i]-data.vhostuser;
if (vah_add_file_chardev(buf, vhu-data.nix.path, rw,
vhu-type) != 0)
goto cleanup;
}
}
However, there is a restriction for the socket file types in
valid_path()
function:
switch (sb.st_mode S_IFMT) {
case S_IFSOCK:
return 1;
break;
default:
break;
}
That prevents this from working.
May I ask why the socket file types are restricted? Vhost-user uses
sockets
so if I want to use apparmor virt-aa-helper has to be able to add
the line
for the socket file into /etc/apparmor.d/libvirt/libvirt-UUID.files.
They are restricted only because at the time virt-aa-helper.c was written
there
wasn't a valid use for them. There were more checks in this part of the
code but
over the years as more valid types were added to libvirt, they've been
removed
and now we are left with just this one. Since there is now a valid usecase
for
S_IFSOCK, it seems this can simply be removed.
--
Jamie Strandboge http://www.ubuntu.com/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Socket files in virt-aa-helper
Hi all,
May I kindly ask someone for some advice on this topic?
Regards,
Michal
On 21 May 2015 at 20:23, Michał Dubiel m...@semihalf.com wrote:
Hi guys,
I have got a question. I need to add apparmor support for vhost-user
socket files used to communicate with the vhost-user server app. Those ones
defined with something like:
interface type='vhostuser'
mac address='02:ed:f3:5d:de:f3'/
source type='unix' path='/var/run/vrouter/uvh_vif_tapa8396c51-2a'
mode='client'/
model type='virtio'/
address type='pci' domain='0x' bus='0x00' slot='0x03'
function='0x0'/
/interface
I added something like this into get_files() function in virt-aa-helper.c:
for (i = 0; i ctl-def-nnets; i++) {
if (ctl-def-nets[i]
ctl-def-nets[i]-type == VIR_DOMAIN_NET_TYPE_VHOSTUSER
ctl-def-nets[i]-data.vhostuser) {
virDomainChrSourceDefPtr vhu =
ctl-def-nets[i]-data.vhostuser;
if (vah_add_file_chardev(buf, vhu-data.nix.path, rw,
vhu-type) != 0)
goto cleanup;
}
}
However, there is a restriction for the socket file types in valid_path()
function:
switch (sb.st_mode S_IFMT) {
case S_IFSOCK:
return 1;
break;
default:
break;
}
That prevents this from working.
May I ask why the socket file types are restricted? Vhost-user uses
sockets so if I want to use apparmor virt-aa-helper has to be able to add
the line for the socket file into
/etc/apparmor.d/libvirt/libvirt-UUID.files.
Regards,
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Socket files in virt-aa-helper
Hi guys,
I have got a question. I need to add apparmor support for vhost-user socket
files used to communicate with the vhost-user server app. Those ones
defined with something like:
interface type='vhostuser'
mac address='02:ed:f3:5d:de:f3'/
source type='unix' path='/var/run/vrouter/uvh_vif_tapa8396c51-2a'
mode='client'/
model type='virtio'/
address type='pci' domain='0x' bus='0x00' slot='0x03'
function='0x0'/
/interface
I added something like this into get_files() function in virt-aa-helper.c:
for (i = 0; i ctl-def-nnets; i++) {
if (ctl-def-nets[i]
ctl-def-nets[i]-type == VIR_DOMAIN_NET_TYPE_VHOSTUSER
ctl-def-nets[i]-data.vhostuser) {
virDomainChrSourceDefPtr vhu =
ctl-def-nets[i]-data.vhostuser;
if (vah_add_file_chardev(buf, vhu-data.nix.path, rw,
vhu-type) != 0)
goto cleanup;
}
}
However, there is a restriction for the socket file types in valid_path()
function:
switch (sb.st_mode S_IFMT) {
case S_IFSOCK:
return 1;
break;
default:
break;
}
That prevents this from working.
May I ask why the socket file types are restricted? Vhost-user uses sockets
so if I want to use apparmor virt-aa-helper has to be able to add the line
for the socket file into /etc/apparmor.d/libvirt/libvirt-UUID.files.
Regards,
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
