Encrypt CI/CD builds tokens
Brings back 1e8f1de0
reverted in !23644
Closes #52342
See merge request gitlab-org/gitlab-ce!23436
This commit is contained in:
parent
a2e06ad303
commit
a910c09bbc
9 changed files with 128 additions and 6 deletions
|
@ -120,7 +120,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
|
||||
|
|
5
changelogs/unreleased/fix-gb-encrypt-ci-build-token.yml
Normal file
5
changelogs/unreleased/fix-gb-encrypt-ci-build-token.yml
Normal file
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Encrypt CI/CD builds authentication tokens
|
||||
merge_request: 23436
|
||||
author:
|
||||
type: security
|
|
@ -0,0 +1,11 @@
|
|||
# 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
|
|
@ -0,0 +1,17 @@
|
|||
# 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
|
|
@ -345,6 +345,7 @@ ActiveRecord::Schema.define(version: 20181203002526) 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
|
||||
|
@ -361,6 +362,7 @@ ActiveRecord::Schema.define(version: 20181203002526) 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
|
||||
|
|
|
@ -143,6 +143,7 @@ excluded_attributes:
|
|||
statuses:
|
||||
- :trace
|
||||
- :token
|
||||
- :token_encrypted
|
||||
- :when
|
||||
- :artifacts_file
|
||||
- :artifacts_metadata
|
||||
|
|
|
@ -1925,7 +1925,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}
|
||||
|
@ -2141,7 +2141,7 @@ describe Ci::Build do
|
|||
end
|
||||
|
||||
before do
|
||||
build.token = 'my-token'
|
||||
build.set_token('my-token')
|
||||
build.yaml_variables = []
|
||||
end
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in a new issue