diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-09-20 04:44:54 +0000 |
---|---|---|
committer | Ruben Davila <rdavila84@gmail.com> | 2016-09-21 16:20:08 -0500 |
commit | 62ba6805912c45fa06aafef5f7b2be696a6758b6 (patch) | |
tree | 99b7d808e9c91fe6fca64e1c778d75749f9efc5e | |
parent | c525ec9bc82f3ddd325d2fa2a8850a600ca3c70b (diff) | |
download | gitlab-ce-62ba6805912c45fa06aafef5f7b2be696a6758b6.tar.gz |
Merge branch 'feature/import-export-security-specs' into 'master'
Import/Export security specs
Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/20857
Related: https://gitlab.com/gitlab-org/gitlab-ce/issues/20821/
See merge request !1987
8 files changed, 840 insertions, 1 deletions
diff --git a/spec/features/projects/import_export/export_file_spec.rb b/spec/features/projects/import_export/export_file_spec.rb new file mode 100644 index 00000000000..7e2c701e401 --- /dev/null +++ b/spec/features/projects/import_export/export_file_spec.rb @@ -0,0 +1,80 @@ +require 'spec_helper' + +# Integration test that exports a file using the Import/Export feature +# It looks up for any sensitive word inside the JSON, so if a sensitive word is found +# we''l have to either include it adding the model that includes it to the +safe_list+ +# or make sure the attribute is blacklisted in the +import_export.yml+ configuration +feature 'Import/Export - project export integration test', feature: true, js: true do + include Select2Helper + include ExportFileHelper + + let(:user) { create(:admin) } + let(:export_path) { "#{Dir::tmpdir}/import_file_spec" } + let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys } + + let(:sensitive_words) { %w[pass secret token key] } + let(:safe_list) do + { + token: [ProjectHook, Ci::Trigger], + key: [Project, Ci::Variable, :yaml_variables] + } + end + let(:safe_hashes) { { yaml_variables: %w[key value public] } } + + let(:project) { setup_project } + + background do + allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) + end + + after do + FileUtils.rm_rf(export_path, secure: true) + end + + context 'admin user' do + before do + login_as(user) + end + + scenario 'exports a project successfully' do + visit edit_namespace_project_path(project.namespace, project) + + expect(page).to have_content('Export project') + + click_link 'Export project' + + visit edit_namespace_project_path(project.namespace, project) + + expect(page).to have_content('Download export') + + in_directory_with_expanded_export(project) do |exit_status, tmpdir| + expect(exit_status).to eq(0) + + project_json_path = File.join(tmpdir, 'project.json') + expect(File).to exist(project_json_path) + + project_hash = JSON.parse(IO.read(project_json_path)) + + sensitive_words.each do |sensitive_word| + found = find_sensitive_attributes(sensitive_word, project_hash) + + expect(found).to be_nil, failure_message(found.try(:key_found), found.try(:parent), sensitive_word) + end + end + end + + def failure_message(key_found, parent, sensitive_word) + <<-MSG + Found a new sensitive word <#{key_found}>, which is part of the hash #{parent.inspect} + + If you think this information shouldn't get exported, please exclude the model or attribute in IMPORT_EXPORT_CONFIG. + + Otherwise, please add the exception to +safe_list+ in CURRENT_SPEC using #{sensitive_word} as the key and the + correspondent hash or model as the value. + + IMPORT_EXPORT_CONFIG: #{Gitlab::ImportExport.config_file} + CURRENT_SPEC: #{__FILE__} + MSG + end + end +end diff --git a/spec/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb index f707ccf4e93..09cd6369881 100644 --- a/spec/features/projects/import_export/import_file_spec.rb +++ b/spec/features/projects/import_export/import_file_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -feature 'project import', feature: true, js: true do +feature 'Import/Export - project import integration test', feature: true, js: true do include Select2Helper let(:admin) { create(:admin) } diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml new file mode 100644 index 00000000000..30968ba2d5f --- /dev/null +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -0,0 +1,175 @@ +--- +issues: +- subscriptions +- award_emoji +- author +- assignee +- updated_by +- milestone +- notes +- label_links +- labels +- todos +- user_agent_detail +- moved_to +- events +events: +- author +- project +- target +notes: +- award_emoji +- project +- noteable +- author +- updated_by +- resolved_by +- todos +- events +label_links: +- target +- label +label: +- subscriptions +- project +- lists +- label_links +- issues +- merge_requests +milestone: +- project +- issues +- labels +- merge_requests +- participants +- events +snippets: +- author +- project +- notes +releases: +- project +project_members: +- created_by +- user +- source +- project +merge_requests: +- subscriptions +- award_emoji +- author +- assignee +- updated_by +- milestone +- notes +- label_links +- labels +- todos +- target_project +- source_project +- merge_user +- merge_request_diffs +- merge_request_diff +- events +merge_request_diff: +- merge_request +pipelines: +- project +- user +- statuses +- builds +- trigger_requests +statuses: +- project +- pipeline +- user +variables: +- project +triggers: +- project +- trigger_requests +deploy_keys: +- user +- deploy_keys_projects +- projects +services: +- project +- service_hook +hooks: +- project +protected_branches: +- project +- merge_access_levels +- push_access_levels +project: +- taggings +- base_tags +- tag_taggings +- tags +- creator +- group +- namespace +- board +- last_event +- services +- campfire_service +- drone_ci_service +- emails_on_push_service +- builds_email_service +- irker_service +- pivotaltracker_service +- hipchat_service +- flowdock_service +- assembla_service +- asana_service +- gemnasium_service +- slack_service +- buildkite_service +- bamboo_service +- teamcity_service +- pushover_service +- jira_service +- redmine_service +- custom_issue_tracker_service +- bugzilla_service +- gitlab_issue_tracker_service +- external_wiki_service +- forked_project_link +- forked_from_project +- forked_project_links +- forks +- merge_requests +- fork_merge_requests +- issues +- labels +- events +- milestones +- notes +- snippets +- hooks +- protected_branches +- project_members +- users +- requesters +- deploy_keys_projects +- deploy_keys +- users_star_projects +- starrers +- releases +- lfs_objects_projects +- lfs_objects +- project_group_links +- invited_groups +- todos +- notification_settings +- import_data +- commit_statuses +- pipelines +- builds +- runner_projects +- runners +- variables +- triggers +- environments +- deployments +- project_feature
\ No newline at end of file diff --git a/spec/lib/gitlab/import_export/attribute_configuration_spec.rb b/spec/lib/gitlab/import_export/attribute_configuration_spec.rb new file mode 100644 index 00000000000..2ba344092ce --- /dev/null +++ b/spec/lib/gitlab/import_export/attribute_configuration_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' + +# Part of the test security suite for the Import/Export feature +# Checks whether there are new attributes in models that are currently being exported as part of the +# project Import/Export feature. +# If there are new attributes, these will have to either be added to this spec in case we want them +# to be included as part of the export, or blacklist them using the import_export.yml configuration file. +# Likewise, new models added to import_export.yml, will need to be added with their correspondent attributes +# to this spec. +describe 'Import/Export attribute configuration', lib: true do + include ConfigurationHelper + + let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys } + let(:relation_names) do + names = names_from_tree(config_hash['project_tree']) + + # Remove duplicated or add missing models + # - project is not part of the tree, so it has to be added manually. + # - milestone, labels have both singular and plural versions in the tree, so remove the duplicates. + names.flatten.uniq - ['milestones', 'labels'] + ['project'] + end + + let(:safe_attributes_file) { 'spec/lib/gitlab/import_export/safe_model_attributes.yml' } + let(:safe_model_attributes) { YAML.load_file(safe_attributes_file) } + + it 'has no new columns' do + relation_names.each do |relation_name| + relation_class = relation_class_for_name(relation_name) + + expect(safe_model_attributes[relation_class.to_s]).not_to be_nil, "Expected exported class #{relation_class.to_s} to exist in safe_model_attributes" + + current_attributes = parsed_attributes(relation_name, relation_class.attribute_names) + safe_attributes = safe_model_attributes[relation_class.to_s] + new_attributes = current_attributes - safe_attributes + + expect(new_attributes).to be_empty, failure_message(relation_class.to_s, new_attributes) + end + end + + def failure_message(relation_class, new_attributes) + <<-MSG + It looks like #{relation_class}, which is exported using the project Import/Export, has new attributes: #{new_attributes.join(',')} + + Please add the attribute(s) to SAFE_MODEL_ATTRIBUTES if you consider this can be exported. + Otherwise, please blacklist the attribute(s) in IMPORT_EXPORT_CONFIG by adding it to its correspondent + model in the +excluded_attributes+ section. + + SAFE_MODEL_ATTRIBUTES: #{File.expand_path(safe_attributes_file)} + IMPORT_EXPORT_CONFIG: #{Gitlab::ImportExport.config_file} + MSG + end + + class Author < User + end +end diff --git a/spec/lib/gitlab/import_export/model_configuration_spec.rb b/spec/lib/gitlab/import_export/model_configuration_spec.rb new file mode 100644 index 00000000000..9b492d1b9c7 --- /dev/null +++ b/spec/lib/gitlab/import_export/model_configuration_spec.rb @@ -0,0 +1,57 @@ +require 'spec_helper' + +# Part of the test security suite for the Import/Export feature +# Finds if a new model has been added that can potentially be part of the Import/Export +# If it finds a new model, it will show a +failure_message+ with the options available. +describe 'Import/Export model configuration', lib: true do + include ConfigurationHelper + + let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys } + let(:model_names) do + names = names_from_tree(config_hash['project_tree']) + + # Remove duplicated or add missing models + # - project is not part of the tree, so it has to be added manually. + # - milestone, labels have both singular and plural versions in the tree, so remove the duplicates. + # - User, Author... Models we do not care about for checking models + names.flatten.uniq - ['milestones', 'labels', 'user', 'author'] + ['project'] + end + + let(:all_models_yml) { 'spec/lib/gitlab/import_export/all_models.yml' } + let(:all_models) { YAML.load_file(all_models_yml) } + let(:current_models) { setup_models } + + it 'has no new models' do + model_names.each do |model_name| + new_models = Array(current_models[model_name]) - Array(all_models[model_name]) + expect(new_models).to be_empty, failure_message(model_name.classify, new_models) + end + end + + # List of current models between models, in the format of + # {model: [model_2, model3], ...} + def setup_models + all_models_hash = {} + + model_names.each do |model_name| + model_class = relation_class_for_name(model_name) + + all_models_hash[model_name] = associations_for(model_class) - ['project'] + end + + all_models_hash + end + + def failure_message(parent_model_name, new_models) + <<-MSG + New model(s) <#{new_models.join(',')}> have been added, related to #{parent_model_name}, which is exported by + the Import/Export feature. + + If you think this model should be included in the export, please add it to IMPORT_EXPORT_CONFIG. + Definitely add it to MODELS_JSON to signal that you've handled this error and to prevent it from showing up in the future. + + MODELS_JSON: #{File.expand_path(all_models_yml)} + IMPORT_EXPORT_CONFIG: #{Gitlab::ImportExport.config_file} + MSG + end +end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml new file mode 100644 index 00000000000..f2d272ca7e2 --- /dev/null +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -0,0 +1,310 @@ +--- +Issue: +- id +- title +- assignee_id +- author_id +- project_id +- created_at +- updated_at +- position +- branch_name +- description +- state +- iid +- updated_by_id +- confidential +- deleted_at +- due_date +- moved_to_id +- lock_version +- milestone_id +- weight +Event: +- id +- target_type +- target_id +- title +- data +- project_id +- created_at +- updated_at +- action +- author_id +Note: +- id +- note +- noteable_type +- author_id +- created_at +- updated_at +- project_id +- attachment +- line_code +- commit_id +- noteable_id +- system +- st_diff +- updated_by_id +- type +- position +- original_position +- resolved_at +- resolved_by_id +- discussion_id +- original_discussion_id +LabelLink: +- id +- label_id +- target_id +- target_type +- created_at +- updated_at +Label: +- id +- title +- color +- project_id +- created_at +- updated_at +- template +- description +- priority +Milestone: +- id +- title +- project_id +- description +- due_date +- created_at +- updated_at +- state +- iid +ProjectSnippet: +- id +- title +- content +- author_id +- project_id +- created_at +- updated_at +- file_name +- type +- visibility_level +Release: +- id +- tag +- description +- project_id +- created_at +- updated_at +ProjectMember: +- id +- access_level +- source_id +- source_type +- user_id +- notification_level +- type +- created_at +- updated_at +- created_by_id +- invite_email +- invite_token +- invite_accepted_at +- requested_at +- expires_at +User: +- id +- username +- email +MergeRequest: +- id +- target_branch +- source_branch +- source_project_id +- author_id +- assignee_id +- title +- created_at +- updated_at +- state +- merge_status +- target_project_id +- iid +- description +- position +- locked_at +- updated_by_id +- merge_error +- merge_params +- merge_when_build_succeeds +- merge_user_id +- merge_commit_sha +- deleted_at +- in_progress_merge_commit_sha +- lock_version +- milestone_id +- approvals_before_merge +- rebase_commit_sha +MergeRequestDiff: +- id +- state +- st_commits +- merge_request_id +- created_at +- updated_at +- base_commit_sha +- real_size +- head_commit_sha +- start_commit_sha +Ci::Pipeline: +- id +- project_id +- ref +- sha +- before_sha +- push_data +- created_at +- updated_at +- tag +- yaml_errors +- committed_at +- gl_project_id +- status +- started_at +- finished_at +- duration +- user_id +CommitStatus: +- id +- project_id +- status +- finished_at +- trace +- created_at +- updated_at +- started_at +- runner_id +- coverage +- commit_id +- commands +- job_id +- name +- deploy +- options +- allow_failure +- stage +- trigger_request_id +- stage_idx +- tag +- ref +- user_id +- type +- target_url +- description +- artifacts_file +- gl_project_id +- artifacts_metadata +- erased_by_id +- erased_at +- artifacts_expire_at +- environment +- artifacts_size +- when +- yaml_variables +- queued_at +Ci::Variable: +- id +- project_id +- key +- value +- encrypted_value +- encrypted_value_salt +- encrypted_value_iv +- gl_project_id +Ci::Trigger: +- id +- token +- project_id +- deleted_at +- created_at +- updated_at +- gl_project_id +DeployKey: +- id +- user_id +- created_at +- updated_at +- key +- title +- type +- fingerprint +- public +Service: +- id +- type +- title +- project_id +- created_at +- updated_at +- active +- properties +- template +- push_events +- issues_events +- merge_requests_events +- tag_push_events +- note_events +- pipeline_events +- build_events +- category +- default +- wiki_page_events +- confidential_issues_events +ProjectHook: +- id +- url +- project_id +- created_at +- updated_at +- type +- service_id +- push_events +- issues_events +- merge_requests_events +- tag_push_events +- note_events +- pipeline_events +- enable_ssl_verification +- build_events +- wiki_page_events +- token +- group_id +- confidential_issues_events +ProtectedBranch: +- id +- project_id +- name +- created_at +- updated_at +Project: +- description +- issues_enabled +- merge_requests_enabled +- wiki_enabled +- snippets_enabled +- visibility_level +- archived +Author: +- name +ProjectFeature: +- id +- project_id +- merge_requests_access_level +- issues_access_level +- wiki_access_level +- snippets_access_level +- builds_access_level +- created_at +- updated_at
\ No newline at end of file diff --git a/spec/support/import_export/configuration_helper.rb b/spec/support/import_export/configuration_helper.rb new file mode 100644 index 00000000000..f752508d48c --- /dev/null +++ b/spec/support/import_export/configuration_helper.rb @@ -0,0 +1,29 @@ +module ConfigurationHelper + # Returns a list of models from hashes/arrays contained in +project_tree+ + def names_from_tree(project_tree) + project_tree.map do |branch_or_model| + branch_or_model = branch_or_model.to_s if branch_or_model.is_a?(Symbol) + + branch_or_model.is_a?(String) ? branch_or_model : names_from_tree(branch_or_model) + end + end + + def relation_class_for_name(relation_name) + relation_name = Gitlab::ImportExport::RelationFactory::OVERRIDES[relation_name.to_sym] || relation_name + relation_name.to_s.classify.constantize + end + + def parsed_attributes(relation_name, attributes) + excluded_attributes = config_hash['excluded_attributes'][relation_name] + included_attributes = config_hash['included_attributes'][relation_name] + + attributes = attributes - JSON[excluded_attributes.to_json] if excluded_attributes + attributes = attributes & JSON[included_attributes.to_json] if included_attributes + + attributes + end + + def associations_for(safe_model) + safe_model.reflect_on_all_associations.map { |assoc| assoc.name.to_s } + end +end diff --git a/spec/support/import_export/export_file_helper.rb b/spec/support/import_export/export_file_helper.rb new file mode 100644 index 00000000000..be0772d6a4a --- /dev/null +++ b/spec/support/import_export/export_file_helper.rb @@ -0,0 +1,133 @@ +require './spec/support/import_export/configuration_helper' + +module ExportFileHelper + include ConfigurationHelper + + ObjectWithParent = Struct.new(:object, :parent, :key_found) + + def setup_project + project = create(:project, :public) + + create(:release, project: project) + + issue = create(:issue, assignee: user, project: project) + snippet = create(:project_snippet, project: project) + label = create(:label, project: project) + milestone = create(:milestone, project: project) + merge_request = create(:merge_request, source_project: project, milestone: milestone) + commit_status = create(:commit_status, project: project) + + create(:label_link, label: label, target: issue) + + ci_pipeline = create(:ci_pipeline, + project: project, + sha: merge_request.diff_head_sha, + ref: merge_request.source_branch, + statuses: [commit_status]) + + create(:ci_build, pipeline: ci_pipeline, project: project) + create(:milestone, project: project) + create(:note, noteable: issue, project: project) + create(:note, noteable: merge_request, project: project) + create(:note, noteable: snippet, project: project) + create(:note_on_commit, + author: user, + project: project, + commit_id: ci_pipeline.sha) + + create(:event, target: milestone, project: project, action: Event::CREATED, author: user) + create(:project_member, :master, user: user, project: project) + create(:ci_variable, project: project) + create(:ci_trigger, project: project) + key = create(:deploy_key) + key.projects << project + create(:service, project: project) + create(:project_hook, project: project, token: 'token') + create(:protected_branch, project: project) + + project + end + + # Expands the compressed file for an exported project into +tmpdir+ + def in_directory_with_expanded_export(project) + Dir.mktmpdir do |tmpdir| + export_file = project.export_project_path + _output, exit_status = Gitlab::Popen.popen(%W{tar -zxf #{export_file} -C #{tmpdir}}) + + yield(exit_status, tmpdir) + end + end + + # Recursively finds key/values including +key+ as part of the key, inside a nested hash + def deep_find_with_parent(sensitive_key_word, object, found = nil) + sensitive_key_found = object_contains_key?(object, sensitive_key_word) + + # Returns the parent object and the object found containing a sensitive word as part of the key + if sensitive_key_found && object[sensitive_key_found] + ObjectWithParent.new(object[sensitive_key_found], object, sensitive_key_found) + elsif object.is_a?(Enumerable) + # Recursively lookup for keys containing sensitive words in a Hash or Array + object_with_parent = nil + + object.find do |*hash_or_array| + object_with_parent = deep_find_with_parent(sensitive_key_word, hash_or_array.last, found) + end + + object_with_parent + end + end + + # Return true if the hash has a key containing a sensitive word + def object_contains_key?(object, sensitive_key_word) + return false unless object.is_a?(Hash) + + object.keys.find { |key| key.include?(sensitive_key_word) } + end + + # Returns the offended ObjectWithParent object if a sensitive word is found inside a hash, + # excluding the whitelisted safe hashes. + def find_sensitive_attributes(sensitive_word, project_hash) + loop do + object_with_parent = deep_find_with_parent(sensitive_word, project_hash) + + return nil unless object_with_parent && object_with_parent.object + + if is_safe_hash?(object_with_parent.parent, sensitive_word) + # It's in the safe list, remove hash and keep looking + object_with_parent.parent.delete(object_with_parent.key_found) + else + return object_with_parent + end + + nil + end + end + + # Returns true if it's one of the excluded models in +safe_list+ + def is_safe_hash?(parent, sensitive_word) + return false unless parent && safe_list[sensitive_word.to_sym] + + # Extra attributes that appear in a model but not in the exported hash. + excluded_attributes = ['type'] + + safe_list[sensitive_word.to_sym].each do |model| + # Check whether this is a hash attribute inside a model + if model.is_a?(Symbol) + return true if (safe_hashes[model] - parent.keys).empty? + else + return true if safe_model?(model, excluded_attributes, parent) + end + end + + false + end + + # Compares model attributes with those those found in the hash + # and returns true if there is a match, ignoring some excluded attributes. + def safe_model?(model, excluded_attributes, parent) + excluded_attributes += associations_for(model) + parsed_model_attributes = parsed_attributes(model.name.underscore, model.attribute_names) + + (parsed_model_attributes - parent.keys - excluded_attributes).empty? + end +end |