Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VFP and VDPs for partial responses #2554

Closed
wants to merge 2 commits into from

Conversation

Dridi
Copy link
Member

@Dridi Dridi commented Jan 31, 2018

Otherwise all bets are off :)

Some user agents like Safari may "probe" specific resources like medias
before getting the full resources usually asking for the first 2 or 11
bytes, probably to peek at magic numbers to figure early whether a
potentially large resource may not be supported (read: video).

If the user agent also advertises gzip support, and the transaction is
known beforehand to not be cacheable, varnishd will forward the Range
header to the backend:

    Accept-Encoding: gzip (when http_gzip_support is on)
    Range: bytes=0-1

If the response happens to be both encoded and partial, the gunzip test
cannot be performed. Otherwise we systematically end up with a broken
transaction closed prematuraly:

    FetchError b tGunzip failed
    Gzip b u F - 2 0 0 0 0

Refs varnishcache#2530
Otherwise we can't reliably parse the response body looking for ESI
tags. Instead a workaround is available in VCL and documented, and
the regression test exercises a similar workaround.

Bonus whitespace and RST cleanup in the ESI guide.

Fixes varnishcache#2530
@Dridi
Copy link
Member Author

Dridi commented Jan 31, 2018

I decided not to wait for the 4.1 patch to be ready to submit this one.

@fgsch
Copy link
Member

fgsch commented Jan 31, 2018

Why failing? Can we ignore it instead?
I think it's pretty common that people blindly sets do_esi.

@Dridi
Copy link
Member Author

Dridi commented Jan 31, 2018

Why failing? Can we ignore it instead?

Because we can't guarantee the ESI expansion on a partial backend response, so we need to either fail or retry a new bereq without the range.

I think it's pretty common that people blindly sets do_esi.

Then maybe it shouldn't be ported to 4.1 in this case (only the testGunzip fix).

@fgsch
Copy link
Member

fgsch commented Jan 31, 2018

Because we can't guarantee the ESI expansion on a partial backend response, so we need to either fail or retry a new bereq without the range.

We cannot guarantee it, ever.
do_esi is just an indicator - we might not find any ESI tags even for non-partial requests.

Since this can be workaround in VCL with a few lines I don't think we should go for this.
There are too many assumptions and potential fallout to make it worth it.

If anything I'd document it.

@Dridi
Copy link
Member Author

Dridi commented Jan 31, 2018

We cannot guarantee it, ever.
do_esi is just an indicator - we might not find any ESI tags even for non-partial requests.

This case is fine, if there are no ESI tags then so be it. It's different than missing ESI tags because they are out of range or truncated.

edit: a partial response could also fail the XML check enabled by default and therefore not expand the body.

There are too many assumptions and potential fallout to make it worth it.

The one case we briefly discussed is having a cacheable 206, but if it's cacheable the Range header is unset in bereq so we always cache the whole object. And if we were on the cacheable track and get an unattended partial response (backend misbehaving, covered by the test case) then beresp is marked as uncacheable and I don't think we can revert that decision.

@fgsch
Copy link
Member

fgsch commented Jan 31, 2018

Except you could insert the Range header in v_b_f{}.

I still fail to understand why we want to add this as C code. Why not VCL?

@Dridi
Copy link
Member Author

Dridi commented Jan 31, 2018

Except you could insert the Range header in v_b_f{}.

At this point you should know what you are doing.

I still fail to understand why we want to add this as C code. Why not VCL?

I wouldn't mind a VCL fix, so that would be a check in the built-in's v_b_r? But in this case you could still bypass the check.

@fgsch
Copy link
Member

fgsch commented Jan 31, 2018

At this point you should know what you are doing.

We could say the same about ESI since it's disabled by default.

Anyhow, if we add it in vcl_backend_response you could still bypass it, correct. (edited)

@Dridi
Copy link
Member Author

Dridi commented Jan 31, 2018

I think we have finally reached mutual understanding, I hope we'll get to decide on a solution for next bugwash.

@bsdphk
Copy link
Contributor

bsdphk commented Feb 7, 2018

The bigger picture here is that doing any VFP/VDP processing on range responses is iffy.

Presently we only have "our own" filters (gzip/gunzip/esi) but VMOD filters are in our future, and it seems to me that filters will have to be told that a response is partial at ->init() time, and decide for themselves what to do.

This mechanism needs to ensure that you cannot bypass mandatory filtering by sending a range for the entire object with Cookie or Authentication header.

