Hi Willy,
On Mon, Jan 16, Willy Tarreau wrote:
> On Fri, Jan 13, 2017 at 07:28:38PM +0200, Jarno Huuskonen wrote:
> > This is my first attempt in adding deny_status to:
> > http-request tarpit [deny_status <status>]
> >
> > First patch updates parse_http_req_cond so config parser accepts
> > [deny_status <status>] for http-request tarpit (and sets
> > rule->deny_status).
> >
> > The second patch updates http_process_req_common/http_process_tarpit to
> > use deny_status. http_process_tarpit has a switch statement for mapping
> > txn->status (200<->504) to HTTP_ERR_<num>(enum). Is this reasonable ?
>
> Yes it's reasonable however it would be a good idea to instead write a
> short function in charge of returning this index based on the status code
> and then to use it everywhere we need this index, including in your own
> code. When you look at http_error_message(), everywhere it's called,
> txn->status already contains the status code, except in one place :
> http_return_srv_error() where we set the status code. Thus I'd proceed
> like this :
>
> 1) change http_server_error() to set the status code separately from the
> message :
>
> if (status > 0)
> s->txn->status = status;
> if (msg)
> bo_inject(si_ic(si), msg->str, msg->len);
>
> 2) implement your switch/case as a function (eg: http_get_status_idx())
Does it make sense to try to optimize the switch statement
and have most common status codes at the top ? Something like
switch(txn->status) {
case 500:
http_err_code_idx = HTTP_ERR_500;
break;
case 403:
http_err_code_idx = HTTP_ERR_403;
break;
...
> 3) make http_error_message() pass s->txn->status to htpt_get_status_idx()
> to retrieve the message index, and drop its second argument ; modify
> http_return_srv_error() to pass NULL to all calls to http_server_error()
> instead of http_error_message(), and add a final call in this function
> to do this :
>
> bo_putchk(&s->res, http_error_message(s));
>
> This way adding new status codes will be more straight forward and will
> not require to touch multiple places all the time.
Sounds good, I'll (try to) work on this over the weekend.
> However I'm seeing the config parser become really ugly around the
> deny_status part (it already was but now it's worse).
>
> What would you think about removing the rule->deny_status = HTTP_ERR_403
> entry that's hidden before the rules are parsed, and to place this and
> the action this way under the deny/block/tarpit "if" block :
>
> + if (!strcmp(args[0], "tarpit")) {
> + rule->action = ACT_HTTP_REQ_TARPIT;
> + rule->deny_status = HTTP_ERR_500;
> + }
> + else {
> + rule->action = ACT_ACTION_DENY;
> + rule->deny_status = HTTP_ERR_403;
> + }
>
> Also, you can simplify this part :
>
> > @@ -9031,13 +9035,10 @@ struct act_rule *parse_http_req_cond(const char
> > **args, const char *file, int li
> > }
> >
> > if (hc >= HTTP_ERR_SIZE) {
> > - Warning("parsing [%s:%d] : status code %d
> > not handled, using default code 403.\n",
> > - file, linenum, code);
> > + Warning("parsing [%s:%d] : status code %d
> > not handled, using default code %d.\n",
> > + file, linenum, code, rule->action
> > == ACT_ACTION_DENY ? 403: 500);
> > }
>
> Simply pass "rule->deny_status" instead of the condition, you'll always
> report the status that is programmed to be returned, it will always be
> more accurate and makes the code simpler.
I'm including a new patch for the parser changes. Is this what you had
in mind for the parser ?
-Jarno
--
Jarno Huuskonen
>From e3140bc1a52ffee8a79e87c086ae456cc8524ed6 Mon Sep 17 00:00:00 2001
From: Jarno Huuskonen <[email protected]>
Date: Wed, 1 Feb 2017 15:38:09 +0200
Subject: [PATCH] parse_http_req_cond: parse deny_status code for http-request
tarpit. DOC: http-request tarpit deny_status.
To: [email protected]
X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4
---
doc/configuration.txt | 7 ++++---
src/proto_http.c | 19 +++++++++++--------
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index c2ede71..23c6cfe 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -3646,8 +3646,8 @@ http-check send-state
See also : "option httpchk", "http-check disable-on-404"
-http-request { allow | tarpit | auth [realm <realm>] | redirect <rule> |
- deny [deny_status <status>] |
+http-request { allow | auth [realm <realm>] | redirect <rule> |
+ tarpit [deny_status <status>] | deny [deny_status <status>] |
add-header <name> <fmt> | set-header <name> <fmt> |
capture <sample> [ len <length> | id <id> ] |
del-header <name> | set-nice <nice> | set-log-level <level> |
@@ -3691,7 +3691,8 @@ http-request { allow | tarpit | auth [realm <realm>] |
redirect <rule> |
- "tarpit" : this stops the evaluation of the rules and immediately blocks
the request without responding for a delay specified by "timeout tarpit"
or "timeout connect" if the former is not set. After that delay, if the
- client is still connected, an HTTP error 500 is returned so that the
+ client is still connected, an HTTP error 500 (or optionally the status
+ code specified as an argument to "deny_status") is returned so that the
client does not suspect it has been tarpitted. Logs will report the flags
"PT". The goal of the tarpit rule is to slow down robots during an attack
when they're limited on the number of concurrent requests. It can be very
diff --git a/src/proto_http.c b/src/proto_http.c
index 3490aa7..e63df73 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -9039,15 +9039,21 @@ struct act_rule *parse_http_req_cond(const char **args,
const char *file, int li
goto out_err;
}
- rule->deny_status = HTTP_ERR_403;
if (!strcmp(args[0], "allow")) {
rule->action = ACT_ACTION_ALLOW;
cur_arg = 1;
- } else if (!strcmp(args[0], "deny") || !strcmp(args[0], "block")) {
+ } else if (!strcmp(args[0], "deny") || !strcmp(args[0], "block") ||
!strcmp(args[0], "tarpit")) {
int code;
int hc;
- rule->action = ACT_ACTION_DENY;
+ if (!strcmp(args[0], "tarpit")) {
+ rule->action = ACT_HTTP_REQ_TARPIT;
+ rule->deny_status = HTTP_ERR_500;
+ }
+ else {
+ rule->action = ACT_ACTION_DENY;
+ rule->deny_status = HTTP_ERR_403;
+ }
cur_arg = 1;
if (strcmp(args[cur_arg], "deny_status") == 0) {
cur_arg++;
@@ -9067,13 +9073,10 @@ struct act_rule *parse_http_req_cond(const char **args,
const char *file, int li
}
if (hc >= HTTP_ERR_SIZE) {
- Warning("parsing [%s:%d] : status code %d not
handled, using default code 403.\n",
- file, linenum, code);
+ Warning("parsing [%s:%d] : status code %d not
handled, using default code %d.\n",
+ file, linenum, code,
http_err_codes[rule->deny_status]);
}
}
- } else if (!strcmp(args[0], "tarpit")) {
- rule->action = ACT_HTTP_REQ_TARPIT;
- cur_arg = 1;
} else if (!strcmp(args[0], "auth")) {
rule->action = ACT_HTTP_REQ_AUTH;
cur_arg = 1;
--
1.8.3.1
