Don't cache and don't forward hop-by-hop headers #650

vankoven · 2016-11-24T13:07:41Z

Fix issue #409

Parse Connection header during parsing HTTP message;
Store raw headers listed in Connection header in temporary array allocated in message's pool: every raw header is marked as hop-by-hop if it's name present in that array. When connection header parsed in the first time, parser checks already parsed raw headers and mark them as hop-by-hop headers if needed. If Connection header does not contain any raw headers names no extra allocations or string comparison happen;
Special headers have their own marking procedure which does not involve string comparisons. Some of the special headers are always hop-by-hop (Connection, we also mark Server in responses as hop-by-hop) and using generic comparison like for Raw headers will ruin performance;
Unit tests for responses and requests with hop-by-hop marking checks;
little optimisation of __hdr_lookup(): reduce string comparisons.

TBD:
When response contains Connection: keep-alive, Keep-alive header will be marked as hop-by-hop header and will not be saved in cache (that was the requirement in #409 ). RFC 7230 6.1 also say:

 A proxy or gateway MUST parse a received Connection header field before
   a message is forwarded and, for each connection-option in this field,
   remove any header field(s) from the message with the same name as the
   connection-option, and then remove the Connection header field itself
   (or replace it with the intermediary's own connection options for the
   forwarded message).

Currently we have no realised way to set default Keep-Alive header (not found in issues but here is corresponding TODO: https://github.com/tempesta-tech/tempesta/blob/master/tempesta_fw/http.c#L639 ). So question is: delete Keep-Alive header in forwarded messages or not? But messages served from cache will have no such header. Currently i remove it from both forwarded messages: response and request, disabling that is one-line patch.

krizhanovsky · 2016-11-30T13:58:25Z

tempesta_fw/http_parser.c

+		msg->parser.hdr.flags |= TFW_STR_HBH_HDR;
+
+/**
+ * Mark raaw header @hdr as hop-by-hop if its name was listed in Connection


raaw -> raw

krizhanovsky · 2016-11-30T14:05:12Z

tempesta_fw/http_parser.c

+	if (!ht)
+		return;
+
+	for (i = 0; i < ht->off; ++i) {


Does RFC allow setting headers, mentioned in Connection, before Connection header? The code is dangerous, just consider the example:

GET / HTTP/1.1\r\n Connection: keep-alive, Host, User-Agent\r\n Host: foo.com\r\n User-Agent: malicious client\r\n \r\n

So server will receive malformed request.

We should run the loop at least from TFW_HTTP_HDR_RAW. Probably there is some limitation which headers can be mentioned in Connection header... The other way (I think the safest one) is to remove Keep-Alive only and ignore any other mentioned headers in Connection. However, we still use current mechanism for the headers removal such that we'll be able to remove other headers which are known to be mentioned in Connection. I argue against blind headers removal.

RFC does not have any restrictions where headers listed in Connection header must be placed. So both variants are ok: before or after Connection header. More over i tried to set up custom hop-by-hop headers in ngingx and apache, both allow setting custom headers before and after the header. Here example from RFC 6455 with hop-by-hop header before Connection header:

GET /chat HTTP/1.1 Host: server.example.com Upgrade: websocket Connection: Upgrade Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ== Origin: http://example.com Sec-WebSocket-Protocol: chat, superchat Sec-WebSocket-Version: 13

Table you point to stores names for only raw headers. I agree, it could be confusing, but i used data structure which suit me perfectly. In situation you show user-agent and every other spec header listed in connection header (except keep-alive) will not be removed

Obsoleted now, in new revision this array is statically allocated

krizhanovsky · 2016-11-30T14:07:17Z

tempesta_fw/http_parser.c

+	TfwStr *hdr, *append;
+	int r;
+	TfwHttpHdrTbl *ht = hm->parser.hbh_parser.raw;
+	if (!ht) {


This isn't mentioned in our Coding Style, but FreeBSD KNF requires split variables definitions and code. It's bit hard to read when code immediately follows variable definitions immediately.

krizhanovsky · 2016-11-30T14:07:25Z

tempesta_fw/http_parser.c

+}
+
+/**
+ * Add header name listed in Connection header to rable of raw headers.


rable -> table

krizhanovsky · 2016-11-30T14:09:56Z

tempesta_fw/http_parser.c

+		if (hid == ht->size)
+			if (__tfw_http_msg_grow_hdr_tbl(&hm->parser.hbh_parser.raw,
+							hm->pool))
+				return -ENOMEM;


This is true and powerful headers parser. Do we really allow that Connection mentioning more than 16 header? I believe it has sense to block the header. Meantime it will make the code faster and easier and that's good.

krizhanovsky · 2016-11-30T14:20:43Z

tempesta_fw/http_parser.c

@@ -917,8 +1069,24 @@ __parse_connection(TfwHttpMsg *hm, unsigned char *data, size_t len)
 	 * it could be names of any headers, including custom headers.
 	 */
 	__FSM_STATE(I_ConnOther) {
-		__FSM_I_MATCH_MOVE(token, I_ConnOther);
+		/* Unwind __FSM_I_MATCH_MOVE(token, I_ConnOther): */
+		__fsm_n = __data_remain(p);


There is MATCH_MOVE macroses which embody the code.

krizhanovsky · 2016-11-30T14:22:02Z

tempesta_fw/http_parser.c

+		case 'k':
+			if (likely(__data_available(p, 11)
+				   && C4_INT_LCM(p + 1, 'e', 'e', 'p', '-')
+				   && C4_INT_LCM(p + 5, 'a', 'l', 'i', 'v')


The discussed bug to fix

krizhanovsky · 2016-11-30T14:25:27Z

tempesta_fw/t/unit/test_http_parser.c

@@ -877,6 +877,219 @@ TEST(http_parser, cookie)
 		"\r\n");
 }

+TEST(http_parser, req_hop_by_hop)
+{


Please add test case described above for "Connection: Host\r\n' to mangled message.

krizhanovsky · 2016-11-30T14:26:20Z

tls/ttls.c

@@ -128,7 +128,7 @@ module_exit(ttls_exit);

 MODULE_AUTHOR("Tempesta Technologies");
 MODULE_VERSION("2.3.0");
-MODULE_LICENSE("GPLv2");
+MODULE_LICENSE("GPL v2");


Doesn't it taints kernel? The change is fine if not.

krizhanovsky · 2016-12-03T11:24:21Z

Please address the comment #409 (comment) or move the requirement to #634

vankoven · 2016-12-04T13:09:13Z

Changes since last review:

rebased to current master
fixed error in current master with Transfer-Encoding header: the header now is SPEC header but it missed in __http_msg_hdr_val()
Connection header is limited to have less than 16 connection tokens for raw headers. Otherwise request or response will be filtered
Drop message if some of connection tokens is end-to-end headers used by tempesta and parsed by TFW_HTTP_PARSE_RAWHDR_VAL macro. Comparing of connection tokens with all headers defined in RFC 7230, 7231 as end-to-end will lead to a lot of string comparisons inside of parsing of Connection header
Unit tests updated to have more test cases for hop-by-hop headers marking

The feature have no special requirements for #634 . (Headers listed there are not hop-by-hop)

I tried to compare behaviour with nginx, but it support hop-by-hop headers in terms of obsoleted RFC 2616, not 7230

keshonok · 2016-12-06T09:56:38Z

tempesta_fw/cache.c

+		} else {
+			--ce->hdr_num;
+			continue;
+		}


Now that n is not used as an array index, perhaps it's best to postpone its calculation until the else if case.

if () { } else if (field - resp->h_tbl->tbl < TFW_HTTP_HDR_RAW) { } else { }

keshonok · 2016-12-06T09:59:11Z

tempesta_fw/cache.c

+		else if (n < TFW_HTTP_HDR_RAW)
+			h = &empty;
+		else
+			continue;


Please see the comment above regarding the use of n variable. In this case, n is not used anywhere else.

keshonok · 2016-12-06T10:02:55Z

tempesta_fw/http.h

 * Http headers table.
 *
 * Singular headers (in terms of RFC 7230 3.2.2) go first to protect header
 * repetition attacks. See __hdr_is_singular() and don't forget to
 * update the static headers array when add a new singular header here.
+ * If new header can be hop-by-hop header update of __parse_connection() is
+ * also needed.


The whole comment should be reworked to clearly state which arrays and where must be updated when adding a certain type of an HTTP header field.

keshonok · 2016-12-06T10:13:56Z

tempesta_fw/http.h

+ * group.
+ *
+ * @spec	- bit array for special headers. Hop-by-hop special header is
+ *		  stored as (0x1 << tfw_http_hdr_t[hid]);


Perhaps a corresponding proper BUILD_BUG_ON() is justified to ensure that the type used for the structure member can accommodate the necessary number of headers (has the required number of bits).

keshonok · 2016-12-06T10:15:11Z

tempesta_fw/http.h

@@ -262,6 +288,7 @@ typedef struct {
 * @content_length	- the value of Content-Length header field;
 * @conn		- connection which the message was received on;
 * @jtstamp		- time the message has been received, in jiffies;
+ * @keep_alive		- keep-alive timeout


The timeout specified in the Keep-Alive header field? Or something else?

keshonok · 2016-12-06T13:22:07Z

tempesta_fw/http_parser.c

+			memmove(&hbh->raw[i], &hbh->raw[i + 1],
+				sizeof(TfwStr) * (TFW_HTTP_HDR_NUM - i -1));
+		TFW_STR_INIT(&hbh->raw[TFW_HTTP_HDR_NUM - 1]);
+	}


If you want to exclude this header from further comparisons, then why not use the same TFW_STR_HBH_HDR flag on headers in hbh->raw[] and skip those headers in the comparison loop? That is easier and faster than doing a memmove().

keshonok · 2016-12-06T13:23:24Z

tempesta_fw/http_parser.c

@@ -619,6 +739,7 @@ __FSM_STATE(st_curr) {							\
 		/* The header value is fully parsed, move forward. */	\
 		if (saveval)						\
 			__msg_hdr_chunk_fixup(p, __fsm_n);		\
+		MARK_SPEC_HBH(id);					\


I believe this should be made an inline function to complement mark_raw_hbh().

keshonok · 2016-12-06T13:33:32Z

tempesta_fw/http_parser.c

+			hdr = &hbh->raw[i];
+			break;
+		}
+	}


I believe that there should be the current index into the array - analogous to ht->off. You don't have to search for the current entry.

keshonok · 2016-12-07T00:01:23Z

tempesta_fw/http_msg.c

+
+	h = &ht->tbl[hid];
+	TFW_STR_FOR_EACH_DUP(d, h, end)
+		d->flags |= TFW_STR_HBH_HDR;


Do we really need to mark each duplicate as HBH? I don't think so, but maybe there's a case to prove otherwise?

keshonok · 2016-12-07T00:06:59Z

tempesta_fw/http_msg.c

+ * name only is enough in @hdr.
+ */
+void
+tfw_http_msg_mark_hbh_hdr(TfwHttpMsg *hm, TfwStr *hdr)


I believe that this function (under a better name) belongs in http_parser.c.
If I understand correctly, the reason for placing it here was that __hdr_lookup() was located here, and it's a local (static) function to this sub-module. In my opinion though it should be the other way around: __hdr_lookup() should be made public (and given a proper name for that) and used from http_parser.c

keshonok

Looks good to me. Only minor changes requested.

keshonok · 2016-12-12T00:49:32Z

tempesta_fw/http_parser.c

@@ -2002,7 +2080,7 @@ tfw_http_parse_req(void *req_data, unsigned char *data, size_t len)
 	TfwHttpReq *req = (TfwHttpReq *)req_data;
 	__FSM_DECLARE_VARS(req);

-	__hbh_parser_init(&parser->hbh_parser, true);
+	__hbh_parser_init_req(req);


This should be called once per a message. However here it's called each time the parsing function is called. If a message is transferred one byte at a time, then this will be executed for each byte. That's unnecessary. I would suggest to call it in Req_0 state, and make another state Req_0_CRLF for skipping empty lines at the start of a message. What do you think?

keshonok · 2016-12-12T00:49:49Z

tempesta_fw/http_parser.c

@@ -3452,7 +3530,7 @@ tfw_http_parse_resp(void *resp_data, unsigned char *data, size_t len)
 	TfwHttpResp *resp = (TfwHttpResp *)resp_data;
 	__FSM_DECLARE_VARS(resp);

-	__hbh_parser_init(&parser->hbh_parser, false);
+	__hbh_parser_init_resp(resp);


Please see the comment above.

keshonok · 2016-12-12T00:58:07Z

tempesta_fw/http_parser.c

+	unsigned int id;
+
+	for (id = 0; id < TFW_HTTP_HDR_RAW; ++id) {
+		hdr = &ht->tbl[id];


TfwStr *hdr = &ht->tbl[id];

Or perhaps even

TfwStr *hdr = &hm->h_tbl->tbl[id];

keshonok · 2016-12-12T01:01:36Z

tempesta_fw/http_parser.c

@@ -495,29 +516,52 @@ static void
 mark_raw_hbh(TfwHttpMsg *hm, TfwStr *hdr)
 {
 	TfwHttpHbhHdrs *hbh = &hm->parser.hbh_parser;
+	TfwStr * hbh_name;


Extra space.

TfwStr *hbh_name;

keshonok · 2016-12-12T01:02:41Z

tempesta_fw/http_parser.c

+	 * corresponding hop-by-hop header was found.
+	*/
+	for (i = 0; i < hbh->off; ++i) {
+		hbh_name = &hbh->raw[i];


TfwStr *hbh_name = &hbh->raw[i];

keshonok · 2016-12-12T01:04:29Z

tempesta_fw/http_parser.c

+	for (i = 0; i < hbh->off; ++i) {
+		hbh_name = &hbh->raw[i];
+		if ((hbh_name->flags & TFW_STR_HBH_HDR)
+		    && !(tfw_stricmpspn(&hbh->raw[i], hdr, ':'))) {


The body of this if statement is better visible in this case when the opening bracket is on a separate line.

keshonok · 2016-12-12T07:35:22Z

tempesta_fw/http_parser.c

+/* Add spec header of id @hid and @name to list of spec hop-by-hop headers */
+#define TRY_HBH_SPEC_HDR_LAMBDA(name, hid, lambda)				\
+	TRY_HBH_TOKEN(name, {						\
+		BUG_ON(hid > sizeof(parser->hbh_parser.spec) * CHAR_BIT);\


This should be an one-time operation per whole Tempesta run. Please take a look at BUILD_BUG_ON() macro which is a compile-time check.

keshonok · 2016-12-12T09:09:57Z

tempesta_fw/http_parser.c

+{
+	TfwHttpHbhHdrs *hbh_hdrs = &req->parser.hbh_parser;
+	/* Connection is hop-by-hop header by RFC 7230 6.1 */
+	hbh_hdrs->spec |= 0x1 << TFW_HTTP_HDR_CONNECTION;


As this should be performed only once per message, shouldn't this be a direct assignment, not an OR operation?

keshonok · 2016-12-12T09:10:25Z

tempesta_fw/http_parser.c

 	 */
 	hbh_hdrs->spec |= 0x1 << TFW_HTTP_HDR_CONNECTION;
-	if (!req)
-		hbh_hdrs->spec |= 0x1 << TFW_HTTP_HDR_SERVER;
+	hbh_hdrs->spec |= 0x1 << TFW_HTTP_HDR_SERVER;


Please see the comment above regarding direct assignment vs OR operation.

__hdr_is_singular() claims to be slow, so no reason to call it multiple times.

…y-hop

replace duplicated code with macro

Applied only to headers parsed by TFW_HTTP_PARSE_RAWHDR_VAL macro

keshonok · 2016-12-14T20:14:56Z

tempesta_fw/http_parser.c

@@ -1348,6 +1348,7 @@ __parse_transfer_encoding(TfwHttpMsg *hm, unsigned char *data, size_t len)
 /* Main (parent) HTTP request processing states. */
 enum {
 	Req_0,
+	Req_CRLF,


In my opinion, this may appear too generic. Something like Req_0_CRLF may visibly clarify that this is one of the initial states.

keshonok · 2016-12-14T20:18:55Z

tempesta_fw/http_parser.c

 	__FSM_STATE(Req_0) {
+		__hbh_parser_init_req(req);
+		/* fall through */


I suspect that it's not correct to move from one parser state to another by just falling through. Generally speaking, let's suppose there are no more bytes to process. The parser still believes it's in the state where it came from, while it should be in the new state already, and it should return to the new state when more bytes are available.

All moves from one FSM state to another must be done via a macro that moves the parser to the new state. Please check a few other occurrences of this fall through use.

I have checked out all the places in code, where fall through used instead of FSM_MOVE directives. No errors there and fall through can be replaced with __FSM_JMP. Since desired state is the next one fall through is used in order not to make jump. I have no idea if jump could be optimized out, so fall through is the best option here.

__FSM_STATE saves current parser state to __fsm_const_state: https://github.com/tempesta-tech/tempesta/blob/master/tempesta_fw/http_parser.c#L120. In all the places in parser fall through is used only when we do not move our current position in message and we want to work with the same data in next stage or stages.

Any flavour of FSM_MOVE* is not an option here since parser->to_go will be set to 0 for situation you are describing ( https://github.com/tempesta-tech/tempesta/blob/master/tempesta_fw/http_parser.c#L137 ). And next stage will take next character, not the same one.

Add unit test with messages with leading CRLF

keshonok

Looks good to me.

krizhanovsky · 2016-12-28T12:54:33Z

tempesta_fw/http_parser.c

@@ -466,6 +470,186 @@ parse_int_hex(unsigned char *data, size_t len, unsigned long *acc)
 	return CSTR_POSTPONE;
 }

+/**
+ * Add spec header indexes to list of hop-by-hop headers


No need to issue a special fix now, but comments are written in English, so they should obey English rules: start from capital letter and finish with dot. Exception is same-line comments which are continuation of C operators, i.e.

a += b; /* this is same-line comment */

krizhanovsky · 2016-12-28T13:49:51Z

tempesta_fw/http.h

+ * adjusting as well as saves cache storage.
+ *
+ * Headers unconditionaly treated as hop-by-hop must be listed in
+ * __hbh_parser_init() function and must be members of Special headers


It seems the comment is outdated since there is no function with name __hbh_parser_init().

krizhanovsky · 2016-12-28T14:28:40Z

tempesta_fw/http_parser.c

+}
+
+/**
+ * Mark existing spec headers of http message @hm as hop-by-hop if they was


"they was" -> "they were"

krizhanovsky · 2016-12-28T15:44:49Z

tempesta_fw/http_parser.c

+static int
+__hbh_parser_add_data(TfwHttpMsg *hm, char *data, unsigned long len, bool last)
+{
+	TfwStr *hdr = NULL, *append;


No need to initialize hdr.

krizhanovsky · 2016-12-28T16:03:53Z

tempesta_fw/http_parser.c

+		parser->hbh_parser.spec |= 0x1 << hid;			\
+		if (!TFW_STR_EMPTY(&msg->h_tbl->tbl[hid]))		\
+			msg->h_tbl->tbl[hid].flags |= TFW_STR_HBH_HDR;	\
+	})


The macro is used in only one place, so the code would be more readable if you place the macro as inline code at the appropriate place.

krizhanovsky · 2016-12-28T16:04:33Z

tempesta_fw/http_parser.c

+			msg->h_tbl->tbl[hid].flags |= TFW_STR_HBH_HDR;	\
+	})
+
+#define TRY_SPEC_HBH_HDR(name, hid) TRY_SPEC_HBH_HDR_LAMBDA(name, hid, {})


The macro is not used at all.

krizhanovsky · 2016-12-28T16:13:10Z

tempesta_fw/http_parser.c

 	__FSM_STATE(Req_0) {
+		__hbh_parser_init_req(req);


Why did you create a new parser state for the initialization routine? It seems you can call _hbh_parser_init*() functions in tfw_http_msg_alloc() where base parser is initialized.

krizhanovsky · 2016-12-28T16:13:40Z

tempesta_fw/http_parser.c

 	__FSM_STATE(Resp_0) {
+		__hbh_parser_init_resp(resp);


The same as for request

krizhanovsky · 2016-12-28T16:17:48Z

tempesta_fw/t/unit/test_http_parser.c

+
+	/* Connection header lists end-to-end spec headers */
+	FOR_REQ(REQ_HBH_START
+		"Connection: Host, Content-Length, Content-Type, Connection,"


I believe the request must be blocked since there is special header in Connection

…nection header

vankoven mentioned this pull request Nov 24, 2016

Don't cache and don't forward hop-by-hop headers #633

Closed

krizhanovsky requested changes Nov 30, 2016

View reviewed changes

vankoven force-pushed the ik-hbh-parser branch from c05dcd7 to 8bf9ac6 Compare December 4, 2016 11:20

vankoven mentioned this pull request Dec 4, 2016

Properly store and build HTTP headers #634

Open

keshonok suggested changes Dec 7, 2016

View reviewed changes

keshonok approved these changes Dec 12, 2016

View reviewed changes

vankoven added 20 commits December 14, 2016 14:48

tls: update license indentifier

69f989e

HttpMsg: fix signed-unsigned conversions

52b8531

HttpMsg: remove multiple checks for header's singularity

44fce2c

__hdr_is_singular() claims to be slow, so no reason to call it multiple times.

Http Parser: parse keep-alive as special headers for both req and resp

a324f43

remove and don't cache headers marked as hop-by hop

9af66cb

http parser: parse connection header and mark listed headers as hop-b…

983a970

…y-hop

unit tests: http parser: add tests for requests with hop-by-hop headers

66090da

unit tests: http parser: add tests for responses with hop-by-hop headers

4f4d636

http msg: fix missed transfer-encoding header in __http_msg_hdr_val

e8c64b3

unit tests: update tests for http parser

be64e79

Fix code style according to code review

1611092

http parser: fix wrong usage of C4_INT_LCM macro

69fffd8

limit count of raw hop-by-hop headers to TFW_HTTP_HDR_NUM

298b35a

fix code according code review

e057b1a

replace duplicated code with macro

http parser: don't allow raw end-to-end headers be marked as hop-by-hop

8585f26

Applied only to headers parsed by TFW_HTTP_PARSE_RAWHDR_VAL macro

unit tests: update unit tests for hop-by hop header

5db01cf

http parser: optimize matching of raw hop-by-hop headers

f0df55d

unit tests: update tests for http parser and hop-by-hop headers

ab7b431

fix code according to code review

bd357b4

fix code according code review

386f212

vankoven force-pushed the ik-hbh-parser branch from 3f101a6 to 386f212 Compare December 14, 2016 11:00

keshonok reviewed Dec 14, 2016

View reviewed changes

http parser: fix too generic names of parser states and set rigth states

dda3d56

Add unit test with messages with leading CRLF

keshonok approved these changes Dec 16, 2016

View reviewed changes

krizhanovsky requested changes Dec 28, 2016

View reviewed changes

vankoven added 3 commits December 28, 2016 22:08

fix code according code review

8b5f00e

http parser: block messages that lists end-to-end spec headers in Con…

9f26a4b

…nection header

http parser: get rid of macro used only once

fbf0082

vankoven merged commit 72a24ae into master Dec 29, 2016

vankoven deleted the ik-hbh-parser branch December 29, 2016 05:17

Don't cache and don't forward hop-by-hop headers #650

Don't cache and don't forward hop-by-hop headers #650

Conversation

vankoven commented Nov 24, 2016

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 commented Dec 3, 2016

vankoven commented Dec 4, 2016 • 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

keshonok left a comment

Choose a reason for hiding this comment

keshonok Dec 12, 2016 • 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

keshonok Dec 12, 2016 • 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

keshonok Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

vankoven Dec 16, 2016 • 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

vankoven commented Dec 4, 2016 •

edited

Loading

keshonok Dec 12, 2016 •

edited

Loading

keshonok Dec 12, 2016 •

edited

Loading

keshonok Dec 14, 2016 •

edited

Loading

vankoven Dec 16, 2016 •

edited

Loading