Change the mirror user along with pull mirror settings

Pull mirroring stores a "mirror_user", which is used to attribute
changes performed by the mirroring option and, in some cases, for
checking permissions. In general, the mirror user should be the user
who set up the pull mirror. However, **changes** to mirror attributes
do not reset the mirror_user at present.

This property allows a mirror to be changed by one project maintainer,
and for the changes to be wrongly attributed to another maintainer. In
the worst case, this could lead to permissions bypasses or credential
theft.

To mitigate this, we remove the ability to specify who the mirror user
will be, and ensure it is always set to the ID of the user who modifies
the pull mirroring settings. The only exception is if you're an admin
user making changes via the API - in this circumstance, we allow the
mirror_user_id to be set arbitrarily. This preserves an API endpoint
that may be widely used.

doc/api/projects.md

@@ -1195,7 +1195,7 @@ PUT /projects/:id
 | `approvals_before_merge` | integer | no | **(STARTER)** How many approvers should approve merge request by default |
 | `external_authorization_classification_label` | string | no | **(PREMIUM)** The classification label for the project |
 | `mirror` | boolean | no | **(STARTER)** Enables pull mirroring in a project |
-| `mirror_user_id` | integer | no | **(STARTER)** User responsible for all the activity surrounding a pull mirror event |
+| `mirror_user_id` | integer | no | **(STARTER)** User responsible for all the activity surrounding a pull mirror event. Can only be set by admins. |
 | `mirror_trigger_builds` | boolean | no | **(STARTER)** Pull mirroring triggers builds |
 | `only_mirror_protected_branches` | boolean | no | **(STARTER)** Only mirror protected branches |
 | `mirror_overwrites_diverged_branches` | boolean | no | **(STARTER)** Pull mirror overwrites diverged branches |

ee/app/controllers/concerns/safe_mirror_params.rb

-# frozen_string_literal: true
-
-module SafeMirrorParams
-  extend ActiveSupport::Concern
-
-  included do
-    helper_method :default_mirror_users
-  end
-
-  private
-
-  def valid_mirror_user?(mirror_params)
-    return true unless mirror_params[:mirror_user_id].present?
-
-    default_mirror_users.map(&:id).include?(mirror_params[:mirror_user_id].to_i)
-  end
-
-  def default_mirror_users
-    [current_user, project.mirror_user].compact.uniq
-  end
-end

ee/app/controllers/ee/projects/imports_controller.rb

@@ -6,23 +6,16 @@ module EE
       extend ActiveSupport::Concern
       extend ::Gitlab::Utils::Override
 
-      prepended do
-        include SafeMirrorParams
-      end
-
       private
 
       override :import_params_attributes
       def import_params_attributes
-        super + [:mirror, :mirror_user_id]
+        super + [:mirror]
       end
 
       override :import_params
       def import_params
-        base_import_params = super
-        return base_import_params if valid_mirror_user?(base_import_params)
-
-        base_import_params.merge(mirror_user_id: current_user.id)
+        super.merge(mirror_user_id: current_user&.id)
       end
     end
   end

ee/app/controllers/ee/projects/mirrors_controller.rb

@@ -59,16 +59,10 @@ module EE
       private
 
       def mirror_params_attributes_ee
-        [
-          :mirror,
-          :import_url,
-          :username_only_import_url,
-          :mirror_user_id,
-          :mirror_trigger_builds,
-          :only_mirror_protected_branches,
-          :mirror_overwrites_diverged_branches,
-          :pull_mirror_branch_prefix,
-
+        attrs = Projects::UpdateService::PULL_MIRROR_ATTRIBUTES.dup
+        attrs.delete(:mirror_user_id) # Cannot be set by the frontend
+        attrs.delete(:import_data_attributes) # We need more detail here
+        attrs.push(
           import_data_attributes: %i[
             id
             auth_method
@@ -76,7 +70,7 @@ module EE
             ssh_known_hosts
             regenerate_ssh_private_key
           ]
-        ]
+        )
       end
 
       def safe_mirror_params

