diff --git a/CHANGELOG b/CHANGELOG
index 3ecc45cde07065fd04cadfe1b0840e6a2f8ab3b6..5ee64e6f540ac96269d1a85ddf905a4c2de9c61d 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 a85db10e01978304b765c1c74a27ed807178369d..c4a0fefb7ab8f351608b529e966c04cf086cf318 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 70af7a829c4d553cfbb25ef702c81cda0ea30564..15c1ae9466f2cf33c43875212c48da031ee3f2b9 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 ee877e099b17ae22ec5e12d25bd94ec0305c2f40..ffe4565ef1edef82c02f5b712f9061cb410e75b5 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 0000000000000000000000000000000000000000..8538f3f6ecab3eaf0069455447ba47c84fa9693b
--- /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 768312f0028805f736cf3d6cc85c546869361d02..d0aad54f677fa11e79a95c897a2689e91ae4de44 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 0000000000000000000000000000000000000000..2ac496fd669a62cb3b48b0dd9377d580a6df8bbf
--- /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