Store OTP secret key in secrets.yml

.secret stores the secret token used for both encrypting login cookies
and for encrypting stored OTP secrets. We can't rotate this, because
that would invalidate all existing OTP secrets.

If the secret token is present in the .secret file or an environment
variable, save it as otp_key_base in secrets.yml. Now .secret can be
rotated without invalidating OTP secrets.

If the secret token isn't present (initial setup), then just generate a
separate otp_key_base and save in secrets.yml.

Update the docs to reflect that secrets.yml needs to be retained past
upgrades, but .secret doesn't.

6 files changed, 199 insertions, 53 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 472faa05b75..f61c4d78433 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -133,6 +133,7 @@ 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/app/models/user.rb b/app/models/user.rb
index db747434959..73368be7b1b 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -23,13 +23,13 @@ class User < ActiveRecord::Base
   default_value_for :theme_id, gitlab_config.default_theme
 
   attr_encrypted :otp_secret,
-    key:       Gitlab::Application.config.secret_key_base,
+    key:       Gitlab::Application.secrets.otp_key_base,
     mode:      :per_attribute_iv_and_salt,
     insecure_mode: true,
     algorithm: 'aes-256-cbc'
 
   devise :two_factor_authenticatable,
-         otp_secret_encryption_key: Gitlab::Application.config.secret_key_base
+         otp_secret_encryption_key: Gitlab::Application.secrets.otp_key_base
 
   devise :two_factor_backupable, otp_number_of_backup_codes: 10
   serialize :otp_backup_codes, JSON
diff --git a/config/initializers/secret_token.rb b/config/initializers/secret_token.rb
index dae3a4a9a93..40c93c32dca 100644
--- a/config/initializers/secret_token.rb
+++ b/config/initializers/secret_token.rb
@@ -2,49 +2,58 @@
 
 require 'securerandom'
 
-# Your secret key for verifying the integrity of signed cookies.
-# If you change this key, all old signed cookies will become invalid!
-# Make sure the secret is at least 30 characters and all random,
-# no regular words or you'll be exposed to dictionary attacks.
-
-def find_secure_token
-  token_file = Rails.root.join('.secret')
-  if ENV.key?('SECRET_KEY_BASE')
-    ENV['SECRET_KEY_BASE']
-  elsif File.exist? token_file
-    # Use the existing token.
-    File.read(token_file).chomp
-  else
-    # Generate a new token of 64 random hexadecimal characters and store it in token_file.
-    token = SecureRandom.hex(64)
-    File.write(token_file, token)
-    token
-  end
-end
-
-Rails.application.config.secret_token = find_secure_token
-Rails.application.config.secret_key_base = find_secure_token
-
-# CI
 def generate_new_secure_token
   SecureRandom.hex(64)
 end
 
-if Rails.application.secrets.db_key_base.blank?
-  warn "Missing `db_key_base` for '#{Rails.env}' environment. The secrets will be generated and stored in `config/secrets.yml`"
+def warn_missing_secret(secret)
+  warn "Missing `#{secret}` for '#{Rails.env}' environment. The secret will be generated and stored in `config/secrets.yml`"
+end
+
+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
 
-  all_secrets = YAML.load_file('config/secrets.yml') if File.exist?('config/secrets.yml')
-  all_secrets ||= {}
+  otp_key_base = Rails.application.secrets.otp_key_base
+  db_key_base = Rails.application.secrets.db_key_base
+  yaml_additions = {}
 
-  # generate secrets
-  env_secrets = all_secrets[Rails.env.to_s] || {}
-  env_secrets['db_key_base'] ||= generate_new_secure_token
-  all_secrets[Rails.env.to_s] = env_secrets
+  if otp_key_base.blank?
+    warn_missing_secret('otp_key_base')
 
-  # save secrets
-  File.open('config/secrets.yml', 'w', 0600) do |file|
-    file.write(YAML.dump(all_secrets))
+    otp_key_base ||= env_key || file_key || generate_new_secure_token
+    yaml_additions['otp_key_base'] = otp_key_base
   end
 
-  Rails.application.secrets.db_key_base = env_secrets['db_key_base']
+  Rails.application.secrets.otp_key_base = otp_key_base
+
+  if db_key_base.blank?
+    warn_missing_secret('db_key_base')
+
+    yaml_additions['db_key_base'] = db_key_base = generate_new_secure_token
+  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)
+    all_secrets ||= {}
+
+    env_secrets = all_secrets[Rails.env.to_s] || {}
+    all_secrets[Rails.env.to_s] = env_secrets.merge(yaml_additions)
+
+    File.write(secrets_yml, YAML.dump(all_secrets), mode: 'w', perm: 0600)
+  end
 end
+
+create_tokens
diff --git a/doc/raketasks/backup_restore.md b/doc/raketasks/backup_restore.md
index 5fa96736d59..b48a3ea00f4 100644
--- a/doc/raketasks/backup_restore.md
+++ b/doc/raketasks/backup_restore.md
@@ -11,12 +11,13 @@ You can only restore a backup to exactly the same version of GitLab that you cre
 on, for example 7.2.1. The best way to migrate your repositories from one server to
 another is through backup restore.
 
-You need to keep a separate copy of `/etc/gitlab/gitlab-secrets.json`
-(for omnibus packages) or `/home/git/gitlab/.secret` (for installations
-from source). This file contains the database encryption key used
-for two-factor authentication. If you restore a GitLab backup without
-restoring the database encryption key, users who have two-factor
-authentication enabled will lose access to your GitLab server.
+You need to keep a separate copy of `/etc/gitlab/gitlab-secrets.json` (for
+omnibus packages) or `/home/git/gitlab/config/secrets.yml` (for installations
+from source). This file contains the database encryption keys used for
+two-factor authentication and project import credentials, among other things. If
+you restore a GitLab backup without restoring the database encryption key, users
+who have two-factor authentication enabled will lose access to your GitLab
+server.
 
 ```
 # use this command if you've installed GitLab with the Omnibus package
@@ -221,10 +222,10 @@ of using encryption in the first place!
 
 If you use an Omnibus package please see the [instructions in the readme to backup your configuration](https://gitlab.com/gitlab-org/omnibus-gitlab/blob/master/README.md#backup-and-restore-omnibus-gitlab-configuration).
 If you have a cookbook installation there should be a copy of your configuration in Chef.
-If you have an installation from source, please consider backing up your `.secret` file, `gitlab.yml` file, any SSL keys and certificates, and your [SSH host keys](https://superuser.com/questions/532040/copy-ssh-keys-from-one-server-to-another-server/532079#532079).
+If you have an installation from source, please consider backing up your `config/secrets.yml` file, `gitlab.yml` file, any SSL keys and certificates, and your [SSH host keys](https://superuser.com/questions/532040/copy-ssh-keys-from-one-server-to-another-server/532079#532079).
 
 At the very **minimum** you should backup `/etc/gitlab/gitlab-secrets.json`
-(Omnibus) or `/home/git/gitlab/.secret` (source) to preserve your
+(Omnibus) or `/home/git/gitlab/config/secrets.yml` (source) to preserve your
 database encryption key.
 
 ## Restore a previously created backup
@@ -240,11 +241,11 @@ the SQL database it needs to import data into ('gitlabhq_production').
 All existing data will be either erased (SQL) or moved to a separate
 directory (repositories, uploads).
 
-If some or all of your GitLab users are using two-factor authentication
-(2FA) then you must also make sure to restore
-`/etc/gitlab/gitlab-secrets.json` (Omnibus) or `/home/git/gitlab/.secret`
-(installations from source). Note that you need to run `gitlab-ctl
-reconfigure` after changing `gitlab-secrets.json`.
+If some or all of your GitLab users are using two-factor authentication (2FA)
+then you must also make sure to restore `/etc/gitlab/gitlab-secrets.json`
+(Omnibus) or `/home/git/gitlab/config/secrets.yml` (installations from
+source). Note that you need to run `gitlab-ctl reconfigure` after changing
+`gitlab-secrets.json`.
 
 ### Installation from source
 
diff --git a/doc/raketasks/user_management.md b/doc/raketasks/user_management.md
index 629d38efc53..8a5e2d6e16b 100644
--- a/doc/raketasks/user_management.md
+++ b/doc/raketasks/user_management.md
@@ -60,8 +60,8 @@ block_auto_created_users: false
 ## Disable Two-factor Authentication (2FA) for all users
 
 This task will disable 2FA for all users that have it enabled. This can be
-useful if GitLab's `.secret` file has been lost and users are unable to login,
-for example.
+useful if GitLab's `config/secrets.yml` file has been lost and users are unable
+to login, for example.
 
 ```bash
 # omnibus-gitlab
diff --git a/spec/initializers/secret_token_spec.rb b/spec/initializers/secret_token_spec.rb
new file mode 100644
index 00000000000..063d1cdd447
--- /dev/null
+++ b/spec/initializers/secret_token_spec.rb
@@ -0,0 +1,135 @@
+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(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 '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]
+
+        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('otp_key_base')
+        expect(self).to receive(:warn_missing_secret).with('db_key_base')
+
+        create_tokens
+      end
+
+      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)
+
+          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)
+        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
+
+        create_tokens
+
+        expect(secret_key_base).to eq(config.secret_key_base)
+      end
+    end
+
+    context 'when the other secrets all exist' do
+      before do
+        secrets.db_key_base = 'db_key_base'
+
+        allow(ENV).to receive(:[]).with('SECRET_KEY_BASE').and_return('env_key')
+        allow(File).to receive(:exist?).with('.secret').and_return(true)
+        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' }
+
+        it 'does not write any files' do
+          expect(File).not_to receive(:write)
+
+          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
+          create_tokens
+
+          expect(config.secret_key_base).to eq('env_key')
+          expect(secrets.otp_key_base).to eq('otp_key_base')
+          expect(secrets.db_key_base).to eq('db_key_base')
+        end
+      end
+
+      context 'when the otp_key_base secret does 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)
+          allow(self).to receive(:warn_missing_secret)
+        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)
+
+            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')
+          end
+
+          create_tokens
+
+          expect(secrets.otp_key_base).to eq('env_key')
+        end
+
+        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
+          expect(self).to receive(:warn_missing_secret).with('otp_key_base')
+
+          create_tokens
+        end
+      end
+    end
+  end
+end