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

1 files changed, 200 insertions, 0 deletions

diff --git a/spec/initializers/secret_token_spec.rb b/spec/initializers/secret_token_spec.rb
new file mode 100644
index 00000000000..837b0de9a4c
--- /dev/null
+++ b/spec/initializers/secret_token_spec.rb
@@ -0,0 +1,200 @@
+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