Merge branch 'jsalazar-spamcheck-gitaly-grpc-tls-fixes' into 'master'

Add DEFAULT_CERT_DIR handling to Spamcheck and fix Gitaly GRPC Bug

See merge request gitlab-org/gitlab!72386

lib/gitlab/gitaly_client.rb

@@ -24,7 +24,6 @@ module Gitlab
       end
     end
 
-    PEM_REGEX = /\-+BEGIN CERTIFICATE\-+.+?\-+END CERTIFICATE\-+/m.freeze
     SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'
     MAXIMUM_GITALY_CALLS = 30
     CLIENT_NAME = (Gitlab::Runtime.sidekiq? ? 'gitlab-sidekiq' : 'gitlab-web').freeze
@@ -62,28 +61,9 @@ module Gitlab
     end
     private_class_method :channel_args
 
-    def self.stub_cert_paths
-      cert_paths = Dir["#{OpenSSL::X509::DEFAULT_CERT_DIR}/*"]
-      cert_paths << OpenSSL::X509::DEFAULT_CERT_FILE if File.exist? OpenSSL::X509::DEFAULT_CERT_FILE
-      cert_paths
-    end
-
-    def self.stub_certs
-      return @certs if @certs
-
-      @certs = stub_cert_paths.flat_map do |cert_file|
-        File.read(cert_file).scan(PEM_REGEX).map do |cert|
-          OpenSSL::X509::Certificate.new(cert).to_pem
-        rescue OpenSSL::OpenSSLError => e
-          Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, cert_file: cert_file)
-          nil
-        end.compact
-      end.uniq.join("\n")
-    end
-
     def self.stub_creds(storage)
       if URI(address(storage)).scheme == 'tls'
-        GRPC::Core::ChannelCredentials.new stub_certs
+        GRPC::Core::ChannelCredentials.new ::Gitlab::X509::Certificate.ca_certs_bundle
       else
         :this_channel_is_insecure
       end

lib/gitlab/spamcheck/client.rb

@@ -5,6 +5,7 @@ module Gitlab
   module Spamcheck
     class Client
       include ::Spam::SpamConstants
+
       DEFAULT_TIMEOUT_SECS = 2
 
       VERDICT_MAPPING = {
@@ -27,12 +28,7 @@ module Gitlab
         # connect with Spamcheck
         @endpoint_url = @endpoint_url.gsub(%r(^grpc:\/\/), '')
 
-        @creds =
-          if Rails.env.development? || Rails.env.test?
-            :this_channel_is_insecure
-          else
-            GRPC::Core::ChannelCredentials.new
-          end
+        @creds = stub_creds
       end
 
       def issue_spam?(spam_issue:, user:, context: {})
@@ -98,6 +94,14 @@ module Gitlab
                                         nanos: ar_timestamp.to_time.nsec)
       end
 
+      def stub_creds
+        if Rails.env.development? || Rails.env.test?
+          :this_channel_is_insecure
+        else
+          GRPC::Core::ChannelCredentials.new ::Gitlab::X509::Certificate.ca_certs_bundle
+        end
+      end
+
       def grpc_client
         @grpc_client ||= ::Spamcheck::SpamcheckService::Stub.new(@endpoint_url, @creds,
                                                         interceptors: interceptors,

lib/gitlab/x509/certificate.rb

@@ -33,6 +33,26 @@ module Gitlab
         from_strings(File.read(key_path), File.read(cert_path), ca_certs_string)
       end
 
+      # Returns all top-level, readable files in the default CA cert directory
+      def self.ca_certs_paths
+        cert_paths = Dir["#{OpenSSL::X509::DEFAULT_CERT_DIR}/*"].select do |path|
+          !File.directory?(path) && File.readable?(path)
+        end
+        cert_paths << OpenSSL::X509::DEFAULT_CERT_FILE if File.exist? OpenSSL::X509::DEFAULT_CERT_FILE
+        cert_paths
+      end
+
+      # Returns a concatenated array of Strings, each being a PEM-coded CA certificate.
+      def self.ca_certs_bundle
+        return @certs if @certs
+
+        @certs = ca_certs_paths.flat_map do |cert_file|
+          load_ca_certs_bundle(File.read(cert_file))
+        rescue OpenSSL::OpenSSLError => e
+          Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, cert_file: cert_file)
+        end.uniq.join("\n")
+      end
+
       # Returns an array of OpenSSL::X509::Certificate objects, empty array if none found
       #
       # Ruby OpenSSL::X509::Certificate.new will only load the first

