Reorganize http_receive()

[DimitriPapadopoulos]

Prepare for adding an external HTTP library later on.

The pull request will help use external HTTP / XML libraries later on. At this point I am not certain we will benefit from using PicoHTTPParser in terms of maintainability and less code. Yet I'm opening the pull request to perform some code clean up, add a few assertions here and there, and raise a few issues.

For example we do not catch SSL errors if the header has already been read. Here is the relevant part in the new code:
https://github.com/DimitriPapadopoulos/openfortivpn/blob/230b158/src/http.c#L177-L181
and in the old code:
https://github.com/adrienverge/openfortivpn/blob/8b0c235/src/http.c#L217-L221
@adrienverge Is this on purpose? Are we lenient on purpose? On my machine I have modified the code to be strict on SSL errors, whether the HTTP header has been read or not. I haven't seen any occurrence of SSL errors.

[adrienverge]

@DimitriPapadopoulos I don't know about SSL errors, I can't remember.

All I can tell is that I tried to avoid having external dependencies, hence the HTTP decoding inside our source.

[DimitriPapadopoulos]

Oh, agreed about the dependencies. I'm after small libraries that can be included in the source code.

PicoHTTPParser is just two files picohttpparser.c and picohttpparser.h.

[adrienverge]

I'm after small libraries that can be included in the source code.

👍

[DimitriPapadopoulos]

I have pushed the code that is stricter on SSL errors. I will keep testing it for a few days.

[DimitriPapadopoulos]

OK, it appears we are lenient on SSL errors after the header has been read because of change 70dadda. Specifically the change from:

	if (n < 0) {

to:

	if (!header_size) {

The comment reads:

So, let's just obey the Content-Length header. If it's missing (chunked
encoding or standards violation) then read until the EOF, but still
succeed if the header was complete. This way we're hopefully on the safe
side.

Yet I don't see fundamental differences in the way we process chunked encoding vs. Content-Length-based responses. If Content-Length is known, we stop as soon as we read the expected length:

openfortivpn/src/http.c

Lines 204 to 205 in 8b0c235

	if (bytes_read >= header_size + content_size)
	break;

If on the other hand the response is chunked-encoded, we stop as soon as we detect the last chunk terminator:

openfortivpn/src/http.c

Lines 198 to 202 in 8b0c235

	/* Last chunk terminator. Done naively. */
	if (bytes_read >= 7 &&
	!memcmp(&buffer[bytes_read - 7],
	"\r\n0\r\n\r\n", 7))
	break;

So what is this "'safe side" about? I have verified that checking strictly for SSL errors still works when parsing chunked-encoded responses.

The code that handles the chunked encoding has been introduced in f1bfe16, a few days after 70dadda. My guess is that the "safe side" part had probably been left over because the author wasn't yet certain about the robustness of the chunked encoding code:

FortiOS 5 talks chunked. We don't really follow; but let's assume that
the if the packet ends with a zero-sized chunk we're done.

Kind of a change that works in testing and breaks in production.

Yet I believe the chunked encoding code has proven quite astute and robust over the years. It believe it is time we parse HTTP following the rules - so that we can move to 3rd party HTTP parsing code if needed.

[mrbaseman]

I have a few small comments just to be sure I have understood the changes.

[mrbaseman]

With the explanations now, I think the changes are good.

[DimitriPapadopoulos]

I'll try to provide more comprehensive explanations in the commit.

[mrbaseman]

maybe just a few buzzwords like "code cleanup and improve readability" would have lead my thoughts in the right direction :-)