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

[Garri Djavadyan]

On Tue, 2016-11-29 at 15:42 -0700, Alex Rousskov wrote:
> 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-Nov
> > > > ember/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.

Alex, Amos thanks for comments! I've attached third version of the
patch. It is based on the proposal by Alex. The patch introduces
following changes:

* Matched If-None-Match requests are logged as TCP_INM_HIT (proposed by
Amos)
* If-Modified-Since requests for modified cached objects are processed
as normal HITs
* Not matched If-None-Match requests for cached objects are processed
as normal HITs
* If-Modified-Since header is ignored if If-None-Match header exists
(RFC7232 compliance) 

I've tested different conditional requests (including 'If-None-Match:
*' and 'If-None-Match: "matched-tag", "not-matched-tag"') and found no
problems. Thanks.

Garri
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix_if-none-match_v3.patch
Type: text/x-patch
Size: 3789 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20161130/747828d1/attachment.bin>