Commit fc4af9b1 authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

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
parents 533b5721 56d87db3
......@@ -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
......
......@@ -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"]
#
......
......@@ -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
......
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?
......
# 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
......@@ -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
......
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