Re: Helpers and Notes

On 28/01/2013 6:32 a.m., Tsantilas Christos wrote:
> On 01/27/2013 07:12 AM, Amos Jeffries wrote:
>> On 27/01/2013 8:27 a.m., Tsantilas Christos wrote:
>>> On 01/26/2013 06:05 AM, Amos Jeffries wrote:
>>>> On 26/01/2013 10:39 a.m., Tsantilas Christos wrote:
>>> I did not found such cases in the existing code.
>>> What I am seeing is the following usage:
>>> Note::Pointer statusNote = reply.notes.find("status");
>>> ...
>>> const char * result = statusNote->firstValue();
>>> status = (http_status) atoi(result)
>>> ...
>>> urlNote = reply.notes.find("rewrite-url");
>>> if (urlNote != NULL && strcmp(urlNote->firstValue(), http->uri)
>>> ....
>> Which requires two local Pointers to access the value.
>> firstValue() is the accessor Alex had me add to hide the worst part of
>> the de-referencing.
>>
>> Without it we would have:
>>
>> reply.notes.find("status")->values[0]->value.termedBuf()
>>
>> where it is preferrable to go:
>> reply.notes.find("status")->firstValue();
>>
>>
>>> The above if we use NotePairs instead of Notes can be easily written as
>>> follows:
>>> String status = reply.notes.getByName("status");
>>> status = (http_status) atoi(status.termedBuf())
>> Your example works for numeric values because ',' is not a valid digit
>> and atoi() will abort before parsing the already parsed values.
>>
>> It gets worse when handling URLs or any other value type which coudl
>> ontain whitespace or ',' values internally:
>>
>> 1)
>> helper reply: OK url=http://example.com/ url=http://invalid.invalid/
>>
>> translates to a single URL. String uri = reply.notes.getByName("uri");
>> will produce URL "http://example.com/, http://invalid.invalid/" which
>> the lenient URL parser (default behaviour) will see as
>> "http://example.com/,%20http://invalid.invalid/".
> Is it valid for a helper to return more than one kv-pairs with the same
> key?

Yes and no.

For some of the planned keys like group= and tag= (maybe store-id=), it
is expected that there will be duplicates. For others like status= and
url=, redirect-url=, it is expected that they will be used once. The
code with firstValue() is writen so that only the first value matters
and any duplicates are ignored until logging.

> The current code does not permit it. It will take care only for the
> first kvpair. It adds multiple values to the same note key, but it uses
> only the first.

They are permitted at present but not used.
The HelperReply parser scope does not have enough information to do the
duplicate checking, so the higher level callback handler it delivers the
details to is left to cope with duplicates.

> So if no more than one values should used then the HelperReply.cc code
> should not add the second "url=" kvpair. It is simple.

The HelperReply.cc code does not know which interface the helper is
using. Multiple url= parameters are valid (as unused annotations) on all
the auth helpers interfaces. But not so much on the URL helper interfaces.

> If we are going to support more than kv-pairs with the same key then the
> HTTP header like behavior is very good choice.
> See also my comment bellow for the "tag=".
>
>> 2)
>> helper reply: OK user="J, Smith" group="group1,A" group=group2
>> user=alias group=A
>>
>> String group --> "group1,A, group2, group3" ... how does one
>> determine that a group match is supposed to be equal to "group1,A" and
>> not match "group1" or "A" individually, BUT must match "group2" or "B"
>> individually?
>>
>> String user ---> "J, Smith, alias" ... what username is this? it won'
>> match any, and splitting on ',' will also likey not match any. But "J,
>> Smith" could match.
> Again the current code does not support multiple kv-pairs with the same
> key.
> The helper coders can use different tokens for example "J.Smith" or
> "group1+A"
>
> Also see my comment bellow for the "tag=" case.

The tokens are coming straight from the actual user/password/group names
from whatever backend the network is using. We have zero control over
what they contain.
On delivery to Squid the value is de-coded into those original plain
binary characters (UTF-8? ISO-8859-1?) before being added to the notes
kv-pair list. We can't count on the ',' staying coded in some
helper-controlled encoding for its whole usage through Squid.

