Re: [PATCH] Unknown cfg function

On 31/07/2013 7:22 p.m., Tsantilas Christos wrote:
> 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

I that would prevent the intentional update to quoted-values for that
option.

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

I think that making NextQuotedOrToEol() obey the RecognizeQuotedValues
flag will solve all these cases. The behaviour changing when it is
altered by the admin is not a big issue if we document it clearly which
ones are affected.
The problem here is more from changing the default behaviour silently,
than from what it is changed to.

Amos
Received on Thu Aug 01 2013 - 07:35:31 MDT