Re: [PATCH] Compliance: Forward 1xx control messages to clients

On 08/31/2010 04:39 AM, Amos Jeffries wrote:
> Alex Rousskov wrote:
>> Compliance: Forward 1xx control messages to clients that support them.
>>
>> Forward 1xx control messages to all HTTP/1.1 clients and to HTTP/1.0
>> clients that sent an Expect: 100-continue header unless the 1xx
>> message fails the optional http_reply_access check described below.
>>
>> RFC 2616 requires clients to accept 1xx control messages, even if they
>> did not send Expect headers. However, 1xx control messages prohibited
>> by http_reply_access are ignored and not forwarded. This can be used
>> to protect broken HTTP/1.1 clients or naive HTTP/1.0 clients that
>> unknowingly forward 100-continue headers, for example. Only fast
>> checks are supported at this time.
>>
>> The patch removes ignore_expect_100 squid.conf option and the
>> corresponding code because
>>
>> - the reasons to treat 100-continue specially have changed since we
>> can now forward 1xx responses;
>>
>> - rejection of 100-continue request can still be done using a
>> combination of the existing http_access and deny_info options;
>>
>> - hiding of 100-continue header from next hops can still be done using
>> an existing request_header_access option;
>>
>> - 100 (Continue) responses can be hidden from clients using
>> http_reply_access check described above.
>>
>>
>> We still respond with 417 Expectation Failed to requests with
>> expectations other than 100-continue.
>>
>> Implementation notes:
>>
>> We forward control messages one-at-a-time and stop processing the
>> server response while the 1xx message is being written to client, to
>> avoid server-driven DoS attacks with large number of 1xx messages.
>>
>> 1xx forwarding is done via async calls from HttpStateData to
>> ConnStateData/ClientSocketContext. The latter then calls back to
>> notify HttpStateData that the message was written out to client. If
>> any one of the two async messages is not fired, HttpStateData will get
>> stuck unless it is destroyed due to an external event/error. The code
>> assumes such event/error will always happen because when
>> ConnStateData/ClientSocketContext is gone, HttpStateData job should be
>> terminated. This requires more testing/thought, but should still be
>> better than not forwarding 1xx messages at all.
>
> This makes me wonder why only fast ACL are supported in the
> http_reply_access for these messages.

I did not add slow ACL support because I doubt slow ACLs would be used
here, and because I wanted to reduce implementation time and complexity.
Once somebody discovers the genuine need for slow ACLs here, they should
add the corresponding code.

> This write-through might be doable as a two-step sequence:
> Step 1: check ACLs -> OK go to step 2 or ERR drain the 1xx from server
> and loop back.
> Step 2: write the 1xx to client, drain from server while doing so.
>
> Not a request. Just wondering about implementation choices.

Adding slow ACL support is straightforward because the code already has
an async step, but we cannot easily read from the server while writing
1xx to the client because we will not be able to do anything with the
incoming response data:

1) We may read another 1xx and would not be able to handle it. HTTP
allows the server to send any number of 1xx control messages without
receiving any more request data. If we start creating and queuing 1xx
messages, we will be subject to DoS attacks due to memory exhaustion.

We could discover and keep the second 1xx in the read buffer, but that
would complicate an already non-trivial state. I think we should not do
that until we find some compelling reasons to add such complexity.

2) We may read the beginning of the true response but would not be able
to send it to Store because the client-side would not be ready to write
it to the client because there can be only one writer at a time and we
are writing 1xx.

>> ----
>>
>> I believe the current approach addresses Henrik's comments regarding
>> forwarding to HTTP/1.0 clients without drop_expect_100. An admin would
>> be able to prohibit such forwarding (globally or selectively using
>> http_reply_access). If HTTPbis changes the specs later, we can change
>> the default.
>
> Agreed.

> +1.
>
> sendControlMsg() and writeControlMsg() could do with being doxygen
> format on their documentation comments.

Will fix writeControlMsg(). For sendControlMsg(), they are there:

> client_side.h- // HttpControlMsgSink API
> client_side.h: virtual void sendControlMsg(HttpControlMsg msg);
>
> HttpControlMsg.h- /// called to send the 1xx message and notify the Source
> HttpControlMsg.h: virtual void sendControlMsg(HttpControlMsg msg) = 0;

The "this is parent's API" comments should not use doxygen syntax,
right? I hope doxygen understands virtual functions and handles their
documentation correctly. Does it?

> Now that this is done where is request de-chunking at?

The next step is implementing the server version cache.

Thank you,

Alex.
Received on Tue Aug 31 2010 - 16:12:28 MDT