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') + 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 + } - otp_key_base ||= env_key || file_key || generate_new_secure_token - yaml_additions['otp_key_base'] = otp_key_base + defaults.stringify_keys.each do |key, default| + if Rails.application.secrets[key].blank? + warn_missing_secret(key) + + yaml_additions[key] = Rails.application.secrets[key] = default + end end - 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) @@ -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