ee/app/controllers/ee/projects/settings/repository_controller.rb

@@ -7,8 +7,6 @@ module EE
         extend ActiveSupport::Concern
 
         prepended do
-          include SafeMirrorParams
-
           before_action :push_rule, only: [:show, :create_deploy_token]
         end
 

ee/app/controllers/ee/projects_controller.rb

@@ -106,7 +106,6 @@ module EE
       %i[
         mirror
         mirror_trigger_builds
-        mirror_user_id
       ]
     end
 

ee/app/helpers/ee/mirror_helper.rb

@@ -29,7 +29,7 @@ module EE
     end
 
     def options_for_mirror_user
-      options_from_collection_for_select(default_mirror_users, :id, :name, @project.mirror_user_id || current_user.id)
+      options_from_collection_for_select([current_user], :id, :name, current_user.id)
     end
 
     def mirrored_repositories_count(project = @project)

ee/app/services/ee/projects/update_service.rb

@@ -6,19 +6,27 @@ module EE
       extend ::Gitlab::Utils::Override
       include CleanupApprovers
 
+      PULL_MIRROR_ATTRIBUTES = %i[
+        mirror
+        mirror_user_id
+        import_url
+        username_only_import_url
+        mirror_trigger_builds
+        only_mirror_protected_branches
+        mirror_overwrites_diverged_branches
+        pull_mirror_branch_prefix
+        import_data_attributes
+      ].freeze
+
       override :execute
       def execute
         should_remove_old_approvers = params.delete(:remove_old_approvers)
-        wiki_was_enabled = project.wiki_enabled?
-
         limit = params.delete(:repository_size_limit)
+        wiki_was_enabled = project.wiki_enabled?
 
-        unless valid_mirror_user?
-          project.errors.add(:mirror_user_id, 'is invalid')
-          return project
-        end
-
+        mirror_user_setting
         compliance_framework_setting
+        return update_failed! if project.errors.any?
 
         result = super do
           # Repository size limit comes as MB from the view
@@ -41,13 +49,19 @@ module EE
 
       private
 
-      def valid_mirror_user?
-        return true unless params[:mirror_user_id].present?
-
-        mirror_user_id = params[:mirror_user_id].to_i
-
-        mirror_user_id == current_user.id ||
-          mirror_user_id == project.mirror_user&.id
+      # A user who changes any aspect of pull mirroring settings must be made
+      # into the mirror user, to prevent them from acquiring capabilities
+      # owned by the previous user, such as writing to a protected branch.
+      #
+      # Only admins can set the mirror user to be an arbitrary user.
+      def mirror_user_setting
+        return unless PULL_MIRROR_ATTRIBUTES.any? { |symbol| params.key?(symbol) }
+
+        if params[:mirror_user_id] && params[:mirror_user_id] != project.mirror_user_id
+          project.errors.add(:mirror_user_id, 'is invalid') unless current_user&.admin?
+        else
+          params[:mirror_user_id] = current_user.id
+        end
       end
 
       def compliance_framework_setting

ee/app/views/projects/mirrors/_mirror_repos_form.html.haml

 - import_data = @project.import_data || @project.build_import_data
-- is_one_user_option = default_mirror_users.count == 1
 - protocols = Gitlab::UrlSanitizer::ALLOWED_SCHEMES.join('|')
 - direction_options = [[_('Push'), 'push']]
 - has_existing_pull_mirror = @project.mirror.present?
@@ -29,15 +28,10 @@
     = render partial: 'projects/mirrors/authentication_method', locals: { f: import_form }
 
   .form-group
-    = f.label :mirror_user_id, _('Mirror user'), class: 'label-light'
-    - if is_one_user_option
-      = select_tag(:mirror_user_id_select, options_for_mirror_user, class: 'js-mirror-user select2 lg append-bottom-5', required: true, disabled: true)
-      = f.hidden_field :mirror_user_id, value: default_mirror_users.first.id if is_one_user_option
-    - else
-      = f.select(:mirror_user_id, options_for_mirror_user, {}, class: 'js-mirror-user select2 lg append-bottom-5', required: true)
-
+    = f.label :mirror_user_id_select, _('Mirror user'), class: 'label-light'
+    = select_tag(:mirror_user_id_select, options_for_mirror_user, class: 'js-mirror-user select2 lg append-bottom-5', required: true, disabled: true)
     .help-block
