Mark Bergsma has uploaded a new change for review.
https://gerrit.wikimedia.org/r/75860
Change subject: Fix XFF handling on all Varnish clusters
......................................................................
Fix XFF handling on all Varnish clusters
X-Forwarded-For handling was a total mess.
Because both the GeoIP code and the Zero "spoof ip" code only look
at the first IP in the XFF header, we now always make sure the XFF
header is either cleared at the start of the request (in case of a
direct client connection), or only contains the last field (if
the client is NGINX, an Opera Mini proxy or a carrier testing IP).
Change-Id: Ica2bc6b7c2ed8844e1442e48ee92df13541a952f
---
M modules/varnish/templates/vcl/wikimedia.vcl.erb
M templates/varnish/mobile-backend.inc.vcl.erb
M templates/varnish/mobile-frontend.inc.vcl.erb
M templates/varnish/text-backend.inc.vcl.erb
M templates/varnish/text-frontend.inc.vcl.erb
M templates/varnish/zero.inc.vcl.erb
6 files changed, 25 insertions(+), 27 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/operations/puppet
refs/changes/60/75860/1
diff --git a/modules/varnish/templates/vcl/wikimedia.vcl.erb
b/modules/varnish/templates/vcl/wikimedia.vcl.erb
index 9d4319b..383844e 100644
--- a/modules/varnish/templates/vcl/wikimedia.vcl.erb
+++ b/modules/varnish/templates/vcl/wikimedia.vcl.erb
@@ -224,14 +224,19 @@
<% if has_variable?("xff_sources") and xff_sources.length > 0 -%>
/* Ensure we only accept Forwarded headers from the SSL proxies */
- if (client.ip ~ allow_xff || client.ip ~ opera_mini) {
- // Do nothing. It seems you can't do !~ with IP matches
- } else {
- // Strip the headers, we shouldn't trust these from anything
other
- // than hosts we specify. Needed for the geoiplookup code later
on
- // as it will use xff. MediaWiki uses xfp.
- set req.http.X-Forwarded-For = client.ip;
- unset req.http.X-Forwarded-Proto;
+ if (req.restarts == 0) {
+ if (client.ip ~ allow_xff || client.ip ~ opera_mini) {
+ /* As nginx doesn't filter, keep only the last IP */
+ set req.http.X-Forwarded-For =
regsub(req.http.X-Forwarded-For, "^.*([0-9a-f.:]+)$", "\1");
+ } else {
+ /* Strip the headers, we shouldn't trust these from
anything other
+ * than hosts we specify. Needed for the geoiplookup
code later on
+ * as it will use XFF. MediaWiki uses XFP.
+ */
+ unset req.http.X-Forwarded-For;
+ unset req.http.X-Forwarded-Proto;
+ }
+ call vcl_recv_append_xff;
}
<% end -%>
diff --git a/templates/varnish/mobile-backend.inc.vcl.erb
b/templates/varnish/mobile-backend.inc.vcl.erb
index 089de00..7c56953 100644
--- a/templates/varnish/mobile-backend.inc.vcl.erb
+++ b/templates/varnish/mobile-backend.inc.vcl.erb
@@ -19,7 +19,6 @@
<% end -%>
/* Default (now modified) Varnish vcl_recv function */
- call vcl_recv_append_xff;
if (req.request != "GET" && req.request != "HEAD") {
/* We only deal with GET and HEAD by default */
return (pass);
diff --git a/templates/varnish/mobile-frontend.inc.vcl.erb
b/templates/varnish/mobile-frontend.inc.vcl.erb
index d418201..ead79b8 100644
--- a/templates/varnish/mobile-frontend.inc.vcl.erb
+++ b/templates/varnish/mobile-frontend.inc.vcl.erb
@@ -88,7 +88,6 @@
set req.hash_ignore_busy = true;
/* Default (now modified) Varnish vcl_recv function */
- call vcl_recv_append_xff;
if (req.request != "GET" && req.request != "HEAD") {
/* We only deal with GET and HEAD by default */
return (pass);
diff --git a/templates/varnish/text-backend.inc.vcl.erb
b/templates/varnish/text-backend.inc.vcl.erb
index e059458..304d95a 100644
--- a/templates/varnish/text-backend.inc.vcl.erb
+++ b/templates/varnish/text-backend.inc.vcl.erb
@@ -4,7 +4,6 @@
include "text-common.inc.vcl";
sub vcl_recv {
- call vcl_recv_append_xff;
call vcl_recv_purge;
call restrict_access;
diff --git a/templates/varnish/text-frontend.inc.vcl.erb
b/templates/varnish/text-frontend.inc.vcl.erb
index def9600..b9ac21e 100644
--- a/templates/varnish/text-frontend.inc.vcl.erb
+++ b/templates/varnish/text-frontend.inc.vcl.erb
@@ -30,7 +30,6 @@
sub vcl_recv {
call filter_headers;
- call vcl_recv_append_xff;
/* Allow purging */
call vcl_recv_purge;
diff --git a/templates/varnish/zero.inc.vcl.erb
b/templates/varnish/zero.inc.vcl.erb
index 5792d2a..600bbc3 100644
--- a/templates/varnish/zero.inc.vcl.erb
+++ b/templates/varnish/zero.inc.vcl.erb
@@ -460,22 +460,19 @@
if (req.http.X-Forwarded-For && (client.ip ~ opera_mini || client.ip ~
carrier_testing)) {
set req.http.X-Orig-Client-IP = client.ip;
C{
- struct sockaddr_storage *client_ip_ss = VRT_r_client_ip(sp);
- struct sockaddr_in *client_ip_si = (struct sockaddr_in *)
client_ip_ss;
- struct in_addr *client_ip_ia = &(client_ip_si->sin_addr);
- char *last;
+ struct sockaddr_storage *client_ip_ss =
VRT_r_client_ip(sp);
+ struct sockaddr_in *client_ip_si = (struct sockaddr_in
*) client_ip_ss;
+ struct in_addr *client_ip_ia =
&(client_ip_si->sin_addr);
+ char *xff, *xff_ip, *last;
- char *xff_ip = strtok(VRT_GetHdr(sp, HDR_REQ,
"\020X-Forwarded-For:"), ",");
-
- if (xff_ip == NULL) {
- xff_ip = VRT_GetHdr(sp, HDR_REQ,
"\020X-Forwarded-For:");
- }
-
- if (xff_ip != NULL) {
- inet_pton(AF_INET, xff_ip, client_ip_ia);
- }
+ xff = VRT_GetHdr(sp, HDR_REQ, "\020X-Forwarded-For:");
+ xff_ip = strtok_r(xff, ",", &last);
+ if (xff_ip == NULL) {
+ inet_pton(AF_INET, xff, client_ip_ia);
+ } else {
+ inet_pton(AF_INET, xff_ip, client_ip_ia);
+ while (strtok_r(NULL, ",", &last) != NULL);
+ }
}C
-
- unset req.http.X-Forwarded-For;
}
}
--
To view, visit https://gerrit.wikimedia.org/r/75860
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ica2bc6b7c2ed8844e1442e48ee92df13541a952f
Gerrit-PatchSet: 1
Gerrit-Project: operations/puppet
Gerrit-Branch: production
Gerrit-Owner: Mark Bergsma <m...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
