Hi Krzysztof,
On Tue, Jan 27, 2009 at 05:50:08PM +0100, Krzysztof Piotr Oledzki wrote:
> >From 5368532099b8e8c2c6970df8a2d1463a7eaa72bc Mon Sep 17 00:00:00 2001
> From: Krzysztof Piotr Oledzki <[email protected]>
> Date: Tue, 27 Jan 2009 16:47:15 +0100
> Subject: [MEDIUM] access control (block) rework - rfc
>
> Allow more advanced access control:
> - rename 'block' to 'deny'
> - implement 'allow'
> - implement 'order': allow,deny and deny,allow (default)
> - implement new check_cond_list() helper function
>
> RFC quality patch.
I'm interested by your change, but not 100% because of the "order" addition.
I've come across many cases where it's not possible to write some ACLs
using only this "order" approach. As soon as you have three conflicting
sets, you cannot write the config.
Example : you want to allow access to your application from the company,
but not from your training room, except the instructor's PC. So you want
to allow 192.168.0.0/16 except 192.168.0.0/24 but you still want to allow
192.168.0.1.
With sequential rules, it could be written that way :
allow 192.168.0.1
deny 192.168.0.0/24
allow 192.168.0.0/16
deny 0.0.0.0/0
With "order", you're blocked.
After seeing many configurations that people write, I have realized that
what is really needed is this sequential ordering, but not only with
allow/deny, but also with some actions such as use_backend, redirect,
tarpit and things like this. In fact, everything which is acting on the
traffic's destination.
However, there was a long term project of supporting ACLs from files.
I have still not worked on this because I don't know how those should
be handled. If we only put values there, it's limited to little usages
such as blacklisting. If we put more complex rules, the files will be
hard to write and might often prevent the process from starting up due
to syntax errors.
But with your order idea, it might get simpler. People who need ACLs
from files are basically ISPs or large network admins. They don't
want complex configs, they want to enumerate the hosts and network
which are granted some access or denied some access. So the principle
of the order could help them write two sets files, one of which
enumerates allowed hosts, and another one enumerating denied networks,
for instance. The config could look something like this :
acl admin src 192.168.0.1
acl visitors src 192.168.0.0/24
acl monitoring src 172.16.0.0/24
allow if admin or monitoring
deny if visitors
access allow /etc/acl/allow.acl deny /etc/acl/deny.acl
The "access" options (allow, deny) would then tell in which order
to check the allow and deny files.
Also BTW, there's something else I had planned. Right now you cannot
write rules without an ACL for whatever object. While this is handy
for complex configs, this is very boring for small configs. I'd like
to be able to use the verb as a function in the rules.
For instance, instead of always having to write :
acl admin src 192.168.0.1
allow if admin
you could write :
allow if src(192.168.0.1)
The last changes I made to the rules in order to add better checks
and typing have made that a lot easier. It would of course prevent
caching, as "src(192.168.0.1)" would automatically be compiled into
an implicit ACL, but caching is still not implemented, and people
who would need caching will certainly not use this anyway because
they'd want to factor out common information.
I still have a lot of drafts on paper that I need to write down in
files and drawings about the proposed evolution of content processing.
Next week I'm on holidays and I hope to be able to advance on the
doc and on publishing all those old thoughts.
That said, I'm clearly in favor of "deny" instead of "block" and
adding "allow" ;-)
Feel free to comment, as I think we're still lacking a lot of features
in this area, and at the same time it's the one that we cannot break
with newer versions due to the complexity of some setups. I've seen
configs with hundreds of "reqiallow"/"reqideny" that will almost
certainly be converted to the ACLs due to the huge amount of work.
I don't want to fall in that trap again.
Best regards,
Willy
> TODO: documentation
> ---
> include/proto/acl.h | 5 +++++
> include/types/proxy.h | 7 ++++++-
> src/acl.c | 19 +++++++++++++++++++
> src/cfgparse.c | 33 +++++++++++++++++++++++++++++----
> src/haproxy.c | 8 +++++++-
> src/proto_http.c | 39 +++++++++++++++++++++++++--------------
> 6 files changed, 91 insertions(+), 20 deletions(-)
>
> diff --git a/include/proto/acl.h b/include/proto/acl.h
> index fffce48..8ccc8f0 100644
> --- a/include/proto/acl.h
> +++ b/include/proto/acl.h
> @@ -92,6 +92,11 @@ int acl_exec_cond(struct acl_cond *cond, struct proxy *px,
> struct session *l4, v
> */
> struct acl *cond_find_require(struct acl_cond *cond, unsigned int require);
>
> +/* Walk through <cond_list> and return 0 if all tests fail or 1 if at last
> one test succeeds.
> + * This function checks the polarity required by IF/UNLESS.
> + */
> +int check_cond_list(struct list *cond_list, struct proxy *px, struct session
> *l4, void *l7, int dir);
> +
> /* Return a pointer to the ACL <name> within the list starting at <head>, or
> * NULL if not found.
> */
> diff --git a/include/types/proxy.h b/include/types/proxy.h
> index 5d0869c..b70a7e4 100644
> --- a/include/types/proxy.h
> +++ b/include/types/proxy.h
> @@ -112,6 +112,9 @@
> #define PR_O2_SPLIC_AUT 0x00000004 /* automatically use linux
> kernel's splice() */
> #define PR_O2_SPLIC_ANY
> (PR_O2_SPLIC_REQ|PR_O2_SPLIC_RTR|PR_O2_SPLIC_AUT)
>
> +#define PR_ORDER_DENY_ALLOW 0 /* last type of match wins: first
> evaluate deny_cond then allow_cond, allow by default */
> +#define PR_ORDER_ALLOW_DENY 1 /* last type of match wins: first
> evaluate allow_cond then deny_cond, deny by default */
> +
> /* This structure is used to apply fast weighted round robin on a server
> group */
> struct fwrr_group {
> struct eb_root curr; /* tree for servers in "current" time range */
> @@ -130,13 +133,15 @@ struct proxy {
> int options; /* PR_O_REDISP, PR_O_TRANSP,
> ... */
> int options2; /* PR_O2_* */
> int mode; /* mode = PR_MODE_TCP,
> PR_MODE_HTTP or PR_MODE_HEALTH */
> + int order; /* order = PR_ORDER_DENY_ALLOW
> or PR_ORDER_ALLOW_DENY */
> struct sockaddr_in dispatch_addr; /* the default address to
> connect to */
> union {
> struct proxy *be; /* default backend, or NULL if
> none set */
> char *name; /* default backend name during
> config parse */
> } defbe;
> struct list acl; /* ACL declared on this proxy */
> - struct list block_cond; /* early blocking conditions
> (chained) */
> + struct list allow_cond; /* early allow blocking
> conditions (chained) */
> + struct list deny_cond; /* early deny blocking
> conditions (chained) */
> struct list redirect_rules; /* content redirecting rules
> (chained) */
> struct list switching_rules; /* content switching rules
> (chained) */
> struct { /* TCP request processing */
> diff --git a/src/acl.c b/src/acl.c
> index ed41e91..8cab8df 100644
> --- a/src/acl.c
> +++ b/src/acl.c
> @@ -1119,6 +1119,25 @@ struct acl *cond_find_require(struct acl_cond *cond,
> unsigned int require)
> return NULL;
> }
>
> +/* Walk through <cond_list> and return 0 if all tests fail or 1 if at last
> one test succeeds.
> + * This function checks the polarity required by IF/UNLESS.
> + */
> +int check_cond_list(struct list *cond_list, struct proxy *px, struct session
> *l4, void *l7, int dir) {
> +
> + struct acl_cond *cond;
> +
> + list_for_each_entry(cond, cond_list, list) {
> + int ret = acl_exec_cond(cond, px, l4, l7, dir);
> +
> + if (cond->pol == ACL_COND_UNLESS)
> + ret = !ret;
> +
> + if (ret)
> + return 1;
> + }
> +
> + return 0;
> +}
>
> /************************************************************************/
> /* All supported keywords must be declared here. */
> diff --git a/src/cfgparse.c b/src/cfgparse.c
> index f98d07e..46f4ba7 100644
> --- a/src/cfgparse.c
> +++ b/src/cfgparse.c
> @@ -561,13 +561,15 @@ static void init_default_instance()
> memset(&defproxy, 0, sizeof(defproxy));
> defproxy.mode = PR_MODE_TCP;
> defproxy.state = PR_STNEW;
> + defproxy.order = PR_ORDER_DENY_ALLOW;
> defproxy.maxconn = cfg_maxpconn;
> defproxy.conn_retries = CONN_RETRIES;
> defproxy.logfac1 = defproxy.logfac2 = -1; /* log disabled */
>
> LIST_INIT(&defproxy.pendconns);
> LIST_INIT(&defproxy.acl);
> - LIST_INIT(&defproxy.block_cond);
> + LIST_INIT(&defproxy.allow_cond);
> + LIST_INIT(&defproxy.deny_cond);
> LIST_INIT(&defproxy.mon_fail_cond);
> LIST_INIT(&defproxy.switching_rules);
>
> @@ -641,7 +643,8 @@ int cfg_parse_listen(const char *file, int linenum, char
> **args, int inv)
> proxy = curproxy;
> LIST_INIT(&curproxy->pendconns);
> LIST_INIT(&curproxy->acl);
> - LIST_INIT(&curproxy->block_cond);
> + LIST_INIT(&curproxy->allow_cond);
> + LIST_INIT(&curproxy->deny_cond);
> LIST_INIT(&curproxy->redirect_rules);
> LIST_INIT(&curproxy->mon_fail_cond);
> LIST_INIT(&curproxy->switching_rules);
> @@ -668,6 +671,7 @@ int cfg_parse_listen(const char *file, int linenum, char
> **args, int inv)
> curproxy->state = defproxy.state;
> curproxy->options = defproxy.options;
> curproxy->options2 = defproxy.options2;
> + curproxy->order = defproxy.order;
> curproxy->lbprm.algo = defproxy.lbprm.algo;
> curproxy->except_net = defproxy.except_net;
> curproxy->except_mask = defproxy.except_mask;
> @@ -1109,7 +1113,19 @@ int cfg_parse_listen(const char *file, int linenum,
> char **args, int inv)
> }
> curproxy->conn_retries = atol(args[1]);
> }
> - else if (!strcmp(args[0], "block")) { /* early blocking based on ACLs
> */
> + else if (!strcmp(args[0], "order")) {
> + if (!strcasecmp(args[1], "allow,deny"))
> + curproxy->order = PR_ORDER_ALLOW_DENY;
> + else if (!strcasecmp(args[1], "deny,allow"))
> + curproxy->order = PR_ORDER_DENY_ALLOW;
> + else {
> + Alert("parsing [%s:%d]: unknow order '%s'.\n",
> + file, linenum, args[1]);
> +
> + return -1;
> + }
> + }
> + else if (!strcmp(args[0], "block") || !strcmp(args[0], "allow") ||
> !strcmp(args[0], "deny")) { /* early blocking based on ACLs */
> int pol = ACL_COND_NONE;
> struct acl_cond *cond;
>
> @@ -1134,8 +1150,17 @@ int cfg_parse_listen(const char *file, int linenum,
> char **args, int inv)
> file, linenum);
> return -1;
> }
> +
> cond->line = linenum;
> - LIST_ADDQ(&curproxy->block_cond, &cond->list);
> +
> + if (!strcmp(args[0], "block"))
> + Alert("parsing [%s:%d]: '%s' is deprecated, please use
> 'deny' instead.\n",
> + file, linenum, args[0]);
> +
> + if (!strcmp(args[0], "allow"))
> + LIST_ADDQ(&curproxy->allow_cond, &cond->list);
> + else
> + LIST_ADDQ(&curproxy->deny_cond, &cond->list);
> }
> else if (!strcmp(args[0], "redirect")) {
> int pol = ACL_COND_NONE;
> diff --git a/src/haproxy.c b/src/haproxy.c
> index abf6eef..e7b8855 100644
> --- a/src/haproxy.c
> +++ b/src/haproxy.c
> @@ -687,7 +687,13 @@ void deinit(void)
> for (i = 0; i < p->nb_rspadd; i++)
> free(p->rsp_add[i]);
>
> - list_for_each_entry_safe(cond, condb, &p->block_cond, list) {
> + list_for_each_entry_safe(cond, condb, &p->allow_cond, list) {
> + LIST_DEL(&cond->list);
> + prune_acl_cond(cond);
> + free(cond);
> + }
> +
> + list_for_each_entry_safe(cond, condb, &p->deny_cond, list) {
> LIST_DEL(&cond->list);
> prune_acl_cond(cond);
> free(cond);
> diff --git a/src/proto_http.c b/src/proto_http.c
> index ecbc887..34399aa 100644
> --- a/src/proto_http.c
> +++ b/src/proto_http.c
> @@ -1847,10 +1847,10 @@ int http_process_request(struct session *s, struct
> buffer *req)
> */
>
> do {
> - struct acl_cond *cond;
> struct redirect_rule *rule;
> struct proxy *rule_set = s->be;
> cur_proxy = s->be;
> + int denyreq = 0;
>
> /* first check whether we have some ACLs set to redirect this
> request */
> list_for_each_entry(rule, &cur_proxy->redirect_rules, list) {
> @@ -1958,21 +1958,32 @@ int http_process_request(struct session *s, struct
> buffer *req)
> }
> }
>
> - /* first check whether we have some ACLs set to block this
> request */
> - list_for_each_entry(cond, &cur_proxy->block_cond, list) {
> - int ret = acl_exec_cond(cond, cur_proxy, s, txn,
> ACL_DIR_REQ);
> + /* check whether we should block this request */
> + if (cur_proxy->order == PR_ORDER_ALLOW_DENY) {
> + denyreq = 1;
>
> - ret = acl_pass(ret);
> - if (cond->pol == ACL_COND_UNLESS)
> - ret = !ret;
> + if (check_cond_list(&cur_proxy->allow_cond, cur_proxy,
> s, txn, ACL_DIR_REQ))
> + denyreq = 0;
>
> - if (ret) {
> - txn->status = 403;
> - /* let's log the request time */
> - s->logs.tv_request = now;
> - stream_int_retnclose(req->prod,
> error_message(s, HTTP_ERR_403));
> - goto return_prx_cond;
> - }
> + if (check_cond_list(&cur_proxy->deny_cond, cur_proxy,
> s, txn, ACL_DIR_REQ))
> + denyreq = 1;
> +
> + } else {
> +
> + if (check_cond_list(&cur_proxy->deny_cond, cur_proxy,
> s, txn, ACL_DIR_REQ))
> + denyreq = 1;
> +
> + if (check_cond_list(&cur_proxy->allow_cond, cur_proxy,
> s, txn, ACL_DIR_REQ))
> + denyreq = 0;
> +
> + }
> +
> + if (denyreq) {
> + txn->status = 403;
> + /* let's log the request time */
> + s->logs.tv_request = now;
> + stream_int_retnclose(req->prod, error_message(s,
> HTTP_ERR_403));
> + goto return_prx_cond;
> }
>
> /* try headers filters */
> --
> 1.6.0.4
>
