Use keygenerate to ensure encyprtion key is of correct length

- Generates a new key or correct length pased on the provided base key
- Updates the naming of the key param passed to EncryptedConfiguration
- Adds tests for key length to the spec tests

Increase cipher to 256

The 128 was the default from upstream configuration encryption but
is lower then the message encryptor default. Not sure why, so we
might as well go with the larger one.

config/settings.rb

@@ -157,7 +157,7 @@ class Settings < Settingslogic
 
       Gitlab::EncryptedConfiguration.new(
         content_path: Settings.absolute(path),
-        key: Gitlab::Application.secrets.enc_settings_key_base,
+        base_key: Gitlab::Application.secrets.enc_settings_key_base,
         previous_keys: Gitlab::Application.secrets.rotated_enc_settings_key_base || []
       )
     end

lib/gitlab/encrypted_configuration.rb

@@ -6,11 +6,20 @@ module Gitlab
     delegate_missing_to :options
     attr_reader :content_path, :key, :previous_keys
 
-    CIPHER = "aes-128-gcm"
+    CIPHER = "aes-256-gcm"
+    SALT = "GitLabEncryptedConfigSalt"
 
-    def initialize(content_path: nil, key: nil, previous_keys: [])
+    def self.generate_key(base_key)
+      # Because the salt is static, we want uniqueness to be coming from the base_key
+      # Error if the base_key is empty or suspiciously short
+      raise 'Base key too small' if base_key.blank? || base_key.length < 16
+
+      ActiveSupport::KeyGenerator.new(base_key).generate_key(SALT, ActiveSupport::MessageEncryptor.key_len(CIPHER))
+    end
+
+    def initialize(content_path: nil, base_key: nil, previous_keys: [])
       @content_path = Pathname.new(content_path).yield_self { |path| path.symlink? ? path.realpath : path } if content_path
-      @key = key
+      @key = self.class.generate_key(base_key) if base_key
       @previous_keys = previous_keys
     end
 
@@ -62,11 +71,11 @@ module Gitlab
     def encryptor
       return @encryptor if @encryptor
 
-      @encryptor = ActiveSupport::MessageEncryptor.new([key].pack("H*"), cipher: CIPHER)
+      @encryptor = ActiveSupport::MessageEncryptor.new(key, cipher: CIPHER)
 
       # Allow fallback to previous keys
       @previous_keys.each do |key|
-        @encryptor.rotate([key].pack("H*"))
+        @encryptor.rotate(self.class.generate_key(key))
       end
 
       @encryptor

spec/config/settings_spec.rb

@@ -141,11 +141,11 @@ RSpec.describe Settings do
     end
 
     it 'defaults to using the enc_settings_key_base for the key' do
-      expect(Gitlab::EncryptedConfiguration).to receive(:new).with(hash_including(key: Gitlab::Application.secrets.enc_settings_key_base))
+      expect(Gitlab::EncryptedConfiguration).to receive(:new).with(hash_including(base_key: Gitlab::Application.secrets.enc_settings_key_base))
       Settings.encrypted('tmp/tests/test.enc')
     end
 
-    it 'defaults the configpath within the rails root' do
+    it 'defaults the config path within the rails root' do
       expect(Settings.encrypted('tmp/tests/test.enc').content_path.fnmatch?(File.join(Rails.root, '**'))).to be true
     end
 

spec/lib/gitlab/encrypted_configuration_spec.rb

@@ -12,12 +12,28 @@ RSpec.describe Gitlab::EncryptedConfiguration do
       expect(configuration.key).to be_nil
       expect(configuration.previous_keys).to be_empty
     end
+
+    it 'generates 32 byte key when provided a larger base key' do
+      configuration = described_class.new(base_key: 'A' * 64)
+
+      expect(configuration.key.bytesize).to eq 32
+    end
+
+    it 'generates 32 byte key when provided a smaller base key' do
+      configuration = described_class.new(base_key: 'A' * 16)
+
+      expect(configuration.key.bytesize).to eq 32
+    end
+
+    it 'throws an error when the base key is too small' do
+      expect { described_class.new(base_key: 'A' * 12) }.to raise_error 'Base key too small'
+    end
   end
 
   context 'when provided key and config file' do
     let!(:config_tmp_dir) { Dir.mktmpdir('config-') }
     let(:credentials_config_path) { File.join(config_tmp_dir, 'credentials.yml.enc') }
-    let(:credentials_key) { ActiveSupport::EncryptedConfiguration.generate_key }
+    let(:credentials_key) { SecureRandom.hex(64) }
 
     after do
       FileUtils.rm_f(config_tmp_dir)
@@ -25,8 +41,8 @@ RSpec.describe Gitlab::EncryptedConfiguration do
 
     describe '#write' do
       it 'encrypts the file using the provided key' do
