Re: [PATCH] Unknown cfg function

On 07/30/2013 11:15 PM, Amos Jeffries wrote:
> On 31/07/2013 6:47 a.m., Tsantilas Christos wrote:
>> On 07/30/2013 09:17 PM, Amos Jeffries wrote:
>>>> However Amos refers to an other case. For the following line:
>>>> "Simple Tokens"
>>>> we may want to retrieve the token "Simple
>>>>
>>>> Do we have any example where this is required? (Not for regex, for
>>>> regex
>>>> we have an exception...)
>>> In most places where we accept header values in the legacy parser.
>> Well the request_header_add already understand quoted strings.
>>
>> The parse_http_header_replace should use the
>> ConfigParser::NextQuotedOrToEol.
>> I just show that there is a problem here. It will work as is. But the
>> code is not correct....
>
> I don't think it will work as-is. If I were to be replacing ETag headers
> they contain a single value with "" around it. Or any other header
> holding a quoted-string field value, this is more likely with custom
> headers. Then the NextToken it is currently using will corrupt the
> resulting HTTP traffic.
>
>> The other functions hae only header names...
>
> The ACLs doing matching on header values are all regex I think.

The ACLs yes.
But we are already planning to handle regexs as special cases ...

>
>>
>>> I have seen a few people who have things like "quoted string" for auth
>>> realm, and since the old squid.conf sent the quotes out as-is they ended
>>> up with helpers and browsers security records using the quoted form.
>>> There might be any amount of unknown pain ahead if we suddenly start
>>> stripping quotes off strings in the backward compatible parse mode.
>> The realm uses the parse_eol which uses the
>> ConfigParser::NextQuotedOrToEol method which handles such cases....
>
> The point was that the "" are now being stripped whereas before they
> were not.

This is true if the realm start from "

>
> When the new parse mode is enabled that is fine. When the mode is
> disabled we are likely breaking things.
>
> If there is anything important which requires those quotes to be sent in
> the HTTP request then the change will break it. The obvious case is
> Digest auth where the realm is hashed into the nonces as-is and
> federated remote servers may store that hashed value for re-use. If
> Squid is no longer sending the same realm hash logins will fail in an
> almost undetectable way.
> There are people also doing similar things using the Basic auth realm in
> nasty ways so I would not be surprised if a quote changing there broken
> stuff for them as well.

OK.

>
> What I am hearing is that ConfigParser::NextQuotedOrToEol() is missing a
> check for ConfigParser::RecognizeQuotedValues to determine whether it
> treats quoted values as the "ToEol" case. Anything which is using it
> that used to have quoted-string support may need to preserve
> RecognizeQuotedValues, set it to true, run the function then reset it to
> the old value.

An opinion here.
Should the quoted-strings support enabled/disabled using the
configuration_includes_quoted_values on/off
configuration parameter, or do you believe that it should hardcoded in
realms parsing for example?
For example, should we use the following:
configuration_includes_quoted_values off
auth_param basic realm "Squid proxy-caching web server"
configuration_includes_quoted_values on

Or just implement a new ConfigParser::NextToEol() method for such cases?

>
>>
>>>
>>>>
>>>>>
>>>>>>> For the new style if someone want to use '(' character on a regex
>>>>>>> for
>>>>>>> example, he should use quotes:
>>>>>>> 'test(.*/)'
>>>>>>> "test(.*/)"
>>>>> Double quotes do not work well because REs use backslashes of their
>>>>> own.
>>>>> It becomes too messy as Amos noted above. I think we should not
>>>>> support
>>>>> REs in a new parser until we add Perl-like RE quoting (or better).
>>>> OK. This is something we can do it.
>>>> We can add a method ConfigParser::NextWord which will return a raw word
>>>> which will consist by any non whitespace character. Whitespaces can be
>>>> escaped.
>>>> Or use an On/Off global variable which enables/disables for the next
>>>> token this behaviour....
>>> Hmm. Might work.
>>>
>>> I have in mind to shuffle the ConfigParser methods into groups. One for
>>> the parser mechanics and one group for the type-specific GetX() methods.
>>> Some of those methods are provided in ConfigParser already, the rest are
>>> mixed up with src/Parsing.cc.
>>>
>>> A ConfigParser::RegexPattern() method should be perfectly fine to add.
>>>
>> This is looks good.
>
>
> Amos
>
Received on Wed Jul 31 2013 - 07:22:55 MDT