diff --git a/Gemfile b/Gemfile index 00cf77b0301..eae7aba60bd 100644 --- a/Gemfile +++ b/Gemfile @@ -30,7 +30,6 @@ gem 'phonelib' gem 'phony_rails' gem 'premailer-rails' gem 'proofer', github: '18F/identity-proofer-gem', branch: 'master' -gem 'rack-attack' gem 'rack-cors', require: 'rack/cors' gem 'readthis' gem 'redis-session-store', github: '18F/redis-session-store', branch: 'master' diff --git a/Gemfile.lock b/Gemfile.lock index 7499ff35de2..b53beb5c5a0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -712,7 +712,6 @@ DEPENDENCIES premailer-rails proofer! pry-byebug - rack-attack rack-cors rack-mini-profiler rack-test diff --git a/config/application.rb b/config/application.rb index ab977b40760..a74e6b20854 100644 --- a/config/application.rb +++ b/config/application.rb @@ -11,8 +11,6 @@ class Application < Rails::Application config.autoload_paths << Rails.root.join('app', 'mailers', 'concerns') config.time_zone = 'UTC' - # config.middleware.use Rack::Attack unless Figaro.env.disable_email_sending == 'true' - config.browserify_rails.force = true config.browserify_rails.commandline_options = '-t [ babelify --presets [ es2015 ] ]' config.i18n.load_path += Dir[Rails.root.join('config', 'locales', '**', '*.{yml}')] diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb deleted file mode 100644 index f75a491a419..00000000000 --- a/config/initializers/rack_attack.rb +++ /dev/null @@ -1,115 +0,0 @@ -module Rack - class Attack - # If the app is behind a load balancer, `ip` will return the IP of the - # load balancer instead of the actual IP the request came from, and since - # all requests will seem to come from the same IP, throttling will be - # triggered right away. To make sure we have the correct IP, we use - # ActionDispatch#remote_ip, which determines the correct IP more thoroughly - # than Rack. - class Request < ::Rack::Request - def remote_ip - @remote_ip ||= (env['action_dispatch.remote_ip'] || ip).to_s - end - end - - ### Configure Cache ### - - # Note: The store is only used for throttling (not blacklisting and - # whitelisting). It must implement .increment and .write like - # ActiveSupport::Cache::Store - - cache = Readthis::Cache.new( - expires_in: 2.weeks.to_i, - redis: { url: Figaro.env.redis_url, driver: :hiredis } - ) - - Rack::Attack.cache.store = cache - - ### Configure Whitelisting ### - - # Always allow requests from localhost - # (blacklist & throttles are skipped) - unless Rails.env.production? - safelist('allow from localhost') do |req| - req.remote_ip == '127.0.0.1' || req.remote_ip == '::1' - end - end - - ### Throttle Spammy Clients ### - - # If any single client IP is making tons of requests, then they're - # probably malicious or a poorly-configured scraper. Either way, they - # don't deserve to hog all of the app server's CPU. Cut them off! - # - # Note: If you're serving assets through rack, those requests may be - # counted by rack-attack and this throttle may be activated too - # quickly. If so, enable the condition to exclude them from tracking. - - # Throttle all requests by IP (60rpm) - # - # Key: "rack::attack:#{Time.now.to_i/:period}:req/ip:#{req.ip}" - throttle( - 'req/ip', - limit: Figaro.env.requests_per_ip_limit.to_i, - period: Figaro.env.requests_per_ip_period.to_i.seconds - ) do |req| - req.remote_ip unless req.path.starts_with?('/assets') - end - - ### Prevent Brute-Force Login Attacks ### - - # The most common brute-force login attack is a brute-force password - # attack where an attacker simply tries a large number of emails and - # passwords to see if any credentials match. - # - # Another common method of attack is to use a swarm of computers with - # different IPs to try brute-forcing a password for a specific account. - - # Throttle sign in attempts by IP address with an exponential backoff - # - # Key: "rack::attack:#{Time.now.to_i/:period}:logins/ip:#{req.ip}" - # - # Allows 20 requests in 8 seconds - # 40 requests in 64 seconds - # ... - # 100 requests in 0.38 days (~250 requests/day) - (1..5).each do |level| - throttle( - "logins/ip/level_#{level}", - limit: (Figaro.env.logins_per_ip_limit.to_i * level), - period: (Figaro.env.logins_per_ip_period.to_i**level).seconds - ) do |req| - req.remote_ip if req.path == '/' && req.post? - end - end - - ### Custom Throttle Response ### - - # By default, Rack::Attack returns an HTTP 429 for throttled responses, - # which is just fine. - # - # If you want to return 503 so that the attacker might be fooled into - # believing that they've successfully broken your app (or you just want to - # customize the response), then uncomment these lines. - self.throttled_response = lambda do |_env| - [ - 429, # status - { 'Content-Type' => 'text/html' }, # headers - [::File.read('public/429.html')] # body - ] - end - - self.blocklisted_response = throttled_response - end -end - -ActiveSupport::Notifications.subscribe('rack.attack') do |_name, _start, _finish, _request_id, req| - discriminator = req.env['rack.attack.match_discriminator'] || req.env['warden'].user&.uuid - - Rails.logger.warn({ - discriminator: discriminator, - event: 'throttle', - type: req.env['rack.attack.matched'], - user_ip: req.remote_ip, - }.to_json) -end diff --git a/docs/SECURITY.md b/docs/SECURITY.md index e69183967be..82fff91c105 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -31,7 +31,6 @@ public key. #### Network security -We rely on [fail2ban](http://www.fail2ban.org/wiki/index.php/Main_Page) within [Rack::Attack](https://github.com/kickstarter/rack-attack) to identify malicious requestors and ban appropriately. Rack::Attack enables throttling for prevention of DoS attacks and mitigates abusive requests, allowing us to rely less on short-term, one-off hacks to block a particular attack. ``` @jgrevich - relevant network security? diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 5c106cb0494..ab518c8571f 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -35,7 +35,6 @@ config.before(:suite) do Rails.application.load_seed - Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new end config.before(:each) do @@ -44,7 +43,6 @@ config.before(:each) do allow(ValidateEmail).to receive(:mx_valid?).and_return(true) - Rack::Attack.cache.store.clear end config.before(:each, twilio: true) do diff --git a/spec/requests/rack_attack_spec.rb b/spec/requests/rack_attack_spec.rb deleted file mode 100644 index 1328021f04c..00000000000 --- a/spec/requests/rack_attack_spec.rb +++ /dev/null @@ -1,273 +0,0 @@ -require 'rails_helper' - -xdescribe 'throttling requests' do - include Rack::Test::Methods - - def app - Rails.application - end - - describe 'whitelists' do - it 'allows all requests from localhost' do - get '/' - - expect(last_request.env['rack.attack.throttle_data']).to be_nil - end - end - - describe 'high requests per ip' do - it 'reads the limit and period from ENV vars' do - get '/', params: {}, headers: { 'REMOTE_ADDR' => '1.2.3.4' } - - data = { - count: 1, - limit: Figaro.env.requests_per_ip_limit.to_i, - period: Figaro.env.requests_per_ip_period.to_i.seconds, - } - - expect(last_request.env['rack.attack.throttle_data']['req/ip']).to eq(data) - end - - context 'when the number of requests is lower than the limit' do - it 'does not throttle' do - 2.times do - get '/', params: {}, headers: { 'HTTP_X_FORWARDED_FOR' => '1.2.3.4' } - end - - expect(last_response.status).to_not eq(429) - end - end - - context 'when the request is for an asset' do - it 'does not throttle' do - 4.times do - get '/assets/application.js', params: {}, headers: { 'HTTP_X_FORWARDED_FOR' => '1.2.3.4' } - end - - expect(last_response.status).to_not eq(429) - end - end - - context 'when the number of requests is higher than the limit' do - before do - allow(Rails.logger).to receive(:warn) - - 4.times do - get '/', params: {}, headers: { 'HTTP_X_FORWARDED_FOR' => '1.2.3.4' } - end - end - - it 'throttles' do - expect(last_response.status).to eq(429) - end - - it 'returns a custom body' do - expect(last_response.body). - to include('Your request was denied because of unusual activity.') - end - - it 'returns text/html for Content-type' do - expect(last_response.header['Content-type']).to include('text/html') - end - - it 'logs the throttle' do - analytics_hash = { - discriminator: '1.2.3.4', - event: 'throttle', - type: 'req/ip', - user_ip: '1.2.3.4', - } - - expect(Rails.logger).to have_received(:warn).with(analytics_hash.to_json) - end - end - end - - describe 'logins per ip' do - it 'reads the limit and period from ENV vars' do - post( - '/', - params: { - user: { email: 'test@example.com' }, - }, - headers: { 'REMOTE_ADDR' => '1.2.3.4' } - ) - - data = { - count: 1, - limit: Figaro.env.logins_per_ip_limit.to_i, - period: Figaro.env.logins_per_ip_period.to_i.seconds, - } - - expect(last_request.env['rack.attack.throttle_data']['logins/ip/level_1']).to eq(data) - end - - it 'uses an exponential backoff' do - 3.times do - post '/', params: { - user: { email: 'test@example.com' }, - }, headers: { 'HTTP_X_FORWARDED_FOR' => '1.2.3.4' } - Timecop.travel(120.seconds) - end - - (1..5).each do |level| - expect(last_request.env['rack.attack.throttle_data']["logins/ip/level_#{level}"][:period]). - to eq(Figaro.env.logins_per_ip_period.to_i**level) - end - - (1..5).each do |level| - expect(last_request.env['rack.attack.throttle_data']["logins/ip/level_#{level}"][:limit]). - to eq(Figaro.env.logins_per_ip_limit.to_i * level) - end - - Timecop.return - end - - context 'when the number of requests is lower than the limit' do - it 'does not throttle' do - 2.times do - post '/', params: { - user: { email: 'test@example.com' }, - }, headers: { 'HTTP_X_FORWARDED_FOR' => '1.2.3.4' } - end - - expect(last_response.status).to_not eq(429) - end - end - - context 'when the request is not a sign in attempt' do - it 'does not throttle' do - expect(Rails.logger).to_not receive(:warn) - - 3.times do - get '/', params: {}, headers: { 'HTTP_X_FORWARDED_FOR' => '1.2.3.4' } - end - end - end - - context 'when the number of logins per ip is higher than the limit per period' do - before do - allow(Rails.logger).to receive(:warn) - - 3.times do - post '/', params: { - user: { email: 'test@example.com' }, - }, headers: { 'HTTP_X_FORWARDED_FOR' => '1.2.3.4' } - end - end - - it 'throttles' do - expect(last_response.status).to eq(429) - end - - it 'returns a custom body' do - expect(last_response.body). - to include('Your request was denied because of unusual activity.') - end - - it 'returns text/html for Content-type' do - expect(last_response.header['Content-type']).to include('text/html') - end - - it 'logs the throttle' do - analytics_hash = { - discriminator: '1.2.3.4', - event: 'throttle', - type: 'logins/ip/level_1', - user_ip: '1.2.3.4', - } - - expect(Rails.logger).to have_received(:warn).with(analytics_hash.to_json) - end - end - end - - describe '#remote_ip' do - let(:env) { double 'env' } - - it 'uses ActionDispatch to calculate the IP' do - expect(env).to receive(:[]).with('action_dispatch.remote_ip').and_return('127.0.0.1') - - Rack::Attack::Request.new(env).remote_ip - end - end - - describe 'OTP delivery blocklist' do - it 'blocks the user for bantime after maxretry OTP requests within findtime period' do - maxretry_limit = Figaro.env.otp_delivery_blocklist_maxretry.to_i - over_maxretry_limit = maxretry_limit + 1 - allow(Figaro.env).to receive(:requests_per_ip_limit).and_return('300') - allow(Rails.logger).to receive(:warn) - user = create(:user, :signed_up, phone: '+1 (202) 555-0100') - second_user_with_same_number = create(:user, :signed_up, phone: '+1 (202) 555-0100') - - # sign in with first user and have them trigger the throttle - post( - new_user_session_path, - params: { - 'user[email]' => user.email, - 'user[password]' => user.password, - } - ) - - get( - '/otp/send', - params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }, - headers: { 'REMOTE_ADDR' => '1.2.3.4' } - ) - - expect(last_response.status).to eq 302 - - over_maxretry_limit.times do - get( - '/otp/send', - params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }, - headers: { 'REMOTE_ADDR' => '1.2.3.4' } - ) - end - - analytics_hash = { - discriminator: user.uuid, - event: 'throttle', - type: 'OTP delivery', - user_ip: '1.2.3.4', - } - - expect(last_response.status).to eq(429) - expect(Rails.logger).to have_received(:warn).exactly(:twice).with(analytics_hash.to_json) - - delete destroy_user_session_path - - # sign in with second user, and make sure they are blocked on the first - # attempt since they have the same number and the throttling is based - # on the phone number - post( - new_user_session_path, - params: { - 'user[email]' => second_user_with_same_number.email, - 'user[password]' => second_user_with_same_number.password, - } - ) - - get( - '/otp/send', - params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }, - headers: { 'REMOTE_ADDR' => '1.2.3.5' } - ) - - analytics_hash = { - discriminator: second_user_with_same_number.uuid, - event: 'throttle', - type: 'OTP delivery', - user_ip: '1.2.3.5', - } - - expect(last_response.status).to eq(429) - expect(Rails.logger).to have_received(:warn).with(analytics_hash.to_json) - end - - it 'uses the throttled_response for the blocklisted_response' do - expect(Rack::Attack.blocklisted_response).to eq Rack::Attack.throttled_response - end - end -end