Fix #731: HTTP tables introduction. #1017

aleksostapenko · 2018-05-15T12:36:30Z

No description provided.

vankoven · 2018-05-15T18:55:24Z

tempesta_fw/http.c

+		TFW_INC_STAT_BH(clnt.msgs_otherr);
+		tfw_client_drop(req, 500, "cannot find vhost for request");
+	}
+	return -EINVAL;


I would say, the function return value meaning is pass or filter the request, so it should return TFW_BLOCK or TFW_PASS

vankoven · 2018-05-16T10:11:24Z

tempesta_fw/cfg.c

 		return -EINVAL;

-	if (!check_identifier(name_src, name_len))
+	if (!rule) {
+		e->name = e->ftoken;


e->name is already set to e->ftoken if !rule.

e->ftoken is temporally assigned to local name variable (not to e->name) for uniform checking of identifier.

vankoven · 2018-05-17T08:29:54Z

tempesta_fw/cfg.c

@@ -193,6 +202,7 @@ entry_reset(TfwCfgEntry *e)
 	BUG_ON(!e);

 	kfree(e->name);


~~Seems that e->name may point to TFW_CFG_RULE_NAME constant string so it mustn't be freed.~~ Ok here, never happens actually.

vankoven · 2018-05-17T08:39:35Z

tempesta_fw/sched/tfw_sched_http.c

+ *       ...
+ *       mark != 1 -> storage;
+ *   }
+ *   http_chain {


Configuration has changed, please, update etc/tempesta_fw.conf

vankoven · 2018-05-17T10:31:11Z

tempesta_fw/http_match.c

-	return match_fn(req, rule);
+	matched = match_fn(req, rule);
+	if (rule->inv)
+		matched = !matched;


Same as matched = match_fn(req, rule) ^ rule->inv, but may be your variant is more easy to read.

Yes, bit operations are always preferred to conditions.

vankoven · 2018-05-17T11:25:48Z

tempesta_fw/sched/tfw_sched_http.c

 */
 static TfwVhost *
-tfw_sched_http_sched_vhost(TfwMsg *msg)
+tfw_sched_http_sched_vhost(TfwMsg *msg, bool *block)


For me no target vhost found means that request must be filtered. In current http_chain definition block is defined as default action. So rule block must be used if no target vhost found and variable vhost must be set to true. That means, that block variable duplicates return value. But the function works different: it doesn't set variable block to true, if no vhost found and no explicit block action encountered.

I see, why you've introduced block variable. Request without target vhost can be filtered in two ways: blocking of entire client connection or dropping a single request. I believe, HTTP scheduler is wrong place to make a that decision, it must be done in http.c. But let's look to the problem. Some clients may use proxies to reach TempestaFW, one client connection can multiplicate a lot of end clients. Some of them malicious, but most of them - not. We don't want to block good clients for others deeds, so dropping of single requests seems more reasonable for me. We can still block connections with lot of malicious requests using http_resp_code_block frang limit.

It seems a request with not found vhost is dropped in tfw_http_req_set_context() with cannot find vhost for request and I think it has sense: a vhost isn't configured for the request - maybe it's just an error, not an attack. Anyway we do not pass the request (which also could be malicious) to a backend, so we're good.

Done: connection dropped in case of block action; request dropped in case of vhost miss.

vankoven · 2018-05-17T11:26:27Z

tempesta_fw/http.c

@@ -2596,12 +2600,23 @@ tfw_http_req_add_seq_queue(TfwHttpReq *req)
 static int
 tfw_http_req_set_context(TfwHttpReq *req)
 {
-	if (!(req->vhost = tfw_vhost_match((TfwMsg *)req)))
-		return -EINVAL;
+	bool block = false;


See my comment for tfw_sched_http_sched_vhost()

vankoven · 2018-05-17T11:30:46Z

tempesta_fw/sched.c

@@ -60,7 +60,7 @@ tfw_sched_get_vhost(TfwMsg *msg)
 	list_for_each_entry_rcu(sched, &sched_list, list) {
 		if (!sched->sched_vhost)
 			break;
-		if ((vhost = sched->sched_vhost(msg))) {
+		if ((vhost = sched->sched_vhost(msg, block))) {


It was said in one of the last reviews by @krizhanovsky : http (vhost) scheduler isn't one of possible schedulers and it's API is far away of hash and rr schedulers. Foreach cycle must be replaced with direct http (vhost) scheduler call. This wasn't done in this PR, please add issue for it.

Yes, there is no need for the loop, even probably the whole tfw_sched_get_vhost() should be just replaced by direct http tables call, especially given that tempesta_fw/sched/tfw_sched_http.c should be moved & renamed to tempesta_fw/http_tbl.c

krizhanovsky · 2018-05-19T17:02:24Z

tempesta_fw/sched/tfw_sched_http.c

@@ -5,7 +5,8 @@
 *


This isn't just a scheduler any more, so please move it to tempesta_fw directory and rename to http_tbl.c, it should be built into tempesta_fw.ko. Also please update tempesta.sh to remove the module management. Preferably, move tfw_sched_ratio.c and tfw_sched_hash.c to tempesta_fw as http_sched_ratio.c and http_sched_hash.c correspondingly as well.

krizhanovsky · 2018-05-19T18:43:18Z

tempesta_fw/sched.c

@@ -60,7 +60,7 @@ tfw_sched_get_vhost(TfwMsg *msg)
 	list_for_each_entry_rcu(sched, &sched_list, list) {
 		if (!sched->sched_vhost)
 			break;
-		if ((vhost = sched->sched_vhost(msg))) {
+		if ((vhost = sched->sched_vhost(msg, block))) {


Yes, there is no need for the loop, even probably the whole tfw_sched_get_vhost() should be just replaced by direct http tables call, especially given that tempesta_fw/sched/tfw_sched_http.c should be moved & renamed to tempesta_fw/http_tbl.c

krizhanovsky · 2018-05-19T18:48:09Z

tempesta_fw/http_match.c

 *  etc...
 * Each such match_*() function takes TfwHttpReq and TfwHttpMatchRule and
 * returns true if the given request matches to the given rule.
 * Such approach allows to keep the code structured and eases adding new
- * @field/@op combinations.
+ * @field types.
 * Currently that is implemented with a multi-dimensional array of pointers
 * (the match_fn_tbl). However the code is critical for performance, so perhaps
 * this may be optimized to a kind of jump table.


Please update copyrights year at the below

krizhanovsky · 2018-05-19T18:51:04Z

tempesta_fw/http_match.c

+	BUG_ON(rule->op <= 0 || rule->op >= _TFW_HTTP_MATCH_O_COUNT);
+	BUG_ON(rule->act.type <= 0 || rule->act.type >= _TFW_HTTP_MATCH_ACT_COUNT);
+	BUG_ON(rule->arg.type <= 0 || rule->arg.type >= _TFW_HTTP_MATCH_A_COUNT);
+	BUG_ON(rule->arg.len < 0 || rule->arg.len >= TFW_HTTP_MATCH_MAX_ARG_LEN);


too long lines

krizhanovsky · 2018-05-19T19:02:05Z

tempesta_fw/http_match.c

-	return match_fn(req, rule);
+	matched = match_fn(req, rule);
+	if (rule->inv)
+		matched = !matched;


Yes, bit operations are always preferred to conditions.

krizhanovsky · 2018-05-19T20:09:38Z

tempesta_fw/http_match.h

 	tfw_http_match_op_t 	op;    /* Comparison operator. */
+	TfwHttpAction		act;   /* Rule action. */
 	TfwHttpMatchArg 	arg;   /* A value to be compared with the field.
 					  note: the @arg has variable length. */
 } TfwHttpMatchRule;


Please arrange members of this structure and the nested structures to minimize memory gaps (e.g. integer enum types should be adjacent where possible, probably it has sense to replace the bool fields with integer flags).

krizhanovsky · 2018-05-19T20:20:07Z

tempesta_fw/sched/tfw_sched_http.c

 */
 static TfwVhost *
-tfw_sched_http_sched_vhost(TfwMsg *msg)
+tfw_sched_http_sched_vhost(TfwMsg *msg, bool *block)


It seems a request with not found vhost is dropped in tfw_http_req_set_context() with cannot find vhost for request and I think it has sense: a vhost isn't configured for the request - maybe it's just an error, not an attack. Anyway we do not pass the request (which also could be malicious) to a backend, so we're good.

krizhanovsky · 2018-05-19T20:26:15Z

tempesta_fw/sched/tfw_sched_http.c

+			return r;
+	} else {
+		BUG();
+		// TODO: parsing rules of other types


Please don't use C++-style for persistent comments.

Why don't you parse the other types?

Corrected: processing for method type has been added.
Addr type is not used in TempestaFW and not implemented at all.

krizhanovsky · 2018-05-19T20:37:23Z

tempesta_fw/http_match.h

@@ -37,6 +57,7 @@ typedef enum {
 	TFW_HTTP_MATCH_F_HOST,
 	TFW_HTTP_MATCH_F_METHOD,
 	TFW_HTTP_MATCH_F_URI,
+	TFW_HTTP_MATCH_F_MARK,
 	_TFW_HTTP_MATCH_F_COUNT
 } tfw_http_match_fld_t;


Comment from #731

Currently not all special headers are handled in tfw_http_match_fld_t, e.g. it doesn't handle X-Forwarded-For, User-Agent or Cookie.

Isn't addressed. Please also update the Wiki after implementing handling of the headers.

krizhanovsky · 2018-05-19T20:44:00Z

tempesta_fw/sock.c

-		         smp_processor_id(), __func__,
-		         skb, skb->data_len, skb->len);
+		/* Propagate mark of message head skb.*/
+		skb->mark = mark;


Please extend #883 by a requirement to test the mark iteroperability between Tempesta FW and iptables/nftables in both the directions (or just write the test on your own). Also add an example with iptables/nftables to https://github.com/tempesta-tech/tempesta/wiki/HTTP-tables

Wiki has been updated with iptables example.

vankoven · 2018-06-08T12:46:30Z

etc/tempesta_fw.conf

@@ -259,8 +259,18 @@
 #       The value of the "Host" header field.
 #   - hdr_conn
 #       The value of the "Connection" header field.
+#   - hdr_ctype


Not the issue of this PR, but it seems that all that hdr_ctype, hdr_agent and Co is not very user friendly. In the same time we have hdr_raw. It would better to provide single directive for all headers:

http_chain NAME { [ hdr "HDR_NAME" == (!=) ARG ] -> ACTION [ = VAL]; ... }

Like it's done for hdr_raw now. And give a notice that comparing of the Content-Type, User-Agent, .... headers is accelerated, while processing of other headers is slow.

We have tfw_http_msg_req_spec_hid() function which can convert header name to it's internal representation in spec headers. So i believe that the change would be easy to implement.

I created issue #1033

vankoven

Good after minor fixes

vankoven · 2018-06-08T13:36:05Z

tempesta_fw/http.c

 			TFW_INC_STAT_BH(clnt.msgs_filtout);
-			tfw_client_block(req, 403, "parsed request"
-				       " has been filtered out");
+			tfw_client_block(req, 500, "request has been filtered"


I'd prefer 403 Forbidden here. It's not server or Tempesta fault that client has requested forbidden location. Think of it from http_resp_code_block directive perspective. It worth to block clients that cause many 403 errors, while legitimate users have margin for mistake,

vankoven · 2018-06-08T13:44:13Z

tempesta_fw/http.c

+		if ((req->vhost = tfw_http_tbl_vhost((TfwMsg *)req, &block))) {
+			req->location = tfw_location_match(req->vhost,
+							   &req->uri_path);
+			r = tfw_gfsm_move(&conn->state, TFW_HTTP_FSM_REQ_MSG,


The hook is not called if vhost is not found. That is not right. Need to run the hook even if vhost is not found, default vhost and default location will be used as context.

Currently only frang is behind that hook, it still may want to block the client even if no vhost found.

Yeah, good catch - this must be fixed before the merge.

krizhanovsky · 2018-06-09T12:39:07Z

tempesta_fw/http_match.c

+
+	*op_out = TFW_HTTP_MATCH_O_EQ;
+	if (len > 1 && arg[len - 1] == '*' && arg[len - 2] != '\\')
+			*op_out = TFW_HTTP_MATCH_O_PREFIX;


Something happened with identations at this line and line 564.

krizhanovsky · 2018-06-09T13:08:03Z

etc/tempesta_fw.conf

@@ -259,8 +259,18 @@
 #       The value of the "Host" header field.
 #   - hdr_conn
 #       The value of the "Connection" header field.
+#   - hdr_ctype


I created issue #1033

krizhanovsky

Good to merge after small fixes and cleanups.

krizhanovsky · 2018-06-09T13:10:05Z

tempesta_fw/http.c

+		if ((req->vhost = tfw_http_tbl_vhost((TfwMsg *)req, &block))) {
+			req->location = tfw_location_match(req->vhost,
+							   &req->uri_path);
+			r = tfw_gfsm_move(&conn->state, TFW_HTTP_FSM_REQ_MSG,


Yeah, good catch - this must be fixed before the merge.

aleksostapenko added 4 commits May 5, 2018 16:47

Fix #731: HTTP tables introduction.

187b357

Merge branch 'master' into ao-731

b3a9f98

Add HTTP tables processing (#731).

126ed95

Minor corrections (#731).

0c892e2

aleksostapenko requested review from krizhanovsky and vankoven May 15, 2018 12:38

aleksostapenko added the ready for review label May 15, 2018

Correction of http table cleanup procedure (#731).

07a322c

vankoven reviewed May 17, 2018

View reviewed changes

vankoven suggested changes May 17, 2018

View reviewed changes

aleksostapenko added 2 commits May 18, 2018 23:57

Resolve bug during cleanup (#731).

e6688c7

Change manual in tempesta_fw.conf (#731).

73a1a0e

krizhanovsky requested changes May 19, 2018

View reviewed changes

aleksostapenko removed the ready for review label Jun 4, 2018

Changes according review comments (#731).

bbea5b4

aleksostapenko added the ready for review label Jun 8, 2018

Merge branch 'master' into ao-731

ddd3e34

vankoven reviewed Jun 8, 2018

View reviewed changes

vankoven approved these changes Jun 8, 2018

View reviewed changes

Add processing of 'method' field into HTTP tables (#731).

fccc9e2

krizhanovsky mentioned this pull request Jun 9, 2018

HttpTables: better header names #1033

Closed

krizhanovsky reviewed Jun 9, 2018

View reviewed changes

krizhanovsky approved these changes Jun 9, 2018

View reviewed changes

Additional changes according comments (#731).

7eb43b4

aleksostapenko merged commit 333ec1c into master Jun 9, 2018

aleksostapenko deleted the ao-731 branch June 9, 2018 20:21

vankoven mentioned this pull request Jul 24, 2018

Fast sticky session scheduler is broken #1043

Closed

		@@ -193,6 +202,7 @@ entry_reset(TfwCfgEntry *e)
		BUG_ON(!e);

		kfree(e->name);

Fix #731: HTTP tables introduction. #1017

Fix #731: HTTP tables introduction. #1017

Conversation

aleksostapenko commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

krizhanovsky May 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven May 17, 2018 •

edited

Loading

aleksostapenko Jun 7, 2018 •

edited

Loading

krizhanovsky May 19, 2018 •

edited

Loading