nicolasff / webdis

A Redis HTTP interface with JSON output
https://webd.is
BSD 2-Clause "Simplified" License
2.83k stars 304 forks source link

ACL config disable ["*"] supress all enabled commands #18

Closed fabware closed 13 years ago

fabware commented 13 years ago

I want to enable just "PUBLISH" command for one host. I suppose the following configuration will work, which is not.

"acl": [ { "ip": "127.0.0.1", "disabled": ["*"], "enabled": ["PUBLISH"] } ]

403 got when try to PUBLISH.

It should work like nginx deny all and allow one directive.

nicolasff commented 13 years ago

Hello,

Disabled always has the priority over enabled, and you can put them in any order (it's an object, not an ordered list). You can add a block with "disabled": "*", followed by another block with "ip": "127.0.0.1", "enabled": ["PUBLISH"].

fabware commented 13 years ago

Thanks for your comment.

The following config works for 127.0.0.1, but it also allows other host(eg: 192.168.1.99) to have the same permission. I'd like to only allow access for 127.0.0.1 and prohibit the others, how would I do it?

           {
                    "ip": "127.0.0.1",
                    "disabled": ["*"]
            },
            {
                    "ip": "127.0.0.1",
                    "enabled": ["PUBLISH"]
            }
nicolasff commented 13 years ago

Try the following:

            {
                    "disabled": ["*"]
            },
            {
                    "ip": "127.0.0.1",
                    "enabled": ["PUBLISH"]
            }
fabware commented 13 years ago

Hi,

It's not working, I believe there is a bug. Other hosts have the same access permission as 127.0.0.1.

result:

127.0.0.1 SET: 403 PUBLISH: 200 192.168.1.99 SET: 403 PUBLISH: 200

nicolasff commented 13 years ago

Hi. Thanks for trying it out, you must be right. Unfortunately I'm traveling at the moment and won't be able to fix it before the 22nd of May. I'd be happy to accept a patch if you manage to find the issue, otherwise I'm afraid you'll have to wait. One last idea: try to use "ip": "0.0.0.0/32" in the "disabled": "*" block to match all clients.

Sorry about that.

Nicolas

fabware commented 13 years ago

I appreciate you are looking into this while you are traveling. Your last idea won't work, 127.0.0.1 got permission to issue other commands other than PUBLISH in that case. I think the ACL part needs more tests, it's a core part of production use.

I'll try to fix it by myself if I can make it right. Wish you happy traveling.

fabware commented 13 years ago

Hi, Nicolas

For my case, I get it work with the following patch. I never tested with other case.

patch

diff -u -r /mnt/hgfs/share/webdis/acl.c webdis/acl.c
--- /mnt/hgfs/share/webdis/acl.c    2011-05-11 17:00:34.000000000 +0800
+++ webdis/acl.c    2011-05-13 19:27:42.000000000 +0800
@@ -42,7 +42,7 @@
    char *always_off[] = {"MULTI", "EXEC", "WATCH", "DISCARD"};

    unsigned int i;
-   int authorized = 1;
+   int authorized = 0;
    struct acl *a;

    in_addr_t client_addr;
diff -u -r /mnt/hgfs/share/webdis/conf.c webdis/conf.c
--- /mnt/hgfs/share/webdis/conf.c   2011-05-11 17:00:34.000000000 +0800
+++ webdis/conf.c   2011-05-13 19:28:17.000000000 +0800
@@ -150,7 +150,7 @@
            mask_bits = (unsigned short)atoi(p+1);
        }
        a->cidr.enabled = 1;
-       a->cidr.mask = (mask_bits == 0 ? 0 : (0xffffffff << (32 - mask_bits)));
+       a->cidr.mask = (mask_bits == 0 ? 0xffffffff : (0xffffffff << (32 - mask_bits)));
        a->cidr.subnet = ntohl(inet_addr(ip)) & a->cidr.mask;
        free(ip);
    }
nicolasff commented 13 years ago

I pushed a fix, it should work now. Please let me know if you still have problems.

fabware commented 13 years ago

It working fine with the following config:

            {
                    "disabled": ["*"]
            },
            {
                    "ip": "127.0.0.1",
                    "enabled": ["PUBLISH"]
            }

Thanks a lot.