Fix unstable JS challange passing #1077
Conversation
- Fix incorrect calculation of allowed time frame for a new request. 'delay_range' was stored in Tempesta in jiffies while it was used in msecs in JS code. That could lead to defferent calculation results on TempestaFW and client. - Don't close connection, if non-chalengeable resource was requested before passing JS challenge.
tempesta_fw/http_sess.c
Outdated
| * @delay_limit - maximum timeout between client's requests to pass the | ||
| * challenge, in jiffies; | ||
| * @delay_range - allowed time range for client to make a repeated request, | ||
| * in msecs; |
There was a problem hiding this comment.
Perhaps maximum range for random timeout added to delay_min or something like that would be more appropriate description for delay_range field (the same for Wiki).
| } | ||
| if (limit < min_limit) { | ||
| else if (tfw_cfg_js_ch->delay_limit < min_limit) { | ||
| TFW_WARN_NL("%s: delay limit is too low, " |
There was a problem hiding this comment.
Why not just exit with configuration error here? It seems that min_limit is too small value for tfw_cfg_js_ch->delay_limit anyway, and will lead to rejections of legitimate clients.
There was a problem hiding this comment.
That was a requirement of the JS Challenge task: #536 (comment)
Configuration process must enforce the value to be greater than delay_range + delay_min and show warning if it's less than delay_range + delay_min + 100. Default value is delay_range + delay_min + 1000 (enough ever fro cross atlantic connections and relatively slow hardware and software).
The last moment of time browser can send a request - delay_range + delay_min, and some time is required to deliver the request. min_limit adds extra 0.1s. Yes, this configuration is not very reliable: delivering via cross-Atlantic is slower, browsers can be slow, etc., while default limit (+1s) is reliable enough. But user may want to use less or more strict time lines and we should honour his choices.
|
|
||
| if (!tfw_cfg_js_ch) | ||
| return 0; | ||
|
|
||
| cur_time = jiffies; | ||
| min_time = sv->ts + tfw_cfg_js_ch->delay_min | ||
| + sv->ts % tfw_cfg_js_ch->delay_range; | ||
| + msecs_to_jiffies(sv->ts % tfw_cfg_js_ch->delay_range); |
There was a problem hiding this comment.
I see, that this non-trivial time calculation is intended for consistency with calculations in js_challenge.js.tpl, but an explanatory comment would be appropriate here.
| @@ -121,18 +121,23 @@ static struct kmem_cache *sess_cache; | |||
| /** | |||
| * JavaScript challenge. | |||
| * | |||
| * To pass JS challenge client must repeat it's request in exact time frame | |||
| @@ -150,7 +155,7 @@ static const unsigned short tfw_cfg_redirect_st_code_dflt = 302; | |||
| * 'Accept: text/html' and GET method. | |||
| */ | |||
| static bool | |||
| tfw_http_sticky_redirect_allied(TfwHttpReq *req) | |||
| tfw_http_sticky_redirect_apllied(TfwHttpReq *req) | |||
|
|
||
| if (!tfw_cfg_js_ch) | ||
| return 0; | ||
|
|
||
| cur_time = jiffies; | ||
| /* | ||
| * When a client calculates it's own random delay, it uses range value |
|
@i-rinat Thank you for comments! The PR is already merged, but I'll fix this in my next PR. |
Related to JS challenge issue described in #1074
@krizhanovsky @aleksostapenko Please, test this fix in your environment. I could reproduce the issue, but I couldn't reproduce the exact behaviour you had.