Re: [PATCH] Consumption of POST body

On 22/01/2013 3:04 a.m., Steve Hill wrote:
> On 18.01.13 18:37, Alex Rousskov wrote:
>
> Further to this...
>
>> * Use expectNoForwarding() instead of adding forwardToBitbucket().
>>
>> * In ConnStateData::noteBodyConsumerAborted() do not stop reading if
>> flags.readMore is set. Just keep auto-consuming. This may prevent
>> authenticating connections closures even though expectNoForwarding() is
>> used. Needs to be tested: Will we correctly stop and switch to the new
>> request when the body finally ends?
>
> I have changed to using expectNoForwarding() and removed the
> stopReceiving() call from noteBodyConsumerAborted() and this appears
> to work correctly:
>
> 1. Making a request which results in an error, with "Connection:
> close" produces a response to the client, and then once the request
> body has been fully received, Squid closes the connection.

Does this work with a 70,000 clients all doing 100 byte request bodies
sent 1 byte per minute (header sent normally)?
  The DoS attack PoC expectation is that the proxy becomes unresponsive
for about an hour while the whole TCP available socket range has
ESTALISHED connections.
  The normal Squid operation expects that no TCP sockets held in
ESTABLISHED state past the first second, and resumes full availability
within 15 minutes. Although some requests may fail if the sockets stay
in TIME_WAIT for long. If possible the TIME_WAIT is avoided and full
availability is never lost.

>
> 2. Making a request which results in an error, with "Connection:
> keep-alive" produces a response to the client, and then once the
> request body has been fully received Squid reads the next request.
>
> 3. Making a request which does *NOT* result in an error, with
> "Connection: keep-alive" works as expected and then waits for the next
> request.
>
> 4. Making a request which does *NOT* result in an error, with
> "Connection: close" works as expected and then closes the connection.
>
> 5. Making a request which results in an ICAP error (I shut down a
> non-bypassable ICAP service), with "Connection: keep-alive" produces a
> 500 error response and then waits for the next request.
>
> 6. Making a request which results in an ICAP error, with "Connection:
> close" produces a 500 error response and then Squid closes the
> connection.
>
> I don't really understand the purpose of the readMore flag, can you
> explain it?
>

It's a hack due to some internals of Squid needing direct read-access to
the client socket to perform their actions. SSL handshake and CONNECT
tunnel body handling being the big remaining ones these days. It signals
the ConnStateData object which is normally responsible for the I/O read
handler that it is *not* to process any incoming data on the socket
until the flags is set back to true.
  It is/was very likely also being abused to signal that the error cases
have occured and the input is to be dropped by TCP shutdown.

>> * Discuss and possibly remove readNextRequest setting from
>> ClientHttpRequest::handleAdaptationFailure(), to remove inconsistency
>> discussed in #2 above.
>
> As mentioned above, I don't understand what readMore is actually for.
> However, as I mentioned in my previous email, the only time I can see
> us wanting to blindly close the connection for adaptation failures is
> when we've already started sending a response to the client and the
> adaptation service blows up in the middle of the response body. It
> doesn't _look_ like handleAdaptationFailure() handles that event
> though, unless I'm mistaken?
>
>> * Discuss whether we are OK with clients tying up Squid after an
>> [authentication] error. Perhaps we need another timeout there, perhaps
>> there are already mechanisms to address that, or perhaps we do not
>> really care.
>
> There are 4 possible ways I can see of a client tieing up Squid:
>
> 1. The client connects but doesn't send anything.
> 2. The client connects, makes a keep-alive request that results in an
> error, then doesn't send any new request.
> 3. The client makes a request that generates an error, but doesn't
> send the complete request body.
> 4. The client makes a request that generates an error, uses chunked
> encoding and sends an infinitely long body.
>
> (1) and (2) are basically the same thing. I've tested this and Squid
> times out the connections and closes them after a few minutes.
>
> (3) similarly gets timed out after a while.
>
> I haven't tested (4), but I have no reason to believe there's anything
> that currently stops it from happening. I'm not sure whether or not
> we should do something to prevent it - that's one for discussion.
>

If the data is being bropped on arrival by Squid we should be watching
for these cases and aborting client connections which go on too long. I
suspect the read_timout timer should be kept ticking on these reads and
not reset to 0. That would cap the abuse time at 15 minutes per client
rather than infinite.

Amos
Received on Mon Jan 21 2013 - 23:19:41 MST