Merge branch 'security/sh-fix-ldap-bypass-2fa' into 'master'

Prevent users from bypassing 2FA on certain pages

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

lib/gitlab/auth/auth_finders.rb

@@ -342,6 +342,10 @@ module Gitlab
         Gitlab::PathRegex.repository_git_lfs_route_regex.match?(current_request.path)
       end
 
+      def git_or_lfs_request?
+        git_request? || git_lfs_request?
+      end
+
       def archive_request?
         current_request.path.include?('/-/archive/')
       end

lib/gitlab/auth/request_authenticator.rb

@@ -35,13 +35,31 @@ module Gitlab
           find_user_from_static_object_token(request_format) ||
           find_user_from_basic_auth_job ||
           find_user_from_job_token ||
-          find_user_from_lfs_token ||
-          find_user_from_personal_access_token ||
-          find_user_from_basic_auth_password
+          find_user_from_personal_access_token_for_api_or_git ||
+          find_user_for_git_or_lfs_request
       rescue Gitlab::Auth::AuthenticationError
         nil
       end
 
+      # To prevent Rack Attack from incorrectly rate limiting
+      # authenticated Git activity, we need to authenticate the user
+      # from other means (e.g. HTTP Basic Authentication) only if the
+      # request originated from a Git or Git LFS
+      # request. Repositories::GitHttpClientController or
+      # Repositories::LfsApiController normally does the authentication,
+      # but Rack Attack runs before those controllers.
+      def find_user_for_git_or_lfs_request
+        return unless git_or_lfs_request?
+
+        find_user_from_lfs_token || find_user_from_basic_auth_password
+      end
+
+      def find_user_from_personal_access_token_for_api_or_git
+        return unless api_request? || git_or_lfs_request?
+
+        find_user_from_personal_access_token
+      end
+
       def valid_access_token?(scopes: [])
         validate_access_token!(scopes: scopes)
 

spec/lib/gitlab/auth/request_authenticator_spec.rb

@@ -81,32 +81,72 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do
       expect(subject.find_sessionless_user(:api)).to eq job_token_user
     end
 
-    it 'returns lfs_token user if no job_token user found' do
-      allow_any_instance_of(described_class)
-        .to receive(:find_user_from_lfs_token)
-        .and_return(lfs_token_user)
-
-      expect(subject.find_sessionless_user(:api)).to eq lfs_token_user
-    end
-
-    it 'returns basic_auth_access_token user if no lfs_token user found' do
+    it 'returns nil even if basic_auth_access_token is available' do
       allow_any_instance_of(described_class)
         .to receive(:find_user_from_personal_access_token)
         .and_return(basic_auth_access_token_user)
 
-      expect(subject.find_sessionless_user(:api)).to eq basic_auth_access_token_user
+      expect(subject.find_sessionless_user(:api)).to be_nil
     end
 
-    it 'returns basic_auth_access_password user if no basic_auth_access_token user found' do
+    it 'returns nil even if find_user_from_lfs_token is available' do
       allow_any_instance_of(described_class)
-        .to receive(:find_user_from_basic_auth_password)
-        .and_return(basic_auth_password_user)
+        .to receive(:find_user_from_lfs_token)
+        .and_return(lfs_token_user)
 
-      expect(subject.find_sessionless_user(:api)).to eq basic_auth_password_user
+      expect(subject.find_sessionless_user(:api)).to be_nil
     end
 
     it 'returns nil if no user found' do
