From e74db6bfa85dbeb243dafcdbf03c0e5aff3f6069 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 29 Jun 2022 14:30:51 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-1-stable-ee --- .../projects/settings/access_dropdown.js | 2 +- app/models/clusters/applications/runner.rb | 2 +- app/policies/project_policy.rb | 26 ++++- .../container_registry_authentication_service.rb | 6 +- .../projects/graphql/get_project_query.rb | 12 --- .../transformers/project_attributes_transformer.rb | 14 +-- .../decompressed_archive_size_validator.rb | 20 +++- .../projects/settings/access_dropdown_spec.js | 17 +++ .../projects/pipelines/project_pipeline_spec.rb | 26 +---- .../project_attributes_transformer_spec.rb | 21 +++- .../decompressed_archive_size_validator_spec.rb | 59 +++++++++++ spec/policies/project_policy_spec.rb | 118 ++++++++++++++++++--- .../file_decompression_service_spec.rb | 3 +- .../project_features_shared_context.rb | 28 +++++ .../policies/project_policy_shared_examples.rb | 13 +-- ...tainer_registry_auth_service_shared_examples.rb | 6 +- 16 files changed, 292 insertions(+), 81 deletions(-) create mode 100644 spec/support/shared_contexts/project_features_shared_context.rb diff --git a/app/assets/javascripts/projects/settings/access_dropdown.js b/app/assets/javascripts/projects/settings/access_dropdown.js index 7fb7a416dca..79dfa166b1a 100644 --- a/app/assets/javascripts/projects/settings/access_dropdown.js +++ b/app/assets/javascripts/projects/settings/access_dropdown.js @@ -537,7 +537,7 @@ export default class AccessDropdown { return `
  • - ${key.title} + ${escape(key.title)}

    ${sprintf( __('Owned by %{image_tag}'), diff --git a/app/models/clusters/applications/runner.rb b/app/models/clusters/applications/runner.rb index bed0eab5a58..1ac4cbac1da 100644 --- a/app/models/clusters/applications/runner.rb +++ b/app/models/clusters/applications/runner.rb @@ -3,7 +3,7 @@ module Clusters module Applications class Runner < ApplicationRecord - VERSION = '0.41.0' + VERSION = '0.42.1' self.table_name = 'clusters_applications_runners' diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 6ddd83544bc..2594310c498 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -59,7 +59,13 @@ class ProjectPolicy < BasePolicy desc "Container registry is disabled" condition(:container_registry_disabled, scope: :subject) do - !access_allowed_to?(:container_registry) + if user.is_a?(DeployToken) + (!user.read_registry? && !user.write_registry?) || + user.revoked? || + !project.container_registry_enabled? + else + !access_allowed_to?(:container_registry) + end end desc "Container registry is enabled for everyone with access to the project" @@ -88,6 +94,16 @@ class ProjectPolicy < BasePolicy user.is_a?(DeployKey) && user.can_push_to?(project) end + desc "Deploy token with read_container_image scope" + condition(:read_container_image_deploy_token) do + user.is_a?(DeployToken) && user.has_access_to?(project) && user.read_registry? + end + + desc "Deploy token with create_container_image scope" + condition(:create_container_image_deploy_token) do + user.is_a?(DeployToken) && user.has_access_to?(project) && user.write_registry? + end + desc "Deploy token with read_package_registry scope" condition(:read_package_registry_deploy_token) do user.is_a?(DeployToken) && user.has_access_to?(project) && user.read_package_registry @@ -697,6 +713,14 @@ class ProjectPolicy < BasePolicy enable :push_code end + rule { read_container_image_deploy_token }.policy do + enable :read_container_image + end + + rule { create_container_image_deploy_token }.policy do + enable :create_container_image + end + rule { read_package_registry_deploy_token }.policy do enable :read_package enable :read_project diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb index 6d6d8641d9d..e806bef46fe 100644 --- a/app/services/auth/container_registry_authentication_service.rb +++ b/app/services/auth/container_registry_authentication_service.rb @@ -215,15 +215,13 @@ module Auth def deploy_token_can_pull?(requested_project) has_authentication_ability?(:read_container_image) && deploy_token.present? && - deploy_token.has_access_to?(requested_project) && - deploy_token.read_registry? + can?(deploy_token, :read_container_image, requested_project) end def deploy_token_can_push?(requested_project) has_authentication_ability?(:create_container_image) && deploy_token.present? && - deploy_token.has_access_to?(requested_project) && - deploy_token.write_registry? + can?(deploy_token, :create_container_image, requested_project) end ## diff --git a/lib/bulk_imports/projects/graphql/get_project_query.rb b/lib/bulk_imports/projects/graphql/get_project_query.rb index b3d7f3f4683..76475893ac1 100644 --- a/lib/bulk_imports/projects/graphql/get_project_query.rb +++ b/lib/bulk_imports/projects/graphql/get_project_query.rb @@ -10,20 +10,8 @@ module BulkImports <<-'GRAPHQL' query($full_path: ID!) { project(fullPath: $full_path) { - description visibility - archived created_at: createdAt - shared_runners_enabled: sharedRunnersEnabled - container_registry_enabled: containerRegistryEnabled - only_allow_merge_if_pipeline_succeeds: onlyAllowMergeIfPipelineSucceeds - only_allow_merge_if_all_discussions_are_resolved: onlyAllowMergeIfAllDiscussionsAreResolved - request_access_enabled: requestAccessEnabled - printing_merge_request_link_enabled: printingMergeRequestLinkEnabled - remove_source_branch_after_merge: removeSourceBranchAfterMerge - autoclose_referenced_issues: autocloseReferencedIssues - suggestion_commit_message: suggestionCommitMessage - wiki_enabled: wikiEnabled } } GRAPHQL diff --git a/lib/bulk_imports/projects/transformers/project_attributes_transformer.rb b/lib/bulk_imports/projects/transformers/project_attributes_transformer.rb index 24c55d8dbb1..38730a7723b 100644 --- a/lib/bulk_imports/projects/transformers/project_attributes_transformer.rb +++ b/lib/bulk_imports/projects/transformers/project_attributes_transformer.rb @@ -7,16 +7,18 @@ module BulkImports PROJECT_IMPORT_TYPE = 'gitlab_project_migration' def transform(context, data) + project = {} entity = context.entity visibility = data.delete('visibility') - data['name'] = entity.destination_name - data['path'] = entity.destination_name.parameterize - data['import_type'] = PROJECT_IMPORT_TYPE - data['visibility_level'] = Gitlab::VisibilityLevel.string_options[visibility] if visibility.present? - data['namespace_id'] = Namespace.find_by_full_path(entity.destination_namespace)&.id if entity.destination_namespace.present? + project[:name] = entity.destination_name + project[:path] = entity.destination_name.parameterize + project[:created_at] = data['created_at'] + project[:import_type] = PROJECT_IMPORT_TYPE + project[:visibility_level] = Gitlab::VisibilityLevel.string_options[visibility] if visibility.present? + project[:namespace_id] = Namespace.find_by_full_path(entity.destination_namespace)&.id if entity.destination_namespace.present? - data.transform_keys!(&:to_sym) + project end end end diff --git a/lib/gitlab/import_export/decompressed_archive_size_validator.rb b/lib/gitlab/import_export/decompressed_archive_size_validator.rb index 61b37256964..a185eb4df1c 100644 --- a/lib/gitlab/import_export/decompressed_archive_size_validator.rb +++ b/lib/gitlab/import_export/decompressed_archive_size_validator.rb @@ -8,6 +8,8 @@ module Gitlab DEFAULT_MAX_BYTES = 10.gigabytes.freeze TIMEOUT_LIMIT = 210.seconds + ServiceError = Class.new(StandardError) + def initialize(archive_path:, max_bytes: self.class.max_bytes) @archive_path = archive_path @max_bytes = max_bytes @@ -29,6 +31,8 @@ module Gitlab pgrp = nil valid_archive = true + validate_archive_path + Timeout.timeout(TIMEOUT_LIMIT) do stdin, stdout, stderr, wait_thr = Open3.popen3(command, pgroup: true) stdin.close @@ -78,15 +82,29 @@ module Gitlab false end + def validate_archive_path + Gitlab::Utils.check_path_traversal!(@archive_path) + + raise(ServiceError, 'Archive path is not a string') unless @archive_path.is_a?(String) + raise(ServiceError, 'Archive path is a symlink') if File.lstat(@archive_path).symlink? + raise(ServiceError, 'Archive path is not a file') unless File.file?(@archive_path) + end + def command "gzip -dc #{@archive_path} | wc -c" end def log_error(error) + archive_size = begin + File.size(@archive_path) + rescue StandardError + nil + end + Gitlab::Import::Logger.info( message: error, import_upload_archive_path: @archive_path, - import_upload_archive_size: File.size(@archive_path) + import_upload_archive_size: archive_size ) end end diff --git a/spec/frontend/projects/settings/access_dropdown_spec.js b/spec/frontend/projects/settings/access_dropdown_spec.js index 65b01172e7e..d51360a7597 100644 --- a/spec/frontend/projects/settings/access_dropdown_spec.js +++ b/spec/frontend/projects/settings/access_dropdown_spec.js @@ -159,4 +159,21 @@ describe('AccessDropdown', () => { expect(template).not.toContain(user.name); }); }); + + describe('deployKeyRowHtml', () => { + const deployKey = { + id: 1, + title: 'title ', + fullname: 'fullname ', + avatar_url: '', + username: '', + }; + + it('escapes deploy key title and fullname', () => { + const template = dropdown.deployKeyRowHtml(deployKey); + + expect(template).not.toContain(deployKey.title); + expect(template).not.toContain(deployKey.fullname); + }); + }); }); diff --git a/spec/lib/bulk_imports/projects/pipelines/project_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/project_pipeline_spec.rb index c53c0849931..567a0a4fcc3 100644 --- a/spec/lib/bulk_imports/projects/pipelines/project_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/project_pipeline_spec.rb @@ -25,18 +25,7 @@ RSpec.describe BulkImports::Projects::Pipelines::ProjectPipeline do let(:project_data) do { 'visibility' => 'private', - 'created_at' => 10.days.ago, - 'archived' => false, - 'shared_runners_enabled' => true, - 'container_registry_enabled' => true, - 'only_allow_merge_if_pipeline_succeeds' => true, - 'only_allow_merge_if_all_discussions_are_resolved' => true, - 'request_access_enabled' => true, - 'printing_merge_request_link_enabled' => true, - 'remove_source_branch_after_merge' => true, - 'autoclose_referenced_issues' => true, - 'suggestion_commit_message' => 'message', - 'wiki_enabled' => true + 'created_at' => '2016-08-12T09:41:03' } end @@ -58,17 +47,8 @@ RSpec.describe BulkImports::Projects::Pipelines::ProjectPipeline do expect(imported_project).not_to be_nil expect(imported_project.group).to eq(group) - expect(imported_project.suggestion_commit_message).to eq('message') - expect(imported_project.archived?).to eq(project_data['archived']) - expect(imported_project.shared_runners_enabled?).to eq(project_data['shared_runners_enabled']) - expect(imported_project.container_registry_enabled?).to eq(project_data['container_registry_enabled']) - expect(imported_project.only_allow_merge_if_pipeline_succeeds?).to eq(project_data['only_allow_merge_if_pipeline_succeeds']) - expect(imported_project.only_allow_merge_if_all_discussions_are_resolved?).to eq(project_data['only_allow_merge_if_all_discussions_are_resolved']) - expect(imported_project.request_access_enabled?).to eq(project_data['request_access_enabled']) - expect(imported_project.printing_merge_request_link_enabled?).to eq(project_data['printing_merge_request_link_enabled']) - expect(imported_project.remove_source_branch_after_merge?).to eq(project_data['remove_source_branch_after_merge']) - expect(imported_project.autoclose_referenced_issues?).to eq(project_data['autoclose_referenced_issues']) - expect(imported_project.wiki_enabled?).to eq(project_data['wiki_enabled']) + expect(imported_project.visibility).to eq(project_data['visibility']) + expect(imported_project.created_at).to eq(project_data['created_at']) end end diff --git a/spec/lib/bulk_imports/projects/transformers/project_attributes_transformer_spec.rb b/spec/lib/bulk_imports/projects/transformers/project_attributes_transformer_spec.rb index 822bb9a5605..a1d77b9732d 100644 --- a/spec/lib/bulk_imports/projects/transformers/project_attributes_transformer_spec.rb +++ b/spec/lib/bulk_imports/projects/transformers/project_attributes_transformer_spec.rb @@ -25,8 +25,8 @@ RSpec.describe BulkImports::Projects::Transformers::ProjectAttributesTransformer let(:data) do { - 'name' => 'source_name', - 'visibility' => 'private' + 'visibility' => 'private', + 'created_at' => '2016-11-18T09:29:42.634Z' } end @@ -76,8 +76,21 @@ RSpec.describe BulkImports::Projects::Transformers::ProjectAttributesTransformer end end - it 'converts all keys to symbols' do - expect(transformed_data.keys).to contain_exactly(:name, :path, :import_type, :visibility_level, :namespace_id) + context 'when data has extra keys' do + it 'returns a fixed number of keys' do + data = { + 'visibility' => 'private', + 'created_at' => '2016-11-18T09:29:42.634Z', + 'my_key' => 'my_key', + 'another_key' => 'another_key', + 'last_key' => 'last_key' + } + + transformed_data = described_class.new.transform(context, data) + + expect(transformed_data.keys) + .to contain_exactly(:created_at, :import_type, :name, :namespace_id, :path, :visibility_level) + end end end end diff --git a/spec/lib/gitlab/import_export/decompressed_archive_size_validator_spec.rb b/spec/lib/gitlab/import_export/decompressed_archive_size_validator_spec.rb index fe3b638d20f..dea584e5019 100644 --- a/spec/lib/gitlab/import_export/decompressed_archive_size_validator_spec.rb +++ b/spec/lib/gitlab/import_export/decompressed_archive_size_validator_spec.rb @@ -86,6 +86,65 @@ RSpec.describe Gitlab::ImportExport::DecompressedArchiveSizeValidator do include_examples 'logs raised exception and terminates validator process group' end end + + context 'archive path validation' do + let(:filesize) { nil } + + before do + expect(Gitlab::Import::Logger) + .to receive(:info) + .with( + import_upload_archive_path: filepath, + import_upload_archive_size: filesize, + message: error_message + ) + end + + context 'when archive path is traversed' do + let(:filepath) { '/foo/../bar' } + let(:error_message) { 'Invalid path' } + + it 'returns false' do + expect(subject.valid?).to eq(false) + end + end + + context 'when archive path is not a string' do + let(:filepath) { 123 } + let(:error_message) { 'Archive path is not a string' } + + it 'returns false' do + expect(subject.valid?).to eq(false) + end + end + + context 'which archive path is a symlink' do + let(:filepath) { File.join(Dir.tmpdir, 'symlink') } + let(:error_message) { 'Archive path is a symlink' } + + before do + FileUtils.ln_s(filepath, filepath, force: true) + end + + it 'returns false' do + expect(subject.valid?).to eq(false) + end + end + + context 'when archive path is not a file' do + let(:filepath) { Dir.mktmpdir } + let(:filesize) { File.size(filepath) } + let(:error_message) { 'Archive path is not a file' } + + after do + FileUtils.rm_rf(filepath) + end + + it 'returns false' do + expect(subject.valid?).to eq(false) + end + end + end end def create_compressed_file diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 59fe601ed43..d363a822d18 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -1080,25 +1080,117 @@ RSpec.describe ProjectPolicy do subject { described_class.new(deploy_token, project) } - context 'a deploy token with read_package_registry scope' do - let(:deploy_token) { create(:deploy_token, read_package_registry: true) } + context 'private project' do + let(:project) { private_project } - it { is_expected.to be_allowed(:read_package) } - it { is_expected.to be_allowed(:read_project) } - it { is_expected.to be_disallowed(:create_package) } + context 'a deploy token with read_registry scope' do + let(:deploy_token) { create(:deploy_token, read_registry: true, write_registry: false) } - it_behaves_like 'package access with repository disabled' + it { is_expected.to be_allowed(:read_container_image) } + it { is_expected.to be_disallowed(:create_container_image) } + + context 'with registry disabled' do + include_context 'registry disabled via project features' + + it { is_expected.to be_disallowed(:read_container_image) } + it { is_expected.to be_disallowed(:create_container_image) } + end + end + + context 'a deploy token with write_registry scope' do + let(:deploy_token) { create(:deploy_token, read_registry: false, write_registry: true) } + + it { is_expected.to be_disallowed(:read_container_image) } + it { is_expected.to be_allowed(:create_container_image) } + + context 'with registry disabled' do + include_context 'registry disabled via project features' + + it { is_expected.to be_disallowed(:read_container_image) } + it { is_expected.to be_disallowed(:create_container_image) } + end + end + + context 'a deploy token with no registry scope' do + let(:deploy_token) { create(:deploy_token, read_registry: false, write_registry: false) } + + it { is_expected.to be_disallowed(:read_container_image) } + it { is_expected.to be_disallowed(:create_container_image) } + end + + context 'a deploy token with read_package_registry scope' do + let(:deploy_token) { create(:deploy_token, read_repository: false, read_registry: false, read_package_registry: true) } + + it { is_expected.to be_allowed(:read_project) } + it { is_expected.to be_allowed(:read_package) } + it { is_expected.to be_disallowed(:create_package) } + + it_behaves_like 'package access with repository disabled' + end + + context 'a deploy token with write_package_registry scope' do + let(:deploy_token) { create(:deploy_token, read_repository: false, read_registry: false, write_package_registry: true) } + + it { is_expected.to be_allowed(:create_package) } + it { is_expected.to be_allowed(:read_package) } + it { is_expected.to be_allowed(:read_project) } + it { is_expected.to be_disallowed(:destroy_package) } + + it_behaves_like 'package access with repository disabled' + end end - context 'a deploy token with write_package_registry scope' do - let(:deploy_token) { create(:deploy_token, write_package_registry: true) } + context 'public project' do + let(:project) { public_project } + + context 'a deploy token with read_registry scope' do + let(:deploy_token) { create(:deploy_token, read_registry: true, write_registry: false) } - it { is_expected.to be_allowed(:create_package) } - it { is_expected.to be_allowed(:read_package) } - it { is_expected.to be_allowed(:read_project) } - it { is_expected.to be_disallowed(:destroy_package) } + it { is_expected.to be_allowed(:read_container_image) } + it { is_expected.to be_disallowed(:create_container_image) } - it_behaves_like 'package access with repository disabled' + context 'with registry disabled' do + include_context 'registry disabled via project features' + + it { is_expected.to be_disallowed(:read_container_image) } + it { is_expected.to be_disallowed(:create_container_image) } + end + + context 'with registry private' do + include_context 'registry set to private via project features' + + it { is_expected.to be_allowed(:read_container_image) } + it { is_expected.to be_disallowed(:create_container_image) } + end + end + + context 'a deploy token with write_registry scope' do + let(:deploy_token) { create(:deploy_token, read_registry: false, write_registry: true) } + + it { is_expected.to be_allowed(:read_container_image) } + it { is_expected.to be_allowed(:create_container_image) } + + context 'with registry disabled' do + include_context 'registry disabled via project features' + + it { is_expected.to be_disallowed(:read_container_image) } + it { is_expected.to be_disallowed(:create_container_image) } + end + + context 'with registry private' do + include_context 'registry set to private via project features' + + it { is_expected.to be_allowed(:read_container_image) } + it { is_expected.to be_allowed(:create_container_image) } + end + end + + context 'a deploy token with no registry scope' do + let(:deploy_token) { create(:deploy_token, read_registry: false, write_registry: false) } + + it { is_expected.to be_disallowed(:read_container_image) } + it { is_expected.to be_disallowed(:create_container_image) } + end end end diff --git a/spec/services/bulk_imports/file_decompression_service_spec.rb b/spec/services/bulk_imports/file_decompression_service_spec.rb index 1d6aa79a37f..77348428d60 100644 --- a/spec/services/bulk_imports/file_decompression_service_spec.rb +++ b/spec/services/bulk_imports/file_decompression_service_spec.rb @@ -80,7 +80,8 @@ RSpec.describe BulkImports::FileDecompressionService do subject { described_class.new(tmpdir: tmpdir, filename: 'symlink.gz') } it 'raises an error and removes the file' do - expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid file') + expect { subject.execute } + .to raise_error(BulkImports::FileDecompressionService::ServiceError, 'File decompression error') expect(File.exist?(symlink)).to eq(false) end diff --git a/spec/support/shared_contexts/project_features_shared_context.rb b/spec/support/shared_contexts/project_features_shared_context.rb new file mode 100644 index 00000000000..40d9cb29c14 --- /dev/null +++ b/spec/support/shared_contexts/project_features_shared_context.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +RSpec.shared_context 'repository disabled via project features' do + before do + project.project_feature.update_columns( + # Disable merge_requests and builds as well, since merge_requests and + # builds cannot have higher visibility than repository. + merge_requests_access_level: ProjectFeature::DISABLED, + builds_access_level: ProjectFeature::DISABLED, + repository_access_level: ProjectFeature::DISABLED) + end +end + +RSpec.shared_context 'registry disabled via project features' do + before do + project.project_feature.update_columns( + container_registry_access_level: ProjectFeature::DISABLED + ) + end +end + +RSpec.shared_context 'registry set to private via project features' do + before do + project.project_feature.update_columns( + container_registry_access_level: ProjectFeature::PRIVATE + ) + end +end diff --git a/spec/support/shared_examples/policies/project_policy_shared_examples.rb b/spec/support/shared_examples/policies/project_policy_shared_examples.rb index 63e4d458ad4..c4083df47e2 100644 --- a/spec/support/shared_examples/policies/project_policy_shared_examples.rb +++ b/spec/support/shared_examples/policies/project_policy_shared_examples.rb @@ -345,16 +345,7 @@ RSpec.shared_examples 'project policies as admin without admin mode' do end RSpec.shared_examples 'package access with repository disabled' do - context 'when repository is disabled' do - before do - project.project_feature.update!( - # Disable merge_requests and builds as well, since merge_requests and - # builds cannot have higher visibility than repository. - merge_requests_access_level: ProjectFeature::DISABLED, - builds_access_level: ProjectFeature::DISABLED, - repository_access_level: ProjectFeature::DISABLED) - end + include_context 'repository disabled via project features' - it { is_expected.to be_allowed(:read_package) } - end + it { is_expected.to be_allowed(:read_package) } end diff --git a/spec/support/shared_examples/services/container_registry_auth_service_shared_examples.rb b/spec/support/shared_examples/services/container_registry_auth_service_shared_examples.rb index 7677e5d8cb2..f18869fb380 100644 --- a/spec/support/shared_examples/services/container_registry_auth_service_shared_examples.rb +++ b/spec/support/shared_examples/services/container_registry_auth_service_shared_examples.rb @@ -142,9 +142,9 @@ RSpec.shared_examples 'logs an auth warning' do |requested_actions| requested_project_path: project.full_path, requested_actions: requested_actions, authorized_actions: [], - user_id: current_user.id, - username: current_user.username - } + user_id: current_user&.id, + username: current_user&.username + }.compact end it do -- cgit v1.2.1