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
