Merge branch 'fix/gb/encrypt-ci-build-token-ee' into 'master'

Encrypt CI/CD builds tokens / EE

See merge request gitlab-org/gitlab-ee!8750

app/models/ci/build.rb

@@ -122,7 +122,7 @@ module Ci
 
     acts_as_taggable
 
-    add_authentication_token_field :token
+    add_authentication_token_field :token, encrypted: true, fallback: true
 
     before_save :update_artifacts_size, if: :artifacts_file_changed?
     before_save :ensure_token

changelogs/unreleased/fix-gb-encrypt-ci-build-token.yml

+---
+title: Encrypt CI/CD builds authentication tokens
+merge_request: 23436
+author:
+type: security

db/migrate/20181129104854_add_token_encrypted_to_ci_builds.rb

+# frozen_string_literal: true
+
+class AddTokenEncryptedToCiBuilds < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :ci_builds, :token_encrypted, :string
+  end
+end

db/migrate/20181129104944_add_index_to_ci_builds_token_encrypted.rb

+# frozen_string_literal: true
+
+class AddIndexToCiBuildsTokenEncrypted < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :ci_builds, :token_encrypted, unique: true, where: 'token_encrypted IS NOT NULL'
+  end
+
+  def down
+    remove_concurrent_index :ci_builds, :token_encrypted
+  end
+end

db/schema.rb

@@ -439,6 +439,7 @@ ActiveRecord::Schema.define(version: 20181204135932) do
     t.boolean "protected"
     t.integer "failure_reason"
     t.datetime_with_timezone "scheduled_at"
+    t.string "token_encrypted"
     t.index ["artifacts_expire_at"], name: "index_ci_builds_on_artifacts_expire_at", where: "(artifacts_file <> ''::text)", using: :btree
     t.index ["auto_canceled_by_id"], name: "index_ci_builds_on_auto_canceled_by_id", using: :btree
     t.index ["commit_id", "stage_idx", "created_at"], name: "index_ci_builds_on_commit_id_and_stage_idx_and_created_at", using: :btree
@@ -455,6 +456,7 @@ ActiveRecord::Schema.define(version: 20181204135932) do
     t.index ["stage_id"], name: "index_ci_builds_on_stage_id", using: :btree
     t.index ["status", "type", "runner_id"], name: "index_ci_builds_on_status_and_type_and_runner_id", using: :btree
     t.index ["token"], name: "index_ci_builds_on_token", unique: true, using: :btree
+    t.index ["token_encrypted"], name: "index_ci_builds_on_token_encrypted", unique: true, where: "(token_encrypted IS NOT NULL)", using: :btree
     t.index ["updated_at"], name: "index_ci_builds_on_updated_at", using: :btree
     t.index ["user_id"], name: "index_ci_builds_on_user_id", using: :btree
   end

ee/lib/ee/gitlab/auth/user_auth_finders.rb

@@ -7,21 +7,19 @@ module EE
         JOB_TOKEN_HEADER = "HTTP_JOB_TOKEN".freeze
         JOB_TOKEN_PARAM = :job_token
 
-        # rubocop: disable CodeReuse/ActiveRecord
         def find_user_from_job_token
           return unless route_authentication_setting[:job_token_allowed]
 
           token = (params[JOB_TOKEN_PARAM] || env[JOB_TOKEN_HEADER]).to_s
           return unless token.present?
 
-          job = ::Ci::Build.find_by(token: token)
+          job = ::Ci::Build.find_by_token(token)
           raise ::Gitlab::Auth::UnauthorizedError unless job
 
           @job_token_authentication = true # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
           job.user
         end
-        # rubocop: enable CodeReuse/ActiveRecord
       end
     end
   end

lib/gitlab/import_export/import_export.yml

@@ -156,6 +156,7 @@ excluded_attributes:
   statuses:
     - :trace
     - :token
+    - :token_encrypted
     - :when
     - :artifacts_file
     - :artifacts_metadata

