Fix #658: Add possibility to configure error response behaviour. #848

aleksostapenko · 2017-10-09T16:05:17Z

No description provided.

krizhanovsky

This is unfinished review: just couple of significant missed things. I'll continue the review.

krizhanovsky · 2017-10-12T19:34:41Z

tempesta_fw/sock_clnt.c

+			tfw_sock_clnt_cfg_handle_block_action,
+			.allow_repeat = true,
+			.allow_none = true
+		},


The configuration option wasn't documented. Please create "Handling clients" page in Wiki -> Configuration and add there description of the option. Also please copy https://github.com/tempesta-tech/tempesta#listening-address and https://github.com/tempesta-tech/tempesta#keep-alive-timeout from README.md to the new Wiki page.

tempesta/etc/tempesta_fw.conf also must be updated accordingly.

krizhanovsky · 2017-10-12T19:36:05Z

tempesta_fw/sock_clnt.c

+			tfw_sock_clnt_cfg_handle_block_action,
+			.allow_repeat = true,
+			.allow_none = true
+		},


Please create a new issue with description of tests for the new functionality and assign it to @intelfx .

UPD: the test must ensure correct handling of pipelined requests as in the example from #658 (comment)

krizhanovsky · 2017-10-12T22:51:18Z

tempesta_fw/http.h

+#define TFW_BLOCK_ACTION_ATTACK_REPLY		0x0002
+#define TFW_BLOCK_ACTION_ERROR_NOLOG		0x0004
+#define TFW_BLOCK_ACTION_ATTACK_NOLOG		0x0008
+


The flags are somewhat misleading: they're essentially single bit flag, so, given that there is no comments, a one can assume that they can ORed together, but that's not true. Probably they should be split to ATTACK, ERROR, REPLY, DROP (this one seems just 0, i.e. default), and NOLOG. Also usually shorter names are appreciated, e.g. TFW_BLK_ACT_ERR_REPLY or something like that - there is just no sense to keep so long prefix TFW_BLOCK_ACTION_ which is just the same for all the flags.

krizhanovsky · 2017-10-12T23:07:22Z

tempesta_fw/http.c

+		tfw_http_send_500(req, reason);
+		break;
+	}
+}


The comment #658 (comment) isn't addressed. Please load files named like 404.html or 500.html from etc directory and use the content as a response body. Otherwise a user just sees blank pages. Currently we load TLS certificates from the files, so it'd be good to generalize the routines and use the same code for TLS certificates and the error responses. Probably a new variables, $TFW_PATH/etc should be passed to tempesta_fw.ko - the path can be used for TLS certificates as well as for the error codes.

Corrected: configurable custom error pages added.

krizhanovsky

More notes after a deeper review.

krizhanovsky · 2017-10-14T17:25:22Z

tempesta_fw/http.c

