From 56d87db32cffc4c1e7be410da08c3b3e4bd1dcc0 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sun, 15 Mar 2015 19:07:23 -0700 Subject: [PATCH] Reduce Rack Attack false positives by clearing out auth failure count upon successful Git over HTTP authentication. Add logging when a ban goes into effect for debugging. Issue #1171 --- CHANGELOG | 1 + config/gitlab.yml.example | 3 ++ config/initializers/1_settings.rb | 1 + lib/gitlab/backend/grack_auth.rb | 45 +++++++++++----- lib/gitlab/backend/rack_attack_helpers.rb | 31 +++++++++++ spec/lib/gitlab/backend/grack_auth_spec.rb | 52 ++++++++++++++++++- .../backend/rack_attack_helpers_spec.rb | 35 +++++++++++++ 7 files changed, 153 insertions(+), 15 deletions(-) create mode 100644 lib/gitlab/backend/rack_attack_helpers.rb create mode 100644 spec/lib/gitlab/backend/rack_attack_helpers_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 3ecc45cde07..5ee64e6f540 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -8,6 +8,7 @@ v 7.10.0 (unreleased) - Update poltergeist to version 1.6.0 to support PhantomJS 2.0 (Zeger-Jan van de Weg) - Fix cross references when usernames, milestones, or project names contain underscores (Stan Hu) - Disable reference creation for comments surrounded by code/preformatted blocks (Stan Hu) + - Reduce Rack Attack false positives causing 403 errors during HTTP authentication (Stan Hu) - enable line wrapping per default and remove the checkbox to toggle it (Hannes Rosenögger) - extend the commit calendar to show the actual commits made on a date (Hannes Rosenögger) - Fix a link in the patch update guide diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index a85db10e019..c4a0fefb7ab 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -285,6 +285,9 @@ production: &base rack_attack: git_basic_auth: + # Rack Attack IP banning enabled + # enabled: true + # # Whitelist requests from 127.0.0.1 for web proxies (NGINX/Apache) with incorrect headers # ip_whitelist: ["127.0.0.1"] # diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 70af7a829c4..15c1ae9466f 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -183,6 +183,7 @@ Settings['extra'] ||= Settingslogic.new({}) # Settings['rack_attack'] ||= Settingslogic.new({}) Settings.rack_attack['git_basic_auth'] ||= Settingslogic.new({}) +Settings.rack_attack.git_basic_auth['enabled'] = true if Settings.rack_attack.git_basic_auth['enabled'].nil? Settings.rack_attack.git_basic_auth['ip_whitelist'] ||= %w{127.0.0.1} Settings.rack_attack.git_basic_auth['maxretry'] ||= 10 Settings.rack_attack.git_basic_auth['findtime'] ||= 1.minute diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb index ee877e099b1..ffe4565ef1e 100644 --- a/lib/gitlab/backend/grack_auth.rb +++ b/lib/gitlab/backend/grack_auth.rb @@ -1,3 +1,4 @@ +require_relative 'rack_attack_helpers' require_relative 'shell_env' module Grack @@ -85,25 +86,41 @@ module Grack user = oauth_access_token_check(login, password) end - return user if user.present? - - # At this point, we know the credentials were wrong. We let Rack::Attack - # know there was a failed authentication attempt from this IP. This - # information is stored in the Rails cache (Redis) and will be used by - # the Rack::Attack middleware to decide whether to block requests from - # this IP. + # If the user authenticated successfully, we reset the auth failure count + # from Rack::Attack for that IP. A client may attempt to authenticate + # with a username and blank password first, and only after it receives + # a 401 error does it present a password. Resetting the count prevents + # false positives from occurring. + # + # Otherwise, we let Rack::Attack know there was a failed authentication + # attempt from this IP. This information is stored in the Rails cache + # (Redis) and will be used by the Rack::Attack middleware to decide + # whether to block requests from this IP. config = Gitlab.config.rack_attack.git_basic_auth - Rack::Attack::Allow2Ban.filter(@request.ip, config) do - # Unless the IP is whitelisted, return true so that Allow2Ban - # increments the counter (stored in Rails.cache) for the IP - if config.ip_whitelist.include?(@request.ip) - false + + if config.enabled + if user + # A successful login will reset the auth failure count from this IP + Rack::Attack::Allow2Ban.reset(@request.ip, config) else - true + banned = Rack::Attack::Allow2Ban.filter(@request.ip, config) do + # Unless the IP is whitelisted, return true so that Allow2Ban + # increments the counter (stored in Rails.cache) for the IP + if config.ip_whitelist.include?(@request.ip) + false + else + true + end + end + + if banned + Rails.logger.info "IP #{@request.ip} failed to login " \ + "as #{login} but has been temporarily banned from Git auth" + end end end - nil # No user was found + user end def authorized_request? diff --git a/lib/gitlab/backend/rack_attack_helpers.rb b/lib/gitlab/backend/rack_attack_helpers.rb new file mode 100644 index 00000000000..8538f3f6eca --- /dev/null +++ b/lib/gitlab/backend/rack_attack_helpers.rb @@ -0,0 +1,31 @@ +# rack-attack v4.2.0 doesn't yet support clearing of keys. +# Taken from https://github.com/kickstarter/rack-attack/issues/113 +class Rack::Attack::Allow2Ban + def self.reset(discriminator, options) + findtime = options[:findtime] or raise ArgumentError, "Must pass findtime option" + + cache.reset_count("#{key_prefix}:count:#{discriminator}", findtime) + cache.delete("#{key_prefix}:ban:#{discriminator}") + end +end + +class Rack::Attack::Cache + def reset_count(unprefixed_key, period) + epoch_time = Time.now.to_i + # Add 1 to expires_in to avoid timing error: http://git.io/i1PHXA + expires_in = period - (epoch_time % period) + 1 + key = "#{(epoch_time / period).to_i}:#{unprefixed_key}" + delete(key) + end + + def delete(unprefixed_key) + store.delete("#{prefix}:#{unprefixed_key}") + end +end + +class Rack::Attack::StoreProxy::RedisStoreProxy + def delete(key, options={}) + self.del(key) + rescue Redis::BaseError + end +end diff --git a/spec/lib/gitlab/backend/grack_auth_spec.rb b/spec/lib/gitlab/backend/grack_auth_spec.rb index 768312f0028..d0aad54f677 100644 --- a/spec/lib/gitlab/backend/grack_auth_spec.rb +++ b/spec/lib/gitlab/backend/grack_auth_spec.rb @@ -6,7 +6,7 @@ describe Grack::Auth do let(:app) { lambda { |env| [200, {}, "Success!"] } } let!(:auth) { Grack::Auth.new(app) } - let(:env) { + let(:env) { { "rack.input" => "", "REQUEST_METHOD" => "GET", @@ -85,6 +85,17 @@ describe Grack::Auth do it "responds with status 401" do expect(status).to eq(401) end + + context "when the user is IP banned" do + before do + expect(Rack::Attack::Allow2Ban).to receive(:filter).and_return(true) + allow_any_instance_of(Rack::Request).to receive(:ip).and_return('1.2.3.4') + end + + it "responds with status 401" do + expect(status).to eq(401) + end + end end context "when authentication succeeds" do @@ -109,10 +120,49 @@ describe Grack::Auth do end context "when the user isn't blocked" do + before do + expect(Rack::Attack::Allow2Ban).to receive(:reset) + end + it "responds with status 200" do expect(status).to eq(200) end end + + context "when blank password attempts follow a valid login" do + let(:options) { Gitlab.config.rack_attack.git_basic_auth } + let(:maxretry) { options[:maxretry] - 1 } + let(:ip) { '1.2.3.4' } + + before do + allow_any_instance_of(Rack::Request).to receive(:ip).and_return(ip) + Rack::Attack::Allow2Ban.reset(ip, options) + end + + after do + Rack::Attack::Allow2Ban.reset(ip, options) + end + + def attempt_login(include_password) + password = include_password ? user.password : "" + env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, password) + Grack::Auth.new(app) + auth.call(env).first + end + + it "repeated attempts followed by successful attempt" do + for n in 0..maxretry do + expect(attempt_login(false)).to eq(401) + end + + expect(attempt_login(true)).to eq(200) + expect(Rack::Attack::Allow2Ban.send(:banned?, ip)).to eq(nil) + + for n in 0..maxretry do + expect(attempt_login(false)).to eq(401) + end + end + end end context "when the user doesn't have access to the project" do diff --git a/spec/lib/gitlab/backend/rack_attack_helpers_spec.rb b/spec/lib/gitlab/backend/rack_attack_helpers_spec.rb new file mode 100644 index 00000000000..2ac496fd669 --- /dev/null +++ b/spec/lib/gitlab/backend/rack_attack_helpers_spec.rb @@ -0,0 +1,35 @@ +require "spec_helper" + +describe 'RackAttackHelpers' do + describe 'reset' do + let(:discriminator) { 'test-key'} + let(:maxretry) { 5 } + let(:period) { 1.minute } + let(:options) { { findtime: period, bantime: 60, maxretry: maxretry } } + + def do_filter + for i in 1..maxretry - 1 do + status = Rack::Attack::Allow2Ban.filter(discriminator, options) { true } + expect(status).to eq(false) + end + end + + def do_reset + Rack::Attack::Allow2Ban.reset(discriminator, options) + end + + before do + do_reset + end + + after do + do_reset + end + + it 'user is not banned after n - 1 retries' do + do_filter + do_reset + do_filter + end + end +end