Merge branch 'security-342481-deploy-token-support-rack-attack-14-10' into '14-10-stable-ee'

Allow rate limiting of deploy tokens

See merge request gitlab-org/security/gitlab!2395

lib/gitlab/auth/request_authenticator.rb

@@ -13,6 +13,10 @@ module Gitlab
         @request = request
       end
 
+      def find_authenticated_requester(request_formats)
+        user(request_formats) || deploy_token_from_request
+      end
+
       def user(request_formats)
         request_formats.each do |format|
           user = find_sessionless_user(format)
@@ -84,7 +88,8 @@ module Gitlab
       def route_authentication_setting
         @route_authentication_setting ||= {
           job_token_allowed: api_request?,
-          basic_auth_personal_access_token: api_request? || git_request?
+          basic_auth_personal_access_token: api_request? || git_request?,
+          deploy_token_allowed: api_request? || git_request?
         }
       end
 

lib/gitlab/metrics/subscribers/rack_attack.rb

@@ -73,12 +73,16 @@ module Gitlab
             matched: req.env['rack.attack.matched']
           }
 
-          if THROTTLES_WITH_USER_INFORMATION.include? req.env['rack.attack.matched'].to_sym
-            user_id = req.env['rack.attack.match_discriminator']
-            user = User.find_by(id: user_id) # rubocop:disable CodeReuse/ActiveRecord
+          discriminator = req.env['rack.attack.match_discriminator'].to_s
+          discriminator_id = discriminator.split(':').last
 
-            rack_attack_info[:user_id] = user_id
+          if discriminator.starts_with?('user:')
+            user = User.find_by(id: discriminator_id) # rubocop:disable CodeReuse/ActiveRecord
+
+            rack_attack_info[:user_id] = discriminator_id.to_i
             rack_attack_info['meta.user'] = user.username unless user.nil?
+          elsif discriminator.starts_with?('deploy_token:')
+            rack_attack_info[:deploy_token_id] = discriminator_id.to_i
           end
 
           Gitlab::InstrumentationHelper.add_instrumentation_data(rack_attack_info)

lib/gitlab/rack_attack.rb

@@ -95,7 +95,7 @@ module Gitlab
         authenticated_options = Gitlab::Throttle.options(throttle, authenticated: true)
         throttle_or_track(rack_attack, "throttle_authenticated_#{throttle}", authenticated_options) do |req|
           if req.throttle?(throttle, authenticated: true)
-            req.throttled_user_id([:api])
+            req.throttled_identifer([:api])
           end
         end
       end
@@ -117,7 +117,7 @@ module Gitlab
 
       throttle_or_track(rack_attack, 'throttle_authenticated_web', Gitlab::Throttle.authenticated_web_options) do |req|
         if req.throttle_authenticated_web?
-          req.throttled_user_id([:api, :rss, :ics])
+          req.throttled_identifer([:api, :rss, :ics])
         end
       end
 
@@ -129,19 +129,19 @@ module Gitlab
 
       throttle_or_track(rack_attack, 'throttle_authenticated_protected_paths_api', Gitlab::Throttle.protected_paths_options) do |req|
         if req.throttle_authenticated_protected_paths_api?
-          req.throttled_user_id([:api])
+          req.throttled_identifer([:api])
         end
       end
 
       throttle_or_track(rack_attack, 'throttle_authenticated_protected_paths_web', Gitlab::Throttle.protected_paths_options) do |req|
         if req.throttle_authenticated_protected_paths_web?
-          req.throttled_user_id([:api, :rss, :ics])
+          req.throttled_identifer([:api, :rss, :ics])
         end
       end
 
       throttle_or_track(rack_attack, 'throttle_authenticated_git_lfs', Gitlab::Throttle.throttle_authenticated_git_lfs_options) do |req|
         if req.throttle_authenticated_git_lfs?
-          req.throttled_user_id([:api])
+          req.throttled_identifer([:api])
         end
       end
 

lib/gitlab/rack_attack/request.rb

@@ -9,18 +9,22 @@ module Gitlab
       GROUP_PATH_REGEX = %r{^/api/v\d+/groups/[^/]+/?$}.freeze
 
       def unauthenticated?
-        !(authenticated_user_id([:api, :rss, :ics]) || authenticated_runner_id)
+        !(authenticated_identifier([:api, :rss, :ics]) || authenticated_runner_id)
       end
 
-      def throttled_user_id(request_formats)
-        user_id = authenticated_user_id(request_formats)
+      def throttled_identifer(request_formats)
+        identifier = authenticated_identifier(request_formats)
+        return unless identifier
 
