-
Notifications
You must be signed in to change notification settings - Fork 380
VIP8: No pipe in builtin.vcl in V5
No matter how VIP6: What does pipe mean in Varnish5? turns out, pipe will not work as it used to in V5, and we need to decide what to do about builtin::vcl_recv{}
The return(pipe) clause in builtin.vcl dates back to V1 where a lot of corner cases of HTTP were not implemented, but we have added those over time, and I belive today pass can handle all relevant HTTP traffic.
A couple of odd-ball requests remain, CONNECT, OPTIONS and TRACE, and pipe might still be a relevant handling in some settings, but in most cases they will be evidence of ill intent on the part of the client.
Here is my suggestion:
sub vcl_recv {
if (req.method == "PRI" || /* HTTP/2.0 */
req.method == "CONNECT" ||
req.method == "OPTIONS" ||
req.method == "TRACE") {
return (synth(405));
}
if (req.method != "GET" && req.method != "HEAD") {
/* We only deal with GET and HEAD by default */
return (pass);
}
if (req.http.Authorization || req.http.Cookie) {
/* Not cacheable by default */
return (pass);
}
return (hash);
}
-
@fgsch: For CORS, you need the OPTIONS method for preflight requests.
-
@phk: So you want to pass OPTIONS also ?
-
@fgsch: I don't know how common this is tbh. Perhaps it's just a matter of documenting it? OTOH, PROPFIND and other more obscure methods will be allowed so I'm more inclined to allow this by default. Is the not allowed list meant to catch up all the methods that would otherwise require pipe'ing?
On July 4th, this has been discussed again, with a slight overlap over VIP6: What does pipe mean in Varnish5?:
13:39 < fgs> btw, someone asked today on #varnish why we don't handle PATCH on the pipe case in the builtin VCL. Anyone has any idea? I think it should be on that list.
13:39 < slink> PATCH?
13:39 < phk> fgs, never came up before.
13:39 < dridi_> slink: it's part of webdav methods
13:39 < fgs> ok, so the list is non exhaustive
13:39 < slink> ah. ok
13:40 < phk> why would it need pipe ?
13:40 < fgs> it should not
13:40 < scn> it is not not-pipe list, isn't it?
13:40 < fgs> that's the issue :)
13:40 < scn> uh. is the not-pipe list.
13:40 < phk> so why does it need pipe ?
13:40 < fgs> it doesn't but it's currently handled as pipe
13:40 < scn> it is piped now, it should perhaps be passed instead
13:40 < fgs> the list if is req.method != .. && .. return pipe
13:41 < scn> (are there more "recent" methods we've forgotten about?)
13:41 < fgs> PATCH is not explicitly mentioned so is piped
13:41 < phk> actually, shouldn't we turn that list around, and only pipe things we know need it.
13:41 < phk> (ie: OPTIONS ?)
13:41 < dridi_> that's because we handle pipe backwards, we should only pipe when there's a 101 switching protocols, after a beresp, not a req
13:42 < phk> dridi_, relevant point there.
13:42 < scn> phk: because future proofness wrt protocol we don't know about yet.
13:42 < fgs> Ok, since there seems to be agreement I will add it
13:42 < phk> How about "if (req.method == "OPTIONS" || req.method == "CONNECT" || req.http.upgrade) { return (pipe); }
13:43 < phk> scn, future protocols should be passable
13:43 < fgs> why do we need to pipe OPTIONS?
13:43 < scn> quick google search: https://annevankesteren.nl/2007/10/http-methods
13:43 < dridi_> you can remove OPTIONS
13:43 < phk> fgs, because we don't respond to it.
13:43 < phk> actuall: I suggest we remove the entire pipe clause.
13:44 < phk> a normal website should not need any of that, and we just send attack traffic to the backend by having it there.
13:44 < phk> if people need pipe, they can add it.
13:44 < fgs> how is different than pass wrt to being an attack vector?
13:45 < phk> fgs, you get direct access to the backend without any VCL filtering
13:45 < fgs> you still have some control on the first request
13:45 < fgs> but fair enough
13:46 < fgs> should we sleep on it a bit before killing it?
13:46 < phk> yes. ponder it.