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

[11.4] Ensure that build token is always running

See merge request gitlab/gitlabhq!2663

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