Follow-up from "Disallow local URls for build_runner_session if dictated by app setting"
The following discussion from gitlab-org/security/gitlab!2863 should be addressed:
-
@fabiopitino started a discussion: (+3 comments) build.errors
could contain any other errors related to state machine transitions, not necessarily model validations.When wrong session URL is passed by the runner, we should see an exception being raised here. We assign the runner_session_attributes and when we do
build.run!
it would raise a state machine error because when validatingCi::Build
we would also validate the relatedCi::BuildRunnerSession
.This means that before we could have returned
409 Conflict
and now we could return a new status.🤔 At the same time, if a user sets an invalid (or non https) URL, today they should still get
409 Conflict
.If I'm not wrong, this causes the runner to try again to pick other builds and always be unsuccessful.
Proposal
Apply the following patch:
Patch
diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb
index f11577feb886..5a1b119b2c43 100644
--- a/app/services/ci/register_job_service.rb
+++ b/app/services/ci/register_job_service.rb
@@ -10,7 +10,9 @@ class RegisterJobService
TEMPORARY_LOCK_TIMEOUT = 3.seconds
- Result = Struct.new(:build, :build_json, :valid?)
+ Result = Struct.new(:build, :build_json, :valid?, :error)
+
+ RunnerSessionError = Class.new(StandardError)
##
# The queue depth limit number has been determined by observing 95
@@ -52,6 +54,7 @@ def execute(params = {})
def process_queue(params)
valid = true
+ error = nil
depth = 0
each_build(params) do |build|
@@ -93,6 +96,7 @@ def process_queue(params)
# The usage of valid: is described in
# handling of ActiveRecord::StaleObjectError
valid = false
+ error = result.error
end
end
@@ -101,7 +105,7 @@ def process_queue(params)
@metrics.observe_queue_depth(:not_found, depth) if valid
@metrics.register_failure
- Result.new(nil, nil, valid)
+ Result.new(nil, nil, valid, error)
end
# rubocop: disable CodeReuse/ActiveRecord
@@ -184,6 +188,10 @@ def process_build(build, params)
if assign_runner!(build, params)
present_build!(build)
end
+ rescue RunnerSessionError => ex
+ @metrics.increment_queue_operation(:runner_invalid_session)
+
+ Result.new(nil, nil, false, ex.message)
rescue ActiveRecord::StaleObjectError
# We are looping to find another build that is not conflicting
# It also indicates that this build can be picked and passed to runner.
@@ -244,7 +252,11 @@ def log_build_dependencies_size(presented_build)
def assign_runner!(build, params)
build.runner_id = runner.id
- build.runner_session_attributes = params[:session] if params[:session].present?
+ if params[:session].present?
+ build.runner_session_attributes = params[:session]
+
+ check_build_runner_session_valid(build, runner)
+ end
failure_reason, _ = pre_assign_runner_checks.find { |_, check| check.call(build, params) }
@@ -293,6 +305,20 @@ def track_exception_for_build(ex, build)
)
end
+ def check_build_runner_session_valid(build, runner)
+ return if build.runner_session.valid?
+
+ error_messages = build.runner_session.errors.full_messages.join(', ')
+
+ Gitlab::AppLogger.error(
+ event: 'runner_provided_invalid_session_data',
+ build_id: build.id,
+ runner_id: runner.id,
+ runner_session_errors: error_messages)
+
+ raise RunnerSessionError, error_messages
+ end
+
def pre_assign_runner_checks
{
missing_dependency_failure: -> (build, _) { !build.has_valid_build_dependencies? },
diff --git a/ee/spec/requests/api/ci/runner_spec.rb b/ee/spec/requests/api/ci/runner_spec.rb
index f1a9952267e8..346933670d7f 100644
--- a/ee/spec/requests/api/ci/runner_spec.rb
+++ b/ee/spec/requests/api/ci/runner_spec.rb
@@ -90,7 +90,7 @@
context 'job does not have secrets configured' do
let(:secrets) { {} }
- it 'doesn not return secrets configuration' do
+ it 'does not return secrets configuration' do
request_job_with_secrets_supported
expect(response).to have_gitlab_http_status(:created)
@@ -115,10 +115,10 @@
end
end
end
- end
- def request_job_with_secrets_supported
- request_job info: { features: { vault_secrets: true } }
+ def request_job_with_secrets_supported
+ request_job info: { features: { vault_secrets: true } }
+ end
end
def request_job(token = runner.token, **params)
diff --git a/lib/api/ci/runner.rb b/lib/api/ci/runner.rb
index df39ef1bafa1..b76d882fe503 100644
--- a/lib/api/ci/runner.rb
+++ b/lib/api/ci/runner.rb
@@ -101,7 +101,8 @@ class Runner < ::API::Base
http_codes [[201, 'Job was scheduled'],
[204, 'No job for Runner'],
[403, 'Forbidden'],
- [409, 'Conflict']]
+ [409, 'Conflict'],
+ [422, 'Unprocessable Entity']]
end
params do
requires :token, type: String, desc: %q(Runner's authentication token)
@@ -171,7 +172,12 @@ class Runner < ::API::Base
else
# We received build that is invalid due to concurrency conflict
Gitlab::Metrics.add_event(:build_invalid)
- conflict!
+
+ if result.error
+ unprocessable_entity!(result.error)
+ else
+ conflict!
+ end
end
end
diff --git a/lib/gitlab/ci/queue/metrics.rb b/lib/gitlab/ci/queue/metrics.rb
index 5cee73238ca6..b0152b77ec08 100644
--- a/lib/gitlab/ci/queue/metrics.rb
+++ b/lib/gitlab/ci/queue/metrics.rb
@@ -31,6 +31,7 @@ class Metrics
:queue_iteration,
:queue_depth_limit,
:queue_replication_lag,
+ :runner_invalid_session,
:runner_pre_assign_checks_failed,
:runner_pre_assign_checks_success,
:runner_queue_tick,
diff --git a/spec/requests/api/ci/runner/jobs_request_post_spec.rb b/spec/requests/api/ci/runner/jobs_request_post_spec.rb
index baa16b052a07..0006e60ec756 100644
--- a/spec/requests/api/ci/runner/jobs_request_post_spec.rb
+++ b/spec/requests/api/ci/runner/jobs_request_post_spec.rb
@@ -977,8 +977,10 @@
it 'returns :unprocessable_entity status code', :aggregate_failures do
request_job(**job_params)
- expect(response).to have_gitlab_http_status(:conflict)
- expect(response.body).to include('409 Conflict')
+ expect(response).to have_gitlab_http_status(:unprocessable_entity)
+ expect(response.body).to include(
+ 'Url is blocked: Requests to localhost are not allowed'
+ )
end
end
end