[squid-dev] Fix If-None-Match processing and related bug 4169

[Alex Rousskov]

On 11/29/2016 02:23 PM, Amos Jeffries wrote:
> On 30/11/2016 1:47 a.m., Garri Djavadyan wrote:
>> On Tue, 2016-11-29 at 14:51 +0500, Garri Djavadyan wrote:
>>> Hello,
>>>
>>> Please review the attached patch prepared for r14958, it fixes the
>>> If-
>>> None-Match processing (incorrect logging [1]) and the bug [2] report
>>> 4169 depending on the incorrect (IMO) behavior.
>>>
>>> An If-None-Match request for a non-matched (ETag) but cached object
>>> should be processed as normal HIT.
>>>
>>> [1] http://lists.squid-cache.org/pipermail/squid-users/2016-November/013483.html
>>> [2] http://bugs.squid-cache.org/show_bug.cgi?id=4169

>> In the first version I
>> removed strings (not related to the fix) which mark a request as non-
>> IMS, because I thought the same request could not
>> enter processConditional twice and the removed actions are redundant.

I agree that the second version is better: Unless you are absolutely
sure, it is best to stay focused on the problem and leave the IMS
deletion code "as is" even if it may not be necessary. If you still
suspect it is not necessary, add a TODO comment to investigate separately.


> Checking this whole processConditional() sequence for compliance with
> RFC 7232 and 7234 the rest of the logic seems to be fine except this case:
> 
> <https://tools.ietf.org/html/rfc7234#section-4.3.2>
> "
>    If the field-value is "*", or if the
>    field-value is a list of entity-tags and at least one of them matches
>    the entity-tag of the selected stored response,

> [ e->hasIfNoneMatchEtag(r) == false --> !N == true ]

I am not sure I am interpreting your [...] mnemonic correctly, but
AFAICT, the patched code works when the RFC conditions quoted above are
_not_ meant:

>     if (r.header.has(Http::HdrType::IF_NONE_MATCH)) {
>         if (!e->hasIfNoneMatchEtag(r)) {
>             // RFC 2616: ignore IMS if If-None-Match did not match
>             r.flags.ims = false;
>             r.ims = -1;
>             r.imslen = 0;
>             r.header.delById(Http::HdrType::IF_MODIFIED_SINCE);
>             http->logType = LOG_TCP_MISS;
>             sendMoreData(result);
>             return true;
>         }


In other words, hasIfNoneMatchEtag() returns false and, hence, the
quoted code is executed when _none_ of the requested ETags matches the
cached ETag. The "if If-None-Match did not match" comment inside that
code agrees with this interpretation.

Thus, the recommended RFC action that you found does _not_ apply to the
fixed code:

>    a cache recipient
>    SHOULD generate a 304 (Not Modified) response (using the metadata of
>    the selected stored response) instead of sending that stored
>    response.
> "


If you were talking about processConditional() code outside the if
statement fixed by Garri (i.e., when hasIfNoneMatchEtag() returns true),
then it appears to match the quoted requirement only if there is no IMS
header:

> if (!r.flags.ims) {
>     // RFC 2616: if If-None-Match matched and there is no IMS,
>     // reply with 304 Not Modified or 412 Precondition Failed
>     sendNotModifiedOrPreconditionFailedError();
>     return true;
> }
> 
> // otherwise check IMS below to decide if we reply with 304 or 412
> matchedIfNoneMatch = true;


If there is an IMS header, then Squid violates the following RFC 7232 MUST:

> A recipient MUST ignore If-Modified-Since if the request contains an
> If-None-Match header field

The HTTP requirements changed here since RFC 2616, and we probably
should adjust the code to delete IMS header/flags after discovering the
If-None-Match header:

if (r.header.has(Http::HdrType::IF_NONE_MATCH)) {
    // RFC 7232: If-None-Match recipient MUST ignore IMS
    r.flags.ims = false;
    r.ims = -1;
    r.imslen = 0;
    r.header.delById(Http::HdrType::IF_MODIFIED_SINCE);

    if (e->hasIfNoneMatchEtag(r)) {
        sendNotModifiedOrPreconditionFailedError();
        return true;
    }

    // If-None-Match did not match; treat as an unconditional hit
    return false;
}

If the above sketch is correct, then the matchedIfNoneMatch variable may
become unnecessary.


HTH,

Alex.