fixups for sticky cookie calculation in case of segmented data

[i-rinat]

If request comes in more than one packet, sticky cookie value may be in chunks. The patches handle that case, and also force cookie to be regenerated if inbound cookie was of wrong length.

[vankoven]

I've tried to run this branch, but it still seems to be unstable if cookie value is split between chunks.

1. Start Tempesta with config:

server 127.0.0.1:8080;
sticky enforce;
sticky_secret "f00)9eR59*_/22";

2. start nc and send a couple of requests:

% nc 192.168.122.12 80                                                                                                              :(
GET /hello HTTP/1.1
Host: 192.168.122.12
User-Agent: curl/7.61.1
Accept: */*

HTTP/1.1 302 Found
Date: Mon, 29 Oct 2018 14:01:53 GMT
Location: http://192.168.122.12/hello
Set-Cookie: __tfw=000000010016beaefe7b50e07fbd97c049f3ddbdb8629d1691a84167
Content-Length: 0

GET /hello HTTP/1.1
Host: 192.168.122.12
Cookie: __tfw=000000010016beaefe7b50e07fbd97c049f3ddbdb8629d1691a84167
My-Hdrff: sdffsf
User-Agent: curl/7.61.155fff
Accept: */*

HTTP/1.1 302 Found
Date: Mon, 29 Oct 2018 14:02:13 GMT
Location: http://192.168.122.12/hello
Set-Cookie: __tfw=000000010016beae6a86104393bc02de1f2a3b8cc7f154d16a4ffdeb
Content-Length: 0

3. In the same time I see following issues in the logs:

[ 6295.920463] [tempesta] Warning: http_sess: bad received HMAC value for 192.168.122.1: f(pos=0), ts=0x10016beae orig_hmac=[6a86104393bc02de1f2a3b8cc7f154d16a4ffdeb]

4. If I use the cookie from the last 302 response, I got my 200 OK reply.

[i-rinat]

@ikoveshnikov,
it looks like you're using different user agent in the second request: curl/7.61.155fff, while in the first one it was curl/7.61.1. I believe that's the reason hmac value is different.

[vankoven]

Ah, yes, shame on me. I should have checked it before posting the comment.

[vankoven]

Good to merge, but i would like to see additional test for http parser, since sticky cookie module highly relies on its behaviour. Please, also backport the patch to 0.5 release.

[vankoven]

Doesn't really matter for this particular test, but we have a special macro for plain strings: TFW_STR_PLAIN().

[i-rinat]

Code was modified to use TFW_STR_PLAIN().

[vankoven]

Please, also add a case, when stop character couldn't be found at the beginning of a new chunk, so resulting string won't overcome end limit. This is checked in tests above, but not for the case when stop character is provided.

[i-rinat]

Added.

[vankoven]

Not the issue of this PR actually, but can you add one more test for the http parser? Cookie calculations add several requirements for the http parser:

• cookie name starts at a new chunk, and this chunk is marked with TFW_STR_NAME flag
• cookie value starts at a new chunk, and this chunk is marked with TFW_STR_VALUE flag
• cookie delimeter character(;) starts at a new chunk

This requirements aren't checked anywhere, but http_sess.c code highly relies on it.

[i-rinat]

Ack. I'll add it.

[aleksostapenko]

Good to merge, with minor comment.

[aleksostapenko]

This function was intended to collect cookie value in any case (or BUG_ON must be triggered), so it didn't return value. But now it can return with no value found and caller will not know anything about this. Perhaps it is worth to change it to return true/false or 0/-ENOENT; also the similar checks before tfw_str_collect_cmp calls - looks redundant now.

[i-rinat]

I can't shake the feeling that it's wrong to add any return value to this function. It has a verb in the name, so by convention should return 0 if there are no errors, and other values otherwise. But this function cannot fail. It's not an error to return empty string if an empty slice was provided. If we add a return value, somebody reading code at the call site will get an impression of error handling, which it is not.

So I think it is good to leave testing for chunk == end before calling this function. It's a duplication, but I don't think checking should be removed from this function, as it may result in accidental reading outside of allocated memory.