(I'm closing #2530 so we only have one issue for this)

@bsdphk bsdphk changed the title Don't push VFPs for partial responses VFP and VDPs for partial responses Feb 7, 2018
@bsdphk
Copy link
Contributor

bsdphk commented Feb 12, 2018

Ok, having thought about this and sounded the bugwash, There are no credible uses of VDP/VFP on the response-body when we pass a range request to the backend, and we will reflect that.

Documentation warnings may be in order, re: for instance spying on esi-tag contents via range requests.

@Dridi
Copy link
Member Author

Dridi commented Feb 13, 2018

@bsdphk I failed to capture the final decision:

  • The first patch disables VFPs on partial responses, so far so good?
  • The second patch fails the fetch transaction for partial responses with do_esi == true (explicitly requested vs header-negotiated). Do we ultimately want this behavior? I think we vetoed the idea of a feature flag to control it.
  • The second patch has both a FetchError that could be considered documentation, and actual documentation. Is that enough, or should we have more bells and whistles in vcl(7)?
  • Do we also want to fail a do_g[un]zip == true on a partial backend response?
  • Should I update this pull request to neuter VDPs on partial responses as well?

@rezan
Copy link
Member

rezan commented Feb 13, 2018

I am a little bit nervous reading some of the language here myself. The solution is to disable VFP/VDPs that we know about and which break in this configuration, in particular, gzip and esi. I just want to make sure... but we cannot assume all processors need to be skipped when a partial is encountered. This is the exact challenge processors bring up, how do we know what any processor actually does and when they need to be used? In particular, if I do a range of 0-99999999, I should not be able to bypass security, encoding, and storage processors.

@Dridi
Copy link
Member Author

Dridi commented Feb 13, 2018

@rezan I wouldn't worry about that at this point, for the March release we aren't planning to officially open processors to VMODs so you should be fine. What we discussed during the bugwash was how to handle what we have today.

See above:

VMOD filters are in our future, and it seems to me that filters will have to be told that a response is partial at ->init() time, and decide for themselves what to do.

Dridi added a commit that referenced this pull request Feb 23, 2018
Some user agents like Safari may "probe" specific resources like medias
before getting the full resources usually asking for the first 2 or 11
bytes, probably to peek at magic numbers to figure early whether a
potentially large resource may not be supported (read: video).

If the user agent also advertises gzip support, and the transaction is
known beforehand to not be cacheable, varnishd will forward the Range
header to the backend:

    Accept-Encoding: gzip (when http_gzip_support is on)
    Range: bytes=0-1

If the response happens to be both encoded and partial, the gunzip test
cannot be performed. Otherwise we systematically end up with a broken
transaction closed prematuraly:

    FetchError b tGunzip failed
    Gzip b u F - 2 0 0 0 0

Refs #2530
Refs #2554
Dridi added a commit that referenced this pull request Feb 23, 2018
Some user agents like Safari may "probe" specific resources like medias
before getting the full resources usually asking for the first 2 or 11
bytes, probably to peek at magic numbers to figure early whether a
potentially large resource may not be supported (read: video).

If the user agent also advertises gzip support, and the transaction is
known beforehand to not be cacheable, varnishd will forward the Range
header to the backend:

    Accept-Encoding: gzip (when http_gzip_support is on)
    Range: bytes=0-1

If the response happens to be both encoded and partial, the gunzip test
cannot be performed. Otherwise we systematically end up with a broken
transaction closed prematuraly:

    FetchError b tGunzip failed
    Gzip b u F - 2 0 0 0 0

Refs #2530
Refs #2554
@Dridi
Copy link
Member Author

Dridi commented Feb 23, 2018

I fixed the testGunzip bug in trunk (5384df7) and 4.1 (94a7f42) branches. I'll be happy to revisit this pull request once we reach a decision for ESI and processors in general.

@hermunn
Copy link
Member

hermunn commented Apr 12, 2018

About back porting this: The fix by @Dridi is already in the 4.1 branch, and as far as I can see, 08bf3c8 is not necessary for backporting to 4.1. Yell if I am wrong.

dmatetelki pushed a commit to dmatetelki/varnish-cache that referenced this pull request Mar 14, 2019
Some user agents like Safari may "probe" specific resources like medias
before getting the full resources usually asking for the first 2 or 11
bytes, probably to peek at magic numbers to figure early whether a
potentially large resource may not be supported (read: video).

If the user agent also advertises gzip support, and the transaction is
known beforehand to not be cacheable, varnishd will forward the Range
header to the backend:

    Accept-Encoding: gzip (when http_gzip_support is on)
    Range: bytes=0-1

If the response happens to be both encoded and partial, the gunzip test
cannot be performed. Otherwise we systematically end up with a broken
transaction closed prematuraly:

    FetchError b tGunzip failed
    Gzip b u F - 2 0 0 0 0

Refs varnishcache#2530
Refs varnishcache#2554
dmatetelki pushed a commit to dmatetelki/varnish-cache that referenced this pull request Mar 14, 2019
…same.

Document workaround for ESI (also works for gzipery).

Fixes varnishcache#2554
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants