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

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

changelogs/unreleased/ensure-that-build-token-is-always-running.yml

+---
+title: Ensure that build token is only used when running
+merge_request:
+author:
+type: security

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

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])
-        validate_job!(job) do
-          forbidden! unless job_token_valid?(job)
+      def authenticate_job_by_token!
+        token = (params[JOB_TOKEN_PARAM] || env[JOB_TOKEN_HEADER]).to_s
+        Ci::Build.find_by_token(token).tap do |job|
+          validate_job!(job)
         end
-        job
+      end
+      # 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

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

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

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