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

2 files changed, 62 insertions, 14 deletions

diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb
index ee877e099b1..ffe4565ef1e 100644
--- a/lib/gitlab/backend/grack_auth.rb
+++ b/lib/gitlab/backend/grack_auth.rb
@@ -1,3 +1,4 @@
+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?
diff --git a/lib/gitlab/backend/rack_attack_helpers.rb b/lib/gitlab/backend/rack_attack_helpers.rb
new file mode 100644
index 00000000000..8538f3f6eca
--- /dev/null
+++ b/lib/gitlab/backend/rack_attack_helpers.rb
@@ -0,0 +1,31 @@
+# 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