Merge branch '35617-only-blacklist-git-auth' into 'master'

Only check for blacklisted IPs on Git requests

See merge request gitlab-org/gitlab!20828

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

@@ -867,5 +867,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,