Mark Bergsma has submitted this change and it was merged.
Change subject: Fix XFF handling on all Varnish clusters
......................................................................
Fix XFF handling on all Varnish clusters
X-Forwarded-For handling is a total mess.
Change-Id: Ica2bc6b7c2ed8844e1442e48ee92df13541a952f
---
M manifests/role/cache.pp
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
7 files changed, 43 insertions(+), 46 deletions(-)
Approvals:
Mark Bergsma: Looks good to me, approved
jenkins-bot: Verified
diff --git a/manifests/role/cache.pp b/manifests/role/cache.pp
index d32ef0f..153115b 100644
--- a/manifests/role/cache.pp
+++ b/manifests/role/cache.pp
@@ -566,6 +566,7 @@
'purge_regex' =>
'^http://(?!upload\.wikimedia\.org)',
'cluster_tier' => $cluster_tier,
'layer' => 'backend',
+ 'ssl_proxies' => $wikimedia_networks,
},
backend_options => [
{
@@ -586,7 +587,6 @@
'weight' => $backend_weight,
}],
wikimedia_networks => $wikimedia_networks,
- xff_sources => $wikimedia_networks,
}
varnish::instance { "text-frontend":
@@ -605,6 +605,7 @@
'purge_regex' =>
'^http://(?!upload\.wikimedia\.org)',
'cluster_tier' => $cluster_tier,
'layer' => 'frontend',
+ 'ssl_proxies' => $wikimedia_networks,
},
backend_options => [
{
@@ -616,7 +617,6 @@
'probe' => "varnish",
'weight' => $backend_weight,
}],
- xff_sources => $wikimedia_networks,
}
include role::cache::varnish::logging
@@ -716,6 +716,7 @@
'purge_regex' =>
'^http://upload\.wikimedia\.org/',
'cluster_tier' => $cluster_tier,
'layer' => 'backend',
+ 'ssl_proxies' => $wikimedia_networks,
},
backend_options => [
{
@@ -733,7 +734,6 @@
}],
cluster_options => $cluster_options,
wikimedia_networks => $wikimedia_networks,
- xff_sources => $wikimedia_networks,
}
varnish::instance { "upload-frontend":
@@ -750,6 +750,7 @@
'purge_regex' =>
'^http://upload\.wikimedia\.org/',
'cluster_tier' => $cluster_tier,
'layer' => 'frontend',
+ 'ssl_proxies' => $wikimedia_networks,
},
backend_options => [
{
@@ -762,7 +763,6 @@
'weight' => $backend_weight,
}],
cluster_options => $cluster_options,
- xff_sources => $wikimedia_networks,
}
include role::cache::varnish::logging
@@ -885,6 +885,7 @@
'retry5xx' => 1,
'cache4xx' => "1m",
'layer' => 'frontend',
+ 'ssl_proxies' => $wikimedia_networks,
},
backend_options => {
'port' => 80,
@@ -895,7 +896,6 @@
'probe' => $probe,
},
cluster_options => $cluster_options,
- xff_sources => $wikimedia_networks,
}
include role::cache::varnish::logging::eventlistener
@@ -965,6 +965,7 @@
'purge_regex' =>
'^http://(?!upload\.wikimedia\.org)',
'cluster_tier' => $cluster_tier,
'layer' => 'backend',
+ 'ssl_proxies' => $wikimedia_networks,
},
backend_options => [
{
@@ -985,7 +986,6 @@
'max_connections' => 600,
}],
wikimedia_networks => $wikimedia_networks,
- xff_sources => $wikimedia_networks,
}
varnish::instance { "mobile-frontend":
@@ -1007,6 +1007,7 @@
'purge_regex' =>
'^http://(?!upload\.wikimedia\.org)',
'cluster_tier' => $cluster_tier,
'layer' => 'frontend',
+ 'ssl_proxies' => $wikimedia_networks,
},
backend_options => [
{
@@ -1018,7 +1019,6 @@
'max_connections' => 100000,
'probe' => "varnish",
}],
- xff_sources => $wikimedia_networks,
}
include role::cache::varnish::logging
@@ -1067,6 +1067,7 @@
},
vcl_config => {
'retry5xx' => 1,
+ 'ssl_proxies' => $wikimedia_networks,
},
backend_options => [
{
@@ -1076,7 +1077,6 @@
'between_bytes_timeout' => "20s",
'max_connections' => 600,
}],
- xff_sources => $wikimedia_networks,
}
varnish::instance { "parsoid-frontend":
@@ -1094,6 +1094,7 @@
},
vcl_config => {
'retry5xx' => 0,
+ 'ssl_proxies' => $wikimedia_networks,
},
backend_options => {
'port' => 3128,
@@ -1104,7 +1105,6 @@
'max_connections' => 100000,
'probe' => "varnish",
},
- xff_sources => $wikimedia_networks,
}
}
}
diff --git a/modules/varnish/templates/vcl/wikimedia.vcl.erb
b/modules/varnish/templates/vcl/wikimedia.vcl.erb
index 9d4319b..64afa0e 100644
--- a/modules/varnish/templates/vcl/wikimedia.vcl.erb
+++ b/modules/varnish/templates/vcl/wikimedia.vcl.erb
@@ -41,10 +41,9 @@
}
<% end -%>
-# Hosts we trust for XFF
-<% if ! xff_sources.empty? -%>
-acl allow_xff {
-<% xff_sources.each do |entry|
+<% if ! @vcl_config.fetch("ssl_proxies", []).empty? -%>
+acl ssl_proxies {
+<% @vcl_config.fetch("ssl_proxies", []).each do |entry|
subnet, mask = entry.split("/", 2)
-%>
"<%= subnet %>"/<%= mask %>;
@@ -222,18 +221,13 @@
set req.grace = 60m;
}
-<% 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;
+<% if vcl_config.fetch("layer", "frontend") == "frontend" -%>
+ /* Ensure we only accept Forwarded-Proto headers from the SSL proxies */
+ if (client.ip !~ ssl_proxies) {
unset req.http.X-Forwarded-Proto;
}
<% end -%>
+ call vcl_recv_append_xff;
if ( req.http.host ~ "^varnishcheck" ) {
error 200 "OK";
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 fe8beb1..103ffad 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..2cc6b00 100644
--- a/templates/varnish/zero.inc.vcl.erb
+++ b/templates/varnish/zero.inc.vcl.erb
@@ -452,30 +452,37 @@
/* Please keep the above list alphabetized by the ACL variable */
}
+sub replace_clientip {
+ C{
+ struct sockaddr_in *client_ip_si = (struct sockaddr_in
*)VRT_r_client_ip(sp);
+ char *xff, *last_comma;
+
+ xff = VRT_GetHdr(sp, HDR_REQ, "\017X-Stripped-XFF:");
+ if (xff) {
+ last_comma = strrchr(xff, ',');
+ inet_pton(AF_INET, (last_comma ? last_comma+1 : xff),
&(client_ip_si->sin_addr));
+ }
+ }C
+}
+
sub spoof_clientip {
- /* In case the XFF header is present, trust it in case it comes from
- * Opera Mini's accelerating proxies, or from a carrier_testing ip
range,
- * and replace client ip value with the first value from the XFF Header
+ /* FIXME: remove this awful hack
+ *
+ * We need to do this because Varnish can only match client.ip and
server.ip against an ACL
+ * Replace client.ip with the last IP in the XFF header, or the
penultimate IP in case
+ * of Opera Mini in front of an SSL proxy.
*/
- if (req.http.X-Forwarded-For && (client.ip ~ opera_mini || client.ip ~
carrier_testing)) {
+ if (req.restarts == 0
+ && (client.ip ~ ssl_proxies || 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;
-
- 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:");
+ set req.http.X-Stripped-XFF = regsub(req.http.X-Forwarded-For,
",[^,]+$", "");
+ if (req.http.X-Forwarded-Proto == "https" && client.ip ~
ssl_proxies) {
+ call replace_clientip;
+ set req.http.X-Stripped-XFF =
regsub(req.http.X-Stripped-XFF, ",[^,]+$", "");
}
-
- if (xff_ip != NULL) {
- inet_pton(AF_INET, xff_ip, client_ip_ia);
+ if (req.http.X-Stripped-XFF && (client.ip ~ opera_mini ||
client.ip ~ carrier_testing)) {
+ call replace_clientip;
}
- }C
-
- unset req.http.X-Forwarded-For;
+ unset req.http.X-Stripped-XFF;
}
}
--
To view, visit https://gerrit.wikimedia.org/r/75860
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ica2bc6b7c2ed8844e1442e48ee92df13541a952f
Gerrit-PatchSet: 18
Gerrit-Project: operations/puppet
Gerrit-Branch: production
Gerrit-Owner: Mark Bergsma <[email protected]>
Gerrit-Reviewer: Mark Bergsma <[email protected]>
Gerrit-Reviewer: MaxSem <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
