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

Delay browsers not sending referer headers #79

Conversation

@zerebubuth
Copy link
Contributor

@zerebubuth zerebubuth commented Aug 11, 2016

Add more delay pools of smaller size to use for browsers which don't send referer headers.

Also re-wrote the many, many lines of config to use ERB. I found this to be necessary for understanding... but it's possible it was done the other way for a very good reason. @Firefishy - should I expand out the ERB explicitly?

Follows on from #78 as a way of defending against malicious use of the no-referer setting.

I don't know Squid that well, so I'd really appreciate help and reviews of this. Hopefully we can avoid a repeat of the over-blocking yesterday.

@Zverik
Copy link

@Zverik Zverik commented Aug 11, 2016

There is a suspicious comment "This doesn't seem to work! Revisit later!" — maybe it should be tested first on a development server.

@zerebubuth
Copy link
Contributor Author

@zerebubuth zerebubuth commented Aug 11, 2016

That was the old config line which I thought was responsible for blocking everything yesterday. As daniel_hozac mentioned on IRC yesterday, it's more likely to have been the unescaped space in a previous rule which is currently also commented out.

Testing would be great! Could you help with that, please?

@Zverik
Copy link

@Zverik Zverik commented Aug 11, 2016

Alas I can't: I don't have Squid on any of my servers.

The theory about unescaped spaces seems plausible: a similar line above that has spaces replaced with .*.

@Firefishy
Copy link
Member

@Firefishy Firefishy commented Aug 11, 2016

Not near a real connection to test properly, but looks reasonable to me.

@gravitystorm
Copy link
Contributor

@gravitystorm gravitystorm commented Aug 11, 2016

I've been working on getting the cookbooks running with test-kitchen, so that everyone is able to test them locally using virtual machines. But it's still a work in progress (see #84 among others)

@pnorman
Copy link
Contributor

@pnorman pnorman commented Aug 11, 2016

Is it worth me sending out a reminder to dev@ that applications/sites need to either be sending a user-agent identifying the application or a referer?

@zerebubuth
Copy link
Contributor Author

@zerebubuth zerebubuth commented Aug 12, 2016

@pnorman, yes, please! Ideally, non-browser apps would send both. There's a lot of entries in the log identifying "OSMDroid", "libwww-perl", "Dalvik", "Java" which all seem to be default User-Agent strings, and together (without referer) account for 11% of daily usage.

@pnorman
Copy link
Contributor

@pnorman pnorman commented Aug 12, 2016

@pnorman, yes, please!

Sent. I don't think we need to wait for any responses because we're not changing the usage policy, just automatically rate limiting those who don't follow it.

@zerebubuth
Copy link
Contributor Author

@zerebubuth zerebubuth commented Aug 12, 2016

@pnorman that's great, thanks! Agreed about the timing - this is enforcement of the policy rather than a change to it. I want to try and test this config to make sure that it only blocks the things it's intended to. If anyone could help with that, it would be greatly appreciated.

@zerebubuth
Copy link
Contributor Author

@zerebubuth zerebubuth commented Aug 12, 2016

I tried testing it with a hacked around version of a VM from gravitystorm#1, but I'm not getting the responses I expected. When trying with a referer, I was able to do 100 reqs in 152s, but without 100 reqs in 277s. That's 82% slower, but I was expecting 10x slower, given how the config is set up.

I was doing this on a VM with tile.openstreetmap.org as the upstream parent, so there are networking delays. And Squid delay pools only come into effect for misses, so I requested random tiles in an attempt to make sure most were misses from my local cache.

Does that sound like it's working? I'm still unsure.

@Zverik
Copy link

@Zverik Zverik commented Aug 12, 2016

Is it live? Can we test it on the production? :)

@zerebubuth
Copy link
Contributor Author

@zerebubuth zerebubuth commented Aug 12, 2016

I did a longer run, 650 requests, and the gap is closed - only 19% slower using no referer. I don't think it's doing what I wanted it to 😞

@Zverik no, not live yet. I'm not confident enough that it's doing the right thing.

@zerebubuth
Copy link
Contributor Author

@zerebubuth zerebubuth commented Aug 13, 2016

I refined my test VM, mainly by commenting out the line which allows localhost unlimited access, and I'm getting the results expected: With referer, 21s & without 211s.

@Firefishy
Copy link
Member

@Firefishy Firefishy commented Aug 13, 2016

The localhost line is required in prod, so that nginx layer doesn't get rate limited.

@zerebubuth
Copy link
Contributor Author

@zerebubuth zerebubuth commented Aug 13, 2016

Oh, yeah, sorry - I wasn't suggesting it become part of this patch. But I thought I'd record it for posterity if anyone else wanted to set up a test VM to verify that delay pools are doing what's expected.

@tomhughes
Copy link
Member

@tomhughes tomhughes commented Aug 14, 2016

Do we think this is ready to merge then?

@tomhughes
Copy link
Member

@tomhughes tomhughes commented Aug 14, 2016

Do we need to update https://github.com/openstreetmap/chef/blob/master/cookbooks/munin/files/default/plugins/squid_delay_pools at all for this? Do we want separate graphs (if possible) for the two sets of delay pools?

…r, and restrict the existing graph to those with a referer.
@zerebubuth
Copy link
Contributor Author

@zerebubuth zerebubuth commented Aug 14, 2016

The changes in f84e757 split the graphs into two; one for the "normal" delay pools, which is mostly requests with a referer, and the other for the "no referer" delay pools.

I think this is mergeable, as long as the last change looks okay?

@openstreetmap-mirror openstreetmap-mirror merged commit f84e757 into openstreetmap:master Aug 14, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pnorman
Copy link
Contributor

@pnorman pnorman commented Aug 16, 2016

Munin shows the delay is working and avg and max dropped tiles are about 30% of what they were, tiles are fresher, and overall the metrics show an improved experience for mappers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.