Commit f0988c40 authored by John Jarvis's avatar John Jarvis

Merge branch 'ensure-that-build-token-is-always-running-11-5' into 'security-11-5'

[11.5] Ensure that build token is only used when running

See merge request gitlab/gitlabhq!2664
parents 411e5178 4fb58035
......@@ -152,6 +152,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
......
---
title: Ensure that build token is only used when running
merge_request:
author:
type: security
......@@ -1346,7 +1346,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
......@@ -1374,7 +1384,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
......
......@@ -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
......
......@@ -146,7 +146,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]
......@@ -174,7 +173,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']
......@@ -217,8 +215,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
......@@ -261,7 +258,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)
......@@ -308,7 +304,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
......
......@@ -296,7 +296,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
......
......@@ -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
......@@ -983,7 +986,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
......@@ -1024,11 +1027,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
......@@ -1199,7 +1203,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
......@@ -1212,10 +1216,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
......@@ -1248,9 +1252,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
......@@ -1303,10 +1319,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
......@@ -1526,10 +1542,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
......@@ -1555,7 +1574,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
......@@ -1582,6 +1600,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 }
......@@ -1589,8 +1631,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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment