diff options
-rw-r--r-- | app/models/ci/build.rb | 4 | ||||
-rw-r--r-- | changelogs/unreleased/ensure-that-build-token-is-always-running.yml | 5 | ||||
-rw-r--r-- | lib/api/entities.rb | 17 | ||||
-rw-r--r-- | lib/api/helpers/runner.rb | 30 | ||||
-rw-r--r-- | lib/api/runner.rb | 8 | ||||
-rw-r--r-- | lib/gitlab/auth.rb | 2 | ||||
-rw-r--r-- | spec/requests/api/runner_spec.rb | 78 |
7 files changed, 105 insertions, 39 deletions
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index cdfe8175a42..21f998a1c05 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -153,6 +153,10 @@ module Ci .execute(build) # rubocop: enable CodeReuse/ServiceClass end + + def find_running_by_token(token) + running.find_by_token(token) + end end state_machine :status do diff --git a/changelogs/unreleased/ensure-that-build-token-is-always-running.yml b/changelogs/unreleased/ensure-that-build-token-is-always-running.yml new file mode 100644 index 00000000000..ec1f73c70ab --- /dev/null +++ b/changelogs/unreleased/ensure-that-build-token-is-always-running.yml @@ -0,0 +1,5 @@ +--- +title: Ensure that build token is only used when running +merge_request: +author: +type: security diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 120545792f2..160a8b021ee 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1325,7 +1325,17 @@ module API end class Dependency < Grape::Entity - expose :id, :name, :token + expose :id, :name + expose :token do |dependency, options| + # overrides the job's dependency authorization token + # with the token of the job that is being run + # this way we use the parent job auth token + # + # ideally we would change the runner implementation to + # use different token, but this would require upgrade of + # all runners which is impossible + options[:auth_token] + end expose :artifacts_file, using: JobArtifactFile, if: ->(job, _) { job.artifacts? } end @@ -1353,7 +1363,10 @@ module API expose :artifacts, using: Artifacts expose :cache, using: Cache expose :credentials, using: Credentials - expose :dependencies, using: Dependency + expose :dependencies do |model| + Dependency.represent(model.dependencies, + options.merge(auth_token: model.token)) + end expose :features end end diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb index 45d0343bc89..1a296c8ddb2 100644 --- a/lib/api/helpers/runner.rb +++ b/lib/api/helpers/runner.rb @@ -36,26 +36,32 @@ module API def validate_job!(job) not_found! unless job - yield if block_given? - project = job.project - forbidden!('Project has been deleted!') if project.nil? || project.pending_delete? - forbidden!('Job has been erased!') if job.erased? + job_forbidden!(job, 'Project has been deleted!') if project.nil? || project.pending_delete? + job_forbidden!(job, 'Job has been erased!') if job.erased? + job_forbidden!(job, 'Not running!') unless job.running? end - def authenticate_job! - job = Ci::Build.find_by_id(params[:id]) + def authenticate_job_by_token! + token = (params[JOB_TOKEN_PARAM] || env[JOB_TOKEN_HEADER]).to_s - validate_job!(job) do - forbidden! unless job_token_valid?(job) + Ci::Build.find_by_token(token).tap do |job| + validate_job!(job) end + end - job + # we look for a job that has ID and token matching + def authenticate_job! + authenticate_job_by_token!.tap do |job| + job_forbidden!(job, 'Invalid Job ID!') unless job.id == params[:id] + end end - def job_token_valid?(job) - token = (params[JOB_TOKEN_PARAM] || env[JOB_TOKEN_HEADER]).to_s - token && job.valid_token?(token) + # we look for a job that has been shared via pipeline using the ID + def authenticate_pipeline_job! + job = authenticate_job_by_token! + + job.pipeline.builds.find(params[:id]) end def max_artifacts_size diff --git a/lib/api/runner.rb b/lib/api/runner.rb index d8768a54986..cd1ab674404 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -147,7 +147,6 @@ module API end put '/:id' do job = authenticate_job! - job_forbidden!(job, 'Job is not running') unless job.running? job.trace.set(params[:trace]) if params[:trace] @@ -175,7 +174,6 @@ module API end patch '/:id/trace' do job = authenticate_job! - job_forbidden!(job, 'Job is not running') unless job.running? error!('400 Missing header Content-Range', 400) unless request.headers.key?('Content-Range') content_range = request.headers['Content-Range'] @@ -218,8 +216,7 @@ module API require_gitlab_workhorse! Gitlab::Workhorse.verify_api_request!(headers) - job = authenticate_job! - forbidden!('Job is not running') unless job.running? + authenticate_job! if params[:filesize] file_size = params[:filesize].to_i @@ -262,7 +259,6 @@ module API require_gitlab_workhorse! job = authenticate_job! - forbidden!('Job is not running!') unless job.running? artifacts = UploadedFile.from_params(params, :file, JobArtifactUploader.workhorse_local_upload_path) metadata = UploadedFile.from_params(params, :metadata, JobArtifactUploader.workhorse_local_upload_path) @@ -309,7 +305,7 @@ module API optional :direct_download, default: false, type: Boolean, desc: %q(Perform direct download from remote storage instead of proxying artifacts) end get '/:id/artifacts' do - job = authenticate_job! + job = authenticate_pipeline_job! present_carrierwave_file!(job.artifacts_file, supports_direct_download: params[:direct_download]) end diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 3ed66013b41..f36b0308cce 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -294,7 +294,7 @@ module Gitlab private def find_build_by_token(token) - ::Ci::Build.running.find_by_token(token) + ::Ci::Build.find_running_by_token(token) end end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 43ceb332cfb..48fa81c91a7 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -441,9 +441,11 @@ describe API::Runner, :clean_gitlab_redis_shared_state do it 'picks a job' do request_job info: { platform: :darwin } + runner.reload + expect(response).to have_gitlab_http_status(201) expect(response.headers).not_to have_key('X-GitLab-Last-Update') - expect(runner.reload.platform).to eq('darwin') + expect(runner.platform).to eq('darwin') expect(json_response['id']).to eq(job.id) expect(json_response['token']).to eq(job.token) expect(json_response['job_info']).to eq(expected_job_info) @@ -537,8 +539,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(json_response['id']).to eq(test_job.id) expect(json_response['dependencies'].count).to eq(2) expect(json_response['dependencies']).to include( - { 'id' => job.id, 'name' => job.name, 'token' => job.token }, - { 'id' => job2.id, 'name' => job2.name, 'token' => job2.token }) + { 'id' => job.id, 'name' => job.name, 'token' => test_job.token }, + { 'id' => job2.id, 'name' => job2.name, 'token' => test_job.token }) end end @@ -557,7 +559,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(json_response['id']).to eq(test_job.id) expect(json_response['dependencies'].count).to eq(1) expect(json_response['dependencies']).to include( - { 'id' => job.id, 'name' => job.name, 'token' => job.token, + { 'id' => job.id, 'name' => job.name, 'token' => test_job.token, 'artifacts_file' => { 'filename' => 'ci_build_artifacts.zip', 'size' => 106365 } }) end end @@ -582,7 +584,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(response).to have_gitlab_http_status(201) expect(json_response['id']).to eq(test_job.id) expect(json_response['dependencies'].count).to eq(1) - expect(json_response['dependencies'][0]).to include('id' => job2.id, 'name' => job2.name, 'token' => job2.token) + expect(json_response['dependencies'][0]).to include( + 'id' => job2.id, 'name' => job2.name, 'token' => test_job.token) end end @@ -965,7 +968,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do patch_the_trace end - it 'returns Forbidden ' do + it 'returns Forbidden' do expect(response.status).to eq(403) end end @@ -1006,11 +1009,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do context 'when the job is canceled' do before do - job.cancel + job.cancel! patch_the_trace end - it 'receives status in header' do + it 'responds with forbidden and status in header' do + expect(response).to have_gitlab_http_status(403) expect(response.header['Job-Status']).to eq 'canceled' end end @@ -1181,7 +1185,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do it 'fails to authorize artifacts posting' do authorize_artifacts(token: job.project.runners_token) - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(404) end end @@ -1194,10 +1198,10 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end context 'authorization token is invalid' do - it 'responds with forbidden' do + it 'responds with not found' do authorize_artifacts(token: 'invalid', filesize: 100 ) - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(404) end end @@ -1230,9 +1234,21 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end it 'responds with forbidden' do + expect(response).to have_gitlab_http_status(403) + end + end + + context 'when job has been canceled' do + let(:job) { create(:ci_build) } + + before do + job.cancel! upload_artifacts(file_upload, headers_with_token) + end + it 'responds with forbidden' do expect(response).to have_gitlab_http_status(403) + expect(response.header['Job-Status']).to eq('canceled') end end @@ -1285,10 +1301,10 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end context 'when using runners token' do - it 'responds with forbidden' do + it 'responds with not found' do upload_artifacts(file_upload, headers.merge(API::Helpers::Runner::JOB_TOKEN_HEADER => job.project.runners_token)) - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(404) end end end @@ -1508,10 +1524,13 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end describe 'GET /api/v4/jobs/:id/artifacts' do - let(:token) { job.token } + let(:project) { create(:project) } + let(:pipeline) { create(:ci_empty_pipeline, project: project) } + let(:running_job) { create(:ci_build, :running, pipeline: pipeline) } + let(:token) { running_job.token } context 'when job has artifacts' do - let(:job) { create(:ci_build) } + let(:job) { create(:ci_build, pipeline: pipeline) } let(:store) { JobArtifactUploader::Store::LOCAL } before do @@ -1537,7 +1556,6 @@ describe API::Runner, :clean_gitlab_redis_shared_state do context 'when artifacts are stored remotely' do let(:store) { JobArtifactUploader::Store::REMOTE } - let!(:job) { create(:ci_build) } context 'when proxy download is being used' do before do @@ -1564,6 +1582,30 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end end + context 'when using running token from another pipeline' do + let(:running_job) { create(:ci_build, :running, project: project) } + + before do + download_artifact + end + + it 'responds with not found' do + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when using running token from another project' do + let(:running_job) { create(:ci_build, :running) } + + before do + download_artifact + end + + it 'responds with not found' do + expect(response).to have_gitlab_http_status(404) + end + end + context 'when using runnners token' do let(:token) { job.project.runners_token } @@ -1571,8 +1613,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do download_artifact end - it 'responds with forbidden' do - expect(response).to have_gitlab_http_status(403) + it 'responds with not found' do + expect(response).to have_gitlab_http_status(404) end end end |