Re: [PATCH v2] BUG/MEDIUM: cache: Fix hash collision in `accept-encoding` handling for `Vary`

2020-12-31 Thread Willy Tarreau
Hi Tim,

On Tue, Dec 29, 2020 at 12:43:53PM +0100, Tim Duesterhus wrote:
> This patch fixes GitHub Issue #988. Commit 
> ce9e7b25217c46db1ac636b2c885a05bf91ae57e
> was not sufficient, because it fell back to a hash comparison if the bitmap
> of known encodings was not acceptable instead of directly returning the the
> cached response is not compatible.
> 
> This patch also extends the reg-test to test the hash collision that was
> mentioned in #988.
> 
> Vary handling is 2.4, no backport needed.

Many thanks, now applied!

Willy



[PATCH v2] BUG/MEDIUM: cache: Fix hash collision in `accept-encoding` handling for `Vary`

2020-12-29 Thread Tim Duesterhus
This patch fixes GitHub Issue #988. Commit 
ce9e7b25217c46db1ac636b2c885a05bf91ae57e
was not sufficient, because it fell back to a hash comparison if the bitmap
of known encodings was not acceptable instead of directly returning the the
cached response is not compatible.

This patch also extends the reg-test to test the hash collision that was
mentioned in #988.

Vary handling is 2.4, no backport needed.
---
 reg-tests/cache/vary.vtc | 56 +---
 reg-tests/cache/vary_accept_encoding.vtc | 32 ++
 src/cache.c  | 29 +++-
 3 files changed, 91 insertions(+), 26 deletions(-)

diff --git a/reg-tests/cache/vary.vtc b/reg-tests/cache/vary.vtc
index a840799f3..be79b6ad2 100644
--- a/reg-tests/cache/vary.vtc
+++ b/reg-tests/cache/vary.vtc
@@ -5,7 +5,8 @@ varnishtest "Vary support"
 feature ignore_unknown_macro
 
 server s1 {
-   # Response varying on "accept-encoding"
+   # Response varying on "accept-encoding" with
+   # an unacceptable content-encoding
rxreq
expect req.url == "/accept-encoding"
txresp -hdr "Content-Encoding: gzip" \
@@ -16,6 +17,15 @@ server s1 {
# Response varying on "accept-encoding"
rxreq
expect req.url == "/accept-encoding"
+   txresp -hdr "Content-Encoding: gzip" \
+   -hdr "Vary: accept-encoding" \
+   -hdr "Cache-Control: max-age=5" \
+   -bodylen 45
+
+   # Response varying on "accept-encoding" with
+   # no content-encoding
+   rxreq
+   expect req.url == "/accept-encoding"
txresp -hdr "Content-Type: text/plain" \
-hdr "Vary: accept-encoding" \
-hdr "Cache-Control: max-age=5" \
@@ -57,7 +67,7 @@ server s1 {
expect req.url == "/referer-accept-encoding"
txresp -hdr "Vary: referer,accept-encoding" \
-hdr "Cache-Control: max-age=5" \
-   -hdr "Content-Encoding: gzip" \
+   -hdr "Content-Encoding: br" \
-bodylen 54
 
rxreq
@@ -72,14 +82,14 @@ server s1 {
expect req.url == "/multiple_headers"
txresp -hdr "Vary: accept-encoding" \
   -hdr "Cache-Control: max-age=5" \
-   -hdr "Content-Encoding: gzip" \
+   -hdr "Content-Encoding: br" \
   -bodylen 155
 
rxreq
expect req.url == "/multiple_headers"
txresp -hdr "Vary: accept-encoding" \
   -hdr "Cache-Control: max-age=5" \
-   -hdr "Content-Encoding: gzip" \
+   -hdr "Content-Encoding: br" \
   -bodylen 166
 
 
@@ -163,6 +173,16 @@ client c1 -connect ${h1_fe_sock} {
expect resp.http.content-encoding == "gzip"
expect resp.bodylen == 45
 
+   # The response for the first request had an unacceptable 
`content-encoding`
+   # which might happen if that's the only thing the server supports, but
+   # we must not cache that and instead defer to the server.
+   txreq -url "/accept-encoding" -hdr "Accept-Encoding: first_value"
+   rxresp
+   expect resp.status == 200
+   expect resp.http.content-encoding == "gzip"
+   expect resp.bodylen == 45
+   expect resp.http.X-Cache-Hit == 0
+
txreq -url "/accept-encoding" -hdr "Accept-Encoding: second_value"
rxresp
expect resp.status == 200
@@ -170,11 +190,15 @@ client c1 -connect ${h1_fe_sock} {
expect resp.http.content-type == "text/plain"
expect resp.http.X-Cache-Hit == 0
 
+   # This request matches the cache entry for the request above, despite
+   # matching the `accept-encoding` of the first request because the
+   # request above only has the `identity` encoding which is implicitly
+   # added, unless explicitely forbidden.
txreq -url "/accept-encoding" -hdr "Accept-Encoding: first_value"
rxresp
expect resp.status == 200
-   expect resp.bodylen == 45
-   expect resp.http.content-encoding == "gzip"
+   expect resp.bodylen == 48
+   expect resp.http.content-type == "text/plain"
expect resp.http.X-Cache-Hit == 1
 
txreq -url "/accept-encoding" -hdr "Accept-Encoding: second_value"
@@ -227,7 +251,7 @@ client c1 -connect ${h1_fe_sock} {
 
# Mixed Vary (Accept-Encoding + Referer)
txreq -url "/referer-accept-encoding" \
-   -hdr "Accept-Encoding: first_value,second_value" \
+   -hdr "Accept-Encoding: br, gzip" \
-hdr "Referer: referer"
rxresp
expect resp.status == 200
@@ -235,7 +259,7 @@ client c1 -connect ${h1_fe_sock} {
expect resp.http.X-Cache-Hit == 0
 
txreq -url "/referer-accept-encoding" \
-   -hdr "Accept-Encoding: first_value" \
+   -hdr "Accept-Encoding: br" \
-hdr "Referer: other-referer"
rxresp
expect resp.status == 200
@@ -243,7 +267,7 @@ client c1