Merge branch 'security-2943-encrypt-plaintext-tokens-12-5' into '12-5-stable'

GitLab stores AWS, Slack, Askimet, reCaptcha tokens in plaintext

See merge request gitlab/gitlabhq!3543

7 files changed, 239 insertions, 29 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 4028d711fd1..72605af433f 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -313,29 +313,25 @@ class ApplicationSetting < ApplicationRecord
                  algorithm: 'aes-256-cbc',
                  insecure_mode: true
 
-  attr_encrypted :external_auth_client_key,
-                 mode: :per_attribute_iv,
-                 key: Settings.attr_encrypted_db_key_base_truncated,
-                 algorithm: 'aes-256-gcm',
-                 encode: true
-
-  attr_encrypted :external_auth_client_key_pass,
-                 mode: :per_attribute_iv,
-                 key: Settings.attr_encrypted_db_key_base_truncated,
-                 algorithm: 'aes-256-gcm',
-                 encode: true
-
-  attr_encrypted :lets_encrypt_private_key,
-                 mode: :per_attribute_iv,
-                 key: Settings.attr_encrypted_db_key_base_truncated,
-                 algorithm: 'aes-256-gcm',
-                 encode: true
+  private_class_method def self.encryption_options_base_truncated_aes_256_gcm
+    {
+      mode: :per_attribute_iv,
+      key: Settings.attr_encrypted_db_key_base_truncated,
+      algorithm: 'aes-256-gcm',
+      encode: true
+    }
+  end
 
-  attr_encrypted :eks_secret_access_key,
-                 mode: :per_attribute_iv,
-                 key: Settings.attr_encrypted_db_key_base_truncated,
-                 algorithm: 'aes-256-gcm',
-                 encode: true
+  attr_encrypted :external_auth_client_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :external_auth_client_key_pass, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :lets_encrypt_private_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :eks_secret_access_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :akismet_api_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :elasticsearch_aws_secret_access_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :recaptcha_private_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :recaptcha_site_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :slack_app_secret, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :slack_app_verification_token, encryption_options_base_truncated_aes_256_gcm
 
   before_validation :ensure_uuid!
 
diff --git a/changelogs/unreleased/security-2943-encrypt-plaintext-tokens.yml b/changelogs/unreleased/security-2943-encrypt-plaintext-tokens.yml
new file mode 100644
index 00000000000..d040565da73
--- /dev/null
+++ b/changelogs/unreleased/security-2943-encrypt-plaintext-tokens.yml
@@ -0,0 +1,5 @@
+---
+title: Encrypt application setting tokens
+merge_request:
+author:
+type: security
diff --git a/db/migrate/20191120084627_add_encrypted_fields_to_application_settings.rb b/db/migrate/20191120084627_add_encrypted_fields_to_application_settings.rb
new file mode 100644
index 00000000000..4e0886a5121
--- /dev/null
+++ b/db/migrate/20191120084627_add_encrypted_fields_to_application_settings.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+class AddEncryptedFieldsToApplicationSettings < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  PLAINTEXT_ATTRIBUTES = %w[
+    akismet_api_key
+    elasticsearch_aws_secret_access_key
+    recaptcha_private_key
+    recaptcha_site_key
+    slack_app_secret
+    slack_app_verification_token
+  ].freeze
+
+  def up
+    PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute|
+      add_column :application_settings, "encrypted_#{plaintext_attribute}", :text
+      add_column :application_settings, "encrypted_#{plaintext_attribute}_iv", :string, limit: 255
+    end
+  end
+
+  def down
+    PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute|
+      remove_column :application_settings, "encrypted_#{plaintext_attribute}"
+      remove_column :application_settings, "encrypted_#{plaintext_attribute}_iv"
+    end
+  end
+end
diff --git a/db/migrate/20191120115530_encrypt_plaintext_attributes_on_application_settings.rb b/db/migrate/20191120115530_encrypt_plaintext_attributes_on_application_settings.rb
new file mode 100644
index 00000000000..e6b9a40ad4f
--- /dev/null
+++ b/db/migrate/20191120115530_encrypt_plaintext_attributes_on_application_settings.rb
@@ -0,0 +1,87 @@
+# frozen_string_literal: true
+
+class EncryptPlaintextAttributesOnApplicationSettings < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  PLAINTEXT_ATTRIBUTES = %w[
+    akismet_api_key
+    elasticsearch_aws_secret_access_key
+    recaptcha_private_key
+    recaptcha_site_key
+    slack_app_secret
+    slack_app_verification_token
+  ].freeze
+
+  class ApplicationSetting < ActiveRecord::Base
+    self.table_name = 'application_settings'
+
+    def self.encryption_options_base_truncated_aes_256_gcm
+      {
+        mode: :per_attribute_iv,
+        key: Gitlab::Application.secrets.db_key_base[0..31],
+        algorithm: 'aes-256-gcm',
+        encode: true
+      }
+    end
+
+    attr_encrypted :akismet_api_key, encryption_options_base_truncated_aes_256_gcm
+    attr_encrypted :elasticsearch_aws_secret_access_key, encryption_options_base_truncated_aes_256_gcm
+    attr_encrypted :recaptcha_private_key, encryption_options_base_truncated_aes_256_gcm
+    attr_encrypted :recaptcha_site_key, encryption_options_base_truncated_aes_256_gcm
+    attr_encrypted :slack_app_secret, encryption_options_base_truncated_aes_256_gcm
+    attr_encrypted :slack_app_verification_token, encryption_options_base_truncated_aes_256_gcm
+
+    def akismet_api_key
+      decrypt(:akismet_api_key, self[:encrypted_akismet_api_key]) || self[:akismet_api_key]
+    end
+
+    def elasticsearch_aws_secret_access_key
+      decrypt(:elasticsearch_aws_secret_access_key, self[:encrypted_elasticsearch_aws_secret_access_key]) || self[:elasticsearch_aws_secret_access_key]
+    end
+
+    def recaptcha_private_key
+      decrypt(:recaptcha_private_key, self[:encrypted_recaptcha_private_key]) || self[:recaptcha_private_key]
+    end
+
+    def recaptcha_site_key
+      decrypt(:recaptcha_site_key, self[:encrypted_recaptcha_site_key]) || self[:recaptcha_site_key]
+    end
+
+    def slack_app_secret
+      decrypt(:slack_app_secret, self[:encrypted_slack_app_secret]) || self[:slack_app_secret]
+    end
+
+    def slack_app_verification_token
+      decrypt(:slack_app_verification_token, self[:encrypted_slack_app_verification_token]) || self[:slack_app_verification_token]
+    end
+  end
+
+  def up
+    ApplicationSetting.find_each do |application_setting|
+      # We are using the setter from attr_encrypted gem to encrypt the data.
+      # The gem updates the two columns needed to decrypt the value:
+      # - "encrypted_#{plaintext_attribute}"
+      # - "encrypted_#{plaintext_attribute}_iv"
+      application_setting.assign_attributes(
+        PLAINTEXT_ATTRIBUTES.each_with_object({}) do |plaintext_attribute, attributes|
+          attributes[plaintext_attribute] = application_setting.send(plaintext_attribute)
+        end
+      )
+      application_setting.save(validate: false)
+    end
+  end
+
+  def down
+    ApplicationSetting.find_each do |application_setting|
+      application_setting.update_columns(
+        PLAINTEXT_ATTRIBUTES.each_with_object({}) do |plaintext_attribute, attributes|
+          attributes[plaintext_attribute] = application_setting.send(plaintext_attribute)
+          attributes["encrypted_#{plaintext_attribute}"] = nil
+          attributes["encrypted_#{plaintext_attribute}_iv"] = nil
+        end
+      )
+    end
+  end
+end
diff --git a/db/post_migrate/20191122135327_remove_plaintext_columns_from_application_settings.rb b/db/post_migrate/20191122135327_remove_plaintext_columns_from_application_settings.rb
new file mode 100644
index 00000000000..b5cd58b10a8
--- /dev/null
+++ b/db/post_migrate/20191122135327_remove_plaintext_columns_from_application_settings.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+class RemovePlaintextColumnsFromApplicationSettings < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  PLAINTEXT_ATTRIBUTES = %w[
+    akismet_api_key
+    elasticsearch_aws_secret_access_key
+    recaptcha_private_key
+    recaptcha_site_key
+    slack_app_secret
+    slack_app_verification_token
+  ].freeze
+
+  def up
+    PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute|
+      remove_column :application_settings, plaintext_attribute
+    end
+  end
+
+  def down
+    PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute|
+      add_column :application_settings, plaintext_attribute, :text
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index e3413722991..0dce19a29d7 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_11_15_091425) do
+ActiveRecord::Schema.define(version: 2019_11_22_135327) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -180,11 +180,8 @@ ActiveRecord::Schema.define(version: 2019_11_15_091425) do
     t.integer "metrics_timeout", default: 10
     t.integer "metrics_method_call_threshold", default: 10
     t.boolean "recaptcha_enabled", default: false
-    t.string "recaptcha_site_key"
-    t.string "recaptcha_private_key"
     t.integer "metrics_port", default: 8089
     t.boolean "akismet_enabled", default: false
