From 5cf5680f9c8ce251570efce7dd4c348fb68efccf Mon Sep 17 00:00:00 2001 From: "Jacob Vosmaer (GitLab)" Date: Thu, 14 Jun 2018 11:18:25 +0000 Subject: [PATCH] Deny repository disk access in development and test --- app/models/project_services/gemnasium_service.rb | 7 ++++++- ...4141322_migrate_process_commit_worker_jobs.rb | 4 +++- ...161226122833_remove_dot_git_from_usernames.rb | 4 +++- lib/gitlab/gitaly_client.rb | 14 ++++++++------ lib/system_check/orphans/repository_check.rb | 16 +++++++++------- spec/controllers/projects_controller_spec.rb | 12 +++++++++--- spec/helpers/projects_helper_spec.rb | 6 +++++- spec/lib/gitlab/git_access_wiki_spec.rb | 4 +++- .../migrate_process_commit_worker_jobs_spec.rb | 6 +++++- ..._groups_into_regular_groups_for_mysql_spec.rb | 8 ++++++-- spec/models/project_spec.rb | 14 ++++++++++---- spec/models/project_wiki_spec.rb | 6 +++++- spec/models/remote_mirror_spec.rb | 4 +++- spec/requests/api/internal_spec.rb | 1 - spec/services/projects/create_service_spec.rb | 5 ++++- spec/support/gitaly.rb | 2 +- .../repository_remove_remote_worker_spec.rb | 4 +++- 17 files changed, 83 insertions(+), 34 deletions(-) diff --git a/app/models/project_services/gemnasium_service.rb b/app/models/project_services/gemnasium_service.rb index 84248f9590b..8a6b0ed1a5f 100644 --- a/app/models/project_services/gemnasium_service.rb +++ b/app/models/project_services/gemnasium_service.rb @@ -43,13 +43,18 @@ class GemnasiumService < Service def execute(data) return unless supported_events.include?(data[:object_kind]) + # Gitaly: this class will be removed https://gitlab.com/gitlab-org/gitlab-ee/issues/6010 + repo_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + project.repository.path_to_repo + end + Gemnasium::GitlabService.execute( ref: data[:ref], before: data[:before], after: data[:after], token: token, api_key: api_key, - repo: project.repository.path_to_repo # Gitaly: fixed by https://gitlab.com/gitlab-org/security-products/gemnasium-migration/issues/9 + repo: repo_path ) end end diff --git a/db/migrate/20161124141322_migrate_process_commit_worker_jobs.rb b/db/migrate/20161124141322_migrate_process_commit_worker_jobs.rb index a96ea7d9db4..dc16d5c5169 100644 --- a/db/migrate/20161124141322_migrate_process_commit_worker_jobs.rb +++ b/db/migrate/20161124141322_migrate_process_commit_worker_jobs.rb @@ -12,7 +12,9 @@ class MigrateProcessCommitWorkerJobs < ActiveRecord::Migration end def repository_storage_path - Gitlab.config.repositories.storages[repository_storage].legacy_disk_path + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + Gitlab.config.repositories.storages[repository_storage].legacy_disk_path + end end def repository_path diff --git a/db/migrate/20161226122833_remove_dot_git_from_usernames.rb b/db/migrate/20161226122833_remove_dot_git_from_usernames.rb index 8986cd8cb4b..133435523e1 100644 --- a/db/migrate/20161226122833_remove_dot_git_from_usernames.rb +++ b/db/migrate/20161226122833_remove_dot_git_from_usernames.rb @@ -64,7 +64,9 @@ class RemoveDotGitFromUsernames < ActiveRecord::Migration # we rename suffix instead of removing it path = path.sub(/\.git\z/, '_git') - check_routes(path.dup, 0, path) + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + check_routes(path.dup, 0, path) + end end def check_routes(base, counter, path) diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index 36e9adf27da..620362b52a9 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -33,11 +33,6 @@ module Gitlab MAXIMUM_GITALY_CALLS = 35 CLIENT_NAME = (Sidekiq.server? ? 'gitlab-sidekiq' : 'gitlab-web').freeze - # We have a mechanism to let GitLab automatically opt in to all Gitaly - # features. We want to be able to exclude some features from automatic - # opt-in. That is what EXPLICIT_OPT_IN_REQUIRED is for. - EXPLICIT_OPT_IN_REQUIRED = [Gitlab::GitalyClient::StorageSettings::DISK_ACCESS_DENIED_FLAG].freeze - MUTEX = Mutex.new class << self @@ -249,7 +244,7 @@ module Gitlab when MigrationStatus::OPT_OUT true when MigrationStatus::OPT_IN - opt_into_all_features? && !EXPLICIT_OPT_IN_REQUIRED.include?(feature_name) + opt_into_all_features? && !explicit_opt_in_required.include?(feature_name) else false end @@ -259,6 +254,13 @@ module Gitlab false end + # We have a mechanism to let GitLab automatically opt in to all Gitaly + # features. We want to be able to exclude some features from automatic + # opt-in. This function has an override in EE. + def self.explicit_opt_in_required + [] + end + # opt_into_all_features? returns true when the current environment # is one in which we opt into features automatically def self.opt_into_all_features? diff --git a/lib/system_check/orphans/repository_check.rb b/lib/system_check/orphans/repository_check.rb index 5ef0b93ad08..2695c658874 100644 --- a/lib/system_check/orphans/repository_check.rb +++ b/lib/system_check/orphans/repository_check.rb @@ -5,16 +5,18 @@ module SystemCheck attr_accessor :orphans def multi_check - Gitlab.config.repositories.storages.each do |storage_name, repository_storage| - storage_path = repository_storage.legacy_disk_path + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + Gitlab.config.repositories.storages.each do |storage_name, repository_storage| + storage_path = repository_storage.legacy_disk_path - $stdout.puts - $stdout.puts "* Storage: #{storage_name} (#{storage_path})".color(:yellow) + $stdout.puts + $stdout.puts "* Storage: #{storage_name} (#{storage_path})".color(:yellow) - repositories = disk_repositories(storage_path) - orphans = (repositories - fetch_repositories(storage_name)) + repositories = disk_repositories(storage_path) + orphans = (repositories - fetch_repositories(storage_name)) - print_orphans(orphans, storage_name) + print_orphans(orphans, storage_name) + end end end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 5bd22ea803c..705b30f0130 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -296,16 +296,22 @@ describe ProjectsController do shared_examples_for 'updating a project' do context 'when only renaming a project path' do it "sets the repository to the right path after a rename" do - original_repository_path = project.repository.path + original_repository_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + project.repository.path + end expect { update_project path: 'renamed_path' } .to change { project.reload.path } expect(project.path).to include 'renamed_path' + assign_repository_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + assigns(:repository).path + end + if project.hashed_storage?(:repository) - expect(assigns(:repository).path).to eq(original_repository_path) + expect(assign_repository_path).to eq(original_repository_path) else - expect(assigns(:repository).path).to include(project.path) + expect(assign_repository_path).to include(project.path) end expect(response).to have_gitlab_http_status(302) diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 815b4035114..5cf9e9e8f12 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -280,7 +280,11 @@ describe ProjectsHelper do describe '#sanitizerepo_repo_path' do let(:project) { create(:project, :repository) } - let(:storage_path) { Gitlab.config.repositories.storages.default.legacy_disk_path } + let(:storage_path) do + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + Gitlab.config.repositories.storages.default.legacy_disk_path + end + end before do allow(Settings.shared).to receive(:[]).with('path').and_return('/base/repo/export/path') diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb index 730ede99fc9..9c6c9fe13bf 100644 --- a/spec/lib/gitlab/git_access_wiki_spec.rb +++ b/spec/lib/gitlab/git_access_wiki_spec.rb @@ -52,7 +52,9 @@ describe Gitlab::GitAccessWiki do context 'when the wiki repository does not exist' do it 'returns not found' do wiki_repo = project.wiki.repository - FileUtils.rm_rf(wiki_repo.path) + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + FileUtils.rm_rf(wiki_repo.path) + end # Sanity check for rm_rf expect(wiki_repo.exists?).to eq(false) diff --git a/spec/migrations/migrate_process_commit_worker_jobs_spec.rb b/spec/migrations/migrate_process_commit_worker_jobs_spec.rb index 4ee1d255fbd..ac34efa4f9d 100644 --- a/spec/migrations/migrate_process_commit_worker_jobs_spec.rb +++ b/spec/migrations/migrate_process_commit_worker_jobs_spec.rb @@ -6,7 +6,11 @@ require Rails.root.join('db', 'migrate', '20161124141322_migrate_process_commit_ describe MigrateProcessCommitWorkerJobs do let(:project) { create(:project, :legacy_storage, :repository) } # rubocop:disable RSpec/FactoriesInMigrationSpecs let(:user) { create(:user) } # rubocop:disable RSpec/FactoriesInMigrationSpecs - let(:commit) { project.commit.raw.rugged_commit } + let(:commit) do + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + project.commit.raw.rugged_commit + end + end describe 'Project' do describe 'find_including_path' do diff --git a/spec/migrations/turn_nested_groups_into_regular_groups_for_mysql_spec.rb b/spec/migrations/turn_nested_groups_into_regular_groups_for_mysql_spec.rb index 560409f08de..5f5ba426d69 100644 --- a/spec/migrations/turn_nested_groups_into_regular_groups_for_mysql_spec.rb +++ b/spec/migrations/turn_nested_groups_into_regular_groups_for_mysql_spec.rb @@ -49,10 +49,14 @@ describe TurnNestedGroupsIntoRegularGroupsForMysql do end it 'renames the repository of any projects' do - expect(updated_project.repository.path) + repo_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + updated_project.repository.path + end + + expect(repo_path) .to end_with("#{parent_group.name}-#{child_group.name}/#{updated_project.path}.git") - expect(File.directory?(updated_project.repository.path)).to eq(true) + expect(File.directory?(repo_path)).to eq(true) end it 'creates a redirect route for renamed projects' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 585cf7aab44..bc9cce6b0c3 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2943,7 +2943,7 @@ describe Project do project.rename_repo - expect(project.repository.rugged.config['gitlab.fullpath']).to eq(project.full_path) + expect(rugged_config['gitlab.fullpath']).to eq(project.full_path) end end @@ -3104,7 +3104,7 @@ describe Project do it 'updates project full path in .git/config' do project.rename_repo - expect(project.repository.rugged.config['gitlab.fullpath']).to eq(project.full_path) + expect(rugged_config['gitlab.fullpath']).to eq(project.full_path) end end @@ -3525,13 +3525,13 @@ describe Project do it 'writes full path in .git/config when key is missing' do project.write_repository_config - expect(project.repository.rugged.config['gitlab.fullpath']).to eq project.full_path + expect(rugged_config['gitlab.fullpath']).to eq project.full_path end it 'updates full path in .git/config when key is present' do project.write_repository_config(gl_full_path: 'old/path') - expect { project.write_repository_config }.to change { project.repository.rugged.config['gitlab.fullpath'] }.from('old/path').to(project.full_path) + expect { project.write_repository_config }.to change { rugged_config['gitlab.fullpath'] }.from('old/path').to(project.full_path) end it 'does not raise an error with an empty repository' do @@ -3817,4 +3817,10 @@ describe Project do let(:uploader_class) { AttachmentUploader } end end + + def rugged_config + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + project.repository.rugged.config + end + end end diff --git a/spec/models/project_wiki_spec.rb b/spec/models/project_wiki_spec.rb index f1142832f1a..a3c20b3b3c1 100644 --- a/spec/models/project_wiki_spec.rb +++ b/spec/models/project_wiki_spec.rb @@ -188,7 +188,11 @@ describe ProjectWiki do before do subject.wiki # Make sure the wiki repo exists - BareRepoOperations.new(subject.repository.path_to_repo).commit_file(image, 'image.png') + repo_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + subject.repository.path_to_repo + end + + BareRepoOperations.new(repo_path).commit_file(image, 'image.png') end it 'returns the latest version of the file if it exists' do diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb index 4c086eeadfc..3597b080021 100644 --- a/spec/models/remote_mirror_spec.rb +++ b/spec/models/remote_mirror_spec.rb @@ -74,7 +74,9 @@ describe RemoteMirror do mirror.update_attribute(:url, 'http://foo:baz@test.com') - config = repo.raw_repository.rugged.config + config = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + repo.raw_repository.rugged.config + end expect(config["remote.#{mirror.remote_name}.url"]).to eq('http://foo:baz@test.com') end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index bc32372d3a9..a56b913198c 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -522,7 +522,6 @@ describe API::Internal do context 'the project path was changed' do let(:project) { create(:project, :repository, :legacy_storage) } - let!(:old_path_to_repo) { project.repository.path_to_repo } let!(:repository) { project.repository } before do diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index a8f003b1073..e8cbf84e3be 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -272,8 +272,11 @@ describe Projects::CreateService, '#execute' do it 'writes project full path to .git/config' do project = create_project(user, opts) + rugged = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + project.repository.rugged + end - expect(project.repository.rugged.config['gitlab.fullpath']).to eq project.full_path + expect(rugged.config['gitlab.fullpath']).to eq project.full_path end def create_project(user, opts) diff --git a/spec/support/gitaly.rb b/spec/support/gitaly.rb index 5a1dd44bc9d..614aaa73693 100644 --- a/spec/support/gitaly.rb +++ b/spec/support/gitaly.rb @@ -9,7 +9,7 @@ RSpec.configure do |config| # Use 'and_wrap_original' to make sure the arguments are valid allow(Gitlab::GitalyClient).to receive(:feature_enabled?).and_wrap_original do |m, *args| m.call(*args) - !Gitlab::GitalyClient::EXPLICIT_OPT_IN_REQUIRED.include?(args.first) + !Gitlab::GitalyClient.explicit_opt_in_required.include?(args.first) end end end diff --git a/spec/workers/repository_remove_remote_worker_spec.rb b/spec/workers/repository_remove_remote_worker_spec.rb index f22d7c1d073..5968c5da3c9 100644 --- a/spec/workers/repository_remove_remote_worker_spec.rb +++ b/spec/workers/repository_remove_remote_worker_spec.rb @@ -44,7 +44,9 @@ describe RepositoryRemoveRemoteWorker do end def create_remote_branch(remote_name, branch_name, target) - rugged = project.repository.rugged + rugged = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + project.repository.rugged + end rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", target.id) end end