Re: [Qemu-devel] [PATCH v2 2/4] Add access control support to qemu bridge helper
On 10/23/2011 09:10 AM, Blue Swirl wrote:
On Fri, Oct 21, 2011 at 15:07, Corey Bryantcor...@linux.vnet.ibm.com wrote:
We go to great lengths to restrict ourselves to just cap_net_admin as an OS
enforced security mechanism. However, we further restrict what we allow
users
to do to simply adding a tap device to a bridge interface by virtue of the
fact
that this is the only functionality we expose.
This is not good enough though. An administrator is likely to want to
restrict
the bridges that an unprivileged user can access, in particular, to restrict
an unprivileged user from putting a guest on what should be isolated
networks.
This patch implements an ACL mechanism that is enforced by
qemu-bridge-helper.
The ACLs are fairly simple whitelist/blacklist mechanisms with a wildcard of
'all'. All users are blacklisted by default, and deny takes precedence over
allow.
An interesting feature of this ACL mechanism is that you can include external
ACL files. The main reason to support this is so that you can set different
file system permissions on those external ACL files. This allows an
administrator to implement rather sophisicated ACL policies based on
user/group
sophisticated
Yep, thanks.
policies via the file system.
As an example:
/etc/qemu/bridge.conf root:qemu 0640
allow br0
include /etc/qemu/alice.conf
include /etc/qemu/bob.conf
include /etc/qemu/charlie.conf
/etc/qemu/alice.conf root:alice 0640
allow br1
/etc/qemu/bob.conf root:bob 0640
allow br2
/etc/qemu/charlie.conf root:charlie 0640
deny all
I think syntax 'include/etc/qemu/user.d/*.conf' or 'includedir
/etc/qemu/user.d' could be also useful.
That could be useful, though I'm not sure it's necessary right now.
This ACL pattern allows any user in the qemu group to get a tap device
connected to br0 (which is bridged to the physical network).
Users in the alice group can additionally get a tap device connected to br1.
This allows br1 to act as a private bridge for the alice group.
Users in the bob group can additionally get a tap device connected to br2.
This allows br2 to act as a private bridge for the bob group.
Users in the charlie group cannot get a tap device connected to any bridge.
Under no circumstance can the bob group get access to br1 or can the alice
group get access to br2. And under no cicumstance can the charlie group
get access to any bridge.
Signed-off-by: Anthony Liguorialigu...@us.ibm.com
Signed-off-by: Richa Marwaharmar...@linux.vnet.ibm.com
Signed-off-by: Corey Bryantcor...@linux.vnet.ibm.com
---
qemu-bridge-helper.c | 141
++
1 files changed, 141 insertions(+), 0 deletions(-)
diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 2ce82fb..db257d5 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -33,6 +33,105 @@
#include net/tap-linux.h
+#define MAX_ACLS (128)
If all users (or groups) in the system have an ACL, this number could
be way too low. Please use a list instead.
I agree, we shouldn't be hard-coding the limit here. I'll update this.
+#define DEFAULT_ACL_FILE CONFIG_QEMU_CONFDIR /bridge.conf
+
+enum {
+ACL_ALLOW = 0,
+ACL_ALLOW_ALL,
+ACL_DENY,
+ACL_DENY_ALL,
+};
+
+typedef struct ACLRule {
+int type;
+char iface[IFNAMSIZ];
+} ACLRule;
+
+static int parse_acl_file(const char *filename, ACLRule *acls, int
*pacl_count)
+{
+int acl_count = *pacl_count;
+FILE *f;
+char line[4096];
+
+f = fopen(filename, r);
+if (f == NULL) {
+return -1;
+}
+
+while (acl_count != MAX_ACLS
+fgets(line, sizeof(line), f) != NULL) {
+char *ptr = line;
+char *cmd, *arg, *argend;
+
+while (isspace(*ptr)) {
+ptr++;
+}
+
+/* skip comments and empty lines */
+if (*ptr == '#' || *ptr == 0) {
+continue;
+}
+
+cmd = ptr;
+arg = strchr(cmd, ' ');
+if (arg == NULL) {
+arg = strchr(cmd, '\t');
+}
+
+if (arg == NULL) {
+fprintf(stderr, Invalid config line:\n %s\n, line);
+fclose(f);
+errno = EINVAL;
+return -1;
+}
+
+*arg = 0;
+arg++;
+while (isspace(*arg)) {
+arg++;
+}
+
+argend = arg + strlen(arg);
+while (arg != argend isspace(*(argend - 1))) {
+argend--;
+}
These while loops to skip spaces are repeated, but the comment
skipping part is not, so it is not possible to have comments after
rules or split rules to several lines. I'd add a simple state variable
to track at which stage we are in parsing instead.
That could be useful too, but again not sure it's
Re: [Qemu-devel] [PATCH v2 2/4] Add access control support to qemu bridge helper
On Mon, Oct 24, 2011 at 13:44, Corey Bryant cor...@linux.vnet.ibm.com wrote:
On 10/23/2011 09:10 AM, Blue Swirl wrote:
On Fri, Oct 21, 2011 at 15:07, Corey Bryantcor...@linux.vnet.ibm.com
wrote:
We go to great lengths to restrict ourselves to just cap_net_admin as
an OS
enforced security mechanism. However, we further restrict what we
allow users
to do to simply adding a tap device to a bridge interface by virtue of
the fact
that this is the only functionality we expose.
This is not good enough though. An administrator is likely to want to
restrict
the bridges that an unprivileged user can access, in particular, to
restrict
an unprivileged user from putting a guest on what should be isolated
networks.
This patch implements an ACL mechanism that is enforced by
qemu-bridge-helper.
The ACLs are fairly simple whitelist/blacklist mechanisms with a
wildcard of
'all'. All users are blacklisted by default, and deny takes
precedence over
allow.
An interesting feature of this ACL mechanism is that you can include
external
ACL files. The main reason to support this is so that you can set
different
file system permissions on those external ACL files. This allows an
administrator to implement rather sophisicated ACL policies based on
user/group
sophisticated
Yep, thanks.
policies via the file system.
As an example:
/etc/qemu/bridge.conf root:qemu 0640
allow br0
include /etc/qemu/alice.conf
include /etc/qemu/bob.conf
include /etc/qemu/charlie.conf
/etc/qemu/alice.conf root:alice 0640
allow br1
/etc/qemu/bob.conf root:bob 0640
allow br2
/etc/qemu/charlie.conf root:charlie 0640
deny all
I think syntax 'include/etc/qemu/user.d/*.conf' or 'includedir
/etc/qemu/user.d' could be also useful.
That could be useful, though I'm not sure it's necessary right now.
It can be added later.
This ACL pattern allows any user in the qemu group to get a tap device
connected to br0 (which is bridged to the physical network).
Users in the alice group can additionally get a tap device connected
to br1.
This allows br1 to act as a private bridge for the alice group.
Users in the bob group can additionally get a tap device connected to
br2.
This allows br2 to act as a private bridge for the bob group.
Users in the charlie group cannot get a tap device connected to any
bridge.
Under no circumstance can the bob group get access to br1 or can the
alice
group get access to br2. And under no cicumstance can the charlie
group
get access to any bridge.
Signed-off-by: Anthony Liguorialigu...@us.ibm.com
Signed-off-by: Richa Marwaharmar...@linux.vnet.ibm.com
Signed-off-by: Corey Bryantcor...@linux.vnet.ibm.com
---
qemu-bridge-helper.c | 141
++
1 files changed, 141 insertions(+), 0 deletions(-)
diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 2ce82fb..db257d5 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -33,6 +33,105 @@
#include net/tap-linux.h
+#define MAX_ACLS (128)
If all users (or groups) in the system have an ACL, this number could
be way too low. Please use a list instead.
I agree, we shouldn't be hard-coding the limit here. I'll update this.
+#define DEFAULT_ACL_FILE CONFIG_QEMU_CONFDIR /bridge.conf
+
+enum {
+ ACL_ALLOW = 0,
+ ACL_ALLOW_ALL,
+ ACL_DENY,
+ ACL_DENY_ALL,
+};
+
+typedef struct ACLRule {
+ int type;
+ char iface[IFNAMSIZ];
+} ACLRule;
+
+static int parse_acl_file(const char *filename, ACLRule *acls, int
*pacl_count)
+{
+ int acl_count = *pacl_count;
+ FILE *f;
+ char line[4096];
+
+ f = fopen(filename, r);
+ if (f == NULL) {
+ return -1;
+ }
+
+ while (acl_count != MAX_ACLS
+ fgets(line, sizeof(line), f) != NULL) {
+ char *ptr = line;
+ char *cmd, *arg, *argend;
+
+ while (isspace(*ptr)) {
+ ptr++;
+ }
+
+ /* skip comments and empty lines */
+ if (*ptr == '#' || *ptr == 0) {
+ continue;
+ }
+
+ cmd = ptr;
+ arg = strchr(cmd, ' ');
+ if (arg == NULL) {
+ arg = strchr(cmd, '\t');
+ }
+
+ if (arg == NULL) {
+ fprintf(stderr, Invalid config line:\n %s\n, line);
+ fclose(f);
+ errno = EINVAL;
+ return -1;
+ }
+
+ *arg = 0;
+ arg++;
+ while (isspace(*arg)) {
+ arg++;
+ }
+
+ argend = arg + strlen(arg);
+ while (arg != argend isspace(*(argend - 1))) {
+ argend--;
+ }
These while loops to skip spaces are
Re: [Qemu-devel] [PATCH v2 2/4] Add access control support to qemu bridge helper
On Fri, Oct 21, 2011 at 15:07, Corey Bryant cor...@linux.vnet.ibm.com wrote:
We go to great lengths to restrict ourselves to just cap_net_admin as an OS
enforced security mechanism. However, we further restrict what we allow users
to do to simply adding a tap device to a bridge interface by virtue of the
fact
that this is the only functionality we expose.
This is not good enough though. An administrator is likely to want to
restrict
the bridges that an unprivileged user can access, in particular, to restrict
an unprivileged user from putting a guest on what should be isolated networks.
This patch implements an ACL mechanism that is enforced by qemu-bridge-helper.
The ACLs are fairly simple whitelist/blacklist mechanisms with a wildcard of
'all'. All users are blacklisted by default, and deny takes precedence over
allow.
An interesting feature of this ACL mechanism is that you can include external
ACL files. The main reason to support this is so that you can set different
file system permissions on those external ACL files. This allows an
administrator to implement rather sophisicated ACL policies based on
user/group
sophisticated
policies via the file system.
As an example:
/etc/qemu/bridge.conf root:qemu 0640
allow br0
include /etc/qemu/alice.conf
include /etc/qemu/bob.conf
include /etc/qemu/charlie.conf
/etc/qemu/alice.conf root:alice 0640
allow br1
/etc/qemu/bob.conf root:bob 0640
allow br2
/etc/qemu/charlie.conf root:charlie 0640
deny all
I think syntax 'include /etc/qemu/user.d/*.conf' or 'includedir
/etc/qemu/user.d' could be also useful.
This ACL pattern allows any user in the qemu group to get a tap device
connected to br0 (which is bridged to the physical network).
Users in the alice group can additionally get a tap device connected to br1.
This allows br1 to act as a private bridge for the alice group.
Users in the bob group can additionally get a tap device connected to br2.
This allows br2 to act as a private bridge for the bob group.
Users in the charlie group cannot get a tap device connected to any bridge.
Under no circumstance can the bob group get access to br1 or can the alice
group get access to br2. And under no cicumstance can the charlie group
get access to any bridge.
Signed-off-by: Anthony Liguori aligu...@us.ibm.com
Signed-off-by: Richa Marwaha rmar...@linux.vnet.ibm.com
Signed-off-by: Corey Bryant cor...@linux.vnet.ibm.com
---
qemu-bridge-helper.c | 141
++
1 files changed, 141 insertions(+), 0 deletions(-)
diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 2ce82fb..db257d5 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -33,6 +33,105 @@
#include net/tap-linux.h
+#define MAX_ACLS (128)
If all users (or groups) in the system have an ACL, this number could
be way too low. Please use a list instead.
+#define DEFAULT_ACL_FILE CONFIG_QEMU_CONFDIR /bridge.conf
+
+enum {
+ ACL_ALLOW = 0,
+ ACL_ALLOW_ALL,
+ ACL_DENY,
+ ACL_DENY_ALL,
+};
+
+typedef struct ACLRule {
+ int type;
+ char iface[IFNAMSIZ];
+} ACLRule;
+
+static int parse_acl_file(const char *filename, ACLRule *acls, int
*pacl_count)
+{
+ int acl_count = *pacl_count;
+ FILE *f;
+ char line[4096];
+
+ f = fopen(filename, r);
+ if (f == NULL) {
+ return -1;
+ }
+
+ while (acl_count != MAX_ACLS
+ fgets(line, sizeof(line), f) != NULL) {
+ char *ptr = line;
+ char *cmd, *arg, *argend;
+
+ while (isspace(*ptr)) {
+ ptr++;
+ }
+
+ /* skip comments and empty lines */
+ if (*ptr == '#' || *ptr == 0) {
+ continue;
+ }
+
+ cmd = ptr;
+ arg = strchr(cmd, ' ');
+ if (arg == NULL) {
+ arg = strchr(cmd, '\t');
+ }
+
+ if (arg == NULL) {
+ fprintf(stderr, Invalid config line:\n %s\n, line);
+ fclose(f);
+ errno = EINVAL;
+ return -1;
+ }
+
+ *arg = 0;
+ arg++;
+ while (isspace(*arg)) {
+ arg++;
+ }
+
+ argend = arg + strlen(arg);
+ while (arg != argend isspace(*(argend - 1))) {
+ argend--;
+ }
These while loops to skip spaces are repeated, but the comment
skipping part is not, so it is not possible to have comments after
rules or split rules to several lines. I'd add a simple state variable
to track at which stage we are in parsing instead.
+ *argend = 0;
+
+ if (strcmp(cmd, deny) == 0) {
+ if (strcmp(arg, all) == 0) {
+ acls[acl_count].type = ACL_DENY_ALL;
+ } else {
+ acls[acl_count].type = ACL_DENY;
+ snprintf(acls[acl_count].iface, IFNAMSIZ, %s, arg);
+ }
+ acl_count++;
+ } else if
[Qemu-devel] [PATCH v2 2/4] Add access control support to qemu bridge helper
We go to great lengths to restrict ourselves to just cap_net_admin as an OS
enforced security mechanism. However, we further restrict what we allow users
to do to simply adding a tap device to a bridge interface by virtue of the fact
that this is the only functionality we expose.
This is not good enough though. An administrator is likely to want to restrict
the bridges that an unprivileged user can access, in particular, to restrict
an unprivileged user from putting a guest on what should be isolated networks.
This patch implements an ACL mechanism that is enforced by qemu-bridge-helper.
The ACLs are fairly simple whitelist/blacklist mechanisms with a wildcard of
'all'. All users are blacklisted by default, and deny takes precedence over
allow.
An interesting feature of this ACL mechanism is that you can include external
ACL files. The main reason to support this is so that you can set different
file system permissions on those external ACL files. This allows an
administrator to implement rather sophisicated ACL policies based on user/group
policies via the file system.
As an example:
/etc/qemu/bridge.conf root:qemu 0640
allow br0
include /etc/qemu/alice.conf
include /etc/qemu/bob.conf
include /etc/qemu/charlie.conf
/etc/qemu/alice.conf root:alice 0640
allow br1
/etc/qemu/bob.conf root:bob 0640
allow br2
/etc/qemu/charlie.conf root:charlie 0640
deny all
This ACL pattern allows any user in the qemu group to get a tap device
connected to br0 (which is bridged to the physical network).
Users in the alice group can additionally get a tap device connected to br1.
This allows br1 to act as a private bridge for the alice group.
Users in the bob group can additionally get a tap device connected to br2.
This allows br2 to act as a private bridge for the bob group.
Users in the charlie group cannot get a tap device connected to any bridge.
Under no circumstance can the bob group get access to br1 or can the alice
group get access to br2. And under no cicumstance can the charlie group
get access to any bridge.
Signed-off-by: Anthony Liguori aligu...@us.ibm.com
Signed-off-by: Richa Marwaha rmar...@linux.vnet.ibm.com
Signed-off-by: Corey Bryant cor...@linux.vnet.ibm.com
---
qemu-bridge-helper.c | 141 ++
1 files changed, 141 insertions(+), 0 deletions(-)
diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 2ce82fb..db257d5 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -33,6 +33,105 @@
#include net/tap-linux.h
+#define MAX_ACLS (128)
+#define DEFAULT_ACL_FILE CONFIG_QEMU_CONFDIR /bridge.conf
+
+enum {
+ACL_ALLOW = 0,
+ACL_ALLOW_ALL,
+ACL_DENY,
+ACL_DENY_ALL,
+};
+
+typedef struct ACLRule {
+int type;
+char iface[IFNAMSIZ];
+} ACLRule;
+
+static int parse_acl_file(const char *filename, ACLRule *acls, int *pacl_count)
+{
+int acl_count = *pacl_count;
+FILE *f;
+char line[4096];
+
+f = fopen(filename, r);
+if (f == NULL) {
+return -1;
+}
+
+while (acl_count != MAX_ACLS
+fgets(line, sizeof(line), f) != NULL) {
+char *ptr = line;
+char *cmd, *arg, *argend;
+
+while (isspace(*ptr)) {
+ptr++;
+}
+
+/* skip comments and empty lines */
+if (*ptr == '#' || *ptr == 0) {
+continue;
+}
+
+cmd = ptr;
+arg = strchr(cmd, ' ');
+if (arg == NULL) {
+arg = strchr(cmd, '\t');
+}
+
+if (arg == NULL) {
+fprintf(stderr, Invalid config line:\n %s\n, line);
+fclose(f);
+errno = EINVAL;
+return -1;
+}
+
+*arg = 0;
+arg++;
+while (isspace(*arg)) {
+arg++;
+}
+
+argend = arg + strlen(arg);
+while (arg != argend isspace(*(argend - 1))) {
+argend--;
+}
+*argend = 0;
+
+if (strcmp(cmd, deny) == 0) {
+if (strcmp(arg, all) == 0) {
+acls[acl_count].type = ACL_DENY_ALL;
+} else {
+acls[acl_count].type = ACL_DENY;
+snprintf(acls[acl_count].iface, IFNAMSIZ, %s, arg);
+}
+acl_count++;
+} else if (strcmp(cmd, allow) == 0) {
+if (strcmp(arg, all) == 0) {
+acls[acl_count].type = ACL_ALLOW_ALL;
+} else {
+acls[acl_count].type = ACL_ALLOW;
+snprintf(acls[acl_count].iface, IFNAMSIZ, %s, arg);
+}
+acl_count++;
+} else if (strcmp(cmd, include) == 0) {
+/* ignore errors */
+parse_acl_file(arg, acls, acl_count);
+} else {
+fprintf(stderr, Unknown command `%s'\n, cmd);
+fclose(f);
+errno = EINVAL;
+return -1;
+}
+}
+
+*pacl_count = acl_count;
+
+fclose(f);
+
+return 0;
+}
+
static int
