Merge branch '31177-upgrade-rack-attack' into 'master'

Upgrade Rack Attack to 6.2.0 See merge request !18977

Merge branch '31177-upgrade-rack-attack' into 'master'
c7d06b48 · Stan Hu · 5d2d1d79 · 497239d4 · c7d06b48 · c7d06b48
Commit c7d06b48 authored 5 years ago by Stan Hu
--- a/Gemfile
+++ b/Gemfile
@@ -259,9 +259,6 @@ gem 'loofah', '~> 2.2'
 # Working with license
 gem 'licensee', '~> 8.9'

-# Protect against bruteforcing
-gem 'rack-attack', '~> 4.4.1'
-
 # Ace editor
 gem 'ace-rails-ap', '~> 4.1.0'

@@ -293,6 +290,9 @@ gem 'base32', '~> 0.3.0'

 gem "gitlab-license", "~> 1.0"

+# Protect against bruteforcing
+gem 'rack-attack', '~> 6.2.0'
+
 # Sentry integration
 gem 'sentry-raven', '~> 2.9'


--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -734,8 +734,8 @@ GEM
    rack (2.0.7)
    rack-accept (0.4.5)
      rack (>= 0.4)
-    rack-attack (4.4.1)
-      rack
+    rack-attack (6.2.0)
+      rack (>= 1.0, < 3)
    rack-cors (1.0.2)
    rack-oauth2 (1.9.3)
      activesupport
@@ -1256,7 +1256,7 @@ DEPENDENCIES
  puma (~> 3.12)
  puma_worker_killer
  rack (~> 2.0.7)
-  rack-attack (~> 4.4.1)
+  rack-attack (~> 6.2.0)
  rack-cors (~> 1.0.0)
  rack-oauth2 (~> 1.9.3)
  rack-proxy (~> 0.6.0)

--- a/config/initializers/rack_attack_git_basic_auth.rb
+++ b/config/initializers/rack_attack_git_basic_auth.rb
-rack_attack_enabled = Gitlab.config.rack_attack.git_basic_auth['enabled']
+# 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|
+  next false unless Gitlab.config.rack_attack.git_basic_auth.enabled

-unless Rails.env.test? || !rack_attack_enabled
-  # 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.
-      # Return false, meaning that we do not see anything wrong with the
-      # request at this time
-      false
-    end
+  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
--- a/config/initializers/rack_attack_logging.rb
+++ b/config/initializers/rack_attack_logging.rb
@@ -2,8 +2,10 @@
 #
 # Adds logging for all Rack Attack blocks and throttling events.

-ActiveSupport::Notifications.subscribe('rack.attack') do |name, start, finish, request_id, req|
-  if [:throttle, :blacklist].include? req.env['rack.attack.match_type']
+ActiveSupport::Notifications.subscribe(/rack_attack/) do |name, start, finish, request_id, payload|
+  req = payload[:request]
+
+  if [:throttle, :blocklist].include? req.env['rack.attack.match_type']
    rack_attack_info = {
      message: 'Rack_Attack',
      env: req.env['rack.attack.match_type'],

--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -450,16 +450,22 @@
          context "when authentication fails" do
            context "when the user is IP banned" do
              before do
-                Gitlab.config.rack_attack.git_basic_auth['enabled'] = true
+                stub_rack_attack_setting(enabled: true)
              end

-              it "responds with status 401" do
+              it "responds with status 403" do
                expect(Rack::Attack::Allow2Ban).to receive(:filter).and_return(true)
-                allow_any_instance_of(ActionDispatch::Request).to receive(:ip).and_return('1.2.3.4')
+                expect(Gitlab::AuthLogger).to receive(:error).with({
+                  message: 'Rack_Attack',
+                  env: :blocklist,
+                  remote_ip: '127.0.0.1',
+                  request_method: 'GET',
+                  path: "/#{path}/info/refs?service=git-upload-pack"
+                })

                clone_get(path, env)

-                expect(response).to have_gitlab_http_status(:unauthorized)
+                expect(response).to have_gitlab_http_status(:forbidden)
              end
            end
          end
@@ -493,7 +499,7 @@

              context "when the user isn't blocked" do
                before do
-                  Gitlab.config.rack_attack.git_basic_auth['enabled'] = true
+                  stub_rack_attack_setting(enabled: true, bantime: 1.minute, findtime: 5.minutes, maxretry: 2, ip_whitelist: [])
                end

                it "resets the IP in Rack Attack on download" do
@@ -652,9 +658,11 @@ def attempt_login(include_password)
                  response.status
                end

+                include_context 'rack attack cache store'
+
                it "repeated attempts followed by successful attempt" do
                  options = Gitlab.config.rack_attack.git_basic_auth
-                  maxretry = options[:maxretry] - 1
+                  maxretry = options[:maxretry]
                  ip = '1.2.3.4'

                  allow_any_instance_of(ActionDispatch::Request).to receive(:ip).and_return(ip)
@@ -666,12 +674,6 @@ def attempt_login(include_password)

                  expect(attempt_login(true)).to eq(200)
                  expect(Rack::Attack::Allow2Ban.banned?(ip)).to be_falsey
-
-                  maxretry.times.each do
-                    expect(attempt_login(false)).to eq(401)
-                  end
-
-                  Rack::Attack::Allow2Ban.reset(ip, options)
                end
              end