From 7512016d51feb6c02c3a0322325564b6b7f5ad9c Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 15 Dec 2014 15:59:16 +0100 Subject: [PATCH 1/7] Update rack-attack to 4.2.0 If we are going to monkey-patch something it might as well be the latest version. --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 4bcb1eb0de5..045b2b60fed 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -356,7 +356,7 @@ GEM rack (1.5.2) rack-accept (0.4.5) rack (>= 0.4) - rack-attack (2.3.0) + rack-attack (4.2.0) rack rack-cors (0.2.9) rack-mini-profiler (0.9.0) From 62ea02740d2fff83d636eb659eb5f80dbf1bd888 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 15 Dec 2014 18:47:26 +0100 Subject: [PATCH 2/7] Block Git HTTP Basic Auth after 10 failed attempts --- CHANGELOG | 3 +++ config/gitlab.yml.example | 11 ++++++++++ config/initializers/1_settings.rb | 9 ++++++++ .../rack_attack_git_basic_auth.rb | 10 +++++++++ config/initializers/redis-store-fix-expiry.rb | 21 +++++++++++++++++++ lib/gitlab/backend/grack_auth.rb | 14 +++++++++++-- 6 files changed, 66 insertions(+), 2 deletions(-) create mode 100644 config/initializers/rack_attack_git_basic_auth.rb create mode 100644 config/initializers/redis-store-fix-expiry.rb diff --git a/CHANGELOG b/CHANGELOG index 2061237fb42..e4d180359b7 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,6 @@ +v 7.7.0 + - Block Git HTTP access after 10 failed authentication attempts + v 7.6.0 - Fork repository to groups - New rugged version diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 7b4c180fccc..b474063505f 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -298,6 +298,17 @@ production: &base # ![Company Logo](http://www.companydomain.com/logo.png) # [Learn more about CompanyName](http://www.companydomain.com/) + rack_attack: + git_basic_auth: + # Limit the number of Git HTTP authentication attempts per IP + # maxretry: 10 + # + # Reset the auth attempt counter per IP after 60 seconds + # findtime: 60 + # + # Ban an IP for one hour (3600s) after too many auth attempts + # bantime: 3600 + development: <<: *base diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 27bb83784ba..4464d9d0001 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -171,6 +171,15 @@ Settings.satellites['timeout'] ||= 30 # Settings['extra'] ||= Settingslogic.new({}) +# +# Rack::Attack settings +# +Settings['rack_attack'] ||= Settingslogic.new({}) +Settings.rack_attack['git_basic_auth'] ||= Settingslogic.new({}) +Settings.rack_attack.git_basic_auth['maxretry'] ||= 10 +Settings.rack_attack.git_basic_auth['findtime'] ||= 1.minute +Settings.rack_attack.git_basic_auth['bantime'] ||= 1.hour + # # Testing settings # diff --git a/config/initializers/rack_attack_git_basic_auth.rb b/config/initializers/rack_attack_git_basic_auth.rb new file mode 100644 index 00000000000..2348768ff16 --- /dev/null +++ b/config/initializers/rack_attack_git_basic_auth.rb @@ -0,0 +1,10 @@ +unless Rails.env.test? + Rack::Attack.blacklist('Git HTTP Basic Auth') do |req| + Rack::Attack::Allow2Ban.filter(req.ip, Gitlab.config.rack_attack.git_basic_auth) do + # This block only gets run if the IP was not already banned. + # Return false, meaning that we do not see anything wrong with the + # request at this time + false + end + end +end diff --git a/config/initializers/redis-store-fix-expiry.rb b/config/initializers/redis-store-fix-expiry.rb new file mode 100644 index 00000000000..dd27596cd0b --- /dev/null +++ b/config/initializers/redis-store-fix-expiry.rb @@ -0,0 +1,21 @@ +# Monkey-patch Redis::Store to make 'setex' and 'expire' work with namespacing + +module Gitlab + class Redis + class Store + module Namespace + def setex(key, expires_in, value, options=nil) + namespace(key) { |key| super(key, expires_in, value) } + end + + def expire(key, expires_in) + namespace(key) { |key| super(key, expires_in) } + end + end + end + end +end + +Redis::Store.class_eval do + include Gitlab::Redis::Store::Namespace +end diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb index 762639414e0..ab5d2ef3da4 100644 --- a/lib/gitlab/backend/grack_auth.rb +++ b/lib/gitlab/backend/grack_auth.rb @@ -72,8 +72,18 @@ module Grack end def authenticate_user(login, password) - auth = Gitlab::Auth.new - auth.find(login, password) + user = Gitlab::Auth.new.find(login, password) + 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 + Rack::Attack::Allow2Ban.filter(@request.ip, Gitlab.config.rack_attack.git_basic_auth) do + # Return true, so that Allow2Ban increments the counter (stored in + # Rails.cache) for the IP + true + end + + nil # No user was found end def authorized_request? From 764eaedf810af307d68d5e6b552988db1cb15f54 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 16 Dec 2014 12:38:44 +0100 Subject: [PATCH 3/7] Improve Redis::Store monkey-patch robustness --- config/initializers/redis-store-fix-expiry.rb | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/config/initializers/redis-store-fix-expiry.rb b/config/initializers/redis-store-fix-expiry.rb index dd27596cd0b..813d4c76c81 100644 --- a/config/initializers/redis-store-fix-expiry.rb +++ b/config/initializers/redis-store-fix-expiry.rb @@ -4,13 +4,36 @@ module Gitlab class Redis class Store module Namespace + # Redis::Store#expire in redis-store 1.1.4 does not respect namespaces; + # this new method does. def setex(key, expires_in, value, options=nil) namespace(key) { |key| super(key, expires_in, value) } end + # Redis::Store#expire in redis-store 1.1.4 does not respect namespaces; + # this new method does. def expire(key, expires_in) namespace(key) { |key| super(key, expires_in) } end + + private + + # Our new definitions of #setex and #expire above assume that the + # #namespace method exists. Because we cannot be sure of that, we + # re-implement the #namespace method from Redis::Store::Namespace so + # that it all Redis::Store instances, whether they use namespacing or + # not. + # + # Based on lib/redis/store/namespace.rb L49-51 (redis-store 1.1.4) + def namespace(key) + if @namespace + yield interpolate(key) + else + # This Redis::Store instance does not use a namespace so we should + # just pass through the key. + yield key + end + end end end end From 49f4fe8c6ea776825461a1d18da27a198fb95b55 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 16 Dec 2014 12:43:38 +0100 Subject: [PATCH 4/7] Fix copy-paste error in comment --- config/initializers/redis-store-fix-expiry.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/redis-store-fix-expiry.rb b/config/initializers/redis-store-fix-expiry.rb index 813d4c76c81..e3139098018 100644 --- a/config/initializers/redis-store-fix-expiry.rb +++ b/config/initializers/redis-store-fix-expiry.rb @@ -4,7 +4,7 @@ module Gitlab class Redis class Store module Namespace - # Redis::Store#expire in redis-store 1.1.4 does not respect namespaces; + # Redis::Store#setex in redis-store 1.1.4 does not respect namespaces; # this new method does. def setex(key, expires_in, value, options=nil) namespace(key) { |key| super(key, expires_in, value) } From 4a389e761635ad17a707d3caa8ec5bf09b849f2f Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 16 Dec 2014 12:46:55 +0100 Subject: [PATCH 5/7] Another comment fix --- config/initializers/redis-store-fix-expiry.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/initializers/redis-store-fix-expiry.rb b/config/initializers/redis-store-fix-expiry.rb index e3139098018..fce0a135330 100644 --- a/config/initializers/redis-store-fix-expiry.rb +++ b/config/initializers/redis-store-fix-expiry.rb @@ -21,8 +21,8 @@ module Gitlab # Our new definitions of #setex and #expire above assume that the # #namespace method exists. Because we cannot be sure of that, we # re-implement the #namespace method from Redis::Store::Namespace so - # that it all Redis::Store instances, whether they use namespacing or - # not. + # that it is available for all Redis::Store instances, whether they use + # namespacing or not. # # Based on lib/redis/store/namespace.rb L49-51 (redis-store 1.1.4) def namespace(key) From c8b2def2be44771ffb479ad989acc7eccf4012f8 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 18 Dec 2014 11:08:11 +0100 Subject: [PATCH 6/7] Add more comments explaining how we block IPs --- config/initializers/rack_attack_git_basic_auth.rb | 2 ++ lib/gitlab/backend/grack_auth.rb | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/config/initializers/rack_attack_git_basic_auth.rb b/config/initializers/rack_attack_git_basic_auth.rb index 2348768ff16..bbbfed68329 100644 --- a/config/initializers/rack_attack_git_basic_auth.rb +++ b/config/initializers/rack_attack_git_basic_auth.rb @@ -1,4 +1,6 @@ unless Rails.env.test? + # Tell the Rack::Attack Rack middleware to maintain an IP blacklist. We will + # update the blacklist from Grack::Auth#authenticate_user. Rack::Attack.blacklist('Git HTTP Basic Auth') do |req| Rack::Attack::Allow2Ban.filter(req.ip, Gitlab.config.rack_attack.git_basic_auth) do # This block only gets run if the IP was not already banned. diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb index ab5d2ef3da4..7bc745bf97e 100644 --- a/lib/gitlab/backend/grack_auth.rb +++ b/lib/gitlab/backend/grack_auth.rb @@ -76,7 +76,10 @@ module Grack 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 + # 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. Rack::Attack::Allow2Ban.filter(@request.ip, Gitlab.config.rack_attack.git_basic_auth) do # Return true, so that Allow2Ban increments the counter (stored in # Rails.cache) for the IP From af56c1dd323ee418eb8dbfa9eb35c7ec9ac58a66 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 6 Jan 2015 16:56:56 +0100 Subject: [PATCH 7/7] White-list requests from 127.0.0.1 On some misconfigured GitLab servers, if you look in production.log it looks like all requests come from 127.0.0.1. To avoid unwanted banning we white-list 127.0.0.1 with this commit. --- config/gitlab.yml.example | 3 +++ config/initializers/1_settings.rb | 1 + lib/gitlab/backend/grack_auth.rb | 13 +++++++++---- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index b474063505f..5d801b9ae5b 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -300,6 +300,9 @@ production: &base rack_attack: git_basic_auth: + # Whitelist requests from 127.0.0.1 for web proxies (NGINX/Apache) with incorrect headers + # ip_whitelist: ["127.0.0.1"] + # # Limit the number of Git HTTP authentication attempts per IP # maxretry: 10 # diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 4464d9d0001..c744577d516 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -176,6 +176,7 @@ Settings['extra'] ||= Settingslogic.new({}) # Settings['rack_attack'] ||= Settingslogic.new({}) Settings.rack_attack['git_basic_auth'] ||= Settingslogic.new({}) +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 Settings.rack_attack.git_basic_auth['bantime'] ||= 1.hour diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb index 7bc745bf97e..1f71906bc8e 100644 --- a/lib/gitlab/backend/grack_auth.rb +++ b/lib/gitlab/backend/grack_auth.rb @@ -80,10 +80,15 @@ module Grack # 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. - Rack::Attack::Allow2Ban.filter(@request.ip, Gitlab.config.rack_attack.git_basic_auth) do - # Return true, so that Allow2Ban increments the counter (stored in - # Rails.cache) for the IP - true + 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 + else + true + end end nil # No user was found