-      expect(subject.find_sessionless_user(:api)).to be_blank
+      expect(subject.find_sessionless_user(:api)).to be_nil
+    end
+
+    context 'in an API request' do
+      before do
+        env['SCRIPT_NAME'] = '/api/v4/projects'
+      end
+
+      it 'returns basic_auth_access_token user if no job_token_user found' do
+        allow_any_instance_of(described_class)
+          .to receive(:find_user_from_personal_access_token)
+          .and_return(basic_auth_access_token_user)
+
+        expect(subject.find_sessionless_user(:api)).to eq basic_auth_access_token_user
+      end
+    end
+
+    context 'in a Git request' do
+      before do
+        env['SCRIPT_NAME'] = '/group/project.git/info/refs'
+      end
+
+      it 'returns lfs_token user if no job_token user found' do
+        allow_any_instance_of(described_class)
+          .to receive(:find_user_from_lfs_token)
+          .and_return(lfs_token_user)
+
+        expect(subject.find_sessionless_user(nil)).to eq lfs_token_user
+      end
+
+      it 'returns basic_auth_access_token user if no lfs_token user found' do
+        allow_any_instance_of(described_class)
+          .to receive(:find_user_from_personal_access_token)
+          .and_return(basic_auth_access_token_user)
+
+        expect(subject.find_sessionless_user(nil)).to eq basic_auth_access_token_user
+      end
+
+      it 'returns basic_auth_access_password user if no basic_auth_access_token user found' do
+        allow_any_instance_of(described_class)
+          .to receive(:find_user_from_basic_auth_password)
+          .and_return(basic_auth_password_user)
+
+        expect(subject.find_sessionless_user(nil)).to eq basic_auth_password_user
+      end
+
+      it 'returns nil if no user found' do
+        expect(subject.find_sessionless_user(nil)).to be_blank
+      end
     end
 
     it 'rescue Gitlab::Auth::AuthenticationError exceptions' do

spec/requests/rack_attack_global_spec.rb

@@ -1144,17 +1144,28 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
     end
 
     context 'authenticated with lfs token' do
-      it 'request is authenticated by token in basic auth' do
-        lfs_token = Gitlab::LfsToken.new(user)
-        encoded_login = ["#{user.username}:#{lfs_token.token}"].pack('m0')
+      let(:lfs_url) { '/namespace/repo.git/info/lfs/objects/batch' }
+      let(:lfs_token) { Gitlab::LfsToken.new(user) }
+      let(:encoded_login) { ["#{user.username}:#{lfs_token.token}"].pack('m0') }
+      let(:headers) { { 'AUTHORIZATION' => "Basic #{encoded_login}" } }
 
+      it 'request is authenticated by token in basic auth' do
         expect_authenticated_request
 
-        get url, headers: { 'AUTHORIZATION' => "Basic #{encoded_login}" }
+        get lfs_url, headers: headers
+      end
+
+      it 'request is not authenticated with API URL' do
+        expect_unauthenticated_request
+
+        get url, headers: headers
       end
     end
 
     context 'authenticated with regular login' do
+      let(:encoded_login) { ["#{user.username}:#{user.password}"].pack('m0') }
+      let(:headers) { { 'AUTHORIZATION' => "Basic #{encoded_login}" } }
+
       it 'request is authenticated after login' do
         login_as(user)
 
@@ -1163,12 +1174,30 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
         get url
       end
 
-      it 'request is authenticated by credentials in basic auth' do
-        encoded_login = ["#{user.username}:#{user.password}"].pack('m0')
+      it 'request is not authenticated by credentials in basic auth' do
+        expect_unauthenticated_request
 
-        expect_authenticated_request
+        get url, headers: headers
+      end
+
+      context 'with POST git-upload-pack' do
+        it 'request is authenticated by credentials in basic auth' do
+          expect(::Gitlab::Workhorse).to receive(:verify_api_request!)
+
+          expect_authenticated_request
 
-        get url, headers: { 'AUTHORIZATION' => "Basic #{encoded_login}" }
+          post '/namespace/repo.git/git-upload-pack', headers: headers
+        end
+      end
+
+      context 'with GET info/refs' do
+        it 'request is authenticated by credentials in basic auth' do
+          expect(::Gitlab::Workhorse).to receive(:verify_api_request!)
+
+          expect_authenticated_request
+
+          get '/namespace/repo.git/info/refs?service=git-upload-pack', headers: headers
+        end
       end
     end
   end