-		if (tfw_cli_conn_send(cli_conn, (TfwMsg *)req->resp)) {
+		if (tfw_cli_conn_send(cli_conn, (TfwMsg *)req->resp) ||
+		    (TFW_CONN_TYPE(cli_conn) & Conn_Suspected &&
+		     req->flags & TFW_HTTP_SUSPECTED)) {


Please place opening curly brace at next line for multi-line conditions, otherwise the complex statement is hard to read.

krizhanovsky · 2017-10-14T17:37:45Z

tempesta_fw/sync_socket.h

+
+/* No requests longer accepted (flag only for client connections) */
+#define	Conn_Suspected		(0x1 << __Flag_Bits)
+


The definitions look inconsistent with connection type flags definition from connection.h. Please define these flags in enum fashion as in connection.h, add BUILD_BUG_ON() in connection.h asserting that the flags don't intersect and write a comment near from the enum that there are also low-level, socket, flags in sync_sock.h.

Also please clarify in the comment what 'Suspected' actually means, that we're suspecting the connection as malicious, not just experiencing some error.

Corrected: added comments and build-stage check for bit flags overlapping.

krizhanovsky · 2017-10-14T17:58:01Z

tempesta_fw/http.c

@@ -1789,7 +1802,9 @@ __tfw_http_resp_fwd(TfwCliConn *cli_conn, struct list_head *ret_queue)
 	list_for_each_entry_safe(req, tmp, ret_queue, msg.seq_list) {
 		BUG_ON(!req->resp);
 		tfw_http_resp_init_ss_flags((TfwHttpResp *)req->resp, req);
-		if (tfw_cli_conn_send(cli_conn, (TfwMsg *)req->resp)) {
+		if (tfw_cli_conn_send(cli_conn, (TfwMsg *)req->resp) ||
+		    (TFW_CONN_TYPE(cli_conn) & Conn_Suspected &&


tfw_http_error_resp_and_log() sets Conn_Suspected for the connection as well as TFW_HTTP_SUSPECTED for the requests and only the function sets TFW_HTTP_SUSPECTED, so there is no sense to check both the flags in the condition. The logic is at HTTP layer, so we should minimize interaction with other layers and work at HTTP layer only as much as possible: reducing inter-layer interaction is always a good thing.

krizhanovsky · 2017-10-14T18:14:34Z

tempesta_fw/http.c

+	}
+	if (!nolog)
+		TFW_WARN("Error response: %s, msg=%p conn=%p\n",
+			 msg, req, req->conn);


Need to print client's IP address - it's important information for security logs.

krizhanovsky · 2017-10-14T18:24:59Z

tempesta_fw/http.c

+tfw_http_drop(TfwHttpReq *req, unsigned short code, const char *msg, bool mark)
+{
+	bool reply = tfw_block_action_flags & TFW_BLOCK_ACTION_ERROR_REPLY;
+ 	bool nolog = tfw_block_action_flags & TFW_BLOCK_ACTION_ERROR_NOLOG;


I know this is a common practice to use such boolean variables just to handle flags, but we don't like so long names (tfw_block_action_flags and TFW_BLOCK_ACTION_ERROR_REPLY) and prefer to use shorter names which can be used just in a function call.

krizhanovsky · 2017-10-14T18:30:54Z

tempesta_fw/http.c

+ * NOTE: @mark must be set only for client connection context
+ * because in this case we must mark client connection in
+ * special manner to delay its closing until transmission
+ * of error response will be finished.


Why? If we find in a server connection context that a request is malicious (e.g. the call in tfw_http_resp_gfsm()), then there is no way to mark the request and the client connection as malicious and close the connection? It seems we're just passing many types of the attacks due to the limitation...

Corrected: added the ability to mark a request and connection for closing from the server context.

krizhanovsky · 2017-10-14T18:32:28Z

tempesta_fw/http.c

+			       TFW_CONN_TYPE(cli_conn) & Conn_Suspected);
+			TFW_CONN_TYPE(cli_conn) |= Conn_Suspected;
+			req->flags |= TFW_HTTP_SUSPECTED;
+			tfw_connection_unlink_msg(req->conn);


...and, yes, we can call tfw_connection_unlink_msg() only in client connection context, while we're sure that conn->req points to exactly this request. Probably the function must be split into 2 functions: for server and client contexts correspondingly.

krizhanovsky · 2017-10-14T19:10:01Z

tempesta_fw/sock_clnt.c

@@ -37,15 +37,17 @@
 * ------------------------------------------------------------------------
 */

+unsigned short tfw_block_action_flags = TFW_BLOCK_ACTION_ERROR_REPLY;


I'm wondering if we can define multiple rules

block_action error reply; block_action attack drop nolog;

as described in #658 (comment) using only this one variable? It seems the requirement is missed.

The are 4 least significant bits to set/unset - so we have 16 possible states. And from the configuration statements:

block_action error reply; block_action attack drop nolog;

we also have 16 possible combinations - so this should be enough.
If we have third directive. e.g.:

block_action error drop nolog;

then the corresponding flag bits will be rewritten to new values (if I understand the task correctly).

keshonok

Hope this helps to make the code better.

keshonok · 2017-10-14T17:38:37Z

tempesta_fw/sync_socket.h

+#define	__Flag_Bits		0x10
+
+/* No requests longer accepted (flag only for client connections) */
+#define	Conn_Suspected		(0x1 << __Flag_Bits)


In my opinion, the name is very misleading. There's no suspicion as a certain decision has been made, and that decision is that the connection is erroneous for whatever reason. The name of the flag should reflect that.

Another thing here is that the SS layer is abstracted from the rest of Tempesta, and therefore it's confusing to have a state here that has the same name as in Tempesta, while the meaning of the state is very specific to the SS layer.

So, according to the actual meaning, perhaps, call it Conn_OnHold? Meaning, that the connection is on hold for the time being, no data is passed to Tempesta, incoming packets in the connection are dropped. This behaviour under this state/flag continues until the request for a close comes and the connection is closed.

keshonok · 2017-10-14T17:43:46Z

tempesta_fw/sock.c

-			 * banned packets, or FIN in the received packet.
+			 * banned packets, or FIN in the received packet,
+			 * and only if we don't wait for sending the error
+			 * response to client (Conn_Suspected bit set).


Considering the comment below for the name of the flag (like Conn_OnHold), the comment should not be that specific about the reasons for the action. It's better to have something a little more abstracted, perhaps something like this: and only if it's not on hold until explicitly closed.

keshonok · 2017-10-14T19:37:57Z

tempesta_fw/http.c

@@ -34,6 +34,8 @@

 #include "sync_socket.h"

+extern unsigned short tfw_block_action_flags;


I am not sure that it's a good idea to refer this way to a configuration variable defined elsewhere. Perhaps, it's better to define a set of inline functions that would allow to access the contents of the variable.

keshonok · 2017-10-14T19:45:23Z

tempesta_fw/http.c

@@ -1789,7 +1802,9 @@ __tfw_http_resp_fwd(TfwCliConn *cli_conn, struct list_head *ret_queue)
 	list_for_each_entry_safe(req, tmp, ret_queue, msg.seq_list) {
 		BUG_ON(!req->resp);
 		tfw_http_resp_init_ss_flags((TfwHttpResp *)req->resp, req);
-		if (tfw_cli_conn_send(cli_conn, (TfwMsg *)req->resp)) {
+		if (tfw_cli_conn_send(cli_conn, (TfwMsg *)req->resp) ||
+		    (TFW_CONN_TYPE(cli_conn) & Conn_Suspected &&


This is HTTP layer. I believe it's sufficient to check the flag for the HTTP layer. It's unnecessary to check flags in both layers as they both are set at the same time. Moreover, it breaks the whole idea of layers.

@keshonok thank you for review!

keshonok · 2017-10-14T19:49:24Z

tempesta_fw/http.c

+ */
+static inline void
+tfw_http_error_resp_and_log(bool reply, bool nolog, TfwHttpReq *req,
+			    unsigned short code, const char *msg, bool mark)


Frankly, I think this is way too many arguments. Perhaps, this can be done in a different way? Please see the comments below to the functions that call this one.

The most confusing thing here is that the notion of marking mixes the necessity of setting the appropriate flags for holding the connection and marking the faulty request with the necessity of adding the request to the seq queue. Is that really the same thing? I wouldn't say so. It seems that there may be cases when a request is in seq queue already, yet it must be marked as faulty and the connection must be put on hold as well.

keshonok · 2017-10-14T19:51:29Z

tempesta_fw/http.c

+	bool reply = tfw_block_action_flags & TFW_BLOCK_ACTION_ATTACK_REPLY;
+ 	bool nolog = tfw_block_action_flags & TFW_BLOCK_ACTION_ATTACK_NOLOG;
+	tfw_http_error_resp_and_log(reply, nolog, req, code, msg, mark);
+}


I believe these functions should be inlined. Also, I believe it would be better if you had separate versions for client and server contexts, and that may help to reduce the number of arguments. Plus there are other consideration that point in that direction, most significant of which is that the request should be put on seq queue first.

@keshonok thank you for the notes!

The modern compiler doesn't treat inline C keyword actually. This is why we have attributes noinline and always_inline, so there is no big deal with inline specifiers in the patch. (However, I also don't like as they are made in the patch). Also, technically speaking, 6 arguments are OK - we have exactly 6 registers for arguments in Linux/x86-64 calling convention. Meantime, I agree that at least mark argument isn't a good one.

There is no sense to split expressions like tfw_blk_flags & TFW_BLK_ERR_REPLY to 2 lines. The original lines fit 80 characters in length, but the change makes them hard to read. It's usually has sense to separate function arguments, rather than make different parts of the same agrument look just like different arguments.

keshonok · 2017-10-14T20:07:57Z

tempesta_fw/http.h

@@ -290,6 +290,7 @@ typedef struct {
 /* URI has form http://authority/path, not just /path */
 #define TFW_HTTP_URI_FULL		0x000400
 #define TFW_HTTP_NON_IDEMP		0x000800
+#define TFW_HTTP_SUSPECTED		0x001000


Like for Conn_Suspected (please see the comment below), I believe the name is misleading as the decision has been made already. Perhaps, simply TFW_HTTP_MSG_BAD? This name may mean either of a message being filtered out, or being malicious, or being erroneous.

I don't agree: TFW_HTTP_MSG_BAD doesn't say what bad is? Is it corrupted or malicious message? I'm OK with suspected.

What I was trying to say was that in my opinion there's nothing that is suspected at this point. The decision had been made, the message IS now either filtered out, malicious, or erroneous. Due to decision that had been made already the client connection will be closed. Yet, all of these messages are now marked as suspected.

So I didn't understand what they are suspected of?

Suspicion is when we don't know what it is yet, it's either this or that, and there are further actions to come up with more detailed knowledge. But it doesn't work that way in the code. Again, the decision had been made at the time this flag is set, with the HTTP error response code and the error message. There's nothing that is suspected. Just the opposite, everything is pretty clear and definite.

Just a thought. :-)

keshonok · 2017-10-14T20:24:24Z

tempesta_fw/http.c

-			tfw_http_conn_msg_free((TfwHttpMsg *)req);
+			TFW_INC_STAT_BH(clnt.msgs_otherr);
+			tfw_http_drop(req, 500, "cannot find"
+				      "Vhost for request", true);


It's been stated that special drop/block processing is for the cases where a request either malformed or malicious (i.e. filtered out). This particular case is more of an internal error nature (or maybe even a configuration). So why tfw_http_drop()? Do we need to close the connection in this case? Does the client need to know of this error in the browser?

There are still regular tfw_http_send_500() and tfw_http_send_502() that weren't converted to this block/drop style, and that's justifiable and there's no need to block a request or drop a connection in those case.

Does the client need to know of this error in the browser?

But in the opposite case - if we don't send error response - the client will see only hang pages.
Maybe it is more suitable for system administrator responsibility - to decide is it worth to send error responses in these cases.

keshonok · 2017-10-14T20:26:32Z

tempesta_fw/http.c

 				TFW_INC_STAT_BH(clnt.msgs_otherr);
+				tfw_http_drop(req, 500, "cannot create"
+					      " sibling request", true);


Again, this seems like an internal error. Why the change to tfw_http_drop() Do we need to close the connection in this case? Does the client need to know of this error in the browser?

UPDATE: Yes, I guess we do, as otherwise we break the pairing of responses to requests. The other question still remains.

krizhanovsky · 2017-10-26T17:37:17Z

etc/tempesta_fw.conf

+# Specifies a path to file with page body for defined HTTP status_code;
+# status_code must be present in three-digit form ('502', '403'), or
+# in form of codes group ('4*' or '5*').
+


Ok, I see the new Wiki page https://github.com/tempesta-tech/tempesta/wiki/Handling-clients , so please replace appropriate sections in README file by references to the Wiki page.

krizhanovsky · 2017-10-26T17:52:46Z

tempesta_fw/http.c

 {
+	TfwStr *msg = &http_predef_resps[code];
+	TfwStr *date = __TFW_STR_CH(msg, 1);
+	TfwStr *crlf = __TFW_STR_CH(msg, 3);


For now we assemble HTTP responses from any number of chunks, so there is must be some agreement to have Date at 2nd position and CRLF at 4th. Please write a comment for http_predef_resps with the requirement or, better, make special defines with the __TFW_STR_CH()'s getting Data and CRLF correspondingly.

krizhanovsky · 2017-10-26T18:25:36Z

tempesta_fw/http.c

 {
+	TfwStr *msg = &http_predef_resps[code];


Previously we constructed the error messages on stack, so each CPU and/or context (process or softirq) worked with unique automatic memory. Now you're working with static http_predef_resps variable and modify it, so it typically should crash on parallel error responses sending. Probably making http_predef_resps per-cpu fix the problem, but you need to check whether we send error messages in softirq only (IIRC currently we have process context on shutdown only).

The other way is to copy the TfwStr descriptor (very fast since the descriptor itself is very small) to automatic variable and adjust only the new variable leaving the global variable unchanged. In this case you'll need a new TfwStr routine copying descriptors only since tfw_strcpy() does deep copy. I'd prefer this way in 2 reasons: (1) no limitation for sending error responses in softirq context only and (2) more code maintainability due to no need to carefully restore previous version of http_predef_resps.

Now you're working with static http_predef_resps variable and modify it, so it typically should crash on parallel error responses sending.

Yes, I missed that :(

krizhanovsky · 2017-10-26T18:32:27Z

tempesta_fw/http.c

+			{ .ptr = NULL, .len = 0 },
+		},
+		.len = SLEN(S_200_PART_01 S_V_DATE S_200_PART_02 S_CRLF),
+		.flags = 4 << TFW_STR_CN_SHIFT


Setting number of chunks to 4, while there are 5 actually, looks very dubious. At least a comment is required that the last item can be probably used for body. Also a macro for resolving 5th item to body is required (see also comment for tfw_http_send_resp()).

krizhanovsky · 2017-10-26T18:55:44Z

tempesta_fw/http.c

+	bool reply = tfw_block_action_flags & TFW_BLOCK_ACTION_ATTACK_REPLY;
+ 	bool nolog = tfw_block_action_flags & TFW_BLOCK_ACTION_ATTACK_NOLOG;
+	tfw_http_error_resp_and_log(reply, nolog, req, code, msg, mark);
+}


There is no sense to split expressions like tfw_blk_flags & TFW_BLK_ERR_REPLY to 2 lines. The original lines fit 80 characters in length, but the change makes them hard to read. It's usually has sense to separate function arguments, rather than make different parts of the same agrument look just like different arguments.

krizhanovsky · 2017-10-26T19:48:03Z

tempesta_fw/http.c

+		TfwStr *body_str = __TFW_STR_CH(&http_predef_resps[i], 4);
+		if (body_str->ptr) {
+			TfwStr *clen_str =
+				__TFW_STR_CH(&http_predef_resps[i], 2);


The line break isn't nice at all, so usually we'd write

TfwStr *clen_str, *body_str; body_str = __TFW_STR_CH(&http_predef_resps[i], 4); if (!body_str->ptr) continue; clen_str = __TFW_STR_CH(&http_predef_resps[i], 2); // ..........

Also there is no sense to define clen_str in outer scope if it's used in inner scope only, so it's better to move clen_str into the next if statement. And yes, you need the line break again if you won't invert the condition and use break as for the if above.

krizhanovsky · 2017-10-26T19:52:59Z

tempesta_fw/http.c

+		return -E2BIG;
+	}
+
+	l_size = 2*SLEN(S_CRLF) + SLEN(S_F_CONTENT_LENGTH) + digs_count;


Please use spaces around * operator just like for all other math operators except unary minus.

krizhanovsky · 2017-10-26T20:10:02Z

tempesta_fw/http.c

+	snprintf(new_length, l_size + 1, "%s%s%s%s",
+		 S_CRLF, S_F_CONTENT_LENGTH , buff, S_CRLF);
+
+	new_body = kmalloc(b_size, GFP_KERNEL);


Firstly, there is no need to call kmalloc() twice for the data always managed together. Secondly, haveing #163 (comment) in mind, it's better to use page allocator to place the data - later in #163 we'll just reuse the pages in transmitted skb to avoid copying.

krizhanovsky · 2017-10-26T21:35:48Z

tempesta_fw/http.c

+		{ .ptr = NULL, .len = 0 },
+	},
+	.len = 0,
+};


It seems you use http_4xx_resp_body and http_5xx_resp_body just to propagate the common data to all 40x and 50x responses correspondingly. If so, then why not to use e.g. http_predef_resps[RESP_403] and http_predef_resps[RESP_500] and propagate their values to all 40x and 50x responses correspondingly?

Yes, I also thought about this approach. But in this case we have no the opportunity to set separately one page body for 500 answer and another page body for the remaining 5xx answers. As I understand it, the code 500 is a separate response code that does not intersect with the rest of the 5xx codes.

Yeah, makes sense. Just write a short comment why the two default static variables are required.

krizhanovsky · 2017-10-26T22:27:10Z

tempesta_fw/sync_socket.h

+	 * requests longer accepted (flag is intended
+	 * only for client connections).
+	 */
+	Conn_OnHold		= 0x1 << __Flag_Bits,


Well, I commented Aleksey's comment #848 (comment) and thought that it's enough and you just leave suspected name untouched. OnHold is bad name, at least because we already have tfw_http_conn_on_hold() which has completely different meaning. Moreover, the whole #658 change is about differentiation errors from security events (attacks), so all the related stuff must clearly define difference between the events instead of using common names.

Please revert the change.

krizhanovsky

Good to merge after minor adjustments

krizhanovsky · 2017-11-01T13:48:15Z

tempesta_fw/str.c

+
+	return 0;
+}
+EXPORT_SYMBOL(tfw_strcpy_desc);


All the string routines from the file must have appropriate tests in t/unit/test_tfw_str.c. Please add the test.

krizhanovsky · 2017-11-01T14:04:55Z

tempesta_fw/http.c

@@ -19,6 +19,7 @@
 * Temple Place - Suite 330, Boston, MA 02111-1307, USA.
 */
 #include <linux/string.h>
+#include <linux/vmalloc.h>


You don't use vmalloc(), so it's better to use <linux/gfp.h> for __alloc_free_pages()

But vfree() function is used in tfw_http_cfg_handle_resp_body() - to release the memory allocated in tfw_cfg_read_file().

keshonok · 2017-11-01T21:42:45Z

tempesta_fw/str.c

+		c2->skb = c1->skb;
+		c2->eolen = c1->eolen;
+		c2->len = c1->len;
+		c2->flags = c1->flags;


Perhaps, a simple *c2 = *c1 would suffice here?

Yes, missed that. Corrected.
Thank you.

Fix #658: Add possibility to configure error response behaviour.

7041f40

aleksostapenko requested review from krizhanovsky and vankoven October 9, 2017 16:05

krizhanovsky requested changes Oct 12, 2017

View reviewed changes

krizhanovsky requested changes Oct 14, 2017

View reviewed changes

keshonok reviewed Oct 14, 2017

View reviewed changes

aleksostapenko added 3 commits October 18, 2017 19:41

Fix code review comments (PR#848).

9056461

Implement configurable custom error pages (PR#848).

aceb4ad

Documentation update (PR#848).

4c5f17a

aleksostapenko force-pushed the ao-658 branch from a5b4bc5 to 4c5f17a Compare October 26, 2017 11:15

krizhanovsky requested changes Oct 26, 2017

View reviewed changes

Fix review comments (PR#848).

27bfd26

krizhanovsky approved these changes Nov 1, 2017

View reviewed changes

keshonok reviewed Nov 1, 2017

View reviewed changes

Unit tests for 'tfw_strcpy_desc' function added (PR#848).

d7052cc

aleksostapenko merged commit a66752a into master Nov 2, 2017


		/* No requests longer accepted (flag only for client connections) */
		#define Conn_Suspected (0x1 << __Flag_Bits)

		@@ -34,6 +34,8 @@

		#include "sync_socket.h"

		extern unsigned short tfw_block_action_flags;

Fix #658: Add possibility to configure error response behaviour. #848

Fix #658: Add possibility to configure error response behaviour. #848

Conversation

aleksostapenko commented Oct 9, 2017

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky Oct 12, 2017 • 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

krizhanovsky 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

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 Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

keshonok 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

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

keshonok Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

keshonok Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

aleksostapenko Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

keshonok Oct 14, 2017 • 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

aleksostapenko Oct 27, 2017 • 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

krizhanovsky Oct 12, 2017 •

edited

Loading

aleksostapenko Oct 18, 2017 •

edited

Loading

keshonok Oct 15, 2017 •

edited

Loading

keshonok Oct 14, 2017 •

edited

Loading

aleksostapenko Oct 18, 2017 •

edited

Loading

keshonok Oct 14, 2017 •

edited

Loading

aleksostapenko Oct 27, 2017 •

edited

Loading