-        if Gitlab::RackAttack.user_allowlist.include?(user_id)
+        identifier_type = identifier[:identifier_type]
+        identifier_id = identifier[:identifier_id]
+
+        if identifier_type == :user && Gitlab::RackAttack.user_allowlist.include?(identifier_id)
           Gitlab::Instrumentation::Throttle.safelist = 'throttle_user_allowlist'
           return
         end
 
-        user_id
+        "#{identifier_type}:#{identifier_id}"
       end
 
       def authenticated_runner_id
@@ -169,8 +173,18 @@ module Gitlab
 
       private
 
-      def authenticated_user_id(request_formats)
-        request_authenticator.user(request_formats)&.id
+      def authenticated_identifier(request_formats)
+        requester = request_authenticator.find_authenticated_requester(request_formats)
+
+        return unless requester
+
+        identifier_type = if requester.is_a?(DeployToken)
+                            :deploy_token
+                          else
+                            :user
+                          end
+
+        { identifier_type: identifier_type, identifier_id: requester.id }
       end
 
       def request_authenticator

spec/lib/gitlab/auth/request_authenticator_spec.rb

@@ -76,6 +76,38 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do
     end
   end
 
+  describe '#find_authenticated_requester' do
+    let_it_be(:api_user) { create(:user) }
+    let_it_be(:deploy_token_user) { create(:user) }
+
+    it 'returns the deploy token if it exists' do
+      allow_next_instance_of(described_class) do |authenticator|
+        expect(authenticator).to receive(:deploy_token_from_request).and_return(deploy_token_user)
+        allow(authenticator).to receive(:user).and_return(nil)
+      end
+
+      expect(subject.find_authenticated_requester([:api])).to eq deploy_token_user
+    end
+
+    it 'returns the user id if it exists' do
+      allow_next_instance_of(described_class) do |authenticator|
+        allow(authenticator).to receive(:deploy_token_from_request).and_return(deploy_token_user)
+        expect(authenticator).to receive(:user).and_return(api_user)
+      end
+
+      expect(subject.find_authenticated_requester([:api])).to eq api_user
+    end
+
+    it 'rerturns nil if no match is found' do
+      allow_next_instance_of(described_class) do |authenticator|
+        expect(authenticator).to receive(:deploy_token_from_request).and_return(nil)
+        expect(authenticator).to receive(:user).and_return(nil)
+      end
+
+      expect(subject.find_authenticated_requester([:api])).to eq nil
+    end
+  end
+
   describe '#find_sessionless_user' do
     let_it_be(:dependency_proxy_user) { build(:user) }
     let_it_be(:access_token_user) { build(:user) }
@@ -380,10 +412,10 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do
   describe '#route_authentication_setting' do
     using RSpec::Parameterized::TableSyntax
 
-    where(:script_name, :expected_job_token_allowed, :expected_basic_auth_personal_access_token) do
-      '/api/endpoint'          | true  | true
-      '/namespace/project.git' | false | true
-      '/web/endpoint'          | false | false
+    where(:script_name, :expected_job_token_allowed, :expected_basic_auth_personal_access_token, :expected_deploy_token_allowed) do
+      '/api/endpoint'          | true  | true  | true
+      '/namespace/project.git' | false | true  | true
+      '/web/endpoint'          | false | false | false
     end
 
     with_them do
@@ -394,7 +426,8 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do
       it 'returns correct settings' do
         expect(subject.send(:route_authentication_setting)).to eql({
           job_token_allowed: expected_job_token_allowed,
-          basic_auth_personal_access_token: expected_basic_auth_personal_access_token
+          basic_auth_personal_access_token: expected_basic_auth_personal_access_token,
+          deploy_token_allowed: expected_deploy_token_allowed
         })
       end
     end

spec/lib/gitlab/metrics/subscribers/rack_attack_spec.rb

@@ -91,72 +91,110 @@ RSpec.describe Gitlab::Metrics::Subscribers::RackAttack, :request_store do
       end
     end
 