-      = _('This user will be the author of all events in the activity feed that are the result of an update, like new branches being created or new commits being pushed to existing branches. Upon creation or when reassigning you can only assign yourself to be the mirror user.')
+      = _('You will be the author of all events in the activity feed that are the result of an update, like new branches being created or new commits being pushed to existing branches.')
 
   - if Feature.enabled?(:pull_mirror_branch_prefix, @project)
     .form-group

ee/changelogs/unreleased/security-211940-track-mirror-user.yml

+---
+title: Change the mirror user along with pull mirror settings
+merge_request:
+author:
+type: security

ee/lib/ee/api/helpers/projects_helpers.rb

@@ -29,7 +29,7 @@ module EE
           end
 
           params :optional_update_params_ee do
-            optional :mirror_user_id, type: Integer, desc: 'User responsible for all the activity surrounding a pull mirror event'
+            optional :mirror_user_id, type: Integer, desc: 'User responsible for all the activity surrounding a pull mirror event. Can only be set by admins'
             optional :only_mirror_protected_branches, type: Grape::API::Boolean, desc: 'Only mirror protected branches'
             optional :mirror_overwrites_diverged_branches, type: Grape::API::Boolean, desc: 'Pull mirror overwrites diverged branches'
             optional :import_url, type: String, desc: 'URL from which the project is imported'

ee/lib/ee/api/projects.rb

@@ -43,12 +43,9 @@ module EE
 
           def verify_mirror_attrs!(project, attrs)
             unless can?(current_user, :admin_mirror, project)
-              attrs.delete(:mirror)
-              attrs.delete(:mirror_user_id)
-              attrs.delete(:mirror_trigger_builds)
-              attrs.delete(:only_mirror_protected_branches)
-              attrs.delete(:mirror_overwrites_diverged_branches)
-              attrs.delete(:import_data_attributes)
+              ::Projects::UpdateService::PULL_MIRROR_ATTRIBUTES.each do |attr_name|
+                attrs.delete(attr_name)
+              end
             end
           end
 

ee/spec/controllers/projects/mirrors_controller_spec.rb

@@ -30,33 +30,29 @@ describe Projects::MirrorsController do
         sign_in(project.owner)
       end
 
-      context 'when trying to create a mirror with the same URL' do
-        it 'does not set up the mirror' do
-          do_put(project, mirror: true, import_url: remote_mirror.url)
+      context 'mirror_user is unset' do
+        it 'sets up a pull mirror with the mirror user set to the signed-in user' do
+          expect(project.mirror_user).to be_nil
 
-          expect(project.reload.mirror).to be_falsey
-          expect(project.reload.import_url).to be_blank
-        end
-      end
-
-      context 'when trying to create a mirror with a different URL' do
-        it 'sets up the mirror' do
-          do_put(project, mirror: true, mirror_user_id: project.owner.id, import_url: 'http://local.dev')
+          do_put(project, mirror: true, import_url: 'http://local.dev')
+          project.reload
 
-          expect(project.reload.mirror).to eq(true)
-          expect(project.reload.import_url).to eq('http://local.dev')
+          expect(project.mirror).to eq(true)
+          expect(project.import_url).to eq('http://local.dev')
+          expect(project.mirror_user).to eq(project.owner)
         end
+      end
 
-        context 'mirror user is not the current user' do
-          it 'does not set up the mirror' do
-            new_user = create(:user)
-            project.add_maintainer(new_user)
+      context 'mirror_user is not the current user' do
+        it 'sets up a pull mirror with the mirror user set to the signed-in user' do
+          new_user = create(:user)
+          project.add_maintainer(new_user)
 
