Only check for blacklisted IPs on Git requests

We block IP addresses when failed Git auths reach a certain
threshold.

This block applied to all requests so for every request, we
had to check Redis to see if the IP is blocked. This caused
unnecessary Redis load.

This MR changes the block to only apply to Git requests as
it is described in the documentation

app/controllers/application_controller.rb

@@ -74,6 +74,18 @@ class ApplicationController < ActionController::Base
     render_403
   end
 
+  rescue_from Gitlab::Auth::IpBlacklisted do
+    Gitlab::AuthLogger.error(
+      message: 'Rack_Attack',
+      env: :blocklist,
+      remote_ip: request.ip,
+      request_method: request.request_method,
+      path: request.fullpath
+    )
+
+    head :forbidden
+  end
+
   rescue_from Gitlab::Auth::TooManyIps do |e|
     head :forbidden, retry_after: Gitlab::Auth::UniqueIpsLimiter.config.unique_ips_limit_time_window
   end

changelogs/unreleased/35617-only-blacklist-git-auth.yml

+---
+title: Only blacklist IPs from Git requests
+merge_request: 20828
+author:
+type: changed

config/initializers/rack_attack_git_basic_auth.rb

-# Tell the Rack::Attack Rack middleware to maintain an IP blacklist.
-# We update the blacklist in Gitlab::Auth::IpRateLimiter.
-Rack::Attack.blocklist('Git HTTP Basic Auth') do |req|
-  rate_limiter = Gitlab::Auth::IpRateLimiter.new(req.ip)
-
-  next false if !rate_limiter.enabled? || rate_limiter.trusted_ip?
-
-  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

lib/gitlab/auth.rb

@@ -3,6 +3,7 @@
 module Gitlab
   module Auth
     MissingPersonalAccessTokenError = Class.new(StandardError)
+    IpBlacklisted = Class.new(StandardError)
 
     # Scopes used for GitLab API access
     API_SCOPES = [:api, :read_user].freeze
@@ -35,6 +36,10 @@ module Gitlab
       def find_for_git_client(login, password, project:, ip:)
         raise "Must provide an IP for rate limiting" if ip.nil?
 
+        rate_limiter = Gitlab::Auth::IpRateLimiter.new(ip)
+
+        raise IpBlacklisted if !skip_rate_limit?(login: login) && rate_limiter.banned?
+
         # `user_with_password_for_git` should be the last check
         # because it's the most expensive, especially when LDAP
         # is enabled.
@@ -48,7 +53,7 @@ module Gitlab
           user_with_password_for_git(login, password) ||
           Gitlab::Auth::Result.new
 
-        rate_limit!(ip, success: result.success?, login: login) unless skip_rate_limit?(login: login)
+        rate_limit!(rate_limiter, success: result.success?, login: login)
         Gitlab::Auth::UniqueIpsLimiter.limit_user!(result.actor)
 
         return result if result.success? || authenticate_using_internal_or_ldap_password?
@@ -96,10 +101,11 @@ module Gitlab
         end
       end
 
+      private
+
       # rubocop:disable Gitlab/RailsLogger
-      def rate_limit!(ip, success:, login:)
-        rate_limiter = Gitlab::Auth::IpRateLimiter.new(ip)
-        return unless rate_limiter.enabled?
+      def rate_limit!(rate_limiter, success:, login:)
+        return if skip_rate_limit?(login: login)
 
         if success
           # Repeated login 'failures' are normal behavior for some Git clients so
@@ -109,18 +115,16 @@ module Gitlab
         else
           # Register a login failure so that Rack::Attack can block the next
           # request from this IP if needed.
-          rate_limiter.register_fail!
-
-          if rate_limiter.banned?
-            Rails.logger.info "IP #{ip} failed to login " \
+          # This returns true when the failures are over the threshold and the IP
+          # is banned.
+          if rate_limiter.register_fail!
+            Rails.logger.info "IP #{rate_limiter.ip} failed to login " \
               "as #{login} but has been temporarily banned from Git auth"
           end
         end
       end
       # rubocop:enable Gitlab/RailsLogger
 
-      private
-
       def skip_rate_limit?(login:)
         ::Ci::Build::CI_REGISTRY_USER == login
       end