-    t.string "akismet_api_key"
     t.integer "metrics_sample_interval", default: 15
     t.boolean "email_author_in_body", default: false
     t.integer "default_group_visibility"
@@ -231,7 +228,6 @@ ActiveRecord::Schema.define(version: 2019_11_15_091425) do
     t.boolean "elasticsearch_aws", default: false, null: false
     t.string "elasticsearch_aws_region", default: "us-east-1"
     t.string "elasticsearch_aws_access_key"
-    t.string "elasticsearch_aws_secret_access_key"
     t.integer "geo_status_timeout", default: 10
     t.string "uuid"
     t.decimal "polling_interval_multiplier", default: "1.0", null: false
@@ -247,8 +243,6 @@ ActiveRecord::Schema.define(version: 2019_11_15_091425) do
     t.string "help_page_support_url"
     t.boolean "slack_app_enabled", default: false
     t.string "slack_app_id"
-    t.string "slack_app_secret"
-    t.string "slack_app_verification_token"
     t.integer "performance_bar_allowed_group_id"
     t.boolean "allow_group_owners_to_manage_ldap", default: true, null: false
     t.boolean "hashed_storage_enabled", default: true, null: false
@@ -355,6 +349,18 @@ ActiveRecord::Schema.define(version: 2019_11_15_091425) do
     t.boolean "sourcegraph_enabled", default: false, null: false
     t.string "sourcegraph_url", limit: 255
     t.boolean "sourcegraph_public_only", default: true, null: false
+    t.text "encrypted_akismet_api_key"
+    t.string "encrypted_akismet_api_key_iv", limit: 255
+    t.text "encrypted_elasticsearch_aws_secret_access_key"
+    t.string "encrypted_elasticsearch_aws_secret_access_key_iv", limit: 255
+    t.text "encrypted_recaptcha_private_key"
+    t.string "encrypted_recaptcha_private_key_iv", limit: 255
+    t.text "encrypted_recaptcha_site_key"
+    t.string "encrypted_recaptcha_site_key_iv", limit: 255
+    t.text "encrypted_slack_app_secret"
+    t.string "encrypted_slack_app_secret_iv", limit: 255
+    t.text "encrypted_slack_app_verification_token"
+    t.string "encrypted_slack_app_verification_token_iv", limit: 255
     t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id"
     t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id"
     t.index ["instance_administration_project_id"], name: "index_applicationsettings_on_instance_administration_project_id"
diff --git a/spec/migrations/encrypt_plaintext_attributes_on_application_settings_spec.rb b/spec/migrations/encrypt_plaintext_attributes_on_application_settings_spec.rb
new file mode 100644
index 00000000000..122da7b3d72
--- /dev/null
+++ b/spec/migrations/encrypt_plaintext_attributes_on_application_settings_spec.rb
@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20191120115530_encrypt_plaintext_attributes_on_application_settings.rb')
+
+describe EncryptPlaintextAttributesOnApplicationSettings, :migration do
+  let(:migration) { described_class.new }
+  let(:application_settings) { table(:application_settings) }
+  let(:plaintext) { 'secret-token' }
+
+  PLAINTEXT_ATTRIBUTES = %w[
+    akismet_api_key
+    elasticsearch_aws_secret_access_key
+    recaptcha_private_key
+    recaptcha_site_key
+    slack_app_secret
+    slack_app_verification_token
+  ].freeze
+
+  describe '#up' do
+    it 'encrypts token and saves it' do
+      application_setting = application_settings.create
+      application_setting.update_columns(
+        PLAINTEXT_ATTRIBUTES.each_with_object({}) do |plaintext_attribute, attributes|
+          attributes[plaintext_attribute] = plaintext
+        end
+      )
+
+      migration.up
+
+      application_setting.reload
+      PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute|
+        expect(application_setting[plaintext_attribute]).not_to be_nil
+        expect(application_setting["encrypted_#{plaintext_attribute}"]).not_to be_nil
+        expect(application_setting["encrypted_#{plaintext_attribute}_iv"]).not_to be_nil
+      end
+    end
+  end
+
+  describe '#down' do
+    it 'decrypts encrypted token and saves it' do
+      application_setting = application_settings.create(
+        PLAINTEXT_ATTRIBUTES.each_with_object({}) do |plaintext_attribute, attributes|
+          attributes[plaintext_attribute] = plaintext
+        end
+      )
+
+      migration.down
+
+      application_setting.reload
+      PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute|
+        expect(application_setting[plaintext_attribute]).to eq(plaintext)
+        expect(application_setting["encrypted_#{plaintext_attribute}"]).to be_nil
+        expect(application_setting["encrypted_#{plaintext_attribute}_iv"]).to be_nil
+      end
+    end
+  end
+end