From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:58616) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RIKpv-0007AU-SJ for qemu-devel@nongnu.org; Mon, 24 Oct 2011 09:45:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RIKpt-0007zF-KP for qemu-devel@nongnu.org; Mon, 24 Oct 2011 09:45:07 -0400 Received: from e3.ny.us.ibm.com ([32.97.182.143]:41047) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RIKpt-0007yb-Aj for qemu-devel@nongnu.org; Mon, 24 Oct 2011 09:45:05 -0400 Received: from /spool/local by e3.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 24 Oct 2011 09:44:54 -0400 Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p9ODilwk273266 for ; Mon, 24 Oct 2011 09:44:47 -0400 Received: from d03av03.boulder.ibm.com (loopback [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p9ODikqC019004 for ; Mon, 24 Oct 2011 07:44:47 -0600 Message-ID: <4EA56BCD.1070202@linux.vnet.ibm.com> Date: Mon, 24 Oct 2011 09:44:45 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1319209643-3866-1-git-send-email-coreyb@linux.vnet.ibm.com> <1319209643-3866-3-git-send-email-coreyb@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/4] Add access control support to qemu bridge helper List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: rmarwah@linux.vnet.ibm.com, aliguori@us.ibm.com, qemu-devel@nongnu.org On 10/23/2011 09:10 AM, Blue Swirl wrote: > On Fri, Oct 21, 2011 at 15:07, Corey Bryant 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 Liguori >> > Signed-off-by: Richa Marwaha >> > Signed-off-by: Corey Bryant >> > --- >> > 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 necessary right now. I really like the simplicity we have with the existing approach. >> > + *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 has_vnet_hdr(int fd) >> > { >> > unsigned int features = 0; >> > @@ -95,6 +194,9 @@ int main(int argc, char **argv) >> > const char *bridge; >> > char iface[IFNAMSIZ]; >> > int index; >> > + ACLRule acls[MAX_ACLS]; >> > + int acl_count = 0; >> > + int i, access_allowed, access_denied; >> > >> > /* parse arguments */ >> > if (argc< 3 || argc> 4) { >> > @@ -115,6 +217,45 @@ int main(int argc, char **argv) >> > bridge = argv[index++]; >> > unixfd = atoi(argv[index++]); >> > >> > + /* parse default acl file */ >> > + if (parse_acl_file(DEFAULT_ACL_FILE, acls,&acl_count) == -1) { >> > + fprintf(stderr, "failed to parse default acl file `%s'\n", >> > + DEFAULT_ACL_FILE); >> > + return -errno; >> > + } >> > + >> > + /* validate bridge against acl -- default policy is to deny >> > + * according acl policy if we have a deny and allow both >> > + * then deny should always win over allow >> > + */ >> > + access_allowed = 0; >> > + access_denied = 0; >> > + for (i = 0; i< acl_count; i++) { >> > + switch (acls[i].type) { >> > + case ACL_ALLOW_ALL: >> > + access_allowed = 1; >> > + break; >> > + case ACL_ALLOW: >> > + if (strcmp(bridge, acls[i].iface) == 0) { >> > + access_allowed = 1; >> > + } >> > + break; >> > + case ACL_DENY_ALL: >> > + access_denied = 1; >> > + break; >> > + case ACL_DENY: >> > + if (strcmp(bridge, acls[i].iface) == 0) { >> > + access_denied = 1; >> > + } >> > + break; >> > + } >> > + } >> > + >> > + if ((access_allowed == 0) || (access_denied == 1)) { >> > + fprintf(stderr, "access denied by acl file\n"); >> > + return -EPERM; >> > + } >> > + >> > /* open a socket to use to control the network interfaces */ >> > ctlfd = socket(AF_INET, SOCK_STREAM, 0); >> > if (ctlfd == -1) { >> > -- >> > 1.7.3.4 >> > >> > >> > -- Regards, Corey