From c1de9198522103ae5af3c5e03beb42ce01c97905 Mon Sep 17 00:00:00 2001 From: Konstantin Haase Date: Tue, 5 Jul 2016 11:46:33 +0200 Subject: [PATCH 1/7] Don't run Rack::Attack for Enterprise. (#287) On enterprise, the reverse proxy is not correctly set up, and therefore the client IP address not passed through properly. For that reason, all requests look like they originate from the same client, and if one gets blocked, everyone gets blocked. --- lib/travis/api/app.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/travis/api/app.rb b/lib/travis/api/app.rb index 1e1ec814..d61a498b 100644 --- a/lib/travis/api/app.rb +++ b/lib/travis/api/app.rb @@ -132,7 +132,7 @@ module Travis::Api use Travis::Api::App::Middleware::UserAgentTracker # make sure this is below ScopeCheck so we have the token - use Rack::Attack if Endpoint.production? + use Rack::Attack if Endpoint.production? and not Travis.config.enterprise # if this is a v3 API request, ignore everything after use Travis::API::V3::OptIn From e6d76079166be037453b886e6df06c237862cdbc Mon Sep 17 00:00:00 2001 From: Igor Wiedler Date: Tue, 5 Jul 2016 12:30:10 +0200 Subject: [PATCH 2/7] safelist github IP range in Rack::Attack --- Gemfile | 1 + Gemfile.lock | 2 ++ lib/travis/api/attack.rb | 6 ++++++ spec/unit/attack_spec.rb | 15 ++++++++++++++- 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index ab1dc3bf..9b5f3f56 100644 --- a/Gemfile +++ b/Gemfile @@ -32,6 +32,7 @@ gem 'micro_migrations' gem 'simplecov' gem 'skylight', '~> 0.6.0.beta.1' gem 'stackprof' +gem 'netaddr' gem 'jemalloc' gem 'customerio' diff --git a/Gemfile.lock b/Gemfile.lock index 0dda637b..ff22e0be 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -252,6 +252,7 @@ GEM multipart-post (2.0.0) net-http-persistent (2.9.4) net-http-pipeline (1.0.1) + netaddr (1.5.1) os (0.9.6) pg (0.18.4) proxies (0.2.1) @@ -389,6 +390,7 @@ DEPENDENCIES micro_migrations mocha (~> 0.12) mustermann! + netaddr pry rack-attack rack-cache! diff --git a/lib/travis/api/attack.rb b/lib/travis/api/attack.rb index 4bf4fd5d..dfb9d7e0 100644 --- a/lib/travis/api/attack.rb +++ b/lib/travis/api/attack.rb @@ -1,4 +1,5 @@ require 'rack/attack' +require 'cidr' class Rack::Attack class Request @@ -35,6 +36,11 @@ class Rack::Attack /\.(png|svg)$/.match(request.path) end + # https://help.github.com/articles/what-ip-addresses-does-github-use-that-i-should-whitelist/ + whitelist('safelist anything coming from github') do |request| + NetAddr::CIDR.create('192.30.252.0/22').contains?(request.ip) + end + #### # Whitelisted IP addresses whitelist('whitelist client requesting from redis') do |request| diff --git a/spec/unit/attack_spec.rb b/spec/unit/attack_spec.rb index 7bae3546..c1b0ea9f 100644 --- a/spec/unit/attack_spec.rb +++ b/spec/unit/attack_spec.rb @@ -10,7 +10,20 @@ describe Rack::Attack do end end - describe 'non-image API request' do + describe 'request from GitHub ip' do + let(:request) { + env = Rack::MockRequest.env_for("https://api-test.travis-ci.org/repos/rails/rails/branches", { + 'REMOTE_ADDR' => '192.30.252.42' + }) + Rack::Attack::Request.new(env) + } + + it 'should not be safelisted' do + expect(Rack::Attack.whitelisted?(request)).to be_falsy + end + end + + describe 'non-safelisted request' do let(:request) { env = Rack::MockRequest.env_for("https://api-test.travis-ci.org/repos/rails/rails/branches") Rack::Attack::Request.new(env) From 7e8b65a31111690eca0fb0710e761d16b3aed667 Mon Sep 17 00:00:00 2001 From: Igor Wiedler Date: Tue, 5 Jul 2016 12:39:32 +0200 Subject: [PATCH 3/7] correct test case for GitHub IP check --- spec/unit/attack_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/unit/attack_spec.rb b/spec/unit/attack_spec.rb index c1b0ea9f..a469ae4f 100644 --- a/spec/unit/attack_spec.rb +++ b/spec/unit/attack_spec.rb @@ -18,8 +18,8 @@ describe Rack::Attack do Rack::Attack::Request.new(env) } - it 'should not be safelisted' do - expect(Rack::Attack.whitelisted?(request)).to be_falsy + it 'should be safelisted' do + expect(Rack::Attack.whitelisted?(request)).to be_truthy end end From 0d90c21dbd1529df6afaa77347b6527fcfa41c8d Mon Sep 17 00:00:00 2001 From: Igor Wiedler Date: Tue, 5 Jul 2016 12:39:44 +0200 Subject: [PATCH 4/7] require netaddr instead of cidr --- lib/travis/api/attack.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/travis/api/attack.rb b/lib/travis/api/attack.rb index dfb9d7e0..96af80bf 100644 --- a/lib/travis/api/attack.rb +++ b/lib/travis/api/attack.rb @@ -1,5 +1,5 @@ require 'rack/attack' -require 'cidr' +require 'netaddr' class Rack::Attack class Request From a210cf86614e7ac806e60997b7cee7e527cc20c1 Mon Sep 17 00:00:00 2001 From: Igor Wiedler Date: Tue, 5 Jul 2016 12:46:32 +0200 Subject: [PATCH 5/7] support nil ip --- lib/travis/api/attack.rb | 2 +- spec/unit/attack_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/travis/api/attack.rb b/lib/travis/api/attack.rb index 96af80bf..a834e38a 100644 --- a/lib/travis/api/attack.rb +++ b/lib/travis/api/attack.rb @@ -38,7 +38,7 @@ class Rack::Attack # https://help.github.com/articles/what-ip-addresses-does-github-use-that-i-should-whitelist/ whitelist('safelist anything coming from github') do |request| - NetAddr::CIDR.create('192.30.252.0/22').contains?(request.ip) + request.ip && NetAddr::CIDR.create('192.30.252.0/22').contains?(request.ip) end #### diff --git a/spec/unit/attack_spec.rb b/spec/unit/attack_spec.rb index a469ae4f..41aac8f8 100644 --- a/spec/unit/attack_spec.rb +++ b/spec/unit/attack_spec.rb @@ -13,7 +13,7 @@ describe Rack::Attack do describe 'request from GitHub ip' do let(:request) { env = Rack::MockRequest.env_for("https://api-test.travis-ci.org/repos/rails/rails/branches", { - 'REMOTE_ADDR' => '192.30.252.42' + 'REMOTE_ADDR' => '192.30.252.42', }) Rack::Attack::Request.new(env) } From d84d3983b8d059106d2714ae032728f1201a8a0e Mon Sep 17 00:00:00 2001 From: Igor Wiedler Date: Tue, 5 Jul 2016 12:58:58 +0200 Subject: [PATCH 6/7] update rack-attack to 5.0.0.beta1, use safelist/blocklist terminology --- Gemfile | 2 +- Gemfile.lock | 4 ++-- lib/travis/api/app/endpoint/authorization.rb | 4 ++-- lib/travis/api/attack.rb | 20 +++++++++++--------- lib/travis/api/v3/router.rb | 2 +- spec/unit/attack_spec.rb | 6 +++--- spec/v3/services/owner/find_spec.rb | 4 ++-- spec_core/model/job_spec.rb | 4 ++-- vendor/travis-core/lib/travis/model/job.rb | 4 ++-- 9 files changed, 26 insertions(+), 24 deletions(-) diff --git a/Gemfile b/Gemfile index 9b5f3f56..961c5542 100644 --- a/Gemfile +++ b/Gemfile @@ -21,7 +21,7 @@ gem 'sentry-raven' gem 'yard-sinatra', github: 'rkh/yard-sinatra' gem 'rack-contrib' gem 'rack-cache', github: 'rtomayko/rack-cache' -gem 'rack-attack' +gem 'rack-attack', '5.0.0.beta1' gem 'gh' gem 'bunny', '~> 0.8.0' gem 'dalli' diff --git a/Gemfile.lock b/Gemfile.lock index ff22e0be..15221eed 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -266,7 +266,7 @@ GEM pusher-signature (~> 0.1.8) pusher-signature (0.1.8) rack (1.4.7) - rack-attack (4.4.1) + rack-attack (5.0.0.beta1) rack rack-contrib (1.4.0) git-version-bump (~> 0.15) @@ -392,7 +392,7 @@ DEPENDENCIES mustermann! netaddr pry - rack-attack + rack-attack (= 5.0.0.beta1) rack-cache! rack-contrib rake (~> 0.9.2) diff --git a/lib/travis/api/app/endpoint/authorization.rb b/lib/travis/api/app/endpoint/authorization.rb index 3cf72469..30d85546 100644 --- a/lib/travis/api/app/endpoint/authorization.rb +++ b/lib/travis/api/app/endpoint/authorization.rb @@ -114,7 +114,7 @@ class Travis::Api::App # access token and user payload to the parent window via postMessage. # # However, the endpoint to send the payload to has to be explicitely - # whitelisted in production, as this is endpoint is only meant to be used + # safelisted in production, as this is endpoint is only meant to be used # with the official Travis CI client at the moment. # # Example usage: @@ -408,7 +408,7 @@ __END__ @@ invalid_target @@ common diff --git a/lib/travis/api/attack.rb b/lib/travis/api/attack.rb index a834e38a..95132907 100644 --- a/lib/travis/api/attack.rb +++ b/lib/travis/api/attack.rb @@ -32,34 +32,36 @@ class Rack::Attack "/auth/post_message/iframe" ] - whitelist('safelist build status images') do |request| + safelist('safelist build status images') do |request| /\.(png|svg)$/.match(request.path) end - # https://help.github.com/articles/what-ip-addresses-does-github-use-that-i-should-whitelist/ - whitelist('safelist anything coming from github') do |request| + # https://help.github.com/articles/what-ip-addresses-does-github-use-that-i-should-safelist/ + safelist('safelist anything coming from github') do |request| request.ip && NetAddr::CIDR.create('192.30.252.0/22').contains?(request.ip) end #### # Whitelisted IP addresses - whitelist('whitelist client requesting from redis') do |request| - Travis.redis.sismember(:api_whitelisted_ips, request.ip) + safelist('safelist client requesting from redis') do |request| + # TODO: deprecate :api_whitelisted_ips in favour of api_safelisted_ips + Travis.redis.sismember(:api_whitelisted_ips, request.ip) || Travis.redis.sismember(:api_safelisted_ips, request.ip) end #### # Ban based on: IP address # Ban time: indefinite # Ban after: manually banned - blacklist('block client requesting from redis') do |request| - Travis.redis.sismember(:api_blacklisted_ips, request.ip) + blocklist('block client requesting from redis') do |request| + # TODO: deprecate :api_blacklisted_ips in favour of api_blocklisted_ips + Travis.redis.sismember(:api_blacklisted_ips, request.ip) || Travis.redis.sismember(:api_blocklisted_ips, request.ip) end #### # Ban based on: IP address or access token # Ban time: 5 hours # Ban after: 10 POST requests within five minutes to /auth/github - blacklist('hammering /auth/github') do |request| + blocklist('hammering /auth/github') do |request| Rack::Attack::Allow2Ban.filter(request.identifier, maxretry: 2, findtime: 5.minutes, bantime: bantime(5.hours)) do request.post? and request.path == '/auth/github' end @@ -69,7 +71,7 @@ class Rack::Attack # Ban based on: IP address or access token # Ban time: 1 hour # Ban after: 10 POST requests within 30 seconds - blacklist('spamming with POST requests') do |request| + blocklist('spamming with POST requests') do |request| Rack::Attack::Allow2Ban.filter(request.identifier, maxretry: 10, findtime: 30.seconds, bantime: bantime(1.hour)) do request.post? and not POST_SAFELIST.include? request.path end diff --git a/lib/travis/api/v3/router.rb b/lib/travis/api/v3/router.rb index 91c5fd4a..2d29b9f1 100644 --- a/lib/travis/api/v3/router.rb +++ b/lib/travis/api/v3/router.rb @@ -29,7 +29,7 @@ module Travis::API::V3 result = service.run metrics.tick(:service) - env_params.each_key { |key| result.ignored_param(key, reason: "not whitelisted".freeze) unless filtered.include?(key) } + env_params.each_key { |key| result.ignored_param(key, reason: "not safelisted".freeze) unless filtered.include?(key) } response = render(result, env_params, env) metrics.tick(:renderer) diff --git a/spec/unit/attack_spec.rb b/spec/unit/attack_spec.rb index 41aac8f8..0f5dec39 100644 --- a/spec/unit/attack_spec.rb +++ b/spec/unit/attack_spec.rb @@ -6,7 +6,7 @@ describe Rack::Attack do } it 'should be safelisted' do - expect(Rack::Attack.whitelisted?(request)).to be_truthy + expect(Rack::Attack.safelisted?(request)).to be_truthy end end @@ -19,7 +19,7 @@ describe Rack::Attack do } it 'should be safelisted' do - expect(Rack::Attack.whitelisted?(request)).to be_truthy + expect(Rack::Attack.safelisted?(request)).to be_truthy end end @@ -30,7 +30,7 @@ describe Rack::Attack do } it 'should not be safelisted' do - expect(Rack::Attack.whitelisted?(request)).to be_falsy + expect(Rack::Attack.safelisted?(request)).to be_falsy end end end diff --git a/spec/v3/services/owner/find_spec.rb b/spec/v3/services/owner/find_spec.rb index 5bb8e591..a15e82f1 100644 --- a/spec/v3/services/owner/find_spec.rb +++ b/spec/v3/services/owner/find_spec.rb @@ -167,7 +167,7 @@ describe Travis::API::V3::Services::Owner::Find, set_app: true do "avatar_url" => nil, "@warnings" => [{ "@type" => "warning", - "message" => "query parameter organization.id not whitelisted, ignored", + "message" => "query parameter organization.id not safelisted, ignored", "warning_type" => "ignored_parameter", "parameter" => "organization.id"}] }} @@ -254,7 +254,7 @@ describe Travis::API::V3::Services::Owner::Find, set_app: true do "synced_at" => nil, "@warnings" => [{ "@type" => "warning", - "message" => "query parameter user.id not whitelisted, ignored", + "message" => "query parameter user.id not safelisted, ignored", "warning_type" => "ignored_parameter", "parameter" => "user.id"}] }} diff --git a/spec_core/model/job_spec.rb b/spec_core/model/job_spec.rb index 14c02f46..f5706e02 100644 --- a/spec_core/model/job_spec.rb +++ b/spec_core/model/job_spec.rb @@ -129,7 +129,7 @@ describe Job do } end - it 'removes addons items which are not whitelisted' do + it 'removes addons items which are not safelisted' do job = Job.new(repository: repo) config = { rvm: '1.8.7', addons: { sauce_connect: true, firefox: '22.0' }, @@ -328,7 +328,7 @@ describe Job do } end - it 'removes addons items which are not whitelisted' do + it 'removes addons items which are not safelisted' do config = { rvm: '1.8.7', addons: { sauce_connect: { diff --git a/vendor/travis-core/lib/travis/model/job.rb b/vendor/travis-core/lib/travis/model/job.rb index 1d86e278..1aac654e 100644 --- a/vendor/travis-core/lib/travis/model/job.rb +++ b/vendor/travis-core/lib/travis/model/job.rb @@ -14,7 +14,7 @@ class Job < Travis::Model require 'travis/model/job/test' require 'travis/model/env_helpers' - WHITELISTED_ADDONS = %w( + SAFELISTED_ADDONS = %w( apt apt_packages apt_sources @@ -167,7 +167,7 @@ class Job < Travis::Model def delete_addons(config) if config[:addons].is_a?(Hash) - config[:addons].keep_if { |key, _| WHITELISTED_ADDONS.include? key.to_s } + config[:addons].keep_if { |key, _| SAFELISTED_ADDONS.include? key.to_s } else config.delete(:addons) end From 79c14d697e262e59c34efb4dc42dac95a693f95e Mon Sep 17 00:00:00 2001 From: Igor Wiedler Date: Tue, 5 Jul 2016 13:02:00 +0200 Subject: [PATCH 7/7] only create/allocate GitHub CIDR object once --- lib/travis/api/attack.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/travis/api/attack.rb b/lib/travis/api/attack.rb index a834e38a..130431b0 100644 --- a/lib/travis/api/attack.rb +++ b/lib/travis/api/attack.rb @@ -32,13 +32,15 @@ class Rack::Attack "/auth/post_message/iframe" ] + GITHUB_CIDR = NetAddr::CIDR.create('192.30.252.0/22') + whitelist('safelist build status images') do |request| /\.(png|svg)$/.match(request.path) end # https://help.github.com/articles/what-ip-addresses-does-github-use-that-i-should-whitelist/ whitelist('safelist anything coming from github') do |request| - request.ip && NetAddr::CIDR.create('192.30.252.0/22').contains?(request.ip) + request.ip && GITHUB_CIDR.contains?(request.ip) end ####