Re: [PATCH] meta_header option

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.

>
> or even use if(theCauseRep) to wrap around two different sets of casts
> for REQMOD and REPMOD setup.

You are correct here.

>
> Amos
>
Received on Thu Sep 20 2012 - 16:56:50 MDT