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

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