Add latest changes from gitlab-org/gitlab@12-8-stable-ee

11 files changed, 204 insertions, 52 deletions

diff --git a/app/helpers/x509_helper.rb b/app/helpers/x509_helper.rb
new file mode 100644
index 00000000000..c330b599d74
--- /dev/null
+++ b/app/helpers/x509_helper.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+require 'net/ldap/dn'
+
+module X509Helper
+  def x509_subject(subject, search_keys)
+    subjects = {}
+
+    Net::LDAP::DN.new(subject).each_pair do |key, value|
+      if key.upcase.start_with?(*search_keys.map(&:upcase))
+        subjects[key.upcase] = value
+      end
+    end
+
+    subjects
+  rescue
+    {}
+  end
+end
diff --git a/app/views/projects/commit/x509/_certificate_details.html.haml b/app/views/projects/commit/x509/_certificate_details.html.haml
index 2357c6d803b..51667010d6f 100644
--- a/app/views/projects/commit/x509/_certificate_details.html.haml
+++ b/app/views/projects/commit/x509/_certificate_details.html.haml
@@ -1,17 +1,15 @@
 .gpg-popover-certificate-details
   %strong= _('Certificate Subject')
   %ul
-    - signature.x509_certificate.subject.split(",").each do |i|
-      - if i.start_with?("CN", "O")
-        %li= i
+    - x509_subject(signature.x509_certificate.subject, ["CN", "O"]).map do |key, value|
+      %li= key + "=" + value
     %li= _('Subject Key Identifier:')
     %li.unstyled= signature.x509_certificate.subject_key_identifier.gsub(":", " ")
 
 .gpg-popover-certificate-details
   %strong= _('Certificate Issuer')
   %ul
-    - signature.x509_certificate.x509_issuer.subject.split(",").each do |i|
-      - if i.start_with?("CN", "OU", "O")
-        %li= i
+    - x509_subject(signature.x509_certificate.x509_issuer.subject, ["CN", "OU", "O"]).map do |key, value|
+      %li= key + "=" + value
     %li= _('Subject Key Identifier:')
     %li.unstyled= signature.x509_certificate.x509_issuer.subject_key_identifier.gsub(":", " ")
diff --git a/changelogs/unreleased/fix-x509-signed-commit.yml b/changelogs/unreleased/fix-x509-signed-commit.yml
new file mode 100644
index 00000000000..d3d0f70ce15
--- /dev/null
+++ b/changelogs/unreleased/fix-x509-signed-commit.yml
@@ -0,0 +1,5 @@
+---
+title: Fix crl_url parsing and certificate visualization
+merge_request: 25876
+author: Roger Meier
+type: fixed
diff --git a/lib/gitlab/middleware/multipart.rb b/lib/gitlab/middleware/multipart.rb
index 0ee9563c227..cb4213d23a4 100644
--- a/lib/gitlab/middleware/multipart.rb
+++ b/lib/gitlab/middleware/multipart.rb
@@ -84,12 +84,6 @@ module Gitlab
         end
 
         def open_file(params, key)
-          allowed_paths = [
-            ::FileUploader.root,
-            Gitlab.config.uploads.storage_path,
-            File.join(Rails.root, 'public/uploads/tmp')
-          ]
-
           ::UploadedFile.from_params(params, key, allowed_paths)
         end
 
@@ -106,6 +100,16 @@ module Gitlab
           # inside other env keys, here we ensure everything is updated correctly
           ActionDispatch::Request.new(@request.env).update_param(key, value)
         end
+
+        private
+
+        def allowed_paths
+          [
+            ::FileUploader.root,
+            Gitlab.config.uploads.storage_path,
+            File.join(Rails.root, 'public/uploads/tmp')
+          ]
+        end
       end
 
       def initialize(app)
@@ -125,3 +129,5 @@ module Gitlab
     end
   end
 end
