Re: [PATCH] Add request_header_add option and [request|reply]_header_* manglers fixes

On 1/07/2012 2:28 a.m., Alex Rousskov wrote:
> On 06/29/2012 08:45 PM, Amos Jeffries wrote:
>> On 30/06/2012 3:53 a.m., Alex Rousskov wrote:
>>> On 06/29/2012 01:44 AM, Amos Jeffries wrote:
>>>> Can you please make this use the more flexible (logformat) tokens
>>>> from format/libformat.la instead? I'm trying to kill off the older
>>>> token sets and this is adding yet another format tokenizer to keep
>>>> track of.
>>> I do share the desire to unify macro parsing and substitution in various
>>> Squid places (logging, error pages, helpers, and various squid.conf
>>> options with string values). It will give us a lot more configuration
>>> power and better code. However, I think that should not be placed as a
>>> requirement on this feature because:
>>>
>>> a) We already have several different pieces of code that do macro
>>> substitution or implement a similar functionality. It would be best to
>>> have a dedicated project that will unify all that code, taking into
>>> consideration all the different code requirements we already have. This
>>> will be a difficult, sizable project, focusing on internal code
>>> structure rather than user-facing functionality.
>> So why add a 5th one? Particulary, a 5th with distinct set of tokens.
> because the existing ones could not satisfy our needs (without
> significant changes) _and_ adding yet another one did not make the
> problem much worse. Now, it is possible that we got it wrong, and the
> libformat API can actually be used without much changes. I will double
> check.

Thank you. In most places the problem is that AccessLogEntry is the
closest thing we have to a XactionData object for the whole HTTP
transaction. (I still have patches waiting around to polish up and
submit for audit creating that master transaction object).
AccessLogEntry is available from the ConnStateData / conn() almost
everywhere in the code. So it *is* actually available everywhere through
the transaction, just not filled out. We have a lot of code cleaning to
make it consistent, (but outside this patches scope).

For now it is available via "request->conn()->ale". Or a new temporary
one created in local scope for the function doing display output. It may
need filling out with pointers to things required (is SSL content, HTTP
request, client connection), the code before logging is very
inconsistent/non-existent.

>
>
>> There is a dedicated project to unify the tokenizers. It produced
>> libformat.la. I'm just asking for that project not to be undermined by
>> new code. We are already going to have to create special
>> backward-compatible parsing for ssl_crtd and external_acl_type formats.
> What special parsing does ssl_crtd need in libformat area?

Sorry. Mea culpa. It was the OpenSSL errors codes for %D expansion. The
error page formats can also be added to that list of special token sets.

>> The main objection from me is the UI tokens.
>>
>> I would be happy with a temporary tokenizer provided:
>> * is used the same logformat token *names* where an existing one
>> matches the content (eg %SRC ---> %[tcp::]>a)
>> * tokens for SSL used a namespaced format consistent with the
>> documented logformat tokens. Example:" %ssl::>{field}cert " - when
>> libformat does not (currently) provide a matching token already. In a
>> namespaced form consistent with the other documented logformat tokens.
>> But why are these needed immedately in the first round of header_add
>> patch anyway?
> I am happy to rename new macros in the patch to standardize in the
> logformat direction. I probably picked the names from the parts of
> squid.conf that have not been renamed in the right direction yet. Our
> uncertainty which names to use should have been pointed out before or
> during the patch submission, sorry.

Going by the patch display functions these are what the token names
should be for consistency:

%LOGIN ==> %ul [username from authentication]
%SRC ==> %>a [client IP address]
%DST ==> %<A [destination FQDN]
%% [no change]

The SSL tokens we have not exactly defined a namespace for since logging
does not log them yet. Since my earlier posts about %ssl:: namespace it
occurs to me we should probably be taking the opportunity to move to TLS
terminology. Which would make it "%tls::" prefix to TLS/SSL format
tokens. Agreed?

Proposed tokens:

%user_cert_subject ==> %tls::{DN}>cert

%user_ca ==> %tls::{DN}>ca

* since we have no need to limit the attributes pulled from the
certificate or CA I've used the same syntax as HTTP headers to access
particular named field-values.

* Is there a long name we can use instead of "DN" ?

Attached is a quick experimental patch to add these tokens to libformat.
Its got a few bugs I can see already, but you get the idea?

(snipping the rest of this since we get WAY off topic of the patch. You
seem to understand what the problems well enough. )

Amos