-        encryptor = ActiveSupport::MessageEncryptor.new([credentials_key].pack('H*'), cipher: 'aes-128-gcm')
-        config = described_class.new(content_path: credentials_config_path, key: credentials_key)
+        encryptor = ActiveSupport::MessageEncryptor.new(Gitlab::EncryptedConfiguration.generate_key(credentials_key), cipher: 'aes-256-gcm')
+        config = described_class.new(content_path: credentials_config_path, base_key: credentials_key)
 
         config.write('sample-content')
         expect(encryptor.decrypt_and_verify(File.read(credentials_config_path))).to eq('sample-content')
@@ -35,14 +51,14 @@ RSpec.describe Gitlab::EncryptedConfiguration do
 
     describe '#read' do
       it 'reads yaml configuration' do
-        config = described_class.new(content_path: credentials_config_path, key: credentials_key)
+        config = described_class.new(content_path: credentials_config_path, base_key: credentials_key)
 
         config.write({ foo: { bar: true } }.to_yaml)
         expect(config[:foo][:bar]).to be true
       end
 
       it 'allows referencing top level keys via dot syntax' do
-        config = described_class.new(content_path: credentials_config_path, key: credentials_key)
+        config = described_class.new(content_path: credentials_config_path, base_key: credentials_key)
 
         config.write({ foo: { bar: true } }.to_yaml)
         expect(config.foo[:bar]).to be true
@@ -51,7 +67,7 @@ RSpec.describe Gitlab::EncryptedConfiguration do
 
     describe '#change' do
       it 'changes yaml configuration' do
-        config = described_class.new(content_path: credentials_config_path, key: credentials_key)
+        config = described_class.new(content_path: credentials_config_path, base_key: credentials_key)
 
         config.write({ foo: { bar: true } }.to_yaml)
         config.change do |unencrypted_contents|
@@ -65,8 +81,8 @@ RSpec.describe Gitlab::EncryptedConfiguration do
 
     context 'when provided previous_keys for rotation' do
       let!(:config_tmp_dir) { Dir.mktmpdir('config-') }
-      let(:credential_key_original) { ActiveSupport::EncryptedConfiguration.generate_key }
-      let(:credential_key_latest) { ActiveSupport::EncryptedConfiguration.generate_key }
+      let(:credential_key_original) { SecureRandom.hex(64) }
+      let(:credential_key_latest) { SecureRandom.hex(64) }
       let(:config_path_original) { File.join(config_tmp_dir, 'credentials-orig.yml.enc') }
       let(:config_path_latest) { File.join(config_tmp_dir, 'credentials-latest.yml.enc') }
 
@@ -75,21 +91,21 @@ RSpec.describe Gitlab::EncryptedConfiguration do
       end
 
       def encryptor(key)
-        ActiveSupport::MessageEncryptor.new([key].pack('H*'), cipher: 'aes-128-gcm')
+        ActiveSupport::MessageEncryptor.new(Gitlab::EncryptedConfiguration.generate_key(key), cipher: 'aes-256-gcm')
       end
 
       describe '#write' do
         it 'rotates the key when provided a new key' do
-          config1 = described_class.new(content_path: config_path_original, key: credential_key_original)
+          config1 = described_class.new(content_path: config_path_original, base_key: credential_key_original)
           config1.write('sample-content1')
 
-          config2 = described_class.new(content_path: config_path_latest, key: credential_key_latest, previous_keys: [credential_key_original])
+          config2 = described_class.new(content_path: config_path_latest, base_key: credential_key_latest, previous_keys: [credential_key_original])
           config2.write('sample-content2')
 
           original_key_encryptor = encryptor(credential_key_original) # can read with the initial key
           latest_key_encryptor = encryptor(credential_key_latest) # can read with the new key
           both_key_encryptor = encryptor(credential_key_latest) # can read with either key
-          both_key_encryptor.rotate([credential_key_original].pack("H*"))
+          both_key_encryptor.rotate(Gitlab::EncryptedConfiguration.generate_key(credential_key_original))
 
           expect(original_key_encryptor.decrypt_and_verify(File.read(config_path_original))).to eq('sample-content1')
           expect(both_key_encryptor.decrypt_and_verify(File.read(config_path_original))).to eq('sample-content1')
@@ -101,9 +117,9 @@ RSpec.describe Gitlab::EncryptedConfiguration do
 
       describe '#read' do
         it 'supports reading using rotated config' do
-          described_class.new(content_path: config_path_original, key: credential_key_original).write({ foo: { bar: true } }.to_yaml)
+          described_class.new(content_path: config_path_original, base_key: credential_key_original).write({ foo: { bar: true } }.to_yaml)
 
-          config = described_class.new(content_path: config_path_original, key: credential_key_latest,  previous_keys: [credential_key_original])
+          config = described_class.new(content_path: config_path_original, base_key: credential_key_latest,  previous_keys: [credential_key_original])
           expect(config[:foo][:bar]).to be true
         end
       end