+
+::Gitlab::Middleware::Multipart::Handler.prepend_if_ee('EE::Gitlab::Middleware::Multipart::Handler')
diff --git a/lib/gitlab/x509/commit.rb b/lib/gitlab/x509/commit.rb
index ce298b80a4c..b1d15047981 100644
--- a/lib/gitlab/x509/commit.rb
+++ b/lib/gitlab/x509/commit.rb
@@ -105,13 +105,22 @@ module Gitlab
 
       def certificate_crl
         extension = get_certificate_extension('crlDistributionPoints')
-        extension.split('URI:').each do |item|
-          item.strip
+        crl_url = nil
 
-          if item.start_with?("http")
-            return item.strip
+        extension.each_line do |line|
+          break if crl_url
+
+          line.split('URI:').each do |item|
+            item.strip
+
+            if item.start_with?("http")
+              crl_url = item.strip
+              break
+            end
           end
         end
+
+        crl_url
       end
 
       def get_certificate_extension(extension)
diff --git a/spec/helpers/x509_helper_spec.rb b/spec/helpers/x509_helper_spec.rb
new file mode 100644
index 00000000000..dcdf57ce035
--- /dev/null
+++ b/spec/helpers/x509_helper_spec.rb
@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe X509Helper do
+  describe '#x509_subject' do
+    let(:search_uppercase) { %w[CN OU O] }
+    let(:search_lowercase) { %w[cn ou o] }
+    let(:certificate_attributes) do
+      {
+        'CN' => 'CA Issuing',
+        'OU' => 'Trust Center',
+        'O' => 'Example'
+      }
+    end
+
+    context 'with uppercase DN' do
+      let(:upper_dn) { 'CN=CA Issuing,OU=Trust Center,O=Example,L=World,C=Galaxy' }
+
+      it 'returns the attributes on any case search' do
+        expect(x509_subject(upper_dn, search_lowercase)).to eq(certificate_attributes)
+        expect(x509_subject(upper_dn, search_uppercase)).to eq(certificate_attributes)
+      end
+    end
+
+    context 'with lowercase DN' do
+      let(:lower_dn) { 'cn=CA Issuing,ou=Trust Center,o=Example,l=World,c=Galaxy' }
+
+      it 'returns the attributes on any case search' do
+        expect(x509_subject(lower_dn, search_lowercase)).to eq(certificate_attributes)
+        expect(x509_subject(lower_dn, search_uppercase)).to eq(certificate_attributes)
+      end
+    end
+
+    context 'with comma within DN' do
+      let(:comma_dn) { 'cn=CA\, Issuing,ou=Trust Center,o=Example,l=World,c=Galaxy' }
+      let(:certificate_attributes) do
+        {
+          'CN' => 'CA, Issuing',
+          'OU' => 'Trust Center',
+          'O' => 'Example'
+        }
+      end
+
+      it 'returns the attributes on any case search' do
+        expect(x509_subject(comma_dn, search_lowercase)).to eq(certificate_attributes)
+        expect(x509_subject(comma_dn, search_uppercase)).to eq(certificate_attributes)
+      end
+    end
+
+    context 'with mal formed DN' do
+      let(:bad_dn) { 'cn=CA, Issuing,ou=Trust Center,o=Example,l=World,c=Galaxy' }
+
+      it 'returns nil on any case search' do
+        expect(x509_subject(bad_dn, search_lowercase)).to eq({})
+        expect(x509_subject(bad_dn, search_uppercase)).to eq({})
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/middleware/multipart_spec.rb b/spec/lib/gitlab/middleware/multipart_spec.rb
index 33797817578..1d3ad3afb56 100644
--- a/spec/lib/gitlab/middleware/multipart_spec.rb
+++ b/spec/lib/gitlab/middleware/multipart_spec.rb
@@ -5,9 +5,7 @@ require 'spec_helper'
 require 'tempfile'
 
 describe Gitlab::Middleware::Multipart do
-  let(:app) { double(:app) }
-  let(:middleware) { described_class.new(app) }
-  let(:original_filename) { 'filename' }
+  include_context 'multipart middleware context'
 
   shared_examples_for 'multipart upload files' do
     it 'opens top-level files' do
