Commit 56d87db3 authored by Stan Hu's avatar Stan Hu

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
parent c3c97034
...@@ -8,6 +8,7 @@ v 7.10.0 (unreleased) ...@@ -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) - 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) - 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) - 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) - 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) - extend the commit calendar to show the actual commits made on a date (Hannes Rosenögger)
- Fix a link in the patch update guide - Fix a link in the patch update guide
......
...@@ -285,6 +285,9 @@ production: &base ...@@ -285,6 +285,9 @@ production: &base
rack_attack: rack_attack:
git_basic_auth: 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 # Whitelist requests from 127.0.0.1 for web proxies (NGINX/Apache) with incorrect headers
# ip_whitelist: ["127.0.0.1"] # ip_whitelist: ["127.0.0.1"]
# #
......
...@@ -183,6 +183,7 @@ Settings['extra'] ||= Settingslogic.new({}) ...@@ -183,6 +183,7 @@ Settings['extra'] ||= Settingslogic.new({})
# #
Settings['rack_attack'] ||= Settingslogic.new({}) Settings['rack_attack'] ||= Settingslogic.new({})
Settings.rack_attack['git_basic_auth'] ||= 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['ip_whitelist'] ||= %w{127.0.0.1}
Settings.rack_attack.git_basic_auth['maxretry'] ||= 10 Settings.rack_attack.git_basic_auth['maxretry'] ||= 10
Settings.rack_attack.git_basic_auth['findtime'] ||= 1.minute Settings.rack_attack.git_basic_auth['findtime'] ||= 1.minute
......
require_relative 'rack_attack_helpers'
require_relative 'shell_env' require_relative 'shell_env'
module Grack module Grack
...@@ -85,15 +86,24 @@ module Grack ...@@ -85,15 +86,24 @@ module Grack
user = oauth_access_token_check(login, password) user = oauth_access_token_check(login, password)
end end
return user if user.present? # If the user authenticated successfully, we reset the auth failure count
# from Rack::Attack for that IP. A client may attempt to authenticate
# At this point, we know the credentials were wrong. We let Rack::Attack # with a username and blank password first, and only after it receives
# know there was a failed authentication attempt from this IP. This # a 401 error does it present a password. Resetting the count prevents
# information is stored in the Rails cache (Redis) and will be used by # false positives from occurring.
# the Rack::Attack middleware to decide whether to block requests from #
# this IP. # 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 config = Gitlab.config.rack_attack.git_basic_auth
Rack::Attack::Allow2Ban.filter(@request.ip, config) do
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
banned = Rack::Attack::Allow2Ban.filter(@request.ip, config) do
# Unless the IP is whitelisted, return true so that Allow2Ban # Unless the IP is whitelisted, return true so that Allow2Ban
# increments the counter (stored in Rails.cache) for the IP # increments the counter (stored in Rails.cache) for the IP
if config.ip_whitelist.include?(@request.ip) if config.ip_whitelist.include?(@request.ip)
...@@ -103,7 +113,14 @@ module Grack ...@@ -103,7 +113,14 @@ module Grack
end end
end end
nil # No user was found if banned
Rails.logger.info "IP #{@request.ip} failed to login " \
"as #{login} but has been temporarily banned from Git auth"
end
end
end
user
end end
def authorized_request? def authorized_request?
......
# 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
...@@ -85,6 +85,17 @@ describe Grack::Auth do ...@@ -85,6 +85,17 @@ describe Grack::Auth do
it "responds with status 401" do it "responds with status 401" do
expect(status).to eq(401) expect(status).to eq(401)
end 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 end
context "when authentication succeeds" do context "when authentication succeeds" do
...@@ -109,10 +120,49 @@ describe Grack::Auth do ...@@ -109,10 +120,49 @@ describe Grack::Auth do
end end
context "when the user isn't blocked" do context "when the user isn't blocked" do
before do
expect(Rack::Attack::Allow2Ban).to receive(:reset)
end
it "responds with status 200" do it "responds with status 200" do
expect(status).to eq(200) expect(status).to eq(200)
end end
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 end
context "when the user doesn't have access to the project" do context "when the user doesn't have access to the project" do
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment