Re: [PATCH] meta_header option

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.

>
> From user point of view,

you mean "admin" surely?

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

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

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

Amos
Received on Thu Sep 20 2012 - 05:57:02 MDT