-            do_put(project, mirror: true, mirror_user_id: new_user.id, import_url: 'http://local.dev')
+          do_put(project, mirror: true, mirror_user_id: new_user.id, import_url: 'http://local.dev')
 
-            expect(project.reload.mirror).to be_falsey
-            expect(project.reload.import_url).to be_blank
-          end
+          expect(project.mirror).to eq(true)
+          expect(project.import_url).to eq('http://local.dev')
+          expect(project.mirror_user).to eq(project.owner)
         end
       end
     end
@@ -78,7 +74,7 @@ describe Projects::MirrorsController do
           sign_in(admin)
 
           expect do
-            do_put(project, mirror: true, mirror_user_id: admin.id, import_url: url)
+            do_put(project, mirror: true, import_url: url)
           end.to change { Project.mirror.count }.to(1)
         end
       end
@@ -88,7 +84,7 @@ describe Projects::MirrorsController do
           sign_in(project.owner)
 
           expect do
-            do_put(project, mirror: true, mirror_user_id: project.owner.id, import_url: url)
+            do_put(project, mirror: true, import_url: url)
           end.not_to change { Project.mirror.count }
         end
       end
@@ -194,8 +190,7 @@ describe Projects::MirrorsController do
 
         do_put(project, { mirror_user_id: other_user.id }, format: :json)
 
-        expect(response).to have_gitlab_http_status(:unprocessable_entity)
-        expect(json_response['mirror_user_id'].first).to eq("is invalid")
+        expect(project.reload.mirror_user).to eq(project.owner)
       end
     end
 

ee/spec/controllers/projects_controller_spec.rb

@@ -271,7 +271,6 @@ describe ProjectsController do
         {
           mirror: true,
           mirror_trigger_builds: true,
-          mirror_user_id: user.id,
           import_url: 'https://example.com'
         }
       end
@@ -297,6 +296,20 @@ describe ProjectsController do
           expect(project.mirror_user).to eq(user)
           expect(project.import_url).to eq('https://example.com')
         end
+
+        it 'ignores mirror_user_id' do
+          other_user = create(:user)
+
+          put :update,
+            params: {
+              namespace_id: project.namespace,
+              id: project,
+              project: params.merge(mirror_user_id: other_user.id)
+            }
+          project.reload
+
+          expect(project.mirror_user).to eq(user)
+        end
       end
 
       context 'when unlicensed' do

ee/spec/features/projects/settings/ee/repository_mirrors_settings_spec.rb

@@ -23,7 +23,7 @@ describe 'Project settings > [EE] repository' do
         expect(page).to have_selector('#url')
         expect(page).to have_selector('#mirror_direction')
         expect(page).to have_no_selector('#project_mirror', visible: false)
-        expect(page).to have_no_selector('#project_mirror_user_id', visible: false)
+        expect(page).to have_no_selector('#mirror_user_id_select', visible: false)
         expect(page).to have_no_selector('#project_mirror_overwrites_diverged_branches')
         expect(page).to have_no_selector('#project_mirror_trigger_builds')
         expect(page).to have_no_selector('#project_pull_mirror_branch_prefix')
@@ -43,7 +43,7 @@ describe 'Project settings > [EE] repository' do
         expect(page).to have_selector('#url')
         expect(page).to have_selector('#mirror_direction')
         expect(page).to have_selector('#project_mirror', visible: false)
-        expect(page).to have_selector('#project_mirror_user_id', visible: false)
+        expect(page).to have_selector('#mirror_user_id_select', visible: false)
         expect(page).to have_selector('#project_mirror_overwrites_diverged_branches')
         expect(page).to have_selector('#project_mirror_trigger_builds')
         expect(page).to have_selector('#project_pull_mirror_branch_prefix')

ee/spec/requests/api/projects_spec.rb