@@ -82,22 +80,12 @@ describe Gitlab::Middleware::Multipart do
   end
 
   it 'allows files in uploads/tmp directory' do
-    Dir.mktmpdir do |dir|
-      uploads_dir = File.join(dir, 'public/uploads/tmp')
-      FileUtils.mkdir_p(uploads_dir)
-
-      allow(Rails).to receive(:root).and_return(dir)
-      allow(Dir).to receive(:tmpdir).and_return(File.join(Dir.tmpdir, 'tmpsubdir'))
-
-      Tempfile.open('top-level', uploads_dir) do |tempfile|
-        env = post_env({ 'file' => tempfile.path }, { 'file.name' => original_filename, 'file.path' => tempfile.path }, Gitlab::Workhorse.secret, 'gitlab-workhorse')
-
-        expect(app).to receive(:call) do |env|
-          expect(get_params(env)['file']).to be_a(::UploadedFile)
-        end
-
-        middleware.call(env)
+    with_tmp_dir('public/uploads/tmp') do |dir, env|
+      expect(app).to receive(:call) do |env|
+        expect(get_params(env)['file']).to be_a(::UploadedFile)
       end
+
+      middleware.call(env)
     end
   end
 
@@ -127,22 +115,4 @@ describe Gitlab::Middleware::Multipart do
       middleware.call(env)
     end
   end
-
-  # Rails 5 doesn't combine the GET/POST parameters in
-  # ActionDispatch::HTTP::Parameters if action_dispatch.request.parameters is set:
-  # https://github.com/rails/rails/blob/aea6423f013ca48f7704c70deadf2cd6ac7d70a1/actionpack/lib/action_dispatch/http/parameters.rb#L41
-  def get_params(env)
-    req = ActionDispatch::Request.new(env)
-    req.GET.merge(req.POST)
-  end
-
-  def post_env(rewritten_fields, params, secret, issuer)
-    token = JWT.encode({ 'iss' => issuer, 'rewritten_fields' => rewritten_fields }, secret, 'HS256')
-    Rack::MockRequest.env_for(
-      '/',
-      method: 'post',
-      params: params,
-      described_class::RACK_ENV_KEY => token
-    )
-  end
 end
diff --git a/spec/lib/gitlab/x509/commit_spec.rb b/spec/lib/gitlab/x509/commit_spec.rb
index 9cddf27ddce..c31e9e4b8e6 100644
--- a/spec/lib/gitlab/x509/commit_spec.rb
+++ b/spec/lib/gitlab/x509/commit_spec.rb
@@ -204,5 +204,38 @@ describe Gitlab::X509::Commit do
         expect(described_class.new(commit).signature).to be_nil
       end
     end
+
+    context 'certificate_crl' do
+      let!(:commit) { create :commit, project: project, sha: commit_sha, created_at: Time.utc(2019, 1, 1, 20, 15, 0), committer_email: X509Helpers::User1.emails.first }
+      let(:signed_commit) { described_class.new(commit) }
+
+      describe 'valid crlDistributionPoints' do
+        before do
+          allow(signed_commit).to receive(:get_certificate_extension).and_call_original
+
+          allow(signed_commit).to receive(:get_certificate_extension)
+            .with('crlDistributionPoints')
+            .and_return("\nFull Name:\n  URI:http://ch.siemens.com/pki?ZZZZZZA2.crl\n  URI:ldap://cl.siemens.net/CN=ZZZZZZA2,L=PKI?certificateRevocationList\n  URI:ldap://cl.siemens.com/CN=ZZZZZZA2,o=Trustcenter?certificateRevocationList\n")
+        end
+
+        it 'returns an unverified signature' do
+          expect(signed_commit.signature.x509_certificate.x509_issuer).to have_attributes(user1_issuer_attributes)
+        end
+      end
+
+      describe 'valid crlDistributionPoints providing multiple http URIs' do
+        before do
+          allow(signed_commit).to receive(:get_certificate_extension).and_call_original
+
+          allow(signed_commit).to receive(:get_certificate_extension)
+            .with('crlDistributionPoints')
+            .and_return("\nFull Name:\n  URI:http://cdp1.pca.dfn.de/dfn-ca-global-g2/pub/crl/cacrl.crl\n\nFull Name:\n  URI:http://cdp2.pca.dfn.de/dfn-ca-global-g2/pub/crl/cacrl.crl\n")
+        end
+
+        it 'extracts the first URI' do
+          expect(signed_commit.signature.x509_certificate.x509_issuer.crl_url).to eq("http://cdp1.pca.dfn.de/dfn-ca-global-g2/pub/crl/cacrl.crl")
+        end
+      end
+    end
   end
 end
diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb
index d2a54c3eea7..d6f77e581b9 100644
--- a/spec/models/pages_domain_spec.rb
+++ b/spec/models/pages_domain_spec.rb
@@ -7,6 +7,11 @@ describe PagesDomain do
 
   subject(:pages_domain) { described_class.new }
 
+  # Locking in date due to cert expiration date https://gitlab.com/gitlab-org/gitlab/-/issues/210557#note_304749257
+  around do |example|
+    Timecop.travel(Time.new(2020, 3, 12)) { example.run }
+  end
+
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
   end
diff --git a/spec/support/shared_contexts/lib/gitlab/middleware/multipart_shared_contexts.rb b/spec/support/shared_contexts/lib/gitlab/middleware/multipart_shared_contexts.rb
new file mode 100644
index 00000000000..f1554ea8e9f
--- /dev/null
+++ b/spec/support/shared_contexts/lib/gitlab/middleware/multipart_shared_contexts.rb
@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+RSpec.shared_context 'multipart middleware context' do
+  let(:app) { double(:app) }
+  let(:middleware) { described_class.new(app) }
+  let(:original_filename) { 'filename' }
+
+  # Rails 5 doesn't combine the GET/POST parameters in
+  # ActionDispatch::HTTP::Parameters if action_dispatch.request.parameters is set:
+  # https://github.com/rails/rails/blob/aea6423f013ca48f7704c70deadf2cd6ac7d70a1/actionpack/lib/action_dispatch/http/parameters.rb#L41
+  def get_params(env)
+    req = ActionDispatch::Request.new(env)
+    req.GET.merge(req.POST)
+  end
+
+  def post_env(rewritten_fields, params, secret, issuer)
+    token = JWT.encode({ 'iss' => issuer, 'rewritten_fields' => rewritten_fields }, secret, 'HS256')
+    Rack::MockRequest.env_for(
+      '/',
+      method: 'post',
+      params: params,
+      described_class::RACK_ENV_KEY => token
+    )
+  end
+
+  def with_tmp_dir(uploads_sub_dir, storage_path = '')
+    Dir.mktmpdir do |dir|
+      upload_dir = File.join(dir, storage_path, uploads_sub_dir)
+      FileUtils.mkdir_p(upload_dir)
+
+      allow(Rails).to receive(:root).and_return(dir)
+      allow(Dir).to receive(:tmpdir).and_return(File.join(Dir.tmpdir, 'tmpsubdir'))
+      allow(GitlabUploader).to receive(:root).and_return(File.join(dir, storage_path))
+
+      Tempfile.open('top-level', upload_dir) do |tempfile|
+        env = post_env({ 'file' => tempfile.path }, { 'file.name' => original_filename, 'file.path' => tempfile.path }, Gitlab::Workhorse.secret, 'gitlab-workhorse')
+
+        yield dir, env
+      end
+    end
+  end
+end
diff --git a/spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb b/spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb
index 736acc40371..ae66122b4de 100644
--- a/spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb
+++ b/spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb
@@ -7,6 +7,11 @@ describe PagesDomainSslRenewalCronWorker do
 
   subject(:worker) { described_class.new }
 
+  # Locking in date due to cert expiration date https://gitlab.com/gitlab-org/gitlab/-/issues/210557#note_304749257
+  around do |example|
+    Timecop.travel(Time.new(2020, 3, 12)) { example.run }
+  end
+
   before do
     stub_lets_encrypt_settings
   end