>
>> 3)
>> helper reply: OK tag=1 tag=something tag=magic
>>
>> Will produce a single tag String "1, something, magic". What does the
>> tag ACL type do?
> What the tag currently does?
> Good question. Thanks you for this question.
>
> Assume that you have one ACL helper. You want with one lookup to this
> ACL helper to take 3-4 decisions. For example:
> 1) Do or not sslbump
> 2) Send or not to one or more ICAP services for example to an antivirus
> service.
>
> If I am not wrong (please fix me) currently the helper can return:
> tag=DOSSLBUMP
> or
> tag=DOSSLBUMP:ANTIVIRUS
> or
> tag="ANTIVIRUS"
>
> How are you going to use it in acls? The acls will look a little strange.
>
> We can allow multiple tag kvpairs and then we can support the followings:
> 1) support regex in tag acls. We can enable it using the "-e" flag in
> acls or just use the regex:"Value" in acls to specify that a regex
> expression should used to match this acl value.
> 2) The tag acl will modified to match ANY of the strings in list (for
> example using the HttpHeader::hasByNameListMember method).
>
> I prefer the first one.
>
>> Notice that all these are parsing the strings a second time just to
>> interpret the way NotePairs has merged (using a memory copy probably)
>> into a single String.
>>
>> Things like this is where the "desired" behaviour of NotesPairs key
>> merging is more of a problem to be worked around than a good thing.
>>
>>
>> I hope you can see that these are *not* desired behaviours. But they are
>> an integral part of NotePairs due to its implementation using objects
>> optimized for Mime header semantics. NotesList is not quite right either
>> but offers cleaner access suitable for all these use cases.
> NoteList is not the correct structure. Re-writing the NotePairs to be
> implemented with a similar way the NoteList is implemented (a vector) is
> easy. But we may loose some features required when logging the notes.

I don't think it needs to be a vector. But it does need an extra set of
accessors outside of the HttpHeader dependencies and semantics.

>> If you know of an interface which can store and use the kv-pair from
>> helpers in a way which the helpers kv-pairs need to be accessed I would
>> be happy to audit the patch.
>>
>>
>>> ...
>>> status urlNote = reply.notes.getByName("rewrite-url");
>>> if (!urlNote.empty() && strcmp(urlNote->termedBuf(), http->uri)
>>>
>>> Moreover we can add a member in NotePairs the
>>> const char *NotePairs::find(const char *key)
>>> to simplify it more.
>> We could. Notice that this is already present in NotesList.
>>
>>> Also we have an interface in configured Notes to deny user to use some
>>> names for notes which are reserved. We can add here the helper reserved
>>> names like "status", "rewrite-url" etc.
>> They are not forbidden for use. They are in fact *expected* for the
>> helper to use. The only significance a reserved key has in the helper
>> protocol is that Squid performs special processing for them. With side
>> effects that may or may not be wanted, but are up to the users choice.
>>
>>> Maybe we should require by the user to use always the X- prefix in his
>>> kv-pairs keys. For example the "status" is a key which can be selected
>>> by anyone. The user is better to use the "X-Status" as kv-pair key.
>> Which would make x-status the reserved key name and status key name mean
>> nothing. Which brings us back to the problem of reserving X-Status ...
>>
>> ... "user" is not what you seem to think. "user" in this case is an
>> automated helper. The only possible limits on key name are that it may
>> not contain whitespace, CR, LF, \0, or '=' characters. *anything* else
>> confirming to the key=["]value["] syntax is acceptible.
>>
>> X-* Mime tokens are nothing special and due to Notes use in ICAP headers
>> X-* is a potential overlap with reserved key names. For example;
>> possibly an external_acl_type helper may be wanted to produce
>> X-Client-Username:foo for relay to ICAP dynamically.
>> The helpers interface defines '_' suffix as the reserved marker for
>> custom local key names on all helper interfaces. This is intentionally
>> specific to the helper interface and intentionally invalid Mime header
>> character to prevent the above cases unexpectedly adding protocol
>> injection vulnerabilities from helper supplied keys. Crossover between
>> notes and ICAP/HTTP in future could still do injection if desirable and
>> valid header names used as keys, but explicit use of _ for custom
>> prevents mistakes causing problems.
> But still the "_" is not a requirement. It is an notation. We can simple
> not use the "_" and use something else....
>
> To many comments :(
> I will try to response to the others in a separate mail....
>

Ok.

Amos
Received on Sun Jan 27 2013 - 23:36:41 MST