Store all secret keys in secrets.yml

Move the last secret from .secret to config/secrets.yml, and delete
.secret if it exists.

3 files changed, 53 insertions, 58 deletions

diff --git a/CHANGELOG b/CHANGELOG
index f61c4d78433..057b4c083e6 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -23,6 +23,7 @@ v 8.11.0 (unreleased)
   - Retrieve rendered HTML from cache in one request
   - Fix renaming repository when name contains invalid chararacters under project settings
   - Optimize checking if a user has read access to a list of issues !5370
+  - Store all DB secrets in secrets.yml, under descriptive names !5274
   - Nokogiri's various parsing methods are now instrumented
   - Add simple identifier to public SSH keys (muteor)
   - Add a way to send an email and create an issue based on private personal token. Find the email address from issues page. !3363
@@ -133,7 +134,6 @@ v 8.10.0
   - Add API "deploy_keys" for admins to get all deploy keys
   - Allow to pull code with deploy key from public projects
   - Use limit parameter rather than hardcoded value in `ldap:check` rake task (Mike Ricketts)
-  - Store OTP secret key in secrets.yml with other DB encryption keys
   - Add Sidekiq queue duration to transaction metrics.
   - Add a new column `artifacts_size` to table `ci_builds`. !4964
   - Let Workhorse serve format-patch diffs
diff --git a/config/initializers/secret_token.rb b/config/initializers/secret_token.rb
index 40c93c32dca..ac99dcb59fc 100644
--- a/config/initializers/secret_token.rb
+++ b/config/initializers/secret_token.rb
@@ -14,36 +14,22 @@ def create_tokens
   secret_file = Rails.root.join('.secret')
   file_key = File.read(secret_file).chomp if File.exist?(secret_file)
   env_key = ENV['SECRET_KEY_BASE']
-  secret_key_base = env_key.present? ? env_key : file_key
-
-  if secret_key_base.blank?
-    secret_key_base = generate_new_secure_token
-    File.write(secret_file, secret_key_base)
-  end
-
-  Rails.application.config.secret_key_base = secret_key_base
-
-  otp_key_base = Rails.application.secrets.otp_key_base
-  db_key_base = Rails.application.secrets.db_key_base
   yaml_additions = {}
 
-  if otp_key_base.blank?
-    warn_missing_secret('otp_key_base')
-
-    otp_key_base ||= env_key || file_key || generate_new_secure_token
-    yaml_additions['otp_key_base'] = otp_key_base
-  end
-
-  Rails.application.secrets.otp_key_base = otp_key_base
+  defaults = {
+    secret_key_base: env_key || file_key || generate_new_secure_token,
+    otp_key_base: env_key || file_key || generate_new_secure_token,
+    db_key_base: generate_new_secure_token
+  }
 
-  if db_key_base.blank?
-    warn_missing_secret('db_key_base')
+  defaults.stringify_keys.each do |key, default|
+    if Rails.application.secrets[key].blank?
+      warn_missing_secret(key)
 
-    yaml_additions['db_key_base'] = db_key_base = generate_new_secure_token
+      yaml_additions[key] = Rails.application.secrets[key] = default
+    end
   end
 
-  Rails.application.secrets.db_key_base = db_key_base
-
   unless yaml_additions.empty?
     secrets_yml = Rails.root.join('config/secrets.yml')
     all_secrets = YAML.load_file(secrets_yml) if File.exist?(secrets_yml)
@@ -54,6 +40,12 @@ def create_tokens
 
     File.write(secrets_yml, YAML.dump(all_secrets), mode: 'w', perm: 0600)
   end
+
+  begin
+    File.delete(secret_file) if file_key
+  rescue => e
+    warn "Error deleting useless .secret file: #{e}"
+  end
 end
 
 create_tokens
diff --git a/spec/initializers/secret_token_spec.rb b/spec/initializers/secret_token_spec.rb
index 063d1cdd447..aa97a8e1d42 100644
--- a/spec/initializers/secret_token_spec.rb
+++ b/spec/initializers/secret_token_spec.rb
@@ -2,37 +2,36 @@ require 'spec_helper'
 require_relative '../../config/initializers/secret_token'
 
 describe 'create_tokens', lib: true do
-  let(:config) { ActiveSupport::OrderedOptions.new }
   let(:secrets) { ActiveSupport::OrderedOptions.new }
 
   before do
     allow(ENV).to receive(:[]).and_call_original
     allow(File).to receive(:write)
-    allow(Rails).to receive_message_chain(:application, :config).and_return(config)
+    allow(File).to receive(:delete)
     allow(Rails).to receive_message_chain(:application, :secrets).and_return(secrets)
     allow(Rails).to receive_message_chain(:root, :join) { |string| string }
   end
 
-  context 'setting otp_key_base' do
+  context 'setting secret_key_base and otp_key_base' do
     context 'when none of the secrets exist' do
       before do
         allow(ENV).to receive(:[]).with('SECRET_KEY_BASE').and_return(nil)
         allow(File).to receive(:exist?).with('.secret').and_return(false)
         allow(File).to receive(:exist?).with('config/secrets.yml').and_return(false)
-        allow(File).to receive(:write)
         allow(self).to receive(:warn_missing_secret)
       end
 
       it 'generates different secrets for secret_key_base, otp_key_base, and db_key_base' do
         create_tokens
 
-        keys = [config.secret_key_base, secrets.otp_key_base, secrets.db_key_base]
+        keys = secrets.values_at(:secret_key_base, :otp_key_base, :db_key_base)
 
         expect(keys.uniq).to eq(keys)
         expect(keys.map(&:length)).to all(eq(128))
       end
 
       it 'warns about the secrets to add to secrets.yml' do
+        expect(self).to receive(:warn_missing_secret).with('secret_key_base')
         expect(self).to receive(:warn_missing_secret).with('otp_key_base')
         expect(self).to receive(:warn_missing_secret).with('db_key_base')
 
@@ -41,25 +40,20 @@ describe 'create_tokens', lib: true do
 
       it 'writes the secrets to secrets.yml' do
         expect(File).to receive(:write).with('config/secrets.yml', any_args) do |filename, contents, options|
-          new_secrets_yml = YAML.load(contents)
+          new_secrets = YAML.load(contents)[Rails.env]
 
-          expect(new_secrets_yml['test']['otp_key_base']).to eq(secrets.otp_key_base)
-          expect(new_secrets_yml['test']['db_key_base']).to eq(secrets.db_key_base)
+          expect(new_secrets['secret_key_base']).to eq(secrets.secret_key_base)
+          expect(new_secrets['otp_key_base']).to eq(secrets.otp_key_base)
+          expect(new_secrets['db_key_base']).to eq(secrets.db_key_base)
         end
 
         create_tokens
       end
 
-      it 'writes the secret_key_base to .secret' do
-        secret_key_base = nil
-
-        expect(File).to receive(:write).with('.secret', any_args) do |filename, contents|
-          secret_key_base = contents
-        end
+      it 'does not write a .secret file' do
+        expect(File).not_to receive(:write).with('.secret')
 
         create_tokens
-
-        expect(secret_key_base).to eq(config.secret_key_base)
       end
     end
 
@@ -72,8 +66,11 @@ describe 'create_tokens', lib: true do
         allow(File).to receive(:read).with('.secret').and_return('file_key')
       end
 
-      context 'when the otp_key_base secret exists' do
-        before { secrets.otp_key_base = 'otp_key_base' }
+      context 'when secret_key_base and otp_key_base exist' do
+        before do
+          secrets.secret_key_base = 'secret_key_base'
+          secrets.otp_key_base = 'otp_key_base'
+        end
 
         it 'does not write any files' do
           expect(File).not_to receive(:write)
@@ -81,22 +78,22 @@ describe 'create_tokens', lib: true do
           create_tokens
         end
 
-        it 'does not generate any new keys' do
-          expect(SecureRandom).not_to receive(:hex)
-
-          create_tokens
-        end
-
-        it 'sets the the keys to the values from the environment and secrets.yml' do
+        it 'sets the the keys to the values from secrets.yml' do
           create_tokens
 
-          expect(config.secret_key_base).to eq('env_key')
+          expect(secrets.secret_key_base).to eq('secret_key_base')
           expect(secrets.otp_key_base).to eq('otp_key_base')
           expect(secrets.db_key_base).to eq('db_key_base')
         end
+
+        it 'deletes the .secret file' do
+          expect(File).to receive(:delete).with('.secret')
+
+          create_tokens
+        end
       end
 
-      context 'when the otp_key_base secret does not exist' do
+      context 'when secret_key_base and otp_key_base do not exist' do
         before do
           allow(File).to receive(:exist?).with('config/secrets.yml').and_return(true)
           allow(YAML).to receive(:load_file).with('config/secrets.yml').and_return('test' => secrets.to_h.stringify_keys)
@@ -104,12 +101,12 @@ describe 'create_tokens', lib: true do
         end
 
         it 'uses the env secret' do
-          expect(SecureRandom).not_to receive(:hex)
           expect(File).to receive(:write) do |filename, contents, options|
-            new_secrets_yml = YAML.load(contents)
+            new_secrets = YAML.load(contents)[Rails.env]
 
-            expect(new_secrets_yml['test']['otp_key_base']).to eq('env_key')
-            expect(new_secrets_yml['test']['db_key_base']).to eq('db_key_base')
+            expect(new_secrets['secret_key_base']).to eq('env_key')
+            expect(new_secrets['otp_key_base']).to eq('env_key')
+            expect(new_secrets['db_key_base']).to eq('db_key_base')
           end
 
           create_tokens
@@ -120,15 +117,21 @@ describe 'create_tokens', lib: true do
         it 'keeps the other secrets as they were' do
           create_tokens
 
-          expect(config.secret_key_base).to eq('env_key')
           expect(secrets.db_key_base).to eq('db_key_base')
         end
 
-        it 'warns about the missing secret' do
+        it 'warns about the missing secrets' do
+          expect(self).to receive(:warn_missing_secret).with('secret_key_base')
           expect(self).to receive(:warn_missing_secret).with('otp_key_base')
 
           create_tokens
         end
+
+        it 'deletes the .secret file' do
+          expect(File).to receive(:delete).with('.secret')
+
+          create_tokens
+        end
       end
     end
   end