From 9cf62118390c0cfba3d36a4231a30a7836f06e2f Mon Sep 17 00:00:00 2001 From: Laura Montemayor <lmontemayor@gitlab.com> Date: Thu, 31 Mar 2022 00:01:42 +0000 Subject: [PATCH] Masks variables in error messages Merge branch 'security-lm-mask-variables-in-error-14-9' into '14-9-stable-ee' See merge request gitlab-org/security/gitlab!2308 Changelog: security --- lib/gitlab/ci/config/external/context.rb | 10 +++++++++ .../ci/config/external/file/artifact.rb | 2 +- lib/gitlab/ci/config/external/file/base.rb | 14 ++++++++---- lib/gitlab/ci/config/external/file/local.rb | 6 ++--- lib/gitlab/ci/config/external/file/project.rb | 4 ++-- lib/gitlab/ci/config/external/file/remote.rb | 10 ++++----- .../ci/config/external/file/template.rb | 2 +- lib/gitlab/ci/config/external/mapper.rb | 6 ++++- .../ci/config/external/file/artifact_spec.rb | 8 ++++++- .../ci/config/external/file/base_spec.rb | 8 ++++--- .../ci/config/external/file/local_spec.rb | 22 ++++++++++++++----- .../ci/config/external/file/project_spec.rb | 16 +++++++++----- .../ci/config/external/file/remote_spec.rb | 15 +++++++------ .../ci/config/external/file/template_spec.rb | 6 +++-- .../gitlab/ci/config/external/mapper_spec.rb | 8 +++++-- .../creation_errors_and_warnings_spec.rb | 18 +++++++++++++++ 16 files changed, 112 insertions(+), 43 deletions(-) diff --git a/lib/gitlab/ci/config/external/context.rb b/lib/gitlab/ci/config/external/context.rb index 308414af47d6..512cfdde474e 100644 --- a/lib/gitlab/ci/config/external/context.rb +++ b/lib/gitlab/ci/config/external/context.rb @@ -70,6 +70,16 @@ def sentry_payload } end + def mask_variables_from(location) + variables.reduce(location.dup) do |loc, variable| + if variable[:masked] + Gitlab::Ci::MaskSecret.mask!(loc, variable[:value]) + else + loc + end + end + end + protected attr_writer :expandset, :execution_deadline, :logger diff --git a/lib/gitlab/ci/config/external/file/artifact.rb b/lib/gitlab/ci/config/external/file/artifact.rb index e6ff33d6f793..4f79e64ca9ae 100644 --- a/lib/gitlab/ci/config/external/file/artifact.rb +++ b/lib/gitlab/ci/config/external/file/artifact.rb @@ -37,7 +37,7 @@ def project def validate_content! return unless ensure_preconditions_satisfied! - errors.push("File `#{location}` is empty!") unless content.present? + errors.push("File `#{masked_location}` is empty!") unless content.present? end def ensure_preconditions_satisfied! diff --git a/lib/gitlab/ci/config/external/file/base.rb b/lib/gitlab/ci/config/external/file/base.rb index 7d3fddd850d2..a660dd339d8f 100644 --- a/lib/gitlab/ci/config/external/file/base.rb +++ b/lib/gitlab/ci/config/external/file/base.rb @@ -79,21 +79,21 @@ def validate_execution_time! def validate_location! if invalid_location_type? - errors.push("Included file `#{location}` needs to be a string") + errors.push("Included file `#{masked_location}` needs to be a string") elsif invalid_extension? - errors.push("Included file `#{location}` does not have YAML extension!") + errors.push("Included file `#{masked_location}` does not have YAML extension!") end end def validate_content! if content.blank? - errors.push("Included file `#{location}` is empty or does not exist!") + errors.push("Included file `#{masked_location}` is empty or does not exist!") end end def validate_hash! if to_hash.blank? - errors.push("Included file `#{location}` does not have valid YAML syntax!") + errors.push("Included file `#{masked_location}` does not have valid YAML syntax!") end end @@ -104,6 +104,12 @@ def expand_includes(hash) def expand_context_attrs {} end + + def masked_location + strong_memoize(:masked_location) do + context.mask_variables_from(location) + end + end end end end diff --git a/lib/gitlab/ci/config/external/file/local.rb b/lib/gitlab/ci/config/external/file/local.rb index 3839c43bd536..3aa665c7d18a 100644 --- a/lib/gitlab/ci/config/external/file/local.rb +++ b/lib/gitlab/ci/config/external/file/local.rb @@ -23,11 +23,11 @@ def content def validate_content! if context.project&.repository.nil? - errors.push("Local file `#{location}` does not have project!") + errors.push("Local file `#{masked_location}` does not have project!") elsif content.nil? - errors.push("Local file `#{location}` does not exist!") + errors.push("Local file `#{masked_location}` does not exist!") elsif content.blank? - errors.push("Local file `#{location}` is empty!") + errors.push("Local file `#{masked_location}` is empty!") end end diff --git a/lib/gitlab/ci/config/external/file/project.rb b/lib/gitlab/ci/config/external/file/project.rb index 114d493381c8..27e097ba980c 100644 --- a/lib/gitlab/ci/config/external/file/project.rb +++ b/lib/gitlab/ci/config/external/file/project.rb @@ -35,9 +35,9 @@ def validate_content! elsif sha.nil? errors.push("Project `#{project_name}` reference `#{ref_name}` does not exist!") elsif content.nil? - errors.push("Project `#{project_name}` file `#{location}` does not exist!") + errors.push("Project `#{project_name}` file `#{masked_location}` does not exist!") elsif content.blank? - errors.push("Project `#{project_name}` file `#{location}` is empty!") + errors.push("Project `#{project_name}` file `#{masked_location}` is empty!") end end diff --git a/lib/gitlab/ci/config/external/file/remote.rb b/lib/gitlab/ci/config/external/file/remote.rb index 4bd8e250d7a9..8335a9ef625b 100644 --- a/lib/gitlab/ci/config/external/file/remote.rb +++ b/lib/gitlab/ci/config/external/file/remote.rb @@ -24,7 +24,7 @@ def validate_location! super unless ::Gitlab::UrlSanitizer.valid?(location) - errors.push("Remote file `#{location}` does not have a valid address!") + errors.push("Remote file `#{masked_location}` does not have a valid address!") end end @@ -32,17 +32,17 @@ def fetch_remote_content begin response = Gitlab::HTTP.get(location) rescue SocketError - errors.push("Remote file `#{location}` could not be fetched because of a socket error!") + errors.push("Remote file `#{masked_location}` could not be fetched because of a socket error!") rescue Timeout::Error - errors.push("Remote file `#{location}` could not be fetched because of a timeout error!") + errors.push("Remote file `#{masked_location}` could not be fetched because of a timeout error!") rescue Gitlab::HTTP::Error - errors.push("Remote file `#{location}` could not be fetched because of HTTP error!") + errors.push("Remote file `#{masked_location}` could not be fetched because of HTTP error!") rescue Gitlab::HTTP::BlockedUrlError => e errors.push("Remote file could not be fetched because #{e}!") end if response&.code.to_i >= 400 - errors.push("Remote file `#{location}` could not be fetched because of HTTP code `#{response.code}` error!") + errors.push("Remote file `#{masked_location}` could not be fetched because of HTTP code `#{response.code}` error!") end response.body if errors.none? diff --git a/lib/gitlab/ci/config/external/file/template.rb b/lib/gitlab/ci/config/external/file/template.rb index 47441fa3818f..c3d120dfdcec 100644 --- a/lib/gitlab/ci/config/external/file/template.rb +++ b/lib/gitlab/ci/config/external/file/template.rb @@ -26,7 +26,7 @@ def validate_location! super unless template_name_valid? - errors.push("Template file `#{location}` is not a valid location!") + errors.push("Template file `#{masked_location}` is not a valid location!") end end diff --git a/lib/gitlab/ci/config/external/mapper.rb b/lib/gitlab/ci/config/external/mapper.rb index 7f1de6ce1ab6..79a04ad409e7 100644 --- a/lib/gitlab/ci/config/external/mapper.rb +++ b/lib/gitlab/ci/config/external/mapper.rb @@ -142,7 +142,7 @@ def select_first_matching_without_instrumentation(location) file_class.new(location, context) end.select(&:matching?) - raise AmbigiousSpecificationError, "Include `#{location.to_json}` needs to match exactly one accessor!" unless matching.one? + raise AmbigiousSpecificationError, "Include `#{masked_location(location.to_json)}` needs to match exactly one accessor!" unless matching.one? matching.first end @@ -177,6 +177,10 @@ def transform(data) def expand(data) ExpandVariables.expand(data, -> { context.variables_hash }) end + + def masked_location(location) + context.mask_variables_from(location) + end end end end diff --git a/spec/lib/gitlab/ci/config/external/file/artifact_spec.rb b/spec/lib/gitlab/ci/config/external/file/artifact_spec.rb index 8dd92c5b5fd9..b59fc95a8cc5 100644 --- a/spec/lib/gitlab/ci/config/external/file/artifact_spec.rb +++ b/spec/lib/gitlab/ci/config/external/file/artifact_spec.rb @@ -127,6 +127,12 @@ let!(:metadata) { create(:ci_job_artifact, :metadata, job: generator_job) } context 'when file is empty' do + let(:params) { { artifact: 'secret_stuff/generated.yml', job: 'generator' } } + let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret_stuff', 'masked' => true }]) } + let(:context) do + Gitlab::Ci::Config::External::Context.new(parent_pipeline: parent_pipeline, variables: variables) + end + before do allow_next_instance_of(Gitlab::Ci::ArtifactFileReader) do |reader| allow(reader).to receive(:read).and_return('') @@ -134,7 +140,7 @@ end let(:expected_error) do - 'File `generated.yml` is empty!' + 'File `xxxxxxxxxxxx/generated.yml` is empty!' end it_behaves_like 'is invalid' diff --git a/spec/lib/gitlab/ci/config/external/file/base_spec.rb b/spec/lib/gitlab/ci/config/external/file/base_spec.rb index 445edb253fd0..536f48ecba67 100644 --- a/spec/lib/gitlab/ci/config/external/file/base_spec.rb +++ b/spec/lib/gitlab/ci/config/external/file/base_spec.rb @@ -3,7 +3,8 @@ require 'spec_helper' RSpec.describe Gitlab::Ci::Config::External::File::Base do - let(:context_params) { { sha: 'HEAD' } } + let(:variables) { } + let(:context_params) { { sha: 'HEAD', variables: variables } } let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) } let(:test_class) do @@ -76,7 +77,8 @@ def initialize(params, context) end context 'when there are YAML syntax errors' do - let(:location) { 'some/file/config.yml' } + let(:location) { 'some/file/secret_file_name.yml' } + let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret_file_name', 'masked' => true }]) } before do allow_any_instance_of(test_class) @@ -85,7 +87,7 @@ def initialize(params, context) it 'is not a valid file' do expect(subject).not_to be_valid - expect(subject.error_message).to match /does not have valid YAML syntax/ + expect(subject.error_message).to eq('Included file `some/file/xxxxxxxxxxxxxxxx.yml` does not have valid YAML syntax!') end end end diff --git a/spec/lib/gitlab/ci/config/external/file/local_spec.rb b/spec/lib/gitlab/ci/config/external/file/local_spec.rb index dec3eebe7b12..b9314dfc44ec 100644 --- a/spec/lib/gitlab/ci/config/external/file/local_spec.rb +++ b/spec/lib/gitlab/ci/config/external/file/local_spec.rb @@ -7,6 +7,7 @@ let_it_be(:user) { create(:user) } let(:sha) { '12345' } + let(:variables) { project.predefined_variables.to_runner_variables } let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) } let(:params) { { local: location } } let(:local_file) { described_class.new(params, context) } @@ -18,7 +19,7 @@ sha: sha, user: user, parent_pipeline: parent_pipeline, - variables: project.predefined_variables.to_runner_variables + variables: variables } end @@ -66,7 +67,7 @@ end end - context 'when is not a valid local path' do + context 'when it is not a valid local path' do let(:location) { '/lib/gitlab/ci/templates/non-existent-file.yml' } it 'returns false' do @@ -74,7 +75,7 @@ end end - context 'when is not a yaml file' do + context 'when it is not a yaml file' do let(:location) { '/config/application.rb' } it 'returns false' do @@ -82,6 +83,16 @@ end end + context 'when it is an empty file' do + let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret', 'masked' => true }]) } + let(:location) { '/lib/gitlab/ci/templates/secret/existent-file.yml' } + + it 'returns false and adds an error message about an empty file' do + allow_any_instance_of(described_class).to receive(:fetch_local_content).and_return("") + expect(local_file.errors).to include("Local file `/lib/gitlab/ci/templates/xxxxxx/existent-file.yml` is empty!") + end + end + context 'when the given sha is not valid' do let(:location) { '/lib/gitlab/ci/templates/existent-file.yml' } let(:sha) { ':' } @@ -126,10 +137,11 @@ end describe '#error_message' do - let(:location) { '/lib/gitlab/ci/templates/non-existent-file.yml' } + let(:location) { '/lib/gitlab/ci/templates/secret_file.yml' } + let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret_file', 'masked' => true }]) } it 'returns an error message' do - expect(local_file.error_message).to eq("Local file `#{location}` does not exist!") + expect(local_file.error_message).to eq("Local file `/lib/gitlab/ci/templates/xxxxxxxxxxx.yml` does not exist!") end end diff --git a/spec/lib/gitlab/ci/config/external/file/project_spec.rb b/spec/lib/gitlab/ci/config/external/file/project_spec.rb index c53914c57725..74720c0a3ca9 100644 --- a/spec/lib/gitlab/ci/config/external/file/project_spec.rb +++ b/spec/lib/gitlab/ci/config/external/file/project_spec.rb @@ -11,6 +11,7 @@ let(:parent_pipeline) { double(:parent_pipeline) } let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) } let(:project_file) { described_class.new(params, context) } + let(:variables) { project.predefined_variables.to_runner_variables } let(:context_params) do { @@ -18,7 +19,7 @@ sha: '12345', user: context_user, parent_pipeline: parent_pipeline, - variables: project.predefined_variables.to_runner_variables + variables: variables } end @@ -108,18 +109,19 @@ context 'when an empty file is used' do let(:params) do - { project: project.full_path, file: '/file.yml' } + { project: project.full_path, file: '/secret_file.yml' } end + let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret_file', 'masked' => true }]) } let(:root_ref_sha) { project.repository.root_ref_sha } before do - stub_project_blob(root_ref_sha, '/file.yml') { '' } + stub_project_blob(root_ref_sha, '/secret_file.yml') { '' } end it 'returns false' do expect(project_file).not_to be_valid - expect(project_file.error_message).to include("Project `#{project.full_path}` file `/file.yml` is empty!") + expect(project_file.error_message).to include("Project `#{project.full_path}` file `/xxxxxxxxxxx.yml` is empty!") end end @@ -135,13 +137,15 @@ end context 'when non-existing file is requested' do + let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret-invalid-file', 'masked' => true }]) } + let(:params) do - { project: project.full_path, file: '/invalid-file.yml' } + { project: project.full_path, file: '/secret-invalid-file.yml' } end it 'returns false' do expect(project_file).not_to be_valid - expect(project_file.error_message).to include("Project `#{project.full_path}` file `/invalid-file.yml` does not exist!") + expect(project_file.error_message).to include("Project `#{project.full_path}` file `/xxxxxxxxxxxxxxxxxxx.yml` does not exist!") end end diff --git a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb index ab60ac215ba3..2613bfbfdcff 100644 --- a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb +++ b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb @@ -5,11 +5,12 @@ RSpec.describe Gitlab::Ci::Config::External::File::Remote do include StubRequests - let(:context_params) { { sha: '12345' } } + let(:variables) {Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret_file', 'masked' => true }]) } + let(:context_params) { { sha: '12345', variables: variables } } let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) } let(:params) { { remote: location } } let(:remote_file) { described_class.new(params, context) } - let(:location) { 'https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.gitlab-ci-1.yml' } + let(:location) { 'https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.secret_file.yml' } let(:remote_file_content) do <<~HEREDOC before_script: @@ -144,10 +145,10 @@ subject { remote_file.error_message } context 'when remote file location is not valid' do - let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.gitlab-ci-1.yml' } + let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-foss/blob/1234/?secret_file.yml' } it 'returns an error message describing invalid address' do - expect(subject).to match /does not have a valid address!/ + expect(subject).to eq('Remote file `not-valid://gitlab.com/gitlab-org/gitlab-foss/blob/1234/?xxxxxxxxxxx.yml` does not have a valid address!') end end @@ -157,7 +158,7 @@ end it 'returns error message about a timeout' do - expect(subject).to match /could not be fetched because of a timeout error!/ + expect(subject).to eq('Remote file `https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.xxxxxxxxxxx.yml` could not be fetched because of a timeout error!') end end @@ -167,7 +168,7 @@ end it 'returns error message about a HTTP error' do - expect(subject).to match /could not be fetched because of HTTP error!/ + expect(subject).to eq('Remote file `https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.xxxxxxxxxxx.yml` could not be fetched because of HTTP error!') end end @@ -177,7 +178,7 @@ end it 'returns error message about a timeout' do - expect(subject).to match /could not be fetched because of HTTP code `404` error!/ + expect(subject).to eq('Remote file `https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.xxxxxxxxxxx.yml` could not be fetched because of HTTP code `404` error!') end end diff --git a/spec/lib/gitlab/ci/config/external/file/template_spec.rb b/spec/lib/gitlab/ci/config/external/file/template_spec.rb index 75b22c1516c7..66a06de3d285 100644 --- a/spec/lib/gitlab/ci/config/external/file/template_spec.rb +++ b/spec/lib/gitlab/ci/config/external/file/template_spec.rb @@ -54,11 +54,13 @@ end context 'with invalid template name' do - let(:template) { 'Template.yml' } + let(:template) { 'SecretTemplate.yml' } + let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'SecretTemplate', 'masked' => true }]) } + let(:context_params) { { project: project, sha: '12345', user: user, variables: variables } } it 'returns false' do expect(template_file).not_to be_valid - expect(template_file.error_message).to include('Template file `Template.yml` is not a valid location!') + expect(template_file.error_message).to include('`xxxxxxxxxxxxxx.yml` is not a valid location!') end end diff --git a/spec/lib/gitlab/ci/config/external/mapper_spec.rb b/spec/lib/gitlab/ci/config/external/mapper_spec.rb index f8754d7e124d..f69feba5e59d 100644 --- a/spec/lib/gitlab/ci/config/external/mapper_spec.rb +++ b/spec/lib/gitlab/ci/config/external/mapper_spec.rb @@ -11,7 +11,8 @@ let(:local_file) { '/lib/gitlab/ci/templates/non-existent-file.yml' } let(:remote_url) { 'https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.gitlab-ci-1.yml' } let(:template_file) { 'Auto-DevOps.gitlab-ci.yml' } - let(:context_params) { { project: project, sha: '123456', user: user, variables: project.predefined_variables } } + let(:variables) { project.predefined_variables } + let(:context_params) { { project: project, sha: '123456', user: user, variables: variables } } let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) } let(:file_content) do @@ -92,13 +93,16 @@ end context 'when the key is a hash of file and remote' do + let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'GITLAB_TOKEN', 'value' => 'secret-file', 'masked' => true }]) } + let(:local_file) { 'secret-file.yml' } + let(:remote_url) { 'https://gitlab.com/secret-file.yml' } let(:values) do { include: { 'local' => local_file, 'remote' => remote_url }, image: 'ruby:2.7' } end it 'returns ambigious specification error' do - expect { subject }.to raise_error(described_class::AmbigiousSpecificationError) + expect { subject }.to raise_error(described_class::AmbigiousSpecificationError, 'Include `{"local":"xxxxxxxxxxx.yml","remote":"https://gitlab.com/xxxxxxxxxxx.yml"}` needs to match exactly one accessor!') end end diff --git a/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb b/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb index e62a94b6df89..a920b90b97d6 100644 --- a/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb +++ b/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb @@ -53,6 +53,24 @@ end context 'when failed to create the pipeline' do + context 'when errors are raised and masked variables are involved' do + let_it_be(:variable) { create(:ci_variable, project: project, key: 'GL_TOKEN', value: 'test_value', masked: true) } + + let(:config) do + <<~YAML + include: + - local: $GL_TOKEN/gitlab-ci.txt + YAML + end + + it 'contains errors and masks variables' do + error_message = "Included file `xxxxxxxxxx/gitlab-ci.txt` does not have YAML extension!" + expect(pipeline.yaml_errors).to eq(error_message) + expect(pipeline.error_messages.map(&:content)).to contain_exactly(error_message) + expect(pipeline.errors.full_messages).to contain_exactly(error_message) + end + end + context 'when warnings are raised' do let(:config) do <<~YAML -- GitLab