Re: Helpers and Notes

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?
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.
So if no more than one values should used then the HelperReply.cc code
should not add the second "url=" kvpair. It is simple.
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.

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

>
> 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....
Received on Sun Jan 27 2013 - 17:32:16 MST