spec/models/ci/build_spec.rb

@@ -1926,7 +1926,7 @@ describe Ci::Build do
 
     context 'when token is empty' do
       before do
-        build.token = nil
+        build.update_columns(token: nil, token_encrypted: nil)
       end
 
       it { is_expected.to be_nil}
@@ -2142,7 +2142,7 @@ describe Ci::Build do
       end
 
       before do
-        build.token = 'my-token'
+        build.set_token('my-token')
         build.yaml_variables = []
       end
 

spec/models/concerns/token_authenticatable_spec.rb

@@ -351,3 +351,89 @@ describe PersonalAccessToken, 'TokenAuthenticatable' do
     end
   end
 end
+
+describe Ci::Build, 'TokenAuthenticatable' do
+  let(:token_field) { :token }
+  let(:build) { FactoryBot.build(:ci_build) }
+
+  it_behaves_like 'TokenAuthenticatable'
+
+  describe 'generating new token' do
+    context 'token is not generated yet' do
+      describe 'token field accessor' do
+        it 'makes it possible to access token' do
+          expect(build.token).to be_nil
+
+          build.save!
+
+          expect(build.token).to be_present
+        end
+      end
+
+      describe "ensure_token" do
+        subject { build.ensure_token }
+
+        it { is_expected.to be_a String }
+        it { is_expected.not_to be_blank }
+
+        it 'does not persist token' do
+          expect(build).not_to be_persisted
+        end
+      end
+
+      describe 'ensure_token!' do
+        it 'persists a new token' do
+          expect(build.ensure_token!).to eq build.reload.token
+          expect(build).to be_persisted
+        end
+
+        it 'persists new token as an encrypted string' do
+          build.ensure_token!
+
+          encrypted = Gitlab::CryptoHelper.aes256_gcm_encrypt(build.token)
+
+          expect(build.read_attribute('token_encrypted')).to eq encrypted
+        end
+
+        it 'does not persist a token in a clear text' do
+          build.ensure_token!
+
+          expect(build.read_attribute('token')).to be_nil
+        end
+      end
+    end
+
+    describe '#reset_token!' do
+      it 'persists a new token' do
+        build.save!
+
+        build.token.yield_self do |previous_token|
+          build.reset_token!
+
+          expect(build.token).not_to eq previous_token
+          expect(build.token).to be_a String
+        end
+      end
+    end
+  end
+
+  describe 'setting a new token' do
+    subject { build.set_token('0123456789') }
+
+    it 'returns the token' do
+      expect(subject).to eq '0123456789'
+    end
+
+    it 'writes a new encrypted token' do
+      expect(build.read_attribute('token_encrypted')).to be_nil
+      expect(subject).to eq '0123456789'
+      expect(build.read_attribute('token_encrypted')).to be_present
+    end
+
+    it 'does not write a new cleartext token' do
+      expect(build.read_attribute('token')).to be_nil
+      expect(subject).to eq '0123456789'
+      expect(build.read_attribute('token')).to be_nil
+    end
+  end
+end

spec/services/ci/retry_build_service_spec.rb

@@ -20,9 +20,9 @@ describe Ci::RetryBuildService do
   CLONE_ACCESSORS = described_class::CLONE_ACCESSORS
 
   REJECT_ACCESSORS =
-    %i[id status user token coverage trace runner artifacts_expire_at
-       artifacts_file artifacts_metadata artifacts_size created_at
-       updated_at started_at finished_at queued_at erased_by
+    %i[id status user token token_encrypted coverage trace runner
+       artifacts_expire_at artifacts_file artifacts_metadata artifacts_size
+       created_at updated_at started_at finished_at queued_at erased_by
        erased_at auto_canceled_by job_artifacts job_artifacts_archive
        job_artifacts_metadata job_artifacts_trace job_artifacts_junit
        job_artifacts_sast job_artifacts_dependency_scanning