spec/lib/gitlab/gitaly_client_spec.rb

@@ -5,14 +5,6 @@ require 'spec_helper'
 # We stub Gitaly in `spec/support/gitaly.rb` for other tests. We don't want
 # those stubs while testing the GitalyClient itself.
 RSpec.describe Gitlab::GitalyClient do
-  let(:sample_cert) { Rails.root.join('spec/fixtures/clusters/sample_cert.pem').to_s }
-
-  before do
-    allow(described_class)
-      .to receive(:stub_cert_paths)
-      .and_return([sample_cert])
-  end
-
   def stub_repos_storages(address)
     allow(Gitlab.config.repositories).to receive(:storages).and_return({
       'default' => { 'gitaly_address' => address }
@@ -142,21 +134,6 @@ RSpec.describe Gitlab::GitalyClient do
     end
   end
 
-  describe '.stub_certs' do
-    it 'skips certificates if OpenSSLError is raised and report it' do
-      expect(Gitlab::ErrorTracking)
-        .to receive(:track_and_raise_for_dev_exception)
-        .with(
-          a_kind_of(OpenSSL::X509::CertificateError),
-          cert_file: a_kind_of(String)).at_least(:once)
-
-      expect(OpenSSL::X509::Certificate)
-        .to receive(:new)
-        .and_raise(OpenSSL::X509::CertificateError).at_least(:once)
-
-      expect(described_class.stub_certs).to be_a(String)
-    end
-  end
   describe '.stub_creds' do
     it 'returns :this_channel_is_insecure if unix' do
       address = 'unix:/tmp/gitaly.sock'

spec/lib/gitlab/x509/certificate_spec.rb

@@ -5,6 +5,9 @@ require 'spec_helper'
 RSpec.describe Gitlab::X509::Certificate do
   include SmimeHelper
 
+  let(:sample_ca_certs_path) { Rails.root.join('spec/fixtures/clusters').to_s }
+  let(:sample_cert) { Rails.root.join('spec/fixtures/x509_certificate.crt').to_s }
+
   # cert generation is an expensive operation and they are used read-only,
   # so we share them as instance variables in all tests
   before :context do
@@ -13,6 +16,11 @@ RSpec.describe Gitlab::X509::Certificate do
     @cert = generate_cert(signer_ca: @intermediate_ca)
   end
 
+  before do
+    stub_const("OpenSSL::X509::DEFAULT_CERT_DIR", sample_ca_certs_path)
+    stub_const("OpenSSL::X509::DEFAULT_CERT_FILE", sample_cert)
+  end
+
   describe 'testing environment setup' do
     describe 'generate_root' do
       subject { @root_ca }
@@ -103,6 +111,43 @@ RSpec.describe Gitlab::X509::Certificate do
     end
   end
 
+  describe '.ca_certs_paths' do
+    it 'returns all files specified by OpenSSL defaults' do
+      cert_paths = Dir["#{OpenSSL::X509::DEFAULT_CERT_DIR}/*"]
+
+      expect(described_class.ca_certs_paths).to match_array(cert_paths + [sample_cert])
+    end
+  end
+
+  describe '.ca_certs_bundle' do
+    it 'skips certificates if OpenSSLError is raised and report it' do
+      expect(Gitlab::ErrorTracking)
+        .to receive(:track_and_raise_for_dev_exception)
+        .with(
+          a_kind_of(OpenSSL::X509::CertificateError),
+          cert_file: a_kind_of(String)).at_least(:once)
+
+      expect(OpenSSL::X509::Certificate)
+        .to receive(:new)
+        .and_raise(OpenSSL::X509::CertificateError).at_least(:once)
+
+      expect(described_class.ca_certs_bundle).to be_a(String)
+    end
+
+    it 'returns a list certificates as strings' do
+      expect(described_class.ca_certs_bundle).to be_a(String)
+    end
+  end
+
+  describe '.load_ca_certs_bundle' do
+    it 'loads a PEM-encoded certificate bundle into an OpenSSL::X509::Certificate array' do
+      ca_certs_string = described_class.ca_certs_bundle
+      ca_certs = described_class.load_ca_certs_bundle(ca_certs_string)
+
+      expect(ca_certs).to all(be_an(OpenSSL::X509::Certificate))
+    end
+  end
+
   def common_cert_tests(parsed_cert, cert, signer_ca, with_ca_certs: nil)
     expect(parsed_cert.cert).to be_a(OpenSSL::X509::Certificate)
     expect(parsed_cert.cert.subject).to eq(cert[:cert].subject)