-    context 'when matched throttle requires user information' do
-      context 'when user not found' do
-        let(:event) do
-          ActiveSupport::Notifications::Event.new(
-            event_name, Time.current, Time.current + 2.seconds, '1', request: double(
-              :request,
-              ip: '1.2.3.4',
-              request_method: 'GET',
-              fullpath: '/api/v4/internal/authorized_keys',
-              env: {
-                'rack.attack.match_type' => match_type,
-                'rack.attack.matched' => 'throttle_authenticated_api',
-                'rack.attack.match_discriminator' => 'not_exist_user_id'
-              }
+    context 'matching user or deploy token authenticated information' do
+      context 'when matching for user' do
+        context 'when user not found' do
+          let(:event) do
+            ActiveSupport::Notifications::Event.new(
+              event_name, Time.current, Time.current + 2.seconds, '1', request: double(
+                :request,
+                ip: '1.2.3.4',
+                request_method: 'GET',
+                fullpath: '/api/v4/internal/authorized_keys',
+                env: {
+                  'rack.attack.match_type' => match_type,
+                  'rack.attack.matched' => 'throttle_authenticated_api',
+                  'rack.attack.match_discriminator' => "user:#{non_existing_record_id}"
+                }
+              )
             )
-          )
+          end
+
+          it 'logs request information and user id' do
+            expect(Gitlab::AuthLogger).to receive(:error).with(
+              include(
+                message: 'Rack_Attack',
+                env: match_type,
+                remote_ip: '1.2.3.4',
+                request_method: 'GET',
+                path: '/api/v4/internal/authorized_keys',
+                matched: 'throttle_authenticated_api',
+                user_id: non_existing_record_id
+              )
+            )
+            subscriber.send(match_type, event)
+          end
         end
 
-        it 'logs request information and user id' do
-          expect(Gitlab::AuthLogger).to receive(:error).with(
-            include(
-              message: 'Rack_Attack',
-              env: match_type,
-              remote_ip: '1.2.3.4',
-              request_method: 'GET',
-              path: '/api/v4/internal/authorized_keys',
-              matched: 'throttle_authenticated_api',
-              user_id: 'not_exist_user_id'
+        context 'when user found' do
+          let(:user) { create(:user) }
+          let(:event) do
+            ActiveSupport::Notifications::Event.new(
+              event_name, Time.current, Time.current + 2.seconds, '1', request: double(
+                :request,
+                ip: '1.2.3.4',
+                request_method: 'GET',
+                fullpath: '/api/v4/internal/authorized_keys',
+                env: {
+                  'rack.attack.match_type' => match_type,
+                  'rack.attack.matched' => 'throttle_authenticated_api',
+                  'rack.attack.match_discriminator' => "user:#{user.id}"
+                }
+              )
             )
-          )
-          subscriber.send(match_type, event)
+          end
+
+          it 'logs request information and user meta' do
+            expect(Gitlab::AuthLogger).to receive(:error).with(
+              include(
+                message: 'Rack_Attack',
+                env: match_type,
+                remote_ip: '1.2.3.4',
+                request_method: 'GET',
+                path: '/api/v4/internal/authorized_keys',
+                matched: 'throttle_authenticated_api',
+                user_id: user.id,
+                'meta.user' => user.username
+              )
+            )
+            subscriber.send(match_type, event)
+          end
         end
       end
 
-      context 'when user found' do
-        let(:user) { create(:user) }
-        let(:event) do
-          ActiveSupport::Notifications::Event.new(
-            event_name, Time.current, Time.current + 2.seconds, '1', request: double(
-              :request,
-              ip: '1.2.3.4',
-              request_method: 'GET',
-              fullpath: '/api/v4/internal/authorized_keys',
-              env: {
-                'rack.attack.match_type' => match_type,
-                'rack.attack.matched' => 'throttle_authenticated_api',
-                'rack.attack.match_discriminator' => user.id
-              }
+      context 'when matching for deploy token' do
+        context 'when deploy token found' do
+          let(:deploy_token) { create(:deploy_token) }
+          let(:event) do
+            ActiveSupport::Notifications::Event.new(
+              event_name, Time.current, Time.current + 2.seconds, '1', request: double(
+                :request,
+                ip: '1.2.3.4',
+                request_method: 'GET',
+                fullpath: '/api/v4/internal/authorized_keys',
+                env: {
+                  'rack.attack.match_type' => match_type,
+                  'rack.attack.matched' => 'throttle_authenticated_api',
+                  'rack.attack.match_discriminator' => "deploy_token:#{deploy_token.id}"
+                }
+              )
             )
-          )
-        end
-
-        it 'logs request information and user meta' do
-          expect(Gitlab::AuthLogger).to receive(:error).with(
-            include(
-              message: 'Rack_Attack',
-              env: match_type,
-              remote_ip: '1.2.3.4',
-              request_method: 'GET',
-              path: '/api/v4/internal/authorized_keys',
-              matched: 'throttle_authenticated_api',
-              user_id: user.id,
-              'meta.user' => user.username
+          end
+
+          it 'logs request information and user meta' do
+            expect(Gitlab::AuthLogger).to receive(:error).with(
+              include(
+                message: 'Rack_Attack',
+                env: match_type,
+                remote_ip: '1.2.3.4',
+                request_method: 'GET',
+                path: '/api/v4/internal/authorized_keys',
+                matched: 'throttle_authenticated_api',
+                deploy_token_id: deploy_token.id
+              )
             )
-          )
-          subscriber.send(match_type, event)
+            subscriber.send(match_type, event)
+          end
         end
       end
     end

spec/support/helpers/rack_attack_spec_helpers.rb

@@ -10,11 +10,11 @@ module RackAttackSpecHelpers
   end
 
   def private_token_headers(user)
-    { 'HTTP_PRIVATE_TOKEN' => user.private_token }
+    { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => user.private_token }
   end
 
   def personal_access_token_headers(personal_access_token)
-    { 'HTTP_PRIVATE_TOKEN' => personal_access_token.token }
+    { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => personal_access_token.token }
   end
 
   def oauth_token_headers(oauth_access_token)
@@ -26,6 +26,10 @@ module RackAttackSpecHelpers
     { 'AUTHORIZATION' => "Basic #{encoded_login}" }
   end
 
+  def deploy_token_headers(deploy_token)
+    basic_auth_headers(deploy_token, deploy_token)
+  end
+
   def expect_rejection(name = nil, &block)
     yield
 

spec/support/shared_examples/requests/rack_attack_shared_examples.rb

@@ -8,7 +8,50 @@
 # * requests_per_period
 # * period_in_seconds
 # * period
-RSpec.shared_examples 'rate-limited token-authenticated requests' do
+RSpec.shared_examples 'rate-limited user based token-authenticated requests' do
+  context 'when the throttle is enabled' do
+    before do
+      settings_to_set[:"#{throttle_setting_prefix}_enabled"] = true
+      stub_application_setting(settings_to_set)
+    end
+
+    it 'does not reject requests if the user is in the allowlist' do
+      stub_env('GITLAB_THROTTLE_USER_ALLOWLIST', user.id.to_s)
+      Gitlab::RackAttack.configure_user_allowlist
+
+      expect(Gitlab::Instrumentation::Throttle).to receive(:safelist=).with('throttle_user_allowlist').at_least(:once)
+
+      (requests_per_period + 1).times do
+        make_request(request_args)
+        expect(response).not_to have_gitlab_http_status(:too_many_requests)
+      end
+
+      stub_env('GITLAB_THROTTLE_USER_ALLOWLIST', nil)
+      Gitlab::RackAttack.configure_user_allowlist
+    end
+  end
+
+  include_examples 'rate-limited token requests' do
+    let(:log_data) do
+      {
+        user_id: user.id,
+        'meta.user' => user.username
+      }
+    end
+  end
+end
+
+RSpec.shared_examples 'rate-limited deploy-token-authenticated requests' do
+  include_examples 'rate-limited token requests' do
+    let(:log_data) do
+      {
+        deploy_token_id: deploy_token.id
+      }
+    end
+  end
+end
+
+RSpec.shared_examples 'rate-limited token requests' do
   let(:throttle_types) do
     {
       "throttle_protected_paths" => "throttle_authenticated_protected_paths_api",
@@ -51,18 +94,6 @@ RSpec.shared_examples 'rate-limited token-authenticated requests' do
       expect_rejection { make_request(request_args) }
     end
 
-    it 'does not reject requests if the user is in the allowlist' do
-      stub_env('GITLAB_THROTTLE_USER_ALLOWLIST', user.id.to_s)
-      Gitlab::RackAttack.configure_user_allowlist
-
-      expect(Gitlab::Instrumentation::Throttle).to receive(:safelist=).with('throttle_user_allowlist').at_least(:once)
-
-      (requests_per_period + 1).times do
-        make_request(request_args)
-        expect(response).not_to have_gitlab_http_status(:too_many_requests)
-      end
-    end
-
     it 'allows requests after throttling and then waiting for the next period' do
       requests_per_period.times do
         make_request(request_args)
@@ -81,7 +112,7 @@ RSpec.shared_examples 'rate-limited token-authenticated requests' do
       end
     end
 
-    it 'counts requests from different users separately, even from the same IP' do
+    it 'counts requests from different requesters separately, even from the same IP' do
       requests_per_period.times do
         make_request(request_args)
         expect(response).not_to have_gitlab_http_status(:too_many_requests)
@@ -92,7 +123,7 @@ RSpec.shared_examples 'rate-limited token-authenticated requests' do
       expect(response).not_to have_gitlab_http_status(:too_many_requests)
     end
 
-    it 'counts all requests from the same user, even via different IPs' do
+    it 'counts all requests from the same requesters, even via different IPs' do
       requests_per_period.times do
         make_request(request_args)
         expect(response).not_to have_gitlab_http_status(:too_many_requests)
@@ -122,10 +153,8 @@ RSpec.shared_examples 'rate-limited token-authenticated requests' do
         remote_ip: '127.0.0.1',
         request_method: request_method,
         path: request_args.first,
-        user_id: user.id,
-        'meta.user' => user.username,
         matched: throttle_types[throttle_setting_prefix]
-      })
+      }.merge(log_data))
 
       expect(Gitlab::AuthLogger).to receive(:error).with(arguments).once