Re: [PATCH] Consumption of POST body

On 01/21/2013 07:04 AM, Steve Hill wrote:

> I have changed to using expectNoForwarding() and removed the
> stopReceiving() call from noteBodyConsumerAborted() and this appears to
> work correctly:

Good, thank you for checking this! We just need to decide whether
reading the entire request is what we want in all cases. The earlier
changes were done to stop Squid reading that request (because reading
the whole requests wastes a lot of resources if the request is huge or,
worse, malicious).

> I don't really understand the purpose of the readMore flag, can you
> explain it?

It is one of the factors that Squid connection context uses to read the
new request from the client connection.
ConnStateData::clientParseRequests() is key here. Please note that Squid
connection context is not the only object that may do reading and that,
in theory, that context does not deal with reading request bodies. See
also: mayUseConnection().

The whole thing is complex, messy, and, hence, buggy.

>> * 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.

It is difficult (for me) to describe all its current [side] effects,
which is a part of the problem, but it is close to "Squid client side
needs to read more incoming data, either for the current request or to
get the next request".

> 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?

Yes, handleAdaptationFailure() may be called when adaptation fails in
the middle of the response. However, handleAdaptationFailure() is for
REQMOD adaptations (which may overlap with RESPMOD adaptations in some
cases!).

>> * 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.

> 4. The client makes a request that generates an error, uses chunked
> encoding and sends an infinitely long body.

> 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.

Yes, (4) is the interesting case here. We do prevent (4) on certain
adaptation errors. This is what prompted the original changes in this
area! It feels wrong to just undo those changes without a discussion
(because they were deemed useful because they were solving a real
problem for some users at the time).

However, I agree that we do not prevent (4) in many (all?) other cases.

So, should we

a) undo the existing limited protection (because it is limited and
conflicts with the cases were we do want to read the whole thing, at
least as far as code is concerned);

b) expand the existing protection to all cases (because it is better
than allowing case #4);

c) adjust behavior based on existing and/or new configuration options,
to make the choice between a and b depend on the current circumstances
and expand the configurable protection to all cases; or

d) polish and apply your changes, resulting in protection for the
previously covered cas(es) and no added protection for other cases. Wait
for somebody to care enough about a more comprehensive and consistent
solution to make it happen. This is essentially what my suggestions were
meant to accomplish if "discuss" items lead nowhere.

> On 01/21/2013 04:19 PM, Amos Jeffries wrote:
>> 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.

This idea is for option (c) above. I like it a lot because the proposed
behavior is meaningful, flexible, and does not require new options.
Steve, do you think you have the cycles to implement Amos' suggestion?
It is not going to be easy, but it would be the best outcome of this
discussion (that I can think of).

Thank you,

Alex.
Received on Tue Jan 22 2013 - 02:42:19 MST