diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-03-25 00:51:40 +0300 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-03-25 00:51:40 +0300 |
commit | fc4af9b1975827d4e5ead18dc3468d9aa29cd9ac (patch) | |
tree | de14fb7bdf83713dda2602f164b4762d7cc23831 /spec | |
parent | 533b5721c62203c190c229d2bde91817277e9563 (diff) | |
parent | 56d87db32cffc4c1e7be410da08c3b3e4bd1dcc0 (diff) |
Merge branch 'git-auth-rack-attack-improvements' into 'master'
Reduce Rack Attack false positives causing 403 errors during HTTP authentication
### What does this MR do?
This MR reduces false positives causing `403 Forbidden` messages after HTTP authentication.
A Git client may attempt to access a repository without a password. If it receives a 401 error, the client often will try again, this time supplying a password. The problem is that `grack_auth.rb` considers a blank password an authentication failure and increases a Redis counter each time this happens. With enough requests, an IP can be banned temporarily even though previous attempts may have been successful. This leads users to see `403 Forbidden` errors until the ban times out (default: 1 hour).
To reduce the chance of a false positive, this MR resets the counter upon a successful authentication from an IP.
In addition, this MR logs when a user has been banned and introduces the ability to disable Rack Attack via a config variable.
### Are there points in the code the reviewer needs to double check?
rack-attack v4.2.0 doesn't support the ability to clear counters out of the box, so `rack_attack_helpers.rb` includes a number of monkey patches to make it work. It looks like this functionality may be added in v4.3.0. I've also sent pull requests to rack-attack to add the functionality necessary to delete a key.
Each time an authentication is successful, the Redis counter for that IP is cleared. I deemed it better to clear the counter than to allow for blank passwords, since the latter seems like a security risk.
### Why was this MR needed?
It was quite difficult to figure out why users were seeing `403 Forbidden`, which is why the log message was added. Users were getting a lot of false positives when accessing repositories with HTTPS. Including the username in the HTTPS URL (e.g. `https://username@mydomain.com/account/repo.git`) caused authentication failures because while the git client provided the username, it left the password blank, leading to an authentication failure.
### What are the relevant issue numbers / [Feature requests](http://feedback.gitlab.com/)?
See Issue #1171
https://github.com/kickstarter/rack-attack/issues/113
See merge request !392
Diffstat (limited to 'spec')
-rw-r--r-- | spec/lib/gitlab/backend/grack_auth_spec.rb | 52 | ||||
-rw-r--r-- | spec/lib/gitlab/backend/rack_attack_helpers_spec.rb | 35 |
2 files changed, 86 insertions, 1 deletions
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 |