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 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
>>
>
> 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 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.
>>
>
> 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.
It's not necessary, more like cleanup _if_ it turns out to be even simpler.
>>> > + *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
>