@@ -567,7 +567,6 @@ describe API::Projects do
         {
           mirror: true,
           import_url: import_url,
-          mirror_user_id: user.id,
           mirror_trigger_builds: true,
           only_mirror_protected_branches: true,
           mirror_overwrites_diverged_branches: true
@@ -588,7 +587,9 @@ describe API::Projects do
 
         it 'updates mirror related attributes when user is admin' do
           admin = create(:admin)
-          mirror_params[:mirror_user_id] = admin.id
+          unrelated_user = create(:user)
+
+          mirror_params[:mirror_user_id] = unrelated_user.id
           project.add_maintainer(admin)
 
           expect_any_instance_of(EE::ProjectImportState).to receive(:force_import_job!).once
@@ -599,7 +600,7 @@ describe API::Projects do
           expect(project.reload).to have_attributes(
             mirror: true,
             import_url: import_url,
-            mirror_user_id: admin.id,
+            mirror_user_id: unrelated_user.id,
             mirror_trigger_builds: true,
             only_mirror_protected_branches: true,
             mirror_overwrites_diverged_branches: true

ee/spec/requests/projects/mirrors_controller_spec.rb

@@ -23,7 +23,6 @@ describe Projects::MirrorsController do
                 project: {
           mirror: '1',
           import_url: '',
-          mirror_user_id: user.id,
           mirror_trigger_builds: '0'
         }
       }

ee/spec/services/projects/update_service_spec.rb

@@ -6,26 +6,48 @@ describe Projects::UpdateService, '#execute' do
   include EE::GeoHelpers
 
   let(:user) { create(:user) }
+  let(:admin) { create(:user, :admin) }
   let(:project) { create(:project, :repository, creator: user, namespace: user.namespace) }
 
   context 'repository mirror' do
-    let!(:opts) do
-      {
-      }
-    end
+    let(:opts) { { mirror: true, import_url: 'http://foo.com' } }
 
     before do
       stub_licensed_features(repository_mirrors: true)
     end
 
-    it 'forces an import job' do
-      opts = {
-        import_url: 'http://foo.com',
-        mirror: true,
-        mirror_user_id: user.id,
-        mirror_trigger_builds: true
-      }
+    it 'sets mirror attributes' do
+      result = update_project(project, user, opts)
 
+      expect(result).to eq(status: :success)
+      expect(project).to have_attributes(opts)
+      expect(project.mirror_user).to eq(user)
+    end
+
+    it 'does not touch mirror_user_id for non-mirror changes' do
+      result = update_project(project, user, description: 'anything')
+
+      expect(result).to eq(status: :success)
+      expect(project.mirror_user).to be_nil
+    end
+
+    it 'forbids non-admins from setting mirror_user_id explicitly' do
+      project.team.add_maintainer(admin)
+      result = update_project(project, user, opts.merge(mirror_user_id: admin.id))
+
+      expect(result).to eq(status: :error, message: 'Mirror user is invalid')
+      expect(project.mirror_user).to be_nil
+    end
+
+    it 'allows admins to set mirror_user_id' do
+      project.team.add_maintainer(admin)
+      result = update_project(project, admin, opts.merge(mirror_user_id: user.id))
+
+      expect(result).to eq(status: :success)
+      expect(project.mirror_user).to eq(user)
+    end
+
+    it 'forces an import job' do
       expect_any_instance_of(EE::ProjectImportState).to receive(:force_import_job!).once
 
       update_project(project, user, opts)

locale/gitlab.pot

@@ -21851,9 +21851,6 @@ msgstr ""
 msgid "This user will be the author of all events in the activity feed that are the result of an update, like new branches being created or new commits being pushed to existing branches."
 msgstr ""
 
-msgid "This user will be the author of all events in the activity feed that are the result of an update, like new branches being created or new commits being pushed to existing branches. Upon creation or when reassigning you can only assign yourself to be the mirror user."
-msgstr ""
-
 msgid "This variable can not be masked."
 msgstr ""
 
@@ -24573,6 +24570,9 @@ msgstr ""
 msgid "You will be removed from existing projects/groups"
 msgstr ""
 
+msgid "You will be the author of all events in the activity feed that are the result of an update, like new branches being created or new commits being pushed to existing branches."
+msgstr ""
+
 msgid "You will first need to set up Jira Integration to use this feature."
 msgstr ""