Add latest changes from gitlab-org/security/gitlab@14-3-stable-ee

5 files changed, 119 insertions, 27 deletions

diff --git a/lib/gitlab/auth/auth_finders.rb b/lib/gitlab/auth/auth_finders.rb
index f6ee08defcf..9c33a5fc872 100644
--- a/lib/gitlab/auth/auth_finders.rb
+++ b/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
diff --git a/lib/gitlab/auth/request_authenticator.rb b/lib/gitlab/auth/request_authenticator.rb
index dfc682e8a5c..08214bbd449 100644
--- a/lib/gitlab/auth/request_authenticator.rb
+++ b/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)
 
diff --git a/spec/lib/gitlab/auth/request_authenticator_spec.rb b/spec/lib/gitlab/auth/request_authenticator_spec.rb
index 28e93a8da52..2543eb3a5e9 100644
--- a/spec/lib/gitlab/auth/request_authenticator_spec.rb
+++ b/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
diff --git a/spec/requests/rack_attack_global_spec.rb b/spec/requests/rack_attack_global_spec.rb
index 87ef6fa1a18..be942f6ae86 100644
--- a/spec/requests/rack_attack_global_spec.rb
+++ b/spec/requests/rack_attack_global_spec.rb
@@ -933,17 +933,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)
 
@@ -952,12 +963,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
diff --git a/workhorse/internal/artifacts/entry.go b/workhorse/internal/artifacts/entry.go
index 91a0843f1ee..d5b3dfb672c 100644
--- a/workhorse/internal/artifacts/entry.go
+++ b/workhorse/internal/artifacts/entry.go
@@ -14,6 +14,7 @@ import (
 	"syscall"
 
 	"gitlab.com/gitlab-org/labkit/log"
+	"gitlab.com/gitlab-org/labkit/mask"
 
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/senddata"
@@ -35,7 +36,7 @@ func (e *entry) Inject(w http.ResponseWriter, r *http.Request, sendData string)
 
 	log.WithContextFields(r.Context(), log.Fields{
 		"entry":   params.Entry,
-		"archive": params.Archive,
+		"archive": mask.URL(params.Archive),
 		"path":    r.URL.Path,
 	}).Print("SendEntry: sending")