Encrypt application settings with pre and post deployments

We had concerns about the cached values on Redis with the previous two
releases strategy:

First release (this commit):
  - Create new encrypted fields in the database.
  - Start populating new encrypted fields, read the encrypted fields or
    fallback to the plaintext fields.
  - Backfill the data removing the plaintext fields to the encrypted
    fields.
Second release:
  - Remove the virtual attribute (created in step 2).
  - Drop plaintext columns from the database (empty columns after
    step 3).

We end up with a better strategy only using migration scripts in one
release:
  - Pre-deployment migration: Add columns required for storing encrypted
    values.
  - Pre-deployment migration: Store the encrypted values in the new
    columns.
  - Post-deployment migration: Remove the old unencrypted columns

6 files changed, 31 insertions, 83 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index d18f96d82f7..fb702b3898e 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -322,30 +322,6 @@ class ApplicationSetting < ApplicationRecord
     Gitlab::ThreadMemoryCache.cache_backend
   end
 
-  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
-
   def recaptcha_or_login_protection_enabled
     recaptcha_enabled || login_recaptcha_protection_enabled
   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
index d7abb29fd75..e6b9a40ad4f 100644
--- a/db/migrate/20191120115530_encrypt_plaintext_attributes_on_application_settings.rb
+++ b/db/migrate/20191120115530_encrypt_plaintext_attributes_on_application_settings.rb
@@ -70,12 +70,6 @@ class EncryptPlaintextAttributesOnApplicationSettings < ActiveRecord::Migration[
         end
       )
       application_setting.save(validate: false)
-
-      application_setting.update_columns(
-        PLAINTEXT_ATTRIBUTES.each_with_object({}) do |plaintext_attribute, attributes|
-          attributes[plaintext_attribute] = nil
-        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 2994fb20770..1e722b519eb 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_20_115530) 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"
@@ -179,11 +179,8 @@ ActiveRecord::Schema.define(version: 2019_11_20_115530) 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"
@@ -230,7 +227,6 @@ ActiveRecord::Schema.define(version: 2019_11_20_115530) 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
@@ -246,8 +242,6 @@ ActiveRecord::Schema.define(version: 2019_11_20_115530) 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
diff --git a/spec/migrations/encrypt_plaintext_attributes_on_application_settings_spec.rb b/spec/migrations/encrypt_plaintext_attributes_on_application_settings_spec.rb
index 6435e43f38c..122da7b3d72 100644
--- a/spec/migrations/encrypt_plaintext_attributes_on_application_settings_spec.rb
+++ b/spec/migrations/encrypt_plaintext_attributes_on_application_settings_spec.rb
@@ -18,7 +18,7 @@ describe EncryptPlaintextAttributesOnApplicationSettings, :migration do
   ].freeze
 
   describe '#up' do
-    it 'encrypts token, saves it and removes plaintext token' 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|
@@ -30,7 +30,7 @@ describe EncryptPlaintextAttributesOnApplicationSettings, :migration do
 
       application_setting.reload
       PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute|
-        expect(application_setting[plaintext_attribute]).to be_nil
+        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
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 619d4447941..7bef3d30064 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -13,50 +13,6 @@ describe ApplicationSetting do
   it { expect(setting.uuid).to be_present }
   it { expect(setting).to have_db_column(:auto_devops_enabled) }
 
-  context "with existing plaintext attributes" do
-    before do
-      setting.update_columns(
-        akismet_api_key: "akismet_api_key",
-        elasticsearch_aws_secret_access_key: "elasticsearch_aws_secret_access_key",
-        recaptcha_private_key: "recaptcha_private_key",
-        recaptcha_site_key: "recaptcha_site_key",
-        slack_app_secret: "slack_app_secret",
-        slack_app_verification_token: "slack_app_verification_token"
-      )
-    end
-
-    it "returns the attributes" do
-      expect(setting.akismet_api_key).to eq("akismet_api_key")
-      expect(setting.elasticsearch_aws_secret_access_key).to eq("elasticsearch_aws_secret_access_key")
-      expect(setting.recaptcha_private_key).to eq("recaptcha_private_key")
-      expect(setting.recaptcha_site_key).to eq("recaptcha_site_key")
-      expect(setting.slack_app_secret).to eq("slack_app_secret")
-      expect(setting.slack_app_verification_token).to eq("slack_app_verification_token")
-    end
-  end
-
-  context "with encrypted attributes" do
-    before do
-      setting.update(
-        akismet_api_key: "akismet_api_key",
-        elasticsearch_aws_secret_access_key: "elasticsearch_aws_secret_access_key",
-        recaptcha_private_key: "recaptcha_private_key",
-        recaptcha_site_key: "recaptcha_site_key",
-        slack_app_secret: "slack_app_secret",
-        slack_app_verification_token: "slack_app_verification_token"
-      )
-    end
-
-    it "returns the attributes" do
-      expect(setting.akismet_api_key).to eq("akismet_api_key")
-      expect(setting.elasticsearch_aws_secret_access_key).to eq("elasticsearch_aws_secret_access_key")
-      expect(setting.recaptcha_private_key).to eq("recaptcha_private_key")
-      expect(setting.recaptcha_site_key).to eq("recaptcha_site_key")
-      expect(setting.slack_app_secret).to eq("slack_app_secret")
-      expect(setting.slack_app_verification_token).to eq("slack_app_verification_token")
-    end
-  end
-
   describe 'validations' do
     let(:http)  { 'http://example.com' }
     let(:https) { 'https://example.com' }