Allow API access with CI_JOB_TOKENzj-ci-job-token-api-access

In the current implementation the `initial_current_user` can be set to
the user whom triggered the job if the token matches the CI_JOB_TOKEN.
This however grants the key to kingdom, which also might be a feature.
To limit the exposure and make interation less likely, the token is
NULLIFIED when the job is finished.

The attack vector now does include all jobs which either
- Playable jobs
- Jobs which never finish somehow

7 files changed, 55 insertions, 11 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index a300536532b..79ff8e02651 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -43,7 +43,7 @@ module Ci
     add_authentication_token_field :token
 
     before_save :update_artifacts_size, if: :artifacts_file_changed?
-    before_save :ensure_token
+    before_create :ensure_token
     before_destroy { unscoped_project }
 
     after_create :execute_hooks
diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb
index 1ca7f91dc03..7dd9e0b2b7d 100644
--- a/app/models/concerns/token_authenticatable.rb
+++ b/app/models/concerns/token_authenticatable.rb
@@ -52,6 +52,10 @@ module TokenAuthenticatable
         write_new_token(token_field)
         save!
       end
+
+      define_method("clear_#{token_field}!") do
+        update_attribute(token_field, nil)
+      end
     end
   end
 end
diff --git a/app/workers/build_finished_worker.rb b/app/workers/build_finished_worker.rb
index 466410bf08c..44e1085c35a 100644
--- a/app/workers/build_finished_worker.rb
+++ b/app/workers/build_finished_worker.rb
@@ -3,9 +3,11 @@ class BuildFinishedWorker
   include BuildQueue
 
   def perform(build_id)
-    Ci::Build.find_by(id: build_id).try do |build|
+    Ci::Build.find_by(id: build_id)&.tap do |build|
       BuildCoverageWorker.new.perform(build.id)
       BuildHooksWorker.new.perform(build.id)
+
+      build.clear_token!
     end
   end
 end
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index 9fcf04efa38..e446fa6b520 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -8,6 +8,7 @@ module API
 
     PRIVATE_TOKEN_HEADER = "HTTP_PRIVATE_TOKEN".freeze
     PRIVATE_TOKEN_PARAM = :private_token
+    CI_JOB_TOKEN_PARAM = :ci_job_token
 
     included do |base|
       # OAuth2 Resource Server Authentication
@@ -70,6 +71,14 @@ module API
         find_user_by_authentication_token(token_string) || find_user_by_personal_access_token(token_string, scopes)
       end
 
+      def find_user_by_ci_token
+        job_token = params[CI_JOB_TOKEN_PARAM].to_s
+
+        return nil unless job_token.present?
+
+        Ci::Build.find_by_token(job_token)&.user
+      end
+
       def current_user
         @current_user
       end
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 2c73a6fdc4e..9ffd04cd1e0 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -313,7 +313,7 @@ module API
 
     def present_artifacts!(artifacts_file)
       return not_found! unless artifacts_file.exists?
-  
+
       if artifacts_file.file_storage?
         present_file!(artifacts_file.path, artifacts_file.filename)
       else
@@ -341,10 +341,12 @@ module API
 
     def initial_current_user
       return @initial_current_user if defined?(@initial_current_user)
+
       Gitlab::Auth::UniqueIpsLimiter.limit_user! do
         @initial_current_user ||= find_user_by_private_token(scopes: @scopes)
         @initial_current_user ||= doorkeeper_guard(scopes: @scopes)
         @initial_current_user ||= find_user_from_warden
+        @initial_current_user ||= find_user_by_ci_token
 
         unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed?
           @initial_current_user = nil
diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb
index 8d647eb1c7e..b03095ff5e5 100644
--- a/spec/requests/api/jobs_spec.rb
+++ b/spec/requests/api/jobs_spec.rb
@@ -1,19 +1,19 @@
 require 'spec_helper'
 
 describe API::Jobs, :api do
-  let!(:project) do
+  set(:project) do
     create(:project, :repository, public_builds: false)
   end
 
-  let!(:pipeline) do
+  set(:pipeline) do
     create(:ci_empty_pipeline, project: project,
                                sha: project.commit.id,
                                ref: project.default_branch)
   end
 
-  let!(:job) { create(:ci_build, pipeline: pipeline) }
+  set(:job) { create(:ci_build, pipeline: pipeline) }
 
-  let(:user) { create(:user) }
+  set(:user) { create(:user) }
   let(:api_user) { user }
   let(:reporter) { create(:project_member, :reporter, project: project).user }
   let(:guest) { create(:project_member, :guest, project: project).user }
@@ -189,13 +189,13 @@ describe API::Jobs, :api do
   end
 
   describe 'GET /projects/:id/jobs/:job_id/artifacts' do
-    before do
-      get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user)
-    end
-
     context 'job with artifacts' do
       let(:job) { create(:ci_build, :artifacts, pipeline: pipeline) }
 
+      before do
+        get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user)
+      end
+
       context 'authorized user' do
         let(:download_headers) do
           { 'Content-Transfer-Encoding' => 'binary',
@@ -218,7 +218,28 @@ describe API::Jobs, :api do
       end
     end
 
+    context 'authorized by ci_job_token' do
+      let(:job) { create(:ci_build, :artifacts, pipeline: pipeline, user: user) }
+
+      let(:download_headers) do
+        { 'Content-Transfer-Encoding' => 'binary',
+          'Content-Disposition' => 'attachment; filename=ci_build_artifacts.zip' }
+      end
+
+      before do
+        get api("/projects/#{project.id}/jobs/#{job.id}/artifacts"), ci_job_token: job.token
+      end
+
+      it 'returns specific job artifacts' do
+        expect(response).to have_http_status(200)
+        expect(response.headers).to include(download_headers)
+        expect(response.body).to match_file(job.artifacts_file.file.file)
+      end
+    end
+
     it 'does not return job artifacts if not uploaded' do
+      get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user)
+
       expect(response).to have_http_status(404)
     end
   end
diff --git a/spec/workers/build_finished_worker_spec.rb b/spec/workers/build_finished_worker_spec.rb
index 2868167c7d4..a4f7ca85a3e 100644
--- a/spec/workers/build_finished_worker_spec.rb
+++ b/spec/workers/build_finished_worker_spec.rb
@@ -18,6 +18,12 @@ describe BuildFinishedWorker do
 
         described_class.new.perform(build.id)
       end
+
+      it 'clears the token field' do
+        described_class.new.perform(build.id)
+
+        expect(build.reload.token).to be(nil)
+      end
     end
 
     context 'when build does not exist' do