Re: [PATCH] meta_header option

On 21/09/2012 4:56 a.m., Tsantilas Christos wrote:
> On 09/20/2012 08:56 AM, Amos Jeffries wrote:
>> This documentation leaves a lot to be desired in terminology clarity.
>> Critique below...
>>
>>
>> On 20/09/2012 3:46 a.m., Tsantilas Christos wrote:
>>> Hi all,
>>>
>>> This patch adds meta_header option to squid.conf. It is similar to
>>> adaptation_meta but is applied after all adaptation and before logging.
>>> Values of named meta headers can be logged using %{name}meta_header
>>> macros.
>>>
>>> meta_header name value acl ...
>>> logformat myFormat ... %{name}meta_header ...
>>>
>>> This option may be initially used to log custom information about the
>>> master transaction. For example, an admin may configure Squid to log
>>> which "user group" the transaction belongs to, where "user group" will
>>> be determined based on a set of ACLs and not [just] authentication
>>> information.
>> Um at first reading that seems to make sense, until the particular
>> concepts are investigated. Then it does not make as much sense at all.
>>
>> In absence of authentication there is no "user". Without user there is
>> no "user group". This absence of authentication is important due to the
>> property of client != user. Where 1 client may be multiple "user" each
>> belonging to multiple or unique "user groups".
>>
>> I think what is actually being described here is a transaction group
>> label, aka flow label in some of the networking documents, squid-2 used
>> to have a urlgroup concept matching this which we dropped in favour of
>> myportname in squid-3.
> I agree, we should talk about a "transaction group" not a "user group"...
>
>>> From user point of view,
>> you mean "admin" surely?
> Admin or customer or whatever...
>
>>> adaptation_header sets/implies meta_header
>>> (i.e., setting adaptation_header is sufficient to be able to log it
>>> using %meta_header) but the meta_header option itself (if any) is
>>> evaluated later, so it has no effect on ICAP headers.
>> If I'm translating that "adaptation_header" adds an HTTP header before
>> adaptation, "meta_header" adds a header field *after* adaptation?
>>
>> Which begs the question:
>> * why are we adding these via special directives to unject headers into
>> the HTTP data stream and not using the new header_add ?
> They are not real headers. They are not sent to the HTTP server or the
> client. They are for internal squid use.
>
>> * why not simply use %<h{} and %>h{} to pull out the custom inserted
>> headers by name instead of adding a new %meta_header log option? surely
>> the admin knows what headers they inserted into the client data.
> They can not be logged using %?h{} formating codes... They are a
> different think
>
>> ** If not *why the heck* is arbitrary bytes being injected directly
>> into the HTTP stream? this will be an information leak vulnerability on
>> one HTTP traffic flow or the other.
> They are not added to http stream. They are for internal use...
>
>>
>> Adaptation::Ecap::XactionRep::start()
>> * there should be no need to remove const from the clients raw HTTP data
>> stream objects (virgin request/reply). They are const to protect them
>> from accidental modification/corruption and this method appears to be
>> only reading them for adding a *copy* to the ah.
> Unfortunately the following code:
> const char *v = (*i)->match(request, reply);
> some lines after requires non-const HttpRequest object. It is a call to
> MetaHeader::match method which calls the ACLFilledChecklist constructor,
> which requires non-const HttpRequest object
>
>
>> * also, how can theVirginRep.raw().header be cast to both Request and
>> Reply without one being wrong? surely the second should be
>> const HttpReply *reply = NULL;
>> if (theCauseRep)
>> reply = dynamic_cast<const HttpReply*>(theVirginRep.raw().header);
> It is a part of code used in many places inside ecap and icap.

Add this to your code to see what I mean:

   if (!theCauseRep)
     assert(request == reply);

in the patch as submitted previously the assert will be tested on
requests. I dont expect it to stop squid running though.

Due to:

   HttpRequest*request = ... theVirginRep.raw().header);
   HttpReply*reply = ...theVirginRep.raw().header);

Amos
Received on Fri Sep 21 2012 - 09:31:39 MDT