Hi Jarno,
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())
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.
> Do tarpitted (http-request tarpit / req(i)tarpit) requests always
> go thru http_process_req_common (so txn->status is always set to
> txn->status = http_err_codes[deny_status]) ?
I think so, yes.
> Or is there a possibility that int deny_status could be overwritten
> in http_process_req_common (with multiple deny/block/tarpit rules) ?
Rules evaluation is stopped when you have a deny/block/tarpit so it
will not be overwritten.
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.
Otherwise it looks good.
Thanks!
Willy