lib/gitlab/auth/ip_rate_limiter.rb

@@ -9,41 +9,48 @@ module Gitlab
 
       def initialize(ip)
         @ip = ip
-        @banned = false
-      end
-
-      def enabled?
-        config.enabled
       end
 
       def reset!
+        return if skip_rate_limit?
+
         Rack::Attack::Allow2Ban.reset(ip, config)
       end
 
       def register_fail!
-        return false if trusted_ip?
+        return false if skip_rate_limit?
 
         # Allow2Ban.filter will return false if this IP has not failed too often yet
-        @banned = Rack::Attack::Allow2Ban.filter(ip, config) do
+        Rack::Attack::Allow2Ban.filter(ip, config) do
           # We return true to increment the count for this IP
           true
         end
       end
 
       def banned?
-        @banned
-      end
+        return false if skip_rate_limit?
 
-      def trusted_ip?
-        trusted_ips.any? { |netmask| netmask.include?(ip) }
+        Rack::Attack::Allow2Ban.banned?(ip)
       end
 
       private
 
+      def skip_rate_limit?
+        !enabled? || trusted_ip?
+      end
+
+      def enabled?
+        config.enabled
+      end
+
       def config
         Gitlab.config.rack_attack.git_basic_auth
       end
 
+      def trusted_ip?
+        trusted_ips.any? { |netmask| netmask.include?(ip) }
+      end
+
       def trusted_ips
         strong_memoize(:trusted_ips) do
           config.ip_whitelist.map do |proxy|

spec/controllers/application_controller_spec.rb

@@ -869,5 +869,31 @@ describe ApplicationController do
 
       it { is_expected.not_to redirect_to users_sign_up_welcome_path }
     end
+
+    describe 'rescue_from Gitlab::Auth::IpBlacklisted' do
+      controller(described_class) do
+        skip_before_action :authenticate_user!
+
+        def index
+          raise Gitlab::Auth::IpBlacklisted
+        end
+      end
+
+      it 'returns a 403 and logs the request' do
+        expect(Gitlab::AuthLogger).to receive(:error).with({
+          message: 'Rack_Attack',
+          env: :blocklist,
+          remote_ip: '1.2.3.4',
+          request_method: 'GET',
+          path: '/anonymous'
+        })
+
+        request.remote_addr = '1.2.3.4'
+
+        get :index
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
   end
 end

spec/lib/gitlab/auth/ip_rate_limiter_spec.rb

@@ -62,4 +62,36 @@ describe Gitlab::Auth::IpRateLimiter, :use_clean_rails_memory_store_caching do
       it_behaves_like 'whitelisted IPs'
     end
   end
+
+  shared_examples 'skips the rate limiter' do
+    it 'does not call Rack::Attack::Allow2Ban.reset!' do
+      expect(Rack::Attack::Allow2Ban).not_to receive(:reset!)
+
+      subject.reset!
+    end
+
+    it 'does not call Rack::Attack::Allow2Ban.banned?' do
+      expect(Rack::Attack::Allow2Ban).not_to receive(:banned?)
+
+      subject.banned?
+    end
+
+    it 'does not call Rack::Attack::Allow2Ban.filter' do
+      expect(Rack::Attack::Allow2Ban).not_to receive(:filter)
+
+      subject.register_fail!
+    end
+  end
+
+  context 'when IP is whitlisted' do
+    let(:ip) { '127.0.0.1' }
+
+    it_behaves_like 'skips the rate limiter'
+  end
+
+  context 'when rate limiter is disabled' do
+    let(:options) { { enabled: false } }
+
+    it_behaves_like 'skips the rate limiter'
+  end
 end

spec/requests/git_http_spec.rb

@@ -456,7 +456,7 @@ describe 'Git HTTP requests' do
               end
 
               it "responds with status 403" do
-                expect(Rack::Attack::Allow2Ban).to receive(:filter).and_return(true)
+                expect(Rack::Attack::Allow2Ban).to receive(:banned?).and_return(true)
                 expect(Gitlab::AuthLogger).to receive(:error).with({
                   message: 'Rack_Attack',
                   env: :blocklist,