Generate lets_encrypt_private_key on the fly

Remove migration generating lets encrypt key
Don't generate private_key if database is readonly

For reference:
This reverts commit 988a7f70489b99383b95e9f271a2caf6bb5b3a44.
This reverts commit 21acbe531592d55caf0e5b8716a3b551dafd6233.

db/migrate/20190524062810_generate_lets_encrypt_private_key.rb

@@ -9,23 +9,8 @@ class GenerateLetsEncryptPrivateKey < ActiveRecord::Migration[5.1]
   # Set this constant to true if this migration requires downtime.
   DOWNTIME = false
 
-  class ApplicationSetting < ActiveRecord::Base
-    self.table_name = 'application_settings'
-
-    attr_encrypted :lets_encrypt_private_key,
-                   mode: :per_attribute_iv,
-                   key: Settings.attr_encrypted_db_key_base_truncated,
-                   algorithm: 'aes-256-gcm',
-                   encode: true
-  end
-
+  # we now generate this key on the fly, but since this migration was merged to master, we don't remove it
   def up
-    ApplicationSetting.reset_column_information
-
-    private_key = OpenSSL::PKey::RSA.new(4096).to_pem
-    ApplicationSetting.find_each do |setting|
-      setting.update!(lets_encrypt_private_key: private_key)
-    end
   end
 
   def down

lib/gitlab/lets_encrypt/client.rb

@@ -3,6 +3,8 @@
 module Gitlab
   module LetsEncrypt
     class Client
+      include Gitlab::Utils::StrongMemoize
+
       PRODUCTION_DIRECTORY_URL = 'https://acme-v02.api.letsencrypt.org/directory'
       STAGING_DIRECTORY_URL = 'https://acme-staging-v02.api.letsencrypt.org/directory'
 
@@ -35,6 +37,8 @@ module Gitlab
       def enabled?
         return false unless Feature.enabled?(:pages_auto_ssl)
 
+        return false unless private_key
+
         Gitlab::CurrentSettings.lets_encrypt_terms_of_service_accepted
       end
 
@@ -45,7 +49,11 @@ module Gitlab
       end
 
       def private_key
-        @private_key ||= OpenSSL::PKey.read(Gitlab::CurrentSettings.lets_encrypt_private_key)
+        strong_memoize(:private_key) do
+          private_key_string = Gitlab::CurrentSettings.lets_encrypt_private_key
+          private_key_string ||= generate_private_key
+          OpenSSL::PKey.read(private_key_string) if private_key_string
+        end
       end
 
       def admin_email
@@ -69,6 +77,19 @@ module Gitlab
           STAGING_DIRECTORY_URL
         end
       end
+
+      def generate_private_key
+        return if Gitlab::Database.read_only?
+
+        application_settings = Gitlab::CurrentSettings.current_application_settings
+        application_settings.with_lock do
+          unless application_settings.lets_encrypt_private_key
+            application_settings.update(lets_encrypt_private_key: OpenSSL::PKey::RSA.new(4096).to_pem)
+          end
+
+          application_settings.lets_encrypt_private_key
+        end
+      end
     end
   end
 end

spec/lib/gitlab/lets_encrypt/client_spec.rb

@@ -5,14 +5,12 @@ require 'spec_helper'
 describe ::Gitlab::LetsEncrypt::Client do
   include LetsEncryptHelpers
 
-  set(:private_key) { OpenSSL::PKey::RSA.new(4096).to_pem }
   let(:client) { described_class.new }
 
   before do
     stub_application_setting(
       lets_encrypt_notification_email: 'myemail@test.example.com',
-      lets_encrypt_terms_of_service_accepted: true,
-      lets_encrypt_private_key: private_key
+      lets_encrypt_terms_of_service_accepted: true
     )
   end
 
@@ -28,6 +26,36 @@ describe ::Gitlab::LetsEncrypt::Client do
       )
     end
 
+    it 'generates and stores private key and initialize acme client with it' do
+      expect(Gitlab::CurrentSettings.lets_encrypt_private_key).to eq(nil)
+
+      subject
+
+      saved_private_key = Gitlab::CurrentSettings.lets_encrypt_private_key
+
+      expect(saved_private_key).to be
+      expect(Acme::Client).to have_received(:new).with(
+        hash_including(private_key: eq_pem(saved_private_key))
+      )
+    end
+
+    context 'when private key is saved in settings' do
+      let!(:saved_private_key) do
+        key = OpenSSL::PKey::RSA.new(4096).to_pem
+        Gitlab::CurrentSettings.current_application_settings.update(lets_encrypt_private_key: key)
+        key
+      end
+
+      it 'uses current value of private key' do
+        subject
+
+        expect(Acme::Client).to have_received(:new).with(
+          hash_including(private_key: eq_pem(saved_private_key))
+        )
+        expect(Gitlab::CurrentSettings.lets_encrypt_private_key).to eq(saved_private_key)
+      end
+    end
+
     context 'when acme integration is disabled' do
       before do
         stub_application_setting(lets_encrypt_terms_of_service_accepted: false)
@@ -94,6 +122,18 @@ describe ::Gitlab::LetsEncrypt::Client do
     context 'when terms of service are accepted' do
       it { is_expected.to eq(true) }
 
+      context "when private_key isn't present and database is read only" do
+        before do
+          allow(::Gitlab::Database).to receive(:read_only?).and_return(true)
+        end
+
+        it 'returns false' do
+          expect(::Gitlab::CurrentSettings.lets_encrypt_private_key).to eq(nil)
+
+          is_expected.to eq(false)
+        end
+      end
+
       context 'when feature flag is disabled' do
         before do
           stub_feature_flags(pages_auto_ssl: false)

spec/migrations/generate_lets_encrypt_private_key_spec.rb

@@ -3,17 +3,9 @@ require Rails.root.join('db', 'migrate', '20190524062810_generate_lets_encrypt_p
 
 describe GenerateLetsEncryptPrivateKey, :migration do
   describe '#up' do
-    let(:applications_settings) { table(:applications_settings) }
-
-    it 'generates RSA private key and saves it in application settings' do
-      application_setting = described_class::ApplicationSetting.create!
-
-      described_class.new.up
-      application_setting.reload
-
-      expect(application_setting.lets_encrypt_private_key).to be_present
+    it 'does not fail' do
       expect do
-        OpenSSL::PKey::RSA.new(application_setting.lets_encrypt_private_key)
+        described_class.new.up
       end.not_to raise_error
     end
   end

spec/support/matchers/eq_pem.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+RSpec::Matchers.define :eq_pem do |expected_pem_string|
+  match do |actual|
+    actual.to_pem == expected_pem_string
+  end
+
+  description do
+    "contain pem #{expected_pem_string}"
+  end
+end