Hi Ruoshan,
On Thu, Jul 14, 2016 at 03:24:42PM +0800, Ruoshan Huang wrote:
> hi,
> here is my (final?) patch for implementing `http-response track-sc*`
> directive. mainly I have:
> - duplicate config parsing (`parse_http_res_cond`) and validation
> (`check_config_validity`) code for track-sc
> - add ACT_ACTION_TRK_SC* case in `http_res_get_intercept_rule` to do the
> tracking
> - rename http_req_trk_idx to http_trk_idx
> - doc of course
>
> and the <key> smp expr for this directive requires smp with capability of
> `SMP_VAL_FE/BE_HRS_HDR`,
> I try to explain this in the document, but the `fetch_cap` is very
> complicated, hope I didn't fail explaining it :)
>
> please review these patchs when you're avaliable, Thanks.
So that's a very good work overall. I only found one minor issue that's easy
to fix : the http error rate was not updated. Normally it's updated immediately
when the response is received. But here, since we're doing the tracking after
this ought to be done, we have to explicitly do it. With the patch below it
works (sorry for the copy paste) :
diff --git a/src/proto_http.c b/src/proto_http.c
index 96f19a6..80e7a4c 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -3808,6 +3808,16 @@ resume_execution:
stkctr_set_flags(&s->stkctr[http_trk_idx(rule->action)], STKCTR_TRACK_CONTENT);
if (sess->fe != s->be)
stkctr_set_flags(&s->stkctr[http_trk_idx(rule->action)], STKCTR_TRACK_BACKEND);
+
+ /* When the client triggers a 4xx from
the server, it's most often due
+ * to a missing object or permission.
These events should be tracked
+ * because if they happen often, it may
indicate a brute force or a
+ * vulnerability scan. Normally this is
done when receiving the response
+ * but here we're tracking after this
ought to have been done so we have
+ * to do it on purpose.
+ */
+ if ((unsigned)(txn->status - 400) < 100)
+ stream_inc_http_err_ctr(s);
}
}
break;
Regarding the doc, I'd just remove the suggestion about trying and seeing
if it works or not, as that only incites users to be lazy and to rely on
the config parser instead of understanding what they are doing. I'll also
merge your two patches into a single one as it's better to have the doc
with the feature when it comes as a single patch.
If you're fine with this, I'll simply merge them with the change above,
it's clean enough, no need to respin. Please just confirm that it's fine
for you as well.
Thanks,
Willy
