From 3394c6532e4ef4582d806aed8c5285bf462fea3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= Date: Thu, 11 Jul 2019 16:41:04 -0400 Subject: [PATCH 1/9] Added migration to encrypt token in DeployToken records Added migrations to make token column accepting null values and to add encrypted token column. --- ...053_change_deploy_tokens_token_not_null.rb | 11 ++++++++ ...08_add_token_encrypted_to_deploy_tokens.rb | 11 ++++++++ ...0711201818_encrypt_deploy_tokens_tokens.rb | 27 +++++++++++++++++++ db/schema.rb | 3 ++- 4 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20190711200053_change_deploy_tokens_token_not_null.rb create mode 100644 db/migrate/20190711200508_add_token_encrypted_to_deploy_tokens.rb create mode 100644 db/post_migrate/20190711201818_encrypt_deploy_tokens_tokens.rb diff --git a/db/migrate/20190711200053_change_deploy_tokens_token_not_null.rb b/db/migrate/20190711200053_change_deploy_tokens_token_not_null.rb new file mode 100644 index 00000000000..14ccf544d0b --- /dev/null +++ b/db/migrate/20190711200053_change_deploy_tokens_token_not_null.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class ChangeDeployTokensTokenNotNull < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + change_column_null :deploy_tokens, :token, true + end +end diff --git a/db/migrate/20190711200508_add_token_encrypted_to_deploy_tokens.rb b/db/migrate/20190711200508_add_token_encrypted_to_deploy_tokens.rb new file mode 100644 index 00000000000..3c352441057 --- /dev/null +++ b/db/migrate/20190711200508_add_token_encrypted_to_deploy_tokens.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AddTokenEncryptedToDeployTokens < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :deploy_tokens, :token_encrypted, :string + end +end diff --git a/db/post_migrate/20190711201818_encrypt_deploy_tokens_tokens.rb b/db/post_migrate/20190711201818_encrypt_deploy_tokens_tokens.rb new file mode 100644 index 00000000000..6245a87dc62 --- /dev/null +++ b/db/post_migrate/20190711201818_encrypt_deploy_tokens_tokens.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class EncryptDeployTokensTokens < ActiveRecord::Migration[5.1] + DOWNTIME = false + + class DeploymentTokens < ActiveRecord::Base + self.table_name = 'deploy_tokens' + end + + def up + say_with_time("Encrypting tokens from deploy_tokens") do + DeploymentTokens.where('token_encrypted is NULL AND token IS NOT NULL').find_each do |deploy_token| + token_encrypted = Gitlab::CryptoHelper.aes256_gcm_encrypt(deploy_token.token) + deploy_token.update!(token_encrypted: token_encrypted) + end + end + end + + def down + say_with_time("Decrypting tokens from deploy_tokens") do + DeploymentTokens.where('token_encrypted IS NOT NULL AND token IS NULL').find_each do |deploy_token| + token = Gitlab::CryptoHelper.aes256_gcm_decrypt(deploy_token.token_encrypted) + deploy_token.update!(token: token) + end + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 36bf2371994..f36ee372382 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1121,8 +1121,9 @@ ActiveRecord::Schema.define(version: 2019_08_20_163320) do t.datetime_with_timezone "expires_at", null: false t.datetime_with_timezone "created_at", null: false t.string "name", null: false - t.string "token", null: false + t.string "token" t.string "username" + t.string "token_encrypted" t.index ["token", "expires_at", "id"], name: "index_deploy_tokens_on_token_and_expires_at_and_id", where: "(revoked IS FALSE)" t.index ["token"], name: "index_deploy_tokens_on_token", unique: true end From 63ade4b819bd222eb436f271a4a919a199bc023b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= Date: Fri, 12 Jul 2019 11:24:18 -0400 Subject: [PATCH 2/9] Added EncryptDeployTokensTokens spec file --- .../encrypt_deploy_tokens_tokens_spec.rb | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 spec/migrations/encrypt_deploy_tokens_tokens_spec.rb diff --git a/spec/migrations/encrypt_deploy_tokens_tokens_spec.rb b/spec/migrations/encrypt_deploy_tokens_tokens_spec.rb new file mode 100644 index 00000000000..a398e079731 --- /dev/null +++ b/spec/migrations/encrypt_deploy_tokens_tokens_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require Rails.root.join('db', 'post_migrate', '20190711201818_encrypt_deploy_tokens_tokens.rb') + +describe EncryptDeployTokensTokens, :migration do + let(:migration) { described_class.new } + let(:deployment_tokens) { table(:deploy_tokens) } + let(:plaintext) { "secret-token" } + let(:expires_at) { DateTime.now + 1.year } + let(:ciphertext) { Gitlab::CryptoHelper.aes256_gcm_encrypt(plaintext) } + + describe '#up' do + it 'keeps plaintext token the same and populates token_encrypted if not present' do + deploy_token = deployment_tokens.create!( + name: 'test_token', + read_repository: true, + expires_at: expires_at, + username: 'gitlab-token-1', + token: plaintext + ) + + migration.up + + expect(deploy_token.reload.token).to eq(plaintext) + expect(deploy_token.reload.token_encrypted).to eq(ciphertext) + end + end + + describe '#down' do + it 'decrypts encrypted token and saves it' do + deploy_token = deployment_tokens.create!( + name: 'test_token', + read_repository: true, + expires_at: expires_at, + username: 'gitlab-token-1', + token_encrypted: ciphertext + ) + + migration.down + + expect(deploy_token.reload.token).to eq(plaintext) + expect(deploy_token.reload.token_encrypted).to eq(ciphertext) + end + end +end From 93cf4124737b20e3f810f0c0ad366271a9a0c251 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= Date: Fri, 12 Jul 2019 16:49:47 -0400 Subject: [PATCH 3/9] Add encrypted optional option to DeployToken authentication field --- app/models/deploy_token.rb | 2 +- config/database.yml.example | 0 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 config/database.yml.example diff --git a/app/models/deploy_token.rb b/app/models/deploy_token.rb index 33f0be91632..85f5a2040c0 100644 --- a/app/models/deploy_token.rb +++ b/app/models/deploy_token.rb @@ -5,7 +5,7 @@ class DeployToken < ApplicationRecord include TokenAuthenticatable include PolicyActor include Gitlab::Utils::StrongMemoize - add_authentication_token_field :token + add_authentication_token_field :token, encrypted: :optional AVAILABLE_SCOPES = %i(read_repository read_registry).freeze GITLAB_DEPLOY_TOKEN_NAME = 'gitlab-deploy-token'.freeze diff --git a/config/database.yml.example b/config/database.yml.example new file mode 100644 index 00000000000..e69de29bb2d From 3e48d419318b319ba5629669472234ec7ecd77d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= Date: Mon, 15 Jul 2019 11:39:01 -0400 Subject: [PATCH 4/9] Added changelog file for this security fix --- changelogs/unreleased/63502-encrypt-deploy-token.yml | 5 +++++ config/database.yml.example | 0 2 files changed, 5 insertions(+) create mode 100644 changelogs/unreleased/63502-encrypt-deploy-token.yml delete mode 100644 config/database.yml.example diff --git a/changelogs/unreleased/63502-encrypt-deploy-token.yml b/changelogs/unreleased/63502-encrypt-deploy-token.yml new file mode 100644 index 00000000000..249f52014cd --- /dev/null +++ b/changelogs/unreleased/63502-encrypt-deploy-token.yml @@ -0,0 +1,5 @@ +--- +title: Encrypt existing deploy tokens and new ones when storing them +merge_request: 30679 +author: +type: security diff --git a/config/database.yml.example b/config/database.yml.example deleted file mode 100644 index e69de29bb2d..00000000000 From a6b60fee67f6ea4d22ab5cb34901d0707829eec0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= Date: Wed, 17 Jul 2019 09:38:28 -0400 Subject: [PATCH 5/9] Iterating through token to encrypt with find_each --- .../20190711201818_encrypt_deploy_tokens_tokens.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/post_migrate/20190711201818_encrypt_deploy_tokens_tokens.rb b/db/post_migrate/20190711201818_encrypt_deploy_tokens_tokens.rb index 6245a87dc62..2eb8d1ee11c 100644 --- a/db/post_migrate/20190711201818_encrypt_deploy_tokens_tokens.rb +++ b/db/post_migrate/20190711201818_encrypt_deploy_tokens_tokens.rb @@ -9,7 +9,7 @@ class EncryptDeployTokensTokens < ActiveRecord::Migration[5.1] def up say_with_time("Encrypting tokens from deploy_tokens") do - DeploymentTokens.where('token_encrypted is NULL AND token IS NOT NULL').find_each do |deploy_token| + DeploymentTokens.where('token_encrypted is NULL AND token IS NOT NULL').find_each(batch_size: 10000) do |deploy_token| token_encrypted = Gitlab::CryptoHelper.aes256_gcm_encrypt(deploy_token.token) deploy_token.update!(token_encrypted: token_encrypted) end @@ -18,7 +18,7 @@ class EncryptDeployTokensTokens < ActiveRecord::Migration[5.1] def down say_with_time("Decrypting tokens from deploy_tokens") do - DeploymentTokens.where('token_encrypted IS NOT NULL AND token IS NULL').find_each do |deploy_token| + DeploymentTokens.where('token_encrypted IS NOT NULL AND token IS NULL').find_each(batch_size: 10000) do |deploy_token| token = Gitlab::CryptoHelper.aes256_gcm_decrypt(deploy_token.token_encrypted) deploy_token.update!(token: token) end From 84d6dcbe5040b9f0475f6b2155800617e397db94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= Date: Fri, 19 Jul 2019 13:44:30 -0400 Subject: [PATCH 6/9] Updated call to find deploy token --- lib/gitlab/auth.rb | 3 +-- spec/factories/deploy_tokens.rb | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index e17a096ef19..fe40d553b2f 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -202,8 +202,7 @@ module Gitlab def deploy_token_check(login, password) return unless password.present? - token = - DeployToken.active.find_by(token: password) + token = DeployToken.active.find_by_token(password) return unless token && login return if login != token.username diff --git a/spec/factories/deploy_tokens.rb b/spec/factories/deploy_tokens.rb index a96258f5cbe..99486acc2ab 100644 --- a/spec/factories/deploy_tokens.rb +++ b/spec/factories/deploy_tokens.rb @@ -2,7 +2,8 @@ FactoryBot.define do factory :deploy_token do - token { SecureRandom.hex(50) } + token nil + token_encrypted { Gitlab::CryptoHelper.aes256_gcm_encrypt( SecureRandom.hex(50) ) } sequence(:name) { |n| "PDT #{n}" } read_repository true read_registry true From 64e24d6492f39a4ab7f2202897755afd1704f147 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= Date: Fri, 19 Jul 2019 14:00:56 -0400 Subject: [PATCH 7/9] Added new index to deploy_tokens table --- ...dd_index_to_deploy_tokens_token_encrypted.rb | 17 +++++++++++++++++ db/schema.rb | 1 + 2 files changed, 18 insertions(+) create mode 100644 db/migrate/20190719174505_add_index_to_deploy_tokens_token_encrypted.rb diff --git a/db/migrate/20190719174505_add_index_to_deploy_tokens_token_encrypted.rb b/db/migrate/20190719174505_add_index_to_deploy_tokens_token_encrypted.rb new file mode 100644 index 00000000000..d58d1d8348c --- /dev/null +++ b/db/migrate/20190719174505_add_index_to_deploy_tokens_token_encrypted.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddIndexToDeployTokensTokenEncrypted < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :deploy_tokens, :token_encrypted, unique: true, name: "index_deploy_tokens_on_token_encrypted" + end + + def down + remove_concurrent_index_by_name :deploy_tokens, "index_deploy_tokens_on_token_encrypted" + end +end diff --git a/db/schema.rb b/db/schema.rb index f36ee372382..1393737b8b9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1126,6 +1126,7 @@ ActiveRecord::Schema.define(version: 2019_08_20_163320) do t.string "token_encrypted" t.index ["token", "expires_at", "id"], name: "index_deploy_tokens_on_token_and_expires_at_and_id", where: "(revoked IS FALSE)" t.index ["token"], name: "index_deploy_tokens_on_token", unique: true + t.index ["token_encrypted"], name: "index_deploy_tokens_on_token_encrypted", unique: true end create_table "deployments", id: :serial, force: :cascade do |t| From bc4efd18a06e0b31a30b8e63828506ad647b2d8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= Date: Fri, 19 Jul 2019 15:16:07 -0400 Subject: [PATCH 8/9] Removed rubocop disable flags, updated changelog --- changelogs/unreleased/63502-encrypt-deploy-token.yml | 4 ++-- lib/gitlab/auth.rb | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/changelogs/unreleased/63502-encrypt-deploy-token.yml b/changelogs/unreleased/63502-encrypt-deploy-token.yml index 249f52014cd..81ce1e6c3dd 100644 --- a/changelogs/unreleased/63502-encrypt-deploy-token.yml +++ b/changelogs/unreleased/63502-encrypt-deploy-token.yml @@ -1,5 +1,5 @@ --- -title: Encrypt existing deploy tokens and new ones when storing them +title: Encrypt existing and new deploy tokens merge_request: 30679 author: -type: security +type: other diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index fe40d553b2f..6769bd95c2b 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -198,7 +198,6 @@ module Gitlab end.uniq end - # rubocop: disable CodeReuse/ActiveRecord def deploy_token_check(login, password) return unless password.present? @@ -213,7 +212,6 @@ module Gitlab Gitlab::Auth::Result.new(token, token.project, :deploy_token, scopes) end end - # rubocop: enable CodeReuse/ActiveRecord def lfs_token_check(login, encoded_token, project) deploy_key_matches = login.match(/\Alfs\+deploy-key-(\d+)\z/) From 2dd6f423b77c82436e3e0b3978d9bda513207b4b Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 26 Aug 2019 19:44:28 -0700 Subject: [PATCH 9/9] Add limit: 255 to token_ecnrypted column --- .../20190711200508_add_token_encrypted_to_deploy_tokens.rb | 2 +- db/schema.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/db/migrate/20190711200508_add_token_encrypted_to_deploy_tokens.rb b/db/migrate/20190711200508_add_token_encrypted_to_deploy_tokens.rb index 3c352441057..ea0956fdf7f 100644 --- a/db/migrate/20190711200508_add_token_encrypted_to_deploy_tokens.rb +++ b/db/migrate/20190711200508_add_token_encrypted_to_deploy_tokens.rb @@ -6,6 +6,6 @@ class AddTokenEncryptedToDeployTokens < ActiveRecord::Migration[5.1] DOWNTIME = false def change - add_column :deploy_tokens, :token_encrypted, :string + add_column :deploy_tokens, :token_encrypted, :string, limit: 255 end end diff --git a/db/schema.rb b/db/schema.rb index 1393737b8b9..f30dad3d030 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1123,7 +1123,7 @@ ActiveRecord::Schema.define(version: 2019_08_20_163320) do t.string "name", null: false t.string "token" t.string "username" - t.string "token_encrypted" + t.string "token_encrypted", limit: 255 t.index ["token", "expires_at", "id"], name: "index_deploy_tokens_on_token_and_expires_at_and_id", where: "(revoked IS FALSE)" t.index ["token"], name: "index_deploy_tokens_on_token", unique: true t.index ["token_encrypted"], name: "index_deploy_tokens_on_token_encrypted", unique: true