BBlack has submitted this change and it was merged. Change subject: V4 XFF Fixup 3/3 ......................................................................
V4 XFF Fixup 3/3 This moves varnish3 set_xff above fe_ip_processing, so that v3 and v4 cases are identical on entry (XFF has already been created or appended-to), and then re-factors the logic within to handle the fact that XFF has already been locally appended-to. Change-Id: I1572c9d30b54ba7315b90a794f1a825c07078b77 --- M modules/varnish/templates/vcl/wikimedia-frontend.vcl.erb 1 file changed, 54 insertions(+), 58 deletions(-) Approvals: BBlack: Verified; Looks good to me, approved diff --git a/modules/varnish/templates/vcl/wikimedia-frontend.vcl.erb b/modules/varnish/templates/vcl/wikimedia-frontend.vcl.erb index 33562eb..b147e53 100644 --- a/modules/varnish/templates/vcl/wikimedia-frontend.vcl.erb +++ b/modules/varnish/templates/vcl/wikimedia-frontend.vcl.erb @@ -109,71 +109,68 @@ unset req.http.X-Connection-Properties; } - if (req.http.X-Forwarded-For) { - // To make further parsing/sanitizing simpler, convert all whitespace - // in XFF to single spaces, and make sure all commas have a space - // suffix but no space prefix. - set req.http.X-Forwarded-For = regsuball(req.http.X-Forwarded-For, "[ \t]+", " "); - set req.http.X-Forwarded-For = regsuball(req.http.X-Forwarded-For, " ?, ?", ", "); + // To make further parsing/sanitizing simpler, convert all whitespace + // in XFF to single spaces, and make sure all commas have a space + // suffix but no space prefix. + set req.http.X-Forwarded-For = regsuball(req.http.X-Forwarded-For, "[ \t]+", " "); + set req.http.X-Forwarded-For = regsuball(req.http.X-Forwarded-For, " ?, ?", ", "); - // Now fully-sanitize it to only the strict form "X(, X)*", where X is - // a string of legal characters in IPv[46] addresses. Note - // that injections can still leave well-formed junk on the - // left, but it's up to the trusted proxy code to ignore that, - // e.g.: - // "junk2, 123.123.123.123" -> "2, 123.123.123.123" - set req.http.X-Forwarded-For = regsub(req.http.X-Forwarded-For, - "^.*?([0-9A-Fa-f:.]+(, [0-9A-Fa-f:.]+)*)? ?$", "\1"); - - // Clear header if empty after all the above, to avoid messing - // up our normal XFF-append code later - if (req.http.X-Forwarded-For == "") { - unset req.http.X-Forwarded-For; - } - } + // Now fully-sanitize it to only the strict form "X(, X)*", where X is + // a string of legal characters in IPv[46] addresses. Note + // that injections can still leave well-formed junk on the + // left, but it's up to the trusted proxy code to ignore that, + // e.g.: + // "junk2, 123.123.123.123" -> "2, 123.123.123.123" + set req.http.X-Forwarded-For = regsub(req.http.X-Forwarded-For, + "^.*?([0-9A-Fa-f:.]+(, [0-9A-Fa-f:.]+)*)? ?$", "\1"); // There are two possible cases here: either nginx acted as our TLS // proxy and already set X-Client-IP (as well as appended the same value - // to XFF), or the traffic was direct to varnish-fe, in which case - // XCIP is not yet set and XFF is directly from external. + // to XFF, and we appended nginx's IP to XFF already as well...), or the + // traffic was direct to varnish-fe, in which case XCIP is not yet set + // and XFF is external + our addition of client.ip. + if (!req.http.X-Client-IP) { - // direct-to-port-80 case, set XCIP ourselves set req.http.X-Client-IP = client.ip; - set req.http.X-Trusted-Proxy = netmapper.map("proxies", req.http.X-Client-IP); - // normalize to boolean post-netmapper (varnish-3.0.4...) - if (req.http.X-Trusted-Proxy == "") { - unset req.http.X-Trusted-Proxy; - } - if (req.http.X-Trusted-Proxy && req.http.X-Forwarded-For) { - // get last from trusted-proxy-supplied XFF - set req.http.maybe-xcip = regsub(req.http.X-Forwarded-For, "^([^,]+, )+", ""); - if(<%= @std_ipcast %>.ip(req.http.maybe-xcip, "127.0.0.1") !~ wikimedia_nets) { - set req.http.X-Client-IP = req.http.maybe-xcip; - } - unset req.http.maybe-xcip; - } + unset req.http.via-nginx; } else { - // XCIP from nginx, XFF set/appended by nginx and contains at - // least XCIP at the end, possibly prepended by other addrs - // set externally by some proxy. - set req.http.X-Trusted-Proxy = netmapper.map("proxies", req.http.X-Client-IP); - // normalize to boolean post-netmapper (varnish-3.0.4...) - if (req.http.X-Trusted-Proxy == "") { - unset req.http.X-Trusted-Proxy; + set req.http.via-nginx = 1; + } + + set req.http.X-Trusted-Proxy = netmapper.map("proxies", req.http.X-Client-IP); + // normalize to boolean post-netmapper (varnish-3.0.4...) + if (req.http.X-Trusted-Proxy == "") { + unset req.http.X-Trusted-Proxy; + } + + if (req.http.X-Trusted-Proxy) { + // We need the right-most entry of XFF that was set externally + // to our software stack. If this was direct-to-varnish, that + // means the 2nd entry in from the right (varnish already + // appended client.ip). If this was nginx->varnish, we want + // the 3rd entry in from the right (nginx appended a client IP, + // then varnish appended nginx's IP). + + // set maybe-xcip to XFF with final entry stripped (added by this varnish) + set req.http.maybe-xcip = regsub(req.http.X-Forwarded-For, ", [^,]+$", ""); + + // repeat final-strip if this was via-nginx to remove the one added by nginx + if (req.http.via-nginx) { + unset req.http.via-nginx; + set req.http.maybe-xcip = regsub(req.http.maybe-xcip, ", [^,]+$", ""); } - if (req.http.X-Trusted-Proxy) { - // We want the second-to-last XFF entry here, assuming - // there's two or more IPs. Note that with the - // regsub's below if there was only one (which would - // alias XCIP by definition), there would be no commas - // to match and XCIP gets reset to its original value. - set req.http.maybe-xcip = regsub(req.http.X-Forwarded-For, ", [^,]+$", ""); - set req.http.maybe-xcip = regsub(req.http.maybe-xcip, "^([^,]+, )+", ""); - if(<%= @std_ipcast %>.ip(req.http.maybe-xcip, "127.0.0.1") !~ wikimedia_nets) { - set req.http.X-Client-IP = req.http.maybe-xcip; - } - unset req.http.maybe-xcip; + + // set maybe-xcip to the right-most entry left in itself + set req.http.maybe-xcip = regsub(req.http.maybe-xcip, "^([^,]+, )+", ""); + + // if it's an outside IP, use it to set XCIP (otherwise leave + // XCIP alone and it's still the proxy's IP) + if(<%= @std_ipcast %>.ip(req.http.maybe-xcip, "127.0.0.1") !~ wikimedia_nets) { + set req.http.X-Client-IP = req.http.maybe-xcip; } + + // cleanup + unset req.http.maybe-xcip; } // Now check carrier database for setting X-Carrier based on XCIP @@ -238,11 +235,10 @@ call normalize_request; // IP processing is req->req mangling that shouldn't be re-done on restart - call recv_fe_ip_processing; - <% if not @varnish_version4 -%> call wm_common_recv_set_xff; <% end -%> + call recv_fe_ip_processing; } call wm_common_recv_early; -- To view, visit https://gerrit.wikimedia.org/r/289258 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I1572c9d30b54ba7315b90a794f1a825c07078b77 Gerrit-PatchSet: 2 Gerrit-Project: operations/puppet Gerrit-Branch: production Gerrit-Owner: BBlack <bbl...@wikimedia.org> Gerrit-Reviewer: BBlack <bbl...@wikimedia.org> Gerrit-Reviewer: Ema <e...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits