Merge branch 'decouple-secret-keys' into 'master'

Store OTP secret key in secrets.yml

## What does this MR do?

Migrate the value of `.secret` to `config/secrets.yml` if present, so that `.secret` can be rotated without preventing all users with 2FA from logging in. (On a clean setup, generate different keys for each.)

## Are there points in the code the reviewer needs to double check?

I'm not sure we actually need `.secret` at all after this, but it seems safer not to touch it.

## Why was this MR needed?

We have some DB encryption keys in `config/secrets.yml`, and one in `.secret`. They should all be in the same place.

## What are the relevant issue numbers?

#3963, which isn't closed until I make the relevant changes in Omnibus too.

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- ~~API support added~~
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5274

CHANGELOG

@@ -40,6 +40,7 @@ v 8.11.0 (unreleased)
   - Fix devise deprecation warnings.
   - Update version_sorter and use new interface for faster tag sorting
   - 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)
   - Admin page now references docs instead of a specific file !5600 (AnAverageHuman)

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

config/initializers/secret_token.rb

@@ -2,49 +2,86 @@
 
 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
+# Transition material in .secret to the secret_key_base key in config/secrets.yml.
+# Historically, ENV['SECRET_KEY_BASE'] takes precedence over .secret, so we maintain that
+# behavior.
+#
+# It also used to be the case that the key material in ENV['SECRET_KEY_BASE'] or .secret
+# was used to encrypt OTP (two-factor authentication) data so if present, we copy that key
+# material into config/secrets.yml under otp_key_base.
+#
+# Finally, if we have successfully migrated all secrets to config/secrets.yml, delete the
+# .secret file to avoid confusion.
+#
+def create_tokens
+  secret_file = Rails.root.join('.secret')
+  file_secret_key = File.read(secret_file).chomp if File.exist?(secret_file)
+  env_secret_key = ENV['SECRET_KEY_BASE']
+
+  # Ensure environment variable always overrides secrets.yml.
+  Rails.application.secrets.secret_key_base = env_secret_key if env_secret_key.present?
+
+  defaults = {
+    secret_key_base: file_secret_key || generate_new_secure_token,
+    otp_key_base: env_secret_key || file_secret_key || generate_new_secure_token,
+    db_key_base: generate_new_secure_token
+  }
+
+  missing_secrets = set_missing_keys(defaults)
+  write_secrets_yml(missing_secrets) unless missing_secrets.empty?
+
+  begin
+    File.delete(secret_file) if file_secret_key
+  rescue => e
+    warn "Error deleting useless .secret file: #{e}"
   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 Rails.application.secrets.#{secret} for #{Rails.env} environment. The secret will be generated and stored in config/secrets.yml."
+end
 
-  all_secrets = YAML.load_file('config/secrets.yml') if File.exist?('config/secrets.yml')
-  all_secrets ||= {}
+def set_missing_keys(defaults)
+  defaults.stringify_keys.each_with_object({}) do |(key, default), missing|
+    if Rails.application.secrets[key].blank?
+      warn_missing_secret(key)
 
-  # 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
+      missing[key] = Rails.application.secrets[key] = default
+    end
+  end
+end
+
+def write_secrets_yml(missing_secrets)
+  secrets_yml = Rails.root.join('config/secrets.yml')
+  rails_env = Rails.env.to_s
+  secrets = YAML.load_file(secrets_yml) if File.exist?(secrets_yml)
+  secrets ||= {}
+  secrets[rails_env] ||= {}
+
+  secrets[rails_env].merge!(missing_secrets) do |key, old, new|
+    # Previously, it was possible this was set to the literal contents of an Erb
+    # expression that evaluated to an empty value. We don't want to support that
+    # specifically, just ensure we don't break things further.
+    #
+    if old.present?
+      warn <<EOM
+Rails.application.secrets.#{key} was blank, but the literal value in config/secrets.yml was:
+  #{old}
 
-  # save secrets
-  File.open('config/secrets.yml', 'w', 0600) do |file|
-    file.write(YAML.dump(all_secrets))
+This probably isn't the expected value for this secret. To keep using a literal Erb string in config/secrets.yml, replace `<%` with `<%%`.
+EOM
+
+      exit 1 # rubocop:disable Rails/Exit
+    end
+
+    new
   end
 
-  Rails.application.secrets.db_key_base = env_secrets['db_key_base']
+  File.write(secrets_yml, YAML.dump(secrets), mode: 'w', perm: 0600)
 end
+
+create_tokens

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 separate copies of `/etc/gitlab/gitlab-secrets.json` and
+`/etc/gitlab/gitlab.rb` (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 CI
+secret variables, 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,11 +222,12 @@ 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
-database encryption key.
+At the very **minimum** you should backup `/etc/gitlab/gitlab.rb` and
+`/etc/gitlab/gitlab-secrets.json` (Omnibus), or
+`/home/git/gitlab/config/secrets.yml` (source) to preserve your database
+encryption key.
 
 ## Restore a previously created backup
 
@@ -240,11 +242,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.rb` and
+`/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
 

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

spec/initializers/secret_token_spec.rb

+require 'spec_helper'
+require_relative '../../config/initializers/secret_token'
+
+describe 'create_tokens', lib: true do
+  let(:secrets) { ActiveSupport::OrderedOptions.new }
+
+  before do
+    allow(ENV).to receive(:[]).and_call_original
+    allow(File).to receive(:write)
+    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 }
+    allow(self).to receive(:warn)
+    allow(self).to receive(:exit)
+  end
+
+  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(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 = 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')
+
+        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 = YAML.load(contents)[Rails.env]
+
+          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 'does not write a .secret file' do
+        expect(File).not_to receive(:write).with('.secret')
+
+        create_tokens
+      end
+    end
+
+    context 'when the other secrets all exist' do
+      before do
+        secrets.db_key_base = 'db_key_base'
+
+        allow(File).to receive(:exist?).with('.secret').and_return(true)
+        allow(File).to receive(:read).with('.secret').and_return('file_key')
+      end
+
+      context 'when secret_key_base exists in the environment and secrets.yml' do
+        before do
+          allow(ENV).to receive(:[]).with('SECRET_KEY_BASE').and_return('env_key')
+          secrets.secret_key_base = 'secret_key_base'
+          secrets.otp_key_base = 'otp_key_base'
+        end
+
+        it 'does not issue a warning' do
+          expect(self).not_to receive(:warn)
+
+          create_tokens
+        end
+
+        it 'uses the environment variable' do
+          create_tokens
+
+          expect(secrets.secret_key_base).to eq('env_key')
+        end
+
+        it 'does not update secrets.yml' do
+          expect(File).not_to receive(:write)
+
+          create_tokens
+        end
+      end
+
+      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)
+
+          create_tokens
+        end
+
+        it 'sets the the keys to the values from the environment and secrets.yml' do
+          create_tokens
+
+          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 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)
+          allow(self).to receive(:warn_missing_secret)
+        end
+
+        it 'uses the file secret' do
+          expect(File).to receive(:write) do |filename, contents, options|
+            new_secrets = YAML.load(contents)[Rails.env]
+
+            expect(new_secrets['secret_key_base']).to eq('file_key')
+            expect(new_secrets['otp_key_base']).to eq('file_key')
+            expect(new_secrets['db_key_base']).to eq('db_key_base')
+          end
+
+          create_tokens
+
+          expect(secrets.otp_key_base).to eq('file_key')
+        end
+
+        it 'keeps the other secrets as they were' do
+          create_tokens
+
+          expect(secrets.db_key_base).to eq('db_key_base')
+        end
+
+        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
+
+    context 'when db_key_base is blank but exists in secrets.yml' do
+      before do
+        secrets.otp_key_base = 'otp_key_base'
+        secrets.secret_key_base = 'secret_key_base'
+        yaml_secrets = secrets.to_h.stringify_keys.merge('db_key_base' => '<%= an_erb_expression %>')
+
+        allow(File).to receive(:exist?).with('.secret').and_return(false)
+        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' => yaml_secrets)
+        allow(self).to receive(:warn_missing_secret)
+      end
+
+      it 'warns about updating db_key_base' do
+        expect(self).to receive(:warn_missing_secret).with('db_key_base')
+
+        create_tokens
+      end
+
+      it 'warns about the blank value existing in secrets.yml and exits' do
+        expect(self).to receive(:warn) do |warning|
+          expect(warning).to include('db_key_base')
+          expect(warning).to include('<%= an_erb_expression %>')
+        end
+
+        create_tokens
+      end
+
+      it 'does not update secrets.yml' do
+        expect(self).to receive(:exit).with(1).and_call_original
+        expect(File).not_to receive(:write)
+
+        expect { create_tokens }.to raise_error(SystemExit)
+      end
+    end
+  end
+end