Parse crlDistributionPoints with each_line instead in addition to split URI

Closes gitlab-org/gitlab#207612

app/helpers/x509_helper.rb

+# 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

app/views/projects/commit/x509/_certificate_details.html.haml

 .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(":", " ")

changelogs/unreleased/fix-x509-signed-commit.yml

+---
+title: Fix crl_url parsing and certificate visualization
+merge_request: 25876
+author: Roger Meier
+type: fixed

lib/gitlab/x509/commit.rb

@@ -105,15 +105,24 @@ module Gitlab
 
       def certificate_crl
         extension = get_certificate_extension('crlDistributionPoints')
-        extension.split('URI:').each do |item|
+        crl_url = nil
+
+        extension.each_line do |line|
+          break if crl_url
+
+          line.split('URI:').each do |item|
             item.strip
 
             if item.start_with?("http")
-            return item.strip
+              crl_url = item.strip
+              break
             end
           end
         end
 
+        crl_url
+      end
+
       def get_certificate_extension(extension)
         cert.extensions.each do |ext|
           if ext.oid == extension

spec/helpers/x509_helper_spec.rb

+# 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

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